diff --git a/tests/coverage-gate-thresholds.test.mjs b/tests/coverage-gate-thresholds.test.mjs index f73c5e00..6fbf7583 100644 --- a/tests/coverage-gate-thresholds.test.mjs +++ b/tests/coverage-gate-thresholds.test.mjs @@ -66,23 +66,25 @@ const FLOORS = { // Measured on the suite this commit ships: harness files 100.00% lines / // 99.79% regions (5742/5742 lines, 1938/1942 regions), taken twice -- on // node v22.23.3, the major ci.yml pins, and on node v26.10.0 -- with the two - // runs agreeing to the region. tests/tools/e2e-coverage-report.mjs is the - // only harness file below 100.00%; every other one is at 100.00%. + // runs agreeing to the region. The harness files below 100.00% are + // tests/tools/e2e-coverage-report.mjs and tests/tools/e2e-coverage-run.mjs; + // every other one is at 100.00%. // - // That file's percentage is its ceiling, not a backlog: its uncovered - // regions are the defensive fallbacks the note at the top of the file lists - // as unreachable, so no test can lift it. The per-file floor must stay at - // or below that ceiling until the list shrinks; a probe that lands one of - // them should raise the floor and shorten the list in the same change. + // Those percentages are ceilings, not a backlog: the uncovered regions in + // each are the defensive fallbacks the residual note at the top of that + // file lists as unreachable, so no test can lift them. The per-file floor + // must stay at or below the lowest of those ceilings until a list shrinks; + // a probe that lands one of them should raise the floor and shorten that + // file's note in the same change. // - // The ceiling itself is deliberately not written out here. It is asserted - // below against the note instead, because every attempt to restate it in - // this comment has gone stale while the note moved on: the comment said "6 - // uncovered regions" where the note said 7, and it later said the file was - // capped at 98.57% -- the 7-region figure -- after three of them had been - // deleted and the note had been rewritten to 4 regions. A second test below - // now fails on that shape directly, so the next restatement cannot drift - // silently either. + // The ceilings themselves are deliberately not written out here. They are + // asserted below against the notes instead, because every attempt to restate + // one in this comment has gone stale while the note moved on: the comment + // said "6 uncovered regions" where the note said 7, and it later said the + // file was capped at 98.57% -- the 7-region figure -- after three of them + // had been deleted and the note had been rewritten to 4 regions. A second + // test below now fails on that shape directly, so the next restatement + // cannot drift silently either. // // Both harness floors moved up with this commit. They were previously held // at 97 by tests/helpers-script-sandbox.mjs, whose single uncovered region @@ -109,13 +111,20 @@ function parseGate(command) { // The `--check-harness-file-regions` floor is the only one in this file that // is not set by the suite's measurement but by another file's *claim*: the -// residual note at the top of tests/tools/e2e-coverage-report.mjs enumerates -// the fallbacks no probe could close, totals them, and states the percentage -// they cap that file at. The comment above read that total back by hand and -// went stale -- it said 6 while the note said 7 -- which is the same -// drift-by-omission tests/docs-threshold-parity.test.mjs closes between the -// gate and the contributor docs. Reading it instead of restating it is what -// keeps the two in step. +// residual note at the top of a harness module enumerates the fallbacks no +// probe could close, totals them, and states the percentage they cap that +// file at. The comment above read that total back by hand and went stale -- +// it said 6 while the note said 7 -- which is the same drift-by-omission +// tests/docs-threshold-parity.test.mjs closes between the gate and the +// contributor docs. Reading it instead of restating it is what keeps the two +// in step. +// +// Every harness file that carries such a note is read, not just the lowest +// one of the day. A floor argued from one file alone silently stops being +// argued at all the moment a probe closes that file's residuals: the next +// raise would clear the file it was measured against and land above a ceiling +// nothing here had ever looked at. The lowest ceiling among the notes is the +// one the floor has to clear. // Matched across the comment's own line wrapping: the note is prose in a `//` // block, so any word boundary in it may carry a newline and a `// ` prefix. const RESIDUAL_NOTE = new RegExp( @@ -124,25 +133,27 @@ const RESIDUAL_NOTE = new RegExp( 'they', 'are', '(\\d+)', - 'regions,', + // Singular for a note that is down to its last one. + 'regions?,', 'which', 'caps', 'this', 'file', 'at', '(\\d+(?:\\.\\d+)?)%', - 'regions', + 'regions?', ].join('(?:\\s*\\n\\s*//)?\\s+'), 'u', ); const RESIDUAL_FILE = 'tests/tools/e2e-coverage-report.mjs'; +const RESIDUAL_FILES = [RESIDUAL_FILE, 'tests/tools/e2e-coverage-run.mjs']; -function residualCeiling() { - const source = readFileSync(`${repoRoot}${RESIDUAL_FILE}`, 'utf8'); +function residualCeiling(file = RESIDUAL_FILE) { + const source = readFileSync(`${repoRoot}${file}`, 'utf8'); const match = RESIDUAL_NOTE.exec(source); assert.ok( match, - `${RESIDUAL_FILE} must keep its residual note in the form "Together they are ` + + `${file} must keep its residual note in the form "Together they are ` + 'N regions, which caps this file at P% regions"; the --check-harness-file-regions ' + 'floor below is argued from that sentence, so a rewrite that drops it leaves the ' + 'floor with nothing behind it', @@ -150,21 +161,24 @@ function residualCeiling() { return { regions: Number(match[1]), ceiling: Number(match[2]) }; } -test(`${RESIDUAL_FILE} still caps the harness per-file floor where this file says`, () => { - const { regions, ceiling } = residualCeiling(); - assert.ok( - regions > 0, - `${RESIDUAL_FILE} now lists no residual regions, so it is no longer the lowest ` + - 'harness file; re-measure the harness row and raise --check-harness-file-regions', - ); +test('the harness per-file floor clears every documented residual ceiling', () => { const floor = FLOORS['--check-harness-file-regions']; - assert.ok( - floor <= Math.floor(ceiling), - `--check-harness-file-regions is floored at ${floor}, above the ${ceiling}% ceiling ` + - `${RESIDUAL_FILE} documents for its ${regions} unreachable regions; the gate can ` + - 'only be raised past that by closing one of them and shortening the note in the ' + - 'same change', - ); + for (const file of RESIDUAL_FILES) { + const { regions, ceiling } = residualCeiling(file); + assert.ok( + regions > 0, + `${file} now lists no residual regions, so it no longer caps the harness ` + + 'per-file gate; drop it from RESIDUAL_FILES, re-measure the harness row, ' + + 'and raise --check-harness-file-regions', + ); + assert.ok( + floor <= Math.floor(ceiling), + `--check-harness-file-regions is floored at ${floor}, above the ${ceiling}% ceiling ` + + `${file} documents for its ${regions} unreachable regions; the gate can ` + + 'only be raised past that by closing one of them and shortening the note in the ' + + 'same change', + ); + } }); // Any percentage this file attributes to the residual file by hand, with the diff --git a/tests/e2e-coverage-run-residual-arm.test.mjs b/tests/e2e-coverage-run-residual-arm.test.mjs new file mode 100644 index 00000000..79b6f6a2 --- /dev/null +++ b/tests/e2e-coverage-run-residual-arm.test.mjs @@ -0,0 +1,122 @@ +// The residual note at the top of tests/tools/e2e-coverage-run.mjs names one +// arm no probe could reach: +// +// readProvenance `if (!(error instanceof SyntaxError)) throw error` +// +// It is dead for a reason that lives entirely outside the arm. The `try` it +// sits under wraps a single call, `JSON.parse(raw)`; `raw` is bound from a +// `'utf8'` read, so it is always a string; and `JSON.parse` of a string throws +// nothing but `SyntaxError`. Each of those is a claim about code that can +// change without the arm changing -- widen the `try` over the `readFile`, or +// over a validator called on the parsed payload, and the re-throw goes live in +// an edit that never touches its line. +// +// An unreachable arm asserted by nothing is indistinguishable from an arm +// whose fixture is merely missing, so these tests state the premise directly, +// the way tests/e2e-coverage-error-arms.test.mjs states the premises behind +// the reporter's residuals. The consequence of getting it wrong is not only +// the region count: the re-throw abandons the whole provenance walk, so one +// unexpected failure on one capture file would lose the runtime records of +// every capture after it, where the `SyntaxError` path deliberately degrades +// to a warning and continues. +// +// The reachable half of the same `catch` -- the warning a malformed capture +// produces -- is exercised against the real function by +// tests/e2e-coverage-runtime.test.mjs and is not repeated here. +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { readFile } from 'node:fs/promises'; +import { fileURLToPath } from 'node:url'; + +const RUN_SOURCE = fileURLToPath( + new URL('./tools/e2e-coverage-run.mjs', import.meta.url), +); + +// `readProvenance`'s body, from its signature to the closing brace in the +// first column. Every function in the module is written at top level, so that +// brace is unambiguous without parsing. +async function readProvenanceSource() { + const source = await readFile(RUN_SOURCE, 'utf8'); + const start = source.indexOf('async function readProvenance('); + assert.notEqual( + start, + -1, + 'tests/tools/e2e-coverage-run.mjs no longer declares readProvenance at top ' + + 'level; the residual note at the top of that file names the arm by this ' + + 'function, so rename the note with the function', + ); + const end = source.indexOf('\n}\n', start); + assert.notEqual(end, -1, 'readProvenance has no top-level closing brace'); + return source.slice(start, end); +} + +test('readProvenance guards nothing but the JSON.parse call', async () => { + const body = await readProvenanceSource(); + const guarded = /\btry\s*\{([\s\S]*?)\}\s*catch\s*\(/u.exec(body); + assert.ok(guarded, 'readProvenance still guards its parse with try/catch'); + const statements = guarded[1] + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== '' && !line.startsWith('//')); + assert.deepEqual( + statements, + ['payload = JSON.parse(raw);'], + 'the try block in readProvenance now guards more than the JSON.parse call, ' + + 'so its catch can see an error JSON.parse did not throw and the ' + + '`throw error` arm the residual note calls unreachable is live; either ' + + 'narrow the try back or cover the arm and shorten the note', + ); +}); + +test('the argument readProvenance parses is always a string', async () => { + const body = await readProvenanceSource(); + assert.match( + body, + /const raw = await readFile\([\s\S]*?,\s*'utf8'\);/u, + "readProvenance no longer binds `raw` from a 'utf8' read; a Buffer or a " + + 'non-string argument widens what JSON.parse can throw past SyntaxError, ' + + 'which is the premise the residual note rests on', + ); + // The same binding must stay outside the guard, or the read's own failures + // reach a catch that only knows how to describe a parse failure. + assert.ok( + body.indexOf('const raw = await readFile(') < body.indexOf('try {'), + 'the `raw` binding moved inside the try block in readProvenance; a read ' + + 'error would then reach the catch, which is exactly what the ' + + '`throw error` arm re-throws', + ); +}); + +// Empirical half: the residual note claims no probe could make `JSON.parse` +// fail with anything but a SyntaxError. These are the probes. +const HOSTILE_INPUTS = new Map([ + ['empty', ''], + ['truncated object', '{"runId":'], + ['truncated array', '[1,2,'], + ['trailing comma', '{"a":1,}'], + ['lone surrogate', '"\uD800"'], + ['NUL bytes', '\u0000\u0000\u0000'], + ['a bare identifier', 'undefined'], + ['deeply nested arrays', `${'['.repeat(200000)}1${']'.repeat(200000)}`], + ['deeply nested objects', `${'{"a":'.repeat(200000)}1${'}'.repeat(200000)}`], + ['unterminated deep nesting', '['.repeat(200000)], +]); + +test('JSON.parse of a string throws only SyntaxError', () => { + for (const [name, input] of HOSTILE_INPUTS) { + let thrown = null; + try { + JSON.parse(input); + } catch (error) { + thrown = error; + } + if (thrown === null) continue; + assert.ok( + thrown instanceof SyntaxError, + `JSON.parse threw a ${thrown?.constructor?.name} for ${name}; the ` + + '`throw error` arm in readProvenance is reachable after all, so cover ' + + 'it and shorten the residual note at the top of ' + + 'tests/tools/e2e-coverage-run.mjs', + ); + } +}); diff --git a/tests/tools/e2e-coverage-run.mjs b/tests/tools/e2e-coverage-run.mjs index 5c2be099..4728751d 100644 --- a/tests/tools/e2e-coverage-run.mjs +++ b/tests/tools/e2e-coverage-run.mjs @@ -15,6 +15,34 @@ import { captureRunScripts } from './e2e-coverage-scripts.mjs'; const repoRoot = fileURLToPath(new URL('../..', import.meta.url)); +// Residual uncovered regions, recorded the way tests/tools/e2e-coverage-report.mjs +// records its own so this file's region percentage is not re-investigated every +// cycle. Named by function and expression rather than by line, so the note does +// not go stale when the file moves: +// +// readProvenance `if (!(error instanceof SyntaxError)) throw error` -- the +// re-throw arm needs `JSON.parse` to fail with something other than a +// `SyntaxError`. The `try` wraps that one call and nothing else, and +// `raw` is bound from a `'utf8'` read, so the argument is always a +// string; `JSON.parse` of a string throws only `SyntaxError`. The +// `SyntaxError` half of the same `catch` is reached by +// tests/e2e-coverage-runtime.test.mjs. +// +// That premise is a claim about the code around the arm, not about the arm: a +// later edit that widens the `try` -- over the `readFile`, or over a validator +// -- makes the re-throw live without touching the line. It is asserted +// directly by tests/e2e-coverage-run-residual-arm.test.mjs, the way +// tests/e2e-coverage-error-arms.test.mjs asserts the premises behind the +// reporter's residuals, so a change that makes the arm reachable fails on its +// own name rather than merely moving the region count. Together they are 1 +// region, which caps this file at 99.15% regions; +// tests/coverage-gate-thresholds.test.mjs reads that ceiling beside +// `--check-harness-file-regions` so the floor is never set above it. The +// denominator is V8's, so it drifts a little with the Node major -- 1/117 was +// measured on Node 26 while CI pins Node 22 -- which is why the floor sits +// below the ceiling rather than against it. Close the arm and shorten this +// note in the same change; do not raise the floor past it alone. + export const COVERAGE_RUN_SCHEMA_VERSION = 1; export const COVERAGE_RUN_KIND = 'endusers.e2e.coverage-run'; export const COVERAGE_ARTIFACT_KIND = 'endusers.playwright.v8-coverage';