Conversation
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
c8as a dev dependency and introducetest-coverage/coveragenpm scripts. - Update the PR checks workflow to run coverage-enabled tests and enforce coverage thresholds.
- Ignore generated coverage output in
.gitignoreand 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. |
| "c8": { | ||
| "functions": 80, | ||
| "lines": 80, | ||
| "branches": 80 | ||
| } |
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.
| - name: Run unit tests | ||
| if: always() | ||
| run: npm test | ||
| run: npm run test-coverage | ||
|
|
||
| - name: Check code coverage | ||
| if: always() | ||
| run: npm run coverage |
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'
Adds a dev dependency on
c8and uses it to generate code coverage information for the tests (when the newtest-coveragescript is run). The results can then be checked against the thresholds withnpm run coverage. Both new scripts are now used in thepr-checksworkflow. The previousnpm run testscript 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?
.test.tsfiles).pr-checks).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