Skip to content

Commit 3a689f2

Browse files
authored
Report runtime boundary increases a PR has already accepted as notices (#15183)
The PR-mode runtime boundary check compared the merge result only with the base branch's baseline, so a PR that had already run --update-baseline --accept-increase still got a warning for each new call and the maintainer block. See #15180, where six new Deno calls were accepted in the PR's baseline and still reported as pending. - Read the working-tree baseline in PR mode. An increase the PR raised there, and that the merge result does not exceed, is a notice listed under "Accepted in this PR". - Keep the warnings when the base baseline has room the PR did not take, or when the merge result is now above the accepted count. - Leave errors for Deno APIs that src/deno_ral replaces unchanged. Related to #15153
1 parent 993caf3 commit 3a689f2

3 files changed

Lines changed: 115 additions & 5 deletions

File tree

‎.github/workflows/runtime-boundary.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
# (src, src-js, resources, package/src, tests). Two modes:
44
# - pull_request: compares the merge commit with its first parent (the base
55
# branch tip) and fails only for a Deno.* API that src/deno_ral replaces;
6-
# other increases are warnings plus a maintainer block in the job summary.
6+
# other increases are warnings plus a maintainer block in the job summary,
7+
# or notices once the PR's own baseline accepts them.
78
# - push (main) and workflow_dispatch: fails when a count rises above
89
# tools/runtime-boundary-baseline.json.
910
# See dev-docs/runtime-migration/02-runtime-boundary-check.md.

‎tests/unit/tools/check-runtime-boundary.test.ts‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010

1111
import { unitTest } from "../../test.ts";
1212
import { assert, assertEquals, assertRejects, assertThrows } from "../../asserts.ts";
13-
import { join } from "../../../src/deno_ral/path.ts";
13+
import { join, resolve } from "../../../src/deno_ral/path.ts";
14+
import { quartoDevBinCmd } from "../../quarto-cmd.ts";
1415
import { ensureDirSync, safeRemoveSync } from "../../../src/deno_ral/fs.ts";
1516
import { execProcess } from "../../../src/core/process.ts";
1617
import { createTempContext } from "../../../src/core/temp.ts";
@@ -661,6 +662,76 @@ unitTest("check-runtime-boundary - baselineAt reads the baseline committed at th
661662
}
662663
});
663664

