From 3f6a9efd5763f9e6488bc9b535154599cd5d72e1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 4 Oct 2026 04:16:52 +0200 Subject: [PATCH] test(gates): the register test parses ruff's TOML and holds pytest.ini line by line Round 2 of the E32 review found the key whitelists blind: a line regex saw neither a [lint.per-file-ignores] table nor a quoted key, and pytest.ini's continuation lines and key: value form slipped past. Ruff's config is now parsed (smol-toml) and held to its key paths; pytest.ini to its whole lines; the CI ruff steps to --config backend/ruff.toml, which is what makes that file the whole configuration. ESLint: every glob of a block must reach a sample (reactPlugin.ts and devOnlyGuard.ts rode along unseen), a block's own severity is held to src, a no-restricted-* list is held to the src block's declaration rather than Feed.tsx's effective config, named global ignores count, an ignored sample is named, and linterOptions are checked. tsconfig.node must keep allowJs + checkJs while it lists a .js file; knip's positive glob is pinned. --- docs/gates.md | 10 +- frontend/package-lock.json | 1 + frontend/package.json | 1 + frontend/src/lib/gates.contract.test.ts | 200 ++++++++++++++++-------- 4 files changed, 141 insertions(+), 71 deletions(-) diff --git a/docs/gates.md b/docs/gates.md index 877749d1..c4a0a36e 100644 --- a/docs/gates.md +++ b/docs/gates.md @@ -15,10 +15,12 @@ not an oversight; a narrowing missing from here is a test failure. How the test matches: each narrowing must appear in a table row below as its `code-quoted` id, in a row whose **Scope** cell names the scope exactly as the test reports it. For ESLint it reads each -scope's effective config through a sample file AND every config block on its own: a block that -targets files no sample reaches fails as an `unsampled block` (add a sample), a rule a block or src -sets weaker than src counts, so does a `no-restricted-*` list that leaves out an entry src has, and -so does an option that lets something through (`allow…`, `ignore…`, `except…`) (E32 review). +scope's effective config through its sample files AND every config block on its own: a block glob +that no sample matches fails as an `unsampled block` (add a sample), a rule a block or src sets +weaker than src counts, so does a `no-restricted-*` list that leaves out an entry the src block +declares, so does an option that lets something through (`allow…`, `ignore…`, `except…`), and so +does any `linterOptions` but an erroring `reportUnusedDisableDirectives` (E32 review). Ruff's TOML +is parsed and held to a whitelist of key paths, pytest.ini to a whitelist of whole lines. ## ESLint (frontend) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 38982ff5..880bebdb 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -46,6 +46,7 @@ "knip": "6.27.0", "postcss": "^8.4.39", "prettier": "3.6.2", + "smol-toml": "1.9.0", "tailwindcss": "^4.3.3", "typescript": "^5.5.3", "typescript-eslint": "8.71.0", diff --git a/frontend/package.json b/frontend/package.json index aba641ae..6cf9f47e 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -62,6 +62,7 @@ "knip": "6.27.0", "postcss": "^8.4.39", "prettier": "3.6.2", + "smol-toml": "1.9.0", "tailwindcss": "^4.3.3", "typescript": "^5.5.3", "typescript-eslint": "8.71.0", diff --git a/frontend/src/lib/gates.contract.test.ts b/frontend/src/lib/gates.contract.test.ts index c76edbf2..5f10d77d 100644 --- a/frontend/src/lib/gates.contract.test.ts +++ b/frontend/src/lib/gates.contract.test.ts @@ -2,6 +2,7 @@ import { readdirSync, readFileSync } from "node:fs"; import { fileURLToPath, pathToFileURL } from "node:url"; import { ESLint, type Linter } from "eslint"; import prettierConfig from "eslint-config-prettier"; +import { parse as parseToml } from "smol-toml"; import ts from "typescript"; import tseslint from "typescript-eslint"; import { describe, expect, it } from "vitest"; @@ -68,24 +69,30 @@ const TSC_OWNED = new Set( ); const family = (rule: string) => (rule.includes("/") ? `${rule.split("/")[0]}/*` : rule); -// Each scope is checked through a sample file's EFFECTIVE config (what a rule family not applied there -// means), and every config block that targets files is checked on its own (below) — so a block no -// sample reaches fails rather than passing unseen (E32 review: a narrowing scoped to any other file -// changed none of the sampled configs and needed no row). -const SCOPES: Record = { - src: "src/components/Feed.tsx", - "src/lib": "src/lib/format.ts", - "lib/http.ts": "src/lib/http.ts", - "lib/api/core.ts": "src/lib/api/core.ts", - "lib/messagesSocket.ts": "src/lib/messagesSocket.ts", - "src tests": "src/components/Feed.ytsort.dom.test.tsx", - "FocusSafeButton.tsx": "src/components/ui/FocusSafeButton.tsx", - "AnchoredPopover.tsx": "src/components/ui/AnchoredPopover.tsx", - "design-lab": "src/design-lab/batchA.tsx", - e2e: "e2e/nav.spec.ts", - "ts configs": "vite.config.ts", - "js configs": "eslint.config.js", +// Each scope is checked through its sample files' EFFECTIVE config (what a rule family not applied +// there means), and every config block that targets files is checked on its own (below): each of its +// globs must reach a sample, so a glob no sample reaches fails rather than passing unseen (E32 review: +// a narrowing scoped to any other file changed none of the sampled configs and needed no row; round +// 2: a block was sampled through ONE of its globs, and `reactPlugin.ts` rode along unseen). +const SCOPES: Record = { + src: ["src/components/Feed.tsx"], + "src/lib": ["src/lib/format.ts"], + "lib/http.ts": ["src/lib/http.ts"], + "lib/api/core.ts": ["src/lib/api/core.ts"], + "lib/messagesSocket.ts": ["src/lib/messagesSocket.ts"], + "src tests": ["src/components/Feed.ytsort.dom.test.tsx"], + "FocusSafeButton.tsx": ["src/components/ui/FocusSafeButton.tsx"], + "AnchoredPopover.tsx": ["src/components/ui/AnchoredPopover.tsx"], + "design-lab": ["src/design-lab/batchA.tsx"], + e2e: ["e2e/nav.spec.ts"], + "ts configs": ["vite.config.ts", "reactPlugin.ts", "devOnlyGuard.ts"], + "js configs": ["eslint.config.js"], }; +const SAMPLES = Object.entries(SCOPES).flatMap(([scope, files]) => + files.map((f): [scope: string, file: string] => [scope, f]), +); +// The block every src file gets: the reference a scope's re-declared `no-restricted-*` list is held to. +const SRC_GLOB = "src/**/*.{ts,tsx}"; /** A flat-config `files` glob as a regex — the shapes this config uses: `**`, `*`, `{a,b}`. */ function globRegex(glob: string): RegExp { @@ -138,55 +145,88 @@ function restrictedEntries(entry: Linter.RuleEntry | undefined): Set { async function eslintNarrowings(): Promise<[string, string][]> { const eslint = new ESLint({ cwd: FRONTEND }); - const rules: Record = {}; - for (const [scope, file] of Object.entries(SCOPES)) - rules[scope] = ((await eslint.calculateConfigForFile(file)) as Linter.Config).rules ?? {}; - const src = rules.src!; + const found: [string, string][] = []; + const rules: [scope: string, rules: Rules][] = []; + for (const [scope, file] of SAMPLES) { + const cfg = (await eslint.calculateConfigForFile(file)) as Linter.Config | undefined; + if (!cfg) { + found.push([scope, `${file} is ignored`]); // ignored files get no config at all + continue; + } + rules.push([scope, cfg.rules ?? {}]); + // an unused `eslint-disable` is an error on every file, or a stale suppression stays armed + if (severity(cfg.linterOptions?.reportUnusedDisableDirectives as Linter.RuleEntry) !== 2) + found.push([scope, "reportUnusedDisableDirectives"]); + } + const src = rules.find(([scope]) => scope === "src")![1]; const ignorable = (rule: string, all: Rules) => PRETTIER_OFF.has(rule) || TSC_OWNED.has(rule) || // a core rule off because its typescript-eslint twin replaces it is reported as the twin (!rule.includes("/") && `@typescript-eslint/${rule}` in all); - const found: [string, string][] = []; // src: everything at off or warn, and every rule whose options let something through for (const [rule, v] of Object.entries(src)) { if (ignorable(rule, src)) continue; if (severity(v) < 2 || (Array.isArray(v) && relaxes(v.slice(1)))) found.push(["src", rule]); } - // every other scope: what is WEAKER there than in src (a rule family not applied at all is one + // every other sample: what is WEAKER there than in src (a rule family not applied at all is one // id). A src rule at warn counts too: switched off for a scope, it is narrowed there. - for (const scope of Object.keys(SCOPES).filter((s) => s !== "src")) { + for (const [scope, theirs] of rules.filter(([s]) => s !== "src")) { const ids = new Set(); for (const [rule, v] of Object.entries(src)) { if (ignorable(rule, src) || severity(v) === 0) continue; - const there = severity(rules[scope]![rule]); + const there = severity(theirs[rule]); if (there < severity(v)) ids.add(there < 0 ? family(rule) : rule); } for (const id of ids) found.push([scope, id]); } // every config block: a global `ignores` is a narrowing everywhere; one that targets files must - // reach a sample, and what it re-declares is held to src - for (const block of await blocks()) { - if (!block.files) { - if (Object.keys(block).join() === "ignores") - for (const ignored of block.ignores ?? []) found.push(["everywhere", ignored]); + // reach a sample through EACH glob, and what it declares is held to src on its own — a later block + // that switches a rule back on for the sample would otherwise hide where the first one is off + const all = await blocks(); + const isSrcBlock = (b: Block) => b.files?.flat().join() === SRC_GLOB; + const reference = new Map>(); // the FIRST src-block declaration of each list + for (const block of all.filter(isSrcBlock)) + for (const [rule, v] of Object.entries(block.rules ?? {})) + if (rule.includes("no-restricted-") && !reference.has(rule)) + reference.set(rule, restrictedEntries(v)); + let globalIgnores = 0; + for (const block of all) { + // ESLint reads a block as GLOBAL ignores when `ignores` is its only key besides `name` + if ( + !block.files && + Object.keys(block) + .filter((k) => k !== "name") + .join() === "ignores" + ) { + globalIgnores++; + for (const ignored of block.ignores ?? []) found.push(["everywhere", ignored]); continue; } - const globs = block.files.flat().map(globRegex); - const scope = Object.keys(SCOPES).find((s) => globs.some((g) => g.test(SCOPES[s]!))); - if (!scope) { - found.push(["unsampled block", block.files.flat().join(", ")]); - continue; + for (const [key, v] of Object.entries(block.linterOptions ?? {})) + if (key !== "reportUnusedDisableDirectives" || severity(v as Linter.RuleEntry) !== 2) + found.push(["linterOptions", key]); + if (!block.files) continue; + let scope: string | undefined; + for (const glob of block.files.flat()) { + const hit = SAMPLES.find(([, file]) => globRegex(glob).test(file)); + if (!hit) found.push(["unsampled block", glob]); + scope ??= hit?.[0]; } + if (!scope) continue; if (block.ignores?.length) found.push([scope, `ignores: ${block.ignores.join(", ")}`]); for (const [rule, v] of Object.entries(block.rules ?? {})) { if (ignorable(rule, src)) continue; if (rule.includes("no-restricted-")) { + const ref = reference.get(rule); const own = restrictedEntries(v); - if ([...restrictedEntries(src[rule])].some((e) => !own.has(e))) found.push([scope, rule]); - } else if (Array.isArray(v) && relaxes(v.slice(1))) found.push([scope, rule]); + if (ref && [...ref].some((e) => !own.has(e))) found.push([scope, rule]); + } else if (severity(v) < severity(src[rule]) || (Array.isArray(v) && relaxes(v.slice(1)))) + found.push([scope, rule]); } } + // the detection above must still see the block every config has, or `everywhere` is never checked + if (globalIgnores === 0) found.push(["everywhere", "(no global ignores block found)"]); return found; } @@ -262,29 +302,39 @@ describe("the gate register (docs/gates.md)", () => { .filter((f) => !checked.has(f)) .map((f): [string, string] => ["config files", `${f} — not type-checked`]); expect(missingFrom(found)).toEqual([]); + // a `.js` in `include` is dropped without a word unless allowJs, and read unchecked without + // checkJs (E32 review round 2) + const opts = jsonc("frontend/tsconfig.node.json").compilerOptions as Record; + if ([...checked].some((f) => /[.][mc]?js$/.test(f))) + expect([opts.allowJs, opts.checkJs]).toEqual([true, true]); }); // --- Ruff ----------------------------------------------------------------------------------------- - // Ruff, pyright, pytest and knip are read as KEY whitelists: a key this test does not know (an + // Ruff, pyright, pytest and knip are read as whitelists: a key this test does not know (an // `extend-exclude`, a `lint.extend-ignore`, a narrowing `select`, a pyright `exclude`, a pytest // `-m "not slow"`, a knip `rules`) fails until someone decides whether it narrows (E32 review — - // only `ignore` and the immutable calls used to be read). - const tomlKeys = (toml: string) => - toml - .split("\n") - .map((l) => /^\s*([A-Za-z0-9_.-]+)\s*=/.exec(l)?.[1]) - .filter((k): k is string => k !== undefined); - it("lists every ruff ignore and immutable call, and knows every key", () => { - const toml = read("backend/ruff.toml"); - const list = (key: string) => { - const m = new RegExp(`^${key}\\s*=\\s*\\[([^\\]]*)\\]`, "m").exec(toml); - return m ? [...m[1]!.matchAll(/"([^"]+)"/g)].map((x) => x[1]!) : []; - }; - expect(tomlKeys(toml).sort()).toEqual( - ["extend-immutable-calls", "extend-select", "line-length", "target-version"].sort(), + // only `ignore` and the immutable calls used to be read). The TOML is PARSED, not scanned line by + // line: a `[lint.per-file-ignores]` table or a quoted key is a key path like any other (E32 review + // round 2 — a line regex saw neither). + const keyPaths = (o: Record, prefix = ""): string[] => + Object.entries(o).flatMap(([k, v]) => + v !== null && typeof v === "object" && !Array.isArray(v) + ? keyPaths(v as Record, `${prefix}${k}.`) + : [`${prefix}${k}`], ); + it("lists every ruff ignore and immutable call, and knows every key", () => { + const cfg = parseToml(read("backend/ruff.toml")); + expect(keyPaths(cfg).sort()).toEqual( + [ + "lint.extend-select", + "lint.flake8-bugbear.extend-immutable-calls", + "line-length", + "target-version", + ].sort(), + ); + const lint = cfg.lint as Record; // the selection on top of ruff's defaults (E, F) — dropping one narrows the lane - expect(list("extend-select")).toEqual([ + expect(lint["extend-select"]).toEqual([ "LOG", "G", "T20", @@ -297,12 +347,24 @@ describe("the gate register (docs/gates.md)", () => { "PLW1510", "B", ]); - const found: [string, string][] = [ - ...list("ignore").map((code): [string, string] => ["backend + scripts", code]), - ...list("extend-immutable-calls").map((c): [string, string] => ["backend + scripts", c]), - ]; + const immutable = (lint["flake8-bugbear"] as Record)[ + "extend-immutable-calls" + ]!; + const found = immutable.map((c): [string, string] => ["backend + scripts", c]); expect(missingFrom(found)).toEqual([]); }); + // Every ruff lane passes `--config backend/ruff.toml`, which makes ruff ignore any other config + // file it would discover (measured: a nested `ruff.toml` selecting the `D` rules changed nothing). + // So that flag is what makes this file the whole configuration, and it is held here. + it("runs every CI ruff step on backend/ruff.toml", () => { + const steps = read(".gitea/workflows/check.yml") + .split("\n") + .filter((l) => /python -m ruff /.test(l)); + expect(steps.length).toBe(2); + expect(steps.filter((l) => !l.includes("--config backend/ruff.toml backend scripts"))).toEqual( + [], + ); + }); // --- basedpyright --------------------------------------------------------------------------------- const PYRIGHT_KEYS = [ @@ -330,18 +392,20 @@ describe("the gate register (docs/gates.md)", () => { }); // --- pytest --------------------------------------------------------------------------------------- + // Held line by line, not key by key: pytest's ini reader also takes `key: value` and indented + // continuation lines (`addopts = -q` + ` -m "not slow"`), and a key regex saw neither (E32 review + // round 2). Every line that is not blank or a comment must be one of these, and all must be there. it("keeps pytest strict: the whole tests/ tree, no selection, no warning filters, --strict-markers", () => { - const ini = read("backend/pytest.ini"); - const keys = ini + const lines = read("backend/pytest.ini") .split("\n") - .filter((l) => !l.trim().startsWith("#")) - .map((l) => /^([a-z_]+)\s*=\s*(.*)$/.exec(l)) - .filter((m) => m !== null); - expect(Object.fromEntries(keys.map((m) => [m[1], m[2]!.trim()]))).toEqual({ - testpaths: "tests", - pythonpath: ".", - addopts: "-q -rs --strict-markers", - }); + .map((l) => l.trimEnd()) + .filter((l) => l.trim() !== "" && !/^[#;]/.test(l.trim())); + expect(lines).toEqual([ + "[pytest]", + "testpaths = tests", + "pythonpath = .", + "addopts = -q -rs --strict-markers", + ]); }); // --- knip ----------------------------------------------------------------------------------------- @@ -357,6 +421,8 @@ describe("the gate register (docs/gates.md)", () => { expect(missingFrom(found)).toEqual([]); const pkg = JSON.parse(read("frontend/package.json")) as { scripts: Record }; expect(pkg.scripts.knip).toBe("knip && knip --production"); + // the positive pattern is the project itself: narrowing it drops files from both modes + expect((cfg.project ?? []).filter((p) => !p.startsWith("!"))).toEqual(["**/*.{ts,tsx,js}!"]); }); // --- accepted findings ----------------------------------------------------------------------------