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

Chore: enable unit test coverage comment #2637

Merged
merged 8 commits into from
Oct 17, 2023
Merged

Chore: enable unit test coverage comment #2637

merged 8 commits into from
Oct 17, 2023

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Oct 16, 2023

What it solves

I've switched our CI to a different Jest action that has a better coverage report and the same annotations as the previous one.

Also, I've deleted the pre-commit hook because it was really slow.

@github-actions
Copy link

github-actions bot commented Oct 16, 2023

Branch preview

✅ Deploy successful!

https://coverage--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Oct 16, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh katspaugh marked this pull request as draft October 16, 2023 13:18
@github-actions
Copy link

Coverage report

❌ Getting code coverage data failed.

St.
Category Percentage Covered / Total
🟡 Statements 74.84% 8962/11975
🔴 Branches 48.69% 1806/3709
🔴 Functions 56.96% 1322/2321
🟡 Lines 76.43% 8099/10597

Test suite run success

961 tests passing in 133 suites.

Report generated by 🧪jest coverage report action from 38afdf5

@katspaugh katspaugh force-pushed the coverage branch 4 times, most recently from 9aab136 to ac90a73 Compare October 16, 2023 14:21
@github-actions
Copy link

github-actions bot commented Oct 16, 2023

Coverage report

❌ An unexpected error occurred. For more details, check console

Error: The process '/usr/local/bin/npx' failed with exit code 1
St.
Category Percentage Covered / Total
🟡 Statements 74.84% 8962/11975
🔴 Branches 48.69% 1806/3709
🔴 Functions 56.96% 1322/2321
🟡 Lines 76.43% 8099/10597

Test suite run success

961 tests passing in 133 suites.

Report generated by 🧪jest coverage report action from 585a689

@github-actions
Copy link

github-actions bot commented Oct 17, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 74.84% 8962/11975
🔴 Branches 48.69% 1806/3709
🔴 Functions 56.96% 1322/2321
🟡 Lines 76.43% 8099/10597

Test suite run success

961 tests passing in 133 suites.

Report generated by 🧪jest coverage report action from 008127d

@katspaugh katspaugh marked this pull request as ready for review October 17, 2023 09:03
@safe-global safe-global deleted a comment from github-actions bot Oct 17, 2023
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Annotations and coverage report
uses: ArtiomTr/jest-coverage-report-action@v2
Copy link
Member

@usame-algan usame-algan Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we are not using Coveralls? It seems that we already have some projects on there: https://coveralls.io/github/safe-global

It should be free for open-source projects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point of depending on a 3rd-party service for something available locally.

skip-step: install
package-manager: yarn
test-script: yarn test:ci
github-token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

@schmanu schmanu Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs of the coverage tool suggest to use this together with https://github.com/marocchino/sticky-pull-request-comment

Such that there is only one coverage comment that gets updated on change instead of having a new comment on each run.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps a single comment already, those two other comments in this PR are due to a different configuration while I was testing it.

@@ -32,9 +32,6 @@
"engines": {
"node": ">=16"
},
"pre-commit": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we discuss this? It feels out of scope of the coverage report.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's dicuss it, yes. I'll start a thread on Slack.

Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that this setup is missing is seeing the diff to the base branch.
Or will this happen once it's merged to dev?

@katspaugh
Copy link
Member Author

@schmanu this action runs Jest on both the target branch and the PR branch and is supposed to give you a diff. I'm not sure why it's not reflected in this PR. Maybe it needs to be merged to dev indeed.

@katspaugh katspaugh merged commit 7040aee into dev Oct 17, 2023
10 checks passed
@katspaugh katspaugh deleted the coverage branch October 17, 2023 10:42
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants