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.

Sign up for GitHub

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

Codecov improvements #3156

Merged
merged 7 commits into from
Dec 18, 2024
Merged

Codecov improvements #3156

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
17af03b
remove erronously configured codecov flags
jakkdl Dec 17, 2024
02a8406
add .codecov.yml
jakkdl Dec 17, 2024
d418dc2
move token to yaml file, set fail_ci_if_error
jakkdl Dec 17, 2024
decfb73
restore flags, make alpine python flag dynamic
jakkdl Dec 17, 2024
2fd7fcf
skip codecov if check-formatting is set
jakkdl Dec 17, 2024
a7a2ff9
Apply suggestions from code review
jakkdl Dec 17, 2024
2c53ff2
Merge branch 'main' into codecov_stuff
jakkdl Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .codecov.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# -- repository yaml --

# Explicitly wait for all jobs to finish, as wait_for_ci prematurely triggers.
# See https://github.com/python-trio/trio/issues/2689
codecov:
notify:
# This number needs to be changed whenever the number of runs in CI is changed.
# Another option is codecov-cli: https://github.com/codecov/codecov-cli#send-notifications
after_n_builds: 78
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
wait_for_ci: false
notify_error: true # if uploads fail, replace cov comment with a comment with errors.
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
require_ci_to_pass: false

# only post PR comment if coverage changes
comment:
require_changes: true

coverage:
# required range
range: 99.6..100
status:
# require patches to be 100%
patch:
default:
target: 100%
4 changes: 0 additions & 4 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ jobs:
directory: empty
token: 87cefb17-c44b-4f2f-8b30-1fff5769ce46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to move this token to the config as it would be easier to keep it in one shared place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

looks like v4 & v5 has new tools for managing tokens & tokenless uploads, so if we wanna do it properly we could look at that later. But the minimal downside of exposing the token might not be worth the effort of doing that https://docs.codecov.com/docs/codecov-tokens#uploading-without-a-token

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v4 introduced OIDC, which would only work in trusted contexts (which PRs from forks aren't). They have corner case handling for such PRs.
With v5, it's possible to toggle tokenless uploads in the Codecov UI. Not sure if this is an org-only setting or it could be per-repo. I haven't tested it.

name: Windows (${{ matrix.python }}, ${{ matrix.arch }}${{ matrix.extra_name }})
flags: Windows,${{ matrix.python }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the flags despite the UI alerts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could keep them, but nobody has pressed the button to enable analytics so currently it's entirely unused anyway https://app.codecov.io/gh/python-trio/trio/flags

I still find it somewhat questionable to track coverage for "python3.11" or "windows" as separate entities. Maybe you could diagnose issues by seeing big swings in coverage for one of them*, or any of them being very low, but the actual coverage % for each flag is mostly useless unless we'd do something like marking all platform/version-specific code somehow, exclude from coverage for that flag, and require 100% coverage on common code on all platforms... (which all sounds like way too much effort for close-to-no payoff).

* could use https://docs.codecov.com/docs/commit-status#threshold

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've used the flags once (when looking at updating the codecov action) but I don't remember how + given we haven't figured the issue out I assume it wasn't useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess 1>0 and there's not really any downside to have them, so I'll add them back in. I'll go enable analytics too for fun.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use them heavily in many projects. It's nice to see which likes are covered under which env or a combo of those. There's a way to filter it on the Codecov UI.

As for the checks and 100% coverage requirement, I usually don't make those depend on the flags, except for MyPy, perhaps. Codecov is able to track the combined coverage for those checks. I can look into improving this later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of a pain that https://app.codecov.io/gh/python-trio/trio/flags shows all flags ever used, with 1/3 being for old versions we no longer support. I'm not sure there's a decent way to fix that without doing manual flag management

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can clean them up. Although, to me, the most useful view with flags is the per-file line coverage pages.


Ubuntu:
name: 'Ubuntu (${{ matrix.python }}${{ matrix.extra_name }})'
Expand Down Expand Up @@ -137,7 +136,6 @@ jobs:
directory: empty
token: 87cefb17-c44b-4f2f-8b30-1fff5769ce46
name: Ubuntu (${{ matrix.python }}${{ matrix.extra_name }})
flags: Ubuntu,${{ matrix.python }}

macOS:
name: 'macOS (${{ matrix.python }})'
Expand Down Expand Up @@ -173,7 +171,6 @@ jobs:
directory: empty
token: 87cefb17-c44b-4f2f-8b30-1fff5769ce46
name: macOS (${{ matrix.python }})
flags: macOS,${{ matrix.python }}

# run CI on a musl linux
Alpine:
Expand All @@ -199,7 +196,6 @@ jobs:
directory: empty
token: 87cefb17-c44b-4f2f-8b30-1fff5769ce46
name: Alpine
flags: Alpine,3.12
jakkdl marked this conversation as resolved.
Show resolved Hide resolved

Cython:
name: "Cython"
Expand Down
Loading