// `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 — `run:` steps in the CI workflow, // `"$SCRIPT_DIR/"` and `script/` into other scripts, `make ` // and `$(MAKE) ` through the Makefile shims, the prerequisites of a // Makefile target, `yarn run ` through the `package.json` scripts, and // `docker build` into the `RUN` steps of the Dockerfile it names, heredoc // bodies included — and counts the prettier invocations it finds. // // What that covers is worth stating exactly rather than as "anywhere", because // a comment claiming more than the code delivers is the same defect as an // assertion that cannot fail — and an earlier draft of this header did claim // it, while `check: fmt-check` and `@$(MAKE) fmt-check` both walked straight // past it. Command names are read as `[a-z][a-z0-9_-]*` for Makefile targets // and `script/` files, and `[A-Za-z][A-Za-z0-9_:-]*` for `package.json` // scripts. A command reached by some other means — `sh -c`, a shell alias, an // `include`d makefile, a name outside those charsets, a generated file — is // not followed. Recipe lines reached through a make conditional are followed, // without evaluating the condition, and so is a recipe written on the target // line after a `;`; a recipe built by an `include`d makefile, a pattern rule, // or a target name outside that charset is not. A variable given a literal // value on one line — `VAR := ...`, `VAR = ...`, `VAR ?= ...` — is substituted // wherever the Makefile spells it `$(VAR)` or `${VAR}`, because this Makefile // writes its recipes that way (`@$(YARN) tsc --watch`); the substitution is // one pass over literals, so a value that itself names another variable, a // value built by a make function (`$(shell ...)`, `$(addprefix ...)`), and the // body of a `define`/`endef` block are not expanded and not followed. A // `docker build` resolves to the file named by `-f`, `-f=`, `-f` with // the value attached to the flag, `--file` or `--file=`, wherever in the // invocation that flag sits, and to `Dockerfile` when it names none; a // Dockerfile chosen some other way — a bundled short flag cluster // (`-qf `), a value this walk cannot expand to a literal — resolves to // the default rather than to the real file. Within those edges, a // prettier call is caught wherever it is added. // // Two entrypoints are walked, because they cover different graphs: `make check` // is what a developer runs, and `.gitea/workflows/check.yml` is what CI runs. // The CI walk starts at the workflow file rather than at a hand-picked script, // so "the path CI executes" is read out of the repo instead of assumed; it // reaches `script/cibuild`, and through it the `Dockerfile` image that `make // check` never touches. Walking only `make check` is how a duplicate prettier // pass in `Dockerfile` stayed invisible. // // Undercounting is the failure mode that would make this test worthless. Three // things guard against it: the walk is asserted to have reached the nodes that // matter, an unresolvable or empty node is a thrown error rather than a quiet // zero, and prettier is counted per occurrence rather than per line, so two // invocations chained with `&&` cannot read as one. 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("#"), ); // Every occurrence, not "does this line mention prettier": a line that reads // `yarn run prettier --check . && yarn run prettier --check src` is two passes // over the same tree, which is exactly the bug this file exists to catch, and // counting it as one would hide it. `.prettierrc` and `.prettierignore` are not // invocations and do not match, because `\b` requires a non-word character // after the name. const countPrettier = (line: string): number => (line.match(/\bprettier\b/g) ?? []).length; // 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. // // The prerequisite list is read as well, and it is not decoration: make runs a // target's prerequisites before its recipe, so `check: fmt-check` invokes // fmt-check every bit as much as a `@script/fmt-check` line in the recipe // would. An earlier version of this parser captured the target name and threw // the rest of the line away, which made `check: fmt-check` — a one-token edit, // and the most natural way for someone to wire the host formatting check back // into `make check` — a second prettier pass that this file scored as one. The // shipped Makefile already relies on prerequisites (`install: build-bin`), so // this is the file's own house style rather than a contrived evasion. interface MakeTarget { prerequisites: string[]; recipe: string[]; } // Names are lowercase-with-dashes here, plus digits and underscores. The // leading character is deliberately not uppercase: `YARN := yarn run` is a // variable assignment, not a rule, and `(?!=)` alone would not say so on a // line spelled `YARN: yarn run`. const MAKE_TARGET_NAME = "[a-z][a-z0-9_-]*"; // Conditional directives are not target lines, and they are not recipe lines // either — they are parse-time structure wrapped around the recipe that // encloses them. Treating one as an ordinary non-tab line ends the current // recipe, and every tab-indented line after it is discarded, so // // check: // \t@script/check // ifeq (1,1) // \t@script/fmt-check // endif // // reads as a recipe of one command while `make -n check` prints two. The // condition is deliberately not evaluated: that would mean evaluating make // variables, and the conservative reading — every branch is reachable, so // every branch's lines are recipe lines — is the one that cannot lose an // invocation. Counting a command from a branch make would skip is a false // alarm someone fixes; missing one is the failure this file exists to prevent. const MAKE_CONDITIONAL = /^\s*(?:ifeq|ifneq|ifdef|ifndef|else|endif)\b/; // `@$(FMT)` is one token away from being a second prettier pass, and until the // variable is resolved it reads as a line that invokes nothing: `make -n check` // printed both `script/check` and `script/fmt-check` while this file scored // one. `$(MAKE)` was already special-cased below, which is this same rule // half-applied to a single name; this is the general form of it. // // Deliberately only the simple case: a name assigned a literal on one line, // optionally through an `export`/`override` prefix. `:=` and `=` differ in // when make expands them, which does not change the single value a literal can // take, so those two are read the same way and the last one wins. `?=` differs // in whether it assigns at all — it is skipped when the name already has a // value — so among assignments to one name the first `?=` wins, and reading it // as last-wins would resolve a reference to the string make discards. What is // not read is anything needing evaluation — a value containing another // `$(...)`, a make function, or a `define` body — // because expanding those means implementing make, and a half-implementation // that resolves a variable to the wrong string would count invocations that do // not happen. An unresolved reference is left standing verbatim instead, which // is the same dead end as any other unfollowed edge rather than a wrong answer. const MAKE_ASSIGNMENT = new RegExp( `^(?:(?:export|override)\\s+)*([A-Za-z_][A-Za-z0-9_]*)\\s*(:=|\\?=|=)\\s*(.*)$`, ); const MAKE_VARIABLE_REFERENCE = /\$[({]([A-Za-z_][A-Za-z0-9_]*)[)}]/g; // `define`/`endef` bodies are skipped rather than parsed: the body is a // multi-line value, so reading its lines as assignments would take whatever // `=` they happen to contain and call it a variable. const makeVariables = (text: string): Map => { const variables = new Map(); let inDefine = false; for (const raw of text.split("\n")) { if (/^\s*endef\b/.test(raw)) { inDefine = false; continue; } if (/^\s*define\b/.test(raw)) { inDefine = true; continue; } if (inDefine || raw.startsWith("\t")) continue; const assignment = MAKE_ASSIGNMENT.exec(raw.replace(/#.*$/, "").trim()); if (assignment === null) continue; const [, name, operator, value] = assignment; if (name === undefined || operator === undefined || value === undefined) continue; // `?=` assigns only when the name has no value yet, so the first one // wins where `:=` and `=` let the last one win. Overwriting here would // resolve the reference to a string make never uses. if (operator === "?=" && variables.has(name)) continue; // A value that is itself a reference is the recursive case, and is not // resolved. Recording it would hand the substitution below a string it // cannot finish expanding. if (/\$[({]/.test(value)) continue; variables.set(name, value.trim()); } return variables; }; // One pass, and only over names that were assigned a literal: the result is // never re-scanned for further references, so this cannot recurse or diverge. const expandMakeVariables = ( line: string, variables: Map, ): string => line.replace( MAKE_VARIABLE_REFERENCE, (reference, name: string) => variables.get(name) ?? reference, ); const parseMakefile = (text: string): Map => { const recipes = new Map(); // Collected in a pass of their own because make reads the whole file // before it runs anything: a recipe may spell a variable that the Makefile // assigns further down, and a single pass would leave that one unexpanded // purely because of where its author put it. const variables = makeVariables(text); let current: string | null = null; for (const line of text.split("\n")) { // Expanded before parsing rather than only on recipe lines, so a // prerequisite written `check: $(FMT)` resolves the same way a recipe // line does. Assignment lines are expanded too and are unaffected: a // literal value has nothing to substitute. const raw = expandMakeVariables(line, variables); if (raw.startsWith("\t")) { if (current !== null) { recipes .get(current) ?.recipe.push(raw.trim().replace(/^[@-]+/, "")); } continue; } if (MAKE_CONDITIONAL.test(raw)) continue; const target = new RegExp(`^(${MAKE_TARGET_NAME})\\s*:(?!=)(.*)$`).exec( raw, ); current = target === null ? null : (target[1] ?? null); if (target === null || current === null) continue; // `target: prereqs ; command` puts the first recipe line on the target // line itself. Read as prerequisites the whole of it, `;` and the // command split into tokens that name no target and are filtered out, // and the invocation disappears — `check: ; @script/fmt-check` was a // second prettier pass this parser scored as none. const rest = target[2] ?? ""; const semicolon = rest.indexOf(";"); const inlineRecipe = semicolon === -1 ? "" : rest.slice(semicolon + 1).trim(); // Trailing `# comment` is not a prerequisite; neither is the empty // string a split leaves behind. const prerequisites = ( semicolon === -1 ? rest : rest.slice(0, semicolon) ) .replace(/#.*$/, "") .trim() .split(/\s+/) .filter((name) => name !== ""); const existing = recipes.get(current); if (existing === undefined) { recipes.set(current, { prerequisites, recipe: [] }); } else { existing.prerequisites.push(...prerequisites); } if (inlineRecipe !== "") { recipes .get(current) ?.recipe.push(inlineRecipe.replace(/^[@-]+/, "")); } } return recipes; }; const recipes = parseMakefile(read("Makefile")); // Only prerequisites that name a target of this Makefile are followed: the // rest are filenames, or expansions of variables this parser does not // evaluate, and neither is an invocation of anything it could read. const prerequisitesOf = (node: string): string[] => { if (!node.startsWith("make:")) return []; const target = recipes.get(node.slice("make:".length)); if (target === undefined) return []; return target.prerequisites .filter((name) => recipes.has(name)) .map((name) => `make:${name}`); }; const packageScripts = (): Record => { const pkg = JSON.parse(read("package.json")) as { scripts?: Record; }; return pkg.scripts ?? {}; }; const scripts = packageScripts(); // A `RUN < { const commands: string[] = []; let terminator: string | null = null; for (const line of executable(text)) { if (terminator !== null) { if (line === terminator) terminator = null; else commands.push(line); continue; } if (!line.startsWith("RUN ")) continue; const command = line.slice("RUN ".length); terminator = HEREDOC_OPEN.exec(command)?.[2] ?? null; commands.push(command); } if (terminator !== null) { throw new Error(`unterminated heredoc: ${terminator}`); } return commands; }; // Node keys: `script/`, `docker:`, `make:`, // `yarn:`, `workflow:`. const resolve = (node: string): string[] => { if (node.startsWith("script/")) return executable(read(node)); if (node.startsWith("docker:")) { return dockerRunCommands(read(node.slice("docker:".length))); } // The `run:` steps of a workflow, in file order. `uses:` steps are actions, // not commands, and have no edges into this repo's graph. A `run: |` block // would resolve to the bare `|`, which reaches nothing and therefore fails // the count rather than passing quietly. if (node.startsWith("workflow:")) { return executable(read(node.slice("workflow:".length))) .filter((line) => /^-?\s*run:\s*\S/.test(line)) .map((line) => line.replace(/^-?\s*run:\s*/, "")); } 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.recipe; } if (node.startsWith("yarn:")) { const name = node.slice("yarn:".length); const script = scripts[name]; if (script === undefined) { throw new Error(`no such package.json script: ${name}`); } return [script]; } throw new Error(`unresolvable node: ${node}`); }; // Same reasoning as the missing-target error, applied to every node kind: a // node that resolves to no commands contributes zero prettier invocations and // zero edges, which is indistinguishable from a clean result. Fail instead. // // A Makefile target with prerequisites and an empty recipe is the one case // that is genuinely not vacuous — it runs its prerequisites — so it is not // caught here. const commandsOf = (node: string): string[] => { const commands = resolve(node); if (commands.length === 0 && prerequisitesOf(node).length === 0) { throw new Error(`node resolved to no commands: ${node}`); } return commands; }; 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( new RegExp( `(?:\\$SCRIPT_DIR|\\$\\{SCRIPT_DIR\\}|script)/(${MAKE_TARGET_NAME})`, "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. // // `$(MAKE)` and `${MAKE}` count as `make`. Recursive make is spelled that // way by convention rather than as a literal `make`, and this Makefile // already expands variables into recipes (`@$(YARN) tsc --watch`), so a // case-sensitive literal-only match left `@$(MAKE) fmt-check` — the other // one-token way to put the host prettier pass back into `make check` — // unfollowed. Valueless flags between the command and the target (`make // -n check`, `make -j4 check`) are skipped. A flag that takes a separate // argument is not: `make -C sub build` runs `sub/Makefile`'s target, not // this one's, and resolving it against these recipes would be wrong // rather than merely incomplete. for (const match of line.matchAll( new RegExp( `(?:\\$\\(MAKE\\)|\\$\\{MAKE\\}|\\bmake)(?:\\s+-\\S+)*\\s+(${MAKE_TARGET_NAME})`, "g", ), )) { if (recipes.has(match[1] ?? "")) edges.push(`make:${match[1]}`); } // Same rule for yarn: `yarn run prettier` is the linter itself (counted, // not followed), `yarn run fmt-check` would be a package.json script that // runs it indirectly. // // The name charset is wider than the Makefile's because `package.json` // script names conventionally carry colons, digits and underscores // (`lint:fmt`, `test:e2e`). Under the narrower charset `yarn run lint:fmt` // read as `yarn run lint`, which is not a script, so it produced no edge // and no count. for (const match of line.matchAll( /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/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. // // Every spelling of the file flag is read, not just `-f `. Docker // accepts `--file `, `--file=`, `-f=` and the value // attached to the short flag as one token (`-f`) for the same thing, // and matching `-f ` alone resolved all of them to the default // `Dockerfile` edge — so `docker build --file=Dockerfile.lint .` in a // recipe was followed into the wrong file, counted nothing, and reported // green. The flag is searched for anywhere in the invocation, as `-f` // already was, because a real build line wraps it in `--build-arg` and // other flags. A leading `\s` is required so that a longer flag ending in // the same letters cannot supply the match, and the attached form is // allowed only for the short flag, so that `--force-rm` — a long flag that // merely starts with the same letters — still does not read as one. if (/\bdocker\s+build\b/.test(line)) { const file = /\s(?:--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. // // Counting and edge-following both happen for every line: a line that invokes // prettier can also invoke something else, and skipping the edges of counted // lines silently truncated the graph. 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); // Before the recipe, exactly as make does. for (const edge of prerequisitesOf(node)) { walk(edge, [...path, node], result); } for (const line of commandsOf(node)) { result.prettier += countPrettier(line); 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("prettier runs exactly once per CI build", () => { // Rooted at the workflow file, so this is the graph CI executes rather than // the graph someone believed CI executes. `make check` cannot stand in for // it: CI runs script/cibuild, which builds Dockerfile as well as // Dockerfile.lint, and nothing under `make check` ever reads Dockerfile. const ci = walk("workflow:.gitea/workflows/check.yml"); it("invokes prettier once for the whole CI build", () => { expect(ci.prettier).toBe(1); }); // script/cibuild is here because the workflow is asserted to run it; // Dockerfile is here because it is the half of the CI graph that the // `make check` walk cannot see. it.each([ "script/cibuild", "script/lint", "docker:Dockerfile.lint", "docker:Dockerfile", ])("reaches %s while counting", (node) => { expect([...ci.reached]).toContain(node); }); // The test and build image must not lint: linting is Dockerfile.lint's job, // and a prettier step added here would be a second pass over the same tree // for the same verdict — on the one path where it matters most. it("keeps prettier out of the test and build image", () => { expect(walk("docker:Dockerfile").prettier).toBe(0); }); }); 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", ); }); }); // script/bootstrap installs the dependencies, and it has two install sites: one // for the case where yarn has to be reached through nvm, and one for the case // where yarn is already on PATH. A substring check against the whole file // cannot tell them apart, so it reports the first and says nothing about the // second — which is the one the containers take, because the pinned node image // ships yarn. Both are resolved separately here. const installBranches = (): { withoutYarn: string[]; withYarn: string[] } => { const lines = executable(read("script/bootstrap")); const open = lines.findIndex((line) => /^install_js_deps\s*\(\)/.test(line), ); if (open === -1) { throw new Error("script/bootstrap: no install_js_deps function"); } const close = lines.indexOf("}", open); const body = lines.slice(open + 1, close === -1 ? undefined : close); // The guard has to be a plain positive `missing yarn` test at the front of // the condition. `if ! missing yarn ...` is the same shape to a looser // regex but swaps which branch is which, and since the two branches are // asserted separately below, that would silently relabel them — a failing // test would then name the wrong branch. Anything else throws the named // error below, which is the loud failure this file prefers. const guard = body.findIndex((line) => /^if\s+missing yarn\b/.test(line)); const otherwise = body.indexOf("else", guard); const end = body.indexOf("fi", otherwise); if (guard === -1 || otherwise === -1 || end === -1) { throw new Error( "script/bootstrap: install_js_deps is not the expected " + "if missing yarn / else / fi shape", ); } return { withoutYarn: body.slice(guard + 1, otherwise), withYarn: body.slice(otherwise + 1, end), }; }; // Every `yarn install` in the given lines, with its flags, so an unpinned // install cannot hide next to a pinned one. const yarnInstalls = (lines: string[]): string[] => lines.flatMap((line) => [...line.matchAll(/\byarn install\b[^"'&|;]*/g)].map((match) => match[0].trim(), ), ); 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 from the lockfile on the branch the container takes", () => { // Both images are FROM a node image, which ships yarn, so `missing // yarn` is false and this is the branch that runs in the container. const installs = yarnInstalls(installBranches().withYarn); expect(installs).not.toHaveLength(0); for (const install of installs) { expect(install).toContain("--frozen-lockfile"); } }); it("installs from the lockfile on the nvm branch too", () => { // Not the container's branch, but it is the one a developer without // yarn on PATH gets, and their prettier has to match the container's. const installs = yarnInstalls(installBranches().withoutYarn); expect(installs).not.toHaveLength(0); for (const install of installs) { expect(install).toContain("--frozen-lockfile"); } }); it("runs script/bootstrap inside the lint container", () => { // Without this the lockfile assertions above would be about a script // the container never executes. expect([...walk("docker:Dockerfile.lint").reached]).toContain( "script/bootstrap", ); }); 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"); }); }); describe("the walk cannot pass vacuously", () => { // An earlier draft of this file computed a Makefile target as // `node.slice("make:")` — a string where a number belongs, which coerces to // NaN and made every target resolve to nothing. The count went to zero and // an assertion of "not twice" would have been satisfied by a walk that had // read nothing at all. Every way of reaching nothing is therefore an // error here, and the ways are tested rather than assumed. it("reports zero for a subgraph that does not run prettier", () => { expect(walk("make:clean").prettier).toBe(0); }); it("refuses a Makefile target that does not exist", () => { expect(() => walk("make:no-such-target")).toThrow( /no such Makefile target/, ); }); it("refuses a package.json script that does not exist", () => { expect(() => walk("yarn:no-such-script")).toThrow( /no such package.json script/, ); }); it("refuses a script that does not exist", () => { expect(() => walk("script/no-such-script")).toThrow(/ENOENT/); }); it("refuses a node that resolves to no commands", () => { // .dockerignore has no RUN steps, standing in for a Dockerfile whose // steps a restructure moved somewhere the resolver cannot see. expect(() => walk("docker:.dockerignore")).toThrow( /resolved to no commands/, ); }); it("refuses a node kind it does not understand", () => { expect(() => walk("nonsense")).toThrow(/unresolvable node/); }); it("refuses to walk in circles", () => { expect(() => walk("make:check", ["script/check"])).toThrow( /invocation cycle/, ); }); }); describe("the resolver reads what the shell would run", () => { // Counting per line is how `yarn run prettier --check . && yarn run // prettier --check src` read as a single invocation. it("counts every prettier invocation on a line", () => { expect( countPrettier( "yarn run prettier --check . && yarn run prettier --check src", ), ).toBe(2); }); it("does not count the config files as invocations", () => { expect(countPrettier("COPY .prettierrc .prettierignore ./")).toBe(0); }); // The counting `continue` also dropped every edge that shared a line with a // prettier call, so a whole subtree could be hidden behind one `&&`. it("still follows the edges of a line that invokes prettier", () => { expect( edgesOf('yarn run prettier --check . && "$SCRIPT_DIR/lint"'), ).toContain("script/lint"); }); it("resolves every spelling of a script call to one node", () => { expect( edgesOf('"$SCRIPT_DIR/lint" "${SCRIPT_DIR}/test" script/fmt'), ).toEqual(["script/lint", "script/test", "script/fmt"]); }); // The two edges the previous version of this file could not see, each // pinned directly as well as through the graph. Both were reproduced as // Makefile mutations that gave `make check` two prettier passes with the // whole suite green. it("follows a Makefile prerequisite as an invocation", () => { // Not hypothetical: this is the shipped Makefile's own `install`. expect(prerequisitesOf("make:install")).toEqual(["make:build-bin"]); expect(prerequisitesOf("make:check")).toEqual([]); }); it("treats $(MAKE) and ${MAKE} as make", () => { expect(edgesOf("$(MAKE) fmt-check")).toContain("make:fmt-check"); expect(edgesOf("${MAKE} fmt-check")).toContain("make:fmt-check"); expect(edgesOf("make fmt-check")).toContain("make:fmt-check"); expect(edgesOf("make -n fmt-check")).toContain("make:fmt-check"); }); // Two more constructs that gave `make check` a second prettier pass with // the whole suite green, each reproduced as a Makefile mutation before // being pinned here. it("keeps recipe lines inside a make conditional", () => { const recipes = parseMakefile( [ "check:", "\t@script/check", "ifeq (1,1)", "\t@script/fmt-check", "endif", ].join("\n"), ); // What `make -n check` prints, in order. expect(recipes.get("check")?.recipe).toEqual([ "script/check", "script/fmt-check", ]); }); it("reads a recipe written on the target line after a semicolon", () => { const recipes = parseMakefile( ["check: ; @script/fmt-check", "\t@script/check"].join("\n"), ); expect(recipes.get("check")?.recipe).toEqual([ "script/fmt-check", "script/check", ]); // And the `;` and the command are not mistaken for prerequisites. expect(recipes.get("check")?.prerequisites).toEqual([]); expect( parseMakefile("check: lint ; @script/fmt-check").get("check"), ).toEqual({ prerequisites: ["lint"], recipe: ["script/fmt-check"], }); }); // The fifth construct that gave `make check` two prettier passes with the // whole suite green: `FMT := script/fmt-check` and a recipe of `@$(FMT)`. // The shipped Makefile writes recipe lines this way already // (`@$(YARN) tsc --watch`), so an unexpanded variable was a gap in the // house style rather than in an exotic corner of make. it("expands a variable spelled into a recipe line", () => { const recipes = parseMakefile( [ "FMT := script/fmt-check", "check:", "\t@$(FMT)", "\t@${FMT}", "\t@script/check", ].join("\n"), ); // What `make -n check` prints, in order. expect(recipes.get("check")?.recipe).toEqual([ "script/fmt-check", "script/fmt-check", "script/check", ]); }); it("expands a variable spelled into a prerequisite list", () => { expect( parseMakefile("FMT = fmt-check\ncheck: $(FMT)").get("check") ?.prerequisites, ).toEqual(["fmt-check"]); }); // A recipe may name a variable the Makefile assigns further down. it("expands a variable assigned after the recipe that uses it", () => { const recipes = parseMakefile( ["check:", "\t@$(FMT)", "FMT ?= script/fmt-check"].join("\n"), ); expect(recipes.get("check")?.recipe).toEqual(["script/fmt-check"]); }); // `export FMT := script/fmt-check` is an assignment make honours and the // pattern anchored at the name, so the whole line read as neither an // assignment nor a target and `@$(FMT)` stood unresolved: `make -n check` // printed `script/check` and `script/fmt-check` while this file scored // one. `override` reaches the same place by the same route. it("expands a variable assigned through an export or override prefix", () => { expect( parseMakefile( [ "export FMT := script/fmt-check", "check:", "\t@$(FMT)", "\t@script/check", ].join("\n"), ).get("check")?.recipe, ).toEqual(["script/fmt-check", "script/check"]); expect( parseMakefile( ["override FMT = script/fmt-check", "check:", "\t@$(FMT)"].join( "\n", ), ).get("check")?.recipe, ).toEqual(["script/fmt-check"]); // `export` on its own line names a variable without assigning one, and // is not read as an assignment of the empty string. expect( parseMakefile( [ "FMT := script/fmt-check", "export FMT", "check:", "\t@$(FMT)", ].join("\n"), ).get("check")?.recipe, ).toEqual(["script/fmt-check"]); }); // `?=` assigns only when the name has no value yet, so among assignments // to one name the first `?=` wins where `:=` and `=` let the last one win. // Reading every operator as last-wins resolved `@$(FMT)` to the value make // discards: the file counted an invocation that never happens and missed // the one that does, with the suite green either way. it("keeps the first value when a later assignment is conditional", () => { expect( parseMakefile( [ "FMT := script/fmt-check", "FMT ?= script/build", "check:", "\t@$(FMT)", ].join("\n"), ).get("check")?.recipe, ).toEqual(["script/fmt-check"]); // A `?=` that is itself the first assignment does assign, and a `?=` // after it does not. expect( parseMakefile( [ "FMT ?= script/fmt-check", "FMT ?= script/build", "check:", "\t@$(FMT)", ].join("\n"), ).get("check")?.recipe, ).toEqual(["script/fmt-check"]); // `:=` and `=` stay last-wins, which is what make does. expect( parseMakefile( [ "FMT := script/build", "FMT := script/fmt-check", "check:", "\t@$(FMT)", ].join("\n"), ).get("check")?.recipe, ).toEqual(["script/fmt-check"]); }); // The edges of the expansion, asserted so the header's not-followed list // is the code's behaviour rather than a claim about it. Each of these // leaves the reference standing verbatim, which reaches nothing — the same // dead end as any other unfollowed edge, and not a wrong resolution. it("leaves a value it cannot resolve to a literal unexpanded", () => { // A make function: resolving it would mean running it. expect( parseMakefile( [ "FMT := $(shell echo script/fmt-check)", "check:", "\t@$(FMT)", ].join("\n"), ).get("check")?.recipe, ).toEqual(["$(FMT)"]); // A `define` body is a multi-line value, not a single-line assignment. expect( parseMakefile( [ "define RUNFMT", "@script/fmt-check", "endef", "check:", "\t$(RUNFMT)", ].join("\n"), ).get("check")?.recipe, ).toEqual(["$(RUNFMT)"]); // An undefined name is not silently emptied. expect( parseMakefile(["check:", "\t@$(NOPE)"].join("\n")).get("check") ?.recipe, ).toEqual(["$(NOPE)"]); }); it("reads a package.json script name that contains a colon", () => { // The lookup against `scripts` is what turns this into an edge, and no // script in this repo is colon-named, so the charset is asserted on // the extraction itself. expect( [ ..."yarn run lint:fmt".matchAll( /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g, ), ].map((match) => match[1]), ).toEqual(["lint:fmt"]); }); it("reads the body of a BuildKit heredoc RUN step", () => { expect( dockerRunCommands( [ "FROM scratch", "RUN < { expect(() => dockerRunCommands("FROM scratch\nRUN < { expect(edgesOf("docker build .")).toContain("docker:Dockerfile"); expect(edgesOf("docker build -f Dockerfile.lint .")).toContain( "docker:Dockerfile.lint", ); }); // `-f` is not the only spelling of the flag. `--file=Dockerfile.lint` and // `--file Dockerfile.lint` mean exactly what `-f Dockerfile.lint` means, // and matching only `-f` sent both to the default `Dockerfile` edge // instead. That is the false-green shape this whole file exists to // prevent: a second prettier pass added as // `docker build --file=Dockerfile.lint .` was followed into the wrong // Dockerfile, counted nothing, and left every build green. The long form // is an ordinary thing for a human to write, so it is followed rather than // merely documented as unfollowed. // // The equality rather than containment matters: resolving to the right // file is only half of it, the default edge must not also be emitted. it("follows --file in all three spellings as it does -f", () => { expect(edgesOf("docker build --file=Dockerfile.lint .")).toEqual([ "docker:Dockerfile.lint", ]); expect(edgesOf("docker build --file Dockerfile.lint .")).toEqual([ "docker:Dockerfile.lint", ]); expect(edgesOf("docker build -f=Dockerfile.lint .")).toEqual([ "docker:Dockerfile.lint", ]); }); // The value may be attached to the short flag with no separator at all: // `-fDockerfile.lint` is a single token, and the flag parser reads it as // `-f` naming that file, exactly as `-f Dockerfile.lint` does. Requiring a // separator sent it to the default `Dockerfile` edge instead — the same // false green as the long spellings above, and one the header promised was // followed: a second prettier pass added as `docker build // -fDockerfile.lint .` was resolved into the wrong file, counted nothing, // and left the build green. `-fFILE` is an ordinary thing for a human to // write, so it is followed rather than merely documented as unfollowed. it("follows a short file flag with its value attached", () => { expect(edgesOf("docker build -fDockerfile.lint .")).toEqual([ "docker:Dockerfile.lint", ]); }); // The flag is found anywhere in the invocation rather than at a fixed // position, matching how `-f` was already read, since a real build line // carries `--build-arg` and friends around it. it("finds the file flag after other flags and build args", () => { expect( edgesOf( 'docker build --build-arg LINT_EPOCH="1" --file=Dockerfile.lint --progress=plain .', ), ).toEqual(["docker:Dockerfile.lint"]); }); // A different flag that merely begins with the same letters is not the // file flag: `--force-rm` must not read as one, or its next token would be // resolved as a Dockerfile and thrown on as unreadable. it("does not mistake --force-rm for the file flag", () => { expect(edgesOf("docker build --force-rm .")).toEqual([ "docker:Dockerfile", ]); }); // The header says a bundled short flag cluster resolves to the default // rather than to the file it names, which is a limitation and so has to be // pinned: an unpinned limitation is how the header starts overclaiming // again. If someone teaches the resolver to split clusters, this test is // the one that tells them to correct the header too. it("resolves a bundled -qf cluster to the default Dockerfile", () => { expect(edgesOf("docker build -qf Dockerfile.lint .")).toEqual([ "docker:Dockerfile", ]); }); // This exact-equality assertion is also the block-scalar guard, which is // not obvious from its name: a `run: |` step resolves to the bare `|`, // which reaches nothing, so the prettier count would stay 1 no matter what // the block contains. Pinning the resolved list is what makes such a step // red. The cost is that any legitimate second `run:` step — a cache step, // an `echo` — fails this test for a reason unrelated to prettier. it("reads the run steps of the CI workflow and not its uses steps", () => { expect(commandsOf("workflow:.gitea/workflows/check.yml")).toEqual([ "script/cibuild", ]); }); });