Skip to content

Commit 8fbbd70

Browse files
os-zhuangclaude
andauthored
ci: assert every test vitest counted actually ran (#3825) (#3852)
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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent cd9a210 commit 8fbbd70

3 files changed

Lines changed: 210 additions & 3 deletions

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
---
3+
4+
ci: assert every test vitest counted actually ran (#3825)
5+
6+
A vitest worker can die at the process level — a native module segfault, OOM, an
7+
abort inside a binding. There is no JS error to catch, so the cases that worker
8+
owned never run and the summary reports only what survived:
9+
10+
```
11+
Test Files 1 passed (40)
12+
Tests 21 passed (401)
13+
```
14+
15+
That leads with "passed". It is **380 tests short**. #3812 hit exactly this shape
16+
(17 cases silently skipped, reported as `22 passed (23)`) and it was found by a
17+
human reading the log closely — which is not a control.
18+
19+
**To be precise about the risk:** the run does exit non-zero, so the gate goes
20+
red. The failure mode is not a false green, it is **a red that reads like a
21+
pass** — someone triaging sees "passed" and a plausible file count and concludes
22+
one file flaked, rather than that a fifth of the suite never executed. This turns
23+
that into a specific, quantified error naming the package and the shortfall. It
24+
also covers the genuinely dangerous variant, where a crash lands somewhere that
25+
does not propagate a non-zero exit at all.
26+
27+
`scripts/check-test-completeness.mjs` reads a saved `turbo run test` log and
28+
asserts, per package, that the tallies (`passed | skipped | failed`) sum to the
29+
declared total. Reading the log rather than wrapping vitest means no change to
30+
the 60+ per-package vitest configs.
31+
32+
Wired into `ci.yml`'s Test Core (both the PR and push steps) and both Dogfood
33+
shards, each as a separate `if: always()` step so it runs **when the suite
34+
failed** — that is when it earns its keep. A red suite plus a green completeness
35+
check means real test failures; a red suite plus a red completeness check means a
36+
worker died.
37+
38+
Two details that are load-bearing rather than incidental:
39+
40+
- The test steps now `tee` their output, and `set -o pipefail` goes with it.
41+
GitHub runs these with `bash -e`, which does **not** set pipefail, so
42+
`turbo … | tee` would report *tee's* exit status and a failing suite would go
43+
green. Verified both ways: with pipefail the step exits 7, without it exits 0.
44+
- Zero summaries in the log is a **pass with an explicit note**, not a silent
45+
one — `turbo run test --affected` legitimately runs nothing when a PR touches
46+
no package.
47+
48+
Validated against the real logs from the #3830 Node 20/22 comparison rather than
49+
synthetic fixtures: the Node 22 log passes (`68 packages, 16678 declared and all
50+
16678 accounted for`), and the Node 20 log fails, naming `@objectstack/driver-sql`
51+
and its 380 missing tests.

.github/workflows/ci.yml

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,11 +141,18 @@ jobs:
141141
# in parallel, and it dominated this job's critical path. The exclusion
142142
# subtracts from the affected set (verified: turbo unions inclusive
143143
# filters, then applies `!` negations to the result).
144+
# `set -o pipefail` is load-bearing: GitHub runs these with `bash -e`, which
145+
# does NOT set it, so `turbo … | tee` would report TEE's status and a
146+
# failing suite would go green. That is the same class of bug the tee is
147+
# here to catch, so it must not be introduced by the catching.
144148
- name: Run affected tests (PR)
145149
if: github.event_name == 'pull_request'
146150
env:
147151
TURBO_SCM_BASE: ${{ github.event.pull_request.base.sha }}
148-
run: pnpm turbo run test --affected --filter=!@objectstack/dogfood --concurrency=4
152+
run: |
153+
set -o pipefail
154+
pnpm turbo run test --affected --filter=!@objectstack/dogfood --concurrency=4 \
155+
2>&1 | tee "$RUNNER_TEMP/test-core.log"
149156
150157
# Push to main: full run. Spec's suite runs here plain (uninstrumented);
151158
# the coverage-instrumented pass moved to the nightly Spec Coverage
@@ -155,7 +162,23 @@ jobs:
155162
# Dogfood job runs it.
156163
- name: Run all tests (push)
157164
if: github.event_name == 'push'
158-
run: pnpm turbo run test --filter=!@objectstack/dogfood --concurrency=4
165+
run: |
166+
set -o pipefail
167+
pnpm turbo run test --filter=!@objectstack/dogfood --concurrency=4 \
168+
2>&1 | tee "$RUNNER_TEMP/test-core.log"
169+
170+
# Runs even when the suite failed — that is when it earns its keep. A red
171+
# suite plus a GREEN completeness check means real test failures; a red
172+
# suite plus a RED completeness check means a worker died and the cases it
173+
# owned never ran, which reads almost identically in the log (#3812).
174+
- name: Test completeness guard
175+
if: always()
176+
run: |
177+
if [ ! -f "$RUNNER_TEMP/test-core.log" ]; then
178+
echo "No test log — the test step did not get far enough to produce one."
179+
exit 0
180+
fi
181+
node scripts/check-test-completeness.mjs "$RUNNER_TEMP/test-core.log"
159182
160183
# Seed the shared Turbo cache from main only (see the restore step
161184
# above). always(): keep the seed fresh even when a test fails, matching
@@ -236,7 +259,22 @@ jobs:
236259
# package's `vitest run` and are hashed into the turbo task, so each
237260
# shard caches independently.
238261
- name: Boot example apps and exercise real user flows
239-
run: pnpm turbo run test --filter=@objectstack/dogfood -- --shard=${{ matrix.shard }}/2
262+
run: |
263+
set -o pipefail
264+
pnpm turbo run test --filter=@objectstack/dogfood -- --shard=${{ matrix.shard }}/2 \
265+
2>&1 | tee "$RUNNER_TEMP/dogfood.log"
266+
267+
# Dogfood boots real apps in-process, so a native/OOM abort is likelier
268+
# here than in the unit suites — and a shard that dies silently looks like
269+
# a shard that had less work.
270+
- name: Test completeness guard
271+
if: always()
272+
run: |
273+
if [ ! -f "$RUNNER_TEMP/dogfood.log" ]; then
274+
echo "No test log — the test step did not get far enough to produce one."
275+
exit 0
276+
fi
277+
node scripts/check-test-completeness.mjs "$RUNNER_TEMP/dogfood.log"
240278
241279
# Replaces the former auto-verify dogfood tests: runs the published
242280
# `objectstack verify` engine over each example app through the CLI —
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
#!/usr/bin/env node
2+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
3+
//
4+
// check-test-completeness -- every test vitest COUNTED must actually have RUN.
5+
//
6+
// A vitest worker can die at the process level -- native module segfault, OOM,
7+
// an abort inside a binding. There is no JS error to catch, so the cases that
8+
// worker owned never run, and the summary reports what survived:
9+
//
10+
// Test Files 1 passed (40)
11+
// Tests 21 passed (401)
12+
//
13+
// That line leads with "passed". It is 380 tests short. #3812 hit exactly this
14+
// (17 cases silently skipped, reported as "22 passed (23)") and it was caught
15+
// by a human reading the log closely, which is not a control.
16+
//
17+
// The run does exit non-zero, so the gate goes red -- the failure mode is not a
18+
// false green, it is a red that READS like a pass. Someone triaging sees
19+
// "passed" and a plausible file count and concludes one file flaked. This turns
20+
// that into a specific, quantified error naming the package and the shortfall.
21+
// It also catches the genuinely dangerous variant, where a crash lands somewhere
22+
// that does not propagate a non-zero exit at all.
23+
//
24+
// node scripts/check-test-completeness.mjs <turbo-test-log>
25+
//
26+
// Reads a saved `turbo run test` log rather than wrapping vitest, so it needs no
27+
// change to the 60+ per-package vitest configs. In CI the test step tees its
28+
// output here. NOTE the tee: `cmd | tee f` reports TEE's exit status, so the
29+
// workflow sets `set -o pipefail` -- without it a failing test suite would look
30+
// green because tee succeeded.
31+
32+
import { readFileSync } from 'node:fs';
33+
34+
const logPath = process.argv[2];
35+
if (!logPath) {
36+
console.error('check-test-completeness: usage: check-test-completeness.mjs <turbo-test-log>');
37+
process.exit(1);
38+
}
39+
40+
let raw;
41+
try {
42+
raw = readFileSync(logPath, 'utf8');
43+
} catch (err) {
44+
console.error(`check-test-completeness: cannot read ${logPath} -- ${err.message}`);
45+
process.exit(1);
46+
}
47+
48+
// Strip ANSI first. vitest colours its summary, and the escape bytes sit
49+
// between the number and its label, so every naive column-based parse of a raw
50+
// log silently reads the wrong field.
51+
const text = raw.replace(/\x1B\[[0-9;]*m/g, '');
52+
53+
// `@objectstack/cli:test: Tests 381 passed | 3 skipped (384)`
54+
// ^ turbo prefix (absent when vitest runs directly) ^ tallies ^ declared
55+
const SUMMARY = /^(?:(\S+?):test:)?\s*(Test Files|Tests)\s+(.+?)\s+\((\d+)\)\s*$/;
56+
57+
const rows = [];
58+
for (const line of text.split('\n')) {
59+
const m = line.match(SUMMARY);
60+
if (!m) continue;
61+
const [, pkg, kind, tallies, declared] = m;
62+
63+
// `381 passed | 3 skipped` -> 384. Every bucket counts as "accounted for";
64+
// a skipped test is a decision, an absent one is a hole.
65+
const counted = [...tallies.matchAll(/(\d+)\s+[a-z]+/g)].reduce((sum, t) => sum + Number(t[1]), 0);
66+
67+
rows.push({
68+
pkg: pkg ?? '(vitest)',
69+
kind,
70+
counted,
71+
declared: Number(declared),
72+
line: line.trim(),
73+
});
74+
}
75+
76+
if (rows.length === 0) {
77+
// Legitimate: `turbo run test --affected` runs nothing when a PR touches no
78+
// package. Say so out loud rather than reporting a vacuous pass.
79+
console.log(
80+
'check-test-completeness: no vitest summaries in the log -- nothing to verify ' +
81+
'(expected when --affected selects no packages).',
82+
);
83+
process.exit(0);
84+
}
85+
86+
const holes = rows.filter((r) => r.counted !== r.declared);
87+
88+
if (holes.length === 0) {
89+
const tests = rows.filter((r) => r.kind === 'Tests');
90+
const total = tests.reduce((sum, r) => sum + r.declared, 0);
91+
console.log(
92+
`check-test-completeness: OK (${tests.length} package(s), ` +
93+
`${total} test(s) declared and all ${total} accounted for).`,
94+
);
95+
process.exit(0);
96+
}
97+
98+
const plural = holes.length === 1 ? 'summary reports' : 'summaries report';
99+
console.error(`check-test-completeness: ${holes.length} ${plural} fewer results than it counted\n`);
100+
for (const h of holes) {
101+
const missing = h.declared - h.counted;
102+
console.error(` • ${h.pkg} -- ${h.kind}: ${h.counted} of ${h.declared} accounted for, ${missing} missing`);
103+
console.error(` ${h.line}`);
104+
}
105+
console.error(`
106+
vitest counted these and then did not report an outcome for all of them. The
107+
usual cause is a worker dying at the process level -- a native module segfault,
108+
OOM, or an abort inside a binding -- which produces no JS error, so the cases
109+
that worker owned never ran and the summary still leads with "passed".
110+
111+
This is not a flake; re-running does not make those tests have run. Reproduce on
112+
the runtime CI uses (see .nvmrc) and look for "Worker exited unexpectedly" or a
113+
non-zero signal exit above the summary.
114+
115+
Precedent: #3812, where a test imported a native better-sqlite3 whose engines
116+
required a newer Node than CI ran. It reported "22 passed (23)" while 17 cases
117+
silently did not run.`);
118+
process.exit(1);

0 commit comments

Comments
 (0)