Make lint and test phases of the Dockerfile (closes #96)
check / check (push) Successful in 33s
check / check (push) Successful in 33s
Follows the template: Dockerfile.lint is gone; the Dockerfile has a lint phase (eslint, prettier --check .) and a test phase (vitest, run as the node user so the not-writable-directory tests are not skipped), and its last stage compiles and depends on both. script/lint and script/test build one phase each with --no-cache; script/docker and script/cibuild pass --no-cache, so CHECK_EPOCH and LINT_EPOCH are removed. script/cibuild is the single image build, so CI runs lint and the tests once each. The tests that checked the old layout are deleted, REPO_POLICIES.md is re-copied and the README describes the new layout. Model: opus-5-5
This commit is contained in:
@@ -2,13 +2,13 @@
|
||||
// failures are silent.
|
||||
//
|
||||
// Excluding too little: a worktree left under `.claude/` is copied into the
|
||||
// image, vitest globs its `test/` tree as well as the real one, and the
|
||||
// containerised `make check` runs the whole suite twice over while reporting
|
||||
// success. A compiled `bin/quak` is ~100 MB of context nobody needs.
|
||||
// image, vitest globs its `test/` tree as well as the real one, and the test
|
||||
// phase runs the whole suite twice over while reporting success. A compiled
|
||||
// `bin/quak` is ~100 MB of context nobody needs.
|
||||
//
|
||||
// Excluding too much: Prettier 3 reads `.gitignore` as a default ignore file,
|
||||
// so dropping it from the context silently changes which files
|
||||
// `make fmt-check` looks at inside the image compared to the host.
|
||||
// so dropping it from the context silently changes which files the lint
|
||||
// phase's prettier check looks at compared to `make fmt-check` on the host.
|
||||
//
|
||||
// Neither shows up as a build failure, so they are asserted here.
|
||||
import { describe, expect, it } from "vitest";
|
||||
@@ -47,18 +47,13 @@ describe(".dockerignore", () => {
|
||||
expect(dockerignore).not.toContain(".gitignore");
|
||||
});
|
||||
|
||||
// Both images are built from this same context, and the lint image runs
|
||||
// eslint and prettier across it. BuildKit lets a `<dockerfile>.dockerignore`
|
||||
// shadow the root one for a single build; such a file would silently give
|
||||
// the lint build a different, unreviewed context — and eslint's flat config
|
||||
// does not ignore dot-directories, so a stray `.claude/` worktree would be
|
||||
// linted.
|
||||
it.each(["Dockerfile", "Dockerfile.lint"])(
|
||||
"is not shadowed by a per-Dockerfile ignore file for %s",
|
||||
(name) => {
|
||||
expect(existsSync(join(repoRoot, `${name}.dockerignore`))).toBe(
|
||||
false,
|
||||
);
|
||||
},
|
||||
);
|
||||
// BuildKit lets a `Dockerfile.dockerignore` shadow the root one; such a
|
||||
// file would silently give the build a different, unreviewed context —
|
||||
// and eslint's flat config does not ignore dot-directories, so a stray
|
||||
// `.claude/` worktree would be linted.
|
||||
it("is not shadowed by a Dockerfile.dockerignore", () => {
|
||||
expect(existsSync(join(repoRoot, "Dockerfile.dockerignore"))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,9 +1,7 @@
|
||||
// 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 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
|
||||
// look at them, and `make check` runs the test and lint phases but never the
|
||||
// build, so `tsconfig.json` and `package.json` were free to drift apart. 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.
|
||||
//
|
||||
|
||||
@@ -1,184 +0,0 @@
|
||||
// Linting runs in Docker, one way, everywhere: `script/lint` builds
|
||||
// `Dockerfile.lint`, which COPYs the repo into a digest-pinned image and runs
|
||||
// eslint and prettier as build steps, so a successful build IS a clean lint.
|
||||
//
|
||||
// Three things can quietly undo that, and none of them shows up as a build
|
||||
// failure, which is why they are asserted here:
|
||||
//
|
||||
// 1. Recursion. `script/check` calls `script/lint`, and `script/lint` is now a
|
||||
// `docker build`. Anything that runs `make check` inside a container is
|
||||
// therefore asking for Docker inside Docker, and CI breaks. The image built
|
||||
// from `Dockerfile` runs the suite and the compile only; lint happens once,
|
||||
// in `Dockerfile.lint`.
|
||||
// 2. Cache. A lint build over an unchanged tree returns success in well under a
|
||||
// second having linted nothing. The `LINT_EPOCH` guard is what forces the
|
||||
// linter layers to execute, and it has to fail closed: an unset build
|
||||
// argument is the empty string, which is a perfectly stable cache key, so an
|
||||
// invocation that omits it must be rejected rather than served a cached
|
||||
// green.
|
||||
// 3. A host lint path surviving alongside the container one, which would let a
|
||||
// lint result come from an unpinned local toolchain.
|
||||
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");
|
||||
|
||||
// The executable lines of a shell script or Dockerfile: comments carry the
|
||||
// reasoning and frequently name the very commands these tests forbid, so they
|
||||
// would otherwise trigger every assertion below.
|
||||
const instructions = (name: string): string[] =>
|
||||
read(name)
|
||||
.split("\n")
|
||||
.map((line) => line.trim())
|
||||
.filter((line) => line !== "" && !line.startsWith("#"));
|
||||
|
||||
const lintScript = instructions("script/lint");
|
||||
const dockerfileLint = instructions("Dockerfile.lint");
|
||||
const dockerfile = instructions("Dockerfile");
|
||||
const cibuild = instructions("script/cibuild");
|
||||
|
||||
const has = (lines: string[], pattern: RegExp): boolean =>
|
||||
lines.some((line) => pattern.test(line));
|
||||
|
||||
describe("script/lint", () => {
|
||||
it("lints by building Dockerfile.lint", () => {
|
||||
expect(has(lintScript, /docker build .*-f Dockerfile\.lint/)).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
// The whole point of the ruling: no invocation of a linter against the
|
||||
// working tree survives, so a lint verdict can only come from the pinned
|
||||
// image.
|
||||
it("runs no linter on the host", () => {
|
||||
expect(has(lintScript, /eslint|prettier/)).toBe(false);
|
||||
});
|
||||
|
||||
// Without a fresh epoch the build is served from cache in under a second,
|
||||
// having linted nothing, and still exits 0.
|
||||
it("passes a fresh LINT_EPOCH on every run", () => {
|
||||
expect(
|
||||
has(lintScript, /--build-arg LINT_EPOCH="\$\(date \+%s\)"/),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Dockerfile.lint", () => {
|
||||
// Tag references are server-mutable, so they are remote code execution.
|
||||
it("pins its base image by digest", () => {
|
||||
expect(has(dockerfileLint, /^FROM \S+@sha256:[0-9a-f]{64}/)).toBe(true);
|
||||
});
|
||||
|
||||
it("runs eslint as a build step", () => {
|
||||
expect(has(dockerfileLint, /^RUN .*eslint \./)).toBe(true);
|
||||
});
|
||||
|
||||
it("runs prettier as a build step", () => {
|
||||
expect(has(dockerfileLint, /^RUN .*prettier --check \./)).toBe(true);
|
||||
});
|
||||
|
||||
// An unset ARG is the empty string, and an empty string is a perfectly
|
||||
// stable cache key. Rejecting it is what stops a bare
|
||||
// `docker build -f Dockerfile.lint .` from reporting a green it did not
|
||||
// earn.
|
||||
it("refuses to build without LINT_EPOCH", () => {
|
||||
expect(has(dockerfileLint, /^ARG LINT_EPOCH$/)).toBe(true);
|
||||
expect(
|
||||
has(dockerfileLint, /^RUN \[ -n "\$LINT_EPOCH" \] \|\| exit 1$/),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
// The guard only forces execution of the layers below it, so both linters
|
||||
// have to sit after it. Layer order is the mechanism, not a style choice.
|
||||
it("puts both linters below the epoch guard", () => {
|
||||
const guard = dockerfileLint.findIndex((line) =>
|
||||
/^RUN \[ -n "\$LINT_EPOCH" \]/.test(line),
|
||||
);
|
||||
const linters = dockerfileLint
|
||||
.map((line, index) => ({ line, index }))
|
||||
.filter(({ line }) => /^RUN .*(eslint|prettier)/.test(line));
|
||||
|
||||
expect(linters.length).toBeGreaterThan(0);
|
||||
for (const { line, index } of linters) {
|
||||
expect(
|
||||
index,
|
||||
`${line} must run below the LINT_EPOCH guard`,
|
||||
).toBeGreaterThan(guard);
|
||||
}
|
||||
});
|
||||
|
||||
// Dependency installation is the slow layer and has nothing to do with the
|
||||
// sources, so it caches separately: manifests first, sources afterwards.
|
||||
it("copies the manifests before the sources", () => {
|
||||
const manifests = dockerfileLint.findIndex((line) =>
|
||||
/^COPY package\.json yarn\.lock/.test(line),
|
||||
);
|
||||
const sources = dockerfileLint.findIndex((line) =>
|
||||
/^COPY \. \.$/.test(line),
|
||||
);
|
||||
|
||||
expect(manifests).toBeGreaterThanOrEqual(0);
|
||||
expect(sources).toBeGreaterThan(manifests);
|
||||
});
|
||||
|
||||
// script/lint is a docker build; a lint step that shelled out to it would
|
||||
// recurse.
|
||||
it("does not call script/lint or make lint", () => {
|
||||
expect(has(dockerfileLint, /make lint|script\/lint/)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Dockerfile", () => {
|
||||
// `make check` runs script/lint, which is a docker build, so an image that
|
||||
// ran it would need a Docker daemon inside the container.
|
||||
it("does not run make check, make lint or script/lint", () => {
|
||||
expect(
|
||||
has(dockerfile, /make check|make lint|script\/(check|lint)/),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
// The replaced lint stage took a `COPY --from=lint` dependency to order
|
||||
// itself before the check stage. Dockerfile.lint is that stage now, and
|
||||
// two definitions of how to lint is one too many.
|
||||
it("has no lint stage", () => {
|
||||
expect(has(dockerfile, /AS lint\b|--from=lint\b/)).toBe(false);
|
||||
});
|
||||
|
||||
it("still runs the suite and the build under the epoch guard", () => {
|
||||
expect(has(dockerfile, /^RUN make test$/)).toBe(true);
|
||||
expect(has(dockerfile, /^RUN make build$/)).toBe(true);
|
||||
expect(
|
||||
has(dockerfile, /^RUN \[ -n "\$CHECK_EPOCH" \] \|\| exit 1$/),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("script/cibuild", () => {
|
||||
// CI has to get both verdicts. Lint goes first so the fast failure is
|
||||
// reported before the suite runs.
|
||||
it("builds the lint image before the test and build image", () => {
|
||||
const lint = cibuild.findIndex((line) => /\/lint"/.test(line));
|
||||
const check = cibuild.findIndex((line) =>
|
||||
/docker build .*CHECK_EPOCH/.test(line),
|
||||
);
|
||||
|
||||
expect(lint).toBeGreaterThanOrEqual(0);
|
||||
expect(check).toBeGreaterThan(lint);
|
||||
});
|
||||
});
|
||||
|
||||
describe("package.json", () => {
|
||||
// `yarn lint` was a second, unpinned way to get a lint verdict, from
|
||||
// whatever eslint the working tree happened to have installed.
|
||||
it("exposes no host lint script", () => {
|
||||
const pkg = JSON.parse(read("package.json")) as {
|
||||
scripts: Record<string, string>;
|
||||
};
|
||||
expect(pkg.scripts.lint).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -1,503 +0,0 @@
|
||||
// `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>`
|
||||
// through the Makefile shims, `yarn run <name>` through the `package.json`
|
||||
// scripts, and `docker build -f <file>` 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.
|
||||
//
|
||||
// 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.
|
||||
const makeRecipes = (): Map<string, string[]> => {
|
||||
const recipes = new Map<string, string[]>();
|
||||
let current: string | null = null;
|
||||
for (const raw of read("Makefile").split("\n")) {
|
||||
if (raw.startsWith("\t")) {
|
||||
if (current !== null) {
|
||||
recipes.get(current)?.push(raw.trim().replace(/^[@-]+/, ""));
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const target = /^([a-z][a-z-]*)\s*:(?!=)/.exec(raw);
|
||||
current = target === null ? null : target[1];
|
||||
if (current !== null && !recipes.has(current)) {
|
||||
recipes.set(current, []);
|
||||
}
|
||||
}
|
||||
return recipes;
|
||||
};
|
||||
|
||||
const recipes = makeRecipes();
|
||||
|
||||
const packageScripts = (): Record<string, string> => {
|
||||
const pkg = JSON.parse(read("package.json")) as {
|
||||
scripts?: Record<string, string>;
|
||||
};
|
||||
return pkg.scripts ?? {};
|
||||
};
|
||||
|
||||
const scripts = packageScripts();
|
||||
|
||||
// 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 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);
|
||||
// A renamed or deleted target must be a loud failure: silently walking
|
||||
// an empty recipe would report zero prettier invocations, which reads
|
||||
// like the tidiest possible result.
|
||||
if (recipe === undefined) {
|
||||
throw new Error(`no such Makefile target: ${target}`);
|
||||
}
|
||||
return recipe;
|
||||
}
|
||||
if (node.startsWith("yarn:")) {
|
||||
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[] = [];
|
||||
|
||||
// `"$SCRIPT_DIR/lint"`, `"$ROOT/script/lint"` and a bare `script/lint` are
|
||||
// all the same edge.
|
||||
for (const match of line.matchAll(
|
||||
/(?:\$SCRIPT_DIR|\$\{SCRIPT_DIR\}|script)\/([a-z][a-z-]*)/g,
|
||||
)) {
|
||||
edges.push(`script/${match[1]}`);
|
||||
}
|
||||
|
||||
// Only real targets: `pkg_install gnumake make make make` in
|
||||
// script/bootstrap is a package name, not an invocation of this Makefile.
|
||||
for (const match of line.matchAll(/\bmake\s+([a-z][a-z-]*)/g)) {
|
||||
if (recipes.has(match[1] ?? "")) edges.push(`make:${match[1]}`);
|
||||
}
|
||||
|
||||
// Same rule for yarn: `yarn run prettier` is the linter itself (counted,
|
||||
// not followed), `yarn run fmt-check` would be a package.json script that
|
||||
// runs it indirectly.
|
||||
for (const match of line.matchAll(/\byarn(?:\s+run)?\s+([a-z][a-z-]*)/g)) {
|
||||
if ((match[1] ?? "") in scripts) edges.push(`yarn:${match[1]}`);
|
||||
}
|
||||
|
||||
// The container lint pass lives behind a `docker build`; without following
|
||||
// it the count would miss the one invocation that is supposed to survive.
|
||||
if (/\bdocker\s+build\b/.test(line)) {
|
||||
const file = /\s-f\s+(\S+)/.exec(line);
|
||||
edges.push(`docker:${file === null ? "Dockerfile" : file[1]}`);
|
||||
}
|
||||
|
||||
return edges;
|
||||
};
|
||||
|
||||
interface Walk {
|
||||
prettier: number;
|
||||
reached: Set<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);
|
||||
|
||||
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);
|
||||
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
|
||||
// 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"]);
|
||||
});
|
||||
|
||||
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",
|
||||
]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user