feat(#4446): ban ad hoc timeout literals in tests, ship with full legacy allowlist (#4449)

Nothing enforced CONTRIBUTING.md's own stated preference ("A non-literal value
is trusted — that is the shape you should be writing") for timeout values in
tests. local/no-unbounded-spawn requires SOME bound but allows a bare literal;
no-magic-sleep-in-tests and no-elapsed-assertion cover different anti-patterns
entirely. This is exactly how PR #4428's Windows CI incident happened: two
independently-guessed 15000ms literals (one in production's check-latest-
version.cjs, one in this suite's own worker test) collided exactly and raced
two SIGKILLs against each other.

New rule local/no-adhoc-timeout-literal (eslint-rules/no-adhoc-timeout-
literal.cjs) flags a resolvable numeric timeout/timeoutMs literal; an
Identifier or MemberExpression value is trusted. No marker-comment escape —
the fix is always to extract a named constant, which is trivial.

Ships with a full legacy allowlist (126 files, 352 violations, generated
by running the rule with an empty allowlist against tests/) so it can go
live at error severity without breaking CI, mirroring how no-unbounded-spawn
itself was rolled out. Migration is tracked separately in #4445 (this PR
does not close it — only #4446, introducing the gate itself).

Documents the policy and the compliant shapes in TESTING-STANDARDS.md and
TEST-EXAMPLES.md.

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-09-06 21:23:39 -04:00
committed by GitHub
parent ef30e59860
commit f15887ebb1
5 changed files with 412 additions and 0 deletions

View File

@@ -73,6 +73,51 @@ for (const scenario of cases) {
}
```
## Named Timeout Constants, Not Ad Hoc Literals
A bare numeric `timeout`/`timeoutMs` guessed per call site can silently drift from — or worse,
exactly collide with — an unrelated timeout somewhere else. That collision is not hypothetical: a
test's outer subprocess-wait timeout once matched a worker's own inner `npm view` timeout exactly
(both hardcoded to `15000`), so a slow response raced two SIGKILLs at the same instant and lost —
only on Windows CI, only intermittently. See [`TESTING-STANDARDS.md` — "No ad hoc timeout
literals"](TESTING-STANDARDS.md#no-ad-hoc-timeout-literals) for the full incident and
`local/no-adhoc-timeout-literal` for the lint rule that now catches this.
**Non-compliant — a guessed literal with no relationship to what it's actually bounding:**
```javascript
test('worker run leaves a valid cache', (t) => {
const r = runHookSeam(WORKER_PATH, [], { timeoutMs: 15000 }); // why 15000? nobody knows
assert.equal(r.exitCode, 0);
});
```
**Compliant — reuse a shared class-norm constant when the call is the same class of subprocess:**
```javascript
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
test('gsd-tools reports the resolved config', (t) => {
const r = runNode([TOOLS_PATH, 'config', '--json'], { timeoutMs: PROBE_TIMEOUT_MS });
assert.equal(r.exitCode, 0);
});
```
**Compliant — a genuinely distinct class: name it, and size it relative to what it wraps:**
```javascript
const { NPM_VIEW_TIMEOUT_MS } = require('../gsd-core/bin/check-latest-version.cjs');
// Real headroom beyond the inner timeout the worker itself is bounded by — not a
// second independent guess. See TESTING-STANDARDS.md's "No ad hoc timeout literals".
const WORKER_TEARDOWN_MARGIN_MS = 10_000;
test('worker run leaves a valid cache', (t) => {
const r = runHookSeam(WORKER_PATH, [], { timeoutMs: NPM_VIEW_TIMEOUT_MS + WORKER_TEARDOWN_MARGIN_MS });
assert.equal(r.exitCode, 0);
});
```
## Parser Adversarial Fixtures
Parser tests should cover malformed input and real-world file messiness. Prefer named fixtures under `tests/fixtures/adversarial/<type>/` when the input is reusable.