Conversation
gbrennon
commented
Mar 9, 2026
gbrennon
commented
- chore(scripts): add coverage check script
- chore(scripts): improve test runner
There was a problem hiding this comment.
Pull request overview
This PR refactors the test workflow by introducing a dedicated coverage-check script and wiring just test to run through it, aiming to provide a clearer coverage summary and enforce a minimum coverage threshold.
Changes:
- Add
scripts/check_coverage.shto runcargo llvm-cov, extract JSON totals, print a summary, and enforce a lines-coverage threshold. - Update
just testto run the new coverage-check script instead of invokingcargo llvm-covdirectly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/check_coverage.sh | New coverage-check runner that generates/parses cargo llvm-cov JSON and enforces a threshold. |
| justfile | Routes just test through the new coverage-check script. |
| # Normalize null to 0 | ||
| lines_percent=${lines_percent:-0} | ||
| functions_percent=${functions_percent:-0} | ||
| regions_percent=${regions_percent:-0} |
There was a problem hiding this comment.
The “Normalize null to 0” logic doesn’t work as written: jq -r will emit the literal string null when a field is missing, and ${lines_percent:-0} won’t replace that. Handle the null string explicitly (or make jq default to 0) to avoid printing null% and to keep comparisons predictable.
| # Run all tests with coverage | ||
| test: | ||
| cargo llvm-cov --features test-helpers --ignore-filename-regex "ports/fakes" | ||
| ./scripts/check_coverage.sh |
There was a problem hiding this comment.
Running ./scripts/check_coverage.sh makes just test depend on external tools like jq in addition to cargo-llvm-cov. Consider updating the workflow/docs/dev tooling to ensure jq is available (or have the script emit a clear install hint), otherwise just test may fail unexpectedly on fresh machines/CI images.
| # Generate JSON + text report; do not fail immediately so we can print friendly summary | ||
| cargo llvm-cov --features test-helpers --ignore-filename-regex "ports/fakes" --json --output-path cov.json || true | ||
|
|
||
| if [ ! -f cov.json ]; then | ||
| echo "ERROR: cov.json not found. cargo-llvm-cov failed to produce JSON output." | ||
| exit 1 |
There was a problem hiding this comment.
The cargo llvm-cov ... || true suppresses test/coverage command failures, and since the script doesn’t remove any existing cov.json first, a failed run can still pass by reading a stale cov.json from a previous successful run. Capture the exit code, delete/overwrite cov.json before running, and fail the script if cargo llvm-cov fails (after printing any available summary).
| # Extract totals | ||
| lines_count=$(jq -r '.data[0].totals.lines.count' cov.json) | ||
| lines_covered=$(jq -r '.data[0].totals.lines.covered' cov.json) | ||
| lines_percent=$(jq -r '.data[0].totals.lines.percent' cov.json) | ||
| functions_percent=$(jq -r '.data[0].totals.functions.percent' cov.json) | ||
| regions_percent=$(jq -r '.data[0].totals.regions.percent' cov.json) |
There was a problem hiding this comment.
This script assumes jq is installed, but there’s no preflight check, so the failure mode is a terse jq: command not found. Add a command -v jq check with a clear error message (or document/install it via the dev tooling) so just test fails with actionable guidance.
