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 }); }