bpo-40474: Updated coverage.yml to better report coverage stats by lrjball · Pull Request #19851 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@lrjball

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

@lrjball

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.

@lrjball

@lrjball

How do I add the 'skip news' label to this?

@lrjball

remilapeyre

source ./.venv/bin/activate
python -m pip install -U coverage
python -m test.pythoninfo
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage

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?

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

remilapeyre

source ./.venv/bin/activate
python -m pip install -U coverage
python -m test.pythoninfo
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage

Choose a reason for hiding this comment

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

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.

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.

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

@lrjball

@lrjball

@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

@bedevere-bot

@brettcannon: Please replace # with GH- in the commit message next time. Thanks!

@brettcannon

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request

May 30, 2020

@CuriousLearner

Labels