quak/test/packaging/lint-once.test.ts

// `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",
        ]);
    });
});