diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index f3ca3bccad494..bb757a2e1cd86 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -541,6 +541,136 @@ describe("evaluatePullRequestWatch", () => { assert.deepEqual(evaluatePullRequestWatch(final.next, recovered, noRemarks).changes, []); }); + it("wakes once when the other checks finish after one failed where none is required", () => { + const sonar = check("sonar", "cancelled"); + const early = detail({ checks: [sonar, check("unit", "pending"), check("e2e", "pending")] }); + const first = evaluatePullRequestWatch(watch(), early, noRemarks); + assert.deepEqual(first.changes, [{ kind: "checks-failed", failed: [sonar] }]); + + const partway = detail({ checks: [sonar, check("unit", "success"), check("e2e", "pending")] }); + const waiting = evaluatePullRequestWatch(first.next, partway, noRemarks); + assert.deepEqual(waiting.changes, []); + + const done = detail({ checks: [sonar, check("unit", "success"), check("e2e", "success")] }); + const final = evaluatePullRequestWatch(waiting.next, done, noRemarks); + assert.deepEqual(final.changes, [ + { kind: "checks-finished", failing: [sonar], required: false }, + ]); + assert.deepEqual(evaluatePullRequestWatch(final.next, done, noRemarks).changes, []); + + // An owed check stays owed while it runs, even when the failed one turns required. + const gated = detail({ + checks: [{ ...sonar, required: true }, check("unit", "success"), check("e2e", "pending")], + }); + const still = evaluatePullRequestWatch(waiting.next, gated, noRemarks); + assert.deepEqual(still.changes, []); + assert.deepEqual(still.next.owedChecks, ["e2e"]); + + // Another gate failure owes the gate's running checks on top of those still owed. + const lint = { ...check("lint", "failure"), required: true }; + const another = detail({ checks: [...gated.checks, lint] }); + const both = evaluatePullRequestWatch(still.next, another, noRemarks); + assert.deepEqual(both.changes, [{ kind: "checks-failed", failed: [lint] }]); + assert.deepEqual(both.next.owedChecks, ["e2e"]); + + // The last owed check finishing still wakes when an unrelated check fails in the same read. + const bot = check("bot", "failure"); + const finishedWithBot = detail({ + checks: [sonar, check("unit", "success"), check("e2e", "success"), bot], + }); + assert.deepEqual(evaluatePullRequestWatch(waiting.next, finishedWithBot, noRemarks).changes, [ + { kind: "checks-failed", failed: [bot] }, + { kind: "checks-finished", failing: [sonar, bot], required: false }, + ]); + + // A later failure is its own wake and leaves nothing more owed. + const broke = detail({ checks: [sonar, check("unit", "success"), check("e2e", "failure")] }); + const failure = evaluatePullRequestWatch(waiting.next, broke, noRemarks); + assert.deepEqual(failure.changes, [ + { kind: "checks-failed", failed: [check("e2e", "failure")] }, + ]); + assert.deepEqual(evaluatePullRequestWatch(failure.next, broke, noRemarks).changes, []); + + // A failure while another owed check still runs keeps that one owed. + const unitBroke = detail({ + checks: [sonar, check("unit", "failure"), check("e2e", "pending")], + }); + const second = evaluatePullRequestWatch(first.next, unitBroke, noRemarks); + assert.deepEqual(second.changes, [ + { kind: "checks-failed", failed: [check("unit", "failure")] }, + ]); + const settled = detail({ checks: [sonar, check("unit", "failure"), check("e2e", "success")] }); + const last = evaluatePullRequestWatch(second.next, settled, noRemarks); + assert.deepEqual(last.changes, [ + { kind: "checks-finished", failing: [sonar, check("unit", "failure")], required: false }, + ]); + }); + + it("wakes once when the other required checks finish after a required one failed", () => { + const required = (name: string, status: PullRequestCheck["status"]) => ({ + ...check(name, status), + required: true, + }); + const early = detail({ + checks: [ + required("lint", "failure"), + required("unit", "pending"), + check("bot", "failure"), + check("e2e", "pending"), + ], + }); + const first = evaluatePullRequestWatch(watch(), early, noRemarks); + assert.equal(first.changes.length, 1); + assert.deepEqual(first.next.owedChecks, ["unit"]); + + const done = detail({ + checks: [ + required("lint", "failure"), + required("unit", "success"), + check("bot", "failure"), + check("e2e", "pending"), + ], + }); + const final = evaluatePullRequestWatch(first.next, done, noRemarks); + assert.deepEqual(final.changes, [ + { kind: "checks-finished", failing: [required("lint", "failure")], required: true }, + ]); + assert.deepEqual(evaluatePullRequestWatch(final.next, done, noRemarks).changes, []); + const message = pullRequestWatchMessage({ + number: 7, + url: "https://github.com/o/r/pull/7", + baseBranch: "main", + headSha: "aaaaaaaaaa", + report: final, + }); + assert.equal(message.notification.outcome, "failed"); + + // A new advisory failure keeps what was owed rather than owing a required check started since. + const advisory = detail({ + checks: [ + required("lint", "failure"), + required("unit", "pending"), + required("smoke", "pending"), + check("bot", "failure"), + check("e2e", "failure"), + ], + }); + const later = evaluatePullRequestWatch(first.next, advisory, noRemarks); + assert.deepEqual(later.changes, [{ kind: "checks-failed", failed: [check("e2e", "failure")] }]); + assert.deepEqual(later.next.owedChecks, ["unit"]); + + // A rerun of the failed required check is not the gate finishing, even with an advisory failure. + const rerun = detail({ + checks: [ + required("lint", "pending"), + required("unit", "success"), + check("bot", "failure"), + check("e2e", "pending"), + ], + }); + assert.deepEqual(evaluatePullRequestWatch(first.next, rerun, noRemarks).changes, []); + }); + it("clears old-head owed checks when a new head is pushed", () => { const required = { ...check("test", "success"), required: true }; const early = detail({ checks: [required, check("e2e", "pending")] }); diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index b125496010d6a..beccd4a3a271a 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -17,6 +17,11 @@ const SNIPPET_LENGTH = 200; export type PullRequestWatchChange = | { readonly kind: "checks-failed"; readonly failed: ReadonlyArray } | { readonly kind: "checks-passed"; readonly count: number; readonly required: boolean } + | { + readonly kind: "checks-finished"; + readonly failing: ReadonlyArray; + readonly required: boolean; + } | { readonly kind: "checks-awaiting-approval"; readonly checks: ReadonlyArray } | { readonly kind: "remarks"; readonly remarks: ReadonlyArray } | { readonly kind: "no-checks"; readonly minutes: number } @@ -55,6 +60,7 @@ export function evaluatePullRequestWatch( let passed = headMoved ? false : watch.passed; let passedChecks = headMoved ? [] : watch.passedChecks; let owedChecks = headMoved ? [] : (watch.owedChecks ?? []); + const owedBefore = owedChecks; let awaitingApproval = headMoved ? [] : (watch.awaitingApproval ?? []); let approvalAnnounced = headMoved ? false : (watch.approvalAnnounced ?? false); // An unavailable approval read cannot establish success or that no checks exist. @@ -62,8 +68,8 @@ export function evaluatePullRequestWatch( const checks = detail.checks.filter((check) => check.placeholder !== true); const failed = checks.filter(isFailedCheck); const awaiting = checks.filter((check) => check.status === "action-required"); + const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name)); if (checks.length > 0) { - const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name)); if (!checksBaseline && newlyFailed.length > 0) { changes.push({ kind: "checks-failed", failed: newlyFailed }); } @@ -140,6 +146,25 @@ export function evaluatePullRequestWatch( } else if (settled && owed.length > 0) { changes.push({ kind: "checks-passed", count: checks.length, required: false }); owedChecks = []; + } else if (gate.some(isFailedCheck)) { + // Gate checks still running when one failed are owed one wake when they finish, since + // the gate can no longer pass and so never sends the checks-passed wake. + const running = checks + .filter((check) => check.status === "pending" || check.status === "action-required") + .map((check) => check.name); + const stillOwed = owedBefore.filter((name) => running.includes(name)); + const gateNewlyFailed = newlyFailed.some((check) => gateNames.includes(check.name)); + owedChecks = gateNewlyFailed + ? [...new Set([...stillOwed, ...running.filter((name) => gateNames.includes(name))])] + : stillOwed; + const owedFailed = newlyFailed.some((check) => owedBefore.includes(check.name)); + if (!owedFailed && owedBefore.length > 0 && owedChecks.length === 0) { + changes.push({ + kind: "checks-finished", + failing: gate.filter(isFailedCheck), + required: owedBefore.every((name) => required.some((check) => check.name === name)), + }); + } } passed = passedNow; passedChecks = passedNow ? gateNames : []; @@ -272,6 +297,14 @@ function changeLines( return ` - ${remark.author?.login ?? "someone"}${where}: ${said}${remark.url ? ` ${remark.url}` : ""}`; }), ]; + case "checks-finished": + return [ + `- The ${change.required ? "required " : ""}checks still running when one failed have finished${context.commit}. Still failing:`, + ...listed( + change.failing, + (check) => ` - ${check.name}${check.url ? ` ${check.url}` : ""}`, + ), + ]; case "no-checks": return [ `- No checks have reported${context.commit} after ${change.minutes} minutes. None may be configured to run on this change.`, @@ -284,6 +317,7 @@ function changeLines( const SUMMARY: Record = { "checks-failed": "checks failed", "checks-passed": "checks passed", + "checks-finished": "checks finished", "checks-awaiting-approval": "checks await approval", remarks: "new comments", "no-checks": "no checks reported", @@ -348,7 +382,10 @@ export function pullRequestWatchMessage(input: { : WATCHING_CLOSING, ].join("\n"); const failed = changes.some( - (change) => change.kind === "checks-failed" || change.kind === "conflicting", + (change) => + change.kind === "checks-failed" || + change.kind === "checks-finished" || + change.kind === "conflicting", ); const summary = changes.map((change) => SUMMARY[change.kind]); if (exhausted) summary.push("stopped watching");