From 58e53d5a9230e5c5e0a1d1e1e3d59a0b08d7f456 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 08:32:34 -0400 Subject: [PATCH] fix: state every test-gate timeout instead of inheriting a library default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the six Vitest projects ran on Vitest's own testTimeout: 5000 and five on its hookTimeout/teardownTimeout: 10000, and all 788 Testing Library waits in the web unit project ran on asyncUtilTimeout: 1000. None of those numbers was chosen here, and all of them were sized for an idle machine rather than for three or four concurrent worktree sessions each running a full local:gate on eight cores. #2292, #2278, #2250, #1942 and #1742 were each that class, diagnosed one site at a time. This is the complement of #1596, not a retreat from it: every fix that issue produced stands, and no race is hidden here. What changes is that a correct, deterministic test is no longer cut off by a budget nobody picked. - vitest.shared.mts exports frozen TIMEOUTS / INTEGRATION_TIMEOUTS; all six projects spread one of them, clients/launcher importing the shared module for the first time to do so. - Both web projects configure Testing Library's asyncUtilTimeout. The storybook one needs its own setup file, deliberately outside .storybook/ and free of setProjectAnnotations so the automatic preview-annotation provisioning (#1898) is untouched. - 28 per-suite restatements of the integration project's own budget are gone, so raising the shared value now moves every site under it. The handful that genuinely differ — cross-process lock contention, a full OAuth round trip — keep their budget as a named constant that says why. - AppRenderer's waitFor no longer asks for exactly its enclosing budget, which it could never win (#2292's shape); cli's e2e child timer drops below the project budget so the bound carrying the diagnostic is the one that fires. - The transition auto-settle's slack term is named RAF_SLACK_MS and raised; the term derived from HEADER_ANIM_MS is untouched. The two answer to different things and must not be scaled as one number. - The 19 Playwright budgets scattered across three web smokes become four named constants in scripts/lib/browser-timeouts.mjs. scripts/verify-test-timeouts.mjs resolves each project through Vitest itself and asserts the budgets, the asyncUtilTimeout sites, and that retry stays unset. It was observed red against the pre-change config: unit/tui/launcher at testTimeout 5000, five projects at hookTimeout 10000, both asyncUtilTimeout sites unmet. Closes #2323 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- .claude/skills/pre-push-gate/SKILL.md | 16 + AGENTS.md | 44 +++ clients/cli/__tests__/e2e.test.ts | 13 +- .../cli/__tests__/oauth-interactive.test.ts | 282 +++++++++-------- clients/cli/vitest.config.ts | 6 +- clients/launcher/tsconfig.test.json | 10 +- clients/launcher/vitest.config.ts | 7 + clients/tui/__tests__/ResourcesTab.test.tsx | 16 +- clients/tui/__tests__/logger.test.ts | 13 +- clients/tui/vitest.config.ts | 6 +- clients/web/src/App.test.tsx | 99 +++--- .../elements/AppRenderer/AppRenderer.test.tsx | 20 +- .../groups/ViewHeader/ViewHeader.test.tsx | 14 +- .../integration/auth/node/file-lock.test.ts | 267 +++++++++------- .../auth/node/secret-store-selection.test.ts | 4 + .../integration/auth/revocation-e2e.test.ts | 4 +- .../auth/rfc8414AtOidcPath.test.ts | 4 +- .../inspectorClient-modern-era-oauth.test.ts | 6 +- .../mcp/inspectorClient-modern-era.test.ts | 4 +- ...lient-oauth-direct-mid-session-e2e.test.ts | 6 +- ...lient-oauth-remote-mid-session-e2e.test.ts | 12 +- .../oauth-resource-metadata-challenge.test.ts | 8 +- .../remote/cancel-per-request-stream.test.ts | 2 +- clients/web/src/test/renderWithMantine.tsx | 54 +++- clients/web/src/test/setup.ts | 21 +- clients/web/src/test/storybookSetup.ts | 30 ++ clients/web/vite.config.ts | 43 ++- docs/quality-gate.md | 3 +- package.json | 3 +- scripts/lib/browser-timeouts.mjs | 49 +++ scripts/lib/browser-timeouts.test.mjs | 27 ++ scripts/smoke-web-browser.mjs | 7 +- scripts/smoke-web-elicitation.mjs | 28 +- scripts/smoke-web-tabs.mjs | 17 +- scripts/verify-test-timeouts.mjs | 290 ++++++++++++++++++ scripts/verify-test-timeouts.test.mjs | 150 +++++++++ vitest.shared.mts | 67 +++- 37 files changed, 1267 insertions(+), 385 deletions(-) create mode 100644 clients/web/src/test/storybookSetup.ts create mode 100644 scripts/lib/browser-timeouts.mjs create mode 100644 scripts/lib/browser-timeouts.test.mjs create mode 100644 scripts/verify-test-timeouts.mjs create mode 100644 scripts/verify-test-timeouts.test.mjs diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 1b3379fe..66db5b7c 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -87,6 +87,22 @@ versions. **Align the versions** — bump it in every install that declares it. Do not raise the heap with `--max-old-space-size`; that hides the class rather than fixing it. +### `verify:test-timeouts` + +A Vitest project resolves to a wall-clock budget nobody stated, a web setup +file stopped configuring Testing Library's `asyncUtilTimeout`, or someone added +a `retry`. The shared values live in `vitest.shared.mts` (`TIMEOUTS` / +`INTEGRATION_TIMEOUTS`) and every project spreads one of them — so **raise a +budget there**, not with a per-suite `}, 30_000)` argument, which only moves the +one site and leaves every future file on the default. A per-suite raise is +right only where the work is genuinely different (real cross-process lock +contention, a full OAuth round trip); say so at the site. `retry` stays unset: +it turns a load-induced red into a silent green on the only pre-push gate here. + +A **failing test** is a different problem from a budget — read the failure +before reaching for a number. An assertion that races is #1596's class and is +fixed with fake timers or an awaited condition, not with headroom. + ### `lint` **There is no warning tier** — every `lint` script runs `--max-warnings 0`, so a diff --git a/AGENTS.md b/AGENTS.md index e80ed76f..5ae0690d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -396,6 +396,50 @@ diagnose a failing gate — is the `testing` skill. These are the rules. - **Render React components through `renderWithMantine`** (`src/test/renderWithMantine.tsx`); do not hand-roll a bare `MantineProvider`, which skips the project theme and the helper's options and drifts from every other test. Pass the `colorScheme` option to exercise a forced scheme rather than hand-rolling `defaultColorScheme`. Use `renderWithMantineTransitions` **only** when a test must assert mid-flight transition state, and read the long comment on the helper before changing anything about it. - **The web coverage `include` is a whitelist.** It names `components`/`hooks`/`theme`/`lib`/`utils`/`server` plus the browser-consumed `core/*` runtime, so a module placed **outside** those directories falls out of the gate entirely, silently. Place new modules inside a gated directory. The documented exceptions — `src/App.tsx` and the `src/main.tsx` / `src/index.ts` bootstraps — are called out in a comment on the `include` array itself. +### Test-gate timeouts are chosen values, stated once + +**Every wall-clock budget a test gate runs under is a value somebody picked, +and it lives in one place (#2323).** The shared budgets are `TIMEOUTS` and +`INTEGRATION_TIMEOUTS` in `vitest.shared.mts`; all six Vitest projects spread +one of them, and Testing Library's `asyncUtilTimeout` — which no Vitest config +can see — is configured in each web project's setup file +(`src/test/setup.ts`, `src/test/storybookSetup.ts`). +`npm run verify:test-timeouts` resolves each project through Vitest itself and +enforces all of it. + +This is not a licence to relax the bar. #1596 settled that: **a test that races +is fixed with fake timers or an awaited condition, never with headroom.** The +complementary case is what these budgets exist for — a test that is correct and +deterministic and is simply cut off, because the number bounding it was a +library default sized for an idle machine rather than for the one this repo is +worked on (three or four concurrent agent sessions in separate worktrees, each +free to run a full `local:gate`). Raising a budget nobody chose hides no race. + +- **Raise a budget in the shared object, not at a suite.** A per-suite + `}, 30_000)` moves one site and leaves every future file where it was — which + is how five of the six projects came to have no stated budget at all. Delete a + restatement of the project's own value rather than keeping it in sync. +- **A per-suite raise is right only where the work is genuinely different**, and + then it says so: real cross-process lock contention (`file-lock.test.ts`), a + full interactive OAuth round trip (`oauth-interactive.test.ts`). Name the + constant and state the reason at the site. +- **Never nest a budget inside an equal-or-larger one.** `waitFor({ timeout: N })` + inside a test whose own budget is `N` can never win, and it reports as a + timeout naming neither the wait nor its subject (#2292). Two bounds on the + same work must differ, and the one carrying the useful diagnostic must be the + tighter (`clients/cli/__tests__/e2e.test.ts`). +- **`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. +- **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 + actually flakes (#2250). The single exception is the transition auto-settle in + `clients/web/src/test/renderWithMantine.tsx`, whose failure is silent and + displaced rather than loud and local — and there only its `RAF_SLACK_MS` term + moves, never the term derived from the component's own animation constant. + + ## Mandatory pre-push gate - **ALWAYS run `npm run format` before committing.** The **root** `format` auto-fixes `core/`, the root `scripts/` tooling, the root shared surface, and every client's scope in one shot. `validate` runs the non-fixing `format:check` and will fail in CI on any unformatted file, so run the auto-fixer first rather than letting `format:check` catch it. diff --git a/clients/cli/__tests__/e2e.test.ts b/clients/cli/__tests__/e2e.test.ts index 4c3c990e..b6273ae0 100644 --- a/clients/cli/__tests__/e2e.test.ts +++ b/clients/cli/__tests__/e2e.test.ts @@ -7,6 +7,17 @@ import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-se 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). + */ +const CHILD_TIMEOUT_MS = 12_000; + interface SpawnResult { exitCode: number | null; stdout: string; @@ -36,7 +47,7 @@ function spawnCli(args: string[]): Promise { child.kill("SIGTERM"); } reject(new Error("E2E CLI timed out")); - }, 15000); + }, CHILD_TIMEOUT_MS); child.stdout.on("data", (d) => (stdout += d.toString())); child.stderr.on("data", (d) => (stderr += d.toString())); child.on("error", (err) => { diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index db1cad72..f6c6d77b 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -1,3 +1,12 @@ +/** + * Interactive OAuth against a real HTTP test server. + * + * Every test here raises past the cli project's shared 15s budget, and is meant + * to: each boots a server, waits for its well-known documents, runs a full + * authorization round trip, then disconnects. That is elapsed work, not slack + * for a loaded machine, so it stays stated per test rather than folding into + * the shared value (#2323). + */ import { describe, it, expect, afterEach, vi } from "vitest"; import { rmSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -90,6 +99,9 @@ const callbackUrlConfig = { }; const presetRedirectUrl = "http://127.0.0.1:6276/oauth/callback"; +/** A full interactive OAuth round trip against a freshly booted server. */ +const OAUTH_ROUND_TRIP_MS = 30_000; + describe("CLI interactive OAuth (integration)", () => { let mcpServer: TestServerHttp | null = null; @@ -104,148 +116,156 @@ describe("CLI interactive OAuth (integration)", () => { } catch { // ignore } - }, 30_000); + }, OAUTH_ROUND_TRIP_MS); - it("connects to an OAuth-protected server via the loopback callback server", async () => { - const serverConfig = { - ...getDefaultServerConfig(), - serverType: "streamable-http" as const, - ...createOAuthTestServerConfig({ - requireAuth: true, - supportDCR: true, - }), - }; + it( + "connects to an OAuth-protected server via the loopback callback server", + async () => { + const serverConfig = { + ...getDefaultServerConfig(), + serverType: "streamable-http" as const, + ...createOAuthTestServerConfig({ + requireAuth: true, + supportDCR: true, + }), + }; - mcpServer = new TestServerHttp(serverConfig); - const port = await mcpServer.start(); - const serverUrl = `http://localhost:${port}`; - await waitForOAuthWellKnown(serverUrl); + mcpServer = new TestServerHttp(serverConfig); + const port = await mcpServer.start(); + const serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); - const redirectUrlProvider = new MutableRedirectUrlProvider(); - redirectUrlProvider.redirectUrl = presetRedirectUrl; - const navigation = createAutoCompleteNavigation(redirectUrlProvider); - const client = new InspectorClient( - { - type: "streamable-http", - url: `${serverUrl}/mcp`, - } as MCPServerConfig, - { - environment: { - transport: createTransportNode, - oauth: { - storage: new NodeOAuthStorage(oauthTestStatePath), - navigation, - redirectUrlProvider, + const redirectUrlProvider = new MutableRedirectUrlProvider(); + redirectUrlProvider.redirectUrl = presetRedirectUrl; + const navigation = createAutoCompleteNavigation(redirectUrlProvider); + const client = new InspectorClient( + { + type: "streamable-http", + url: `${serverUrl}/mcp`, + } as MCPServerConfig, + { + environment: { + transport: createTransportNode, + oauth: { + storage: new NodeOAuthStorage(oauthTestStatePath), + navigation, + redirectUrlProvider, + }, }, + directAuthRecovery: true, + oauth: {}, }, - directAuthRecovery: true, - oauth: {}, - }, - ); + ); - await connectInspectorWithOAuth( - client, - { type: "streamable-http", url: `${serverUrl}/mcp` }, - redirectUrlProvider, - callbackUrlConfig, - undefined, - { isTTY: true }, - ); - - const tools = await client.listTools(); - expect(tools.tools.length).toBeGreaterThan(0); - await client.disconnect(); - }, 30_000); - - it("retries an RPC after step-up authorization when the user confirms", async () => { - const baseConfig = getDefaultServerConfig(); - - const serverConfig = { - ...baseConfig, - serverType: "streamable-http" as const, - tools: baseConfig.tools!.map((tool) => - tool.name === "get_temp" - ? { ...tool, requiredScopes: ["weather:read"] } - : tool, - ), - ...createOAuthTestServerConfig({ - requireAuth: true, - supportRefreshTokens: true, - supportDCR: true, - }), - }; - - mcpServer = new TestServerHttp(serverConfig); - const port = await mcpServer.start(); - const serverUrl = `http://localhost:${port}`; - await waitForOAuthWellKnown(serverUrl); - - const redirectUrlProvider = new MutableRedirectUrlProvider(); - redirectUrlProvider.redirectUrl = presetRedirectUrl; - const navigation = createAutoCompleteNavigation(redirectUrlProvider); - const client = new InspectorClient( - { - type: "streamable-http", - url: `${serverUrl}/mcp`, - } as MCPServerConfig, - { - environment: { - transport: createTransportNode, - oauth: { - storage: new NodeOAuthStorage(oauthTestStatePath), - navigation, - redirectUrlProvider, - }, - }, - directAuthRecovery: true, - oauth: { - scope: "mcp tools:read", - }, - }, - ); - - await connectInspectorWithOAuth( - client, - { type: "streamable-http", url: `${serverUrl}/mcp` }, - redirectUrlProvider, - callbackUrlConfig, - undefined, - { isTTY: true }, - ); - - const tools = await client.listTools(); - const getTempTool = tools.tools.find((tool) => tool.name === "get_temp"); - expect(getTempTool).toBeDefined(); - - const stderrSpy = vi - .spyOn(process.stderr, "write") - .mockImplementation(() => true); - - try { - const result = await withCliAuthRecoveryRetry( + await connectInspectorWithOAuth( client, { type: "streamable-http", url: `${serverUrl}/mcp` }, redirectUrlProvider, callbackUrlConfig, - makeFakeServerSettings(), - () => - client.callTool(getTempTool!, { - city: "NYC", - units: "C", - }), - { confirmStepUp: async () => true, isTTY: true }, + undefined, + { isTTY: true }, ); - const stepUpPrompted = stderrSpy.mock.calls.some( - ([chunk]) => - typeof chunk === "string" && - chunk.includes("Proceed with step-up authorization?"), - ); - expect(stepUpPrompted).toBe(true); - expect(result.success).toBe(true); - } finally { - stderrSpy.mockRestore(); + const tools = await client.listTools(); + expect(tools.tools.length).toBeGreaterThan(0); await client.disconnect(); - } - }, 30_000); + }, + OAUTH_ROUND_TRIP_MS, + ); + + it( + "retries an RPC after step-up authorization when the user confirms", + async () => { + const baseConfig = getDefaultServerConfig(); + + const serverConfig = { + ...baseConfig, + serverType: "streamable-http" as const, + tools: baseConfig.tools!.map((tool) => + tool.name === "get_temp" + ? { ...tool, requiredScopes: ["weather:read"] } + : tool, + ), + ...createOAuthTestServerConfig({ + requireAuth: true, + supportRefreshTokens: true, + supportDCR: true, + }), + }; + + mcpServer = new TestServerHttp(serverConfig); + const port = await mcpServer.start(); + const serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); + + const redirectUrlProvider = new MutableRedirectUrlProvider(); + redirectUrlProvider.redirectUrl = presetRedirectUrl; + const navigation = createAutoCompleteNavigation(redirectUrlProvider); + const client = new InspectorClient( + { + type: "streamable-http", + url: `${serverUrl}/mcp`, + } as MCPServerConfig, + { + environment: { + transport: createTransportNode, + oauth: { + storage: new NodeOAuthStorage(oauthTestStatePath), + navigation, + redirectUrlProvider, + }, + }, + directAuthRecovery: true, + oauth: { + scope: "mcp tools:read", + }, + }, + ); + + await connectInspectorWithOAuth( + client, + { type: "streamable-http", url: `${serverUrl}/mcp` }, + redirectUrlProvider, + callbackUrlConfig, + undefined, + { isTTY: true }, + ); + + const tools = await client.listTools(); + const getTempTool = tools.tools.find((tool) => tool.name === "get_temp"); + expect(getTempTool).toBeDefined(); + + const stderrSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + + try { + const result = await withCliAuthRecoveryRetry( + client, + { type: "streamable-http", url: `${serverUrl}/mcp` }, + redirectUrlProvider, + callbackUrlConfig, + makeFakeServerSettings(), + () => + client.callTool(getTempTool!, { + city: "NYC", + units: "C", + }), + { confirmStepUp: async () => true, isTTY: true }, + ); + + const stepUpPrompted = stderrSpy.mock.calls.some( + ([chunk]) => + typeof chunk === "string" && + chunk.includes("Proceed with step-up authorization?"), + ); + expect(stepUpPrompted).toBe(true); + expect(result.success).toBe(true); + } finally { + stderrSpy.mockRestore(); + await client.disconnect(); + } + }, + OAUTH_ROUND_TRIP_MS, + ); }); diff --git a/clients/cli/vitest.config.ts b/clients/cli/vitest.config.ts index 6ebb583b..92a0852c 100644 --- a/clients/cli/vitest.config.ts +++ b/clients/cli/vitest.config.ts @@ -1,7 +1,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { defineConfig } from "vitest/config"; -import { vitestSharedPaths } from "../../vitest.shared.mts"; +import { TIMEOUTS, vitestSharedPaths } from "../../vitest.shared.mts"; const dirname = path.dirname(fileURLToPath(import.meta.url)); const { projectResolve } = vitestSharedPaths(dirname); @@ -13,7 +13,9 @@ export default defineConfig({ environment: "node", include: ["__tests__/**/*.test.ts"], setupFiles: ["__tests__/helpers/mock-open-url.ts"], - testTimeout: 15000, + // Shared budgets (#2323). `testTimeout` was already 15000 here by hand; + // the hook and teardown budgets were Vitest's defaults until now. + ...TIMEOUTS, // The in-process runner (__tests__/helpers/cli-runner.ts) patches // process.std{out,err}.write to capture CLI output. Test files run in // separate forked processes (and tests within a file run sequentially), so diff --git a/clients/launcher/tsconfig.test.json b/clients/launcher/tsconfig.test.json index 19cbf076..2c170602 100644 --- a/clients/launcher/tsconfig.test.json +++ b/clients/launcher/tsconfig.test.json @@ -6,7 +6,15 @@ "extends": "./tsconfig.json", "compilerOptions": { "noEmit": true, - "rootDir": ".", + // Wide enough to contain `vitest.shared.mts`, which `vitest.config.ts` + // imports for the shared test budgets (#2323). `rootDir` only shapes error + // reporting under `noEmit`, and the emitting build config next door keeps + // its own `./src` — this project compiles nothing. `cli` and `tui` reach + // the same file with no `rootDir` at all, which infers the same thing. + "rootDir": "../..", + // Same import: `.mts` in a specifier needs this, and it is safe for the + // same reason — nothing here is emitted. `cli` and `tui` both set it. + "allowImportingTsExtensions": true, "types": ["node"] }, // `vitest.config.ts` lives here (not the src project) because the build diff --git a/clients/launcher/vitest.config.ts b/clients/launcher/vitest.config.ts index eef71846..f0e0b07f 100644 --- a/clients/launcher/vitest.config.ts +++ b/clients/launcher/vitest.config.ts @@ -1,10 +1,17 @@ import { defineConfig } from "vitest/config"; +import { TIMEOUTS } from "../../vitest.shared.mts"; export default defineConfig({ test: { globals: false, environment: "node", include: ["__tests__/**/*.test.ts"], + // Shared budgets (#2323). This project holds a single test file, which is + // exactly why it is pinned rather than left on the defaults: it is cheap to + // include and expensive to remember when a second file arrives. This import + // is the only thing this config takes from the shared module — the resolve + // aliases are for clients that load React or `core/`. + ...TIMEOUTS, coverage: { provider: "v8", reporter: ["text", "html", "json-summary"], diff --git a/clients/tui/__tests__/ResourcesTab.test.tsx b/clients/tui/__tests__/ResourcesTab.test.tsx index ad1fa78d..621abd7c 100644 --- a/clients/tui/__tests__/ResourcesTab.test.tsx +++ b/clients/tui/__tests__/ResourcesTab.test.tsx @@ -27,11 +27,23 @@ const tick = async () => { await new Promise((resolve) => setTimeout(resolve, 4)); }; -/** Poll the frame until it contains `substr` — stable under coverage load. */ +/** + * Poll the frame until it contains `substr` — stable under coverage load. + * + * The default budget (`tries` x the ~32ms `tick` below) matches `App.test.tsx`'s + * `POLL_TRIES`, which was raised twice for exactly this shape (#1742, #1942). + * It used to be 25 here — one third of the same renderer's budget under the + * same v8 instrumentation, for no stated reason, which made this the next one + * to go red. A high ceiling costs a passing assertion nothing: the loop returns + * the instant the predicate holds and only the slow path ever spends it + * (#2323). + */ +const POLL_TRIES = 100; + async function waitForFrame( getFrame: () => string | undefined, substr: string, - tries = 25, + tries = POLL_TRIES, ) { for (let i = 0; i < tries; i++) { if ((getFrame() ?? "").includes(substr)) return; diff --git a/clients/tui/__tests__/logger.test.ts b/clients/tui/__tests__/logger.test.ts index 426f2a16..d3226276 100644 --- a/clients/tui/__tests__/logger.test.ts +++ b/clients/tui/__tests__/logger.test.ts @@ -9,8 +9,19 @@ import { join } from "node:path"; * file (and any parent dir) appears a tick later. Awaiting this before cleanup * also guarantees the background mkdir/open has finished, so `afterEach`'s * rmSync can't race it into an ENOENT. + * + * The budget is a ceiling, not a wait: the loop exits on the first `existsSync` + * that succeeds, so a passing run pays only the 20ms poll interval and the + * headroom is spent only when a real filesystem write is genuinely slow — which + * is what an oversubscribed machine produces (#2323). 2000ms was Vitest-era + * guesswork against an idle one. */ -async function waitForFile(filePath: string, timeoutMs = 2000): Promise { +const FILE_APPEAR_TIMEOUT_MS = 10_000; + +async function waitForFile( + filePath: string, + timeoutMs = FILE_APPEAR_TIMEOUT_MS, +): Promise { const start = Date.now(); while (!existsSync(filePath)) { if (Date.now() - start > timeoutMs) { diff --git a/clients/tui/vitest.config.ts b/clients/tui/vitest.config.ts index d2528b94..84433522 100644 --- a/clients/tui/vitest.config.ts +++ b/clients/tui/vitest.config.ts @@ -1,7 +1,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { defineConfig } from "vitest/config"; -import { vitestSharedPaths } from "../../vitest.shared.mts"; +import { TIMEOUTS, vitestSharedPaths } from "../../vitest.shared.mts"; const dirname = path.dirname(fileURLToPath(import.meta.url)); const { projectResolve } = vitestSharedPaths(dirname); @@ -13,6 +13,10 @@ export default defineConfig({ environment: "node", include: ["__tests__/**/*.test.{ts,tsx}"], setupFiles: ["./__tests__/setup.ts"], + // Shared budgets (#2323). Ink renders through React, so every assertion + // here waits on a React commit under v8 instrumentation — the shape behind + // the poll-budget raises in #1742 and #1942. + ...TIMEOUTS, coverage: { provider: "v8", reporter: ["text", "html", "json-summary"], diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index ab929146..40f54d91 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -4295,59 +4295,58 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)", ); } - // Three full modal interaction sequences against the whole App tree, so this - // one runs long enough to trip the 5s default when the suite is under load. - it( - "suppresses a second clear for another entry with the same URL, and allows one after it settles", - { timeout: 20000 }, - 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. 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(); - 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 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; + }; - // 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; - }), - ); + // 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 clearFromSettings(user, "open-settings"); - await waitFor(() => - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1), - ); + await clearFromSettings(user, "open-settings"); + await waitFor(() => + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1), + ); - // 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); + // 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), - ); - }, - ); + // 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), + ); + }); }); diff --git a/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx b/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx index 66afa287..86402a82 100644 --- a/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx +++ b/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx @@ -557,19 +557,17 @@ describe("AppRenderer", () => { // budget that is too tight rather than of a broken observer. Raised // rather than retried: a `waitFor` that is generous costs nothing on the // passing path, since it returns as soon as the assertion holds. - await waitFor( - () => - expect(bridge.sendHostContextChange).toHaveBeenCalledWith( - expect.objectContaining({ - theme: "dark", - styles: expect.objectContaining({ - variables: expect.objectContaining({ - "--color-background-primary": "#101113", - }), + await waitFor(() => + expect(bridge.sendHostContextChange).toHaveBeenCalledWith( + expect.objectContaining({ + theme: "dark", + styles: expect.objectContaining({ + variables: expect.objectContaining({ + "--color-background-primary": "#101113", }), }), - ), - { timeout: 5000 }, + }), + ), ); } finally { getComputedStyleSpy.mockRestore(); diff --git a/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx b/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx index 133c8617..44b5c409 100644 --- a/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx +++ b/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import userEvent from "@testing-library/user-event"; import { act, + RAF_SLACK_MS, renderWithMantine, renderWithMantineTransitions, screen, @@ -18,7 +19,18 @@ import { HEADER_ANIM_MS, ViewHeader } from "./ViewHeader"; // duration plus rAF scheduling slack, so bumping HEADER_ANIM_MS can't silently // make the settle insufficient. (This is why ViewHeader.tsx exports // HEADER_ANIM_MS — the settle window must track the animation it settles.) -const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200; +// +// The slack half is `RAF_SLACK_MS` rather than a bare `+ 200` (#2323): the two +// terms answer to different things — the first to the component, the second to +// how busy the machine is — and only the second should move when the machine +// gets busier. Naming it is what lets it move in one place, here and in the +// helper's own fallback. +// +// No `enterDelay`/`exitDelay` is passed to any of ViewHeader's four +// `Transition`s, so HEADER_ANIM_MS really is the longest JS chain. The 150ms +// stagger visible in the UI is a CSS `animation-delay` (`App.css`), and a CSS +// animation schedules no JS timer for the settle to drain. +const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + RAF_SLACK_MS; // Mock @mantine/hooks so we can control useMediaQuery results per test. const mediaQueryMock = vi.hoisted(() => ({ value: false })); diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index bd4d5929..ebf9cacc 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -7,6 +7,14 @@ * is one process-wide queue per path, so two in-process callers are ordered * before the lock ever sees them. A child process is not scaffolding here — * it is the only participant that can produce the interleaving. + * + * Four tests here raise their own budget past the integration project's 30s, + * and are among the handful in the repo that still should (#2323). What they + * wait on is not React or a renderer — it is `proper-lockfile`'s stale-lock + * timeout and a second OS process actually contending for the file, which is + * real elapsed work rather than slack. The named constants below say so at + * each site; the restatements of the project's own 30s that used to sit + * alongside them are gone. */ import { describe, @@ -28,6 +36,11 @@ import { withSecretFileLock, resetFileLockWarnings, } from "@inspector/core/auth/node/file-lock.js"; + +/** A test that waits out a full stale-lock takeover in a second process. */ +const LOCK_TAKEOVER_MS = 90_000; +/** A test that waits on cross-process contention but not on a stale window. */ +const LOCK_CONTENTION_MS = 60_000; import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js"; import { SecretStoreUnavailableError } from "@inspector/core/auth/node/secret-store.js"; @@ -150,7 +163,7 @@ describe("withSecretFileLock across processes", () => { // …and having waited, it did not report a degraded write. expect(warnings()).toBe(""); await done; - }, 20_000); + }); it("locks a file that does not exist yet", async () => { // The first `set` on a fresh install has no `secrets.json` — and @@ -204,7 +217,7 @@ describe("withSecretFileLock across processes", () => { expect(await reader.get("srv", "env:FROM_CHILD")).toBe("1"); expect(await reader.get("srv", "env:FROM_PARENT")).toBe("2"); expect(warnings()).toBe(""); - }, 20_000); + }); it("creates the storage directory so the very first save is locked too", async () => { // `writeStoreFile` creates the parent directory, but from *inside* the @@ -232,7 +245,7 @@ describe("withSecretFileLock across processes", () => { expect(await reader.get("srv", "env:FIRST_EVER")).toBe("1"); // No degrade warning: the lock was genuinely held, not skipped. expect(warnings()).toBe(""); - }, 20_000); + }); }); describe("withSecretFileLock degrades rather than failing", () => { @@ -263,131 +276,147 @@ describe("withSecretFileLock degrades rather than failing", () => { expect(warn).toHaveBeenCalledTimes(1); }); - it("refuses the save rather than writing alongside a live holder", async () => { - // `ELOCKED` is evidence the lock is *working*, so degrading here would - // enter the exact interleaving the lock exists to prevent — and enter it - // knowing another writer is there. Held from this process, which is - // indistinguishable to `proper-lockfile` from a remote holder (it is not - // reentrant); the in-process queue is what keeps that out of the way in - // production. - // - // The wait is real: the retry budget deliberately outlasts the 10s stale - // window so a *crashed* holder resolves by takeover instead of failing - // everyone else's saves. This holder is alive and refreshing, so it never - // goes stale and the budget is spent in full. - const target = filePath(); - const lockfile = require_(LOCKFILE_MODULE) as { - lock: (f: string, o: object) => Promise<() => Promise>; - }; - // The **same** `stale` production uses, and that is not incidental: - // `isLockStale` is evaluated against the *waiter's* threshold while the - // holder refreshes on its own `stale / 2`. A holder configured looser - // (say 60s) refreshes every 30s and is therefore declared stale by a - // 10s waiter after 10s — the waiter takes over and the save succeeds, - // quietly testing the opposite of what this test claims. - const release = await lockfile.lock(target, { - realpath: false, - stale: 10_000, - }); + it( + "refuses the save rather than writing alongside a live holder", + async () => { + // `ELOCKED` is evidence the lock is *working*, so degrading here would + // enter the exact interleaving the lock exists to prevent — and enter it + // knowing another writer is there. Held from this process, which is + // indistinguishable to `proper-lockfile` from a remote holder (it is not + // reentrant); the in-process queue is what keeps that out of the way in + // production. + // + // The wait is real: the retry budget deliberately outlasts the 10s stale + // window so a *crashed* holder resolves by takeover instead of failing + // everyone else's saves. This holder is alive and refreshing, so it never + // goes stale and the budget is spent in full. + const target = filePath(); + const lockfile = require_(LOCKFILE_MODULE) as { + lock: (f: string, o: object) => Promise<() => Promise>; + }; + // The **same** `stale` production uses, and that is not incidental: + // `isLockStale` is evaluated against the *waiter's* threshold while the + // holder refreshes on its own `stale / 2`. A holder configured looser + // (say 60s) refreshes every 30s and is therefore declared stale by a + // 10s waiter after 10s — the waiter takes over and the save succeeds, + // quietly testing the opposite of what this test claims. + const release = await lockfile.lock(target, { + realpath: false, + stale: 10_000, + }); - const store = new FileSecretStore({ filePath: target }); - // One call, both assertions off the same rejection: each attempt spends - // the full retry budget, so a second would double the test's runtime to - // re-prove the same thing. - const err = await store - .set("srv", "env:MINE", "1") - .catch((e: unknown) => e); - expect(err).toBeInstanceOf(SecretStoreUnavailableError); - expect((err as Error).message).toMatch(/was not saved/); - await release(); + const store = new FileSecretStore({ filePath: target }); + // One call, both assertions off the same rejection: each attempt spends + // the full retry budget, so a second would double the test's runtime to + // re-prove the same thing. + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + await release(); - // Nothing was written behind the holder's back. - expect(existsSync(target)).toBe(false); - }, 90_000); + // Nothing was written behind the holder's back. + expect(existsSync(target)).toBe(false); + }, + LOCK_TAKEOVER_MS, + ); - it("refuses rather than degrading when a stale lock cannot be cleared", async () => { - // `acquireLock` does not only *create* directories — on finding a stale - // one it removes it and retries, and that removal can fail. A stale lock - // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and - // treating every non-`ELOCKED` error as "locks do not work here" meant - // every Inspector on the box quietly bypassed the *same* stuck lock and - // raced its writes — while the release-failure message was telling the - // operator saves would keep failing until they cleared it. - const target = filePath(); - const lockPath = `${target}.lock`; - await fs.mkdir(path.dirname(target), { recursive: true }); - await fs.mkdir(lockPath); - await fs.writeFile(`${lockPath}/stray`, "", "utf-8"); - // Backdated so it reads as stale — which is what sends `acquireLock` down - // the remove-and-retry path rather than straight to `ELOCKED`. - const longDead = new Date(Date.now() - 60_000); - await fs.utimes(lockPath, longDead, longDead); + it( + "refuses rather than degrading when a stale lock cannot be cleared", + async () => { + // `acquireLock` does not only *create* directories — on finding a stale + // one it removes it and retries, and that removal can fail. A stale lock + // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and + // treating every non-`ELOCKED` error as "locks do not work here" meant + // every Inspector on the box quietly bypassed the *same* stuck lock and + // raced its writes — while the release-failure message was telling the + // operator saves would keep failing until they cleared it. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + await fs.writeFile(`${lockPath}/stray`, "", "utf-8"); + // Backdated so it reads as stale — which is what sends `acquireLock` down + // the remove-and-retry path rather than straight to `ELOCKED`. + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); - const store = new FileSecretStore({ filePath: target }); - const err = await store - .set("srv", "env:MINE", "1") - .catch((e: unknown) => e); + const store = new FileSecretStore({ filePath: target }); + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); - expect(err).toBeInstanceOf(SecretStoreUnavailableError); - expect((err as Error).message).toMatch(/was not saved/); - // Nothing written behind the stuck lock, and no "unprotected" warning: - // this is a refusal, not a degrade. - expect(existsSync(target)).toBe(false); - expect(warnings()).not.toContain("not protected"); - }, 90_000); + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + // Nothing written behind the stuck lock, and no "unprotected" warning: + // this is a refusal, not a degrade. + expect(existsSync(target)).toBe(false); + expect(warnings()).not.toContain("not protected"); + }, + LOCK_TAKEOVER_MS, + ); - it("takes over the lock of a holder that died, rather than failing the save", async () => { - // The invariant behind refusing on `ELOCKED`: refusing is only defensible - // because a *crashed* holder resolves on its own first. `RETRY` therefore - // has to outlast `STALE_MS` — if the budget were the shorter of the two, - // one Inspector killed mid-save would make every later save on the box - // fail until someone deleted the lock by hand. - // - // A dead holder is exactly a lock directory nobody is refreshing, so it - // is staged directly: no child to race, and no dependence on how quickly - // a killed process is reaped. - const target = filePath(); - const lockPath = `${target}.lock`; - await fs.mkdir(path.dirname(target), { recursive: true }); - await fs.mkdir(lockPath); - const longDead = new Date(Date.now() - 60_000); - await fs.utimes(lockPath, longDead, longDead); + it( + "takes over the lock of a holder that died, rather than failing the save", + async () => { + // The invariant behind refusing on `ELOCKED`: refusing is only defensible + // because a *crashed* holder resolves on its own first. `RETRY` therefore + // has to outlast `STALE_MS` — if the budget were the shorter of the two, + // one Inspector killed mid-save would make every later save on the box + // fail until someone deleted the lock by hand. + // + // A dead holder is exactly a lock directory nobody is refreshing, so it + // is staged directly: no child to race, and no dependence on how quickly + // a killed process is reaped. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); - const store = new FileSecretStore({ filePath: target }); - await store.set("srv", "env:AFTER_CRASH", "1"); + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:AFTER_CRASH", "1"); - const reader = new FileSecretStore({ filePath: target }); - expect(await reader.get("srv", "env:AFTER_CRASH")).toBe("1"); - // Took the lock over — did not fall through to an unlocked write. - expect(warnings()).toBe(""); - }, 60_000); + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:AFTER_CRASH")).toBe("1"); + // Took the lock over — did not fall through to an unlocked write. + expect(warnings()).toBe(""); + }, + LOCK_CONTENTION_MS, + ); - it("stays silent per the delete contract when the lock is held", async () => { - // `delete` reports nothing by contract — only `set` hard-fails — so the - // refusal above must not turn a delete into a throw. - const target = filePath(); - const store = new FileSecretStore({ filePath: target }); - await store.set("srv", "env:A", "1"); + it( + "stays silent per the delete contract when the lock is held", + async () => { + // `delete` reports nothing by contract — only `set` hard-fails — so the + // refusal above must not turn a delete into a throw. + const target = filePath(); + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:A", "1"); - const lockfile = require_(LOCKFILE_MODULE) as { - lock: (f: string, o: object) => Promise<() => Promise>; - }; - // The **same** `stale` production uses, and that is not incidental: - // `isLockStale` is evaluated against the *waiter's* threshold while the - // holder refreshes on its own `stale / 2`. A holder configured looser - // (say 60s) refreshes every 30s and is therefore declared stale by a - // 10s waiter after 10s — the waiter takes over and the save succeeds, - // quietly testing the opposite of what this test claims. - const release = await lockfile.lock(target, { - realpath: false, - stale: 10_000, - }); - await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); - await release(); + const lockfile = require_(LOCKFILE_MODULE) as { + lock: (f: string, o: object) => Promise<() => Promise>; + }; + // The **same** `stale` production uses, and that is not incidental: + // `isLockStale` is evaluated against the *waiter's* threshold while the + // holder refreshes on its own `stale / 2`. A holder configured looser + // (say 60s) refreshes every 30s and is therefore declared stale by a + // 10s waiter after 10s — the waiter takes over and the save succeeds, + // quietly testing the opposite of what this test claims. + const release = await lockfile.lock(target, { + realpath: false, + stale: 10_000, + }); + await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); + await release(); - // …and the entry it could not delete is still there, not half-removed. - expect(await store.get("srv", "env:A")).toBe("1"); - }, 90_000); + // …and the entry it could not delete is still there, not half-removed. + expect(await store.get("srv", "env:A")).toBe("1"); + }, + LOCK_TAKEOVER_MS, + ); }); describe("withSecretFileLock reports what it cannot clean up", () => { @@ -457,7 +486,7 @@ describe("withSecretFileLock reports what it cannot clean up", () => { // Unguarded, the winner's directory is gone. expect(existsSync(lockPath)).toBe(false); - }, 30_000); + }); it("tells the operator to clear a lock that cannot expire on its own", async () => { // `rmdir` refuses a non-empty directory — and so does stale takeover, @@ -510,5 +539,5 @@ describe("withSecretFileLock reports what it cannot clean up", () => { // that did nothing wrong. `onCompromised` has already said what happened. expect(warnings()).not.toContain("by hand"); expect(warnings()).not.toContain("Could not release the lock"); - }, 30_000); + }); }); diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index 9cc2b065..9b4c7cf5 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -715,6 +715,10 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(warn).toHaveBeenCalledWith( expect.stringContaining("Could not lock the secrets file"), ); + // Past the integration project's own 30s deliberately, and one of the few + // sites that should be (#2323): this test waits on a second process holding + // the secrets-file lock until `proper-lockfile` gives up on it, which is + // real elapsed contention rather than slack for a loaded machine. }, 60_000); it("takes no lock when there is nothing to migrate (#2082)", async () => { diff --git a/clients/web/src/test/integration/auth/revocation-e2e.test.ts b/clients/web/src/test/integration/auth/revocation-e2e.test.ts index dfbd4d0c..646cf99b 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -96,7 +96,7 @@ describe("OAuth token revocation (RFC 7009)", () => { metadata = (await ( await fetch(`${serverUrl}/.well-known/oauth-authorization-server`) ).json()) as OAuthMetadata; - }, 30_000); + }); afterAll(async () => { await mcpServer?.stop(); @@ -105,7 +105,7 @@ describe("OAuth token revocation (RFC 7009)", () => { rmSync(storageDir, { recursive: true, force: true }); storageDir = ""; } - }, 30_000); + }); /** Run a real authorization-code exchange and return the issued tokens. */ async function authorize( diff --git a/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts b/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts index cbd06e52..353123e6 100644 --- a/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts +++ b/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts @@ -51,12 +51,12 @@ describe("RFC 8414 metadata at the OIDC well-known path (#2172)", () => { const port = await mcpServer.start(); serverUrl = `http://localhost:${port}`; await waitForOAuthWellKnown(serverUrl, { metadataPath: OIDC_PATH }); - }, 30_000); + }); afterAll(async () => { await mcpServer?.stop(); mcpServer = null; - }, 30_000); + }); it("serves plain RFC 8414 metadata only from the OIDC path", async () => { const oidc = await fetch(`${serverUrl}${OIDC_PATH}`); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts index d7a91274..f952a930 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts @@ -70,7 +70,7 @@ describe("OAuth connect on the probing eras (#1805)", () => { await mcpServer.stop(); mcpServer = null; } - }, 30_000); + }); afterAll(() => { try { @@ -166,7 +166,7 @@ describe("OAuth connect on the probing eras (#1805)", () => { expect( (error as AuthRecoveryRequiredError).authorizationUrl, ).toBeInstanceOf(URL); - }, 30_000); + }); it(`connects on the "${era}" era once authorization completes`, async () => { const serverUrl = await startProtectedModernServer(); @@ -183,6 +183,6 @@ describe("OAuth connect on the probing eras (#1805)", () => { expect(authorizing.getProtocolEra()).toBe("modern"); expect(authorizing.getProtocolVersion()).toBe(MODERN_PROTOCOL_VERSION); expect((await authorizing.listTools()).tools.length).toBeGreaterThan(0); - }, 30_000); + }); } }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts index 83e15acb..5c11dcb7 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts @@ -493,7 +493,7 @@ describe("modern-era negotiation (2026-07-28)", () => { await expect(settled).resolves.toBeInstanceOf(ToolCallCancelledError); await expect(aborted).resolves.toBe(true); - }, 30_000); + }); // The test above builds its own never-returning tool, which is the sharpest // way to assert on the server's abort signal but says nothing about the @@ -576,7 +576,7 @@ describe("modern-era negotiation (2026-07-28)", () => { // The handler must return promptly, saying it was cancelled — not run on to // its 30th second, which is what it did before the fix. await expect(outcome).resolves.toContain("cancelled after"); - }, 30_000); + }); it("cancels an in-flight MRTR call while its embedded request is pending", async () => { const started = await startMrtrServer(createMrtrTool()); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-direct-mid-session-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-direct-mid-session-e2e.test.ts index be981518..5f96b788 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-direct-mid-session-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-direct-mid-session-e2e.test.ts @@ -54,7 +54,7 @@ describe("InspectorClient direct mid-session OAuth", () => { await mcpServer.stop(); mcpServer = null; } - }, 30_000); + }); afterAll(() => { try { @@ -139,7 +139,7 @@ describe("InspectorClient direct mid-session OAuth", () => { expect(client.getStatus()).toBe("connected"); await client.disconnect(); - }, 30_000); + }); it("step-up after insufficient_scope throws AuthRecoveryRequiredError then succeeds after OAuth", async () => { const staticClientId = "test-direct-step-up"; @@ -244,5 +244,5 @@ describe("InspectorClient direct mid-session OAuth", () => { expect(result.success).toBe(true); await client.disconnect(); - }, 30_000); + }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-mid-session-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-mid-session-e2e.test.ts index 4cde2b6b..abc8bb47 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-mid-session-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-mid-session-e2e.test.ts @@ -104,7 +104,7 @@ describe("InspectorClient remote mid-session OAuth", () => { } remoteBaseUrl = undefined; remoteAuthToken = undefined; - }, 30_000); + }); afterAll(() => { try { @@ -206,7 +206,7 @@ describe("InspectorClient remote mid-session OAuth", () => { expect(client.getStatus()).toBe("connected"); await client.disconnect(); - }, 15_000); + }); it("recovers on reconnect when stored access token was invalidated before connect", async () => { await setupRemoteServer(); @@ -446,7 +446,7 @@ describe("InspectorClient remote mid-session OAuth", () => { await callbackClient.disconnect(); await client.disconnect(); - }, 30_000); + }); it("resumeAfterOAuth falls back to connect when remote session is dead", async () => { await setupRemoteServer(); @@ -549,7 +549,7 @@ describe("InspectorClient remote mid-session OAuth", () => { await callbackClient.disconnect(); await client.disconnect(); - }, 30_000); + }); it("dispatches authChallengeInteractive for ambient interactive recovery", async () => { await setupRemoteServer(); @@ -640,7 +640,7 @@ describe("InspectorClient remote mid-session OAuth", () => { ).toBeInstanceOf(URL); await client.disconnect(); - }, 15_000); + }); it("recovers from ambient auth notification after idle token invalidation", async () => { await setupRemoteServer(); @@ -731,5 +731,5 @@ describe("InspectorClient remote mid-session OAuth", () => { expect(client.getStatus()).toBe("connected"); await client.disconnect(); - }, 15_000); + }); }); diff --git a/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts b/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts index d9965b68..1663085e 100644 --- a/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts +++ b/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts @@ -74,7 +74,7 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { serverUrl = `http://localhost:${port}`; storageDir = mkdtempSync(join(tmpdir(), "mcp-inspector-2071-")); await waitForOAuthWellKnown(serverUrl); - }, 30_000); + }); afterAll(async () => { await mcpServer?.stop(); @@ -83,7 +83,7 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { rmSync(storageDir, { recursive: true, force: true }); storageDir = ""; } - }, 30_000); + }); it("advertises the non-default metadata URL on the 401 challenge", async () => { const response = await fetch(`${serverUrl}/mcp`, { method: "POST" }); @@ -179,7 +179,7 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { ).toEqual([]); await client.disconnect(); - }, 30_000); + }); it("observes the SSE challenge through a caller-supplied eventSourceInit.fetch", async () => { // That explicit fetch is deliberately not proxy- or intercept-wrapped, so @@ -225,7 +225,7 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { expect(observed).toContainEqual( expect.objectContaining({ resourceMetadataUrl: METADATA_URL_LITERAL }), ); - }, 30_000); + }); it("keeps the showcase config valid and carrying the custom path", () => { const configPath = fileURLToPath( diff --git a/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts b/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts index 612b4cbf..bb440fbd 100644 --- a/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts +++ b/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts @@ -174,7 +174,7 @@ describe("web-client cancellation over a per-request stream (#2140)", () => { // `notifications/cancelled` it was free to ignore (and did), its request // signal never fired, and this promise never resolved. await expect(slow.aborted).resolves.toBe(true); - }, 30_000); + }); it("advertises the per-request stream only for streamable-http", () => { // The flag is what the SDK forks on, and the factory is where a server's diff --git a/clients/web/src/test/renderWithMantine.tsx b/clients/web/src/test/renderWithMantine.tsx index 677d9ca0..a14834fd 100644 --- a/clients/web/src/test/renderWithMantine.tsx +++ b/clients/web/src/test/renderWithMantine.tsx @@ -108,12 +108,46 @@ export function renderWithMantineTransitions( return { ...result, unmount: downgradingUnmount }; } -// Fallback settle window: ≈500ms clears a typical few-hundred-millisecond -// transition plus the two-frame rAF scheduling slack. Callers that know their -// component's animation duration should pass `settleMs` (its longest JS timer -// chain — duration plus any `enterDelay`/`exitDelay` — plus slack) rather than -// rely on this default. -const DEFAULT_SETTLE_MS = 500; +// A settle window is two terms with completely different relationships to the +// machine, and they must not be scaled as one number (#2323). +// +// The first term tracks the component: the longest JS timer chain its +// transition schedules — its duration plus any `enterDelay`/`exitDelay`. That +// is fixed by the component and load does not move it. Callers that know their +// component derive it from the component's own exported constant, which is why +// `ViewHeader.test.tsx` writes `HEADER_ANIM_MS + RAF_SLACK_MS` rather than a +// literal: bumping the animation then cannot silently outrun the settle. +// +// The second term is this one — the slack in which the rAF callbacks, the React +// commit and the terminal `setTimeout` they schedule actually have to *run*. +// That work is CPU-bound, so it is the term an oversubscribed machine eats, and +// the only term that should grow when the machine gets busier. +// +// ⚠️ Unlike a timeout, this is a fixed sleep: it always waits the full window +// and never exits early. Raising it therefore costs every *passing* run (3 call +// sites x 300ms ≈ 0.9s), which is the opposite call from the one made for the +// repo's ordinary fixed sleeps. It is justified here only because this failure +// is silent and displaced rather than loud and local: an insufficient settle +// fails nothing at its own site — the test's assertions have already passed — +// and surfaces either as a leaked timer that `setup.ts` cancels with no report +// at all, or as an uncaught post-teardown `window is not defined` failing the +// whole run from an arbitrary innocent file (#1760). Re-running localizes +// neither. Do not generalize this to the ordinary sleeps. +// +// (A condition wait is not available and should not be re-litigated: a +// completed *enter* transition leaves no DOM signal to `waitFor` on, which is +// the whole reason this is a sleep at all.) +export const RAF_SLACK_MS = 500; + +// The animation term of the fallback window, for a caller that does not know +// its component's duration. A typical Mantine transition is a few hundred ms. +const TYPICAL_ANIM_MS = 300; + +// Fallback settle window, used by `settleTransitions()` called with no +// argument. Built from the same slack constant as the derived call sites so the +// two cannot drift apart. Callers that know their component's animation +// duration should pass `settleMs` instead of relying on this. +const DEFAULT_SETTLE_MS = TYPICAL_ANIM_MS + RAF_SLACK_MS; // Set by `renderWithMantineTransitions`; consumed once by the `afterEach` below. // `armedSettleMs === null` means no real-transitions render happened this test, @@ -165,8 +199,12 @@ function resetArming() { // `setTimeout`, and the React work they schedule against the still-mounted tree // — whichever is the actual escapee, it resolves on a live component before // cleanup unmounts. Because it awaits a *real* `setTimeout`, it deadlocks under -// fake timers, so guard against that with a clear message rather than a 5s -// test-timeout hang. +// fake timers, so guard against that with a clear message rather than leaving +// it to the project's `hookTimeout` — which is what actually bounds this await, +// since the auto-settle runs in an `afterEach`. That is 30000ms +// (`vitest.shared.mts`, #2323), so without this throw a fake-timer deadlock +// would take half a minute to report something naming neither transitions nor +// fake timers. export async function settleTransitions(ms: number = DEFAULT_SETTLE_MS) { if (vi.isFakeTimers()) { throw new Error( diff --git a/clients/web/src/test/setup.ts b/clients/web/src/test/setup.ts index c86b3634..53e6621b 100644 --- a/clients/web/src/test/setup.ts +++ b/clients/web/src/test/setup.ts @@ -1,6 +1,25 @@ import "@testing-library/jest-dom/vitest"; import { afterEach } from "vitest"; -import { cleanup } from "@testing-library/react"; +import { cleanup, configure } from "@testing-library/react"; + +// Testing Library's own `asyncUtilTimeout` default is 1000ms +// (`@testing-library/dom/dist/config.js`), and it governs every `waitFor` and +// `findBy*` in the unit project — 788 call sites — none of which chose it. One +// `configure` call is the whole surface, which is why it is here rather than +// argued a site at a time (#2323). +// +// 5x, because a contended happy-dom render is the worst-measured case on this +// team's machine (8 logical cores, three or four concurrent worktree sessions +// each free to run the full `local:gate`), and because once the project's +// `testTimeout` rises to 15000 this becomes the *binding* constraint on every +// async assertion in the project — the enclosing budget stops being the thing +// that cuts a wait short and this starts being it. +// +// The trade-off, stated rather than hidden: a genuinely-failing async +// assertion now takes 5s to report instead of 1s. Only failing assertions pay +// it, once each, on runs that are already the slow path. A *passing* `waitFor` +// returns the instant its callback stops throwing and is unaffected. +configure({ asyncUtilTimeout: 5000 }); // Node 22+ exposes an experimental `localStorage` placeholder that overrides // happy-dom's implementation. Without `--localstorage-file`, it's an empty diff --git a/clients/web/src/test/storybookSetup.ts b/clients/web/src/test/storybookSetup.ts new file mode 100644 index 00000000..bb49c73a --- /dev/null +++ b/clients/web/src/test/storybookSetup.ts @@ -0,0 +1,30 @@ +/** + * Setup file for the `storybook` Vitest project (#2323). + * + * It exists for one call. Storybook's instrumented Testing Library re-exports + * the same `asyncUtilTimeout: 1000` default as the unit project's, and it + * governs the 80 `waitFor` / `findBy*` sites across the story files. Those play + * functions run in a real Chromium against a real Storybook preview, so 1000ms + * is the *tighter* of the two bounds the project has — the per-test ceiling is + * 15000 (#2292) — and it is the one nobody chose. + * + * ⚠️ Two things about this file that are deliberate and easy to undo: + * + * 1. **It must not live in `.storybook/`, and must not call + * `setProjectAnnotations`.** `@storybook/addon-vitest` provisions the + * preview annotations itself, and skips doing so when it finds a setup file + * that is *both* inside `configDir` and contains that call (#1898) — so the + * old `.storybook/vitest.setup.ts` was opting out of the automatic path. + * Either half of the condition is enough to stay on it; this file misses + * both. `src/test/PreviewAnnotations.stories.tsx` asserts the provisioning + * directly, so a regression here fails a test rather than silently rendering + * every story without the Mantine decorator. + * 2. **The import is `storybook/test`, not `@testing-library/*`.** Storybook + * instruments its own copy so the interactions panel can trace each step; + * configuring a different copy would change nothing that a play function + * actually calls. + */ + +import { configure } from "storybook/test"; + +configure({ asyncUtilTimeout: 5000 }); diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 97034569..1070cfcf 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -14,7 +14,11 @@ import { } from "./server/vite-base-config"; import { buildWebServerConfigFromEnv } from "./server/web-server-config"; import { createBrowserExternalizedBuiltinGate } from "./server/browser-externalized-builtin-gate"; -import { vitestSharedPaths } from "../../vitest.shared.mts"; +import { + INTEGRATION_TIMEOUTS, + TIMEOUTS, + vitestSharedPaths, +} from "../../vitest.shared.mts"; const dirname = typeof __dirname !== "undefined" ? __dirname @@ -330,6 +334,12 @@ export default defineConfig(({ command }) => { // `container.isConnected` self-checks are the real guard against a // future regression. sequence: { hooks: "stack" }, + // Shared budgets (#2323). This is the largest test surface in the + // repo — 342 files — and every one of them ran on Vitest's own + // 5000ms until now. The single site that had been patched by hand + // (`App.test.tsx`'s three-modal sequence) named that default as its + // reason for existing. + ...TIMEOUTS, }, }, { @@ -349,9 +359,12 @@ export default defineConfig(({ command }) => { include: [integrationGlob], // Integration tests spawn real HTTP/stdio servers via test-servers/, // bind sockets, run e2e OAuth flows, and exercise filesystem-backed - // storage. 30s matches the v1.5 core/vitest.config.ts. - testTimeout: 30000, - hookTimeout: 30000, + // storage. 30s matches the v1.5 core/vitest.config.ts. Now stated + // once in `vitest.shared.mts` (#2323) rather than here, together + // with the `teardownTimeout` that was left on the 10000ms default — + // teardown is where these suites unlink that filesystem-backed + // storage and reap the servers they spawned. + ...INTEGRATION_TIMEOUTS, // Inline the MCP SDK so vi.mock("@modelcontextprotocol/client") // hooks the same transformed copy that source files import. // Externalized node_modules are loaded via Node's loader and bypass @@ -384,7 +397,16 @@ export default defineConfig(({ command }) => { // (#2292). A larger ceiling does not hide a defect here: a story // that blows 15s has genuinely failed, and every assertion stays as // strict as it was. - testTimeout: 15000, + // …which is the shared `TIMEOUTS.testTimeout` today (#2323): this + // project is where the value was first chosen, and the other five + // now follow it rather than restate it. The hook and teardown + // budgets come along with it, having been left on the defaults. + ...TIMEOUTS, + // One `configure({ asyncUtilTimeout })` call, and deliberately not + // in `.storybook/` — see the file's own header for why that + // location and the absence of `setProjectAnnotations` are what keep + // the automatic preview-annotation provisioning below intact. + setupFiles: [path.join(dirname, "src/test/storybookSetup.ts")], browser: { enabled: true, headless: true, @@ -395,12 +417,15 @@ export default defineConfig(({ command }) => { }, ], }, - // No `setupFiles`: since Storybook 10.3 `@storybook/addon-vitest` + // The `setupFiles` above is the project's only one, and it + // carries no preview annotations: since Storybook 10.3 + // `@storybook/addon-vitest` // provisions the preview annotations (`.storybook/preview.tsx` plus // `@storybook/addon-a11y/preview`) itself, and *skips* doing so when - // it finds a setup file calling `setProjectAnnotations` — so the old - // `.storybook/vitest.setup.ts` was both redundant and actively - // opting out of the automatic path (#1898). A green suite doesn't + // it finds a setup file that is both inside `configDir` and calls + // `setProjectAnnotations` — so the old `.storybook/vitest.setup.ts` + // was both redundant and actively opting out of the automatic path + // (#1898). A green suite doesn't // prove the automatic provisioning works (stories rendered without // the Mantine decorator would very likely still pass), so // `src/test/PreviewAnnotations.stories.tsx` asserts it directly. diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 08d25b36..833b186b 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -29,7 +29,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | Script | What it does | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs the four durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run validate` | Runs the five durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all), `verify:test-timeouts` (every Vitest project resolves to a budget somebody chose, both web projects configure Testing Library's `asyncUtilTimeout`, and `retry` stays unset) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. Those three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). A fourth, **`smoke:web:tabs`**, drives the core tabs against one server on one browser launch — Tools runs a tool and asserts its `structuredContent` section, Resources reads a resource with templates listed, Prompts fetches one ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)) — asserting the `data-*` contract in [the web README](../clients/web/README.md#core-tab-automation-contract) rather than visible copy. It is **Chromium-only**: those tabs are ordinary React and Mantine, so unlike the sandbox they are not engine-sensitive. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. | @@ -40,6 +40,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Five more arrive with the skills (#2163). `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. `claude-cli.test.mjs` covers the one place that decides how the CLI is spawned — the Windows `.cmd` shim needs a shell, and its arguments then need quoting — with the platform **injected**, since a win32-only bug that no runner here executes is exactly the kind that ships. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`. +| `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #2278, #2250, #1942 and #1742 were each an instance, found one site at a time. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. Three properties beyond the numbers: a **seventh project** with no row is an error rather than a silent skip, since an unstated budget is precisely the drift being prevented; **`retry` must stay unset**, because a retry turns a load-induced red into a silent green on the only pre-push gate here and would re-open #1596; and both web projects must `configure({ asyncUtilTimeout })`, the Testing Library half that no Vitest config can see — it defaults to 1000ms, governs 788 `waitFor`/`findBy*` sites, and becomes the *binding* bound on an async assertion once the enclosing per-test budget rises. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its pure decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`. | `npm run local:gate` | **Mandatory pre-push command.** `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/package.json b/package.json index e92ef833..9c52e81e 100644 --- a/package.json +++ b/package.json @@ -46,9 +46,10 @@ "verify:skills": "node scripts/verify-skills.mjs", "verify:skills:cli": "node scripts/verify-skills-cli.mjs", "test:scripts": "node --test \"scripts/**/*.test.mjs\"", - "validate": "npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run test:scripts && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", + "validate": "npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run verify:test-timeouts && npm run test:scripts && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", "verify:format-coverage": "node scripts/verify-format-coverage.mjs", "verify:dep-lockstep": "node scripts/verify-dep-lockstep.mjs", + "verify:test-timeouts": "node scripts/verify-test-timeouts.mjs", "validate:core": "npm run format:check:core && npm run format:check:scripts && npm run format:check:shared && npm run lint:core && npm run lint:shared", "lint:core": "eslint \"core/**/*.{ts,tsx}\" --max-warnings 0", "lint:shared": "eslint \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js --max-warnings 0", diff --git a/scripts/lib/browser-timeouts.mjs b/scripts/lib/browser-timeouts.mjs new file mode 100644 index 00000000..ffb7b6ac --- /dev/null +++ b/scripts/lib/browser-timeouts.mjs @@ -0,0 +1,49 @@ +/** + * Playwright wall-clock budgets for the web smokes (#2323). + * + * Before this, `smoke-web-tabs.mjs`, `smoke-web-elicitation.mjs` and + * `smoke-web-browser.mjs` carried 15 locator budgets between them across three + * independently-chosen scales, with no config anywhere to raise. They are the + * same three or four decisions repeated, so they are named once here and the + * scripts import them — the same shape `render-smoke.mjs`'s `DEFAULTS` already + * has, and for the same reason: a budget nobody can find is a budget nobody + * revisits. + * + * These are generous on purpose and cost a passing run nothing. Playwright + * budgets are ceilings on a poll, not sleeps: every one of them returns the + * instant its locator condition holds. What they have to absorb is a smoke + * running against a cold `dist/` build on a machine already carrying three + * other agent sessions' gates. + * + * ⚠️ Raising one of these does NOT make a smoke wait longer for a server that + * is never coming: the launcher-death race in `smoke-web-browser.mjs` and the + * `waitForStage` diagnostics in the other two are what report that, and they + * report it with a cause rather than a timeout. + */ + +export const BROWSER_TIMEOUTS = Object.freeze({ + /** + * Load the page, or act on a control the app is already showing — a click on + * a rendered tab or list row, a wait for the first meaningful frame. What it + * absorbs is the browser's own scheduling under load, not a server. + */ + ui: 30_000, + /** + * Wait for something that only appears after a round trip to an MCP server: + * an elicitation modal, a tool result panel. Longer than `ui` because a test + * server has to boot, negotiate and answer before the DOM can change. + */ + roundTrip: 45_000, + /** + * Wait on a sub-element of something already on screen — a row inside a + * populated panel, a section of an open accordion. The enclosing wait has + * already paid for the round trip, so this only covers the render. + */ + nested: 15_000, + /** + * A wait whose expiry is caught and ignored, so it must stay short: it is + * pure added latency on every passing run. `networkidle` is the only one — + * the Google Fonts request may never idle on a restricted network. + */ + bestEffort: 5_000, +}); diff --git a/scripts/lib/browser-timeouts.test.mjs b/scripts/lib/browser-timeouts.test.mjs new file mode 100644 index 00000000..f646e779 --- /dev/null +++ b/scripts/lib/browser-timeouts.test.mjs @@ -0,0 +1,27 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs"; + +test("every budget is a positive finite number of milliseconds", () => { + const entries = Object.entries(BROWSER_TIMEOUTS); + assert.ok(entries.length > 0); + for (const [name, ms] of entries) { + assert.equal(typeof ms, "number", `${name} is not a number`); + assert.ok(Number.isFinite(ms) && ms > 0, `${name} is not a usable budget`); + } +}); + +test("the object is frozen, so a smoke cannot mutate a shared budget", () => { + assert.ok(Object.isFrozen(BROWSER_TIMEOUTS)); +}); + +test("the budgets are ordered by what each one has to absorb", () => { + // Not decoration: the ordering IS the semantics documented on each key. A + // `nested` wait that outlived its enclosing `roundTrip` one, or a + // `bestEffort` wait long enough to be felt on every passing run, would mean + // the names had stopped describing the values. + assert.ok(BROWSER_TIMEOUTS.bestEffort < BROWSER_TIMEOUTS.nested); + assert.ok(BROWSER_TIMEOUTS.nested < BROWSER_TIMEOUTS.ui); + assert.ok(BROWSER_TIMEOUTS.ui < BROWSER_TIMEOUTS.roundTrip); +}); diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index 35018272..32355c48 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -64,6 +64,7 @@ import { setTimeout as delay } from "node:timers/promises"; import { fileURLToPath } from "node:url"; import { dirname, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; +import { BROWSER_TIMEOUTS } from "./lib/browser-timeouts.mjs"; import { attachPageDiagnostics, loadBrowser, @@ -135,7 +136,7 @@ try { // authenticates without a query param. const response = await page.goto(server.baseUrl, { waitUntil: "domcontentloaded", - timeout: 30_000, + timeout: BROWSER_TIMEOUTS.ui, }); if (!response || !response.ok()) { throw new Error( @@ -145,12 +146,12 @@ try { // First meaningful frame: the always-present "Add Servers" control. await page .getByRole("button", { name: /Add Servers/ }) - .waitFor({ state: "visible", timeout: 30_000 }); + .waitFor({ state: "visible", timeout: BROWSER_TIMEOUTS.ui }); // Settle window: let lazily-evaluated chunks that throw a tick after first // paint surface before we assert a clean boot. networkidle is best-effort // (the Google-Fonts request may never idle on a restricted network). await page - .waitForLoadState("networkidle", { timeout: 5_000 }) + .waitForLoadState("networkidle", { timeout: BROWSER_TIMEOUTS.bestEffort }) .catch(() => {}); await delay(500); }; diff --git a/scripts/smoke-web-elicitation.mjs b/scripts/smoke-web-elicitation.mjs index 6cbeaf9a..abeb4227 100644 --- a/scripts/smoke-web-elicitation.mjs +++ b/scripts/smoke-web-elicitation.mjs @@ -57,6 +57,7 @@ import { setTimeout as delay } from "node:timers/promises"; import { join, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; +import { BROWSER_TIMEOUTS } from "./lib/browser-timeouts.mjs"; import { attachPageDiagnostics, loadBrowser, @@ -203,13 +204,16 @@ async function runTool(page) { // The main-view tabs are a Mantine SegmentedControl: a visually-hidden radio // plus a sibling