-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
fix(.github/codecov): Ignore aws-lc-rs-fips for codecov #2023
base: main
Are you sure you want to change the base?
Conversation
It seems from the CI failure that code coverage builds for Windows for More resources:
@mxinden @Ralith @djc I guess a decision has to be made if full coverage for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What goal are you trying to achieve here?
I think we should definitely fix it so coverage builds can succeed (I'm surprised the main CI failures don't generate any notifications for me...). I think rustls has this working so it shouldn't be too hard to fix? https://github.com/rustls/rustls/blob/main/.github/workflows/build.yml#L51 |
6d859e2
to
80a485b
Compare
@djc Should be good to go now. Checks are passing: https://github.com/quinn-rs/quinn/actions/runs/12588265064 I can drop the last commit (meant to be temporary just for testing) if this looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just copying what Rustls is doing makes sense, and the second commit which makes the coverage CI run in PRs combined with the fact that CI passed for this PR seems to indicate that the change works. Thank you for working on this irksome thing!
(We must remember not to merge until the second commit is removed.)
So looking at this more, maybe we should exclude |
Yeah that was my initial thinking. Let me know what you guys decide and I can adjust this PR. |
+1 in favor of excluding it, since our CI is getting pretty big already. |
ff405ef
to
c109d60
Compare
c109d60
to
19273f5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2023 +/- ##
==========================================
+ Coverage 77.43% 77.89% +0.46%
==========================================
Files 69 71 +2
Lines 18167 18722 +555
==========================================
+ Hits 14067 14584 +517
- Misses 4100 4138 +38 ☔ View full report in Codecov by Sentry. |
Updated to include all features except for Note that the last commit needs to be dropped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, although I wish there was a way built in to cargo to do this. But it looks like there might not be.
LLVMCOV_FEATURES=$(cargo metadata --format-version 1 --no-deps \ | ||
| jq -r '[ .packages[].features | keys[] | select(. != "default" and . != "aws-lc-rs-fips" and . != "rustls-aws-lc-rs-fips") ] | unique | join(",")') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just get the flattened version of this, without all the shell/jq magic? That stuff is unfamiliar (at least to me) and thus harder to maintain, and I don't think we change our features all that often.
An attempt to fix https://github.com/quinn-rs/quinn/actions/runs/11562272211/job/32183071070 on
main
and (unreleased)0.5.6