Don't
`) is taken as an apostrophe, never + * valid code there, so it hides nothing. A quote in JSX text after a space or a tag (`It is 'odd
`, + * `"Quoted
`) still opens a string, and blanks what follows on its line. * * It covers test code: specs and the helpers beside them in `test/` and `e2e/` folders, plus the test tooling CI runs on * every OS (`TEST_TOOLING`). @@ -27,25 +42,33 @@ import { join } from "node:path"; import { readFileSync } from "./context.mjs"; const TEST_ROOTS = ["apps", "packages", "packs", "tools", "examples"]; -const SOURCE = /\.(?:[cm]?[jt]s|tsx)$/; +const EXTENSION = String.raw`\.(?:[cm]?[jt]s|[jt]sx)$`; +const SOURCE = new RegExp(EXTENSION); +const SPEC = new RegExp(String.raw`\.(?:spec|test)${EXTENSION}`); /** Test tooling outside `test/` folders that CI runs, Windows included. */ const TEST_TOOLING = new Set(["tools/smoke-widget-tooling.mjs"]); const ALLOW = "invariant-allow: sync-rm-retries"; /** Whether a repo-relative path is test code. */ export function isTestPath(path) { - return TEST_TOOLING.has(path) || /(?:^|\/)(?:test|e2e)\//.test(path) || /\.(?:spec|test)\.[cm]?[jt]sx?$/.test(path); + return TEST_TOOLING.has(path) || /(?:^|\/)(?:test|e2e)\//.test(path) || SPEC.test(path); } /** - * `source` with comments and string-literal text replaced by spaces, newlines kept, so every index and line still - * points at the same place. Template literals are followed into their `${...}` expressions. + * `source` read two ways, each the same length with newlines kept, so every index and line still points at the same + * place: `code` has comments and string-literal text replaced by spaces, and `comments` keeps only the comments. + * Template literals are followed into their `${...}` expressions. */ -export function blankCommentsAndStrings(source) { +function scan(source) { const out = source.split(""); + const comments = source.replace(/[^\n]/g, " ").split(""); const blank = (index) => { if (out[index] !== "\n") out[index] = " "; }; + const blankComment = (index) => { + comments[index] = source[index]; + blank(index); + }; /** What encloses the current position: a template literal, or a `${` expression with its own brace depth. */ const stack = []; let index = 0; @@ -71,11 +94,14 @@ export function blankCommentsAndStrings(source) { continue; } if (character === "/" && next === "/") { - while (index < source.length && source[index] !== "\n") blank(index++); + while (index < source.length && source[index] !== "\n") blankComment(index++); } else if (character === "/" && next === "*") { const end = source.indexOf("*/", index + 2); const stop = end === -1 ? source.length : end + 2; - while (index < stop) blank(index++); + while (index < stop) blankComment(index++); + } else if ((character === '"' || character === "'") && /[\w$]/.test(source[index - 1] ?? "")) { + // A quote right after a name or a number cannot open a string in code; it is an apostrophe in JSX text. + index += 1; } else if (character === '"' || character === "'") { index += 1; while (index < source.length && source[index] !== character && source[index] !== "\n") { @@ -98,6 +124,29 @@ export function blankCommentsAndStrings(source) { index += 1; } } + return { code: out.join(""), comments: comments.join("") }; +} + +/** `source` with comments and string-literal text replaced by spaces, newlines kept. */ +export function blankCommentsAndStrings(source) { + return scan(source).code; +} + +/** + * `code` with each bracket access by a literal key, `["rmSync"]`, written as `.rmSync` padded to the same length, so the + * rest of the check reads it as member access. `code` has the key blanked, so `source` names it. + */ +function bracketAccessAsMember(code, source) { + const out = code.split(""); + for (const match of source.matchAll(/\[(\s*)(["'`])rmSync\2\s*\]/g)) { + const quote = match.index + 1 + match[1].length; + // Only where the brackets and quotes are code: not inside a comment or a string. + if (code[match.index] !== "[" || code[quote] !== match[2] || code[quote + 7] !== match[2]) continue; + const replacement = ".rmSync".padEnd(match[0].length, " "); + for (let offset = 0; offset < match[0].length; offset += 1) { + if (out[match.index + offset] !== "\n") out[match.index + offset] = replacement[offset]; + } + } return out.join(""); } @@ -118,7 +167,8 @@ function callText(code, open) { /** The names `rmSync` goes by in `code`: its own, and any alias the file gives it. */ function rmSyncNames(code) { const names = new Set(["rmSync"]); - for (const pattern of [/\brmSync\s+as\s+([A-Za-z_$][\w$]*)/g, /\brmSync\s*:\s*([A-Za-z_$][\w$]*)/g, /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:[\w$]+\.)?rmSync\b(?!\s*\()/g]) { + const assigned = /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:[\w$]+\s*(?:\([^()]*\))?\s*\.\s*)*rmSync\b(?!\s*(?:\?\.\s*)?\()/g; + for (const pattern of [/\brmSync\s+as\s+([A-Za-z_$][\w$]*)/g, /\brmSync\s*:\s*([A-Za-z_$][\w$]*)/g, assigned]) { for (const match of code.matchAll(pattern)) names.add(match[1]); } return [...names]; @@ -128,17 +178,18 @@ const escapeName = (name) => name.replace(/\$/g, "\\$"); /** The 1-based line of each `rmSync(...)` call in `source` that passes `maxRetries`. */ export function retryingRmSyncLines(source) { - const code = blankCommentsAndStrings(source); - const sourceLines = source.split("\n"); + const scanned = scan(source); + const code = bracketAccessAsMember(scanned.code, source); + const commentLines = scanned.comments.split("\n"); const names = rmSyncNames(code).map(escapeName).join("|"); const lines = []; - for (const match of code.matchAll(new RegExp(`(? { await rm(path, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); diff --git a/tools/test/test-cleanup-retries.spec.ts b/tools/test/test-cleanup-retries.spec.ts index bdf66fe8..e599da3b 100644 --- a/tools/test/test-cleanup-retries.spec.ts +++ b/tools/test/test-cleanup-retries.spec.ts @@ -1,11 +1,12 @@ import { spawn } from "node:child_process"; -import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { describe, expect, it } from "vitest"; -import { isTestPath, retryingRmSyncLines } from "../invariants/test-cleanup-retries-asynchronously.mjs"; +import { repoRelativePath, walk } from "../invariants/context.mjs"; +import runCheck, { isTestPath, retryingRmSyncLines } from "../invariants/test-cleanup-retries-asynchronously.mjs"; import { removeTestDirectory } from "../test-cleanup.ts"; // The sources below are strings, which the check blanks before it reads a file, so this file does not read as the calls @@ -85,6 +86,70 @@ describe("the check on retrying synchronous removal in test code", () => { expect(retryingRmSyncLines(source)).toEqual([5]); }); + it("finds rmSync taken from require under another name", () => { + const source = [ + 'const wipe = require("node:fs").rmSync;', + "const erase = require('fs').rmSync", + "wipe(dir, { maxRetries: 3 });", + "erase(dir, { maxRetries: 3 });", + ].join("\n"); + expect(retryingRmSyncLines(source)).toEqual([3, 4]); + }); + + it("finds an optional call", () => { + const source = ["fs.rmSync?.(dir, { maxRetries: 3 });", "rmSync ?. (dir, { maxRetries: 3 });", "fs.rmSync?.(dir, { recursive: true });"].join("\n"); + expect(retryingRmSyncLines(source)).toEqual([1, 2]); + }); + + it("finds a call by bracket access, and an alias taken that way", () => { + const source = [ + 'fs["rmSync"](dir, { maxRetries: 3 });', + "fs[ 'rmSync' ](dir, {", + " maxRetries: 3 });", + 'const wipe = fs["rmSync"];', + "wipe(dir, { maxRetries: 3 });", + 'fs["rmSync"](dir, { recursive: true });', + 'const text = \'fs["rmSync"](dir, { maxRetries: 3 })\';', + '// fs["rmSync"](dir, { maxRetries: 3 })', + ].join("\n"); + expect(retryingRmSyncLines(source)).toEqual([1, 2, 5]); + }); + + it("counts the marker only in a comment, not in a string", () => { + const source = [ + 'const note = "invariant-allow: sync-rm-retries";', + "rmSync(dir, { maxRetries: 3 });", + "rmSync(dir, { maxRetries: 3, label: 'invariant-allow: sync-rm-retries' });", + "/* invariant-allow: sync-rm-retries */ rmSync(dir, { maxRetries: 3 });", + ].join("\n"); + expect(retryingRmSyncLines(source)).toEqual([2, 3]); + }); + + it("does not let an apostrophe in JSX text hide a call later on its line", () => { + const source = [ + "const view =Don't
; rmSync(dir, { maxRetries: 3 });", + 'const size =6" wide
; rmSync(dir, { maxRetries: 3 });', + "const name = 'it\\'s'; rmSync(dir, { recursive: true });", + ].join("\n"); + expect(retryingRmSyncLines(source)).toEqual([1, 2]); + }); + + it("reads every file it counts as test code, a .jsx spec included", async () => { + const root = mkdtempSync(join(tmpdir(), "clarkcant-cleanup-check-")); + try { + const call = "rmSync(dir, { maxRetries: 3 });\n"; + for (const path of ["apps/web/src/view.spec.jsx", "apps/web/test/helper.jsx", "apps/web/src/view.jsx", "apps/web/test/notes.md"]) { + mkdirSync(dirname(join(root, path)), { recursive: true }); + writeFileSync(join(root, path), call); + } + const result = { failures: [] as string[], notes: [] as string[] }; + runCheck({ repoRoot: root, walk, relative: (target: string) => repoRelativePath(root, target), check: () => result }); + expect(result.failures.map((failure) => failure.split(":")[0]).sort()).toEqual(["apps/web/src/view.spec.jsx", "apps/web/test/helper.jsx"]); + } finally { + await removeTestDirectory(root); + } + }); + it("covers specs, the helpers in test and e2e folders and CI's widget tooling smoke, not product code", () => { expect(isTestPath("apps/runtime/test/live-nodes.ts")).toBe(true); expect(isTestPath("apps/web/e2e/fixtures/server.mjs")).toBe(true); @@ -124,20 +189,21 @@ async function holdAsWorkingDirectory(dir: string): Promise<{ release: () => Pro * say anything. */ describe.runIf(process.platform === "win32")("removing a directory Windows still holds", () => { - it("fails at once in the synchronous form, though it asks for retries", async () => { + // Node 22 and Node 24 before 24.21 fail at once without retrying; from 24.21 the retries run but sleep the main thread. + // Either way the release the test schedules cannot happen during the call, so the outcome is the same on every version. + it("fails in the synchronous form though it asks for retries, because the release it waits for cannot run", async () => { const dir = mkdtempSync(join(tmpdir(), "clarkcant-cleanup-retry-")); writeFileSync(join(dir, "file.txt"), "held"); const held = await holdAsWorkingDirectory(dir); + // Let go 100 ms in, well within the retry budget below (100 + 200 + 300 + 400 ms), as removeTestDirectory's test does. + const released = new Promise