Public: anyone can read this; the crew can change it.
A crash outside any test re-runs that shard alone once before it can fail a check #8248
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/7781-a-crash-outside-any-test-re-runs-that-sh → main 4 commits fork
7 files
changed +109
−8
scripts/test-shards.ts +3
| @@ -89,6 +89,7 @@ import { fastSet, duration } from "../src/checks/tiers.ts"; | ||
| 89 | 89 | import { CHECK_CACHE } from "../src/checks/cache.ts"; |
| 90 | 90 | import { parseCheck } from "../src/apps/manifest.ts"; |
| 91 | 91 | import { FLAKY_ALONE, onlyFiles } from "../src/checks/verify.ts"; |
| 92 | import { crashFlakeLine } from "../src/checks/flake.ts"; | |
| 92 | 93 | |
| 93 | 94 | const TIMES = new URL("./test-times.json", import.meta.url).pathname; |
| 94 | 95 | // The forge hands the map over in the checkout (`src/checks/impact-store.ts`): |
| @@ -584,6 +585,8 @@ async function runShardIn( | ||
| 584 | 585 | process.stderr.write(`test-shards: re-run ${where} after a crash (exit ${first.code})\n`); |
| 585 | 586 | started = performance.now(); |
| 586 | 587 | first = await run(paths, cover, through); |
| 588 | // Green the second time: the crash is a flake, named for the check to record. | |
| 589 | if (first.code === 0) process.stderr.write(`${crashFlakeLine(where, quoted)}\n`); | |
| 587 | 590 | } |
| 588 | 591 | let code = first.code; |
| 589 | 592 | let output = quoted === "" ? first.output : [quoted, ...first.output]; |
src/checks/flake.ts +56
| @@ -29,6 +29,7 @@ const FAIL = /^\(fail\) (.+?)(?: \[[0-9.]+m?s\])?(?: \[shard [0-9]+\])?$/; | ||
| 29 | 29 | const PATH = /^[\w@./+-]{1,200}$/; |
| 30 | 30 | // eslint-disable-next-line no-control-regex |
| 31 | 31 | const CONTROL = /[\u0000-\u001f\u007f-\u009f]/; |
| 32 | const CONTROL_G = new RegExp(CONTROL.source, "g"); | |
| 32 | 33 | const NAME_MAX = 300; |
| 33 | 34 | |
| 34 | 35 | export interface Flaky { |
| @@ -156,6 +157,61 @@ export function passedAlone(lines: readonly string[]): boolean { | ||
| 156 | 157 | return pass === 1 && fail === 0; |
| 157 | 158 | } |
| 158 | 159 | |
| 160 | /** The marker a shard runner prints when a crash outside any test was green on its one re-run (#7781). */ | |
| 161 | export const CRASH_FLAKE = "test-shards: crash flake"; | |
| 162 | ||
| 163 | /** The test name a crash outside any test is recorded under: stable per message, so a repeat counts. */ | |
| 164 | export const CRASH_NAME = "(crash outside any test)"; | |
| 165 | ||
| 166 | /** | |
| 167 | * The marker line for a shard whose quoted crash (` │ ` lines) went green | |
| 168 | * on its re-run: the first error line, cleaned and cut, so one cause counts | |
| 169 | * as one flake. | |
| 170 | */ | |
| 171 | export function crashFlakeLine(where: string, quoted: string): string { | |
| 172 | const cause = | |
| 173 | quoted | |
| 174 | .split("\n") | |
| 175 | .map((l) => l.replace(/^ {2}│ /, "").trim()) | |
| 176 | .find((l) => /^\w*Error\b|^error\b/.test(l)) ?? "unhandled error"; | |
| 177 | const clean = cause.replace(CONTROL_G, " ").replace(/[0-9]+/g, "#").slice(0, 120); | |
| 178 | return `${CRASH_FLAKE} ${where}: ${clean}`; | |
| 179 | } | |
| 180 | ||
| 181 | /** Crashes a check's log says were green on a re-run, as flakes with no file (so no issue is filed). */ | |
| 182 | export function crashFlakes(lines: readonly string[]): Flaky[] { | |
| 183 | const seen = new Set<string>(); | |
| 184 | const out: Flaky[] = []; | |
| 185 | for (const line of lines) { | |
| 186 | const at = line.indexOf(`${CRASH_FLAKE} `); | |
| 187 | if (at < 0) continue; | |
| 188 | const m = /^[0-9]+\/[0-9]+: (.{1,120})$/.exec(line.slice(at + CRASH_FLAKE.length + 1).trim()); | |
| 189 | if (m === null || CONTROL.test(m[1]!)) continue; | |
| 190 | const name = `${CRASH_NAME} ${m[1]}`; | |
| 191 | if (!seen.has(name)) out.push({ file: "", name }); | |
| 192 | seen.add(name); | |
| 193 | } | |
| 194 | return out; | |
| 195 | } | |
| 196 | ||
| 197 | /** | |
| 198 | * Record the crash flakes a check's log names, but only for a check that | |
| 199 | * passed: a red check records nothing. Returns how many were recorded. | |
| 200 | */ | |
| 201 | export function recordCrashFlakes( | |
| 202 | db: Database, | |
| 203 | bus: EventBus, | |
| 204 | repo: Pick<Repo, "id" | "ownerId">, | |
| 205 | lines: readonly string[], | |
| 206 | ref: { runId: string; commit: string; pull?: boolean }, | |
| 207 | ok: boolean, | |
| 208 | ): number { | |
| 209 | if (!ok) return 0; | |
| 210 | const crashes = crashFlakes(lines); | |
| 211 | if (crashes.length > 0) recordFlakes(db, bus, repo, crashes, ref); | |
| 212 | return crashes.length; | |
| 213 | } | |
| 214 | ||
| 159 | 215 | export const flakeTitle = (name: string): string => `Flaky test: ${name.slice(0, 200)}`; |
| 160 | 216 | |
| 161 | 217 | /** |
src/checks/service.ts +9 −1
| @@ -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, pullOwnsRed } from "./flake.ts"; | |
| 60 | import { recordCrashFlakes, passedAlone, recordFlakes, rerunCommand, rerunnable, pullOwnsRed } from "./flake.ts"; | |
| 61 | 61 | import { diskRecovered, withDisk } from "./disk_red.ts"; |
| 62 | 62 | import { ASKED, redMain, unheld } from "./red_main.ts"; |
| 63 | 63 | import { proposeRevert } from "./revert_red.ts"; |
| @@ -1368,6 +1368,14 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 1368 | 1368 | write(`flake re-run failed: ${String(error)}`); |
| 1369 | 1369 | } |
| 1370 | 1370 | } |
| 1371 | // A shard that crashed outside any test and ran green once more (#7781). | |
| 1372 | { | |
| 1373 | try { | |
| 1374 | recordCrashFlakes(app.db, app.bus, repo, checkLines, { runId: run.id, commit: run.commitOid, pull: x.isMain !== true }, ran.ok); | |
| 1375 | } catch (error) { | |
| 1376 | write(`crash flakes not recorded: ${String(error)}`); | |
| 1377 | } | |
| 1378 | } | |
| 1371 | 1379 | if (!ran.ok) { |
| 1372 | 1380 | const { summary, detail } = summariseCheckFailure( |
| 1373 | 1381 | checkLines, |
test/check-flake.test.ts +12 −1
| @@ -5,7 +5,7 @@ import { join } from "node:path"; | ||
| 5 | 5 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| 6 | 6 | import { createApp, type App } from "../src/app.ts"; |
| 7 | 7 | import { redMain } from "../src/checks/red_main.ts"; |
| 8 | import { flakeList, flakeNeeds, passedAlone, recordFlakes, recordQueueFlakes, rerunCommand, rerunnable } from "../src/checks/flake.ts"; | |
| 8 | import { flakeList, flakeNeeds, passedAlone, recordCrashFlakes, recordFlakes, recordQueueFlakes, rerunCommand, rerunnable } from "../src/checks/flake.ts"; | |
| 9 | 9 | import { startChecks } from "../src/checks/service.ts"; |
| 10 | 10 | import { parseBunTestFailure } from "../src/checks/bun-test.ts"; |
| 11 | 11 | import { listRuns } from "../src/core/runs.ts"; |
| @@ -160,3 +160,14 @@ test("a test that fails alone too leaves main red", async () => { | ||
| 160 | 160 | expect(redMain(app.db, repo.id)?.failing).toEqual(["grp > flaky"]); |
| 161 | 161 | expect(flakeList(app.db, Date.now()).some((f) => f.repo === "alice/broken")).toBe(false); |
| 162 | 162 | }, 60_000); |
| 163 | ||
| 164 | test("a crash-flake line in a passing check's log is recorded as a file-less check_flake row; a failed check records nothing (#7781)", async () => { | |
| 165 | const made = await createRepo(app, alice, { name: "crashes", visibility: "public" }); | |
| 166 | if (!made.ok) throw new Error("repo"); | |
| 167 | const log = ["noise", "test-shards: crash flake 4/9: error: Cannot use a closed database #", "ok"]; | |
| 168 | const rows = () => app.db.query<{ test: string; file: string }, [string]>("SELECT test, file FROM check_flake WHERE repo_id = ?1").all(made.value.id); | |
| 169 | expect(recordCrashFlakes(app.db, app.bus, made.value, log, { runId: "r1", commit: "aaaaaaa1" }, false)).toBe(0); | |
| 170 | expect(rows()).toEqual([]); | |
| 171 | expect(recordCrashFlakes(app.db, app.bus, made.value, log, { runId: "r2", commit: "bbbbbbb2", pull: true }, true)).toBe(1); | |
| 172 | expect(rows()).toEqual([{ test: "(crash outside any test) error: Cannot use a closed database #", file: "" }]); | |
| 173 | }); |
test/danger-buttons.test.ts +1 −2
| @@ -11,10 +11,9 @@ const web = join(root, "src", "web"); | ||
| 11 | 11 | |
| 12 | 12 | /** Today's danger POST buttons that do not ask first, one `file — reason` per line. */ |
| 13 | 13 | const allowed: readonly { file: string; button: string; reason: string }[] = [ |
| 14 | { file: "src/web/pages/boards-view.ts", button: `"Delete this board"`, reason: "board delete; waits for its confirm issue" }, | |
| 15 | { file: "src/web/pages/org.ts", button: `"Remove"`, reason: "team member remove; waits for its confirm issue" }, | |
| 16 | 14 | { file: "src/web/pages/edit.ts", button: `"Delete this file"`, reason: "the delete page is the confirmation, reached by a link" }, |
| 17 | 15 | { file: "src/web/pages/pull.ts", button: `"Land past the gate"`, reason: "asks for a typed reason of at least 8 characters" }, |
| 16 | { file: "src/web/studio/change.ts", button: `"Send it back"`, reason: "a changes-requested verdict; the next review replaces it" }, | |
| 18 | 17 | { file: "src/web/studio/notes.ts", button: "label", reason: "owner note ban; undone by unban on the same row" }, |
| 19 | 18 | ]; |
| 20 | 19 |
test/flake.test.ts +22
| @@ -424,3 +424,25 @@ describe("the scheduled run of the files outside the gate", () => { | ||
| 424 | 424 | expect(redFile("Red: test/a.test.ts")).toBe("test/a.test.ts"); |
| 425 | 425 | }); |
| 426 | 426 | }); |
| 427 | ||
| 428 | describe("a crash outside any test that ran green on its re-run (#7781)", () => { | |
| 429 | const quoted = " │ # Unhandled error between tests\n │ error: Cannot use a closed database 42\n"; | |
| 430 | test("is named by one stable line and read back as a flake with no file", async () => { | |
| 431 | const { crashFlakeLine, crashFlakes } = await import("../src/checks/flake.ts"); | |
| 432 | const line = crashFlakeLine("3/9", quoted); | |
| 433 | expect(line).toBe("test-shards: crash flake 3/9: error: Cannot use a closed database #"); | |
| 434 | const got = crashFlakes(["noise", line, line, "(fail) a real failing test"]); | |
| 435 | expect(got).toEqual([{ file: "", name: "(crash outside any test) error: Cannot use a closed database #" }]); | |
| 436 | }); | |
| 437 | test("a real failing test is no crash flake", async () => { | |
| 438 | const { crashFlakes } = await import("../src/checks/flake.ts"); | |
| 439 | expect(crashFlakes(["(fail) a [1ms]", " 1 fail"])).toEqual([]); | |
| 440 | }); | |
| 441 | }); | |
| 442 | ||
| 443 | test("a crash of another Error class keeps its own cause, not one shared bucket (#7781)", async () => { | |
| 444 | const { crashFlakeLine } = await import("../src/checks/flake.ts"); | |
| 445 | expect(crashFlakeLine("1/2", " │ SyntaxError: bad token 7\n")).toBe("test-shards: crash flake 1/2: SyntaxError: bad token #"); | |
| 446 | expect(crashFlakeLine("1/2", " │ AssertionError: x\n")).toContain("AssertionError"); | |
| 447 | expect(crashFlakeLine("1/2", " │ # Unhandled error between tests\n")).toBe("test-shards: crash flake 1/2: unhandled error"); | |
| 448 | }); |
test/shards.test.ts +6 −4
| @@ -920,8 +920,8 @@ describe("a failing file is run once more alone", () => { | ||
| 920 | 920 | env, |
| 921 | 921 | }, |
| 922 | 922 | ); |
| 923 | const [out] = await Promise.all([new Response(child.stdout).text(), new Response(child.stderr).text()]); | |
| 924 | return { code: await child.exited, out }; | |
| 923 | const [out, err] = await Promise.all([new Response(child.stdout).text(), new Response(child.stderr).text()]); | |
| 924 | return { code: await child.exited, out, err }; | |
| 925 | 925 | }; |
| 926 | 926 | |
| 927 | 927 | test("red only beside another file: the shard is green and the tail names it", async () => { |
| @@ -991,8 +991,8 @@ describe("every shard runs to the end, and a crashed one once more", () => { | ||
| 991 | 991 | const env = { ...process.env, FORGE_CHECK_SHARDS: shards, FORGE_CELL_CPUS: "4", SHARD_CRASH_MARK: mark } as Record<string, string | undefined>; |
| 992 | 992 | for (const name of Object.keys(env)) if (name.startsWith("FORGE_CHECK_") && name !== "FORGE_CHECK_SHARDS") delete env[name]; |
| 993 | 993 | const child = Bun.spawn(["bun", script, ...files], { stdin: "ignore", stdout: "pipe", stderr: "pipe", env }); |
| 994 | const [out] = await Promise.all([new Response(child.stdout).text(), new Response(child.stderr).text()]); | |
| 995 | return { code: await child.exited, out }; | |
| 994 | const [out, err] = await Promise.all([new Response(child.stdout).text(), new Response(child.stderr).text()]); | |
| 995 | return { code: await child.exited, out, err }; | |
| 996 | 996 | }; |
| 997 | 997 | |
| 998 | 998 | test("a red file does not stop the files after it", async () => { |
| @@ -1009,6 +1009,8 @@ describe("every shard runs to the end, and a crashed one once more", () => { | ||
| 1009 | 1009 | expect(got.out).toContain("crashed outside any test (exit 1); re-running it once. The crashed run:"); |
| 1010 | 1010 | expect(got.out).toContain(" │ error: Cannot use a closed database"); |
| 1011 | 1011 | expect(got.code).toBe(0); |
| 1012 | // Green on the re-run: the crash is named for the check to record as a flake. | |
| 1013 | expect(got.err).toMatch(/test-shards: crash flake \d+\/\d+: error: Cannot use a closed database/); | |
| 1012 | 1014 | }, 30_000); |
| 1013 | 1015 | |
| 1014 | 1016 | test("a single worker quotes the crashed run too: no bare error lines, and the re-run is green", async () => { |
new above
offline