trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRunsg rEventsFlows

A full-suite issue closes itself when a later full run on main passes its file #9534

Merged masoncrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

mason/forge:mason/9524-a-full-suite-issue-closes-itself-when-a → main 1 commit fork

  • Merged. mason/9524-a-full-suite-issue-closes-itself-when-a was merged into main as 940f81a.

Review

Sign in to review this change.
5 files changed +204 −13
src/checks/report.ts +53 −2
@@ -10,7 +10,7 @@
1010
1111 import type { App } from "../app.ts";
1212 import type { Database } from "bun:sqlite";
13 import { addComment, createLabel, getLabel, listIssues, type Label } from "../core/issues.ts";
13 import { addComment, createLabel, getLabel, listIssues, setIssueState, type Label } from "../core/issues.ts";
1414 import { getRepo, type Repo } from "../core/repos.ts";
1515 import { ownRepoPath } from "../kernel/own_repo.ts";
1616 import type { CheckDetail, Run } from "../core/runs.ts";
@@ -25,6 +25,8 @@ export const MAX_FAILURES = 40;
2525 const NAMED = 10;
2626
2727 export const reportTitle = (file: string): string => `Full suite red: ${file}`;
28 /** How every report issue's body opens: with the label and title, what tells it from one a person wrote. */
29 const FILED_BY = "_Filed by the scheduled full-suite report, not written by a person.";
2830
2931 /** A red verification's detail with the failing tests under its failing files (at most {@link MAX_FAILURES}). */
3032 export function redDetail(
@@ -61,7 +63,7 @@ export function reportRed(app: App, repo: Repo, run: Run): string[] {
6163 const filed = fileIssueOnce(app.db, app.bus, repo, {
6264 title,
6365 body:
64 `_Filed by the scheduled full-suite report, not written by a person. It holds no pull, batch or release._\n\n` +
66 `${FILED_BY} It holds no pull, batch or release._\n\n` +
6567 `\`${file}\` failed in the full suite on ${repo.defaultBranch} (${where}).` +
6668 `${said}${more}\n\nFiles: ${file}`,
6769 ...(label === undefined ? {} : { labelId: label.id }),
@@ -84,6 +86,55 @@ export function reportRed(app: App, repo: Repo, run: Run): string[] {
8486 return lines;
8587 }
8688
89 /**
90 * The lines for a finished verification's log: one per open report issue
91 * whose file ran and passed every test in it (#9524), commented on and
92 * closed as done. `passed` is what the run's own log showed
93 * (`verify.ts` `passedFiles`); a file it never printed (a shard that did
94 * not run, a run cut short) keeps its issue open. Only an issue the report
95 * filed is closed: its label, its exact title, the owner as author and the
96 * report's own opening line, never one a person wrote.
97 */
98 export function reportGreen(app: App, repo: Repo, run: Run, passed: readonly string[]): string[] {
99 if (run.state !== "ok" && run.state !== "failed") return [];
100 if (passed.length === 0) return [];
101 const ran = new Map(passed.map((file) => [reportTitle(file), file]));
102 const open = app.db
103 .query<{ id: string; number: number; title: string; body: string }, [string, string, string]>(
104 `SELECT issue.id, issue.number, issue.title, issue.body FROM issue
105 WHERE issue.repo_id = ?1 AND issue.kind = 'issue' AND issue.state = 'open'
106 AND issue.author_id = ?2
107 AND EXISTS (SELECT 1 FROM issue_label JOIN label ON label.id = issue_label.label_id
108 WHERE issue_label.issue_id = issue.id AND label.name = ?3)
109 ORDER BY issue.number`,
110 )
111 .all(repo.id, repo.ownerId, REPORT_LABEL);
112 const where = `check run ${run.id}, ${run.commitOid.slice(0, 7)}`;
113 const lines: string[] = [];
114 for (const issue of open) {
115 const file = ran.get(issue.title);
116 if (file === undefined || !issue.body.startsWith(FILED_BY)) continue;
117 const body = `Green again in the full suite (${where}).`;
118 const noted = addComment(app.db, { issueId: issue.id, authorId: repo.ownerId, body });
119 if (noted.ok)
120 app.bus.append({
121 kind: "issue.commented",
122 repoId: repo.id,
123 actorId: repo.ownerId,
124 payload: { number: issue.number, body, commentId: noted.value.id },
125 });
126 if (setIssueState(app.db, issue.id, "closed", "done") === undefined) continue;
127 app.bus.append({
128 kind: "issue.closed",
129 repoId: repo.id,
130 actorId: repo.ownerId,
131 payload: { number: issue.number },
132 });
133 lines.push(`report: closed #${issue.number}, ${file} green again`);
134 }
135 return lines;
136 }
137
87138 /** Where the report's issues are listed: the forge's own repository, filtered by the label. */
88139 export const reportPath = (): string => `/${ownRepoPath()}/issues?label=${REPORT_LABEL}`;
89140
src/checks/service.ts +15 −6
@@ -67,12 +67,13 @@ import { proposeRevert } from "./revert_red.ts";
6767 import { rebasesFirst } from "./finishing_first.ts";
6868 import { IMPACT_FILE, keepImpact, seedImpact } from "./impact-store.ts";
6969 import { afterVerify } from "./culprit.ts";
70 import { redDetail, reportRed } from "./report.ts";
70 import { redDetail, reportGreen, reportRed } from "./report.ts";
7171 import {
7272 bisectOrigin,
7373 failedFiles,
7474 inVerifyLane,
7575 onlyEnv,
76 passedFiles,
7677 VERIFY_ENV,
7778 VERIFY_EVERY_MS,
7879 VERIFY_REASON,
@@ -871,8 +872,11 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
871872 log.error("revert proposal failed", { error: String(error) }),
872873 );
873874 // A red verification after landing names its culprit (#7786).
874 if (done && run.env === VERIFY_ENV)
875 if (done && run.env === VERIFY_ENV) {
875876 reportRed(app, repo, done).forEach(write);
877 // A file the run printed passing closes its open report issue (#9524).
878 reportGreen(app, repo, done, outcome.passed ?? []).forEach(write);
879 }
876880 if (done && inVerifyLane(run.env))
877881 await afterVerify(app, repo, done, (r, env, oid, reason) => request(r, env, oid, reason))
878882 .then((lines) => lines.forEach(write))
@@ -888,8 +892,8 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
888892 write: (line: string) => void,
889893 signal: AbortSignal,
890894 ): Promise<
891 | { ok: true; proven?: string }
892 | { ok: false; error: string; detail?: CheckDetail | undefined; infrastructure?: boolean }
895 | { ok: true; proven?: string; passed?: readonly string[] }
896 | { ok: false; error: string; detail?: CheckDetail | undefined; infrastructure?: boolean; passed?: readonly string[] }
893897 > {
894898 const bare = await runDir(app.db, app.reposRoot, repo, run.issueId);
895899 // A pull's check is the fast tier, judged against where the pull left
@@ -967,11 +971,13 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
967971 async function runTier(
968972 x: Execution,
969973 ): Promise<
970 | { ok: true; proven?: string }
974 | { ok: true; proven?: string; passed?: readonly string[] }
971975 | {
972976 ok: false;
973977 error: string;
974978 detail?: CheckDetail | undefined;
979 /** A whole verification: the files its log ran to the end green (#9524). */
980 passed?: readonly string[];
975981 infrastructure?: boolean;
976982 /** Where it stopped red (#5276); none is before any step ran. */
977983 stage?: GateStage;
@@ -1199,6 +1205,8 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
11991205 });
12001206 let ran: Result<void, string>;
12011207 const checkLines: string[] = [];
1208 // A bisection step runs only some files: what it shows green says nothing of the rest.
1209 const wholeVerify = x.tier === "verify" && x.only === undefined;
12021210 if (job !== undefined) {
12031211 ran = await runJobStep(job, {
12041212 runId: run.id,
@@ -1425,6 +1433,7 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
14251433 ok: false,
14261434 error: killed ? ran.error : summary,
14271435 detail: redDetail(detail, files, checkLines),
1436 ...(wholeVerify ? { passed: passedFiles(checkLines) } : {}),
14281437 stage: checkStage(ran.error),
14291438 ...(killed ? { killed } : {}),
14301439 };
@@ -1444,7 +1453,7 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
14441453 }
14451454 }
14461455 if (key !== undefined) recordProven(app.db, key, x.id);
1447 return { ok: true };
1456 return wholeVerify ? { ok: true, passed: passedFiles(checkLines) } : { ok: true };
14481457 } finally {
14491458 if (existsSync(dir)) await removeRunDir(dir);
14501459 await removeRunDir(home);
src/checks/verify.ts +32
@@ -125,6 +125,38 @@ export function failedFiles(lines: readonly string[]): string[] {
125125 return found;
126126 }
127127
128 const PASSED = /^\(pass\) /;
129
130 /**
131 * The test files a `bun test` log ran to the end with every test passing
132 * (#9524), once each, in order. A file counts only when bun printed its
133 * header, at least one `(pass)` and no `(fail)` under it anywhere in the
134 * log, and its block was closed by the next header or bun's closing
135 * summary: a shard killed mid-file leaves that file out. A file the log
136 * never printed (a shard that did not run) is not in it.
137 */
138 export function passedFiles(lines: readonly string[]): string[] {
139 const passed = new Set<string>();
140 const failed = new Set<string>();
141 const finished: string[] = [];
142 let current: string | undefined;
143 const close = (): void => {
144 if (current !== undefined && !finished.includes(current)) finished.push(current);
145 current = undefined;
146 };
147 for (const line of lines) {
148 const header = HEADER.exec(line);
149 if (header) {
150 close();
151 current = header[1]!.replace(/^\.\//, "");
152 } else if (isSummaryLine(line)) close();
153 else if (current === undefined) continue;
154 else if (FAILED.test(line)) failed.add(current);
155 else if (PASSED.test(line)) passed.add(current);
156 }
157 return finished.filter((file) => safeFile(file) && passed.has(file) && !failed.has(file));
158 }
159
128160 /** A test file path the forge hands a check to run alone. */
129161 export const safeFile = (path: string): boolean => PATH.test(path) && !path.split("/").includes("..");
130162
test/check-verify.test.ts +30 −2
@@ -31,14 +31,14 @@ const report = (fail: boolean): string =>
3131 ? `printf 't/a.test.ts:\\n(fail) grp > works\\n 0 pass\\n 1 fail\\n'; exit 1`
3232 : `printf 't/a.test.ts:\\n(pass) grp > works\\n 1 pass\\n 0 fail\\n'`;
3333
34 async function push(fail: boolean): Promise<string> {
34 async function push(fail: boolean, note = ""): Promise<string> {
3535 const pushed = await commitToBranch(app, alice, repo, {
3636 message: fail ? "breaks it" : "works",
3737 files: [
3838 { path: "forge.json", content: JSON.stringify({ check: { run: "true", verify: report(fail), install: "" } }) },
3939 {
4040 path: "t/a.test.ts",
41 content: `import { test, expect, describe } from "bun:test";\ndescribe("grp", () => { test("works", () => { expect(${fail ? 2 : 1}).toBe(1); }); });\n`,
41 content: `${note}import { test, expect, describe } from "bun:test";\ndescribe("grp", () => { test("works", () => { expect(${fail ? 2 : 1}).toBe(1); }); });\n`,
4242 },
4343 ],
4444 });
@@ -116,3 +116,31 @@ test("a red verification that fails alone too names the push that broke it in on
116116 expect(log).toContain("fails alone too");
117117 expect(log).toContain("landed by no pull");
118118 }, 60_000);
119
120 test("a later green verification closes the report's issue for the file it passed (#9524)", async () => {
121 // A tree never proven green before: a proven one is not run again, and prints no file.
122 const oid = await push(false, "// fixed\n");
123 await until(() => finished(repo.defaultBranch, oid));
124 expect(await checks.verifyDue(Date.now() + 62 * 60_000)).toHaveLength(1);
125 const run = await until(() => finished(VERIFY_ENV, oid));
126 expect(run.state).toBe("ok");
127 const closed = await until(() =>
128 app.db
129 .query<{ id: string; state: string; close_reason: string | null }, [string]>(
130 "SELECT id, state, close_reason FROM issue WHERE repo_id = ?1 AND title = 'Full suite red: t/a.test.ts' AND state = 'closed'",
131 )
132 .get(repo.id) ?? undefined,
133 );
134 expect(closed.close_reason).toBe("done");
135 const said = app.db
136 .query<{ body: string }, [string]>("SELECT body FROM comment WHERE issue_id = ?1")
137 .all(closed.id)
138 .map((c) => c.body);
139 expect(said).toContain(`Green again in the full suite (check run ${run.id}, ${oid.slice(0, 7)}).`);
140 // The culprit's issue is not the report's: it stays open.
141 const culprit = app.db
142 .query<{ state: string }, [string]>("SELECT state FROM issue WHERE repo_id = ?1 AND title LIKE 'Red after landing:%'")
143 .get(repo.id);
144 expect(culprit?.state).toBe("open");
145 expect(readRunLog(app.config.dataDir, run.id).lines.join("\n")).toContain("t/a.test.ts green again");
146 }, 60_000);
test/full-suite-report.test.ts +74 −3
@@ -11,11 +11,11 @@ import { join } from "node:path";
1111 import { createApp, type App } from "../src/app.ts";
1212 import { createRepo } from "../src/actions/repos.ts";
1313 import { flowLines } from "../src/tui/screens/flow.ts";
14 import { MAX_FAILURES, redDetail, reportRed, reportTitle, REPORT_LABEL } from "../src/checks/report.ts";
14 import { MAX_FAILURES, redDetail, reportGreen, reportRed, reportTitle, REPORT_LABEL } from "../src/checks/report.ts";
1515 import { failedTests } from "../src/checks/flake.ts";
1616 import { heldByRed, redMain } from "../src/checks/red_main.ts";
17 import { failedFiles, VERIFY_ENV } from "../src/checks/verify.ts";
18 import { listIssues, listComments, labelsFor } from "../src/core/issues.ts";
17 import { failedFiles, passedFiles, VERIFY_ENV } from "../src/checks/verify.ts";
18 import { createIssue, getLabel, listIssues, listComments, labelsFor, setIssueLabels } from "../src/core/issues.ts";
1919 import type { Repo } from "../src/core/repos.ts";
2020 import { createRun, getRun, type Run } from "../src/core/runs.ts";
2121 import { createUser } from "../src/core/users.ts";
@@ -90,6 +90,25 @@ describe("failedTests", () => {
9090 });
9191 });
9292
93 describe("passedFiles", () => {
94 test("only a file printed whole with passes and no failure", () => {
95 expect(passedFiles(BUN_LOG)).toEqual(["test/c.test.ts"]);
96 });
97
98 test("a file cut off before its block closed is not passed", () => {
99 expect(passedFiles(["test/a.test.ts:", "(pass) a", "test/b.test.ts:", "(pass) b"])).toEqual(["test/a.test.ts"]);
100 });
101
102 test("a file that failed in one block and passed in another is not passed", () => {
103 const lines = ["test/a.test.ts:", "(fail) a", " 0 pass", "test/a.test.ts:", "(pass) a", " 1 pass"];
104 expect(passedFiles(lines)).toEqual([]);
105 });
106
107 test("a file with no passing test, or a path that climbs out, is not passed", () => {
108 expect(passedFiles(["test/a.test.ts:", "(skip) a", "../x.test.ts:", "(pass) x", " 1 pass"])).toEqual([]);
109 });
110 });
111
93112 describe("the report", () => {
94113 let dataDir: string;
95114 let app: App;
@@ -208,6 +227,58 @@ describe("the report", () => {
208227 expect(b.body).not.toContain("a > one");
209228 });
210229
230 const green = (commit: string): Run => {
231 const run = createRun(app.db, { repoId: repo.id, kind: "check", env: VERIFY_ENV, commitOid: commit, reason: "schedule" });
232 app.db.query("UPDATE run SET state = 'ok', finished_at = ?2 WHERE id = ?1").run(run.id, new Date().toISOString());
233 return getRun(app.db, run.id)!;
234 };
235 const issueFor = (file: string) => reports().find((i) => i.title === reportTitle(file))!;
236
237 test("red, then a green run that passed the file closes its issue, then a run without the file leaves the other open (#9524)", () => {
238 reportRed(app, repo, red(VERIFY_ENV, "a".repeat(40)));
239 const ok = green("e".repeat(40));
240 expect(reportGreen(app, repo, ok, ["test/a.test.ts", "test/z.test.ts"])).toEqual([
241 `report: closed #${issueFor("test/a.test.ts").number}, test/a.test.ts green again`,
242 ]);
243 const a = issueFor("test/a.test.ts");
244 expect(a.state).toBe("closed");
245 expect(a.closeReason).toBe("done");
246 expect(listComments(app.db, a.id).map((c) => c.body)).toEqual([
247 `Green again in the full suite (check run ${ok.id}, eeeeeee).`,
248 ]);
249 // b's shard never ran in that run: its issue stays open, and stays open again.
250 expect(issueFor("test/b.test.ts").state).toBe("open");
251 expect(reportGreen(app, repo, green("f".repeat(40)), ["test/c.test.ts"])).toEqual([]);
252 expect(issueFor("test/b.test.ts").state).toBe("open");
253 expect(listComments(app.db, issueFor("test/b.test.ts").id)).toHaveLength(0);
254 });
255
256 test("a red run closes the issue of a file it passed, and keeps the one it failed", () => {
257 reportRed(app, repo, red(VERIFY_ENV, "a".repeat(40)));
258 const run = createRun(app.db, { repoId: repo.id, kind: "check", env: VERIFY_ENV, commitOid: "d".repeat(40) });
259 app.db.query("UPDATE run SET state = 'failed' WHERE id = ?1").run(run.id);
260 expect(reportGreen(app, repo, getRun(app.db, run.id)!, ["test/b.test.ts"])).toHaveLength(1);
261 expect(issueFor("test/a.test.ts").state).toBe("open");
262 expect(issueFor("test/b.test.ts").state).toBe("closed");
263 });
264
265 test("an issue a person wrote, with the report's label and title, is never closed", () => {
266 reportRed(app, repo, red(VERIFY_ENV, "a".repeat(40)));
267 const label = getLabel(app.db, repo.id, REPORT_LABEL)!;
268 const made = createIssue(app.db, { repoId: repo.id, authorId: repo.ownerId, title: reportTitle("test/p.test.ts"), body: "I saw this fail." });
269 if (!made.ok) throw new Error("issue");
270 setIssueLabels(app.db, made.value.id, [label.id]);
271 expect(reportGreen(app, repo, green("e".repeat(40)), ["test/p.test.ts"])).toEqual([]);
272 expect(issueFor("test/p.test.ts").state).toBe("open");
273 });
274
275 test("a run that is still going closes nothing", () => {
276 reportRed(app, repo, red(VERIFY_ENV, "a".repeat(40)));
277 const run = createRun(app.db, { repoId: repo.id, kind: "check", env: VERIFY_ENV, commitOid: "e".repeat(40) });
278 expect(reportGreen(app, repo, run, ["test/a.test.ts"])).toEqual([]);
279 expect(issueFor("test/a.test.ts").state).toBe("open");
280 });
281
211282 test("the issue says only what the run recorded", () => {
212283 reportRed(app, repo, red(VERIFY_ENV, "a".repeat(40)));
213284 expect(reports()[0]!.body).not.toContain("run alone");

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs