inspector/scripts/verify-test-timeouts.mjs
cliffhall eff0368491 fix: address Copilot review round 2, and drop the asyncUtilTimeout raise
The inline finding was a real bug: main() never called the discovery check, so
the deny-by-default claim rested on test:scripts happening to run the same
comparison — a different command, which `npm run verify:test-timeouts` does not
invoke. `failures` is now seeded from it, and a probe config under clients/ was
observed to fail the guard.

Three suppressed findings, all correct. The afterEach in oauth-interactive
restated the shared hookTimeout and labelled it as a round trip, when it only
stops a server and unlinks a file; it now carries no budget. The file-lock
constants were named for the opposite of what they bound — 90s covers retries
running out against a holder that never yields, 60s covers the stale-lock
takeover that succeeds — so they are LOCK_RETRIES_EXHAUSTED_MS and
STALE_LOCK_TAKEOVER_MS. The browser-timeouts header said 15 budgets; it is 19.

Separately, and more importantly: the asyncUtilTimeout raise is reverted, on
measurement. Verifying round 2 turned up intermittent 15s timeouts across
unrelated unit files, so three arms of three full unit runs were taken in this
worktree at comparable load — unmodified v2/main 3/3 green, this change with
5000 producing 1, 4 and 0 timing-out files, and the same change with that one
line reverted 3/3 green. A wait that is MEANT to expire spends its whole budget
on the happy path, so a 5x raise is a 5x cost on exactly those tests and
saturates the worker pool, starving tests that were never slow.

Both web setup files therefore state asyncUtilTimeout at the library's own
value rather than raising it, which is what #2323's rule actually asks for: a
budget nobody chose is the defect, not a budget that is small. The tests that
assert absence by letting a wait expire are the real problem and are #2335.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
2026-09-12 11:49:30 -04:00

444 lines
16 KiB
JavaScript

