-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
bpo-40474: Updated coverage.yml to better report coverage stats #19851
Conversation
Currently modules which are imported early are misreported in coverage. This has a fix which is documented in the dev guide, but the fix isn't being used in the CI. This PR adds in that fix.
How do I add the 'skip news' label to this? |
this seems to be working now, for example coverage for re is correctly showing as 93% when previously it was showing 60% with the global statements showing as uncovered. See https://codecov.io/gh/python/cpython/tree/64d521b5d34c25b83d0472608d1eab3a6334bf59/Lib |
.github/workflows/coverage.yml
Outdated
@@ -49,6 +49,7 @@ jobs: | |||
source ./.venv/bin/activate | |||
python -m pip install -U coverage | |||
python -m test.pythoninfo | |||
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage |
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.
This looks like it will break periodically, maybe we could find a way to avoid hardcoding the version?
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.
Agreed, although I couldn't find a simple alternative. I tried .../python*/...
which does set the pythonpath to the right thing when echo'd, it doesn't seem to pick it up on the following command. If anyone can suggest a better alternative that would be great
.github/workflows/coverage.yml
Outdated
@@ -49,6 +49,7 @@ jobs: | |||
source ./.venv/bin/activate | |||
python -m pip install -U coverage | |||
python -m test.pythoninfo | |||
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage |
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.
This is how the python3.9
is built looking at https://github.com/python/cpython/blob/master/Lib/venv/__init__.py#L129-L131:
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage | |
export PYTHONPATH=./.venv/lib/$(./python -c "import sys; print('python%d.%d' % sys.version_info[:2])")/site-packages/coverage/fullcoverage |
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.
That is a clever idea! Thinking about it, export PYTHONPATH=./.venv/lib/$(ls .venv/lib | grep python | head -n 1)/site-packages/coverage/fullcoverage
would also work and might be a bit simpler as it doesn't depend on sys
, it just gets the name of that python* directory.
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.
Whatever works and is accepted by a core dev is fine by me, you can also use find .venv -name fullcoverage
which may be more robust.
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.
yeah I like that one the best, I've updated it now
@vstinner @brettcannon, looks like you have both added to the CI pipeline in the past. Do either of you have any suggestions /opinions on this update to the test coverage calculation? Thanks |
@brettcannon: Please replace |
Thanks! |
* 'master' of github.com:python/cpython: (497 commits) bpo-40061: Fix a possible refleak in _asynciomodule.c (pythonGH-19748) bpo-40798: Generate a different message for already removed elements (pythonGH-20483) closes bpo-29017: Update the bindings for Qt information with PySide2 (pythonGH-20149) bpo-39885: Make IDLE context menu cut and copy work again (pythonGH-18951) bpo-29882: Add an efficient popcount method for integers (python#771) Further de-linting of zoneinfo module (python#20499) bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros (pythonGH-20435) Indicate that abs() method accept argument that implement __abs__(), just like call() method in the docs (pythonGH-20509) bpo-39040: Fix parsing of email mime headers with whitespace between encoded-words. (pythongh-17620) bpo-40784: Fix sqlite3 deterministic test (pythonGH-20448) bpo-30064: Properly skip unstable loop.sock_connect() racing test (pythonGH-20494) Note the output ordering of combinatoric functions (pythonGH-19732) bpo-40474: Updated coverage.yml to better report coverage stats (python#19851) bpo-40806: Clarify that itertools.product immediately consumes its inpt (pythonGH-20492) bpo-1294959: Try to clarify the meaning of platlibdir (pythonGH-20332) bpo-37878: PyThreadState_DeleteCurrent() was not removed (pythonGH-20489) bpo-40777: Initialize PyDateTime_IsoCalendarDateType.tp_base at run-time (pythonGH-20493) bpo-40755: Add missing multiset operations to Counter() (pythonGH-20339) bpo-25920: Remove socket.getaddrinfo() lock on macOS (pythonGH-20177) bpo-40275: Fix test.support.threading_helper (pythonGH-20488) ...
Currently modules which are imported early are misreported in coverage. This has a fix which is documented in the dev guide, but the fix isn't being used in the CI. This PR adds in that fix.
https://bugs.python.org/issue40474