From 5b80a5d834a0bacb8ed9ca3b4c6b86a89c0bfe40 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 15:15:09 -0400 Subject: [PATCH] fix: address Copilot review round 7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two scanner bypasses and a wrong claim in the testing skill. The call scan used `\b` before the binding name, and a word boundary succeeds straight after a dot — so `other.configure({ asyncUtilTimeout: 1000 })` satisfied the check while the imported binding was never called at all. A negative lookbehind excludes a member access and any identifier the name is merely the tail of; both shapes are now tested. The retry scan required a colon, so JavaScript's shorthand property form `it("x", { retry }, …)` — the same declaration with the value bound above — went through. Both forms are recognized now, and `{ retryCount }` still does not match. The testing skill told developers the auto-settle "no-ops under fake timers". It throws, deliberately, so a deadlock reports immediately instead of hanging until the project's hookTimeout. Documenting it as a silent no-op is the more dangerous direction to be wrong in: someone combining the two would expect degraded cleanup and get a failed test. The skill now says which it is and why. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 8 +++++-- scripts/verify-test-timeouts.mjs | 15 +++++++++--- scripts/verify-test-timeouts.test.mjs | 33 +++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index fd923817..6da0df69 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -259,8 +259,12 @@ renders. The rule stands on consistency, not on timer safety. 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 + busier (#2323). Do **not** also use `vi.useFakeTimers()` in that test: + the auto-settle awaits a real `setTimeout`, so under fake timers it **throws** + with a message telling you to call `vi.useRealTimers()` first — it does not + silently skip. That is deliberate (a deadlock would otherwise hang until the + project's `hookTimeout`), but it means the combination fails the test rather + than degrading. 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/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index c3ca4201..f00b83ed 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -318,8 +318,12 @@ export function checkAsyncUtilSource( // 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. + // ⚠️ `(? { `vi.mock("x", () => ({ retry: 2 }));`, `it("has retry: 2 in the name", () => {});`, `const o = { noRetry: 2 };`, + // Shorthand for a DIFFERENT property must not match the shorthand form. + `it("x", { retryCount }, () => {});`, ]) { assert.deepEqual(findTestLevelRetries(src), [], src); } @@ -411,3 +413,34 @@ configure({ testIdAttribute: "data-testid" }); configure({ asyncUtilTimeout: 1000 });`; assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); }); + +test("the shorthand property form is caught", () => { + // `{ retry }` is the same declaration as `{ retry: 2 }` with the value bound + // above, and a colon-requiring pattern misses it entirely (Copilot). + assert.deepEqual( + findTestLevelRetries('const retry = 2; it("x", { retry }, () => {});'), + ["retry (shorthand)"], + ); + assert.deepEqual( + findTestLevelRetries('it("x", { timeout: 100, retry }, () => {});'), + ["retry (shorthand)"], + ); +}); + +test("a member call is not mistaken for the imported binding", () => { + // `\\b` succeeds straight after a dot, so `other.configure(…)` satisfied the + // check while the imported binding was never called at all (Copilot). + const source = `import { configure } from "storybook/test"; +other.configure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [ + "does not call configure()", + ]); +}); + +test("a longer identifier ending in the binding name is not a call to it", () => { + const source = `import { configure } from "storybook/test"; +reconfigure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [ + "does not call configure()", + ]); +});