// `make check` used to run `prettier --check .` twice: once inside the lint // container (`script/lint` builds `Dockerfile.lint`, which runs eslint and // prettier as build steps) and once again on the host, because `script/check` // also called `script/fmt-check`. Two passes, one verdict, and the host one is // the weaker of the two — its prettier is whatever the working tree happens to // have installed, while the container's is digest-pinned and installed under // `--frozen-lockfile`. // // The fix was to delete the host call from `script/check` and `script/precommit`. // Nothing about that fix is self-enforcing: anyone can wire `script/fmt-check` // back in, or add a prettier step to a Dockerfile, and every build stays green // while quietly doing the work twice again. So the count is asserted here // rather than promised in a comment. // // The assertion is a static walk of the invocation graph, not a string match // against one file. Starting from an entrypoint, it follows every edge the repo // actually uses to reach another command — `"$SCRIPT_DIR/"` and // `script/` into other scripts, `make ` through the Makefile // shims, `yarn run ` through the `package.json` scripts, and // `docker build -f ` into that Dockerfile's `RUN` steps — and counts the // prettier invocations it finds. A prettier call added anywhere in that graph // is therefore caught, wherever it is added. // // Undercounting is the failure mode that would make this test worthless, so the // walk is also asserted to have reached the nodes that matter: if a restructure // defeats the resolver, the reachability assertions fail rather than the count // silently dropping to zero and "passing". import { describe, expect, it } from "vitest"; import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { join } from "node:path"; const repoRoot = fileURLToPath(new URL("../../", import.meta.url)); const read = (name: string): string => readFileSync(join(repoRoot, name), "utf-8"); // A backslash at end of line continues the command; the resolver has to see the // whole invocation, since the interesting flags (`-f Dockerfile.lint`) can sit // on the continuation. const joinContinuations = (text: string): string[] => { const joined: string[] = []; for (const raw of text.split("\n")) { const line = raw.trim(); const previous = joined[joined.length - 1]; if (previous !== undefined && previous.endsWith("\\")) { joined[joined.length - 1] = `${previous.slice(0, -1).trim()} ${line}`; } else { joined.push(line); } } return joined; }; // Comments are stripped everywhere. The headers of these scripts explain the // duplication this test exists to prevent, and therefore name `prettier` and // `script/fmt-check` repeatedly; counting them would make the test assert the // prose instead of the behaviour. const executable = (text: string): string[] => joinContinuations(text).filter( (line) => line !== "" && !line.startsWith("#"), ); // Makefile targets are thin shims (`check:` / tab / `@script/check`), so a // `make ` edge has to resolve through them to keep "per `make check`" // meaning what it says. Recipe lines are the tab-indented ones. const makeRecipes = (): Map => { const recipes = new Map(); let current: string | null = null; for (const raw of read("Makefile").split("\n")) { if (raw.startsWith("\t")) { if (current !== null) { recipes.get(current)?.push(raw.trim().replace(/^[@-]+/, "")); } continue; } const target = /^([a-z][a-z-]*)\s*:(?!=)/.exec(raw); current = target === null ? null : target[1]; if (current !== null && !recipes.has(current)) { recipes.set(current, []); } } return recipes; }; const recipes = makeRecipes(); const packageScripts = (): Record => { const pkg = JSON.parse(read("package.json")) as { scripts?: Record; }; return pkg.scripts ?? {}; }; const scripts = packageScripts(); // Node keys: `script/`, `docker:`, `make:`, // `yarn:`. const commandsOf = (node: string): string[] => { if (node.startsWith("script/")) return executable(read(node)); if (node.startsWith("docker:")) { return executable(read(node.slice("docker:".length))) .filter((line) => line.startsWith("RUN ")) .map((line) => line.slice("RUN ".length)); } if (node.startsWith("make:")) { const target = node.slice("make:".length); const recipe = recipes.get(target); // A renamed or deleted target must be a loud failure: silently walking // an empty recipe would report zero prettier invocations, which reads // like the tidiest possible result. if (recipe === undefined) { throw new Error(`no such Makefile target: ${target}`); } return recipe; } if (node.startsWith("yarn:")) { return [scripts[node.slice("yarn:".length)] ?? ""]; } throw new Error(`unresolvable node: ${node}`); }; const edgesOf = (line: string): string[] => { const edges: string[] = []; // `"$SCRIPT_DIR/lint"`, `"$ROOT/script/lint"` and a bare `script/lint` are // all the same edge. for (const match of line.matchAll( /(?:\$SCRIPT_DIR|\$\{SCRIPT_DIR\}|script)\/([a-z][a-z-]*)/g, )) { edges.push(`script/${match[1]}`); } // Only real targets: `pkg_install gnumake make make make` in // script/bootstrap is a package name, not an invocation of this Makefile. for (const match of line.matchAll(/\bmake\s+([a-z][a-z-]*)/g)) { if (recipes.has(match[1])) edges.push(`make:${match[1]}`); } // Same rule for yarn: `yarn run prettier` is the linter itself (counted // below, not followed), `yarn run fmt-check` would be a package.json script // that runs it indirectly. for (const match of line.matchAll(/\byarn(?:\s+run)?\s+([a-z][a-z-]*)/g)) { if (match[1] in scripts) edges.push(`yarn:${match[1]}`); } // The container lint pass lives behind a `docker build`; without following // it the count would miss the one invocation that is supposed to survive. if (/\bdocker\s+build\b/.test(line)) { const file = /\s-f\s+(\S+)/.exec(line); edges.push(`docker:${file === null ? "Dockerfile" : file[1]}`); } return edges; }; interface Walk { prettier: number; reached: Set; } // Repeated invocations must count repeatedly — running the same script twice is // exactly the bug — so nodes are not deduplicated. The path stack is only there // to turn a cycle into a loud failure instead of a hang. const walk = (node: string, path: string[] = [], into?: Walk): Walk => { const result = into ?? { prettier: 0, reached: new Set() }; if (path.includes(node)) { throw new Error(`invocation cycle: ${[...path, node].join(" -> ")}`); } result.reached.add(node); for (const line of commandsOf(node)) { if (/\bprettier\b/.test(line)) { result.prettier += 1; continue; } for (const edge of edgesOf(line)) { walk(edge, [...path, node], result); } } return result; }; describe("prettier runs exactly once per make check", () => { const check = walk("make:check"); // The headline assertion, and the one the issue is about. it("invokes prettier once for the whole of make check", () => { expect(check.prettier).toBe(1); }); // Guards against the count being 1 (or 0) because the walk never got // anywhere. `make check` has to reach the suite, the lint script, and the // Dockerfile whose build IS the lint verdict. it.each(["script/check", "script/test", "script/lint", "Dockerfile.lint"])( "reaches %s while counting", (node) => { const key = node.startsWith("script/") ? node : `docker:${node}`; expect([...check.reached]).toContain(key); }, ); // The one that survives is the container's, not the host's: that is the // authoritative verdict, since a successful Dockerfile.lint build is what // CI treats as proof of a clean tree. it("keeps the surviving invocation inside the lint container", () => { expect(walk("docker:Dockerfile.lint").prettier).toBe(1); }); it("does not reach the host formatting check from make check", () => { expect([...check.reached]).not.toContain("script/fmt-check"); }); }); describe("the standalone entrypoints still do what their names say", () => { // REPO_POLICIES.md requires both `make lint` and `make fmt-check` to exist // and mean something. Dropping fmt-check from script/check must not turn it // into a target nobody can use, and must not leave `make check` passing // because both halves became no-ops. it("still checks formatting under make fmt-check", () => { expect(walk("make:fmt-check").prettier).toBe(1); }); it("still checks formatting under make lint", () => { expect(walk("make:lint").prettier).toBe(1); }); }); describe("script/precommit", () => { // Same duplication as script/check, same fix. The hook still catches a // badly formatted tree before the commit lands, because script/lint is the // container prettier run — that is the whole reason the host call could go. it("checks formatting exactly once", () => { expect(walk("script/precommit").prettier).toBe(1); }); it("gets that check from the lint container", () => { expect([...walk("script/precommit").reached]).toContain( "docker:Dockerfile.lint", ); }); }); describe("host and container prettier cannot disagree", () => { // With the host pass gone from `make check`, `make fmt-check` is the only // host-side formatting check left, and the container is the gate. The two // must keep producing the same verdict on the same tree, or a developer // running `make fmt-check` gets a green that CI then rejects. // // Three things make them agree, and all three are load-bearing: it("pins the same prettier for both", () => { const pkg = JSON.parse(read("package.json")) as { devDependencies: Record; }; // An exact version, not a range: `^3.8.1` would let the container and // the host resolve different builds with different formatting. expect(pkg.devDependencies.prettier).toMatch(/^\d+\.\d+\.\d+$/); }); it("installs it from the lockfile in the container", () => { // script/bootstrap is what Dockerfile.lint runs to install deps. expect(read("script/bootstrap")).toContain( "yarn install --frozen-lockfile", ); }); it("keeps .gitignore in the build context", () => { // Prettier 3 reads .gitignore as a default ignore file, so excluding it // from the context would change which files the container checks. const dockerignore = read(".dockerignore") .split("\n") .map((line) => line.trim()); expect(dockerignore).not.toContain(".gitignore"); }); });