diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index 62a52e5c..fd923817 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -255,8 +255,11 @@ renders. The rule stands on consistency, not on timer safety. `defaultColorScheme="dark"`. - **Mid-flight transition state** (e.g. asserting a `data-anim="out"` cell during an exit crossfade) is the only reason to use `renderWithMantineTransitions`. - Pass `settleMs` derived from the component's real animation duration - (`HEADER_ANIM_MS + 200`), do **not** also use `vi.useFakeTimers()` in that test + Pass `settleMs` derived from the component's real animation duration **plus + the helper's shared slack** — `HEADER_ANIM_MS + RAF_SLACK_MS`, both imported, + never a literal: the first term tracks the component and the second tracks how + busy the machine is, and only the second should move when the machine gets + busier (#2323). Do **not** also use `vi.useFakeTimers()` in that test (the auto-settle no-ops under fake timers), and if the test unmounts the tree itself use the `unmount()` the helper returns. The mechanism is documented at length on the helper — read there before changing it. diff --git a/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx b/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx index 86402a82..b6efc1ed 100644 --- a/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx +++ b/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx @@ -557,17 +557,28 @@ 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", + // + // #2323 briefly deleted this argument as one of its "a budget inside an + // equal budget can never be observed" cases, which it was: 5000 inside a + // 5000ms test could never win. That is fixed from the other end now — the + // enclosing budget is 15000 — so the raise is observable again and has to + // stay, because the value it beats is a measured failure rather than a + // default. It does NOT ride on `asyncUtilTimeout`, which is pinned at + // 1000 precisely because raising it globally was measured worse + // (Copilot). + 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/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index 9b4c7cf5..e560a479 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 @@ -82,6 +82,15 @@ async function loadWithProbe(available: boolean, detail = "no D-Bus session") { return import("@inspector/core/auth/node/secret-store-selection.js"); } +/** + * Past the integration project's own 30s deliberately, and one of the few sites + * that should be (#2323): the test using this waits on a second process holding + * the secrets-file lock until `proper-lockfile`'s retries run out, which is real + * elapsed contention rather than slack for a loaded machine. Named to match + * `file-lock.test.ts`, which bounds the same wait. + */ +const LOCK_RETRIES_EXHAUSTED_MS = 60_000; + describe("parseSecretStoreEnv", () => { it("accepts each kind, case- and whitespace-insensitively", () => { expect(parseSecretStoreEnv("keyring")).toBe("keyring"); @@ -684,42 +693,42 @@ describe("absorbFileSecretsIntoKeyring", () => { return filePath; } - it("does not throw when another process holds the lock (#2082)", async () => { - // `withSecretFileLock` throws on a lock held past its retry budget, which - // is right for a `set` — the user is waiting on that value — and wrong - // here. This function is awaited directly by both `resolveSecretStore` - // branches, so an escaping error fails store resolution and with it the - // whole session: a stuck writer elsewhere on the box would stop the - // Inspector from starting. Leaving the file for the next run is the only - // acceptable outcome. - const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); - process.env.MCP_INSPECTOR_SECRET_FILE = filePath; - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); - const mod = await loadWithProbe(true); + it( + "does not throw when another process holds the lock (#2082)", + async () => { + // `withSecretFileLock` throws on a lock held past its retry budget, which + // is right for a `set` — the user is waiting on that value — and wrong + // here. This function is awaited directly by both `resolveSecretStore` + // branches, so an escaping error fails store resolution and with it the + // whole session: a stuck writer elsewhere on the box would stop the + // Inspector from starting. Leaving the file for the next run is the only + // acceptable outcome. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); - const properLockfile = (await import("proper-lockfile")).default; - const release = await properLockfile.lock(filePath, { - realpath: false, - stale: 10_000, - }); + const properLockfile = (await import("proper-lockfile")).default; + const release = await properLockfile.lock(filePath, { + realpath: false, + stale: 10_000, + }); - await expect( - mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()), - ).resolves.toBeUndefined(); - await release(); + await expect( + mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()), + ).resolves.toBeUndefined(); + await release(); - // Left exactly as it was, and said so — asserting the *lock* message - // specifically, since the pre-existing claim-failure warning also ends in - // "left in place" and would let this pass without the lock ever being hit. - expect(existsSync(filePath)).toBe(true); - 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); + // Left exactly as it was, and said so — asserting the *lock* message + // specifically, since the pre-existing claim-failure warning also ends in + // "left in place" and would let this pass without the lock ever being hit. + expect(existsSync(filePath)).toBe(true); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("Could not lock the secrets file"), + ); + }, + LOCK_RETRIES_EXHAUSTED_MS, + ); it("takes no lock when there is nothing to migrate (#2082)", async () => { // The overwhelmingly common startup: a keychain is available and no file diff --git a/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index 82527348..edafe525 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -212,16 +212,42 @@ export function checkAsyncUtilSource( ) { const code = stripComments(source); const failures = []; - if (!new RegExp(`from\\s+["']${from.replace(/\//g, "\\/")}["']`).test(code)) { + + // Bind-then-check, not two independent regexes. Checking "imports from X" and + // "calls configure()" separately passes on a split import — `cleanup` from + // `@testing-library/react` and `configure` from `@testing-library/dom` — which + // is a configuration applied to a copy no test calls, and green by inspection + // (Copilot). So find the name `configure` is bound to *in the import from the + // required module*, and require the call to use that name. + const importRe = new RegExp( + `import\\s*\\{([^}]*)\\}\\s*from\\s*["']${from.replace(/[.*+?^$()|[\]\\]/g, "\\$&").replace(/\//g, "\\/")}["']`, + ); + const imported = importRe.exec(code); + if (!imported) { failures.push(`does not import from "${from}"`); - } - if (!/\bconfigure\s*\(/.test(code)) { - failures.push("does not call configure()"); return failures; } - const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(code); + + // `configure`, or `configure as somethingElse`. + const binding = imported[1] + .split(",") + .map((spec) => spec.trim()) + .map((spec) => /^configure(?:\s+as\s+([A-Za-z_$][\w$]*))?$/.exec(spec)) + .find(Boolean); + if (!binding) { + failures.push(`does not import configure from "${from}"`); + return failures; + } + const name = binding[1] ?? "configure"; + + const call = new RegExp(`\\b${name}\\s*\\(([\\s\\S]*?)\\)`).exec(code); + if (!call) { + failures.push(`does not call ${name}()`); + return failures; + } + const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(call[1]); if (!match) { - failures.push("calls configure() without an asyncUtilTimeout"); + failures.push(`calls ${name}() without an asyncUtilTimeout`); } else if (Number(match[1].replace(/_/g, "")) !== expected) { failures.push( `configures asyncUtilTimeout as ${match[1]}, expected ${expected}`, diff --git a/scripts/verify-test-timeouts.test.mjs b/scripts/verify-test-timeouts.test.mjs index 2839cefa..5fd35f4c 100644 --- a/scripts/verify-test-timeouts.test.mjs +++ b/scripts/verify-test-timeouts.test.mjs @@ -128,6 +128,42 @@ configure({ testIdAttribute: "data-testid" });`; ]); }); +test("a split import does not satisfy the check", () => { + // The hole this closes: checking "imports from X" and "calls configure()" + // independently passes when `configure` comes from a *different* Testing + // Library copy than the one the tests use, which is a configuration nothing + // reads — and green by inspection (Copilot). + const source = `import { cleanup } from "@testing-library/react"; +import { configure } from "@testing-library/dom"; +configure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "@testing-library/react"), [ + 'does not import configure from "@testing-library/react"', + ]); +}); + +test("configure imported under an alias is followed to its call", () => { + const source = `import { configure as cfg } from "storybook/test"; +cfg({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); + + const unused = `import { configure as cfg } from "storybook/test"; +configure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(unused, "storybook/test"), [ + "does not call cfg()", + ]); +}); + +test("the timeout is read from the configure call, not from anywhere in the file", () => { + // A matching number elsewhere in the module must not stand in for the + // argument actually passed. + const source = `import { configure } from "storybook/test"; +const asyncUtilTimeout = 1000; +configure({ testIdAttribute: "data-testid" });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [ + "calls configure() without an asyncUtilTimeout", + ]); +}); + test("the import is checked, not just the call", () => { // Storybook instruments its own Testing Library copy, so configuring // `@testing-library/*` from a story setup configures a copy no play function @@ -140,11 +176,13 @@ configure({ asyncUtilTimeout: 1000 });`; }); test("a setup file with no configure() at all is caught", () => { - const failures = checkAsyncUtilSource( - "export const nothing = 1;\n", - "storybook/test", + // Reported as the missing import and nothing else: once the module is not + // imported there is no binding to look for a call to, so listing a second + // failure would be guesswork about which of the two the author meant. + assert.deepEqual( + checkAsyncUtilSource("export const nothing = 1;\n", "storybook/test"), + ['does not import from "storybook/test"'], ); - assert.equal(failures.length, 2); }); test("both web projects have an asyncUtilTimeout site", () => {