diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d13f459cd..f6dbf9ea96 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -421,10 +421,36 @@ jobs: --report-dir "$RUNNER_TEMP/stall-reports" -- \ pnpm turbo run test $FILTERS --concurrency=4 - # 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). + # Runs even when the suite failed — that is when it earns its keep. It + # answers TWO questions about a red suite, and needs both to be able to + # say anything at all about a green one. + # + # 1. Was every test vitest COUNTED actually run (#3812)? A worker dying + # at the process level leaves a summary that still leads with + # "passed" while falling short of its own declared count — a red + # that READS like a pass. + # 2. Did every package scheduled on this shard report AT ALL (#10032)? + # Question 1 is answered from summary lines PRESENT in the log, so a + # package that printed nothing contributes no row: neither counted + # nor missed, simply invisible. That is why this step is handed the + # shard's scheduled package list (and the `turbo ls` document it was + # sharded from, which resolves each name to a directory) instead of + # grading the log against itself. + # + # ⛔ THE OLD NOTE HERE WAS WRONG AND COST A REAL TRIAGE. It read "a red + # suite plus a GREEN completeness check means real test failures". That + # inference does not hold for case 2, and #10032 is the bill: Test Core + # (2/3) failed naming @objectstack/example-showcase#test while this guard + # printed OK, because the failing package was the one that printed + # nothing — its complete 5083-line job log mentions that package twice, + # both times in the error summary, with no vitest output anywhere. The + # dichotomy is true again only because green now covers both questions. + # + # ⚠ The guard does NOT charge a silent package to this shard when turbo + # simply never reached it — turbo stops scheduling on the first failure, + # so an ordinary red suite leaves later packages unrun. Those are printed + # as notes. See the script header for the two exemptions and the two + # rules that decide red. - name: Test completeness guard if: always() run: | @@ -432,7 +458,9 @@ jobs: 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" + node scripts/check-test-completeness.mjs "$RUNNER_TEMP/test-core.log" \ + --scheduled "$RUNNER_TEMP/shard-packages.txt" \ + --package-list "$RUNNER_TEMP/turbo-ls.json" # A stall's full diagnostic reports (JS stacks, libuv handles, heap # summary per process) outlive the in-log digest — keep them so a #4250 diff --git a/scripts/check-test-completeness.mjs b/scripts/check-test-completeness.mjs index 1818ea62bf..77ffa18d8b 100644 --- a/scripts/check-test-completeness.mjs +++ b/scripts/check-test-completeness.mjs @@ -1,7 +1,9 @@ #!/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. +// check-test-completeness -- two questions about one `turbo run test` log. +// +// Q1. WAS EVERY TEST vitest COUNTED ACTUALLY RUN? (#3812) // // 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 @@ -18,91 +20,687 @@ // 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 +// Q2. DID EVERY PACKAGE SCHEDULED ON THIS SHARD REPORT AT ALL? (#10032) +// +// Q1 is answered from summary lines PRESENT in the log, so it can only grade +// packages that reported. A package emitting NO summary line contributes no +// row: it is neither counted nor missed, it is invisible. Q1's green therefore +// means "every package that reported was internally consistent" and never +// "every package on the shard reported" -- and the two read identically. +// +// Measured, #10032: `Test Core (2/3)` failed with `Failed: +// @objectstack/example-showcase#test` and NOTHING else about that package -- +// no `Test Files` line, no `Tests` line, no FAIL, no test name, in the whole +// 5083-line job log -- while this guard printed +// `OK (11 package(s), 3617 test(s) declared and all 3617 accounted for)`. +// ci.yml's own note read that green as "so these are real test failures" and +// triage went to a wrong hypothesis and stayed there. The underlying +// zero-output event was never reproduced and is NOT diagnosed here; this only +// makes CI able to say that it happened, and to which package. +// +// Q2 needs one input the log cannot supply -- who was SUPPOSED to report -- +// so `--scheduled` takes ci.yml's `$RUNNER_TEMP/shard-packages.txt` and +// `--package-list` takes the `turbo ls --output=json` document it was sharded +// from (for each package's directory). Both, or neither. +// +// ⛔ TWO WAYS TO PRINT NO SUMMARY THAT ARE NOT DEFECTS. A naive +// scheduled-minus-reported join is a false-red machine; each of these was +// measured on this tree (2026-08-20) before the rules below were written. +// +// NOTHING TO RUN -- `shard-packages.txt` lists every package on the shard, +// including those turbo runs no `test` task for at all. 5 of this repo's 77 +// packages declare no `test` script, and 16 more run `vitest run +// --passWithNoTests`, which prints no summary when a package has no test +// files. So a package is expected to report only if it BOTH declares a +// `test` script and owns at least one test file. (Today those two sets +// coincide exactly -- 72 of 77 -- but they drift in opposite directions, so +// both halves are checked rather than one being assumed from the other.) +// +// NEVER REACHED -- turbo stops scheduling on the first failure, so an +// ordinary red suite leaves later packages unrun and silent through no fault +// of their own. Measured: one failing task in a 4-task run printed +// `Tasks: 1 successful, 4 total`, and the two cancelled packages produced no +// summary. Charging those to this guard would put noise on every red suite. +// +// So Q2 asks for a summary only where its absence is a real finding: +// +// RULE A (the #10032 case) -- turbo named the package in its `Failed:` line +// and the log holds no summary for it. The task ran and failed, so "see +// above for more details" is a promise the log does not keep. Always red. +// +// RULE B (the dangerous inverse) -- the run COMPLETED, every task succeeded +// (`Tasks: N successful, N total`), and a package that should have reported +// did not. Nothing was cancelled, so silence here is a suite that went green +// having reported nothing. Always red. +// +// NEITHER -- the run stopped early and the package was simply never reached. +// Printed as a note, never red. This is a deliberate gap and the only one: on +// an aborted run a genuinely-silent package is indistinguishable from a +// cancelled one from the log alone, and Rule A already covers the package the +// abort was about. +// +// node scripts/check-test-completeness.mjs \ +// [--scheduled --package-list ] +// node scripts/check-test-completeness.mjs --self-test // // 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. +// +// ⛔ WHY THE SELF-TEST RUNS ON EVERY INVOCATION, not from a lint step. This +// repo has already paid for the alternative: `partition-test-shards.mjs` +// carried a `--self-test` that NOTHING ran from the day it was written, so +// every assertion in it evaluated never and `--union-into` wrote a `count: 0` +// document beside two items for as long as nobody looked ("a pin nobody runs +// is not a weaker pin, it is no pin", lint.yml). The rules above are pure +// functions over strings, so running all of them costs ~1ms of a job that +// takes minutes, and wiring them into the guard's own startup is the one +// placement from which they cannot become unrun. `--self-test` also stands +// alone for local use. -import { readFileSync } from 'node:fs'; +import { readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import process from 'node:process'; -const logPath = process.argv[2]; -if (!logPath) { - console.error('check-test-completeness: usage: check-test-completeness.mjs '); - process.exit(1); -} +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); -let raw; -try { - raw = readFileSync(logPath, 'utf8'); -} catch (err) { - console.error(`check-test-completeness: cannot read ${logPath} -- ${err.message}`); - process.exit(1); -} +// `@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*$/; + +// turbo's end-of-run roster, both measured on turbo 2.10.10: +// `Failed: @objectstack/embedder-openai#test, @objectstack/sdui-parser#test` +// ` Tasks: 2 successful, 4 total` +const FAILED = /^Failed:\s+(.+?)\s*$/; +const TASKS = /^\s*Tasks:\s+(\d+) successful, (\d+) total\s*$/; // 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, ''); +export function stripAnsi(raw) { + return 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*$/; +// HOW A SUMMARY LINE IS ATTRIBUTED TO A PACKAGE, and why one spelling is not +// enough. Measured on turbo 2.10.10, both shapes from real runs: +// +// STREAM order (what you get running turbo by hand) prefixes every line: +// `@objectstack/types:test: Tests 356 passed (356)` +// +// GROUPED order (what turbo switches to in GitHub Actions, and therefore the +// ONLY shape CI ever writes) emits a group header and NO per-line prefix: +// `::group::@objectstack/spec:test` <- GitHub renders this `##[group]` +// ` Test Files 415 passed (415)` +// ` Tests 11045 passed (11045)` +// `::endgroup::` +// +// Reading only the prefix therefore attributes NOTHING in CI: every summary +// arrives anonymous, every scheduled package looks silent, and the shard join +// reddens the whole shard. That is not hypothetical -- it is what the first +// version of this check did on its own PR (Test Core (1/3), run 32376757655: +// `@objectstack/spec` reported 415 files and 11045 tests, and the guard said it +// had reported nothing). The group header is turbo's own statement of whose +// output follows, so it is read as the primary attribution, not a heuristic. +const GROUP_OPEN = /^(?:::group::|##\[group\])(.+?)\s*$/; +const GROUP_CLOSE = /^(?:::endgroup::|##\[endgroup\])\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(), - }); +export function parseSummaries(text) { + const rows = []; + let group = null; + for (const line of text.split('\n')) { + if (GROUP_CLOSE.test(line)) { + group = null; + continue; + } + const open = line.match(GROUP_OPEN); + if (open) { + // `@objectstack/spec:test` -> spec, but `@objectstack/spec:build` and + // GitHub's own `Run