ci: run lint and unit tests on every PR with a 95% pass-rate gate - #10
Conversation
Add a CI workflow that runs on every pull request (any base branch), on pushes to master/dev, and on manual dispatch. Two gates, both fully in-process — no Postgres, Docker/testcontainers, network, or PDF fixtures: - oxlint (hard fail on lint errors) - tests/unit via vitest, then a pass-rate check: the build fails if fewer than 95% of executed tests pass The pass-rate gate (.github/scripts/check-pass-rate.mjs) parses vitest's JSON report so the test step itself doesn't abort the job; it also fails on a missing report or suites that error before any test runs (import or compile errors). tests/integration (testcontainers-backed) and tests/perf are intentionally excluded since they need external resources. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, check-pass-rate.mjs, to enforce a minimum test pass-rate threshold from Vitest JSON reports, adds the report artifact to .gitignore, and configures Oxlint to allow console logging in GitHub scripts. The review feedback highlights two key improvements: ensuring that individual test suite compilation or import failures are not silently ignored when other suites pass, and validating that the parsed JSON report is a non-null object to prevent runtime errors on empty or invalid inputs.
| if (failedSuites > 0 && executed === 0) { | ||
| console.error(`${failedSuites} test suite(s) errored before any test ran (likely an import or compile error).`) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
The current check failedSuites > 0 && executed === 0 only catches suite-level compilation or import errors if zero tests were executed across the entire run.
If there are multiple test suites, and one suite fails to compile/import (resulting in 0 executed tests for that suite) while another suite runs and passes successfully, executed will be greater than 0. In this scenario, the compilation/import failure in the first suite is completely ignored, and the script could exit with 0 (success) if the overall pass rate of the remaining tests is above the threshold.
To prevent broken test suites from silently passing CI, we should check if any individual test suite failed with zero executed tests (or an empty assertionResults array), which indicates a suite-level compilation, import, or runtime error.
const hasSuiteError = report.testResults?.some(
suite => suite.status === 'failed' && (!suite.assertionResults || suite.assertionResults.length === 0)
)
if (hasSuiteError) {
console.error('One or more test suites failed to compile or import before running any tests.')
process.exit(1)
}There was a problem hiding this comment.
Good catch, fixed in e364354. The gate now walks each suite in testResults and fails if one has status 'failed' with no assertion results, so a suite that won't import can't ride along on the other suites' pass rate. Checked it with a report where one suite errors and the rest pass: it exits 1 now and prints the broken file name.
| let report | ||
| try { | ||
| report = JSON.parse(readFileSync(reportPath, 'utf8')) | ||
| } catch (error) { | ||
| console.error(`Could not read the vitest report at "${reportPath}": ${error.message}`) | ||
| console.error('The test run probably crashed before writing results. Treating as failure.') | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
If the JSON report file is empty or contains null, JSON.parse will succeed but return null (or a non-object like a string/number). This will cause a TypeError when attempting to access properties like report.numPassedTests on line 26, crashing the script with an unhandled exception.
We should explicitly validate that report is a non-null object after parsing.
| let report | |
| try { | |
| report = JSON.parse(readFileSync(reportPath, 'utf8')) | |
| } catch (error) { | |
| console.error(`Could not read the vitest report at "${reportPath}": ${error.message}`) | |
| console.error('The test run probably crashed before writing results. Treating as failure.') | |
| process.exit(1) | |
| } | |
| let report | |
| try { | |
| report = JSON.parse(readFileSync(reportPath, 'utf8')) | |
| if (!report || typeof report !== 'object') { | |
| throw new Error('Parsed JSON is not an object') | |
| } | |
| } catch (error) { | |
| console.error('Could not read or parse the vitest report at "' + reportPath + '": ' + error.message) | |
| console.error('The test run probably crashed before writing results. Treating as failure.') | |
| process.exit(1) | |
| } |
There was a problem hiding this comment.
Fixed in e364354. Added a non-null object check right after the parse, so an empty or null report exits 1 cleanly instead of throwing on report.numPassedTests.
Address Gemini review on the pass-rate gate: - A suite that fails to compile/import produces no assertion results, so its tests drop out of the denominator instead of counting as failures. The old guard only fired when zero tests ran across the whole job, so a broken suite could pass CI as long as the surviving suites cleared 95%. Now scan testResults per suite for status 'failed' with empty assertionResults and fail with the offending file names. - An empty or null report parses to null; reading numPassedTests off it threw a TypeError. Validate it's a non-null object before use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
The repo had no CI — the one workflow (
publish.yml) builds the Docker image on version tags, so nothing checked a branch before merge. This adds aci.ymlthat runs on every pull request, on pushes tomaster/dev, and on manual dispatch.Two gates, both running fully in-process: oxlint, and the
tests/unitsuite behind a pass-rate threshold. The test gate fails the build when fewer than 95% of executed tests pass, instead of the usual all-or-nothing.What changed
.github/workflows/ci.yml(new) — singleverifyjob onubuntu-latest:oven-sh/setup-bun@v2,bun install --frozen-lockfile, Bun cache keyed onbun.lockbbun run lint(oxlint) as a hard gatetests/unitvia vitest, writing a JSON report; the test step uses|| trueso a failing test doesn't abort the job before the gate reads the numberspull_request(no branch filter, so any base),pushtomaster/dev,workflow_dispatch;concurrencycancels superseded runs on the same ref.github/scripts/check-pass-rate.mjs(new) — parses vitest's JSON report and computespassed / (passed + failed). Exits non-zero below 95%. Also fails on a missing report or on suites that error before any test runs, so an import or compile error can't sneak through as "0 failures"..oxlintrc.json— add.github/scripts/**to the existingno-consoleoverride; the gate script prints its results to the log..gitignore— ignore thevitest-report.jsonartifact.How the pass-rate gate reads
The threshold is on executed tests (passed + failed), so skipped/todo tests don't dilute it.
All 294 unit tests pass today, so the gate is green. The 95% only does anything once tests start failing.
A note on the 95% threshold
This is looser than the default. Vitest already fails CI on any single broken test; a 95% gate deliberately tolerates up to 5% failures and keeps the build green. That was the requested behavior. Worth revisiting if you'd rather have a strict all-pass gate later — it's a one-line change (drop the
|| trueand the gate step).What's left out, and why
tests/integrationspins up real Postgres through testcontainers;tests/perfneeds PDF fixtures and timing headroom. Neither fits a quick PR gate, so CI runs unit tests only.tsc --noEmitfails here onCannot find type definition file for 'vite/client'because of thetypeRootsconfig — this project typechecks through Vite, not standalonetsc. Adding it would paint CI red on the first run. Reasonable follow-up if someone wants a real type gate.Test plan
verifyjob goes greenbun run lintexits 0NODE_ENV=test bunx vitest run tests/unit --reporter=json --outputFile=vitest-report.jsonthennode .github/scripts/check-pass-rate.mjs vitest-report.json 95prints 100% and exits 0