From 80a652c9286106dbdf8a49386bbab816662ded68 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 19:46:32 +0800 Subject: [PATCH] ci: assert every test vitest counted actually ran (#3825) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vitest worker can die at the process level -- a native module segfault, OOM, an abort inside a binding. There is no JS error to catch, so the cases that worker owned never run and the summary reports only what survived: Test Files 1 passed (40) Tests 21 passed (401) That leads with "passed". It is 380 tests short. #3812 hit exactly this shape (17 cases silently skipped, reported as "22 passed (23)") and was found by a human reading the log closely, which is not a control. To be precise about the risk: the run does exit non-zero, so the gate goes red. The failure mode is not a false green, it is a red that READS like a pass -- someone triaging sees "passed" and a plausible file count and concludes one file flaked, rather than that a fifth of the suite never executed. This makes it a specific, quantified error naming the package and the shortfall, and also covers the dangerous variant where a crash does not propagate a non-zero exit at all. check-test-completeness.mjs reads a saved `turbo run test` log and asserts, per package, that the tallies (passed | skipped | failed) sum to the declared total. Reading the log rather than wrapping vitest means no change to the 60+ per-package vitest configs. Wired into Test Core (PR and push steps) and both Dogfood shards as separate `if: always()` steps, so it runs when the suite FAILED -- that is when it earns its keep. Red suite + green completeness = real test failures; red suite + red completeness = a worker died. Two load-bearing details: - The test steps now tee, and `set -o pipefail` goes with it. GitHub runs these with `bash -e`, which does NOT set pipefail, so `turbo … | tee` would report TEE's status and a failing suite would go green -- the same class of bug the tee exists to catch. Verified both ways: with pipefail the step exits 7, without it exits 0. - Zero summaries is a pass with an explicit note, not a silent one: `turbo run test --affected` legitimately runs nothing when a PR touches no package. Validated against the real logs from the #3830 Node 20/22 comparison rather than synthetic fixtures: the Node 22 log passes (68 packages, 16678 declared and all 16678 accounted for); the Node 20 log fails, naming @objectstack/driver-sql and its 380 missing tests. Co-Authored-By: Claude --- .changeset/ci-test-completeness-guard.md | 51 ++++++++++ .github/workflows/ci.yml | 44 ++++++++- scripts/check-test-completeness.mjs | 118 +++++++++++++++++++++++ 3 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 .changeset/ci-test-completeness-guard.md create mode 100644 scripts/check-test-completeness.mjs diff --git a/.changeset/ci-test-completeness-guard.md b/.changeset/ci-test-completeness-guard.md new file mode 100644 index 0000000000..ef949addd0 --- /dev/null +++ b/.changeset/ci-test-completeness-guard.md @@ -0,0 +1,51 @@ +--- +--- + +ci: assert every test vitest counted actually ran (#3825) + +A vitest worker can die at the process level — a native module segfault, OOM, an +abort inside a binding. There is no JS error to catch, so the cases that worker +owned never run and the summary reports only what survived: + +``` +Test Files 1 passed (40) + Tests 21 passed (401) +``` + +That leads with "passed". It is **380 tests short**. #3812 hit exactly this shape +(17 cases silently skipped, reported as `22 passed (23)`) and it was found by a +human reading the log closely — which is not a control. + +**To be precise about the risk:** the run does exit non-zero, so the gate goes +red. The failure mode is not a false green, it is **a red that reads like a +pass** — someone triaging sees "passed" and a plausible file count and concludes +one file flaked, rather than that a fifth of the suite never executed. This turns +that into a specific, quantified error naming the package and the shortfall. It +also covers the genuinely dangerous variant, where a crash lands somewhere that +does not propagate a non-zero exit at all. + +`scripts/check-test-completeness.mjs` reads a saved `turbo run test` log and +asserts, per package, that the tallies (`passed | skipped | failed`) sum to the +declared total. Reading the log rather than wrapping vitest means no change to +the 60+ per-package vitest configs. + +Wired into `ci.yml`'s Test Core (both the PR and push steps) and both Dogfood +shards, each as a separate `if: always()` step so it runs **when the suite +failed** — that is when it earns its keep. A red suite plus a green completeness +check means real test failures; a red suite plus a red completeness check means a +worker died. + +Two details that are load-bearing rather than incidental: + +- The test steps now `tee` their output, and `set -o pipefail` goes with it. + GitHub runs these with `bash -e`, which does **not** set pipefail, so + `turbo … | tee` would report *tee's* exit status and a failing suite would go + green. Verified both ways: with pipefail the step exits 7, without it exits 0. +- Zero summaries in the log is a **pass with an explicit note**, not a silent + one — `turbo run test --affected` legitimately runs nothing when a PR touches + no package. + +Validated against the real logs from the #3830 Node 20/22 comparison rather than +synthetic fixtures: the Node 22 log passes (`68 packages, 16678 declared and all +16678 accounted for`), and the Node 20 log fails, naming `@objectstack/driver-sql` +and its 380 missing tests. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1fcac697c4..30ae56a818 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -141,11 +141,18 @@ jobs: # in parallel, and it dominated this job's critical path. The exclusion # subtracts from the affected set (verified: turbo unions inclusive # filters, then applies `!` negations to the result). + # `set -o pipefail` is load-bearing: GitHub runs these with `bash -e`, which + # does NOT set it, so `turbo … | tee` would report TEE's status and a + # failing suite would go green. That is the same class of bug the tee is + # here to catch, so it must not be introduced by the catching. - name: Run affected tests (PR) if: github.event_name == 'pull_request' env: TURBO_SCM_BASE: ${{ github.event.pull_request.base.sha }} - run: pnpm turbo run test --affected --filter=!@objectstack/dogfood --concurrency=4 + run: | + set -o pipefail + pnpm turbo run test --affected --filter=!@objectstack/dogfood --concurrency=4 \ + 2>&1 | tee "$RUNNER_TEMP/test-core.log" # Push to main: full run. Spec's suite runs here plain (uninstrumented); # the coverage-instrumented pass moved to the nightly Spec Coverage @@ -155,7 +162,23 @@ jobs: # Dogfood job runs it. - name: Run all tests (push) if: github.event_name == 'push' - run: pnpm turbo run test --filter=!@objectstack/dogfood --concurrency=4 + run: | + set -o pipefail + pnpm turbo run test --filter=!@objectstack/dogfood --concurrency=4 \ + 2>&1 | tee "$RUNNER_TEMP/test-core.log" + + # Runs even when the suite failed — that is when it earns its keep. A red + # suite plus a GREEN completeness check means real test failures; a red + # suite plus a RED completeness check means a worker died and the cases it + # owned never ran, which reads almost identically in the log (#3812). + - name: Test completeness guard + if: always() + run: | + if [ ! -f "$RUNNER_TEMP/test-core.log" ]; then + echo "No test log — the test step did not get far enough to produce one." + exit 0 + fi + node scripts/check-test-completeness.mjs "$RUNNER_TEMP/test-core.log" # Seed the shared Turbo cache from main only (see the restore step # above). always(): keep the seed fresh even when a test fails, matching @@ -236,7 +259,22 @@ jobs: # package's `vitest run` and are hashed into the turbo task, so each # shard caches independently. - name: Boot example apps and exercise real user flows - run: pnpm turbo run test --filter=@objectstack/dogfood -- --shard=${{ matrix.shard }}/2 + run: | + set -o pipefail + pnpm turbo run test --filter=@objectstack/dogfood -- --shard=${{ matrix.shard }}/2 \ + 2>&1 | tee "$RUNNER_TEMP/dogfood.log" + + # Dogfood boots real apps in-process, so a native/OOM abort is likelier + # here than in the unit suites — and a shard that dies silently looks like + # a shard that had less work. + - name: Test completeness guard + if: always() + run: | + if [ ! -f "$RUNNER_TEMP/dogfood.log" ]; then + echo "No test log — the test step did not get far enough to produce one." + exit 0 + fi + node scripts/check-test-completeness.mjs "$RUNNER_TEMP/dogfood.log" # Replaces the former auto-verify dogfood tests: runs the published # `objectstack verify` engine over each example app through the CLI — diff --git a/scripts/check-test-completeness.mjs b/scripts/check-test-completeness.mjs new file mode 100644 index 0000000000..1818ea62bf --- /dev/null +++ b/scripts/check-test-completeness.mjs @@ -0,0 +1,118 @@ +#!/usr/bin/env node +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// check-test-completeness -- every test vitest COUNTED must actually have RUN. +// +// A vitest worker can die at the process level -- native module segfault, OOM, +// an abort inside a binding. There is no JS error to catch, so the cases that +// worker owned never run, and the summary reports what survived: +// +// Test Files 1 passed (40) +// Tests 21 passed (401) +// +// That line leads with "passed". It is 380 tests short. #3812 hit exactly this +// (17 cases silently skipped, reported as "22 passed (23)") and it was caught +// by a human reading the log closely, which is not a control. +// +// The run does exit non-zero, so the gate goes red -- the failure mode is not a +// false green, it is a red that READS like a pass. Someone triaging sees +// "passed" and a plausible file count and concludes one file flaked. This turns +// that into a specific, quantified error naming the package and the shortfall. +// It also catches the genuinely dangerous variant, where a crash lands somewhere +// that does not propagate a non-zero exit at all. +// +// node scripts/check-test-completeness.mjs +// +// Reads a saved `turbo run test` log rather than wrapping vitest, so it needs no +// change to the 60+ per-package vitest configs. In CI the test step tees its +// output here. NOTE the tee: `cmd | tee f` reports TEE's exit status, so the +// workflow sets `set -o pipefail` -- without it a failing test suite would look +// green because tee succeeded. + +import { readFileSync } from 'node:fs'; + +const logPath = process.argv[2]; +if (!logPath) { + console.error('check-test-completeness: usage: check-test-completeness.mjs '); + process.exit(1); +} + +let raw; +try { + raw = readFileSync(logPath, 'utf8'); +} catch (err) { + console.error(`check-test-completeness: cannot read ${logPath} -- ${err.message}`); + process.exit(1); +} + +// Strip ANSI first. vitest colours its summary, and the escape bytes sit +// between the number and its label, so every naive column-based parse of a raw +// log silently reads the wrong field. +const text = raw.replace(/\x1B\[[0-9;]*m/g, ''); + +// `@objectstack/cli:test: Tests 381 passed | 3 skipped (384)` +// ^ turbo prefix (absent when vitest runs directly) ^ tallies ^ declared +const SUMMARY = /^(?:(\S+?):test:)?\s*(Test Files|Tests)\s+(.+?)\s+\((\d+)\)\s*$/; + +const rows = []; +for (const line of text.split('\n')) { + const m = line.match(SUMMARY); + if (!m) continue; + const [, pkg, kind, tallies, declared] = m; + + // `381 passed | 3 skipped` -> 384. Every bucket counts as "accounted for"; + // a skipped test is a decision, an absent one is a hole. + const counted = [...tallies.matchAll(/(\d+)\s+[a-z]+/g)].reduce((sum, t) => sum + Number(t[1]), 0); + + rows.push({ + pkg: pkg ?? '(vitest)', + kind, + counted, + declared: Number(declared), + line: line.trim(), + }); +} + +if (rows.length === 0) { + // Legitimate: `turbo run test --affected` runs nothing when a PR touches no + // package. Say so out loud rather than reporting a vacuous pass. + console.log( + 'check-test-completeness: no vitest summaries in the log -- nothing to verify ' + + '(expected when --affected selects no packages).', + ); + process.exit(0); +} + +const holes = rows.filter((r) => r.counted !== r.declared); + +if (holes.length === 0) { + const tests = rows.filter((r) => r.kind === 'Tests'); + const total = tests.reduce((sum, r) => sum + r.declared, 0); + console.log( + `check-test-completeness: OK (${tests.length} package(s), ` + + `${total} test(s) declared and all ${total} accounted for).`, + ); + process.exit(0); +} + +const plural = holes.length === 1 ? 'summary reports' : 'summaries report'; +console.error(`check-test-completeness: ${holes.length} ${plural} fewer results than it counted\n`); +for (const h of holes) { + const missing = h.declared - h.counted; + console.error(` • ${h.pkg} -- ${h.kind}: ${h.counted} of ${h.declared} accounted for, ${missing} missing`); + console.error(` ${h.line}`); +} +console.error(` +vitest counted these and then did not report an outcome for all of them. The +usual cause is a worker dying at the process level -- a native module segfault, +OOM, or an abort inside a binding -- which produces no JS error, so the cases +that worker owned never ran and the summary still leads with "passed". + +This is not a flake; re-running does not make those tests have run. Reproduce on +the runtime CI uses (see .nvmrc) and look for "Worker exited unexpectedly" or a +non-zero signal exit above the summary. + +Precedent: #3812, where a test imported a native better-sqlite3 whose engines +required a newer Node than CI ran. It reported "22 passed (23)" while 17 cases +silently did not run.`); +process.exit(1);