💄 Refactor the coveragepy config by webknjaz · Pull Request #3158 · python-trio/trio

webknjaz · 2024-12-19T01:53:50Z

This is a combination of changes that include:

setting up local coveragepy HTML output to show context
setting up mapping between site-packages and the repo
measuring all the Python files in the repo
including Cython
making xfails not influence the coverage metric (these test functions may start executing different amounts of lines over time, causing random coverage drops/gains)
using relative paths for playing nice with Codecov

codecov · 2024-12-19T02:22:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (4f66fab) to head (6212cdf).
Report is 280 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3158      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files         122      124       +2     
  Lines       18407    18381      -26     
  Branches     1226     1226              
==========================================
- Hits        18338    18312      -26     
  Misses         47       47              
  Partials       22       22

Files with missing lines	Coverage Δ
tests/cython/run_test_cython.py	`100.00% <100.00%> (ø)`
tests/cython/test_cython.pyx	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

CoolCat467

Looks good as far as I can tell, just a question about one of the config changes

CoolCat467 · 2024-12-19T03:10:44Z

pyproject.toml

+  "*/src",
+  '*\src',


Why does it have both forward and backwards slash?

also one below this also has a backwards slash

Probably not, but I saw this practice a lot in the wild. I think modern coveragepy normalizes them but I don't want to take a chance.

Probably not what?

Oh, my brain parsed it as “is this necessary?” 😆 Apologies.

These are basically explicit Windows path variants.

Ah, that makes a lot of sense.

Having recently skimmed a bunch of codecov docs I'm pretty sure there's enough path normalization mentioned in a bunch of places that this shouldn't be needed.

@jakkdl this is not Codecov but coveragepy.

webknjaz · 2024-12-19T03:15:06Z

Yeah, I only had to adapt my typical standalone config (which is easier to maintain) to the toml combo. It's all battle-tested, otherwise.

graingert · 2024-12-19T06:43:35Z

pyproject.toml

 parallel = true
+plugins = []
+relative_files = true
+source = ["."]


Why do we need source and source_pkgs? We should only need one

This is needed to measure coverage outside src/. One setting can only contain importable names while the other can have both importables and paths.

I had to do a lot of experiments to arrive to this config shape. Other variants may output incorrect paths into the XML report, confusing Codecov.

(this also allows including that Cython smoke test)

Probably that smoke test should be inside src/trio/_test

Alternatively we could use source_pkgs = ["trio", "tests"]

@graingert this won't work well. The combination of the two is the only config that is known not to cause problems one way or the other. Plus, it represents the shape of the project exactly — the source is the entire Git repo; the importable is whatever is in site-packages but it's also mapped back to the source for proper representation within both coveragepy and codecov.

I've seen cases where it would display test_*.py files in the root of the repository in Codecov while showing the correct paths in the coveragepy's own HTML output. And that's just one example of how things tend to break.

Can we please rely on my experience here? I happened to maintain the use of coveragepy specifically a lot in the past few years, and would like to avoid weird behaviors just because this project wants to deviate from what's gone through countless hours of debugging rather surprising corner cases.

I added the source_pkgs feature to coveragepy for this use case

@graingert really? I didn't know!

Still, there were cases where this specific configuration layout was the only thing that made it possible for Codecov not to break in some cases. This was in both src- and flat-layout projects (some in aio-libs, some elsewhere).

Though, I feel like it's still important to cover other things that might appear outside src/. Sometimes, these are helper scripts or something similar — by collecting such coverage, it is possible to configure Codecov-reported statuses better and be more conscious about what to take into account through exclusions.

As for adding "tests" into source_pkgs, that might work, but would not address the “covering everything” part. Plus, I'm worried about the side effects it may have on Codecov.

graingert · 2024-12-19T06:50:03Z

tests/cython/run_test_cython.py

@@ -0,0 +1 @@
+from . import test_cython  # noqa: F401


Possibly it makes sense to refactor test_cython to export a main function and we can then call it here

Yeah, I thought so. But it was late and I was lazy :)

Will do!

jakkdl · 2024-12-19T10:33:01Z