/**
* Guards the wall-clock budgets every Vitest project in this repo runs under
* (#2323), and the two decisions that came with them.
*
* The class it encodes against: a budget nobody chose. Three of the six
* projects ran on Vitest's own `testTimeout: 5000` and five on its
* `hookTimeout` / `teardownTimeout: 10000` — values sized for an idle machine,
* not for the one this team works on (three or four concurrent agent sessions
* in separate worktrees, each free to run the full `npm run local:gate`, on
* eight logical cores). A correct, deterministic test cut off mid-flight by
* such a budget fails a gate its diff did not break, which trains people to
* re-run rather than read — the same argument AGENTS.md's "Lint has no warning
* tier" makes from the other direction. #2292, #2278, #2250, #1942 and #1742
* were each that class, found and fixed one site at a time.
*
* Three properties, which is what makes this worth a guard rather than a
* comment:
*
* 1. **Resolved, not declared.** It asks Vitest to resolve each project and
* reads the number a test actually gets. Asserting that a key is absent from
* some config block would pass just as happily on a config that had stopped
* being loaded at all.
* 2. **Unknown projects fail loudly.** A seventh project added without a row
* here is exactly the drift this exists to prevent, so it is an error rather
* than something the table silently skips.
* 3. **`retry` must stay unset.** A retry converts a load-induced red into a
* silent green on the only pre-push gate this repo has, and would re-open
* #1596 by hiding a real race behind a second attempt. That was a deliberate
* "do not raise" decision, so it is enforced rather than remembered.
*
* It also asserts the Testing Library half, which no Vitest config can see:
* `asyncUtilTimeout` governs every `waitFor` / `findBy*` in the web projects
* and is the binding constraint on an async assertion, since it is tighter than
* any per-test budget here. What is enforced is that each web project *states*
* it — raising it was measured and rejected, for the reason recorded in
* `clients/web/src/test/setup.ts`.
*
* ⚠️ Observed to FAIL against the unfixed config before it was trusted: on
* `origin/v2/main` it reports `unit`, `tui` and `launcher` at
* `testTimeout: 5000`, five of the six projects at `hookTimeout: 10000`, and
* both `asyncUtilTimeout` assertions unmet.
*/
import { existsSync, readdirSync, readFileSync } from "node:fs";
import { dirname, join, relative, resolve } from "node:path";
import { fileURLToPath } from "node:url";
const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), "..");
/**
* The budgets, restated here rather than imported from `vitest.shared.mts`.
*
* That duplication is the point: importing the same object the configs spread
* in would make this guard agree with any value at all, including a default
* someone reinstated by deleting the spread. A guard has to state the
* expectation independently or it is only asserting that a file parses.
*/
export const EXPECTED_TIMEOUTS = Object.freeze({
testTimeout: 15_000,
hookTimeout: 30_000,
teardownTimeout: 30_000,
});
/** Web's `integration` project pays for real servers, sockets and OAuth flows. */
export const EXPECTED_INTEGRATION_TIMEOUTS = Object.freeze({
...EXPECTED_TIMEOUTS,
testTimeout: 30_000,
});
/**
* Every Vitest project in the repo, keyed by the resolved project name. Vitest
* appends the browser instance to a browser project's name, so `storybook`
* resolves as `storybook (chromium)`; match on the leading segment.
*/
export const EXPECTED_PROJECTS = Object.freeze({
unit: EXPECTED_TIMEOUTS,
integration: EXPECTED_INTEGRATION_TIMEOUTS,
storybook: EXPECTED_TIMEOUTS,
cli: EXPECTED_TIMEOUTS,
tui: EXPECTED_TIMEOUTS,
launcher: EXPECTED_TIMEOUTS,
});
/**
* The four Vitest configs to resolve, and the project names each must yield.
*
* The node clients name no project, so their single project resolves with an
* empty name — `projects` supplies the name this guard checks it under. Web's
* three name themselves, so its entry lists them and the resolver matches by
* name instead of by position.
*/
export const CONFIG_ROOTS = Object.freeze([
{ root: "clients/web", projects: ["unit", "integration", "storybook"] },
{ root: "clients/cli", projects: ["cli"] },
{ root: "clients/tui", projects: ["tui"] },
{ root: "clients/launcher", projects: ["launcher"] },
]);
/**
* Files that must configure Testing Library's `asyncUtilTimeout`, with the
* import each has to configure it through. Storybook instruments its own copy
* of Testing Library so its interactions panel can trace each step, so
* configuring `@testing-library/*` there would configure a copy no play
* function calls.
*/
export const ASYNC_UTIL_SITES = Object.freeze([
{
file: "clients/web/src/test/setup.ts",
from: "@testing-library/react",
project: "unit",
},
{
file: "clients/web/src/test/storybookSetup.ts",
from: "storybook/test",
project: "storybook",
},
]);
/**
* The value those sites must configure — stated here for the same reason the
* Vitest budgets are: a guard that accepts any number at all would pass on
* `asyncUtilTimeout: 1` (Copilot).
*
* It is Testing Library's own default, and deliberately so: what this guard
* enforces is that the value is *stated*, not that it is large. Raising it was
* measured and rejected — see the long comment in
* `clients/web/src/test/setup.ts`.
*/
export const EXPECTED_ASYNC_UTIL_TIMEOUT = 1_000;
/**
* Directory every Vitest config in this repo lives one level under. Discovery
* (below) walks it rather than trusting a hand-written list.
*/
export const CLIENTS_DIR = "clients";
/**
* Compare one resolved project against its row.
*
* @param {string} name project name as this guard knows it
* @param {{testTimeout?: unknown, hookTimeout?: unknown, teardownTimeout?: unknown, retry?: unknown}} config
* @param {Record<string, Readonly<Record<string, number>>>} [expected]
* @returns {string[]} one message per violation; empty when the project is fine
*/
export function checkProject(name, config, expected = EXPECTED_PROJECTS) {
const row = expected[name];
if (!row) {
return [
`project "${name}" has no row in EXPECTED_PROJECTS — a project whose budgets ` +
`nobody stated is exactly the drift this guard exists to prevent. Add it.`,
];
}
const failures = [];
for (const [key, want] of Object.entries(row)) {
const got = config[key];
if (got !== want) {
failures.push(
`project "${name}" resolves ${key} to ${String(got)}, expected ${want}`,
);
}
}
// Vitest leaves `retry` undefined when nothing sets it; 0 is the same
// decision written out.
const retry = config.retry;
if (retry !== undefined && retry !== 0) {
failures.push(
`project "${name}" sets retry to ${String(retry)} — a retry turns a ` +
`load-induced red into a silent green on the only pre-push gate here (#1596)`,
);
}
return failures;
}
/**
* Strip `//` and block comments so a commented-out call cannot satisfy a check.
*
* Comment-aware rather than exact, deliberately: a naive scan of the raw source
* would accept a `// configure({ asyncUtilTimeout: 1000 })` left behind by
* someone disabling it, which is the most likely way this stops being
* configured (Copilot). String literals are not parsed out — a `configure(` in
* a string would still be accepted — but these two files are fifteen lines each
* and the false-positive that matters is the commented one.
*
* @param {string} source
* @returns {string}
*/
export function stripComments(source) {
return source
.replace(/\/\*[\s\S]*?\*\//g, "")
.replace(/(^|\n)\s*\/\/[^\n]*/g, "$1");
}
/**
* Does this source configure `asyncUtilTimeout` at the expected value, through
* the right import?
*
* Deliberately a source check and not a runtime one: the value only exists
* inside a running test environment, and a `configure` call reached by no
* project would satisfy a runtime probe of this module just as well. What
* closes that second gap is `checkSetupFilesWiring` below, which asks the
* resolved project whether it actually loads the file.
*
* @param {string} source
* @param {string} from module the `configure` must come from
* @param {number} [expected] the value it must configure
* @returns {string[]}
*/
export function checkAsyncUtilSource(
source,
from,
expected = EXPECTED_ASYNC_UTIL_TIMEOUT,
) {
const code = stripComments(source);
const failures = [];
if (!new RegExp(`from\\s+["']${from.replace(/\//g, "\\/")}["']`).test(code)) {
failures.push(`does not import from "${from}"`);
}
if (!/\bconfigure\s*\(/.test(code)) {
failures.push("does not call configure()");
return failures;
}
const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(code);
if (!match) {
failures.push("calls configure() without an asyncUtilTimeout");
} else if (Number(match[1].replace(/_/g, "")) !== expected) {
failures.push(
`configures asyncUtilTimeout as ${match[1]}, expected ${expected}`,
);
}
return failures;
}
/**
* Is each declared site actually loaded by the project it claims to configure?
*
* Without this the guard reads two files and reports both projects configured
* while `setupFiles` had been deleted from `vite.config.ts` and Testing Library
* had silently gone back to its 1000ms default (Copilot). Vitest resolves
* `setupFiles` to absolute paths, so compare by suffix against the declared
* repo-relative path.
*
* @param {string} name project name as this guard knows it
* @param {unknown} setupFiles the project's resolved `setupFiles`
* @param {readonly {file: string, project: string}[]} [sites]
* @returns {string[]}
*/
export function checkSetupFilesWiring(
name,
setupFiles,
sites = ASYNC_UTIL_SITES,
) {
const site = sites.find((s) => s.project === name);
if (!site) return [];
const loaded = Array.isArray(setupFiles) ? setupFiles : [];
const wanted = site.file.split("/").join("/");
const found = loaded.some(
(f) => typeof f === "string" && f.split("\\").join("/").endsWith(wanted),
);
return found
? []
: [
`project "${name}" does not load ${site.file} as a setupFile, so its ` +
`asyncUtilTimeout never takes effect`,
];
}
/**
* Every Vitest config under `clients/`, as repo-relative directories.
*
* Discovery rather than the hand-written `CONFIG_ROOTS` list, so that a new
* client with its own config cannot go unwatched — which would have made the
* "a project with no row is an error" promise vacuous, since an undiscovered
* config yields no project to reject (Copilot). `CONFIG_ROOTS` still exists to
* say which project names each config must produce; this is what proves the
* list is complete.
*
* @param {string} [root] absolute repo root
* @returns {string[]}
*/
export function discoverConfigRoots(root = repoRoot) {
const clients = resolve(root, CLIENTS_DIR);
if (!existsSync(clients)) return [];
return readdirSync(clients, { withFileTypes: true })
.filter((e) => e.isDirectory())
.map((e) => join(clients, e.name))
.filter((dir) =>
["vitest.config.ts", "vite.config.ts"].some((f) =>
existsSync(join(dir, f)),
),
)
.map((dir) => relative(root, dir).split("\\").join("/"))
.sort();
}
/**
* Compare what is on disk against what this guard is configured to check.
*
* @param {string[]} discovered
* @param {readonly {root: string}[]} [configured]
* @returns {string[]}
*/
export function checkConfigRootCoverage(discovered, configured = CONFIG_ROOTS) {
const known = new Set(configured.map((c) => c.root));
const failures = [];
for (const root of discovered) {
if (!known.has(root)) {
failures.push(
`${root} has a Vitest config that this guard does not check — add it to ` +
`CONFIG_ROOTS and EXPECTED_PROJECTS rather than leaving its budgets unstated`,
);
}
}
for (const root of known) {
if (!discovered.includes(root)) {
failures.push(
`CONFIG_ROOTS names ${root}, which has no Vitest config on disk — a stale ` +
`row silently stops checking anything`,
);
}
}
return failures;
}
/**
* Resolve every project of one config, via Vitest's own resolver.
*
* @param {string} root absolute directory holding the `vitest.config.ts` or `vite.config.ts`
* @returns {Promise<{name: string, config: Record<string, unknown>}[]>}
*/
async function resolveProjects(root) {
const { createVitest } = await import("vitest/node");
const config = ["vitest.config.ts", "vite.config.ts"]
.map((f) => join(root, f))
.find((f) => existsSync(f));
if (!config) {
throw new Error(`no vitest/vite config under ${root}`);
}
const vitest = await createVitest("test", {
watch: false,
run: true,
root,
config,
});
try {
return vitest.projects.map((p) => ({
name: p.name,
config: /** @type {Record<string, unknown>} */ (p.config),
}));
} finally {
await vitest.close();
}
}
/**
* Match a resolved project to the name this guard knows it by.
*
* A node client's single project resolves nameless, so its config entry
* supplies the name. Web's three name themselves, but a browser project's name
* carries its instance (`storybook (chromium)`), so compare the leading
* segment rather than the whole string.
*
* @param {{name: string}} project
* @param {string[]} expectedNames
* @returns {string | undefined}
*/
export function identifyProject(project, expectedNames) {
if (expectedNames.length === 1 && !project.name) return expectedNames[0];
const base = project.name.replace(/\s*\(.*\)$/, "");
return expectedNames.find((n) => n === base);
}
async function main() {
// Seeded from discovery, not empty: the unknown-project check below can only
// reject a project this guard actually resolves, so a config it never opens
// is invisible to it. Without this line the deny-by-default claim rested on
// `test:scripts` happening to run the same comparison — a different command,
// which a standalone `npm run verify:test-timeouts` does not invoke (Copilot).
const failures = checkConfigRootCoverage(discoverConfigRoots());
let checked = 0;
for (const { root, projects: expectedNames } of CONFIG_ROOTS) {
const resolved = await resolveProjects(resolve(repoRoot, root));
const seen = new Set();
for (const project of resolved) {
const name = identifyProject(project, expectedNames);
if (!name) {
failures.push(
`${root}: resolved an unexpected project "${project.name}" — add it to ` +
`CONFIG_ROOTS and EXPECTED_PROJECTS rather than leaving its budgets unstated`,
);
continue;
}
seen.add(name);
checked += 1;
failures.push(
...checkProject(name, project.config).map((f) => `${root}: ${f}`),
...checkSetupFilesWiring(name, project.config.setupFiles).map(
(f) => `${root}: ${f}`,
),
);
}
for (const name of expectedNames) {
if (!seen.has(name)) {
failures.push(`${root}: project "${name}" did not resolve at all`);
}
}
}
for (const { file, from, project } of ASYNC_UTIL_SITES) {
const abs = resolve(repoRoot, file);
if (!existsSync(abs)) {
failures.push(
`${file} is missing — the "${project}" project has no asyncUtilTimeout`,
);
continue;
}
failures.push(
...checkAsyncUtilSource(readFileSync(abs, "utf-8"), from).map(
(f) => `${file} (${project} project) ${f}`,
),
);
}
if (failures.length > 0) {
console.error("verify:test-timeouts FAILED\n");
for (const f of failures) console.error(` - ${f}`);
console.error(
"\nEvery test-gate budget must be a value someone chose, sized for a machine\n" +
"running three or four concurrent worktree gates (#2323). The shared values live\n" +
"in `vitest.shared.mts` (TIMEOUTS / INTEGRATION_TIMEOUTS) and every project\n" +
"spreads one of them; Testing Library's own asyncUtilTimeout is configured in\n" +
"each web project's setup file. Raising a budget is a decision to state there,\n" +
"not a per-suite argument to add — and `retry` stays unset.",
);
process.exit(1);
}
console.log(
`verify:test-timeouts OK — ${checked} Vitest projects on stated budgets, ` +
`no retry, ${ASYNC_UTIL_SITES.length} asyncUtilTimeout sites configured.`,
);
}
if (process.argv[1] === fileURLToPath(import.meta.url)) main();