Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: Three things to fix in one round. #8890
Check · Review · Land · Live done — Landed and live in production.
torque/forge:torque/8888-automate-a-review-finding-as-a-lint-or-t → main 3 commits fork 3 threads · 3 unresolved
6 files
changed +143
−60
scripts/gate.ts +14 −2
| @@ -20,7 +20,7 @@ | ||
| 20 | 20 | * runs `outsideFiles` nightly and files a `Red:` issue for each that fails (#812). |
| 21 | 21 | */ |
| 22 | 22 | |
| 23 | import { existsSync, readdirSync } from "node:fs"; | |
| 23 | import { existsSync, readdirSync, readFileSync } from "node:fs"; | |
| 24 | 24 | import { join } from "node:path"; |
| 25 | 25 | |
| 26 | 26 | export type Exclusion = { |
| @@ -274,6 +274,18 @@ export async function whyNowRefusal(cwd: string = process.cwd()): Promise<string | ||
| 274 | 274 | return whyNowProblems(parseNameStatus(diff.out), log.out.split("\0").map((m) => m.trim()).filter(Boolean)); |
| 275 | 275 | } |
| 276 | 276 | |
| 277 | /** Review findings that repeated (#8888): see `src/kernel/review_rules.ts`. */ | |
| 278 | export function reviewRefusal(root = "."): string[] { | |
| 279 | // Loaded by `require` so `treeGate`, which loads this file alone, never needs it. | |
| 280 | const { reviewProblems } = require("../src/kernel/review_rules.ts") as typeof import("../src/kernel/review_rules.ts"); | |
| 281 | const dir = join(root, "test"); | |
| 282 | if (!existsSync(dir)) return []; | |
| 283 | const sources = readdirSync(dir) | |
| 284 | .filter((f) => f.endsWith(".test.ts")) | |
| 285 | .map((f) => ({ path: `test/${f}`, text: readFileSync(join(dir, f), "utf8") })); | |
| 286 | return reviewProblems(sources); | |
| 287 | } | |
| 288 | ||
| 277 | 289 | /** |
| 278 | 290 | * A pull that changes a reachable surface must say how to try it on its |
| 279 | 291 | * preview (#8889). Same base and same pass-when-unknown as `whyNowRefusal`. |
| @@ -295,7 +307,7 @@ export async function tryItRefusal(cwd: string = process.cwd()): Promise<string[ | ||
| 295 | 307 | // `bun scripts/gate.ts`: the audit alone, first in `forge.json`'s check, so |
| 296 | 308 | // a stale exclusion fails in under a second — not after install and tsc. |
| 297 | 309 | if (import.meta.main) { |
| 298 | const refusal = [...gateRefusal(presentTests()), ...(await pinRefusal()), ...(await whyNowRefusal()), ...(await tryItRefusal())]; | |
| 310 | const refusal = [...gateRefusal(presentTests()), ...(await pinRefusal()), ...(await whyNowRefusal()), ...(await tryItRefusal()), ...reviewRefusal()]; | |
| 299 | 311 | for (const line of refusal) console.error(`gate: ${line}`); |
| 300 | 312 | process.exit(refusal.length === 0 ? 0 : 2); |
| 301 | 313 | } |
src/kernel/review_rules.ts added +31
| @@ -0,0 +1,31 @@ | ||
| 1 | /** | |
| 2 | * Review findings that came back often enough to be a check (#8888, after | |
| 3 | * #8716). Each rule names itself in its message, so a failure says what the | |
| 4 | * reviewer would have said. Plain data in, plain data out: `scripts/gate.ts` | |
| 5 | * reads the files and prints the lines. | |
| 6 | * | |
| 7 | * - "a test file does not import another test file" (#7717): importing | |
| 8 | * `./x.test.ts` registers and re-runs every test in it. Share a helper | |
| 9 | * module under `test/helpers/` instead. | |
| 10 | */ | |
| 11 | ||
| 12 | export type Source = { path: string; text: string }; | |
| 13 | ||
| 14 | const TEST_FILE = /^test\/.+\.test\.ts$/; | |
| 15 | // `from "./x.test.ts"`, `import("../x.test")`, `import "./x.test.ts"`. | |
| 16 | const TEST_IMPORT = /(?:\bfrom\s*|\bimport\s*\(?\s*)["'](\.[^"']*\.test(?:\.ts)?)["']/g; | |
| 17 | ||
| 18 | /** One line per problem, each naming its rule, file and line. */ | |
| 19 | export function reviewProblems(sources: readonly Source[]): string[] { | |
| 20 | const out: string[] = []; | |
| 21 | for (const { path, text } of sources) { | |
| 22 | if (!TEST_FILE.test(path)) continue; | |
| 23 | text.split("\n").forEach((line, i) => { | |
| 24 | for (const m of line.matchAll(TEST_IMPORT)) | |
| 25 | out.push( | |
| 26 | `rule "a test file does not import another test file": ${path}:${i + 1} imports ${m[1]}, which re-runs all its tests; move the shared code to test/helpers/ and import that`, | |
| 27 | ); | |
| 28 | }); | |
| 29 | } | |
| 30 | return out; | |
| 31 | } |
test/helpers/toolbar-boot.ts added +60
| @@ -0,0 +1,60 @@ | ||
| 1 | /** A fake document and window to boot the toolbar script in, shared by the toolbar tests. */ | |
| 2 | ||
| 3 | import { toolbarJs } from "../../src/apps/toolbar.ts"; | |
| 4 | ||
| 5 | type L = Record<string, ((e?: any) => void)[]>; | |
| 6 | interface El { | |
| 7 | tag: string; | |
| 8 | style: Record<string, string>; | |
| 9 | children: El[]; | |
| 10 | listeners: L; | |
| 11 | hidden: boolean; | |
| 12 | textContent: string; | |
| 13 | [k: string]: any; | |
| 14 | } | |
| 15 | const make = (tag: string): El => ({ | |
| 16 | tag, | |
| 17 | style: {}, | |
| 18 | children: [], | |
| 19 | listeners: {}, | |
| 20 | hidden: false, | |
| 21 | textContent: "", | |
| 22 | attrs: {} as Record<string, string>, | |
| 23 | setAttribute(k: string, v: string) { this.attrs[k] = v; }, | |
| 24 | appendChild(c: El) { this.children.push(c); return c; }, | |
| 25 | addEventListener(t: string, f: (e?: any) => void) { (this.listeners[t] ??= []).push(f); }, | |
| 26 | focus() {}, | |
| 27 | attachShadow() { return make("shadow"); }, | |
| 28 | }); | |
| 29 | const all = (el: El): El[] => [el, ...el.children.flatMap(all)]; | |
| 30 | ||
| 31 | export function boot(store: Record<string, string> | null, width = 400, height = 800) { | |
| 32 | let root!: El; | |
| 33 | const host = { el: null as El | null }; | |
| 34 | const doc = { | |
| 35 | currentScript: { src: "https://forge.test/assets/t.js", dataset: { app: "notes", env: "production" } }, | |
| 36 | createElement: (tag: string) => { | |
| 37 | const el = make(tag); | |
| 38 | if (tag === "div" && !host.el) { | |
| 39 | host.el = el; | |
| 40 | el.attachShadow = () => (root = make("shadow")); | |
| 41 | } | |
| 42 | return el; | |
| 43 | }, | |
| 44 | addEventListener() {}, | |
| 45 | body: { appendChild() {} }, | |
| 46 | }; | |
| 47 | const win: Record<string, any> = { innerWidth: width, innerHeight: height }; | |
| 48 | if (store) | |
| 49 | win.localStorage = { | |
| 50 | getItem: (k: string) => store[k] ?? null, | |
| 51 | setItem: (k: string, v: string) => { store[k] = v; }, | |
| 52 | }; | |
| 53 | new Function("window", "document", "URL", toolbarJs)(win, doc, URL); | |
| 54 | const els = all(root); | |
| 55 | const pill = els.find((e) => e.tag === "button")!; | |
| 56 | const box = root.children[0]!; | |
| 57 | const panel = els.find((e) => e.attrs.role === "region")!; | |
| 58 | const fire = (el: El, t: string, e: object = {}) => el.listeners[t]!.forEach((f) => f({ preventDefault() {}, type: t, ...e })); | |
| 59 | return { host: host.el!, pill, box, panel, fire }; | |
| 60 | } |
test/review-rules.test.ts added +36
| @@ -0,0 +1,36 @@ | ||
| 1 | import { expect, test } from "bun:test"; | |
| 2 | import { readdirSync, readFileSync } from "node:fs"; | |
| 3 | import { reviewProblems } from "../src/kernel/review_rules.ts"; | |
| 4 | import { reviewRefusal } from "../scripts/gate.ts"; | |
| 5 | ||
| 6 | // Built apart so this file does not itself import a test file. | |
| 7 | const T = ".test"; | |
| 8 | ||
| 9 | test("a test that imports another test file is refused, naming the rule, file and line", () => { | |
| 10 | const [p, ...rest] = reviewProblems([ | |
| 11 | { path: "test/a.test.ts", text: `import { x } from "bun:test";\nimport { boot } from "./b${T}.ts";\n` }, | |
| 12 | ]); | |
| 13 | expect(rest).toEqual([]); | |
| 14 | expect(p).toContain('rule "a test file does not import another test file"'); | |
| 15 | expect(p).toContain("test/a.test.ts:2"); | |
| 16 | expect(p).toContain("test/helpers/"); | |
| 17 | }); | |
| 18 | ||
| 19 | test("dynamic and extensionless imports of a test file are caught too", () => { | |
| 20 | expect(reviewProblems([{ path: "test/a.test.ts", text: `const m = await import("../test/b${T}");` }])).toHaveLength(1); | |
| 21 | }); | |
| 22 | ||
| 23 | test("a helper import, a source import and a non-test file pass", () => { | |
| 24 | expect( | |
| 25 | reviewProblems([ | |
| 26 | { path: "test/a.test.ts", text: 'import { boot } from "./helpers/toolbar-boot.ts";\nimport { f } from "../src/x.ts";' }, | |
| 27 | { path: "test/helpers/h.ts", text: `import "./b${T}.ts";` }, | |
| 28 | ]), | |
This pins that a helper importing a test file passes. That is the same re-run bug in another place; make the rule cover test/helpers/ and flip this case. |
||
| 29 | ).toEqual([]); | |
| 30 | }); | |
| 31 | ||
| 32 | test("the repository's own tests pass the rules", () => { | |
| 33 | expect(reviewRefusal()).toEqual([]); | |
The readdirSync length check and the toolbar-camera source read assert nothing about the rule; they would pass with a broken rule. Keep only the reviewRefusal() assertion. |
||
| 34 | expect(readdirSync("test").length).toBeGreaterThan(0); | |
| 35 | expect(readFileSync("test/toolbar-camera.test.ts", "utf8")).toContain("helpers/toolbar-boot.ts"); | |
| 36 | }); |
test/toolbar-camera.test.ts +1 −1
| @@ -4,7 +4,7 @@ | ||
| 4 | 4 | */ |
| 5 | 5 | |
| 6 | 6 | import { describe, expect, test } from "bun:test"; |
| 7 | import { boot } from "./toolbar-move.test.ts"; | |
| 7 | import { boot } from "./helpers/toolbar-boot.ts"; | |
| 8 | 8 | |
| 9 | 9 | const at = (corner: string, width: number) => boot({ "forge-pill-corner": corner }, width, 844); |
| 10 | 10 |
test/toolbar-move.test.ts +1 −57
| @@ -6,63 +6,7 @@ | ||
| 6 | 6 | |
| 7 | 7 | import { describe, expect, test } from "bun:test"; |
| 8 | 8 | import { toolbarJs } from "../src/apps/toolbar.ts"; |
| 9 | ||
| 10 | type L = Record<string, ((e?: any) => void)[]>; | |
| 11 | interface El { | |
| 12 | tag: string; | |
| 13 | style: Record<string, string>; | |
| 14 | children: El[]; | |
| 15 | listeners: L; | |
| 16 | hidden: boolean; | |
| 17 | textContent: string; | |
| 18 | [k: string]: any; | |
| 19 | } | |
| 20 | const make = (tag: string): El => ({ | |
| 21 | tag, | |
| 22 | style: {}, | |
| 23 | children: [], | |
| 24 | listeners: {}, | |
| 25 | hidden: false, | |
| 26 | textContent: "", | |
| 27 | attrs: {} as Record<string, string>, | |
| 28 | setAttribute(k: string, v: string) { this.attrs[k] = v; }, | |
| 29 | appendChild(c: El) { this.children.push(c); return c; }, | |
| 30 | addEventListener(t: string, f: (e?: any) => void) { (this.listeners[t] ??= []).push(f); }, | |
| 31 | focus() {}, | |
| 32 | attachShadow() { return make("shadow"); }, | |
| 33 | }); | |
| 34 | const all = (el: El): El[] => [el, ...el.children.flatMap(all)]; | |
| 35 | ||
| 36 | export function boot(store: Record<string, string> | null, width = 400, height = 800) { | |
| 37 | let root!: El; | |
| 38 | const host = { el: null as El | null }; | |
| 39 | const doc = { | |
| 40 | currentScript: { src: "https://forge.test/assets/t.js", dataset: { app: "notes", env: "production" } }, | |
| 41 | createElement: (tag: string) => { | |
| 42 | const el = make(tag); | |
| 43 | if (tag === "div" && !host.el) { | |
| 44 | host.el = el; | |
| 45 | el.attachShadow = () => (root = make("shadow")); | |
| 46 | } | |
| 47 | return el; | |
| 48 | }, | |
| 49 | addEventListener() {}, | |
| 50 | body: { appendChild() {} }, | |
| 51 | }; | |
| 52 | const win: Record<string, any> = { innerWidth: width, innerHeight: height }; | |
| 53 | if (store) | |
| 54 | win.localStorage = { | |
| 55 | getItem: (k: string) => store[k] ?? null, | |
| 56 | setItem: (k: string, v: string) => { store[k] = v; }, | |
| 57 | }; | |
| 58 | new Function("window", "document", "URL", toolbarJs)(win, doc, URL); | |
| 59 | const els = all(root); | |
| 60 | const pill = els.find((e) => e.tag === "button")!; | |
| 61 | const box = root.children[0]!; | |
| 62 | const panel = els.find((e) => e.attrs.role === "region")!; | |
| 63 | const fire = (el: El, t: string, e: object = {}) => el.listeners[t]!.forEach((f) => f({ preventDefault() {}, type: t, ...e })); | |
| 64 | return { host: host.el!, pill, box, panel, fire }; | |
| 65 | } | |
| 9 | import { boot } from "./helpers/toolbar-boot.ts"; | |
| 66 | 10 | |
| 67 | 11 | describe("moving the pill", () => { |
| 68 | 12 | test("starts bottom-right and the panel opens to its left", () => { |
new above
offline
reviewRefusal only reads top-level test/*.test.ts, so a test file in a subdirectory escapes the rule. Walk test/ recursively.