-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2951 +/- ##
==========================================
- Coverage 99.60% 92.57% -7.03%
==========================================
Files 121 121
Lines 17882 17882
Branches 3214 3163 -51
==========================================
- Hits 17811 16554 -1257
- Misses 50 1214 +1164
- Partials 21 114 +93 |
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) |
Is codecov just consistently reporting less coverage? It looks like -3% coverage on all platforms. It looks like codecov is regenerating |
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):
On macos (v4 action):
Alright, after that latest workaround attempt:
|
the alpine v3 codecov action is also saying
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?
I think this is the case, since it's under the So this is about codecov not being able to find the |
Random idea: I realized we don't have a |
that made no difference to anything This reverts commit 2474152.
@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. |
Ok last update was 3 months ago, and while there are not merge conflicts I think that it's reasonable to update it again now that this much time has passed. |
It's been a long time, what exactly was the issue for not using v4 again? Loss compared to previous system or something? |
Yeah, the issue was decreased coverage despite the coverage files and coverage config not actually changing. |
codecov/codecov-action#1279 has been resolved, lets see if we can get this working or there are other issues blocking us |
Current status:
|
I think we can close this, and open a new PR trying to update to v5 |
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"