Skip to content
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

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

marinojoey
Copy link
Contributor

@marinojoey marinojoey commented Aug 27, 2023

I moved my work from my fork's develop branch into a feature branch and when I force-pushed it closed my old PR automatically (I've gotten used to BitBucket).

@musoke
Copy link
Collaborator

musoke commented Aug 27, 2023

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!

@marinojoey marinojoey changed the title Add test coverage report to README.md and add it to Github Actions Add test coverage report to README.md and Github actions Aug 27, 2023
pull_request:
branches:
- master
- main
Copy link
Contributor

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:
Copy link
Contributor

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

Copy link
Contributor Author

@marinojoey marinojoey Aug 28, 2023

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.

Copy link
Collaborator

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

Copy link
Contributor

@vnugent vnugent Aug 29, 2023

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

@musoke
Copy link
Collaborator

musoke commented Aug 28, 2023 via email

@vnugent
Copy link
Contributor

vnugent commented Aug 28, 2023

Would we fail commits if coverage decreases? I find that coverage is too fragile for automatic failures

what about setting a very low threshold to start?
https://jestjs.io/docs/configuration#coveragethreshold-object

@marinojoey marinojoey force-pushed the feature/test-coverage branch 3 times, most recently from f98416a to 7dffd8a Compare August 31, 2023 02:12
@marinojoey
Copy link
Contributor Author

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.

@vnugent vnugent merged commit 492d026 into OpenBeta:develop Aug 31, 2023
1 check passed
@vnugent
Copy link
Contributor

vnugent commented Aug 31, 2023

@all-contributors add @marinojoey for code and ideas

@allcontributors
Copy link
Contributor

@vnugent

I've put up a pull request to add @marinojoey! 🎉

@vnugent
Copy link
Contributor

vnugent commented Aug 31, 2023

Currently my commits are fairly atomic. Let me know if we'd prefer to squash them, not sure what is preferred.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants