diff --git a/TODO.md b/TODO.md index 0a73af7..efc43fb 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,22 @@ Update the README API reference section to match the current implementation. # Completed Steps +- 2026-08-10: Made `lint-once.test.ts` enforce what its header claims. It walked + `make check` only, so it never read `Dockerfile` — the image CI builds through + `script/cibuild` — and a second `prettier --check .` could be added there with + the suite staying green. The walk now also starts at + `.gitea/workflows/check.yml` and follows its `run:` steps, so the graph under + test is the one CI executes rather than the one someone assumed it executes. + The lockfile assertion was a substring check against the whole of + `script/bootstrap`, which has two install sites and so reported the branch the + containers never take; the two branches are now resolved separately and every + `yarn install` in each is required to be `--frozen-lockfile`. Prettier is + counted per occurrence instead of per line, so two invocations chained with + `&&` no longer read as one, and edges are followed on counted lines instead of + being skipped. Every way for the walk to reach nothing — an unknown target, an + unknown script, a missing file, a node with no commands, an unknown node kind + — is a thrown error rather than a quiet zero. Every assertion in the file was + mutation-tested individually. - 2026-08-10: Stopped `make check` running `prettier --check .` twice. Since linting moved into Docker, the duplicate was one container pass and one host pass of the same check: `script/lint` builds `Dockerfile.lint`, which runs diff --git a/test/packaging/entrypoints.test.ts b/test/packaging/entrypoints.test.ts index 3a09f64..58149c5 100644 --- a/test/packaging/entrypoints.test.ts +++ b/test/packaging/entrypoints.test.ts @@ -1,7 +1,9 @@ // The package manifest promises three files that only exist after a build: // `main`, `types`, and the `quak` binary. Nothing in the test suite used to -// look at them, and `make check` runs test, lint and fmt-check but never the -// build, so `tsconfig.json` and `package.json` were free to drift apart. They +// look at them, and `make check` runs the suite and the lint container but +// never the build, so `tsconfig.json` and `package.json` were free to drift +// apart. (The formatting check is part of the lint container, not a step of +// its own; `test/packaging/lint-once.test.ts` is what holds that shape.) They // did: `rootDir` was `./src` while `include` also pulled in `bin/**/*`, which // is TS6059, and no build had succeeded for as long as that was true. // diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts index 527fb3e..16fbad1 100644 --- a/test/packaging/lint-once.test.ts +++ b/test/packaging/lint-once.test.ts @@ -14,17 +14,26 @@ // // 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. +// actually uses to reach another command — `run:` steps in the CI workflow, +// `"$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". +// 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"; @@ -62,6 +71,15 @@ const executable = (text: string): string[] => (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. @@ -96,14 +114,23 @@ const packageScripts = (): Record => { const scripts = packageScripts(); // Node keys: `script/`, `docker:`, `make:`, -// `yarn:`. -const commandsOf = (node: string): string[] => { +// `yarn:`, `workflow:`. +const resolve = (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)); } + // 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); @@ -116,11 +143,27 @@ const commandsOf = (node: string): string[] => { return recipe; } if (node.startsWith("yarn:")) { - return [scripts[node.slice("yarn:".length)] ?? ""]; + 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. +const commandsOf = (node: string): string[] => { + const commands = resolve(node); + if (commands.length === 0) { + throw new Error(`node resolved to no commands: ${node}`); + } + return commands; +}; + const edgesOf = (line: string): string[] => { const edges: string[] = []; @@ -135,14 +178,14 @@ const edgesOf = (line: string): string[] => { // 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]}`); + 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. + // 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. 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]}`); + if ((match[1] ?? "") in scripts) edges.push(`yarn:${match[1]}`); } // The container lint pass lives behind a `docker build`; without following @@ -163,6 +206,10 @@ interface Walk { // 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)) { @@ -171,10 +218,7 @@ const walk = (node: string, path: string[] = [], into?: Walk): Walk => { result.reached.add(node); for (const line of commandsOf(node)) { - if (/\bprettier\b/.test(line)) { - result.prettier += 1; - continue; - } + result.prettier += countPrettier(line); for (const edge of edgesOf(line)) { walk(edge, [...path, node], result); } @@ -213,6 +257,37 @@ describe("prettier runs exactly once per make 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 @@ -242,6 +317,48 @@ describe("script/precommit", () => { }); }); +// 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); + const guard = body.findIndex((line) => + /^if\b.*\bmissing 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 @@ -258,10 +375,31 @@ describe("host and container prettier cannot disagree", () => { 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("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", ); }); @@ -274,3 +412,92 @@ describe("host and container prettier cannot disagree", () => { 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"]); + }); + + it("follows a bare docker build to Dockerfile and -f to its file", () => { + expect(edgesOf("docker build .")).toContain("docker:Dockerfile"); + expect(edgesOf("docker build -f Dockerfile.lint .")).toContain( + "docker:Dockerfile.lint", + ); + }); + + it("reads the run steps of the CI workflow and not its uses steps", () => { + expect(commandsOf("workflow:.gitea/workflows/check.yml")).toEqual([ + "script/cibuild", + ]); + }); +});