diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f2f4c0f5..96efe0c1 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -38,8 +38,9 @@ jobs: run: npm install - name: Validate (coverage guards, format, lint, typecheck, build, fast tests) - # Runs the four durable guards first (verify:format-coverage, - # verify:skills, verify:typecheck-coverage, verify:dep-lockstep), then + # Runs the five durable guards first (verify:format-coverage, + # verify:skills, verify:typecheck-coverage, verify:dep-lockstep, + # verify:test-timeouts), then # test:scripts, then validate:core, then each client's # self-validation: format:check + lint + typecheck + build + test (no # coverage instrumentation — fast). This also builds every client bundle diff --git a/AGENTS.md b/AGENTS.md index b645dff6..eb0b13ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -431,7 +431,10 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. - **`retry` stays unset.** A retry turns a load-induced red into a silent green on the only pre-push gate this repo has, and hides a real race behind a second attempt. The guard enforces this. -- **Group F's smoke and CI budgets are tracked on #2333**, not settled here. +- **The Playwright locator budgets the web smokes use are named constants in + `scripts/lib/browser-timeouts.mjs`** — raise one there, not at a call site. + The *remaining* smoke-helper budgets and CI's missing `timeout-minutes` are + tracked on #2333 rather than settled here. - **Do not scale a fixed sleep.** A `setTimeout(r, N)` with no condition is not a timeout: it always waits the full window, so raising it slows every passing run and still races on a loaded one. Replace one with a condition wait when it diff --git a/clients/cli/__tests__/e2e.test.ts b/clients/cli/__tests__/e2e.test.ts index b6273ae0..e12932c8 100644 --- a/clients/cli/__tests__/e2e.test.ts +++ b/clients/cli/__tests__/e2e.test.ts @@ -8,15 +8,25 @@ const here = dirname(fileURLToPath(import.meta.url)); const BIN = resolve(here, "../build/index.js"); /** - * In-test bound on the spawned child, deliberately BELOW the cli project's own - * `testTimeout` (15000, from `vitest.shared.mts`). The two used to be equal, so - * which one fired was arbitrary — and they do different things: this one kills - * the child's process group and rejects with a message naming the CLI, while - * the project budget just fails the test with a generic timeout and leaves a - * detached `node` behind. The useful diagnostic is here, so this is the bound - * that must win (#2323). + * The two bounds on a spawned child, which used to be the identical 15000 — + * so which one fired was arbitrary, and they do different things (#2323). + * + * `CHILD_TIMEOUT_MS` kills the child's process group and rejects with a message + * naming the CLI. The enclosing per-test budget just fails the test generically + * and leaves a detached `node` behind. The useful diagnostic is the child + * timer's, so it has to be the bound that wins — and the way to arrange that is + * to raise the test's budget above it, not to shrink the child's. Shrinking it + * would take load tolerance away from the one suite here that spawns a real + * process, which is the opposite of this change's purpose (Copilot). + * + * `E2E_SPAWN_MS` is therefore an exception to the cli project's shared 15000, + * and one of the few sites that should be: booting the built binary in a fresh + * `node` and waiting for it to speak to an MCP server is elapsed work, not + * slack. It is the child's own budget plus room for the spawn and the reject to + * be observed. */ -const CHILD_TIMEOUT_MS = 12_000; +const CHILD_TIMEOUT_MS = 15_000; +const E2E_SPAWN_MS = 25_000; interface SpawnResult { exitCode: number | null; @@ -64,18 +74,31 @@ function spawnCli(args: string[]): Promise { describe("CLI binary (out-of-process E2E)", () => { const { command, args } = getTestMcpServerCommand(); - it("exits 0 and prints tools JSON on a successful run", async () => { - const result = await spawnCli([command, ...args, "--method", "tools/list"]); + it( + "exits 0 and prints tools JSON on a successful run", + async () => { + const result = await spawnCli([ + command, + ...args, + "--method", + "tools/list", + ]); - expect(result.exitCode).toBe(0); - const json = JSON.parse(result.stdout); - expect(Array.isArray(json.tools)).toBe(true); - }); + expect(result.exitCode).toBe(0); + const json = JSON.parse(result.stdout); + expect(Array.isArray(json.tools)).toBe(true); + }, + E2E_SPAWN_MS, + ); - it("exits non-zero when required --method is missing", async () => { - const result = await spawnCli([command, ...args]); + it( + "exits non-zero when required --method is missing", + async () => { + const result = await spawnCli([command, ...args]); - expect(result.exitCode).not.toBe(0); - expect(result.stderr).toContain("Method is required"); - }); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain("Method is required"); + }, + E2E_SPAWN_MS, + ); }); diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 40f54d91..64bb3cfc 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -4295,58 +4295,67 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)", ); } - // Three full modal interaction sequences against the whole App tree. This - // used to carry `{ timeout: 20000 }` because the project ran on Vitest's 5s - // default and this test tripped it under load; the project now states 15000 - // itself (#2323), so the raise has nothing left to say that the shared value - // does not. - it("suppresses a second clear for another entry with the same URL, and allows one after it settles", async () => { - // `delay: null` drops userEvent's inter-event waits, which dominate here. - const user = userEvent.setup({ delay: null }); - renderWithMantine(); + // Three full modal interaction sequences against the whole App tree — three + // times the work of any other test in this file, and the only one here that + // ever tripped the old 5s default under load. It keeps its own budget after + // #2323 rather than folding into the shared 15000: this is the "genuinely + // different work" exception that change establishes, and the 20000 was a + // value somebody had already chosen against an observed failure. Lowering it + // to the shared ceiling would trade a measured number for a general one and + // could reintroduce the flake (Copilot). + const THREE_MODAL_SEQUENCE_MS = 20_000; - await user.click(screen.getByText("connect")); - await waitFor(() => expect(clientInstances).toHaveLength(1)); - // The instances are typed `EventTarget`; an intersection names the - // test-only spy without erasing that (Copilot). - const client = clientInstances[0] as EventTarget & { - clearOAuthTokens: ReturnType; - }; + it( + "suppresses a second clear for another entry with the same URL, and allows one after it settles", + async () => { + // `delay: null` drops userEvent's inter-event waits, which dominate here. + const user = userEvent.setup({ delay: null }); + renderWithMantine(); - // Hold the first clear open, as a pending RFC 7009 request would. - let settle: (v: { status: string; reason: string }) => void = () => {}; - client.clearOAuthTokens.mockImplementation( - () => - new Promise((resolve) => { - settle = resolve as typeof settle; - }), - ); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + // The instances are typed `EventTarget`; an intersection names the + // test-only spy without erasing that (Copilot). + const client = clientInstances[0] as EventTarget & { + clearOAuthTokens: ReturnType; + }; - await clearFromSettings(user, "open-settings"); - await waitFor(() => - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1), - ); + // Hold the first clear open, as a pending RFC 7009 request would. + let settle: (v: { status: string; reason: string }) => void = () => {}; + client.clearOAuthTokens.mockImplementation( + () => + new Promise((resolve) => { + settle = resolve as typeof settle; + }), + ); - // Entry B: a different catalog id, the same OAuth storage key. Both clears - // route through the live client precisely because they share that key, so - // an id-keyed guard would let this second one straight through. - await clearFromSettings(user, "open-settings-b"); - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1); + await clearFromSettings(user, "open-settings"); + await waitFor(() => + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1), + ); - // Suppression is for the duration of the in-flight clear only — the key - // must be released when it settles, or the control is dead for the rest of - // the session. - await act(async () => { - settle({ status: "skipped", reason: "no_endpoint" }); - await Promise.resolve(); - }); - client.clearOAuthTokens.mockResolvedValue({ - status: "skipped", - reason: "no_endpoint", - }); - await clearFromSettings(user, "open-settings-b"); - await waitFor(() => - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(2), - ); - }); + // Entry B: a different catalog id, the same OAuth storage key. Both clears + // route through the live client precisely because they share that key, so + // an id-keyed guard would let this second one straight through. + await clearFromSettings(user, "open-settings-b"); + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1); + + // Suppression is for the duration of the in-flight clear only — the key + // must be released when it settles, or the control is dead for the rest of + // the session. + await act(async () => { + settle({ status: "skipped", reason: "no_endpoint" }); + await Promise.resolve(); + }); + client.clearOAuthTokens.mockResolvedValue({ + status: "skipped", + reason: "no_endpoint", + }); + await clearFromSettings(user, "open-settings-b"); + await waitFor(() => + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(2), + ); + }, + THREE_MODAL_SEQUENCE_MS, + ); }); diff --git a/clients/web/src/test/PreviewAnnotations.stories.tsx b/clients/web/src/test/PreviewAnnotations.stories.tsx index c05b0ae4..14e01cc0 100644 --- a/clients/web/src/test/PreviewAnnotations.stories.tsx +++ b/clients/web/src/test/PreviewAnnotations.stories.tsx @@ -8,8 +8,15 @@ import { theme } from "../theme/theme"; // `.storybook/vitest.setup.ts` used to call `setProjectAnnotations([...])` to // hand the preview annotations to the Storybook vitest project. Since Storybook // 10.3 `@storybook/addon-vitest` applies them automatically — and *skips* doing -// so when it finds such a setup file — so the file was removed and the -// `setupFiles` entry dropped from the `storybook` project in `vite.config.ts`. +// so when it finds a setup file that is both inside `configDir` and calls +// `setProjectAnnotations` — so that file was removed. +// +// The project does have a `setupFiles` entry again (#2323), and it is a +// different thing: `src/test/storybookSetup.ts` configures Testing Library's +// `asyncUtilTimeout` and nothing else. It satisfies *neither* half of the +// condition above — it lives outside `.storybook/` and names no annotations — +// so the automatic path is still the one in use, and this guard still asserts +// that it worked. // // A green suite is not evidence that the automatic path works. Two things the // setup file used to provision are silently losable: diff --git a/scripts/lib/browser-timeouts.test.mjs b/scripts/lib/browser-timeouts.test.mjs index f646e779..bdab308b 100644 --- a/scripts/lib/browser-timeouts.test.mjs +++ b/scripts/lib/browser-timeouts.test.mjs @@ -1,3 +1,16 @@ +/** + * Pins the invariants of `browser-timeouts.mjs` — a constants module, so what + * is testable about it is the shape of the values rather than any logic. + * + * Worth pinning anyway, because the values are the module's whole contract and + * a smoke cannot report a bad one. A budget that arrived as `undefined` or a + * string reaches Playwright as "no timeout" or NaN and a smoke then hangs + * against the job's own ceiling rather than failing with a cause. And the + * ordering below IS the semantics documented on each key: the names say what + * each budget has to absorb, so a `nested` wait outliving its enclosing + * `roundTrip` one would mean the names had stopped describing the values. + */ + import test from "node:test"; import assert from "node:assert/strict"; diff --git a/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index b4ed7e53..6fab43b4 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -39,8 +39,8 @@ * both `asyncUtilTimeout` assertions unmet. */ -import { existsSync, readFileSync } from "node:fs"; -import { dirname, join, resolve } from "node:path"; +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)), ".."); @@ -114,6 +114,19 @@ export const ASYNC_UTIL_SITES = Object.freeze([ }, ]); +/** + * 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). + */ +export const EXPECTED_ASYNC_UTIL_TIMEOUT = 5_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. * @@ -152,25 +165,151 @@ export function checkProject(name, config, expected = EXPECTED_PROJECTS) { } /** - * Does this source configure `asyncUtilTimeout`, through the right import? + * 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: 5000 })` 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. + * 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) { +export function checkAsyncUtilSource( + source, + from, + expected = EXPECTED_ASYNC_UTIL_TIMEOUT, +) { + const code = stripComments(source); const failures = []; - if (!new RegExp(`from\\s+["']${from.replace("/", "\\/")}["']`).test(source)) { + if (!new RegExp(`from\\s+["']${from.replace(/\//g, "\\/")}["']`).test(code)) { failures.push(`does not import from "${from}"`); } - if (!/\bconfigure\s*\(/.test(source)) { + if (!/\bconfigure\s*\(/.test(code)) { failures.push("does not call configure()"); - } else if (!/asyncUtilTimeout\s*:\s*\d+/.test(source)) { + 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; } @@ -243,6 +382,9 @@ async function main() { 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) { diff --git a/scripts/verify-test-timeouts.test.mjs b/scripts/verify-test-timeouts.test.mjs index e9fffede..b5cc6cb2 100644 --- a/scripts/verify-test-timeouts.test.mjs +++ b/scripts/verify-test-timeouts.test.mjs @@ -18,12 +18,17 @@ import assert from "node:assert/strict"; import { ASYNC_UTIL_SITES, CONFIG_ROOTS, + EXPECTED_ASYNC_UTIL_TIMEOUT, EXPECTED_INTEGRATION_TIMEOUTS, EXPECTED_PROJECTS, EXPECTED_TIMEOUTS, checkAsyncUtilSource, + checkConfigRootCoverage, checkProject, + checkSetupFilesWiring, + discoverConfigRoots, identifyProject, + stripComments, } from "./verify-test-timeouts.mjs"; const ok = { ...EXPECTED_TIMEOUTS }; @@ -148,3 +153,95 @@ test("both web projects have an asyncUtilTimeout site", () => { "unit", ]); }); + +test("a commented-out configure() does not satisfy the check", () => { + // The most likely way this stops being configured is someone disabling it in + // place, and a raw-source scan would read that as configured (Copilot). + const lineComment = `import { configure } from "storybook/test"; +// configure({ asyncUtilTimeout: 5000 });`; + assert.deepEqual(checkAsyncUtilSource(lineComment, "storybook/test"), [ + "does not call configure()", + ]); + + const blockComment = `import { configure } from "storybook/test"; +/* configure({ asyncUtilTimeout: 5000 }); */`; + assert.deepEqual(checkAsyncUtilSource(blockComment, "storybook/test"), [ + "does not call configure()", + ]); +}); + +test("the configured value must be the expected one, not merely present", () => { + const source = `import { configure } from "storybook/test"; +configure({ asyncUtilTimeout: 1 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [ + `configures asyncUtilTimeout as 1, expected ${EXPECTED_ASYNC_UTIL_TIMEOUT}`, + ]); +}); + +test("a numeric separator in the configured value is read, not rejected", () => { + const source = `import { configure } from "storybook/test"; +configure({ asyncUtilTimeout: 5_000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); +}); + +test("stripComments leaves executable code alone", () => { + const source = `/** header */ +import { configure } from "storybook/test"; // trailing +configure({ asyncUtilTimeout: 5000 });`; + const code = stripComments(source); + assert.match(code, /configure\(\{ asyncUtilTimeout: 5000 \}\)/); + assert.doesNotMatch(code, /header/); +}); + +test("a project that does not load its declared setup file is caught", () => { + // Reading the file proves it configures the timeout; only this proves the + // project ever loads it. Deleting the setupFiles entry used to leave the + // guard reporting both projects configured (Copilot). + const failures = checkSetupFilesWiring("unit", []); + assert.equal(failures.length, 1); + assert.match(failures[0], /does not load .*setup\.ts as a setupFile/); +}); + +test("a setup file loaded under an absolute path is recognized", () => { + assert.deepEqual( + checkSetupFilesWiring("unit", [ + "/repo/clients/web/src/test/setup.ts", + "/repo/other.ts", + ]), + [], + ); +}); + +test("a project with no declared site is not required to load one", () => { + assert.deepEqual(checkSetupFilesWiring("cli", []), []); + assert.deepEqual(checkSetupFilesWiring("cli", undefined), []); +}); + +test("a Vitest config this guard does not check is an error", () => { + const failures = checkConfigRootCoverage([ + "clients/web", + "clients/cli", + "clients/tui", + "clients/launcher", + "clients/desktop", + ]); + assert.equal(failures.length, 1); + assert.match(failures[0], /clients\/desktop has a Vitest config/); +}); + +test("a stale row naming a config that no longer exists is an error", () => { + const failures = checkConfigRootCoverage(["clients/web", "clients/cli"]); + assert.equal(failures.length, 2); + for (const f of failures) assert.match(f, /has no Vitest config on disk/); +}); + +test("discovery finds exactly the configs this guard is set up to check", () => { + // The two halves have to agree against the real repo, not only against + // fixtures — that agreement is what makes the unknown-project check below + // mean anything. + assert.deepEqual( + discoverConfigRoots().sort(), + CONFIG_ROOTS.map((c) => c.root).sort(), + ); + assert.deepEqual(checkConfigRootCoverage(discoverConfigRoots()), []); +});