-
Notifications
You must be signed in to change notification settings - Fork 624
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
Add test coverage collecting #128
Changes from all commits
c9ddd65
c012914
cdc64f5
3da192d
89c4f01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
[run] | ||
omit = | ||
*/tests/* | ||
*/setup.py | ||
*/gen/* |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ venv*/ | |
pip-log.txt | ||
|
||
# Unit test / coverage reports | ||
coverage.xml | ||
.coverage | ||
.nox | ||
.tox | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,3 +19,7 @@ install: | |
|
||
script: | ||
- tox | ||
|
||
after_success: | ||
- pip install codecov | ||
- codecov -v |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[pytest] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know @c24t had some reservations about using pytest. We were supposed to discuss this after the alpha release. FWIW I really like pytest and wholeheartedly support using pytest and pytest functionality. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hello @c24t, can you please clarify what kind of reservations you had?
Hi @toumorokoshi, Also, I found a related issue: #28. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My biggest complaints about pytest have to do with fixtures. Fixtures with side effects, autouse fixtures, and the When we considered adding it before it wasn't clear that there were any features of pytest that we couldn't get just as easily from the built in unittest library. It may be that the extra complexity is justified if pytest makes writing and running tests easier. I still don't see a strong reason for using pytest in this PR, but I may be missing something here. What makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @toumorokoshi and @c24t, On the Thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've pinged the appropriate opinionated people regarding the use of pytest, so I'm going to remove my "changes required". I'll let others who are against pytest argue their point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @toumorokoshi To actually dismiss your review you have to expand the "Changes requested" box at the bottom of the PR, and then click "..." -> "Dismiss" next to your review there. Re-requesting your review won't actually dismiss it (I ran into that too once, it's not the most intuitive UI). |
||
addopts = -rs -v | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicate addopts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean that there is the same line in |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#!/bin/bash | ||
|
||
set -e | ||
|
||
function cov { | ||
pytest \ | ||
--ignore-glob=*/setup.py \ | ||
--cov ${1} \ | ||
--cov-append \ | ||
--cov-branch \ | ||
--cov-report='' \ | ||
${1} | ||
} | ||
|
||
|
||
coverage erase | ||
|
||
cov opentelemetry-api | ||
cov opentelemetry-sdk | ||
cov ext/opentelemetry-ext-http-requests | ||
cov ext/opentelemetry-ext-jaeger | ||
cov ext/opentelemetry-ext-opentracing-shim | ||
cov ext/opentelemetry-ext-wsgi | ||
cov examples/opentelemetry-example-app | ||
|
||
coverage report | ||
coverage xml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,11 @@ skip_missing_interpreters = True | |
envlist = | ||
py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim} | ||
pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim} | ||
py3{4,5,6,7,8}-coverage | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this double execute all tests? Since under the hood it's calling pytest --cov, which IIUC will run all the test again. If it is, I think we should eliminate duplicate tests before merging. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it will.
I don't think so currently. There are reasons why:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry that this will double the build times in CI and locally for those running tox. I don't want to block this PR if this is the direction people want to go, but I think we should go back and find a way to not double-execute tests. Can you file a ticket once this is merged so we remember we should go back and hopefully resolve this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll submit a corresponding issue once this PR is merged. |
||
|
||
; Coverage is temporarily disabled for pypy3 due to the pytest bug. | ||
; pypy3-coverage | ||
|
||
lint | ||
py37-tracecontext | ||
py37-{mypy,mypyinstalled} | ||
|
@@ -15,6 +20,8 @@ python = | |
|
||
[testenv] | ||
deps = | ||
test: pytest | ||
coverage: pytest-cov | ||
mypy,mypyinstalled: mypy~=0.740 | ||
|
||
setenv = | ||
|
@@ -45,12 +52,24 @@ commands_pre = | |
jaeger: pip install {toxinidir}/ext/opentelemetry-ext-jaeger | ||
opentracing-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/ext/opentelemetry-ext-opentracing-shim | ||
|
||
; In order to get a healthy coverage report, | ||
; we have to install packages in editable mode. | ||
coverage: pip install -e {toxinidir}/opentelemetry-api | ||
coverage: pip install -e {toxinidir}/opentelemetry-sdk | ||
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-azure-monitor | ||
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-http-requests | ||
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-jaeger | ||
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-opentracing-shim | ||
coverage: pip install -e {toxinidir}/ext/opentelemetry-ext-wsgi | ||
coverage: pip install -e {toxinidir}/examples/opentelemetry-example-app | ||
|
||
; Using file:// here because otherwise tox invokes just "pip install | ||
; opentelemetry-api", leading to an error | ||
mypyinstalled: pip install file://{toxinidir}/opentelemetry-api/ | ||
|
||
commands = | ||
test: python -m unittest discover | ||
test: pytest | ||
coverage: {toxinidir}/scripts/coverage.sh | ||
|
||
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/ | ||
; For test code, we don't want to enforce the full mypy strictness | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file was changed by mistake? Could you please remove these changes from the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, it wasn't.
This file was changed in purpose to make it "importable" for Python <3.7.
Please see a corresponding build. That build failed with:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But doesn't this break the logic of the importing
__init__.py
? You are not supposed to import that file directly anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't.
Python 3.7
Python 3.6
Having non-importable files in the project is not good in general.