665+
// deno-lint-ignore require-await
666+
unitTest("check-runtime-boundary - renderDelta accepts only counts the PR itself raised", async () => {
667+
const report = {
668+
deltas: { "tests": { denoCalls: 1, stdImportFiles: 1 } },
669+
findings: [
670+
{ scope: "tests", file: "tests/unit/a.test.ts", kind: "deno" as const, name: "cwd", delta: 1 },
671+
{ scope: "tests", file: "tests/unit/c.test.ts", kind: "std" as const, name: "testing/asserts", delta: 1, suggestion: "tests/asserts.ts" },
672+
],
673+
};
674+
const current = { "tests": { denoCalls: 830, stdImportFiles: 178 } };
675+
// main sits below its baseline for calls; the PR raises only std-import files.
676+
const base = { "tests": { denoCalls: 832, stdImportFiles: 177 } };
677+
const rendered = renderDelta(report, current, base, { "tests": { denoCalls: 832, stdImportFiles: 178 } });
678+
assertEquals(rendered.accepted.map((a) => a.message), [
679+
"tests: tests/unit/c.test.ts now imports the standard library directly (testing/asserts); accepted by this PR's tools/runtime-boundary-baseline.json.",
680+
]);
681+
assertEquals(rendered.warnings.map((w) => w.file), ["tests/unit/a.test.ts"]);
682+
assert(rendered.summary.includes("- tests std-import files: baseline 177 → 178"));
683+
assert(!rendered.summary.includes(`- tests ${DENO}* calls`));
684+
assert(rendered.summary.includes("### Maintainer action before merging"));
685+
});
686+
687+
/** Runs the script in PR mode on `root`, as CI does, and returns its stdout. */
688+
async function runAgainst(root: string, rev: string): Promise<string> {
689+
const result = await execProcess({
690+
cmd: resolve(quartoDevBinCmd()),
691+
args: [
692+
"run", "--dev", resolve("..", "tools", "check-runtime-boundary.ts"),
693+
"--root", root, "--against", rev,
694+
],
695+
env: { GITHUB_ACTIONS: "true", GITHUB_STEP_SUMMARY: "" },
696+
stdout: "piped",
697+
stderr: "piped",
698+
});
699+
assert(result.success, `check-runtime-boundary failed: ${result.stdout}\n${result.stderr}`);
700+
return result.stdout ?? "";
701+
}
702+
703+
unitTest("check-runtime-boundary - PR mode: an increase the PR's baseline already accepts is a notice, not a warning", async () => {
704+
const temp = createTempContext();
705+
try {
706+
const root = temp.createDir();
707+
makeRepo(root);
708+
writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(kExpected));
709+
await git(root, "init");
710+
await git(root, "add", ".");
711+
await git(root, "-c", "user.name=t", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", "commit", "-m", "base");
712+
713+
writeFixture(root, "tests/j.test.ts", `${DENO}cwd();\n`);
714+
const raised = { ...kExpected, "tests": { denoCalls: 2, stdImportFiles: 2 } };
715+
writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(raised));
716+
717+
const out = await runAgainst(root, "HEAD");
718+
assert(!out.includes("::warning"), out);
719+
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);
720+
assert(out.includes("### Accepted in this PR"), out);
721+
assert(out.includes(`- tests ${DENO}* calls: baseline 1 → 2`), out);
722+
assert(!out.includes("### Maintainer action before merging"), out);
723+
724+
// A baseline raised for a smaller increase than the PR makes leaves the warnings.
725+
writeFixture(root, "tests/k.test.ts", `${DENO}cwd();\n`);
726+
const stale = await runAgainst(root, "HEAD");
727+
assert(stale.includes("::warning file=tests/j.test.ts"), stale);
728+
assert(stale.includes("### Maintainer action before merging"), stale);
729+
assert(!stale.includes("### Accepted in this PR"), stale);
730+
} finally {
731+
temp.cleanup();
732+
}
733+
});
734+
664735
// deno-lint-ignore require-await
665736
unitTest("check-runtime-boundary - a covered API in browser-shared code is a warning, not an error", async () => {
666737
const report = diffChanges([

‎tools/check-runtime-boundary.ts‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88
* standard library directly instead of through src/deno_ral/. Two modes:
99
* without flags it fails when any count exceeds the committed baseline (main);
1010
* with --against <rev> it compares the working tree with <rev> and fails only
11-
* for a Deno.* API that src/deno_ral already replaces (pull requests).
11+
* for a Deno.* API that src/deno_ral already replaces (pull requests); other
12+
* increases are warnings until the working tree's baseline accepts them.
1213
*
1314
* Usage (from the repository root):
1415
* quarto run --dev tools/check-runtime-boundary.ts
@@ -501,6 +502,7 @@ export interface Annotation {
501502
export interface Rendered {
502503
errors: Annotation[];
503504
warnings: Annotation[];
505+
accepted: Annotation[]; // increases the PR's own baseline already covers
504506
notices: string[];
505507
summary: string; // markdown for GITHUB_STEP_SUMMARY
506508
failed: boolean; // errors.length > 0
@@ -514,18 +516,30 @@ const kAcceptSentence = kAcceptClause[0].toUpperCase() + kAcceptClause.slice(1);
514516

515517
const signed = (n: number) => n > 0 ? `+${n}` : `${n}`;
516518

517-
/** `current` is measure() of the working tree (the merge result on CI), so `current - deltas` is the base. */
519+
/** `current` is measure() of the working tree (the merge result on CI), so `current - deltas` is the base.
520+
* `baseline` is the base revision's baseline, `merged` the working tree's: a count the PR raised
521+
* in `merged` and does not exceed is accepted, the state the push check on main will see. */
518522
export function renderDelta(
519523
report: DeltaReport,
520524
current: Baseline,
521525
baseline: Baseline,
526+
merged: Baseline = baseline,
522527
): Rendered {
523528
const errors: Annotation[] = [];
524529
const warnings: Annotation[] = [];
530+
const accepted: Annotation[] = [];
525531
const notices: string[] = [];
526532
const changes: string[] = [];
527533
const warnedScopes = new Set<string>();
528534

535+
const isAccepted = (scope: string, field: keyof BoundaryCounts) => {
536+
const raised = merged[scope]?.[field];
537+
return raised !== undefined &&
538+
raised > (baseline[scope]?.[field] ?? 0) &&
539+
(current[scope]?.[field] ?? 0) <= raised;
540+
};
541+
const acceptedSuffix = `; accepted by this PR's ${kBaselineRelPath}.`;
542+
529543
for (const finding of report.findings) {
530544
const annotation = (message: string): Annotation => {
531545
const result: Annotation = { message, file: finding.file };
@@ -542,6 +556,14 @@ export function renderDelta(
542556
);
543557
continue;
544558
}
559+
if (isAccepted(finding.scope, finding.kind === "deno" ? "denoCalls" : "stdImportFiles")) {
560+
accepted.push(annotation(
561+
finding.kind === "deno"
562+
? `${finding.scope}: +${finding.delta} Deno.${finding.name} in ${finding.file}${acceptedSuffix}`
563+
: `${finding.scope}: ${finding.file} now imports the standard library directly (${finding.name})${acceptedSuffix}`,
564+
));
565+
continue;
566+
}
545567
warnedScopes.add(finding.scope);
546568
if (finding.kind === "deno") {
547569
warnings.push(annotation(
@@ -591,6 +613,18 @@ export function renderDelta(
591613
if (errors.length > 0) {
592614
summary.push("### What to change", "", ...changes, "");
593615
}
616+
const acceptedRows = Object.entries(report.deltas).flatMap(([scope, d]) =>
617+
kFields
618+
.filter((field) => d[field] > 0 && isAccepted(scope, field))
619+
.map((field) =>
620+
`- ${scope} ${kFieldLabels[field]}: baseline ${
621+
baseline[scope]?.[field] ?? 0
622+
} → ${merged[scope][field]}`
623+
)
624+
);
625+
if (acceptedRows.length > 0) {
626+
summary.push("### Accepted in this PR", "", ...acceptedRows, "");
627+
}
594628
if (warnings.length > 0) {
595629
const expected = Object.entries(current)
596630
.filter(([scope]) => warnedScopes.has(scope))
@@ -610,6 +644,7 @@ export function renderDelta(
610644
return {
611645
errors,
612646
warnings,
647+
accepted,
613648
notices,
614649
summary: summary.join("\n"),
615650
failed: errors.length > 0,
@@ -838,10 +873,13 @@ if (import.meta.main) {
838873
} catch (e) {
839874
usage(`cannot use ${kBaselineRelPath} at ${against}: ${(e as Error).message}`);
840875
}
841-
const rendered = renderDelta(diffChanges(changes), current, baseBaseline);
876+
const rendered = renderDelta(diffChanges(changes), current, baseBaseline, baseline);
842877
for (const message of rendered.notices) {
843878
notice(message);
844879
}
880+
for (const { message, file, line } of rendered.accepted) {
881+
notice(message, { file, line });
882+
}
845883
for (const { message, file, line } of rendered.warnings) {
846884
warning(message, { file, line });
847885
}

0 commit comments

Comments
 (0)