-
Notifications
You must be signed in to change notification settings - Fork 937
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
Code Coverage for Tests #2156
base: master
Are you sure you want to change the base?
Code Coverage for Tests #2156
Conversation
What is the intent with how we'd apply this @JamesLMilner ? Is there an automated test that checks if coverage is being improved or if it declines? Or is there are report that we'll be able to see via the checks above? |
@rowanwins the aim here is that we firstly want to be able to see code coverage on push. Ideally we also want to be able to determine how it's changed between master. I'm working on adding another GitHub Action that will run coverage on the current branch and on master so you can compare the difference. I am looking to see if there's a way to write a script that will do a diff on the outputs somehow. |
@@ -38,3 +38,53 @@ jobs: | |||
- run: yarn --frozen-lockfile | |||
- run: git diff --exit-code | |||
- run: yarn test | |||
|
|||
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.
Coverage winds up taking a long time because it has to do all of the builds in the yarn install
step. Circle has the ability to save a workspace and then fork into multiple other tasks. Maybe we can do that here and spend a lot less time on the coverage step? Otherwise we could just calculate it as part of the regular builds since they run tests anyhow.
Also having to re-run coverage on master for every PR seems like its just burning CPU. Maybe we can store the results somewhere and use that for the diff instead of needing to recalculate them every time. I'm not actually sure how people do this in other repos.
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'm not super concerned here because the job processes run in parallel - they shouldn't impact each others performance.
I've removed master
for now, better to just get coverage working and then figure out how to do a diff against master.
d1f4e93
to
3eb09f0
Compare
I think this should be fixed up now |
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.
LGTM I'd like to merge this but the branch is in your fork. Want to update yarn.lock and get this merged?
@JamesLMilner are you still interested in adding coverage data via github actions? |
This adds
nyc
to thealong
module in order to see how it might work for getting information on code coverage. This could eventually close #2138 if we're happy with the approach.Edit: This started as a proof of concept and I transitioned it to a full PR for adding Code Coverage to all packages