GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40474: Updated coverage.yml to better report coverage stats by lrjball · Pull Request #19851 · python/cpython

Conversation

@lrjball

@lrjball lrjball commented May 2, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@lrjball
Updated coverage.yml to better report coverage stats
7bc3795
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.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 2, 2020
@bedevere-bot bedevere-bot added the awaiting review label May 2, 2020
@lrjball
Updated travis.yml as well
653165c
@lrjball

lrjball commented May 2, 2020

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ammaraskar ammaraskar added the skip news label May 2, 2020
lrjball added 3 commits May 2, 2020 11:43
@lrjball
Updated travis.yml as path not found
0a674f7
@lrjball
fixed typos
982d978
@lrjball
Removed PYTHONPATH after test run to prevent memoryerror when xml gen…
64d521b
…erated
@lrjball

lrjball commented May 2, 2020

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros
aeros requested a review from zware May 8, 2020 21:08
remilapeyre
remilapeyre reviewed May 13, 2020
View reviewed changes
Comment thread .github/workflows/coverage.yml Outdated
source ./.venv/bin/activate
python -m pip install -U coverage
python -m test.pythoninfo
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage

@remilapeyre remilapeyre May 13, 2020

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@lrjball lrjball May 13, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
remilapeyre
remilapeyre suggested changes May 13, 2020
View reviewed changes
Comment thread .github/workflows/coverage.yml Outdated
source ./.venv/bin/activate
python -m pip install -U coverage
python -m test.pythoninfo
export PYTHONPATH=./.venv/lib/python3.9/site-packages/coverage/fullcoverage

@remilapeyre remilapeyre May 13, 2020

Copy link
Copy Markdown
Contributor

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:

Suggested change
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@lrjball lrjball May 13, 2020

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@remilapeyre remilapeyre May 14, 2020

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@lrjball lrjball May 14, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels May 13, 2020
@lrjball
updated PYTHONPATH to avoid hardcoding python version
78ed5ed
@lrjball

lrjball commented May 27, 2020

Copy link
Copy Markdown
Contributor Author

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon
brettcannon approved these changes May 28, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 28, 2020
@brettcannon
brettcannon merged commit d9c1f19 into python:master May 28, 2020
@bedevere-bot

bedevere-bot commented May 28, 2020

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label May 28, 2020
@brettcannon

brettcannon commented May 28, 2020

Copy link
Copy Markdown
Member

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request May 30, 2020
@CuriousLearner
Merge branch 'master' of github.com:python/cpython into bpo-26543
d3d7100
* '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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon approved these changes
@zware zware Awaiting requested review from zware
+1 more reviewer
@remilapeyre remilapeyre remilapeyre requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@lrjball @bedevere-bot @brettcannon @remilapeyre @ammaraskar @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.