990 lines
43 KiB
TypeScript
990 lines
43 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. 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. 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/;
|
|
|
|
// `@$(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.
|
|
// `:=` and `=` differ in when make expands them and `?=` in whether it assigns
|
|
// at all, but none of that changes the single value a literal can take, so all
|
|
// three are read the same way. 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<string, string> => {
|
|
const variables = new Map<string, string>();
|
|
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) continue;
|
|
if (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, string>,
|
|
): string =>
|
|
line.replace(
|
|
MAKE_VARIABLE_REFERENCE,
|
|
(reference, name: string) => variables.get(name) ?? reference,
|
|
);
|
|
|
|
const parseMakefile = (text: string): Map<string, MakeTarget> => {
|
|
const recipes = new Map<string, MakeTarget>();
|
|
// 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<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"],
|
|
});
|
|
});
|
|
|
|
// 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 <<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",
|
|
]);
|
|
});
|
|
});
|