.github/workflows/ci.yml

+          echo "version=$(python -V | cut -d' ' -f2 | cut -d'.' -f1,2)"
+          >> "${GITHUB_OUTPUT}"
+      - if: always()
+        uses: codecov/codecov-action@v5


is there any problem with mixing versions? I previously hit errors I thought was caused by that.

No, they just use different uploader and APIs. But newer versions are less prone to flaky upload crashes for some reason, as I noticed.

jakkdl · 2024-12-19T10:34:09Z

pyproject.toml

+  "*/src",
+  '*\src',


Having recently skimmed a bunch of codecov docs I'm pretty sure there's enough path normalization mentioned in a bunch of places that this shouldn't be needed.

graingert

Let's use source, or source_pkgs not both

graingert · 2024-12-19T23:38:00Z

pyproject.toml

 parallel = true
+plugins = []
+relative_files = true
+source = ["."]


Alternatively we could use source_pkgs = ["trio", "tests"]

webknjaz · 2024-12-19T23:53:28Z

Merge queue didn't work for the first time because of the flakiness of the Codecov v3 action, FTR.

I've added it back. As for the test relocation and other suggestions, they are out of the scope because I don't want to fit in more requirements into this effort. Those changes can always be tried out separately and shouldn't be blocking — and it would be easier to see if they work in isolation, since this already combines a bit more changes than I'd like in one PR.

webknjaz added 3 commits December 19, 2024 02:38

💄 Make coveragepy HTML output display contexts

13359c0

🧪 Record relative paths in the coveragepy DB

92e39d1

🧪 Hide irrelevant coverage from term output

e70d040

webknjaz added project meta skip newsfragment Newsfragment is not required labels Dec 19, 2024

webknjaz self-assigned this Dec 19, 2024

webknjaz mentioned this pull request Dec 19, 2024

Codecov stuff #3153

Closed

webknjaz force-pushed the maintenance/coveragepy-refactor branch from 6acc88f to f883fa0 Compare December 19, 2024 01:56

webknjaz added 3 commits December 19, 2024 02:59

🧪 Exclude xfailing tests from coverage

9b23d47

🧪 Collect entire repo coverage

e07911c

🧪 Map site-packages coverage to actual source

aeaa980

webknjaz force-pushed the maintenance/coveragepy-refactor branch 8 times, most recently from ebd6084 to 2ab8c61 Compare December 19, 2024 02:18

webknjaz force-pushed the maintenance/coveragepy-refactor branch 5 times, most recently from ad3067d to 9c872c9 Compare December 19, 2024 02:47

webknjaz requested review from A5rocks, CoolCat467, graingert and jakkdl December 19, 2024 02:47

webknjaz marked this pull request as ready for review December 19, 2024 02:47

CoolCat467 approved these changes Dec 19, 2024

View reviewed changes

graingert reviewed Dec 19, 2024

View reviewed changes

jakkdl reviewed Dec 19, 2024

View reviewed changes

webknjaz force-pushed the maintenance/coveragepy-refactor branch from 9c872c9 to 915b739 Compare December 19, 2024 23:06

webknjaz enabled auto-merge December 19, 2024 23:06

This comment was marked as resolved.

Sign in to view

🧪 Collect coverage of the Cython smoke test

6212cdf

webknjaz force-pushed the maintenance/coveragepy-refactor branch from b9b515c to 6212cdf Compare December 19, 2024 23:08

webknjaz added this pull request to the merge queue Dec 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2024

graingert requested changes Dec 19, 2024

View reviewed changes

webknjaz added this pull request to the merge queue Dec 19, 2024

Merged via the queue into python-trio:main with commit 0f270f9 Dec 20, 2024
36 checks passed

webknjaz deleted the maintenance/coveragepy-refactor branch December 20, 2024 00:00

Uh oh!

Conversation

webknjaz commented Dec 19, 2024

Uh oh!

codecov bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CoolCat467 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

graingert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Dec 19, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 19, 2024 •

edited

Loading

webknjaz commented Dec 19, 2024 •

edited

Loading