-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add test coverage report to README.md and Github actions #152
Conversation
Yeah I don't understand the exact criteria, but GitHub closes PRs when the branch gets deleted or all the changes get removed. Thanks for looking into this! |
.github/workflows/code-coverage.yml
Outdated
pull_request: | ||
branches: | ||
- master | ||
- main |
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.
develop
is our one and only one branch at the moment
README.md
Outdated
@@ -137,6 +137,22 @@ yarn install | |||
yarn test | |||
``` | |||
|
|||
#### Generating Test Coverage Report | |||
|
|||
To generate a test coverage report using Jest, follow these steps: |
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.
Do you think contributors would run it if it's optional? What if we add it to the pre-commit hook?
https://github.com/OpenBeta/sandbag/blob/develop/.husky/pre-commit
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 could add it if you'd prefer! It may be noise to those uninterested is my only hesitation. There's a lot in the pre-commit hook already. That said, it adds negligible time to the test suite which is already ran pre-commit anyways. Maybe I'll look into swapping the coverage report in place of the pure tests? Here is the output of npx jest --coverage
for reference.
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 was also worried about noise but @vnugent's suggestion would probably address my concerns
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.
@marinojoey showed me the report on a call yesterday. I don't remember the exact breakdown but it's actually looking very good! I think we agree to even lower the threshold.
@marinojoey can you update the trigger event to on pull_request_target
? I'm good to merge then.
https://github.com/marketplace/actions/jest-coverage-report#forks-with-no-write-permission
Would we fail commits if coverage decreases? I find that coverage is too
fragile for automatic failures
…On Sun, Aug 27, 2023, 18:36 Viet Nguyen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#152 (comment)>:
> @@ -137,6 +137,22 @@ yarn install
yarn test
```
+#### Generating Test Coverage Report
+
+To generate a test coverage report using Jest, follow these steps:
Do you think contributors would run it if it's optional? What if we add it
to the pre-commit hook?
https://github.com/OpenBeta/sandbag/blob/develop/.husky/pre-commit
—
Reply to this email directly, view it on GitHub
<#152 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD7ET7GP2CWU34Z43CQYYSLXXPRXVANCNFSM6AAAAAA4ATEWPQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
what about setting a very low threshold to start? |
f98416a
to
7dffd8a
Compare
7dffd8a
to
27d571c
Compare
coverage threshold has been added, and it is very low. Currently my commits are fairly atomic. Let me know if we'd prefer to squash them, not sure what is preferred. |
@all-contributors add @marinojoey for code and ideas |
I've put up a pull request to add @marinojoey! 🎉 |
For future reference you can publish multiple commits to an open PR. When a project maintainer is ready to accept the PR, they will squash all the commits. |
I moved my work from my fork's
develop
branch into afeature
branch and when I force-pushed it closed my old PR automatically (I've gotten used to BitBucket).