Skip to content

ci: run lint and unit tests on every PR with a 95% pass-rate gate - #10

Merged
devanfer02 merged 2 commits into
devfrom
ci/unit-tests
May 29, 2026
Merged

ci: run lint and unit tests on every PR with a 95% pass-rate gate#10
devanfer02 merged 2 commits into
devfrom
ci/unit-tests

Conversation

@devanfer02

Copy link
Copy Markdown
Owner

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 a ci.yml that runs on every pull request, on pushes to master/dev, and on manual dispatch.

Two gates, both running fully in-process: oxlint, and the tests/unit suite 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) — single verify job on ubuntu-latest:
    • oven-sh/setup-bun@v2, bun install --frozen-lockfile, Bun cache keyed on bun.lockb
    • bun run lint (oxlint) as a hard gate
    • tests/unit via vitest, writing a JSON report; the test step uses || true so a failing test doesn't abort the job before the gate reads the numbers
    • the pass-rate check as the final step
    • triggers: pull_request (no branch filter, so any base), push to master/dev, workflow_dispatch; concurrency cancels superseded runs on the same ref
  • .github/scripts/check-pass-rate.mjs (new) — parses vitest's JSON report and computes passed / (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 existing no-console override; the gate script prints its results to the log.
  • .gitignore — ignore the vitest-report.json artifact.

How the pass-rate gate reads

The threshold is on executed tests (passed + failed), so skipped/todo tests don't dilute it.

Report passed failed rate result
current suite 294 0 100% pass
example 95 5 95% pass (at the line)
example 90 10 90% fail
suite import error 0 0 fail

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 || true and the gate step).

What's left out, and why

  • tests/integration spins up real Postgres through testcontainers; tests/perf needs PDF fixtures and timing headroom. Neither fits a quick PR gate, so CI runs unit tests only.
  • Typecheck isn't wired in. Bare tsc --noEmit fails here on Cannot find type definition file for 'vite/client' because of the typeRoots config — this project typechecks through Vite, not standalone tsc. Adding it would paint CI red on the first run. Reasonable follow-up if someone wants a real type gate.

Test plan

  • CI runs on this PR and the verify job goes green
  • Locally: bun run lint exits 0
  • Locally: NODE_ENV=test bunx vitest run tests/unit --reporter=json --outputFile=vitest-report.json then node .github/scripts/check-pass-rate.mjs vitest-report.json 95 prints 100% and exits 0
  • Feed the script a synthetic 90% report and confirm it exits 1
  • Confirm integration/perf suites are not invoked

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>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/scripts/check-pass-rate.mjs Outdated
Comment on lines +39 to +42
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)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +17 to +24
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)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@devanfer02
devanfer02 merged commit 5aa1f7a into dev May 29, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant