From 3e0ddb56a7c50eb4f7ab5f9080dbb7fab51a140d Mon Sep 17 00:00:00 2001 From: quality Date: Sun, 11 Oct 2026 12:40:36 -0400 Subject: [PATCH] test: cover realGitPath's null-stdout fallback and ratchet the harness floors tests/helpers-script-sandbox.mjs was the lowest file in the harness coverage row at 97.06% regions, with one uncovered region: the `?? ''` in realGitPath. Its sibling `|| '/usr/bin/git'` is already driven by helpers-script-sandbox-fallbacks.test.mjs through an empty PATH, but `?? ''` guards the case spawnSync signals with `stdout: null` -- a process that never started -- and the call passes `shell: true`, so reaching it inline would mean making /bin/sh unspawnable for one call and not for the rest of the file. Split the normalisation out as an exported gitPathFromLookup so the arm is a function of the one value spawnSync varies, and drive both fallbacks beside the inputs that skip them. realGitPath keeps its behaviour; the integration test that reaches the arm through an empty PATH is unchanged and still passes. That region was load-bearing for the gate. The note in tests/coverage-gate-thresholds.test.mjs held --check-harness-file-regions at 97 "while helpers-script-sandbox.mjs sits at 97.06", and left --check-harness-regions at 97 against a measured 99.57 "until that file has been lifted". It has been, so both floors follow the measurement up: per-file to 98, bound by tests/tools/e2e-coverage-report.mjs at 98.57% whose six remaining regions are the documented-unreachable #1253 fallbacks, and the aggregate to 99 against a measured 99.63%. Measured after the change (TZ=UTC node tests/tools/coverage-report.mjs, node v26.10.0): tests/helpers-script-sandbox.mjs 100.00% lines / 100.00% regions; harness files 100.00% / 99.63% (5352/5352 lines, 1870/1877 regions). npm run test:unit:coverage:check passes at the new floors, and the raised floors were confirmed to fail the gate-threshold test when package.json is loosened back to 97. Signed-off-by: quality --- package.json | 2 +- tests/coverage-gate-thresholds.test.mjs | 33 ++++++++------- .../helpers-script-sandbox-fallbacks.test.mjs | 41 ++++++++++++++++++- tests/helpers-script-sandbox.mjs | 19 ++++++++- 4 files changed, 77 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 386251a5..9babca71 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "pr-queue-hygiene": "node scripts/pr-queue-hygiene.mjs", "test:unit": "TZ=UTC node --test", "test:unit:coverage": "TZ=UTC node tests/tools/coverage-report.mjs", - "test:unit:coverage:check": "TZ=UTC node tests/tools/coverage-report.mjs --check 99 --check-source 100 --check-regions 96 --check-source-regions 100 --check-source-file-regions 100 --check-harness 100 --check-harness-regions 97 --check-harness-file-regions 97 --require-source-files", + "test:unit:coverage:check": "TZ=UTC node tests/tools/coverage-report.mjs --check 99 --check-source 100 --check-regions 96 --check-source-regions 100 --check-source-file-regions 100 --check-harness 100 --check-harness-regions 99 --check-harness-file-regions 98 --require-source-files", "test:e2e": "playwright test", "build:e2e:coverage": "npm run build:e2e:coverage:site && npm run build:e2e:coverage:fixtures", "build:e2e:coverage:site": "DOCUSAURUS_NO_PERSISTENT_CACHE=1 E2E_COVERAGE=1 npm run build:production", diff --git a/tests/coverage-gate-thresholds.test.mjs b/tests/coverage-gate-thresholds.test.mjs index ca7455aa..66ae081d 100644 --- a/tests/coverage-gate-thresholds.test.mjs +++ b/tests/coverage-gate-thresholds.test.mjs @@ -62,28 +62,31 @@ const FLOORS = { // entered none of the ratios above, which are all built from // isSourceFile(). These are the same three shapes over its own row. // Measured on the suite this commit ships: harness files 100.00% lines / - // 99.57% regions (5336/5336 lines, 1867/1875 regions), now that #1294 has - // brought tests/helpers*.mjs into the row. The lowest harness file is - // tests/helpers-script-sandbox.mjs at 97.06%, floored to 97; next is - // tests/tools/e2e-coverage-report.mjs at 98.57%, and every other harness - // file is at 100.00%. + // 99.63% regions (5352/5352 lines, 1870/1877 regions), now that #1294 has + // brought tests/helpers*.mjs into the row and tests/helpers-script-sandbox.mjs + // has been lifted to 100.00%. The lowest harness file is now + // tests/tools/e2e-coverage-report.mjs at 98.57%, floored to 98, and every + // other harness file is at 100.00%. // - // 98.57% is e2e-coverage-report.mjs's ceiling, not a backlog: its 7 + // 98.57% is e2e-coverage-report.mjs's ceiling, not a backlog: its 6 // uncovered regions are the defensive fallbacks the note at the top of the // file lists as unreachable (#1253; three more were deleted rather than // listed in #1304), so no test can lift it. The per-file floor must stay at - // or below 97 while helpers-script-sandbox.mjs sits at 97.06, and at or - // below 98 until that list shrinks; a probe that lands one of them should + // or below 98 until that list shrinks; a probe that lands one of them should // raise the floor and shorten the list in the same change. // - // --check-harness-regions is left at 97 against a measured 99.57 on purpose: - // #1294 only just rescoped the row, and the helper tree's lowest file is - // 0.06 points above the per-file floor, so the aggregate is left with its - // slack until that file has been lifted rather than ratcheted into a gate - // the next helper edit trips. + // Both harness floors moved up with this commit. They were previously held + // at 97 by tests/helpers-script-sandbox.mjs, whose single uncovered region + // was realGitPath's `?? ''` arm: spawnSync only reports `stdout: null` for a + // process that never started, and that call passes `shell: true`, so no test + // could reach it while the expression stayed inline. Splitting the + // normalisation out as gitPathFromLookup made it a function of the one value + // spawnSync varies, and that file is now at 100.00%. The aggregate was + // deliberately left at 97 against a measured 99.57 only "until that file has + // been lifted"; it has been, so it follows the measurement up to 99. '--check-harness': 100, - '--check-harness-regions': 97, - '--check-harness-file-regions': 97, + '--check-harness-regions': 99, + '--check-harness-file-regions': 98, }; function parseGate(command) { diff --git a/tests/helpers-script-sandbox-fallbacks.test.mjs b/tests/helpers-script-sandbox-fallbacks.test.mjs index fbf4410f..884dedca 100644 --- a/tests/helpers-script-sandbox-fallbacks.test.mjs +++ b/tests/helpers-script-sandbox-fallbacks.test.mjs @@ -1,6 +1,9 @@ import assert from 'node:assert/strict'; import test from 'node:test'; -import { runScriptInSandbox } from './helpers-script-sandbox.mjs'; +import { + gitPathFromLookup, + runScriptInSandbox, +} from './helpers-script-sandbox.mjs'; // tests/helpers-spawn-fallbacks.test.mjs pins the same contract for the three // other sandbox helpers (helpers.mjs, helpers-fetch-mock.mjs, @@ -137,3 +140,39 @@ test('runScriptInSandbox falls back to /usr/bin/git when no git is on PATH', () assert.equal(result.stderr, ''); assert.equal(result.outputs['data/awards.json'], AWARDS_FIXTURE); }); + +// The test above reaches realGitPath's `|| '/usr/bin/git'` the way a caller +// does, through an empty PATH. Its sibling fallback, the `?? ''` that absorbs +// a null stdout, has no such route: spawnSync only reports `stdout: null` when +// the process never started, and realGitPath passes `shell: true`, so the +// process in question is /bin/sh. Nothing a test can do to this repository +// makes /bin/sh unspawnable for one call and not for the rest of the file. +// +// gitPathFromLookup is that normalisation as a function of the one value +// spawnSync varies, so the arm is driven by handing it the value spawnSync +// would have produced rather than by trying to produce it. The cases below +// pair each fallback with the input that skips it, so a fallback deleted or +// inverted fails here instead of silently handing the git shim a path it will +// `exec ""`. +test('gitPathFromLookup returns the resolved path when the lookup printed one', () => { + // What `command -v git` prints on a normal run: an absolute path and a + // trailing newline, which must be trimmed and otherwise left alone. + assert.equal(gitPathFromLookup('/usr/local/bin/git\n'), '/usr/local/bin/git'); +}); + +test('gitPathFromLookup falls back when the lookup printed nothing', () => { + // `command -v git` found nothing: it exits non-zero and prints an empty + // string, which trims to '' and must not be handed to the shim. + assert.equal(gitPathFromLookup(''), '/usr/bin/git'); + assert.equal(gitPathFromLookup(' \n'), '/usr/bin/git'); +}); + +test('gitPathFromLookup falls back when the lookup never ran', () => { + // spawnSync reports a process that never started as `stdout: null` rather + // than throwing, and `undefined` is what a caller reading `.stdout` off such + // a result sees when the field is absent. Without the `?? ''` both would + // throw on `.trim()`, so this is the arm that keeps a failed spawn + // indistinguishable from a failed lookup at the shim's edge. + assert.equal(gitPathFromLookup(null), '/usr/bin/git'); + assert.equal(gitPathFromLookup(undefined), '/usr/bin/git'); +}); diff --git a/tests/helpers-script-sandbox.mjs b/tests/helpers-script-sandbox.mjs index b4f76382..89c035f5 100644 --- a/tests/helpers-script-sandbox.mjs +++ b/tests/helpers-script-sandbox.mjs @@ -231,6 +231,23 @@ function writeFiles(base, tree) { } } +// The normalisation half of realGitPath, split out so both of its fallbacks can +// be driven directly. +// +// `|| '/usr/bin/git'` is reachable from the outside: an empty PATH makes +// `command -v git` find nothing and print nothing, which +// tests/helpers-script-sandbox-fallbacks.test.mjs exercises through +// runScriptInSandbox. `?? ''` is not. It guards the case spawnSync signals by +// returning `stdout: null` — the shell itself failing to start — and the call +// passes `shell: true`, so reaching it would mean making /bin/sh unspawnable +// for one call in this process. Leaving it inline left the arm as the single +// uncovered region in this file, which alone held the gate's +// --check-harness-file-regions floor down; as a function it is reachable by +// passing the value spawnSync would have produced. +export function gitPathFromLookup(stdout) { + return (stdout ?? '').trim() || '/usr/bin/git'; +} + // Resolved before the shim directory is prepended to PATH so the shim delegates // to the real binary instead of recursing into itself. function realGitPath() { @@ -238,5 +255,5 @@ function realGitPath() { encoding: 'utf8', shell: true, }); - return (found.stdout ?? '').trim() || '/usr/bin/git'; + return gitPathFromLookup(found.stdout); }