Add Codecov #202
Add Codecov #202
Conversation
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 ☂️ |
.github/workflows/ci.yml
Outdated
| with: | ||
| name: coverage-reports | ||
| path: coverage/ | ||
| - name: Upload coverage to Codecov |
There was a problem hiding this comment.
Personally I’m not a fan of using the name property too much. It acts as a label, which defaults to the action name. IMO codecov/codecov-action@v4 is more descriptive than a custom label.
There was a problem hiding this comment.
I agree that name is not needed when the uses is descriptive, however in this case I don't think codecov/codecov-action@v4 is more descriptive than Upload coverage to Codecov. it's not obvious that codecov/codecov-action@v4 does just an upload.
.github/workflows/ci.yml
Outdated
| with: | ||
| name: coverage-reports | ||
| path: coverage | ||
| - name: Check coverage thresholds |
There was a problem hiding this comment.
In case of a multi-line shell script I do think the name is useful.
.github/workflows/ci.yml
Outdated
| if [ ! -f coverage/coverage-summary.json ]; then | ||
| echo "No coverage report found" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This doesn’t actually check the thresholds. Also doesn’t Vitest already fail if the threshold isn’t met?
.github/workflows/ci.yml
Outdated
| with: | ||
| name: coverage-reports | ||
| path: coverage/ | ||
| - name: Upload coverage to Codecov |
There was a problem hiding this comment.
I agree that name is not needed when the uses is descriptive, however in this case I don't think codecov/codecov-action@v4 is more descriptive than Upload coverage to Codecov. it's not obvious that codecov/codecov-action@v4 does just an upload.
.github/workflows/ci.yml
Outdated
| name: coverage-reports | ||
| path: coverage/ | ||
| - name: Upload coverage to Codecov | ||
| if: matrix.node == 22 # Only upload coverage from the latest Node.js version |
There was a problem hiding this comment.
maybe we can move this into the coverage job, then we don't have to check for node version again also
Co-authored-by: Remco Haszing <[email protected]>
Co-authored-by: Mikael Finstad <[email protected]>
No description provided.