-
Notifications
You must be signed in to change notification settings - Fork 179
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
ci(codecov): fix codecov / jest coverage misconfigurations #8190
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #8190 +/- ##
===========================================
- Coverage 87.44% 74.71% -12.74%
===========================================
Files 444 1650 +1206
Lines 22817 44112 +21295
Branches 0 4408 +4408
===========================================
+ Hits 19952 32957 +13005
- Misses 2865 10411 +7546
- Partials 0 744 +744
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -1,14 +1,6 @@ | |||
fixes: | |||
- 'api/src/opentrons/::opentrons/' |
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.
This was present to preserve historical coverage data. Given that our coverage history is fairly messed up at this point, I figured we could remove it for a more straightforward experience.
With this line in place, the codecov UI presents an opentrons
folder instead of an api
folder, which is confusing
ignore: | ||
- 'audio' |
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.
These don't exist anymore
- '**/node_modules' | ||
- 'protocol-library-kludge' | ||
- 'shared-data/deck/definitions' |
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.
These are redundant with the JSON ignore line below
with: | ||
file: coverage.xml |
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.
This was just wrong; jest/Istanbul writes to coverage/lcov.info
by default
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 guess there's not really any way to be sure other than testing this in "production," but this looks sensible to me!
Edit:
- If we merge this, should we nuke our Codecov data and start fresh?
- I think yes, the historical data is so messed up that I can't see any value in keeping it
Agreed.
Overview
Upon some investigation for #8185, it turns out that the port from TravisCI to incremental builds in GitHub Actions never properly configured code coverage reporting for all of our JS projects, which explains our blatantly wrong coverage percentage in codecov.
This PR fixes coverage uploads and sets up per-project Codecov flags to see if that makes a meaningful improvement to coverage comments in PRs.
Closes #4985, closes #8185
Changelog
Review requests
Risk assessment
N/A, CI config