🧪 Sync coveragepy setup w/ other projects - #13422
Conversation
webknjaz
added
infra
backport-3.14
|
@aiolibsbot review, this is supposed to be a very small scoped patch recovering the coverage metrics of all the lines |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13422 +/- ##
==========================================
- Coverage 98.41% 98.31% -0.10%
==========================================
Files 133 131 -2
Lines 49746 49704 -42
Branches 2629 2619 -10
==========================================
- Hits 48958 48867 -91
- Misses 663 711 +48
- Partials 125 126 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Reviewed. Scope stays small. Confirmed One caveat worth knowing: ci-cd.yml passes Second caveat: bare local Rest: nits only. No blockers. |
PR Review — 🧪 Sync coveragepy setup w/ other projectsMerge-ready. The substantive part of the change is correct and well-reasoned; everything I found is a nit. Specific things done well:
🟢 Suggestions
1. `source = ['.']` is inert in CI and traces an in-tree `.venv` locally
|
| '^ +\.\.\.$', | ||
| 'pytest.fail\(' | ||
| 'pytest.fail\(', | ||
| '^\s*@pytest\.mark\.xfail', |
There was a problem hiding this comment.
Don't ignore xfail tests, these catch real regressions in our code.
There was a problem hiding this comment.
Yeah, you implied this in the past, but I still disagree and this is and accepted practice across many projects. The thing with the xfail tests is that these tend to execute an arbitrary number of instructions which is rather pointless for the measurements and don't contribute any meaningful value. Instead, they are quite harmful in coverage reports, in practice.
| ] | ||
| # https://coverage.rtfd.io/en/latest/contexts.html#dynamic-contexts | ||
| # dynamic_context = 'test_function' # conflicts with `pytest-cov` if set here | ||
| parallel = true |
There was a problem hiding this comment.
I think I have my autobahn job breakage suspect here ^
The tests invoke coverage run in subprocess directly in a non-parallel but append mode which isn't exactly well-integrated.
The CI has a few subtle non-highlighted hints in the middle of the output:
----------------------------- Captured stdout call -----------------------------
Full documentation is at https://coverage.readthedocs.io/en/7.15.4
----------------------------- Captured stderr call -----------------------------
Can't append to data files in parallel mode.
Use 'coverage help' for help.(https://github.com/aio-libs/aiohttp/actions/runs/31819519795/job/94849977268?pr=13422#step:6:199)
But I'm 99% sure this is it.
There's an easy way to check this. I'll try to deal with a better integration later. But for now, I could just unset the option.
| parallel = true | |
| # NOTE: tests/autobahn/test_autobahn.py::test_{client,server} | |
| # NOTE: pass `-a|--append` to `coverage run` wich conflicts | |
| # NOTE: with `-p|--parallel-mode`. We cannot override it on | |
| # NOTE: the CLI level, which is why it's unset here. | |
| # Ref: https://discord.com/channels/267624335836053506/1253355750684753950/1537906072474488973 | |
| # parallel = true |
| ] | ||
| # https://coverage.rtfd.io/en/latest/contexts.html#dynamic-contexts | ||
| # dynamic_context = 'test_function' # conflicts with `pytest-cov` if set here | ||
| parallel = true |
There was a problem hiding this comment.
| parallel = true | |
| # parallel = true |
66daf7f to
cbc8a04
Compare
Specifically, configure the top-level project dir as the global source, put the Python package into `source_pkgs` by name, and set up the path mapping across the Git checkout and installed site-packages location.
It's impossible to override it from the `coverage run` CLI invocation in autobahn tests: https://discord.com/channels/267624335836053506/1253355750684753950/1537906072474488973
Sam's convinced this helps catch bugs. We've agreed to table this for now and try to figure out a way to split the test runs on the infra level to get the best of the both worlds later.
cbc8a04 to
d83589d
Compare
b346d9b to
d9068eb
Compare
| pytest tests/test_client_functional.py tests/test_http_parser.py tests/test_http_writer.py tests/test_web_functional.py tests/test_web_response.py tests/test_websocket_parser.py | ||
| --cov-config=.coveragerc-cython.toml --cov=aiohttp/ --cov=tests/ --numprocesses=auto | ||
| --cov-config=.coveragerc-cython.toml --cov | ||
| --cov-report=xml:cython-coverage.xml |
There was a problem hiding this comment.
I actually wanted to try w/o the :cython-coverage.xml part so that it'd just output the standard coverage.xml and it did, but it was making codecov/codecov-action crazy for reasons beyond my imagination.
| known_third_party=jinja2,pytest,multidict,yarl,gunicorn,freezegun | ||
| known_first_party=aiohttp,aiohttp_jinja2,aiopg | ||
|
|
||
| [report] |
There was a problem hiding this comment.
This is sorta related to the cleanup in terms of “one place for the coverage config”. I was trying to see if we could have just one .coveragerc.toml. But that didn't work out. The env var trick that @nedbat hinted over @ discord worked for the parallel mode setting which was a string/bool but not for the plugins (list of strings).
| sys.executable, | ||
| "-m", |
There was a problem hiding this comment.
This started being necessary as soon as I started passing env= (for setting COVERAGE_PARALLEL_MODE=false to disable parallelism in coveragepy) — looks like it stopped being able to find coverage on $PATH.
| ), | ||
| env={ | ||
| "COVERAGE_PARALLEL_MODE": "false", | ||
| **os.environ.copy(), |
There was a problem hiding this comment.
After giving it a thought, I figured, I'd still pass all those env vars from the parent process just in case.
| # * https://discord.com/channels/267624335836053506/1253355750684753950/1537914331629486130 | ||
| parallel = '${COVERAGE_PARALLEL_MODE-true}' | ||
| plugins = [ | ||
| 'Cython.Coverage', |
There was a problem hiding this comment.
I wonder if we could make loading this conditional via something like an in-tree plugin that injects things into the config or something...
psf-chronographer
Bot
added
the
bot:chronographer:provided
Backport to 3.15: #13423 |
Backport to 3.14: #13424 |
3 participants
What do these changes do?
Specifically, this change configures the top-level project dir as the global source, put the Python package into
source_pkgsby name, and set up the path mapping across the Git checkout and installed site-packages location. This is a follow-up to #13388 that revealed a slight misconfiguration in coveragepy's ability to properly collect coverage from the installed project copy.Are there changes in behavior for the user?
Nope.
Is it a substantial burden for the maintainers to support this?
No.
Related issue number
#13388. Sorta.
Checklist
CONTRIBUTORS.txtCHANGES/foldername it
<issue_or_pr_num>.<type>.rst(e.g.588.bugfix.rst)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature: A new behavior, public APIs. That sort of stuff..deprecation: A declaration of future API removals and breakingchanges in behavior.
.breaking: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc: Notable updates to the documentation structure or buildprocess.
.packaging: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.