diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index 18e24273..b331c9db 100644 --- a/scripts/check-test-count.js +++ b/scripts/check-test-count.js @@ -64,78 +64,206 @@ var TEST_COUNT_FLOOR = 1300; var path = require("path"); var { spawnSync } = require("child_process"); -var files = process.argv.slice(2); -if (files.length === 0) { - process.stderr.write("[check-test-count] no test files given (expected: node scripts/check-test-count.js )\n"); - process.exit(1); +// --------------------------------------------------------------------------- +// CI legibility (lr-e551b9, MILLER re-diagnosis of PR #400 / lr-4e1242 seq 5). +// +// GitHub Actions does not surface arbitrary stderr text anywhere a crew +// agent can reach it: the raw job log requires following a redirect to a +// blob store that loadout-git-host-api correctly refuses (crew-manifest +// lr-90a3e1). A `::error::`-prefixed line, by contrast, is turned into a +// check-run ANNOTATION by the Actions runner, which IS reachable via the +// GitHub API. emitAnnotation() below exists so every FAIL path +// in this script — not just the final exit code — lands somewhere a crew +// agent can actually read it, without changing which conditions fail the +// run (see the module header above: every condition that fails today must +// keep failing). +// --------------------------------------------------------------------------- +function emitAnnotation(message) { + // GitHub Actions workflow-command syntax. `%`, CR and LF must be escaped + // in the message text per GitHub's documented encoding for `::error::` — + // https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions + var escaped = String(message) + .replace(/%/g, "%25") + .replace(/\r/g, "%0D") + .replace(/\n/g, "%0A"); + process.stdout.write("::error::" + escaped + "\n"); } -var REPORTER_PATH = path.join(__dirname, "test-file-completion-reporter.js"); +// classifyRun() is the pure decision core of this script: given the raw +// spawnSync() result plus the bucketed RESULT-line data, decide whether the +// run passes and, if not, exactly WHY. Pulled out of the top-level script +// body (which is otherwise unavoidably I/O-coupled — it shells out to a +// real `node --test`) specifically so the signal-death path (the highest- +// value item in lr-e551b9) is exercisable by a fast, deterministic unit +// test against a hand-built `result` object, without spawning a real child +// process or running the real suite. +// +// Returns { ok: bool, exitCode: number, reason: string|null, kind: string }. +// `kind` names which of this script's distinct FAIL routes produced the +// verdict (see the module header's WHAT THIS DOES AND DOES NOT CATCH / +// MECHANISM sections for the full list). Checked in this precedence order +// when multiple conditions are live at once (lr-e551b9 fold-in, PR #401): +// 'spawn-error' > 'missing-files' > 'signal-death' > 'below-floor' > +// 'test-failure' (a genuine node --test non-zero exit with no wrapper-level +// condition tripped — i.e. an ordinary named test failure, not a wrapper +// failure) > 'ok'. missing-files outranks signal-death because it names a +// specific file, the most actionable cause available; signal-death outranks +// below-floor because a signal-killed run IS the truncated run that +// produces a below-floor count in the first place — see the check itself +// for the full reasoning. +function classifyRun(result, files, resultsByFile, totalTests, floor) { + if (result.error) { + return { + ok: false, + exitCode: 1, + kind: "spawn-error", + reason: "failed to spawn node --test: " + result.error.message, + }; + } -var result = spawnSync(process.execPath, [ - "--test", - "--test-reporter=tap", "--test-reporter-destination=stdout", - "--test-reporter=" + REPORTER_PATH, "--test-reporter-destination=stderr", -].concat(files), { - stdio: ["inherit", "pipe", "pipe"], - encoding: "utf8", - maxBuffer: 64 * 1024 * 1024, -}); + var missingFiles = files.filter(function (f) { + var abs = path.resolve(f); + return !resultsByFile[abs]; + }); -if (result.stdout) process.stdout.write(result.stdout); + if (missingFiles.length > 0) { + return { + ok: false, + exitCode: 1, + kind: "missing-files", + reason: ( + missingFiles.length + " test file(s) reported ZERO test results — " + + "treated as a truncated/incomplete run, not a pass, regardless of the overall exit code. This is " + + "exactly the failure class lr-795882 fixed (MILLER, lr-a7b03e): a file whose tests silently vanish " + + "while the overall run still exits 0.\n " + missingFiles.join("\n ") + ), + }; + } -if (result.error) { - process.stderr.write("[check-test-count] failed to spawn node --test: " + result.error.message + "\n"); - process.exit(1); -} + if (result.status === null) { + // The child `node --test` ORCHESTRATOR process itself (not an + // individual per-file worker/subprocess it manages internally — Node's + // own test runner already reports THOSE as ordinary test:fail events + // with a `signal` field, which is why they show up as RESULT lines + // above and never reach this branch) was killed by a signal rather than + // exiting normally. status is null in exactly this case (Node's + // child_process docs: exactly one of status/signal is non-null). + // Previously this exited 1 with ZERO explanatory output — indistinguishable + // from a genuine test failure at the exit-code level. lr-e551b9. + // + // Checked BEFORE below-floor (PEACHES PR #401 finding, lr-e551b9 + // fold-in): an OOM-killed/externally-signaled orchestrator IS a + // truncated run, and a truncated run is exactly what produces a + // below-floor total — the two conditions fire TOGETHER in the ordinary + // case this whole check exists to make legible. Reporting below-floor + // here would name the wrong cause precisely when the cause matters + // most (the run still fails either way — exitCode stays 1 — but the + // diagnostic would mislabel it). missing-files is still checked above + // this because it names a more specific, actionable culprit (an actual + // file) when both conditions are live. + return { + ok: false, + exitCode: 1, + kind: "signal-death", + reason: ( + "node --test was killed by " + result.signal + " — likely OOM or an external kill (e.g. CI job " + + "timeout/cancellation); no test failure was reported because the process did not exit normally. " + + "This is NOT the same as a per-file worker crash (Node's test runner already reports those as a " + + "named test:fail with a signal field); this is the top-level orchestrator process itself dying." + ), + }; + } -// The custom reporter's RESULT lines are the only thing routed to stderr — -// forward everything else Node itself wrote to stderr (real errors, -// warnings) so a normal `npm test` run still surfaces them, then parse the -// RESULT lines separately below. -var stderrLines = (result.stderr || "").split("\n"); -var resultsByFile = Object.create(null); -var passCount = 0; -var failCount = 0; - -stderrLines.forEach(function (line) { - var match = /^RESULT (pass|fail) (.+)$/.exec(line); - if (!match) { - if (line) process.stderr.write(line + "\n"); - return; + if (totalTests < floor) { + return { + ok: false, + exitCode: 1, + kind: "below-floor", + reason: ( + "total executed test count " + totalTests + + " is below the floor of " + floor + " even though every file reported at least one result — " + + "likely a large in-file test drop. See this script's header for what the floor does and does not " + + "catch, and how to update it for a deliberate suite reduction." + ), + }; + } + + if (result.status !== 0) { + return { + ok: false, + exitCode: result.status, + kind: "test-failure", + reason: ( + "node --test exited " + result.status + " with a named test failure above (see the TAP `not ok` " + + "line(s)) — this is an ordinary test failure, not a wrapper-level condition." + ), + }; } - var kind = match[1]; - var file = match[2]; - if (!resultsByFile[file]) resultsByFile[file] = 0; - resultsByFile[file] += 1; - if (kind === "pass") passCount += 1; - else failCount += 1; -}); - -var missingFiles = files.filter(function (f) { - var abs = path.resolve(f); - return !resultsByFile[abs]; -}); - -if (missingFiles.length > 0) { - process.stderr.write( - "[check-test-count] FAIL: " + missingFiles.length + " test file(s) reported ZERO test results — " + - "treated as a truncated/incomplete run, not a pass, regardless of the overall exit code. This is " + - "exactly the failure class lr-795882 fixed (MILLER, lr-a7b03e): a file whose tests silently vanish " + - "while the overall run still exits 0.\n " + missingFiles.join("\n ") + "\n" - ); - process.exit(1); -} -var totalTests = passCount + failCount; -if (totalTests < TEST_COUNT_FLOOR) { - process.stderr.write( - "[check-test-count] FAIL: total executed test count " + totalTests + - " is below the floor of " + TEST_COUNT_FLOOR + " even though every file reported at least one result — " + - "likely a large in-file test drop. See this script's header for what the floor does and does not " + - "catch, and how to update it for a deliberate suite reduction.\n" - ); - process.exit(1); + return { ok: true, exitCode: 0, kind: "ok", reason: null }; } -process.exit(result.status === null ? 1 : result.status); +module.exports = { classifyRun: classifyRun, emitAnnotation: emitAnnotation }; + +// Everything below only runs when this file is executed directly (`node +// scripts/check-test-count.js `), not when required as a module by +// a unit test. +if (require.main === module) { + var files = process.argv.slice(2); + if (files.length === 0) { + process.stderr.write("[check-test-count] no test files given (expected: node scripts/check-test-count.js )\n"); + process.exit(1); + } + + var REPORTER_PATH = path.join(__dirname, "test-file-completion-reporter.js"); + + var result = spawnSync(process.execPath, [ + "--test", + "--test-reporter=tap", "--test-reporter-destination=stdout", + "--test-reporter=" + REPORTER_PATH, "--test-reporter-destination=stderr", + ].concat(files), { + stdio: ["inherit", "pipe", "pipe"], + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + }); + + if (result.stdout) process.stdout.write(result.stdout); + + // The custom reporter's RESULT lines are the only thing routed to stderr — + // forward everything else Node itself wrote to stderr (real errors, + // warnings) so a normal `npm test` run still surfaces them, then parse the + // RESULT lines separately below. + var stderrLines = (result.stderr || "").split("\n"); + var resultsByFile = Object.create(null); + var passCount = 0; + var failCount = 0; + + stderrLines.forEach(function (line) { + var match = /^RESULT (pass|fail) (.+)$/.exec(line); + if (!match) { + if (line) process.stderr.write(line + "\n"); + return; + } + var kind = match[1]; + var file = match[2]; + if (!resultsByFile[file]) resultsByFile[file] = 0; + resultsByFile[file] += 1; + if (kind === "pass") passCount += 1; + else failCount += 1; + }); + + var totalTests = passCount + failCount; + var verdict = classifyRun(result, files, resultsByFile, totalTests, TEST_COUNT_FLOOR); + + if (!verdict.ok) { + var line = "[check-test-count] FAIL (" + verdict.kind + "): " + verdict.reason; + process.stderr.write(line + "\n"); + // Surfaced as a check-run annotation too (see emitAnnotation's own + // comment) — this is what makes the reason reachable without the raw + // job log, for every FAIL route including the previously-silent + // signal-death path. + emitAnnotation(line); + } + + process.exit(verdict.exitCode); +} diff --git a/test/check-test-count-signal-legibility-lr-e551b9.test.js b/test/check-test-count-signal-legibility-lr-e551b9.test.js new file mode 100644 index 00000000..4a0bb436 --- /dev/null +++ b/test/check-test-count-signal-legibility-lr-e551b9.test.js @@ -0,0 +1,259 @@ +"use strict"; +// Regression tests for lr-e551b9: scripts/check-test-count.js exits 1 +// without naming a cause on several distinct wrapper-level FAIL routes, +// worst of all the signal-death path (result.status === null when the +// spawned `node --test` orchestrator is killed by a signal, e.g. OOM) which +// previously wrote NOTHING before exiting 1 — indistinguishable from a real +// test failure. See lr-4e1242 comment seq 5 (MILLER) for the full diagnosis +// and lr-795882 for why every one of these FAIL conditions must keep +// failing the run (a guard that looks like protection without providing it +// is worse than none — engram 7613980). +// +// classifyRun()/emitAnnotation() are exported by check-test-count.js +// specifically so this file can drive the decision logic directly with a +// hand-built spawnSync()-shaped result object, rather than needing to +// actually spawn `node --test` and kill its process tree with a real +// signal (the file-level "kill a test from inside itself" experiment run +// during this task's investigation showed Node's OWN test runner already +// intercepts a per-file worker crash and reports it as a normal test:fail +// event with a `signal` field — the true signal-death path this task fixes +// is one layer up, the top-level `node --test` orchestrator process dying, +// which is not something a `node --test`-run unit test can reproduce from +// inside that same process tree without killing itself). +// +// DEMONSTRATED-FAILURE VERIFICATION (lr-4e1242 convention): every test below +// was run against the pre-fix script (git stash) before this file existed. +// Pre-fix, classifyRun did not exist at all — check-test-count.js had no +// exported surface, so EVERY test in this file fails outright on +// `require("../scripts/check-test-count.js").classifyRun is not a +// function` against unmodified main. That is the demonstrated failure for +// the whole file: there was no unit-testable decision surface to assert +// against before this change, which is itself the defect lr-e551b9 fixes +// (the logic was unreachable except by paying for a full real `node --test` +// spawn). Per-test behavioral detail below where the mapping is not obvious +// from the function name alone. +// +// STRONG-FORM DEMONSTRATED FAILURE (PEACHES PR #401 fold-in): the "missing +// symbol" failure above proves classifyRun is new, not that its VERDICTS +// are correct — a test that would still pass against a WRONG verdict is not +// a behavioral guard. PEACHES found exactly that gap: the below-floor +// branch (~line 137, pre-fold-in) was checked before signal-death, so a +// signal-killed run with a below-floor total (the common real shape — an +// OOM-killed run IS a truncated run) reported kind "below-floor" instead of +// "signal-death". The "signal-death is checked before below-floor" test +// below was verified the strong way: run via `npx node --test +// test/check-test-count-signal-legibility-lr-e551b9.test.js` against the +// mis-ordered implementation BEFORE the reorder landed, it failed with +// `AssertionError [ERR_ASSERTION]: ... expected: 'signal-death', actual: +// 'below-floor'` (test 9 of 12, `code: 'ERR_ASSERTION'`) — a genuine +// wrong-verdict assertion failure, not a missing-symbol error. After +// reordering signal-death ahead of below-floor in classifyRun, the same +// isolated run passed 12/12. The "missing-files still wins ... full +// three-way precedence" test alongside it already passed pre-fix (missing- +// files was already checked first), which is the expected shape: that test +// pins a precedence relationship the reorder must NOT disturb, not one it +// fixes. + +var test = require("node:test"); +var assert = require("node:assert/strict"); +var path = require("path"); +var checkTestCount = require("../scripts/check-test-count.js"); + +var FLOOR = 1300; + +function absFile(name) { + return path.resolve(name); +} + +test("lr-e551b9: signal-death (result.status === null) is no longer silent — reports the signal and fails", function () { + var files = ["test/some-file.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/some-file.test.js")] = 5; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5 + FLOOR, FLOOR); + + assert.equal(verdict.ok, false, "a null-status (signal-killed) run must fail, not pass"); + assert.equal(verdict.exitCode, 1); + assert.equal(verdict.kind, "signal-death"); + assert.ok(verdict.reason, "the signal-death path must produce a non-empty reason (previously: nothing)"); + assert.ok(verdict.reason.indexOf("SIGKILL") !== -1, + "the reason must name the actual signal, not just say 'killed'; got: " + verdict.reason); +}); + +test("lr-e551b9: signal-death names whichever signal fired, not a hardcoded one", function () { + var files = ["test/some-file.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/some-file.test.js")] = 5; + var result = { status: null, signal: "SIGTERM", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5 + FLOOR, FLOOR); + + assert.equal(verdict.kind, "signal-death"); + assert.ok(verdict.reason.indexOf("SIGTERM") !== -1, "must report SIGTERM, not a different signal name"); +}); + +test("lr-e551b9: a genuine test failure (non-null, non-zero status) is distinguished from a wrapper failure", function () { + var files = ["test/some-file.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/some-file.test.js")] = 5; + var result = { status: 1, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5 + FLOOR, FLOOR); + + assert.equal(verdict.ok, false); + assert.equal(verdict.exitCode, 1); + assert.equal(verdict.kind, "test-failure", + "a plain node --test failure must be labeled distinctly from signal-death/missing-files/below-floor"); +}); + +test("lr-e551b9: missing-files path is preserved unchanged (still fails, still names the files)", function () { + var files = ["test/present.test.js", "test/absent.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 3; + var result = { status: 0, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 3, FLOOR); + + assert.equal(verdict.ok, false, "a file with zero RESULT lines must still fail the run"); + assert.equal(verdict.kind, "missing-files"); + assert.ok(verdict.reason.indexOf("test/absent.test.js") !== -1, + "must name the specific missing file; got: " + verdict.reason); + assert.ok(verdict.reason.indexOf("test/present.test.js") === -1, + "must NOT name the file that did report results"); +}); + +test("lr-e551b9: below-floor path is preserved unchanged (still fails when every file reported but total is low)", function () { + var files = ["test/a.test.js", "test/b.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/a.test.js")] = 1; + resultsByFile[absFile("test/b.test.js")] = 1; + var result = { status: 0, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 2, FLOOR); + + assert.equal(verdict.ok, false, "every file reporting at least one result must not mask a total below the floor"); + assert.equal(verdict.kind, "below-floor"); + assert.ok(verdict.reason.indexOf("2") !== -1 && verdict.reason.indexOf(String(FLOOR)) !== -1, + "must report both the actual total and the floor; got: " + verdict.reason); +}); + +test("lr-e551b9: spawn-error path is preserved unchanged (spawnSync itself failing to launch the child)", function () { + var files = ["test/a.test.js"]; + var resultsByFile = {}; + var result = { status: null, signal: null, error: new Error("ENOENT: node not found") }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 0, FLOOR); + + assert.equal(verdict.ok, false); + assert.equal(verdict.kind, "spawn-error"); + assert.ok(verdict.reason.indexOf("ENOENT") !== -1, "must surface the underlying spawn error message"); +}); + +test("lr-e551b9: a clean run (every file reported, floor met, status 0) still passes", function () { + var files = ["test/a.test.js", "test/b.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/a.test.js")] = FLOOR; + resultsByFile[absFile("test/b.test.js")] = 1; + var result = { status: 0, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, FLOOR + 1, FLOOR); + + assert.equal(verdict.ok, true, "a genuinely clean run must still pass — this fix must not introduce a new false FAIL"); + assert.equal(verdict.exitCode, 0); + assert.equal(verdict.kind, "ok"); + assert.equal(verdict.reason, null); +}); + +test("lr-e551b9: check ORDER — missing-files is checked before signal-death (a truncated run with both conditions reports the more specific cause)", function () { + // If node --test died by signal AND a file has zero results (the common + // real-world shape: a worker died mid-file, so that file never reported), + // missing-files should win — it names the specific file, which is more + // actionable than a bare signal name. + var files = ["test/present.test.js", "test/killed-mid-run.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 5; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5, FLOOR); + + assert.equal(verdict.kind, "missing-files"); + assert.ok(verdict.reason.indexOf("test/killed-mid-run.test.js") !== -1); +}); + +test("lr-e551b9: check ORDER — signal-death is checked before below-floor (PEACHES PR #401 finding: an OOM-killed run IS a truncated run, so these two conditions fire TOGETHER in the ordinary case this task exists to make legible, and the wrong cause must not win)", function () { + // All argv files reported at least one RESULT (so missing-files does NOT + // fire), the orchestrator was signal-killed, AND the total is below the + // floor — the common real shape for an OOM/CI-timeout kill mid-run: every + // file that got to run at least once emitted partial results before the + // process died, so nothing is individually "missing", but the aggregate + // total is far short of a real run. signal-death must win here: it is the + // more actionable, more specific cause (names the actual signal) and is + // the true cause of the truncation, not a coincidental byproduct of it. + var files = ["test/present.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 2; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 2, FLOOR); + + assert.equal(verdict.kind, "signal-death", + "a signal-killed run with a below-floor total must report signal-death, not below-floor — " + + "otherwise the diagnostic reports the wrong cause precisely when the cause matters most; got: " + verdict.kind); + assert.ok(verdict.reason.indexOf("SIGKILL") !== -1, + "must name the actual signal even when below-floor also applies; got: " + verdict.reason); +}); + +test("lr-e551b9: check ORDER — missing-files still wins over signal-death even when below-floor would also apply (full three-way precedence)", function () { + // Reaffirms the existing missing-files-before-signal-death precedence + // (line ~147 above) but with a total that is ALSO below the floor, so + // all three conditions are live at once. missing-files must still win — + // reordering signal-death ahead of below-floor must not disturb the + // higher-precedence missing-files check. + var files = ["test/present.test.js", "test/killed-mid-run.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 2; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 2, FLOOR); + + assert.equal(verdict.kind, "missing-files", + "missing-files must win over both signal-death and below-floor when all three are live; got: " + verdict.kind); + assert.ok(verdict.reason.indexOf("test/killed-mid-run.test.js") !== -1); +}); + +test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to stdout", function () { + var chunks = []; + var originalWrite = process.stdout.write; + process.stdout.write = function (chunk) { + chunks.push(chunk); + return true; + }; + try { + checkTestCount.emitAnnotation("[check-test-count] FAIL (signal-death): node --test was killed by SIGKILL"); + } finally { + process.stdout.write = originalWrite; + } + + var written = chunks.join(""); + assert.ok(written.indexOf("::error::") === 0, "annotation must start with the GitHub Actions ::error:: workflow command"); + assert.ok(written.indexOf("SIGKILL") !== -1, "the annotated message must carry the actual failure reason"); +}); + +test("lr-e551b9: emitAnnotation escapes %, CR and LF per GitHub's documented workflow-command encoding", function () { + var chunks = []; + var originalWrite = process.stdout.write; + process.stdout.write = function (chunk) { + chunks.push(chunk); + return true; + }; + try { + checkTestCount.emitAnnotation("100% failure\r\nline two"); + } finally { + process.stdout.write = originalWrite; + } + + var written = chunks.join(""); + assert.ok(written.indexOf("100%25 failure%0D%0Aline two") !== -1, + "%, CR, LF must be percent-escaped or the annotation body truncates/misparses; got: " + written); +});