From 5d10094edb409b42c863015e46487b736a6ba9dd Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Fri, 23 Oct 2026 14:34:19 +0200 Subject: [PATCH] Treat runtime boundary increases a PR's baseline accepts as notices In pull request mode the check compared the merge result only with the base branch, so a PR that had already run --update-baseline --accept-increase still got one warning per new call and the "Maintainer action before merging" block (seen on PR 15180, which raised tests from 832 to 838). Reviewers could not tell whether the step was still pending. The check now also reads the working tree's baseline. A count the PR raised there and does not exceed is reported as a notice plus an "Accepted in this PR" line; that is the state the push check on main will see after merge. Slack already present in the base baseline does not count as acceptance, so new calls still get a warning unless the PR raises the number itself. A baseline raised before updating from main, and now too low, keeps the warnings. Errors for APIs that src/deno_ral replaces are unchanged. --- .github/workflows/runtime-boundary.yml | 3 +- .../unit/tools/check-runtime-boundary.test.ts | 73 ++++++++++++++++++- tools/check-runtime-boundary.ts | 44 ++++++++++- 3 files changed, 115 insertions(+), 5 deletions(-) diff --git a/.github/workflows/runtime-boundary.yml b/.github/workflows/runtime-boundary.yml index 93747d5001..8f841e2b72 100644 --- a/.github/workflows/runtime-boundary.yml +++ b/.github/workflows/runtime-boundary.yml @@ -3,7 +3,8 @@ # (src, src-js, resources, package/src, tests). Two modes: # - pull_request: compares the merge commit with its first parent (the base # branch tip) and fails only for a Deno.* API that src/deno_ral replaces; -# other increases are warnings plus a maintainer block in the job summary. +# other increases are warnings plus a maintainer block in the job summary, +# or notices once the PR's own baseline accepts them. # - push (main) and workflow_dispatch: fails when a count rises above # tools/runtime-boundary-baseline.json. # See dev-docs/runtime-migration/02-runtime-boundary-check.md. diff --git a/tests/unit/tools/check-runtime-boundary.test.ts b/tests/unit/tools/check-runtime-boundary.test.ts index 9e69ad2593..1cb1bba7e4 100644 --- a/tests/unit/tools/check-runtime-boundary.test.ts +++ b/tests/unit/tools/check-runtime-boundary.test.ts @@ -10,7 +10,8 @@ import { unitTest } from "../../test.ts"; import { assert, assertEquals, assertRejects, assertThrows } from "../../asserts.ts"; -import { join } from "../../../src/deno_ral/path.ts"; +import { join, resolve } from "../../../src/deno_ral/path.ts"; +import { quartoDevBinCmd } from "../../quarto-cmd.ts"; import { ensureDirSync, safeRemoveSync } from "../../../src/deno_ral/fs.ts"; import { execProcess } from "../../../src/core/process.ts"; import { createTempContext } from "../../../src/core/temp.ts"; @@ -661,6 +662,76 @@ unitTest("check-runtime-boundary - baselineAt reads the baseline committed at th } }); +// deno-lint-ignore require-await +unitTest("check-runtime-boundary - renderDelta accepts only counts the PR itself raised", async () => { + const report = { + deltas: { "tests": { denoCalls: 1, stdImportFiles: 1 } }, + findings: [ + { scope: "tests", file: "tests/unit/a.test.ts", kind: "deno" as const, name: "cwd", delta: 1 }, + { scope: "tests", file: "tests/unit/c.test.ts", kind: "std" as const, name: "testing/asserts", delta: 1, suggestion: "tests/asserts.ts" }, + ], + }; + const current = { "tests": { denoCalls: 830, stdImportFiles: 178 } }; + // main sits below its baseline for calls; the PR raises only std-import files. + const base = { "tests": { denoCalls: 832, stdImportFiles: 177 } }; + const rendered = renderDelta(report, current, base, { "tests": { denoCalls: 832, stdImportFiles: 178 } }); + assertEquals(rendered.accepted.map((a) => a.message), [ + "tests: tests/unit/c.test.ts now imports the standard library directly (testing/asserts); accepted by this PR's tools/runtime-boundary-baseline.json.", + ]); + assertEquals(rendered.warnings.map((w) => w.file), ["tests/unit/a.test.ts"]); + assert(rendered.summary.includes("- tests std-import files: baseline 177 → 178")); + assert(!rendered.summary.includes(`- tests ${DENO}* calls`)); + assert(rendered.summary.includes("### Maintainer action before merging")); +}); + +/** Runs the script in PR mode on `root`, as CI does, and returns its stdout. */ +async function runAgainst(root: string, rev: string): Promise { + const result = await execProcess({ + cmd: resolve(quartoDevBinCmd()), + args: [ + "run", "--dev", resolve("..", "tools", "check-runtime-boundary.ts"), + "--root", root, "--against", rev, + ], + env: { GITHUB_ACTIONS: "true", GITHUB_STEP_SUMMARY: "" }, + stdout: "piped", + stderr: "piped", + }); + assert(result.success, `check-runtime-boundary failed: ${result.stdout}\n${result.stderr}`); + return result.stdout ?? ""; +} + +unitTest("check-runtime-boundary - PR mode: an increase the PR's baseline already accepts is a notice, not a warning", async () => { + const temp = createTempContext(); + try { + const root = temp.createDir(); + makeRepo(root); + writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(kExpected)); + await git(root, "init"); + await git(root, "add", "."); + await git(root, "-c", "user.name=t", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", "commit", "-m", "base"); + + writeFixture(root, "tests/j.test.ts", `${DENO}cwd();\n`); + const raised = { ...kExpected, "tests": { denoCalls: 2, stdImportFiles: 2 } }; + writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(raised)); + + const out = await runAgainst(root, "HEAD"); + assert(!out.includes("::warning"), out); + assert(out.includes("::notice file=tests/j.test.ts,line=1::tests: +1 " + DENO + "cwd in tests/j.test.ts; accepted by this PR's tools/runtime-boundary-baseline.json"), out); + assert(out.includes("### Accepted in this PR"), out); + assert(out.includes(`- tests ${DENO}* calls: baseline 1 → 2`), out); + assert(!out.includes("### Maintainer action before merging"), out); + + // A baseline raised for a smaller increase than the PR makes leaves the warnings. + writeFixture(root, "tests/k.test.ts", `${DENO}cwd();\n`); + const stale = await runAgainst(root, "HEAD"); + assert(stale.includes("::warning file=tests/j.test.ts"), stale); + assert(stale.includes("### Maintainer action before merging"), stale); + assert(!stale.includes("### Accepted in this PR"), stale); + } finally { + temp.cleanup(); + } +}); + // deno-lint-ignore require-await unitTest("check-runtime-boundary - a covered API in browser-shared code is a warning, not an error", async () => { const report = diffChanges([ diff --git a/tools/check-runtime-boundary.ts b/tools/check-runtime-boundary.ts index 0ed181b2f4..e3d116c0c4 100644 --- a/tools/check-runtime-boundary.ts +++ b/tools/check-runtime-boundary.ts @@ -8,7 +8,8 @@ * standard library directly instead of through src/deno_ral/. Two modes: * without flags it fails when any count exceeds the committed baseline (main); * with --against it compares the working tree with and fails only - * for a Deno.* API that src/deno_ral already replaces (pull requests). + * for a Deno.* API that src/deno_ral already replaces (pull requests); other + * increases are warnings until the working tree's baseline accepts them. * * Usage (from the repository root): * quarto run --dev tools/check-runtime-boundary.ts @@ -501,6 +502,7 @@ export interface Annotation { export interface Rendered { errors: Annotation[]; warnings: Annotation[]; + accepted: Annotation[]; // increases the PR's own baseline already covers notices: string[]; summary: string; // markdown for GITHUB_STEP_SUMMARY failed: boolean; // errors.length > 0 @@ -514,18 +516,30 @@ const kAcceptSentence = kAcceptClause[0].toUpperCase() + kAcceptClause.slice(1); const signed = (n: number) => n > 0 ? `+${n}` : `${n}`; -/** `current` is measure() of the working tree (the merge result on CI), so `current - deltas` is the base. */ +/** `current` is measure() of the working tree (the merge result on CI), so `current - deltas` is the base. + * `baseline` is the base revision's baseline, `merged` the working tree's: a count the PR raised + * in `merged` and does not exceed is accepted, the state the push check on main will see. */ export function renderDelta( report: DeltaReport, current: Baseline, baseline: Baseline, + merged: Baseline = baseline, ): Rendered { const errors: Annotation[] = []; const warnings: Annotation[] = []; + const accepted: Annotation[] = []; const notices: string[] = []; const changes: string[] = []; const warnedScopes = new Set(); + const isAccepted = (scope: string, field: keyof BoundaryCounts) => { + const raised = merged[scope]?.[field]; + return raised !== undefined && + raised > (baseline[scope]?.[field] ?? 0) && + (current[scope]?.[field] ?? 0) <= raised; + }; + const acceptedSuffix = `; accepted by this PR's ${kBaselineRelPath}.`; + for (const finding of report.findings) { const annotation = (message: string): Annotation => { const result: Annotation = { message, file: finding.file }; @@ -542,6 +556,14 @@ export function renderDelta( ); continue; } + if (isAccepted(finding.scope, finding.kind === "deno" ? "denoCalls" : "stdImportFiles")) { + accepted.push(annotation( + finding.kind === "deno" + ? `${finding.scope}: +${finding.delta} Deno.${finding.name} in ${finding.file}${acceptedSuffix}` + : `${finding.scope}: ${finding.file} now imports the standard library directly (${finding.name})${acceptedSuffix}`, + )); + continue; + } warnedScopes.add(finding.scope); if (finding.kind === "deno") { warnings.push(annotation( @@ -591,6 +613,18 @@ export function renderDelta( if (errors.length > 0) { summary.push("### What to change", "", ...changes, ""); } + const acceptedRows = Object.entries(report.deltas).flatMap(([scope, d]) => + kFields + .filter((field) => d[field] > 0 && isAccepted(scope, field)) + .map((field) => + `- ${scope} ${kFieldLabels[field]}: baseline ${ + baseline[scope]?.[field] ?? 0 + } → ${merged[scope][field]}` + ) + ); + if (acceptedRows.length > 0) { + summary.push("### Accepted in this PR", "", ...acceptedRows, ""); + } if (warnings.length > 0) { const expected = Object.entries(current) .filter(([scope]) => warnedScopes.has(scope)) @@ -610,6 +644,7 @@ export function renderDelta( return { errors, warnings, + accepted, notices, summary: summary.join("\n"), failed: errors.length > 0, @@ -838,10 +873,13 @@ if (import.meta.main) { } catch (e) { usage(`cannot use ${kBaselineRelPath} at ${against}: ${(e as Error).message}`); } - const rendered = renderDelta(diffChanges(changes), current, baseBaseline); + const rendered = renderDelta(diffChanges(changes), current, baseBaseline, baseline); for (const message of rendered.notices) { notice(message); } + for (const { message, file, line } of rendered.accepted) { + notice(message, { file, line }); + } for (const { message, file, line } of rendered.warnings) { warning(message, { file, line }); }