-
Notifications
You must be signed in to change notification settings - Fork 609
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
========================================
Coverage ? 85.5%
========================================
Files ? 33
Lines ? 1608
Branches ? 181
========================================
Hits ? 1375
Misses ? 186
Partials ? 47
Continue to review full report at Codecov.
|
There are some links which you may use to see how it will look like: |
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.
LGTM.
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.
Just want to call out the pytest change. If we have a consensus there I approve.
@@ -0,0 +1,8 @@ | |||
[pytest] |
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I know @c24t had some reservations about using pytest.
Hello @c24t, can you please clarify what kind of reservations you had?
Thank you!
We were supposed to discuss this after the alpha release.
Hi @toumorokoshi,
I think nothing prevents us from discussing it a bit earlier.
Also, I found a related issue: #28.
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.
My biggest complaints about pytest have to do with fixtures. Fixtures with side effects, autouse fixtures, and the monkeypatch
fixture in particular have the potential to turn any test suite into a horrifying mess. Using fixtures as arguments in tests makes working with tests difficult outside of pytest, and adds a lot of magic. I don't mean to say it's not possible to write nice tests with pytest, just that the library encourages some pretty bad (IMHO) behavior.
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 pytest --cov
better than coverage run
?
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.
Hello @toumorokoshi and @c24t,
On the SIG meeting 2019-09-12
we discussed migration to pytest
and, if I recall correctly, we found that most developers are fine with it or have no opinion. Unfortunately, we didn't write out a corresponding decision in the OT Python SIG Notes.
So my question is, can we resolve this thread now and move forward with pytest
or is it necessary to have an additional discussion?
Thank you!
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 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 comment
The 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).
tox.ini
Outdated
@@ -38,7 +39,10 @@ commands = | |||
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/ | |||
; For test code, we don't want to enforce the full mypy strictness | |||
mypy: mypy --namespace-packages --config-file=mypy-relaxed.ini opentelemetry-api/tests/ | |||
test: python -m unittest discover | |||
test: pytest --cov {envsitepackagesdir}/opentelemetry |
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.
there's a few unit tests for opentelemetry-api as well. I feel like we should report coverage on those.
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.
Hello @toumorokoshi,
Coverage on API is being reported for every *-test-*
due to settings in pytest.ini
.
Three lines those follows this one are for additional reporting.
You can find the full build log for Python 3.7 here:
https://travis-ci.org/open-telemetry/opentelemetry-python/jobs/581936178
Also, please let me know if I'm missing something or there is any misunderstanding on my side.
Thanks!
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.
So the combined coverage report (e.g. considering that the ext-wsgi tests will also increase coverage of the API code) is available under https://codecov.io/github/open-telemetry/opentelemetry-python/commit/de8f21ecf59322ddbd71c145c74326824a16bd30? The API package's trace module seems to be missing in that report.
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.
So the combined coverage report (e.g. considering that the ext-wsgi tests will also increase coverage of the API code) is available under codecov.io/github/open-telemetry/opentelemetry-python/commit/de8f21ecf59322ddbd71c145c74326824a16bd30?
Yes, it is.
The API package's trace module seems to be missing in that report.
I've tried to investigate and fix this, but it seems like this is a bug on the Codecov side.
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.
The reason you need {envsitepackagesdir}
here is because we're installing to site-packages after #122 right? This makes for some pretty ugly paths in the coverage report vs. something like:
(cd opentelemetry-sdk/tests; pytest --cov ../src)
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've described this bug here.
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.
@Jamim can you post the text for people like me that can't log into that slack?
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.
The issue is already resolved in the current version of changes, but here that text is:
Hi there,
I found that some files are not visible on Codecov.io for some reason.
I tried to investigate this but I didn't find a reason why some files are shown and other files are not.PR: #128
Build: https://travis-ci.org/open-telemetry/opentelemetry-python/builds/581936174
Codecov: https://codecov.io/gh/open-telemetry/opentelemetry-python/tree/de8f21ecf59322ddbd71c145c74326824a16bd30On https://codecov.io/gh/open-telemetry/opentelemetry-python/tree/de8f21ecf59322ddbd71c145c74326824a16bd30/opentelemetry-api/src/opentelemetry page I'm expecting to see
context/
distributedcontext/
logs/
metrics/
resources/
trace/
loader.py
types.py
version.py
but I can see only
context/
distributedcontext/
loader.py
types.py
And corresponding reports contain stats for missing files, e.g.
https://codecov.io/codecov/v4/raw/2019-09-07/E6CBE819EACB252EDCB65A9B1EE32DAB/de8f21ecf59322ddbd71c145c74326824a16bd30/675278e2-de86-49ca-b141-f260fb05b38c.txt contains stats for API'smetrics/__init__.py
.
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.
Nice work so far, I think I spotted some dead code through the report too 😃 👍
tox.ini
Outdated
@@ -15,6 +15,7 @@ python = | |||
[testenv] | |||
deps = | |||
mypy: mypy~=0.711 | |||
test: pytest-cov |
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.
Please make the coverage a different environment: I want to run unit tests without coverage (assuming that coverage significantly slows down the unit tests, otherwise I don't really care). Maybe https://tox.readthedocs.io/en/latest/config.html#complex-factor-conditions can help, or maybe we can call the environment "TESTENVNAME-cov" and set the --cov
option in a setenv to minimize code duplication in tox.ini
.
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.
The setup is quite complex already.
I think we shouldn't increase the complexity until we really see some performance issues.
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.
Hello @Oberon00,
Eventually, I've switched to the approach that you suggested in order to use the editable mode.
Could you please re-review this PR?
Thank you!
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.
Why is editable mode required? Is that to more quickly iterate on unit tests?
I think it would be great to also benchmark the performance of the unit tests with coverage. In my experience it's not a huge overhead.
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.
Why is editable mode required?
Answered in #128 (comment).
Is that to more quickly iterate on unit tests?
I didn't check, but it's not a reason anyway.
tox.ini
Outdated
@@ -38,7 +39,10 @@ commands = | |||
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/ | |||
; For test code, we don't want to enforce the full mypy strictness | |||
mypy: mypy --namespace-packages --config-file=mypy-relaxed.ini opentelemetry-api/tests/ | |||
test: python -m unittest discover | |||
test: pytest --cov {envsitepackagesdir}/opentelemetry |
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.
So the combined coverage report (e.g. considering that the ext-wsgi tests will also increase coverage of the API code) is available under https://codecov.io/github/open-telemetry/opentelemetry-python/commit/de8f21ecf59322ddbd71c145c74326824a16bd30? The API package's trace module seems to be missing in that report.
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.
The codecov.io integration looks great. Did you have to configure anything outside this repo to get it working? If there are project-specific settings there, who has access to them?
tox.ini
Outdated
@@ -38,7 +39,10 @@ commands = | |||
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/ | |||
; For test code, we don't want to enforce the full mypy strictness | |||
mypy: mypy --namespace-packages --config-file=mypy-relaxed.ini opentelemetry-api/tests/ | |||
test: python -m unittest discover | |||
test: pytest --cov {envsitepackagesdir}/opentelemetry |
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.
The reason you need {envsitepackagesdir}
here is because we're installing to site-packages after #122 right? This makes for some pretty ugly paths in the coverage report vs. something like:
(cd opentelemetry-sdk/tests; pytest --cov ../src)
pytest.ini
Outdated
@@ -0,0 +1,8 @@ | |||
[pytest] | |||
addopts = | |||
--cov-append |
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.
Why put these here instead of tox.ini
? -rsv
seems sane, but I'd be pretty surprised if I appended to the coverage report instead of regenerating it by running pytest --cov
, e.g. after switching branches.
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.
Fair enough. There is only -rs -v
in addopts
now.
tox.ini
Outdated
@@ -38,7 +39,10 @@ commands = | |||
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/ | |||
; For test code, we don't want to enforce the full mypy strictness | |||
mypy: mypy --namespace-packages --config-file=mypy-relaxed.ini opentelemetry-api/tests/ | |||
test: python -m unittest discover | |||
test: pytest --cov {envsitepackagesdir}/opentelemetry | |||
test-sdk: coverage report --include *opentelemetry/sdk/* |
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.
Why filter the results after running coverage instead of passing the path to pytest? E.g.
pytest --cov-report xml --cov ../src/
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.
It was necessary in purpose to collect coverage not only for a current one package, but for api/sdk as well, but now those extra reports are gone.
@@ -0,0 +1,8 @@ | |||
[pytest] |
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.
My biggest complaints about pytest have to do with fixtures. Fixtures with side effects, autouse fixtures, and the monkeypatch
fixture in particular have the potential to turn any test suite into a horrifying mess. Using fixtures as arguments in tests makes working with tests difficult outside of pytest, and adds a lot of magic. I don't mean to say it's not possible to write nice tests with pytest, just that the library encourages some pretty bad (IMHO) behavior.
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 pytest --cov
better than coverage run
?
.codecov.yml
Outdated
@@ -0,0 +1,5 @@ | |||
fixes: | |||
- "^.*/site-packages/opentelemetry/sdk/::opentelemetry-sdk/src/opentelemetry/sdk/" |
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.
We may want to stop testing against site-packages to avoid this ugly hack and others like it.
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.
It's done.
7ffb269
to
b7dea94
Compare
Hello everyone! First of all, I apologize for the huge delay. I've implemented a new approach which is based on thoughts those we discussed at the SIG meeting. Thank you! |
4999fa4
to
294ea63
Compare
@Jamim, @hectorhdzg: So now we have both this PR and #208 active for coverage? |
Yes, and they are quite different in terms of the implementation. I personally vote for the current one 😅 |
Hello @hectorhdzg, I understand that you had good intentions, so I have to say Thank you for pinging me and submitting that PR! |
@@ -0,0 +1,2 @@ | |||
[pytest] | |||
addopts = -rs -v |
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.
duplicate addopts?
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.
Do you mean that there is the same line inpytest-cov.ini
?
That file is a separate config file used in the coverage
testenv only.
It's required in order to add python_files = *.py
setting which, I assume, should not be used in the test
testenv.
@@ -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} | |||
pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger} | |||
py3{4,5,6,7,8}-coverage |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
will this double execute all tests?
Yes, it will.
If it is, I think we should eliminate duplicate tests before merging.
I don't think so currently.
There are reasons why:
- As far as I know, we don't want to move back to using
editable mode
for the main test flow
Please see Fix setup for ext packages. #122 and #208/files#r332781829. - Non-editable mode forces us to provide fixes for Codecov.
- There was (and probably is) a missing files issue on the Codecov side when using
fixes
, so it looks reasonable to useeditable mode
to avoid the issue.
You can find some details in this thread. - Pretty ugly paths in the coverage report for
non-editable mode
. - It was suggested to make the coverage a different environment.
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll submit a corresponding issue once this PR is merged.
tox.ini
Outdated
@@ -15,6 +15,7 @@ python = | |||
[testenv] | |||
deps = | |||
mypy: mypy~=0.711 | |||
test: pytest-cov |
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.
Why is editable mode required? Is that to more quickly iterate on unit tests?
I think it would be great to also benchmark the performance of the unit tests with coverage. In my experience it's not a huge overhead.
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 most of my feedback is not a blocker, but I feel strongly about not double-executing tests in the CI (and hopefully not by default when I'm running tox locally).
@@ -12,32 +12,34 @@ | |||
# See the License for the specific language governing permissions and |
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:
==================================== ERRORS ====================================
_ ERROR collecting opentelemetry-api/src/opentelemetry/context/async_context.py _
ImportError while importing test module '/home/travis/build/Jamim/opentelemetry-python/opentelemetry-api/src/opentelemetry/context/async_context.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
opentelemetry-api/src/opentelemetry/context/async_context.py:16: in <module>
from contextvars import ContextVar
E ModuleNotFoundError: No module named 'contextvars'
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.
But doesn't this break the logic of the importing
__init__.py
?
No, it doesn't.
Python 3.7
$ python
Python 3.7.3 (default, Apr 3 2019, 19:16:38)
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from opentelemetry.context import Context
>>> Context
AsyncRuntimeContext({})
Python 3.6
$ python
Python 3.6.8 (default, Oct 7 2019, 12:59:55)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from opentelemetry.context import Context
>>> Context
ThreadLocalRuntimeContext({})
You are not supposed to import that file directly anyway.
Having non-importable files in the project is not good in general.
294ea63
to
ccb3a32
Compare
coverage.sh
Outdated
@@ -0,0 +1,29 @@ | |||
#!/bin/bash |
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.
can this be moved to a scripts/ directory? I think we should avoid littering the root directory with a bunch of utility scripts.
I'm also putting stuff into a scripts directory: https://github.com/open-telemetry/opentelemetry-python/pull/228/files
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.
It's done. Thank you!
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 have a couple changes I'd love to see (specifically putting scripts into a subdirectory), but there's a lot of good improvements here and we can fix issues incrementally rather than block this pr. Thanks!
ccb3a32
to
086237b
Compare
As discussed in the SIG meeting: This PR is a good improvement even now, imperfect details can be fixed later.
@c24t should we merge it now or wait after alpha 0.2 release? |
I believe that we should be able to see the test coverage. These changes: - make tests use pytest - add displaying of the test coverage - add sending of collected coverage data to https://codecov.io
086237b
to
c9ddd65
Compare
@@ -124,7 +124,7 @@ class emitZipkinBatch_args(object): | |||
|
|||
thrift_spec = ( | |||
None, # 0 | |||
(1, TType.LIST, 'spans', (TType.STRUCT, (zipkincore.ttypes.Span, zipkincore.ttypes.Span.thrift_spec), False), None, ), # 1 | |||
(1, TType.LIST, 'spans', (TType.STRUCT, (zipkincore_ttypes.Span, zipkincore_ttypes.Span.thrift_spec), False), None, ), # 1 |
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.
Why these changes to the generated files?
@Jamim FYI it makes it easier to review if you push new commits instead of amending and force-pushing a single commit. Two reasons for this: comments can get orphaned if they refer to commits that aren't on the feature branch anymore (although that doesn't seem to have happened here), and without the commit history it's hard to tell which changes were made in response to which comments. For example, I don't know when the generated jaeger thrift files were changed, and don't know whether other reviewers chose not to comment on these changes or if they didn't exist at review time. |
AFAICT we needed the change to the generated jaeger files because of test name collisions, which happened because we were trying to import files from all packages on each coverage run. But this seems like the wrong behavior to me: should a line in the API package be reported as "covered" if it's only ever run downstream of e.g. a I changed the test coverage script in cdc64f5 to collect coverage stats separately for each package, but still combine them into a single report. This fixes some false-positive coverage, and means we don't have to change the generated files to work around the test import problem. @Jamim please let me know if you have any comments. @toumorokoshi feel free to merge if this LGTY. |
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.
Hello @c24t, Thank you for working on this PR!
Unfortunately, now we have an issue with some files missing from the report again. Also, we don't need |
The missing files are the ones that have 0% coverage, right? I agree that's a big problem, but the better solution may be to restructure our tests so we don't have this collection problem. If you're ok with solving this in another PR that sounds good to me.
Sure, good call. |
Right. |
It's done. |
Change CONTRIBUTING.md Resolves open-telemetry#117 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Hi everyone,
I believe that we should be able to see the test coverage.
These changes:
fix skipping tests for(fixed in Fix skipping tests for PyPy #133)pypy3.5
Best regards!