diff --git a/AGENTS.md b/AGENTS.md index 726ab538..19aba0ac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -431,11 +431,16 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. exactly those tests and starves the worker pool. Measure before raising a budget that a passing test can spend in full; the call-site comment records the three-arm run. -- **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`). +- **An inner budget must be strictly smaller than the budget enclosing it.** + `waitFor({ timeout: N })` inside a test whose own budget is `N` can never win: + the test expires first and reports a timeout naming neither the wait nor its + subject (#2292). So when two bounds cover the same work they must differ, and + the **inner** one — the one carrying the useful diagnostic — has to be the + tighter, which is arranged by raising the outer rather than shrinking the + inner. Both shapes in this repo read that way: `AppRenderer.test.tsx`'s 5s + `waitFor` inside a 15s test, and `clients/cli/__tests__/e2e.test.ts`'s 15s + child timer (which kills the process group and names the CLI) inside a 25s + test. - **`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. diff --git a/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index da03b634..c3ca4201 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -312,18 +312,41 @@ export function checkAsyncUtilSource( } const name = binding[1] ?? "configure"; - const call = new RegExp(`\\b${name}\\s*\\(([\\s\\S]*?)\\)`).exec(code); - if (!call) { + // EVERY call, not the first. A setup file holding the expected call followed + // by a second one is the shape that matters: the later call wins at runtime, + // so inspecting only the first would approve a file whose effective timeout + // is something else entirely (Copilot). Requiring all of them to agree is + // stricter than checking the last and gives a clearer message than "the + // effective value is X" would. + const calls = []; + const nameRe = new RegExp(`\\b${name}\\s*\\(`, "g"); + let m; + while ((m = nameRe.exec(code)) !== null) { + const open = m.index + m[0].length - 1; + const close = matchBracket(code, open); + if (close === -1) break; + calls.push(code.slice(open + 1, close)); + nameRe.lastIndex = close; + } + if (calls.length === 0) { failures.push(`does not call ${name}()`); return failures; } - const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(call[1]); - if (!match) { + + const configured = calls + .map((args) => /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(args)) + .filter(Boolean) + .map((match) => match[1]); + if (configured.length === 0) { failures.push(`calls ${name}() without an asyncUtilTimeout`); - } else if (Number(match[1].replace(/_/g, "")) !== expected) { - failures.push( - `configures asyncUtilTimeout as ${match[1]}, expected ${expected}`, - ); + return failures; + } + for (const value of configured) { + if (Number(value.replace(/_/g, "")) !== expected) { + failures.push( + `configures asyncUtilTimeout as ${value}, expected ${expected}`, + ); + } } return failures; } @@ -362,6 +385,76 @@ export function checkSetupFilesWiring( ]; } +/** + * Skip whitespace from `i`. + * + * @param {string} code + * @param {number} i + * @returns {number} + */ +function skipWs(code, i) { + while (i < code.length && /\s/.test(code[i])) i += 1; + return i; +} + +/** + * Index just past the string or template literal starting at `i`. + * + * @param {string} code + * @param {number} i index of the opening quote + * @returns {number} index after the closing quote, or `code.length` if unclosed + */ +function skipString(code, i) { + const quote = code[i]; + let j = i + 1; + while (j < code.length) { + if (code[j] === "\\") { + j += 2; + continue; + } + if (code[j] === quote) return j + 1; + j += 1; + } + return code.length; +} + +/** + * Index of the bracket closing the one at `i`, honoring nesting and skipping + * string literals so a bracket inside a string cannot unbalance the count. + * + * This is what a regex cannot do, and the reason this scan is structural: a + * character class like `[^()]*` cannot consume `it.each([makeCase()])` or + * `it.skipIf(() => isWindows())`, so every such suite was a blind spot + * (Copilot). + * + * @param {string} code + * @param {number} i index of the opening bracket + * @returns {number} index of the matching close, or -1 + */ +function matchBracket(code, i) { + const open = code[i]; + const close = open === "(" ? ")" : "}"; + let depth = 0; + let j = i; + while (j < code.length) { + const c = code[j]; + if (c === '"' || c === "'" || c === "`") { + j = skipString(code, j); + continue; + } + if (c === open) depth += 1; + else if (c === close) { + depth -= 1; + if (depth === 0) return j; + } + j += 1; + } + return -1; +} + +/** Test-definition heads a `retry` option can be attached to. */ +const TEST_HEAD = /\b(?:it|test|describe|suite|bench)(?:\.\w+)*/g; + /** * A `retry` declared on an individual test or suite, or passed on a command * line — the two places a project-level check cannot see. @@ -370,24 +463,69 @@ export function checkSetupFilesWiring( * guard that only reads `project.config.retry` leaves `it("…", { retry: 2 })` * and `--retry=2` in an npm script wide open (Copilot). Both are scanned here. * - * The test-definition pattern deliberately requires the `retry` to sit in an - * **options object that follows the test name**, which is the only position - * Vitest reads it from. That keeps a `retry` field in fixture data, a variable - * named `retry`, or a mocked API's option out of it — this repo has several, and - * a bare `/\bretry\b/` scan would fail on all of them. + * Structural rather than a single regex, for two reasons learned one round + * apart. The `retry` must sit in an **options object that follows the test + * name**, which is the only position Vitest reads it from — that is what keeps + * a `retry` field in fixture data, a variable named `retry`, or a mocked API's + * option out of it, and this repo has several. And the chain may carry an + * argument of its own — `it.each([makeCase()])`, `it.skipIf(() => isWin())`, + * `it.each\`table\`` — which a character-class regex cannot step over once it + * contains nested parentheses, silently skipping every such suite. * * @param {string} source * @returns {string[]} the matched declarations, empty when there are none */ export function findTestLevelRetries(source) { const code = stripComments(source); - // The optional group between the chain and the name is `it.each`'s table — - // `it.each([...])("name", …)` or its tagged-template form — which otherwise - // pushes the name out of the position this pattern looks for, so every - // parameterized suite would be a blind spot. - const re = - /\b(?:it|test|describe|suite|bench)(?:\.\w+)*(?:\s*(?:\([^()]*\)|`[^`]*`))?\s*\(\s*(?:"[^"]*"|'[^']*'|`[^`]*`)\s*,\s*\{[^}]*?\bretry\s*:\s*([^,}\s]+)/g; - return [...code.matchAll(re)].map((m) => `retry: ${m[1]}`); + const found = []; + TEST_HEAD.lastIndex = 0; + let head; + while ((head = TEST_HEAD.exec(code)) !== null) { + const decl = readRetryOption(code, head.index + head[0].length); + if (decl) found.push(decl); + } + return found; +} + +/** + * Read a `retry` out of the options object of the test call starting at `i`. + * + * @param {string} code comment-stripped source + * @param {number} i index just past the `it`/`describe`/… chain + * @returns {string | null} + */ +function readRetryOption(code, i) { + let j = skipWs(code, i); + + // `it.each\`table\`` puts a tagged template between the chain and the call. + if (code[j] === "`") j = skipWs(code, skipString(code, j)); + if (code[j] !== "(") return null; + + // The first `(` is either the call itself or the chain's own argument + // (`it.each([...])`, `it.skipIf(…)`). It is the call when a string literal — + // the test name — comes first; otherwise step over it, balanced, and the real + // call is the next `(`. + let open = j; + let name = skipWs(code, open + 1); + if (!/["'`]/.test(code[name] ?? "")) { + const close = matchBracket(code, open); + if (close === -1) return null; + open = skipWs(code, close + 1); + if (code[open] !== "(") return null; + name = skipWs(code, open + 1); + if (!/["'`]/.test(code[name] ?? "")) return null; + } + + let k = skipWs(code, skipString(code, name)); + if (code[k] !== ",") return null; + k = skipWs(code, k + 1); + if (code[k] !== "{") return null; + + const end = matchBracket(code, k); + if (end === -1) return null; + const options = code.slice(k + 1, end); + const retry = /(?:^|[\s,{])retry\s*:\s*([^,}\s]+)/.exec(options); + return retry ? `retry: ${retry[1]}` : null; } /** diff --git a/scripts/verify-test-timeouts.test.mjs b/scripts/verify-test-timeouts.test.mjs index fd6af35b..d193e82c 100644 --- a/scripts/verify-test-timeouts.test.mjs +++ b/scripts/verify-test-timeouts.test.mjs @@ -324,6 +324,11 @@ test("a retry on an individual test or suite is caught", () => { `describe("x", { retry: 3 }, () => {});`, `it.each([1])("x %i", { retry: 2 }, () => {});`, 'describe.each([[1, 2]])("x", { retry: 1 }, () => {});', + // Nested parentheses in the chain's own argument. A character class cannot + // step over either of these, so both were blind spots (Copilot). + 'it.each([makeCase()])("x", { retry: 2 }, () => {});', + 'it.skipIf(() => isWindows())("x", { retry: 2 }, () => {});', + 'it.each`a|b`("x", { retry: 3 }, () => {});', `it('single quoted', { timeout: 100, retry: 2 }, () => {});`, ]) { assert.equal(findTestLevelRetries(src).length, 1, src); @@ -338,6 +343,8 @@ test("a retry that is not a test option is not a false positive", () => { `expect(client.retry).toBe(2);`, `it("x", async () => { await withRetries({ retry: 2 }); });`, `vi.mock("x", () => ({ retry: 2 }));`, + `it("has retry: 2 in the name", () => {});`, + `const o = { noRetry: 2 };`, ]) { assert.deepEqual(findTestLevelRetries(src), [], src); } @@ -386,3 +393,21 @@ test("discovery knows every filename Vitest loads a config from", () => { VITEST_CONFIG_FILENAMES.indexOf("vite.config.ts"), ); }); + +test("every configure() call is inspected, not just the first", () => { + // The later call wins at runtime, so approving a file on its first call would + // approve one whose effective timeout is something else entirely (Copilot). + const overridden = `import { configure } from "storybook/test"; +configure({ asyncUtilTimeout: 1000 }); +configure({ asyncUtilTimeout: 5000 });`; + assert.deepEqual(checkAsyncUtilSource(overridden, "storybook/test"), [ + "configures asyncUtilTimeout as 5000, expected 1000", + ]); +}); + +test("a configure() call that sets something else does not hide the real one", () => { + const source = `import { configure } from "storybook/test"; +configure({ testIdAttribute: "data-testid" }); +configure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); +});