Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests #42

Merged
merged 1 commit into from
Jul 12, 2019
Merged

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Jul 8, 2019

Based on #29.

Adds unit tests using the unittest standard library module, executes them in tox and runs pylint and a basic mypy configuration on the test code.

@Oberon00 Oberon00 mentioned this pull request Jul 8, 2019
; For test code, we don't want to use the full mypy strictness, so we use its
; default flags instead.
py37-mypy: mypy opentelemetry-api/tests/ --config-file=
test: python -m unittest discover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: python -m unittest is equivalent to python -m unittest discover

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I saw that in the unittest docs, but went with "explicit is better than implicit"

py37-lint: pylint opentelemetry-api/src/opentelemetry/
; Prefer putting everything in one pylint command to profit from duplication
; warnings.
py37-lint: pylint opentelemetry-api/src/opentelemetry/ opentelemetry-api/tests/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have the unit tests run before the lint/mypy. When developing, I usually fix the lint issues at the end once all the code has been completed and finalized. If the build were to fail on the tests with perfect lint, after the tests are fixed, the lint might be broken due to the changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better in general to fail fast. If our main use case here is CI I'd like to see errors as soon as possible.

In any case the ordering doesn't matter if we run the tests in parallel (with -p auto), and you can always run the unit tests alone with -e py37-test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, the order would be defined with the envlist at the top.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think unit tests should run before the lint/mypy step ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too, personally I think the ordering in OpenCensus makes sense.

  1. unit test
  2. integration test
  3. code coverage
  4. lint
  5. document, packaging

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sometimes got better error messages from pylint than from my failed unit test about e.g. some non-existent variable. On the other hand I agree that spending time on making the line-breaks nice before the unit test runs through is futile. So personally I think my ordering would be:

  1. mypy (what this finds are not style issues but mostly real bugs, in type annotations or code)
  2. maybe pylint
  3. tests (would it make sense to just run the tests including coverage here, or is that too slow?)
  4. pylint if not run in step 2
  5. flake8 (this one does not find any bugs, only style issues)
  6. coverage (when making changes due to previous steps code coverage is likely to change)

In any case, like @c24t said, the ordering does not matter that much, so I'm really fine with anything.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great if we could run multiple python versions, and avoid changing PYTHONPATH by hand, but otherwise it LGTM.

@@ -1,6 +1,6 @@
[tox]
skipsdist = True
envlist = py37-{lint,mypy}, docs
envlist = py37-{lint,mypy,test}, docs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about running the tests in all supported python versions (#25 notwithstanding)?

Suggested change
envlist = py37-{lint,mypy,test}, docs
envlist = py{34,35,36,37}-test, py37-{lint,mypy}, docs

Also note that this turns up a type annotation in the loader as unsupported in 3.4.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely something we should do but when I first turned that on I run into "mypy does not support Python 3.4 and decided to do that later and reopen #25 first. I'll take another stab at fixing the 3.4 issues.

@@ -11,10 +11,23 @@ deps =
lint: pylint~=2.3.1
mypy: mypy~=0.711

setenv =
PYTHONPATH={toxinidir}/opentelemetry-api/src/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're doing this to get opentelemetry on the path for tests: would installing opentelemetry-api in the testenv work instead? usedevelop may work for this.

Copy link
Member Author

@Oberon00 Oberon00 Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would. Why do we use skipsdist anyway?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC because we've got multiple packages under separate directories so tox didn't know what to install.

# class after reloading `trace`.
reload(sys.modules[__name__])


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like pylint doesn't care about blank lines... do you think it'd be overkill to run flake8 with lint too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your consideration: #46. 😄

@Oberon00
Copy link
Member Author

I'm merging this as-is for now, as other PRs are already based on it. Since I'm on holiday for a longer time now anyway, it's yours to adapt (I'll just add my opinion to the lint/test ordering discussion).

@Oberon00 Oberon00 merged commit eb10c53 into open-telemetry:master Jul 12, 2019
@Oberon00 Oberon00 deleted the unit-tests branch July 12, 2019 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants