Skip to content
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

Update to codecov-action@v4 #2951

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

CoolCat467
Copy link
Contributor

This pull request updates the code coverage action to v4

Reading the release details, it looks like they update node runtime thing, drop support for not telling them the token to use (trio tells them the token so that's fine), and "various argument changes"

Copy link

codecov bot commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (b4c19bc) to head (558c721).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2951      +/-   ##
==========================================
- Coverage   99.63%   92.33%   -7.31%     
==========================================
  Files         117      117              
  Lines       17602    17602              
  Branches     3174     3152      -22     
==========================================
- Hits        17537    16252    -1285     
- Misses         46     1230    +1184     
- Partials       19      120     +101     

see 57 files with indirect coverage changes

mikenerone
mikenerone previously approved these changes Feb 11, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Feb 11, 2024

Surprised at the loss of 3 percentage points of coverage -- is there some platform where things broke?

... interesting: https://github.com/python-trio/trio/actions/runs/7865327122/job/21458126980?pr=2951#step:7:12 (edit: reported to codecov/codecov-action#1279)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@A5rocks
Copy link
Contributor

A5rocks commented Feb 13, 2024

Is codecov just consistently reporting less coverage? It looks like -3% coverage on all platforms.

It looks like codecov is regenerating coverage.xml rather than using the one we made (which has all the configuration we apply).

@A5rocks
Copy link
Contributor

A5rocks commented Feb 13, 2024

I tried a workaround and it didn't work. I'm not entirely sure my hypothesis on why this is taking away coverage is correct. But nonetheless consider me displeased that somehow codecov managed to have 2 changes that affect us in a single major version bump >:[

Here's the logs I find suspicious:

On alpine (v3 action):

[2024-02-13T07:09:43.431Z] ['info'] Searching for coverage files...
[2024-02-13T07:09:43.480Z] ['info'] Warning: Some files located via search were excluded from upload.
[2024-02-13T07:09:43.481Z] ['info'] If Codecov did not locate your files, please review https://docs.codecov.com/docs/supported-report-formats
[2024-02-13T07:09:43.481Z] ['info'] => Found 1 possible coverage files:
  coverage.xml
[2024-02-13T07:09:43.481Z] ['info'] Processing empty/coverage.xml...

On macos (v4 action):

warning - 2024-02-13 07:08:53,180 -- No config file could be found. Ignoring config.
warning - 2024-02-13 07:08:53,199 -- No swift data found.
warning - 2024-02-13 07:08:53,214 -- No gcov data found.
info - 2024-02-13 07:08:53,215 -- Generating coverage.xml report in /Users/runner/work/trio/trio/empty
info - 2024-02-13 07:08:57,046 -- Wrote XML report to coverage.xml
info - 2024-02-13 07:08:57,092 -- Found 1 coverage files to upload
info - 2024-02-13 07:08:57,092 -- > empty/coverage.xml

Alright, after that latest workaround attempt:

  • good news: I got rid of the coverage.xml generation
  • bad news: that wasn't the problem. was the uploader looking at pyproject.toml before? does exclude_lines not effect the coverage.xml file but rather how codecov interprets it? (and someone might want to working-directory -> directory. i'm not sure why we would prefer one over the other)

@jakkdl
Copy link
Member

jakkdl commented Feb 13, 2024

the alpine v3 codecov action is also saying

[2024-02-13T07:23:32.835Z] ['info'] Error fetching git root. Defaulting to /__w/trio/trio/empty. Please try using the -R flag. Error: Error running external program: Error: spawnSync git ENOENT

https://github.com/python-trio/trio/actions/runs/7883042498/job/21509252192?pr=2951#step:7:25

though that's not new (v3 on non-alpine never said it), but maybe also an indication of directory trouble?

does exclude_lines not effect the coverage.xml file but rather how codecov interprets it?

I think this is the case, since it's under the tool.coverage.report heading - whereas tool.coverage.run is for the xml/binary .coverage files. (some local testing suggests the same, modifying the report section after having generated coverage data changes the output of coverage report).

So this is about codecov not being able to find the pyproject.toml file, maybe it's because of us messing around with empty/ and $INSTALLDIR and stuff?

@mikenerone mikenerone dismissed their stale review February 13, 2024 20:15

Knock-on problems turned up

@jakkdl
Copy link
Member

jakkdl commented Feb 20, 2024

Random idea: I realized we don't have a codecov.yaml, which is the codecov-specific configuration file. You don't need one, but maybe it would help resolve our problems https://docs.codecov.com/docs/codecov-yaml

@jakkdl
Copy link
Member

jakkdl commented Apr 9, 2024

@CoolCat467 updating the branch with a merge commit will ping everybody subscribed to a PR (which might be a lot, since joining the trio/ org automatically makes you watch the repository for all activity). AFAIK there's no way to disable merge-commit-notifications in particular, so even though it sometimes is useful it'd be lovely if you refrained from repeatedly bumping PRs unless there's relevant changes or something <3

@CoolCat467
Copy link
Contributor Author

@CoolCat467 updating the branch with a merge commit will ping everybody subscribed to a PR (which might be a lot, since joining the trio/ org automatically makes you watch the repository for all activity). AFAIK there's no way to disable merge-commit-notifications in particular, so even though it sometimes is useful it'd be lovely if you refrained from repeatedly bumping PRs unless there's relevant changes or something <3

Sorry, I just want to make sure things stay up to date, but yes I can do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants