-
Notifications
You must be signed in to change notification settings - Fork 5
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: add codecov integration to Github test action #459
Conversation
b6b17d9
to
7f1ecec
Compare
4e22e13
to
eb71be5
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
13eaa2b
to
a68e88b
Compare
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.
Typically we do squash into one commit before merging.
@@ -85,5 +85,24 @@ jobs: | |||
|
|||
- run: pnpm build | |||
|
|||
- name: Run unit tests | |||
- name: Run unit tests with code coverage |
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 wonder if we still need test-unit
as here I think we're running the tests twice?
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.
It's a good question, likely we don't, though I split them for the sake of the initial testing (at the very least) for a bit of sanity. I should probably remove the test-coverage
command and just stick with unit
for the ci
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.
+1 to @fbwoolf's comment. As this is a small code change, it's more helpful when looking back at git history to see a single commit, and also renders nicer change logs.
Also, we can reference the issue in that commit e.g. ci: add code coverage, closes #34
Nice work Will! To add to what Kyran said, when working an an issue from a different repo, you need to specify the repo to get the issue to link correctly. For example this is an issue from
|
a68e88b
to
ef36acb
Compare
ef36acb
to
a60380f
Compare
Summary in brief
Integrate with codecov to get pretty code coverage metrics in our mono repo. This is just the initial integration commit to test the functionality. I am following their quick start docs and not adding any frills or extras, just to test functionality.
Overview of changes
codecov
integration to repository, addtest-coverage
Github actionv8
reporter for code coverage, remove references toistanbul
coverage reporterpackage
viacodecov.yml
pnpm
to recursively test all of thepackages
directory. 'informational
- as a team we should discuss and commit to thresholdsNote:
mobile
was failing because of@/
import path aliases. I believe this must be fixed in thevitest.config.ts
file but not quite sure how to do so yet. For now I simply didn't include apnpm test:coverage
command in there.Future Enhancements
Specify levels of coverage per package type. For example
apps/*
should probably have a different level of acceptable coverage than most things inpackages/*
.