From 86d935875812c617fd21ecc1abd0501e293049fd Mon Sep 17 00:00:00 2001 From: Lourince Daging Date: Mon, 20 Jul 2026 14:36:57 +0200 Subject: [PATCH] test(ci): add unit-test seam for check-stuck-required-checks.mjs The stuck/threshold detection (findStuckChecksForPr, minutesSince) and the watchdog's dry-run + marker-based idempotency only ran inside the un-guarded live-GitHub driver, so none of it was testable in isolation. Refactor githubApi into an injected dependency (a makeGithubApi factory for the live entrypoint), export minutesSince/findStuckChecksForPr/hasExistingWatchdogComment plus a runStuckCheckWatchdog driver, and gate the env-reading/live-call entrypoint behind import.meta.url === argv[1]. Add scripts/check-stuck-required-checks.d.mts (declaration pairing the imported .mjs) and test/unit/check-stuck-required-checks-script.test.ts covering: a check past threshold is flagged, one under threshold is not, a not-completed check with no started_at is excluded, non-required/completed checks are ignored, the marker makes the watchdog idempotent, and --dry-run never POSTs. Runtime behavior of the script when invoked directly is unchanged. Closes #7455 --- scripts/check-stuck-required-checks.d.mts | 52 +++++++ scripts/check-stuck-required-checks.mjs | 131 ++++++++++-------- ...check-stuck-required-checks-script.test.ts | 127 +++++++++++++++++ 3 files changed, 256 insertions(+), 54 deletions(-) create mode 100644 scripts/check-stuck-required-checks.d.mts create mode 100644 test/unit/check-stuck-required-checks-script.test.ts diff --git a/scripts/check-stuck-required-checks.d.mts b/scripts/check-stuck-required-checks.d.mts new file mode 100644 index 0000000000..744acee61d --- /dev/null +++ b/scripts/check-stuck-required-checks.d.mts @@ -0,0 +1,52 @@ +export type GithubApi = ( + path: string, + options?: { method?: string; body?: string; headers?: Record }, +) => Promise; + +export type PullRequestRef = { + number: number; + draft?: boolean; + head: { sha: string }; +}; + +export type StuckCheck = { + name: string; + status: string; + startedAt: string; + elapsedMinutes: number; + htmlUrl?: string; +}; + +export type CommentScope = { + githubApi: GithubApi; + owner: string; + repoName: string; +}; + +export type FindStuckOptions = CommentScope & { thresholdMinutes: number }; + +export declare const MARKER: string; +export declare const REQUIRED_CONTEXTS: Set; + +export declare function minutesSince(isoString: string): number; + +export declare function findStuckChecksForPr( + pr: PullRequestRef, + requiredContexts: Set, + options: FindStuckOptions, +): Promise; + +export declare function hasExistingWatchdogComment( + prNumber: number, + options: CommentScope, +): Promise; + +export declare function runStuckCheckWatchdog(options: { + githubApi: GithubApi; + owner: string; + repoName: string; + thresholdMinutes: number; + dryRun?: boolean; + requiredContexts?: Set; + log?: (message: string) => void; +}): Promise; diff --git a/scripts/check-stuck-required-checks.mjs b/scripts/check-stuck-required-checks.mjs index df308cf205..4bef2cb84e 100644 --- a/scripts/check-stuck-required-checks.mjs +++ b/scripts/check-stuck-required-checks.mjs @@ -12,31 +12,9 @@ // situation visible fast instead of requiring another manual multi-PR investigation like the one that // found the original incident. -const STUCK_THRESHOLD_MINUTES = Number(process.argv.find((a) => a.startsWith("--threshold-minutes="))?.split("=")[1] ?? 20); -const DRY_RUN = process.argv.includes("--dry-run"); -const MARKER = ""; - -const repo = process.env.GITHUB_REPOSITORY; -if (!repo) throw new Error("GITHUB_REPOSITORY is required"); -const [owner, repoName] = repo.split("/"); -const token = process.env.GITHUB_TOKEN; -if (!token) throw new Error("GITHUB_TOKEN is required"); - -async function githubApi(path, options = {}) { - const response = await fetch(`https://api.github.com${path}`, { - ...options, - headers: { - Authorization: `Bearer ${token}`, - Accept: "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - ...options.headers, - }, - }); - if (!response.ok) { - throw new Error(`GitHub API error ${response.status} on ${path}: ${await response.text()}`); - } - return response.status === 204 ? null : response.json(); -} +import { pathToFileURL } from "node:url"; + +export const MARKER = ""; // Hardcoded, not read live from branch protection: GET /branches/main/protection/required_status_checks // needs "Administration" repository read permission, which the default GITHUB_TOKEN does not get even @@ -45,37 +23,53 @@ async function githubApi(path, options = {}) { // protection is considered too privileged). Update this list by hand if the required checks on `main` // ever change (`gh api repos/{owner}/{repo}/branches/main/protection/required_status_checks` locally, // with a real user token, to check the current list). -const REQUIRED_CONTEXTS = new Set(["validate", "Superagent Security Scan"]); - -async function getOpenPRs() { - const prs = await githubApi(`/repos/${owner}/${repoName}/pulls?state=open&per_page=100`); - return prs.filter((pr) => !pr.draft); +export const REQUIRED_CONTEXTS = new Set(["validate", "Superagent Security Scan"]); + +/** Build the authenticated GitHub API caller used by the live entrypoint. Kept as a factory so the + * pure/testable logic below can take `githubApi` as an injected dependency instead of closing over a + * module-level token. */ +function makeGithubApi(token) { + return async function githubApi(path, options = {}) { + const response = await fetch(`https://api.github.com${path}`, { + ...options, + headers: { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + ...options.headers, + }, + }); + if (!response.ok) { + throw new Error(`GitHub API error ${response.status} on ${path}: ${await response.text()}`); + } + return response.status === 204 ? null : response.json(); + }; } -function minutesSince(isoString) { +export function minutesSince(isoString) { return (Date.now() - new Date(isoString).getTime()) / 60000; } -async function findStuckChecksForPr(pr, requiredContexts) { +export async function findStuckChecksForPr(pr, requiredContexts, { githubApi, owner, repoName, thresholdMinutes }) { const checkRuns = await githubApi(`/repos/${owner}/${repoName}/commits/${pr.head.sha}/check-runs?per_page=100`); const stuck = []; for (const run of checkRuns.check_runs ?? []) { if (!requiredContexts.has(run.name)) continue; if (run.status === "completed") continue; const elapsedMinutes = run.started_at ? minutesSince(run.started_at) : null; - if (elapsedMinutes !== null && elapsedMinutes >= STUCK_THRESHOLD_MINUTES) { + if (elapsedMinutes !== null && elapsedMinutes >= thresholdMinutes) { stuck.push({ name: run.name, status: run.status, startedAt: run.started_at, elapsedMinutes: Math.round(elapsedMinutes), htmlUrl: run.html_url }); } } return stuck; } -async function hasExistingWatchdogComment(prNumber) { +export async function hasExistingWatchdogComment(prNumber, { githubApi, owner, repoName }) { const comments = await githubApi(`/repos/${owner}/${repoName}/issues/${prNumber}/comments?per_page=100`); return comments.some((comment) => comment.body?.includes(MARKER)); } -async function postComment(prNumber, stuckChecks) { +async function postComment(prNumber, stuckChecks, { githubApi, owner, repoName }) { const lines = [ MARKER, "## ⚠️ A required check looks stuck", @@ -94,28 +88,57 @@ async function postComment(prNumber, stuckChecks) { }); } -const prs = await getOpenPRs(); -let flaggedCount = 0; - -for (const pr of prs) { - const stuckChecks = await findStuckChecksForPr(pr, REQUIRED_CONTEXTS); - if (stuckChecks.length === 0) continue; - - console.log(`PR #${pr.number}: ${stuckChecks.length} required check(s) stuck past ${STUCK_THRESHOLD_MINUTES}min: ${stuckChecks.map((c) => `${c.name} (~${c.elapsedMinutes}min)`).join(", ")}`); +/** Scan every open, non-draft PR for stuck required checks, printing a line per stuck PR and posting a + * one-time (marker-guarded) comment unless `dryRun`. `githubApi` is injected so this whole flow is + * testable without live GitHub calls. Returns the number of new comments posted. */ +export async function runStuckCheckWatchdog({ + githubApi, + owner, + repoName, + thresholdMinutes, + dryRun = false, + requiredContexts = REQUIRED_CONTEXTS, + log = console.log, +}) { + const prs = (await githubApi(`/repos/${owner}/${repoName}/pulls?state=open&per_page=100`)).filter((pr) => !pr.draft); + let flaggedCount = 0; + + for (const pr of prs) { + const stuckChecks = await findStuckChecksForPr(pr, requiredContexts, { githubApi, owner, repoName, thresholdMinutes }); + if (stuckChecks.length === 0) continue; + + log(`PR #${pr.number}: ${stuckChecks.length} required check(s) stuck past ${thresholdMinutes}min: ${stuckChecks.map((c) => `${c.name} (~${c.elapsedMinutes}min)`).join(", ")}`); + + if (await hasExistingWatchdogComment(pr.number, { githubApi, owner, repoName })) { + log(` Already flagged this PR -- skipping (idempotent).`); + continue; + } - if (await hasExistingWatchdogComment(pr.number)) { - console.log(` Already flagged this PR -- skipping (idempotent).`); - continue; - } + if (dryRun) { + log(` --dry-run: would post a comment on PR #${pr.number}, skipping the actual POST.`); + continue; + } - if (DRY_RUN) { - console.log(` --dry-run: would post a comment on PR #${pr.number}, skipping the actual POST.`); - continue; + await postComment(pr.number, stuckChecks, { githubApi, owner, repoName }); + flaggedCount += 1; + log(` Posted a new comment on PR #${pr.number}.`); } - await postComment(pr.number, stuckChecks); - flaggedCount += 1; - console.log(` Posted a new comment on PR #${pr.number}.`); + log(`Checked ${prs.length} open PR(s); flagged ${flaggedCount} new stuck-check comment(s).`); + return flaggedCount; } -console.log(`Checked ${prs.length} open PR(s); flagged ${flaggedCount} new stuck-check comment(s).`); +// Entrypoint guard (#7455): the pure detection logic above is importable for tests with an injected +// githubApi; only direct invocation reads env, builds the live API caller, and makes real calls. +if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) { + const thresholdMinutes = Number(process.argv.find((a) => a.startsWith("--threshold-minutes="))?.split("=")[1] ?? 20); + const dryRun = process.argv.includes("--dry-run"); + + const repo = process.env.GITHUB_REPOSITORY; + if (!repo) throw new Error("GITHUB_REPOSITORY is required"); + const [owner, repoName] = repo.split("/"); + const token = process.env.GITHUB_TOKEN; + if (!token) throw new Error("GITHUB_TOKEN is required"); + + await runStuckCheckWatchdog({ githubApi: makeGithubApi(token), owner, repoName, thresholdMinutes, dryRun }); +} diff --git a/test/unit/check-stuck-required-checks-script.test.ts b/test/unit/check-stuck-required-checks-script.test.ts new file mode 100644 index 0000000000..ffe38f53e8 --- /dev/null +++ b/test/unit/check-stuck-required-checks-script.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, it } from "vitest"; + +import { + MARKER, + REQUIRED_CONTEXTS, + findStuckChecksForPr, + minutesSince, + runStuckCheckWatchdog, +} from "../../scripts/check-stuck-required-checks.mjs"; + +// #7455: findStuckChecksForPr's stuck/threshold decision and the watchdog's dry-run + marker-idempotency +// only ran inside the un-guarded live-GitHub driver. With githubApi injected and the driver behind an +// entrypoint guard, both are now testable with mock responses and zero network. + +type CheckRun = { name: string; status: string; started_at?: string; html_url?: string }; +type ApiOptions = { method?: string; body?: string; headers?: Record }; + +function minutesAgoIso(minutes: number): string { + return new Date(Date.now() - minutes * 60_000).toISOString(); +} + +const scope = { owner: "acme", repoName: "widget", thresholdMinutes: 20 }; + +function checkRunApi(checkRuns: CheckRun[]) { + return async (path: string): Promise => { + if (path.includes("/check-runs")) return { check_runs: checkRuns }; + throw new Error(`unexpected path: ${path}`); + }; +} + +describe("minutesSince (#7455)", () => { + it("returns elapsed minutes since an ISO timestamp", () => { + expect(minutesSince(minutesAgoIso(30))).toBeGreaterThanOrEqual(29.9); + expect(minutesSince(minutesAgoIso(30))).toBeLessThanOrEqual(30.1); + }); +}); + +describe("findStuckChecksForPr (#7455)", () => { + const pr = { number: 1, head: { sha: "deadbeef" } }; + + it("flags a required check pending past the threshold", async () => { + const stuck = await findStuckChecksForPr(pr, REQUIRED_CONTEXTS, { + githubApi: checkRunApi([{ name: "validate", status: "in_progress", started_at: minutesAgoIso(30) }]), + ...scope, + }); + expect(stuck).toHaveLength(1); + expect(stuck[0]!.name).toBe("validate"); + expect(stuck[0]!.elapsedMinutes).toBeGreaterThanOrEqual(20); + }); + + it("does not flag a required check still under the threshold", async () => { + const stuck = await findStuckChecksForPr(pr, REQUIRED_CONTEXTS, { + githubApi: checkRunApi([{ name: "validate", status: "in_progress", started_at: minutesAgoIso(5) }]), + ...scope, + }); + expect(stuck).toHaveLength(0); + }); + + it("excludes a not-completed check that has no started_at (elapsedMinutes === null), e.g. still queued", async () => { + const stuck = await findStuckChecksForPr(pr, REQUIRED_CONTEXTS, { + githubApi: checkRunApi([{ name: "validate", status: "queued" }]), + ...scope, + }); + expect(stuck).toHaveLength(0); + }); + + it("ignores non-required and already-completed checks even when old", async () => { + const stuck = await findStuckChecksForPr(pr, REQUIRED_CONTEXTS, { + githubApi: checkRunApi([ + { name: "some-other-check", status: "in_progress", started_at: minutesAgoIso(60) }, + { name: "validate", status: "completed", started_at: minutesAgoIso(60) }, + ]), + ...scope, + }); + expect(stuck).toHaveLength(0); + }); +}); + +describe("runStuckCheckWatchdog (#7455)", () => { + function watchdogApi(opts: { prs: unknown[]; checkRuns: CheckRun[]; comments: Array<{ body?: string }> }) { + const calls: Array<{ path: string; method: string }> = []; + const githubApi = async (path: string, options: ApiOptions = {}): Promise => { + const method = options.method ?? "GET"; + calls.push({ path, method }); + if (path.includes("/pulls?")) return opts.prs; + if (path.includes("/check-runs")) return { check_runs: opts.checkRuns }; + if (path.includes("/comments")) return method === "POST" ? {} : opts.comments; + throw new Error(`unexpected path: ${path}`); + }; + return { githubApi, calls }; + } + + const stuckRun: CheckRun = { name: "validate", status: "in_progress", started_at: minutesAgoIso(30) }; + + it("posts a comment for a stuck PR that has not been flagged yet", async () => { + const { githubApi, calls } = watchdogApi({ + prs: [{ number: 7, draft: false, head: { sha: "s" } }], + checkRuns: [stuckRun], + comments: [], + }); + const flagged = await runStuckCheckWatchdog({ githubApi, ...scope, log: () => {} }); + expect(flagged).toBe(1); + expect(calls.filter((c) => c.method === "POST")).toHaveLength(1); + }); + + it("is idempotent: skips a PR that already has the watchdog marker comment (no POST)", async () => { + const { githubApi, calls } = watchdogApi({ + prs: [{ number: 7, draft: false, head: { sha: "s" } }], + checkRuns: [stuckRun], + comments: [{ body: `${MARKER}\n## previously flagged` }], + }); + const flagged = await runStuckCheckWatchdog({ githubApi, ...scope, log: () => {} }); + expect(flagged).toBe(0); + expect(calls.some((c) => c.method === "POST")).toBe(false); + }); + + it("--dry-run never calls the comment-post endpoint", async () => { + const { githubApi, calls } = watchdogApi({ + prs: [{ number: 7, draft: false, head: { sha: "s" } }], + checkRuns: [stuckRun], + comments: [], + }); + const flagged = await runStuckCheckWatchdog({ githubApi, ...scope, dryRun: true, log: () => {} }); + expect(flagged).toBe(0); + expect(calls.some((c) => c.method === "POST")).toBe(false); + }); +});