Public: anyone can read this; the crew can change it.
A pull's check re-runs alone the one or two failed tests in files the pull did not touch, and passing alone it is green with the flake recorded, as main's check does #7430
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/7426-a-pull-s-check-re-runs-alone-the-one-or → main 4 commits fork 1 thread · 1 unresolved
3 files
changed +220
−10
src/checks/flake.ts +57 −3
| @@ -15,6 +15,7 @@ import type { Repo } from "../core/repos.ts"; | ||
| 15 | 15 | import type { EventBus } from "../core/events.ts"; |
| 16 | 16 | import { id } from "../kernel/id.ts"; |
| 17 | 17 | import type { CheckDetail } from "../core/runs.ts"; |
| 18 | import { importsOf } from "./tiers.ts"; | |
| 18 | 19 | |
| 19 | 20 | /** The most failures that are re-run; more is a broken main, not a flake. */ |
| 20 | 21 | export const MAX_RERUN = 2; |
| @@ -75,6 +76,58 @@ export function rerunnable( | ||
| 75 | 76 | return out; |
| 76 | 77 | } |
| 77 | 78 | |
| 79 | /** Files that shape every test run: a pull changing one owns any red. */ | |
| 80 | const SHAPING = /(^|\/)(package\.json|bun\.lock|bun\.lockb|bunfig\.toml|tsconfig[^/]*\.json|forge\.json|\.env[^/]*)$/; | |
| 81 | const EXTENSIONS = ["", ".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs", ".mts", ".cts", "/index.ts", "/index.tsx", "/index.js"]; | |
| 82 | const IMPORT_FILES_MAX = 2000; | |
| 83 | ||
| 84 | /** | |
| 85 | * Whether a failed test's file is the pull's to answer for: it, or any | |
| 86 | * repository file it imports (followed to the end), is among the changed | |
| 87 | * paths, or a file that shapes every run changed. Conservative: an import that | |
| 88 | * resolves to no readable file, or a closure too large to walk, counts as | |
| 89 | * touched, so only a provably unrelated red is re-run. | |
| 90 | */ | |
| 91 | export function touchedByPull( | |
| 92 | file: string, | |
| 93 | changed: readonly string[], | |
| 94 | read: (path: string) => string | undefined, | |
| 95 | ): boolean { | |
| 96 | const diff = new Set(changed); | |
| 97 | if (changed.some((c) => SHAPING.test(c))) return true; | |
| 98 | const seen = new Set<string>(); | |
| 99 | const queue = [file]; | |
| 100 | while (queue.length > 0) { | |
| 101 | const at = queue.pop()!; | |
| 102 | if (seen.has(at)) continue; | |
| 103 | if (seen.size >= IMPORT_FILES_MAX) return true; | |
| 104 | seen.add(at); | |
| 105 | if (diff.has(at)) return true; | |
| 106 | const source = read(at); | |
| 107 | if (source === undefined) return true; | |
| 108 | for (const spec of importsOf(source, at)) { | |
| 109 | const candidates = EXTENSIONS.map((e) => spec + e); | |
| 110 | if (candidates.some((c) => diff.has(c))) return true; | |
| 111 | const real = candidates.find((c) => read(c) !== undefined); | |
| 112 | if (real === undefined) return true; | |
| 113 | queue.push(real); | |
| 114 | } | |
| 115 | } | |
| 116 | return false; | |
| 117 | } | |
| 118 | ||
| 119 | /** | |
| 120 | * Whether the pull answers for a red: any failed test touched, or no list of | |
| 121 | * changed paths at all (the diff failed), and nothing is re-run. | |
| 122 | */ | |
| 123 | export function pullOwnsRed( | |
| 124 | tests: readonly Flaky[], | |
| 125 | changed: readonly string[] | undefined, | |
| 126 | read: (path: string) => string | undefined, | |
| 127 | ): boolean { | |
| 128 | return changed === undefined || tests.some((t) => touchedByPull(t.file, changed, read)); | |
| 129 | } | |
| 130 | ||
| 78 | 131 | const escapeRegex = (s: string): string => s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); |
| 79 | 132 | const quote = (s: string): string => `'${s.replace(/'/g, `'\\''`)}'`; |
| 80 | 133 | |
| @@ -115,9 +168,10 @@ export function recordFlakes( | ||
| 115 | 168 | bus: EventBus, |
| 116 | 169 | repo: Pick<Repo, "id" | "ownerId">, |
| 117 | 170 | tests: readonly Flaky[], |
| 118 | ref: { runId: string; commit: string }, | |
| 171 | ref: { runId: string; commit: string; pull?: boolean }, | |
| 119 | 172 | now = Date.now(), |
| 120 | 173 | ): number[] { |
| 174 | const where = ref.pull === true ? "a pull's check" : "main's check"; | |
| 121 | 175 | const opened: number[] = []; |
| 122 | 176 | for (const t of tests) { |
| 123 | 177 | db.query( |
| @@ -144,8 +198,8 @@ export function recordFlakes( | ||
| 144 | 198 | title, |
| 145 | 199 | body: |
| 146 | 200 | `_Filed by the check runner from a check log, not written by a person._\n\n` + |
| 147 | `\`${t.name}\` in \`${t.file}\` failed in main's check and passed when run alone, ${count} times in 7 days ` + | |
| 148 | `(latest: check run ${ref.runId}, ${ref.commit.slice(0, 7)}). Main stayed green each time. ` + | |
| 201 | `\`${t.name}\` in \`${t.file}\` failed in ${where} and passed when run alone, ${count} times in 7 days ` + | |
| 202 | `(latest: check run ${ref.runId}, ${ref.commit.slice(0, 7)}). The check stayed green each time. ` + | |
| 149 | 203 | `Find what it depends on: time, load, order, or shared state.\n\nFiles: ${t.file}`, |
| 150 | 204 | }); |
| 151 | 205 | if (!made.ok) continue; |
src/checks/service.ts +27 −7
| @@ -12,7 +12,7 @@ | ||
| 12 | 12 | */ |
| 13 | 13 | |
| 14 | 14 | import { pullDir, runDir } from "../core/pulls.ts"; |
| 15 | import { existsSync, mkdirSync, readdirSync, rmSync } from "node:fs"; | |
| 15 | import { existsSync, mkdirSync, readFileSync, readdirSync, rmSync } from "node:fs"; | |
| 16 | 16 | import { tmpdir } from "node:os"; |
| 17 | 17 | import { join } from "node:path"; |
| 18 | 18 | import type { App } from "../app.ts"; |
| @@ -57,7 +57,7 @@ import { readLearned } from "./learned.ts"; | ||
| 57 | 57 | import { keepCache, restoreCache } from "./cache.ts"; |
| 58 | 58 | import { MAX_FILED, clearedFiles, fileOrderDependent } from "./order_dependent.ts"; |
| 59 | 59 | import { parseBunTestFailure } from "./bun-test.ts"; |
| 60 | import { passedAlone, recordFlakes, rerunCommand, rerunnable } from "./flake.ts"; | |
| 60 | import { passedAlone, recordFlakes, rerunCommand, rerunnable, pullOwnsRed } from "./flake.ts"; | |
| 61 | 61 | import { budgetFiles, budgetReds, budgetRetryCommand, noteBudgetRetry, retryPassed } from "./budget_retry.ts"; |
| 62 | 62 | import { diskRecovered, withDisk } from "./disk_red.ts"; |
| 63 | 63 | import { ASKED, redMain, unheld } from "./red_main.ts"; |
| @@ -1066,9 +1066,28 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 1066 | 1066 | // Main's red costs the forge every pull's check (#6643): one or two |
| 1067 | 1067 | // failed tests are re-run alone, on this commit in this cell, before |
| 1068 | 1068 | // main counts as red. Passing alone is a flake: green, and recorded. |
| 1069 | if (!ran.ok && job === undefined && x.isMain === true) { | |
| 1069 | // A pull's red is the same unless the pull touched the test (#7426). | |
| 1070 | if (!ran.ok && job === undefined && (x.isMain === true || tip !== undefined)) { | |
| 1070 | 1071 | try { |
| 1071 | const tests = rerunnable(checkLines, parseBunTestFailure(checkLines)); | |
| 1072 | let tests = rerunnable(checkLines, parseBunTestFailure(checkLines)); | |
| 1073 | if (tests !== undefined && x.isMain !== true) { | |
| 1074 | const diff = await runGit( | |
| 1075 | ["diff", "--name-only", "-z", `${tip}...${x.commitOid}`], | |
| 1076 | { cwd: bare }, | |
| 1077 | ); | |
| 1078 | const changed = diff.ok ? diff.value.split("\0").filter((p) => p !== "") : undefined; | |
| 1079 | const read = (path: string): string | undefined => { | |
| 1080 | try { | |
| 1081 | return readFileSync(join(dir, path), "utf8"); | |
| 1082 | } catch { | |
| 1083 | return undefined; | |
| 1084 | } | |
| 1085 | }; | |
| 1086 | if (pullOwnsRed(tests, changed, read)) { | |
| 1087 | write("not re-run alone: the pull changed a failed test or what it imports"); | |
| 1088 | tests = undefined; | |
| 1089 | } | |
| 1090 | } | |
| 1072 | 1091 | if (tests !== undefined) { |
| 1073 | 1092 | write(`re-running ${tests.length} failed test(s) alone`); |
| 1074 | 1093 | let alone = true; |
| @@ -1087,13 +1106,14 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 1087 | 1106 | }); |
| 1088 | 1107 | if (!again.ok || !passedAlone(out)) { |
| 1089 | 1108 | alone = false; |
| 1090 | write(`"${test.name}" fails alone too: main is red`); | |
| 1109 | write(`"${test.name}" fails alone too: ${x.isMain === true ? "main" : "the check"} is red`); | |
| 1091 | 1110 | break; |
| 1092 | 1111 | } |
| 1093 | 1112 | } |
| 1094 | 1113 | if (alone) { |
| 1095 | recordFlakes(app.db, app.bus, repo, tests, { runId: run.id, commit: run.commitOid }); | |
| 1096 | write(`flaky: ${tests.map((t) => `"${t.name}"`).join(", ")} failed in the full run and passed alone; recorded, main stays green`); | |
| 1114 | recordFlakes(app.db, app.bus, repo, tests, { runId: run.id, commit: run.commitOid, pull: x.isMain !== true }); | |
| 1115 | for (const t of tests) write(`flake: ${t.file} > ${t.name} passed alone`); | |
| 1116 | write(`flaky: ${tests.map((t) => `"${t.name}"`).join(", ")} failed in the full run and passed alone; recorded, ${x.isMain === true ? "main" : "the check"} stays green`); | |
| 1097 | 1117 | ran = { ok: true, value: undefined }; |
| 1098 | 1118 | } |
| 1099 | 1119 | } |
test/check-pull-flake.test.ts added +136
| @@ -0,0 +1,136 @@ | ||
| 1 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 2 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 3 | import { tmpdir } from "node:os"; | |
| 4 | import { join } from "node:path"; | |
| 5 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 6 | import { openPull } from "../src/actions/pulls.ts"; | |
| 7 | import { createApp, type App } from "../src/app.ts"; | |
| 8 | import { recordFlakes, flakeList, pullOwnsRed, touchedByPull } from "../src/checks/flake.ts"; | |
| 9 | import { startChecks } from "../src/checks/service.ts"; | |
| 10 | import { listRuns } from "../src/core/runs.ts"; | |
| 11 | import { createUser } from "../src/core/users.ts"; | |
| 12 | import { readRunLog } from "../src/runs/log.ts"; | |
| 13 | import { quietLogs } from "./helpers/globals.ts"; | |
| 14 | ||
| 15 | quietLogs(); | |
| 16 | ||
| 17 | test("a test is the pull's when it, or anything it imports, changed; unreadable is the pull's too", () => { | |
| 18 | const files: Record<string, string> = { | |
| 19 | "t/a.test.ts": `import { h } from "./helper.ts";\nimport "../src/x";`, | |
| 20 | "t/helper.ts": `export const h = 1;`, | |
| 21 | "src/x.ts": `import { y } from "./y.ts";`, | |
| 22 | "src/y.ts": ``, | |
| 23 | }; | |
| 24 | const read = (p: string) => files[p]; | |
| 25 | expect(touchedByPull("t/a.test.ts", ["README.md"], read)).toBe(false); | |
| 26 | expect(touchedByPull("t/a.test.ts", ["t/a.test.ts"], read)).toBe(true); | |
| 27 | expect(touchedByPull("t/a.test.ts", ["t/helper.ts"], read)).toBe(true); | |
| 28 | expect(touchedByPull("t/a.test.ts", ["src/y.ts"], read)).toBe(true); | |
| 29 | expect(touchedByPull("t/a.test.ts", ["package.json"], read)).toBe(true); | |
| 30 | expect(touchedByPull("t/gone.test.ts", ["README.md"], read)).toBe(true); | |
| 31 | expect(touchedByPull("t/a.test.ts", ["README.md"], (p) => (p === "src/y.ts" ? undefined : files[p]))).toBe(true); | |
| 32 | }); | |
| 33 | ||
| 34 | test("with no list of changed paths (the diff failed) the pull owns the red", () => { | |
| 35 | const read = () => ""; | |
| 36 | const tests = [{ file: "t/a.test.ts", name: "x" }]; | |
| 37 | expect(pullOwnsRed(tests, undefined, read)).toBe(true); | |
| 38 | expect(pullOwnsRed(tests, ["README.md"], read)).toBe(false); | |
| 39 | }); | |
| 40 | ||
| 41 | let dir: string; | |
| 42 | let app: App; | |
| 43 | let alice: Awaited<ReturnType<typeof makeUser>>; | |
| 44 | async function makeUser() { | |
| 45 | const u = await createUser(app.db, { origin: "seed", trusted: true, username: "alice", email: "[email protected]", password: "a-good-passphrase", adminIfFirst: true }); | |
| 46 | if (!u.ok) throw new Error(u.error); | |
| 47 | return u.value; | |
| 48 | } | |
| 49 | beforeAll(async () => { | |
| 50 | dir = await mkdtemp(join(tmpdir(), "pull-flake-")); | |
| 51 | app = createApp({ dataDir: dir, port: 0, host: "forge.test" }); | |
| 52 | alice = await makeUser(); | |
| 53 | }); | |
| 54 | afterAll(async () => { | |
| 55 | await rm(dir, { recursive: true, force: true }); | |
| 56 | }); | |
| 57 | ||
| 58 | const spec = (body: string) => `import { test, expect, describe } from "bun:test";\ndescribe("grp", () => { test("flaky", () => { ${body} }); });\n`; | |
| 59 | ||
| 60 | /** A pull whose check prints `names` as failed in t/a.test.ts and exits 1; the branch writes `branchFiles`. */ | |
| 61 | async function pullCheck(name: string, names: string[], branchFiles: { path: string; content: string }[]) { | |
| 62 | process.env["FORGE_CELLS"] = "off"; | |
| 63 | const made = await createRepo(app, alice, { name, visibility: "public" }); | |
| 64 | if (!made.ok) throw new Error("repo"); | |
| 65 | const repo = made.value; | |
| 66 | const fails = names.map((n) => `(fail) ${n}\\n`).join(""); | |
| 67 | const fake = `printf 't/a.test.ts:\\n${fails} 0 pass\\n ${names.length} fail\\n'; exit 1`; | |
| 68 | const checks = startChecks(app, { forgeUrl: "http://forge.test", concurrency: 1 }); | |
| 69 | try { | |
| 70 | const base = await commitToBranch(app, alice, repo, { | |
| 71 | message: "base", | |
| 72 | files: [ | |
| 73 | { path: "forge.json", content: JSON.stringify({ check: { run: fake, install: "" } }) }, | |
| 74 | { path: "t/a.test.ts", content: spec("expect(1).toBe(1);") }, | |
| 75 | ], | |
| 76 | }); | |
| 77 | if (!base.ok) throw new Error(JSON.stringify(base.error)); | |
| 78 | const topic = await commitToBranch(app, alice, repo, { branch: "topic", message: "topic", files: branchFiles }); | |
| 79 | if (!topic.ok) throw new Error(JSON.stringify(topic.error)); | |
| 80 | const pull = await openPull(app, alice, repo, { title: "t", body: "", baseBranch: "main", headBranch: "topic" }); | |
| 81 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); | |
| 82 | const oid = topic.value.commitOid; | |
| 83 | const deadline = Date.now() + 40_000; | |
| 84 | const finished = () => listRuns(app.db, repo.id, { kind: "check", limit: 50 }).find((r) => r.commitOid === oid && (r.state === "ok" || r.state === "failed")); | |
| 85 | while (Date.now() < deadline && finished() === undefined) await Bun.sleep(100); | |
| 86 | const run = finished()!; | |
| 87 | return { oid, run, log: readRunLog(app.config.dataDir, run.id).lines.join("\n"), flakes: () => flakeList(app.db, Date.now()).filter((f) => f.repo === `alice/${name}`) }; | |
| 88 | } finally { | |
| 89 | await checks.stop(); | |
| 90 | } | |
| 91 | } | |
| 92 | ||
| 93 | test("an untouched test that fails and passes alone leaves the pull green and the flake recorded", async () => { | |
| 94 | const { run, log, flakes } = await pullCheck("untouched", ["grp > flaky"], [{ path: "notes.txt", content: "x" }]); | |
| 95 | expect(log).toContain("re-running 1 failed test(s) alone"); | |
| 96 | expect(log).toContain("flake: t/a.test.ts > grp > flaky passed alone"); | |
| 97 | expect(run.state).toBe("ok"); | |
| 98 | // Main's push flaked too; the pull's flake counts with it toward FILE_AT. | |
| 99 | expect(flakes()[0]).toMatchObject({ test: "grp > flaky", file: "t/a.test.ts", count: 2 }); | |
| 100 | }, 60_000); | |
| 101 | ||
| 102 | test("a test the pull changed is never re-run", async () => { | |
| 103 | const { oid, run, log, flakes } = await pullCheck("touched", ["grp > flaky"], [{ path: "t/a.test.ts", content: spec("expect(1).toBe(1); /* edit */") }]); | |
| 104 | expect(log).not.toContain("re-running"); | |
| 105 | expect(log).toContain("not re-run alone"); | |
| 106 | expect(run.state).toBe("failed"); | |
| 107 | // Only main's own push recorded a flake; the pull's commit did not. | |
| 108 | expect(flakes().every((f) => f.lastCommit !== oid)).toBe(true); | |
| 109 | }, 60_000); | |
| 110 | ||
| 111 | test("a shaping file in the pull keeps the red", async () => { | |
| 112 | const { run, log } = await pullCheck("shaping", ["grp > flaky"], [{ path: "package.json", content: "{}" }]); | |
| 113 | expect(log).toContain("not re-run alone"); | |
| 114 | expect(run.state).toBe("failed"); | |
| 115 | }); | |
| 116 | ||
| 117 | test("three failures are not re-run", async () => { | |
| 118 | const { run, log } = await pullCheck("three", ["a", "b", "c"], [{ path: "notes.txt", content: "x" }]); | |
| 119 | expect(log).not.toContain("re-running"); | |
| 120 | expect(run.state).toBe("failed"); | |
| 121 | }, 60_000); | |
| 122 | ||
| 123 | test("a flake issue says whether it flaked in a pull's check or main's", async () => { | |
| 124 | const made = await createRepo(app, alice, { name: "wording", visibility: "public" }); | |
| 125 | if (!made.ok) throw new Error("repo"); | |
| 126 | const t = [{ file: "t/a.test.ts", name: "grp > words" }]; | |
| 127 | const body = () => app.db.query<{ body: string }, [string]>("SELECT body FROM issue WHERE repo_id = ?1").all(made.value.id).map((i) => i.body).join("\n"); | |
| 128 | recordFlakes(app.db, app.bus, made.value, t, { runId: "r1", commit: "aaaaaaa1", pull: true }); | |
| 129 | recordFlakes(app.db, app.bus, made.value, t, { runId: "r2", commit: "bbbbbbb2", pull: true }); | |
| 130 | expect(body()).toContain("in a pull's check"); | |
| 131 | expect(body()).not.toContain("main's check"); | |
| 132 | const other = [{ file: "t/b.test.ts", name: "grp > main" }]; | |
| 133 | recordFlakes(app.db, app.bus, made.value, other, { runId: "r3", commit: "ccccccc3" }); | |
| 134 | recordFlakes(app.db, app.bus, made.value, other, { runId: "r4", commit: "ddddddd4" }); | |
| 135 | expect(body()).toContain("in main's check"); | |
| 136 | }); |
new above
offline
No test covers the git-diff-failed branch (changed === undefined). Add one that makes it fail and asserts the log says not re-run alone and the run is failed.