Check code coverage in CI - #3788
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds code coverage measurement and enforcement to CI for the repo’s AVA unit tests, while keeping the default local npm test workflow unchanged for speed.
Changes:
- Add c8 as a dev dependency and introduce test-coverage / coverage npm scripts.
- Update the PR checks workflow to run coverage-enabled tests and enforce coverage thresholds.
- Ignore generated coverage output in .gitignore and increase a flaky test timeout.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file| File | Description |
|---|---|
| src/artifact-scanner.test.ts | Increases timeout for a non-Windows integration-style test. |
| package.json | Adds c8, new scripts, and coverage threshold configuration. |
| package-lock.json | Locks new c8 dependency and transitive packages. |
| .gitignore | Ignores coverage/ output directory. |
| .github/workflows/pr-checks.yml | Runs test-coverage and then checks/enforces coverage in CI. |
Sorry, something went wrong.
| "c8": { | ||
| "functions": 80, | ||
| "lines": 80, | ||
| "branches": 80 | ||
| } |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
c8 is currently run without --all, so the coverage check only applies to files actually loaded during tests; completely untested modules won’t count as 0% and can slip through. Consider enabling all and narrowing src/include/exclude (e.g., to the transpiled build/ output) so coverage reflects the whole codebase under test.
Sorry, something went wrong.
There was a problem hiding this comment.
what do you think about this @mbg?
Sorry, something went wrong.
| - name: Run unit tests | ||
| if: always() | ||
| run: npm test | ||
| run: npm run test-coverage | ||
|
|
||
| - name: Check code coverage | ||
| if: always() | ||
| run: npm run coverage |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
The coverage check step is marked if: always(), so it will run even when the preceding test run fails. In that case it may fail due to missing/partial coverage data and add noise on top of the real test failure; consider running the coverage check only when the test-coverage step succeeds (default success()), or give the test step an id and gate on its outcome.
See below for a potential fix:
id: test-coverage
if: always()
run: npm run test-coverage
- name: Check code coverage
if: steps.test-coverage.outcome == 'success'
Sorry, something went wrong.
| run: npm run test-coverage | ||
|
|
||
| - name: Check code coverage | ||
| if: always() |
There was a problem hiding this comment.
pre-existing, but maybe all the always() in this workflow should really be !cancelled(), just in case?
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Adds a dev dependency on c8 and uses it to generate code coverage information for the tests (when the new test-coverage script is run). The results can then be checked against the thresholds with npm run coverage. Both new scripts are now used in the pr-checks workflow. The previous npm run test script does not calculate code coverage so that local testing is not slowed down.
The thresholds are configured to roughly match our current code coverage. Increasing the code coverage (and thresholds) is outside the scope of this PR.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist