diff --git a/packages/paperclip-runner/scripts/check-bare-real-process-waits.test.mjs b/packages/paperclip-runner/scripts/check-bare-real-process-waits.test.mjs index 742a6b1c8c2..9b336452dc1 100644 --- a/packages/paperclip-runner/scripts/check-bare-real-process-waits.test.mjs +++ b/packages/paperclip-runner/scripts/check-bare-real-process-waits.test.mjs @@ -90,3 +90,83 @@ test("allows an expect.poll( that passes an explicit timeout option", () => { assert.deepEqual(findBareRealProcessWaits(source), []); }); + +test("rejects a vi.waitFor( whose callback body passes an unrelated timeout option", () => { + const source = [ + "async function example() {", + " await vi.waitFor(() => invoke({ timeout: 500 }));", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), [ + { line: 2, pattern: "vi.waitFor(" }, + ]); +}); + +test("allows a vi.waitFor( whose own second argument passes a timeout option, even with a same-shaped callback option", () => { + const source = [ + "async function example() {", + " await vi.waitFor(() => invoke({ timeout: 500 }), { timeout: 10_000 });", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), []); +}); + +test("ignores a vi.waitFor( written inside a line comment", () => { + const source = [ + "async function example() {", + " // do not call vi.waitFor( here, it is not spawned-process bound", + " await Promise.resolve();", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), []); +}); + +test("ignores an expect.poll( written inside a block comment", () => { + const source = [ + "async function example() {", + " /* legacy note: this used to call", + " expect.poll( on the old transport */", + " await Promise.resolve();", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), []); +}); + +test("ignores a vi.waitFor( written inside a string literal", () => { + const source = [ + "async function example() {", + ' const message = "replace this call with vi.waitFor( for a live wait";', + " await Promise.resolve();", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), []); +}); + +test("ignores an expect.poll( written inside a template literal", () => { + const source = [ + "async function example() {", + " const message = `see expect.poll( for the retry loop`;", + " await Promise.resolve();", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), []); +}); + +test("still flags a real bare vi.waitFor( on the line after a comment mentioning the pattern", () => { + const source = [ + "async function example() {", + " // vi.waitFor( is normally the wrong tool here, but this one needs it:", + " await vi.waitFor(() => expect(x).toBe(1));", + "}", + ].join("\n"); + + assert.deepEqual(findBareRealProcessWaits(source), [ + { line: 3, pattern: "vi.waitFor(" }, + ]); +}); diff --git a/packages/paperclip-runner/scripts/lib/bare-real-process-wait.mjs b/packages/paperclip-runner/scripts/lib/bare-real-process-wait.mjs index ba684754fdc..496082b3d29 100644 --- a/packages/paperclip-runner/scripts/lib/bare-real-process-wait.mjs +++ b/packages/paperclip-runner/scripts/lib/bare-real-process-wait.mjs @@ -48,59 +48,149 @@ async function collectSourceFiles(target) { return files; } -// Find the index of the close paren that matches the open paren at -// `openParenIndex`, tracking nested `()`, `{}`, and `[]`, and skipping over -// string and template literals so a brace or paren inside one never throws -// off the count. -function findMatchingCloseParenIndex(source, openParenIndex) { - let depth = 0; - for (let index = openParenIndex; index < source.length; index += 1) { +// Replace every comment and every string- or template-literal body with +// spaces, one for one, so the result keeps the same length and the same +// line breaks as `source`. A line count taken from the result still lines +// up with `source`, and neither a wait-call pattern nor a `timeout` option +// written inside a comment or a literal can survive into the result. +function maskCommentsAndStrings(source) { + const masked = source.split(""); + let index = 0; + while (index < source.length) { + const twoCharacters = source.slice(index, index + 2); const character = source[index]; + if (twoCharacters === "//") { + while (index < source.length && source[index] !== "\n") { + masked[index] = " "; + index += 1; + } + continue; + } + if (twoCharacters === "/*") { + masked[index] = " "; + masked[index + 1] = " "; + index += 2; + while (index < source.length && source.slice(index, index + 2) !== "*/") { + if (source[index] !== "\n") masked[index] = " "; + index += 1; + } + if (index < source.length) { + masked[index] = " "; + masked[index + 1] = " "; + index += 2; + } + continue; + } + if (character === "'" || character === '"' || character === "`") { + const quote = character; + masked[index] = " "; + index += 1; + while (index < source.length && source[index] !== quote) { + if (source[index] === "\\" && index + 1 < source.length) { + masked[index] = " "; + index += 1; + if (source[index] !== "\n") masked[index] = " "; + index += 1; + continue; + } + if (source[index] !== "\n") masked[index] = " "; + index += 1; + } + if (index < source.length) { + masked[index] = " "; + index += 1; + } + continue; + } + index += 1; + } + return masked.join(""); +} + +// Find the index of the close paren that matches the open paren at +// `openParenIndex`, tracking nested `()`, `{}`, and `[]`. Callers pass a +// comment- and string-masked source, so a stray bracket inside a comment or +// a literal never throws off the count. +function findMatchingCloseParenIndex(maskedSource, openParenIndex) { + let depth = 0; + for (let index = openParenIndex; index < maskedSource.length; index += 1) { + const character = maskedSource[index]; if (character === "(" || character === "{" || character === "[") { depth += 1; } else if (character === ")" || character === "}" || character === "]") { depth -= 1; if (depth === 0) return index; - } else if (character === "'" || character === '"' || character === "`") { - const quote = character; - index += 1; - while (index < source.length && source[index] !== quote) { - if (source[index] === "\\") index += 1; - index += 1; - } } } - return source.length; + return maskedSource.length; +} + +// Split a call's argument-list text on its top-level commas, so a comma +// nested inside a callback body, an object, or an array does not split a +// single argument in two. +function splitTopLevelArguments(maskedCallArguments) { + const args = []; + let depth = 0; + let current = ""; + for (const character of maskedCallArguments) { + if (character === "(" || character === "{" || character === "[") { + depth += 1; + current += character; + } else if (character === ")" || character === "}" || character === "]") { + depth -= 1; + current += character; + } else if (character === "," && depth === 0) { + args.push(current); + current = ""; + } else { + current += character; + } + } + args.push(current); + return args; +} + +// Return the wait call's own options argument (`vi.waitFor(callback, +// options)` or `expect.poll(callback, options)`), or `null` when the call +// passes no second argument. Only this text, not the callback body, states +// the call's own timeout. +function findOptionsArgument(maskedSource, openParenIndex, closeParenIndex) { + const callArguments = maskedSource.slice(openParenIndex + 1, closeParenIndex); + const topLevelArguments = splitTopLevelArguments(callArguments); + return topLevelArguments.length >= 2 ? topLevelArguments[1] : null; } /** * Find each real-process-shaped wait call (`vi.waitFor(`, or `expect.poll(`, * including the two-line `expect\n .poll(` form) in `source` that has * neither an `IN_PROCESS_WAIT_MARKER` note on the line directly above it - * nor an explicit `timeout` option in its own argument list. A call site + * nor an explicit `timeout` option in its own options argument. A call site * with the marker declares, in place, that it settles fully in-process. A * call site with an explicit `timeout` option already states its own - * envelope. Either one needs no real-process deadline. + * envelope. Either one needs no real-process deadline. A pattern written + * inside a comment or inside a string or template literal is source text, + * not a call, and never counts as one. */ export function findBareRealProcessWaits(source) { const lines = source.split("\n"); + const masked = maskCommentsAndStrings(source); const found = []; for (const { name, regex } of BARE_WAIT_PATTERNS) { regex.lastIndex = 0; for ( - let match = regex.exec(source); + let match = regex.exec(masked); match !== null; - match = regex.exec(source) + match = regex.exec(masked) ) { - const line = source.slice(0, match.index).split("\n").length; + const line = masked.slice(0, match.index).split("\n").length; const precedingLine = lines[line - 2] ?? ""; if (precedingLine.includes(IN_PROCESS_WAIT_MARKER)) { continue; } const openParenIndex = match.index + match[0].length - 1; - const closeParenIndex = findMatchingCloseParenIndex(source, openParenIndex); - const callArguments = source.slice(openParenIndex + 1, closeParenIndex); - if (EXPLICIT_TIMEOUT_PATTERN.test(callArguments)) { + const closeParenIndex = findMatchingCloseParenIndex(masked, openParenIndex); + const optionsArgument = findOptionsArgument(masked, openParenIndex, closeParenIndex); + if (optionsArgument !== null && EXPLICIT_TIMEOUT_PATTERN.test(optionsArgument)) { continue; } found.push({ line, pattern: name }); diff --git a/packages/paperclip-runner/test/wait-for-live-process.ts b/packages/paperclip-runner/test/wait-for-live-process.ts index b57e1f1c943..b8c336fafbe 100644 --- a/packages/paperclip-runner/test/wait-for-live-process.ts +++ b/packages/paperclip-runner/test/wait-for-live-process.ts @@ -28,10 +28,59 @@ // and for this helper's own 50ms poll granularity. export const CAPABILITY_LIVE_PROCESS_WAIT_DEADLINE_MS = 10_000; +// A sentinel error thrown when a single attempt runs past the wait's +// remaining deadline. It never leaves `raceAttemptAgainstDeadline` to a +// caller other than `waitForCapabilityLiveProcess`, so a plain class with no +// message is enough to tell it apart from a real attempt failure. +class CapabilityLiveProcessAttemptTimeout extends Error {} + +// Run one attempt of `callback` against a deadline of `remainingMs`. A +// pending attempt does not stop when the deadline wins the race, so this +// attaches a no-op rejection handler to the attempt promise. Without that +// handler, an attempt that rejects after its own race has already lost would +// surface as an unhandled rejection instead of the deadline error below. +async function raceAttemptAgainstDeadline( + callback: () => T | Promise, + remainingMs: number, +): Promise { + const attempt = Promise.resolve().then(callback); + attempt.catch(() => {}); + let timer: ReturnType; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new CapabilityLiveProcessAttemptTimeout()), + remainingMs, + ); + }); + try { + return await Promise.race([attempt, timeout]); + } finally { + clearTimeout(timer!); + } +} + +function buildDeadlineExceededError( + label: string, + elapsedMs: number, + lastError: unknown, +): Error { + const detail = + lastError instanceof Error + ? (lastError.stack ?? lastError.message) + : String(lastError); + return new Error( + `Wait for "${label}" did not settle within ${CAPABILITY_LIVE_PROCESS_WAIT_DEADLINE_MS}ms ` + + `(observed ${elapsedMs}ms). Last observed error: ${detail}`, + { cause: lastError }, + ); +} + /** * Poll a capability-live operation that settles only after a real spawned * operating-system process does work. Use a deadline derived from the real - * process envelope described above. Unlike a bare `vi.waitFor` or + * process envelope described above. Race each attempt against the + * deadline's remaining time, so a callback that never settles fails at this + * helper's deadline instead of bypassing it. Unlike a bare `vi.waitFor` or * `expect.poll`, report the last observed error when the deadline expires, * together with `label` and the elapsed time, so a failure names what the * wait was waiting for and how long it waited. @@ -46,22 +95,24 @@ export async function waitForCapabilityLiveProcess( `no attempt of "${label}" settled before the deadline`, ); for (;;) { + const remainingMs = deadline - Date.now(); + if (remainingMs <= 0) { + throw buildDeadlineExceededError(label, Date.now() - start, lastError); + } try { - return await callback(); + return await raceAttemptAgainstDeadline(callback, remainingMs); } catch (error) { + if (error instanceof CapabilityLiveProcessAttemptTimeout) { + throw buildDeadlineExceededError( + label, + Date.now() - start, + lastError, + ); + } lastError = error; } - const elapsedMs = Date.now() - start; if (Date.now() >= deadline) { - const detail = - lastError instanceof Error - ? (lastError.stack ?? lastError.message) - : String(lastError); - throw new Error( - `Wait for "${label}" did not settle within ${CAPABILITY_LIVE_PROCESS_WAIT_DEADLINE_MS}ms ` + - `(observed ${elapsedMs}ms). Last observed error: ${detail}`, - { cause: lastError }, - ); + throw buildDeadlineExceededError(label, Date.now() - start, lastError); } await new Promise((resolve) => setTimeout(resolve, 50)); }