f4ecef8820
All checks were successful
check / check (push) Successful in 15s
makeRecipes() reset the current target on every non-tab line, so an ifeq/endif block ended the recipe and every tab-indented line inside it was discarded; and the target line's tail was read entirely as prerequisites, so `check: ; @script/fmt-check` split to tokens that named no target and vanished. Both gave `make check` a second prettier pass with the suite green. Conditional directives no longer end a recipe, and every branch is treated as reachable rather than evaluating the condition. The target line is split on the first `;`: what precedes it is the prerequisite list, what follows is the first recipe line. Both pinned directly, and the header's exclusion list now names what the parser actually skips.
762 lines
33 KiB
TypeScript
762 lines
33 KiB
TypeScript
// `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/<name>"` and `script/<name>` into other scripts, `make <target>`
|
|
// and `$(MAKE) <target>` through the Makefile shims, the prerequisites of a
|
|
// Makefile target, `yarn run <name>` through the `package.json` scripts, and
|
|
// `docker build -f <file>` into that Dockerfile's `RUN` steps, 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. 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 <target>` 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/;
|
|
|
|
const parseMakefile = (text: string): Map<string, MakeTarget> => {
|
|
const recipes = new Map<string, MakeTarget>();
|
|
let current: string | null = null;
|
|
for (const raw of text.split("\n")) {
|
|
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<string, string> => {
|
|
const pkg = JSON.parse(read("package.json")) as {
|
|
scripts?: Record<string, string>;
|
|
};
|
|
return pkg.scripts ?? {};
|
|
};
|
|
|
|
const scripts = packageScripts();
|
|
|
|
// A `RUN <<EOF` body is a command the image executes, and BuildKit runs it on
|
|
// the default frontend with no `# syntax=` directive, so keeping only the
|
|
// lines that begin `RUN ` would let a whole shell script hide one line below
|
|
// one. The body is returned alongside the `RUN` line that opened it. An
|
|
// unterminated heredoc is a thrown error rather than a silent truncation of
|
|
// the rest of the file, for the same reason every other dead end here is.
|
|
const HEREDOC_OPEN = /<<-?\s*(['"]?)([A-Za-z_][A-Za-z0-9_]*)\1/;
|
|
|
|
const dockerRunCommands = (text: string): string[] => {
|
|
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/<name>`, `docker:<Dockerfile>`, `make:<target>`,
|
|
// `yarn:<package.json script>`, `workflow:<CI workflow file>`.
|
|
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.
|
|
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<string>;
|
|
}
|
|
|
|
// 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<string>() };
|
|
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<string, string>;
|
|
};
|
|
// 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"],
|
|
});
|
|
});
|
|
|
|
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 <<EOF",
|
|
"yarn run prettier --check .",
|
|
"EOF",
|
|
"RUN echo done",
|
|
].join("\n"),
|
|
),
|
|
).toContain("yarn run prettier --check .");
|
|
});
|
|
|
|
it("refuses a heredoc that is never terminated", () => {
|
|
expect(() =>
|
|
dockerRunCommands("FROM scratch\nRUN <<EOF\nyarn run prettier .\n"),
|
|
).toThrow(/unterminated heredoc/);
|
|
});
|
|
|
|
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",
|
|
);
|
|
});
|
|
|
|
// 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",
|
|
]);
|
|
});
|
|
});
|