-
Notifications
You must be signed in to change notification settings - Fork 58
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: adds codecov to CI pipeline and coverage badge to README #261
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #261 +/- ##
=========================================
Coverage ? 93.38%
=========================================
Files ? 1
Lines ? 257
Branches ? 66
=========================================
Hits ? 240
Misses ? 16
Partials ? 1 Continue to review full report at Codecov.
|
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.
Solid effort mate, thank you so much! Just one comment about the coverage commands.
package.json
Outdated
@@ -65,6 +67,7 @@ | |||
"brfs": "^2.0.2", | |||
"browserify": "^17.0.0", | |||
"budo": "^11.6.4", | |||
"codecov": "^3.8.1", |
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.
Rather than include this as a dep, we should just install this in the CI. There zero benefit to ever running this locally.
So can you please do a yarn global add codecov
in the gh actions file, and just running that after the tests run. Similarity get rid of line 33 here.
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.
Love it, thanks! I'll take care of that soon. On that note, we should probably do the same over in the focus-trap-react
repo since it also has codecov
listed as a dev dependency: https://github.com/focus-trap/focus-trap-react/blob/master/package.json#L77
I can make the same change over there once we get this MR approved.
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.
After a few attempts, I think I finally have this working correctly. The CI pipeline passes anyway. 😅
These two issues helped me resolve the problem of the global yarn package not being installed properly or not being installed where it could be found:
@@ -331,7 +331,7 @@ | |||
"@babel/helper-plugin-utils" "^7.13.0" | |||
"@babel/plugin-syntax-logical-assignment-operators" "^7.10.4" | |||
|
|||
"@babel/plugin-proposal-nullish-coalescing-operator@^7.13.0", "@babel/plugin-proposal-nullish-coalescing-operator@^7.13.8": | |||
"@babel/plugin-proposal-nullish-coalescing-operator@^7.13.8": |
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.
FYI: This was the result after I removed the codecov
package with yarn remove codecov
. I checked for what packages are requiring @babel/plugin-proposal-nullish-coalescing-operator
, and it's only @babel/preset-env
which does in fact request @babel/plugin-proposal-nullish-coalescing-operator@^7.13.8
.
So I believe what we see here is correct, and the fact that there were two versions of this package previously installed was unnecessary and that something had gotten out of sync with the lock file.
@maraisr I think this is ready for another round of review whenever you have a minute. Thanks! |
👀 |
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.
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.
Thanks for this one! 🦄
Thanks guys! |
I've added Codecov to the CI Pipeline so that the GitHub Actions workflow will run the unit tests with the coverage report and then pipe the results over to Codecov. I also added a code coverage badge to the README so that we can show off the awesome coverage.
I based my changes off of these two previous MRs in the
focus-trap-react
repo:Closes #260
PR Checklist
Please leave this checklist in your PR.
Unit test coverage added/updated.E2E test coverage added/updated.Typings added/updated.Changeset added (runyarn changeset
locally to add one, and follow the prompts).