diff --git a/TESTING-STANDARDS.md b/TESTING-STANDARDS.md index 4a96cd87a..b289523d5 100644 --- a/TESTING-STANDARDS.md +++ b/TESTING-STANDARDS.md @@ -204,7 +204,7 @@ const r = runHookSeam(WORKER_PATH, [], { timeoutMs: NPM_VIEW_TIMEOUT_MS + WORKER Import an existing class-norm constant from `tests/helpers/timeouts.cjs` (`PROBE_TIMEOUT_MS`, `GIT_TIMEOUT_MS`, `BUILD_TIMEOUT_MS`, `INSTALL_TIMEOUT_MS`) when the call is the same class of subprocess, or declare a local one with a comment justifying why it is a distinct class — see CONTRIBUTING.md's "Use Centralized Test Helpers" section. -**Enforcement:** `local/no-adhoc-timeout-literal` (ESLint, `error`). A non-literal value (an `Identifier`, `MemberExpression`, or `CallExpression`) is trusted; only a resolvable numeric literal is flagged. There is no marker-comment escape — the fix is always to extract a named constant. `allowlist` (`eslint-rules/no-adhoc-timeout-literal.allowlist.json`) exempts pre-existing legacy violations and only ever ratchets down. +**Enforcement:** `local/no-adhoc-timeout-literal` (ESLint, `error`). A non-literal value (an `Identifier`, `MemberExpression`, or `CallExpression`) is trusted; only a resolvable numeric literal is flagged. There is no marker-comment escape — the fix is always to extract a named constant. There is no allowlist. `eslint-rules/no-adhoc-timeout-literal.allowlist.json` grandfathered pre-existing legacy violations; the epic that introduced it (#4445) migrated every site across seventeen batches and deleted the file in its terminal batch, so `local/no-adhoc-timeout-literal` now runs with **no exemption surface**. ### Mutation testing — 80 % threshold diff --git a/eslint-rules/no-adhoc-timeout-literal.allowlist.json b/eslint-rules/no-adhoc-timeout-literal.allowlist.json deleted file mode 100644 index 3e0f5cb9a..000000000 --- a/eslint-rules/no-adhoc-timeout-literal.allowlist.json +++ /dev/null @@ -1,6 +0,0 @@ -[ - "tests/cjs-command-router-adapter.test.cjs", - "tests/dispatcher.test.cjs", - "tests/run-tests-temp-root.test.cjs", - "tests/shell-command-projection-dispatch.test.cjs" -] diff --git a/eslint.config.mjs b/eslint.config.mjs index 5e08a64cc..371aa678c 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -42,7 +42,6 @@ import noAdhocTimeoutLiteral from './eslint-rules/no-adhoc-timeout-literal.cjs'; import noRenderedTextLengthAssert from './eslint-rules/no-rendered-text-length-assert.cjs'; import noUnconfinedPathJoin from './eslint-rules/no-unconfined-path-join.cjs'; -const adhocTimeoutLiteralAllowlist = require('./eslint-rules/no-adhoc-timeout-literal.allowlist.json'); const unconfinedPathJoinAllowlist = require('./eslint-rules/no-unconfined-path-join.allowlist.json'); const localPlugin = { @@ -781,9 +780,9 @@ export default tseslint.config( 'local/no-unbounded-spawn': 'error', // Ban a bare numeric `timeout`/`timeoutMs` literal in tests (DEFECT.AD-HOC-TIMEOUT-LITERAL, // #4428): two independently-guessed copies of the same magic number can drift apart, or - // collide exactly into a zero-margin race. Allowlist starts empty; a pre-existing violation - // gets grandfathered in here as it's found, per eslint-rules/no-adhoc-timeout-literal.allowlist.json. - 'local/no-adhoc-timeout-literal': ['error', { allowlist: adhocTimeoutLiteralAllowlist }], + // collide exactly into a zero-margin race. No allowlist: the epic (#4445) migrated every + // site; the rule runs with no exemption surface. + 'local/no-adhoc-timeout-literal': 'error', // Ban a consolidation-epic folded suite appearing twice in one host file (#3271). // A second copy runs the same tests twice on every lane and drifts silently. 'local/no-duplicate-fold-marker': 'error', diff --git a/tests/cjs-command-router-adapter.test.cjs b/tests/cjs-command-router-adapter.test.cjs index 4409ee304..c85d89077 100644 --- a/tests/cjs-command-router-adapter.test.cjs +++ b/tests/cjs-command-router-adapter.test.cjs @@ -522,6 +522,7 @@ const path = require('node:path'); const os = require('node:os'); const { execFileSync } = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const GSD_TOOLS = path.resolve(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -532,7 +533,7 @@ function run(args, cwd) { stdout: execFileSync(process.execPath, [GSD_TOOLS, ...args], { cwd, encoding: 'utf-8', - timeout: 15000, + timeout: PROBE_TIMEOUT_MS, }), }; } catch (e) { diff --git a/tests/dispatcher.test.cjs b/tests/dispatcher.test.cjs index 4850e5502..a888a57fb 100644 --- a/tests/dispatcher.test.cjs +++ b/tests/dispatcher.test.cjs @@ -452,6 +452,7 @@ const path = require('node:path'); const { spawnSync } = require('node:child_process'); const SDK_CLI = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js'); const fs = require('node:fs'); +const { QUICK_SPAWN_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); describe('bug #3026 (CR Major outside-diff): SDK forwards plain-text help from gsd-tools fallback', () => { test('gsd-sdk query phase --help (fallback path) returns usage, not a JSON parse error', (t) => { @@ -472,7 +473,7 @@ describe('bug #3026 (CR Major outside-diff): SDK forwards plain-text help from g const result = spawnSync(process.execPath, [SDK_CLI, 'query', 'phase', '--help'], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], - timeout: 10000, + timeout: QUICK_SPAWN_TIMEOUT_MS, }); // The fallback gsd-tools.cjs emits exit 0 with usage on stdout. assert.strictEqual(result.status, 0, diff --git a/tests/run-tests-temp-root.test.cjs b/tests/run-tests-temp-root.test.cjs index 4725d01c8..078b70ac2 100644 --- a/tests/run-tests-temp-root.test.cjs +++ b/tests/run-tests-temp-root.test.cjs @@ -23,6 +23,30 @@ const { createTempDir, cleanup } = require('./helpers.cjs'); const RUNNER = path.join(__dirname, '..', 'scripts', 'run-tests.cjs'); +/** + * A `node -e` snippet that requires `scripts/run-tests.cjs` and calls one of + * its exported functions in an isolated child process, purely so the call + * doesn't poison the PARENT process's own os.tmpdir() resolution for every + * other subtest. No subprocess fan-out beneath the single node child. + * Coincides numerically with GENERATOR_SCRIPT_TIMEOUT_MS, BUILD_TIMEOUT_MS, + * and others in tests/helpers/timeouts.cjs, none of which describe this + * shape (scripts/run-tests.cjs is the test runner itself, not a generator/ + * lint script, a hooks bundle, or a git plumbing call) -- kept file-local + * and separately named rather than forced onto an unrelated shared norm. + */ +const RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS = 30000; + +/** + * A REAL end-to-end spawn of scripts/run-tests.cjs itself, running one + * trivial test file through its full lifecycle (temp-root setup, discovery, + * execution, sweep, teardown) -- heavier than + * RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS above, which only calls one exported + * function in isolation. Coincides numerically with INSTALL_TIMEOUT_MS (a + * full bin/install.js run) but describes a completely unrelated operation + * -- disclosed, not merged. + */ +const RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS = 120000; + describe('#4020 — run-tests temp root', () => { // setupRunTempRoot mutates the PROCESS env (TMPDIR/TEMP/TMP), so these rows // drive it in an isolated child — an in-process call would poison every other @@ -43,7 +67,7 @@ describe('#4020 — run-tests temp root', () => { t.after(() => cleanup(outer)); const r = runNode(['-e', setupProbe], { - timeoutMs: 30_000, + timeoutMs: RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS, env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, }); assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`); @@ -68,7 +92,7 @@ describe('#4020 — run-tests temp root', () => { t.after(() => cleanup(outer)); const r = runNode(['-e', probe], { - timeoutMs: 30_000, + timeoutMs: RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS, env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, }); assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`); @@ -136,7 +160,7 @@ describe('#4020 — run-tests temp root', () => { const r = runNode( [RUNNER, '--files', path.basename(target)], - { timeoutMs: 120_000, env: { ...process.env, TMPDIR: sandbox, TEMP: sandbox, TMP: sandbox } }, + { timeoutMs: RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS, env: { ...process.env, TMPDIR: sandbox, TEMP: sandbox, TMP: sandbox } }, ); assert.equal(r.exitCode, 0, `runner should pass: ${r.stderr.slice(-400)}`); const m = /tmp-root=(\S+)/.exec(r.stderr); @@ -159,7 +183,7 @@ describe('#4020 — run-tests temp root', () => { const r = runNode( [RUNNER, '--files', path.basename(target)], - { timeoutMs: 120_000, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } }, + { timeoutMs: RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } }, ); assert.equal(r.exitCode, 0, `nested runner should pass: ${r.stderr.slice(-400)}`); const m = /tmp-root=(\S+)/.exec(r.stderr); @@ -175,7 +199,7 @@ describe('#4020 — run-tests temp root', () => { fs.mkdirSync(sibling); const r2 = runNode( [RUNNER, '--files', path.basename(target)], - { timeoutMs: 120_000, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } }, + { timeoutMs: RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } }, ); assert.equal(r2.exitCode, 0, `second nested runner should pass: ${r2.stderr.slice(-300)}`); assert.ok(fs.existsSync(sibling), diff --git a/tests/shell-command-projection-dispatch.test.cjs b/tests/shell-command-projection-dispatch.test.cjs index f66e4c9c3..0db3f964e 100644 --- a/tests/shell-command-projection-dispatch.test.cjs +++ b/tests/shell-command-projection-dispatch.test.cjs @@ -35,6 +35,23 @@ const { const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); +/** + * NOT a subprocess timeout -- childProcess.spawnSync is fully mocked in + * this test, so no real spawn ever runs. Arbitrary fixture value proving + * execTool forwards its caller-supplied `timeout` option to spawnSync + * verbatim (a pass-through assertion, not a timing assertion). + */ +const EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS = 1234; + +/** + * Deliberately far smaller than any real dispatchGsdCommand invocation + * could complete, forcing a genuine wall-clock timeout so this test can + * assert timedOut:true is reported correctly (mirrors the + * RUN_BASH_SCRIPT_FORCED_TIMEOUT_MS / BOUNDED_SHELL_FORCED_TIMEOUT_MS + * pattern used elsewhere in this migration). + */ +const DISPATCH_FORCED_TIMEOUT_MS = 1; + // ─── execGit ───────────────────────────────────────────────────────────────── describe('execGit', () => { @@ -1114,10 +1131,10 @@ describe('execTool (#3411 windows resolution)', () => { }); t.after(() => mock.restoreAll()); - execTool('some-tool', [], { cwd: '/tmp/x', env: { FOO: 'bar' }, timeout: 1234 }); + execTool('some-tool', [], { cwd: '/tmp/x', env: { FOO: 'bar' }, timeout: EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS }); assert.equal(receivedOptions.cwd, '/tmp/x'); - assert.equal(receivedOptions.timeout, 1234); + assert.equal(receivedOptions.timeout, EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS); assert.equal(receivedOptions.env.FOO, 'bar'); // Case-insensitive: process.env's actual key casing is OS-dependent (Windows // conventionally sets `Path`, not `PATH`), and the merged object here is a @@ -1198,7 +1215,7 @@ describe('dispatchGsdCommand', () => { test('a wall-clock timeout is reported via timedOut:true, ok:false — never throws', () => { assert.doesNotThrow(() => { - const result = dispatchGsdCommand({ family: 'progress', subcommand: 'json', cwd: tmpDir, timeout: 1 }); + const result = dispatchGsdCommand({ family: 'progress', subcommand: 'json', cwd: tmpDir, timeout: DISPATCH_FORCED_TIMEOUT_MS }); assert.equal(result.ok, false); assert.equal(result.timedOut, true); });