Skip to content

Commit 6097f9c

Browse files
authored
fix(bench): apply KNOWN_REGRESSIONS baseline fallback to release publish gate, not just dev (#2129)
* fix(bench): apply KNOWN_REGRESSIONS baseline fallback to release publish gate, not just dev The v3.16.0 release tag (2e0e94f) failed its pre-publish benchmark gate on Full build (+42%) and 1-file rebuild (+53-83%), blocking publish. Both metrics are already tracked as growth-driven baseline drift (#2081) and exempted via KNOWN_REGRESSIONS entries '3.15.0:Full build' / '3.15.0:1-file rebuild' added in #2107 - but that exemption only fired when assertNoRegressions' `latest.version` was the literal string 'dev' (the per-PR gate label). publish.yml's pre-publish-benchmark job labels its measurement with the real version being published (e.g. '3.16.0'), so the fallback never applied on the one run it mattered most for, and the gate failed on the exact regression it was supposed to be exempt from. Drop the `version === 'dev'` restriction so the baseline-anchored fallback applies to any comparison against a known-regressed baseline, whether the latest label is 'dev' or a real release version. Adds unit tests exercising both cases directly against assertNoRegressions. * fix(bench): widen the negative-case test delta so it holds under BENCH_CANARY too The 'does not exempt' test used a 3521->5000 (+42%) delta, which clears the default 25% Full build threshold but not the 50% BENCH_CANARY threshold - under canary mode the check would get filtered out as "not a regression" before ever reaching the KNOWN_REGRESSIONS fallback, making the test pass without exercising the logic it's meant to guard. * fix(bench): decouple KNOWN_REGRESSIONS baseline-fallback tests from live production data Greptile review: the three always-on unit tests exercising the baseline fallback hardcoded '3.15.0:Full build' against the module-level KNOWN_REGRESSIONS set. That entry is documented for pruning once v3.16.0's own benchmark data lands, which would break the first two tests (expecting .not.toThrow()) the moment that routine cleanup PR merges. Add an optional knownRegressions parameter to assertNoRegressions (defaulting to the module-level KNOWN_REGRESSIONS for production use) and have the three unit tests pass a local, test-only fixture instead, fully decoupling them from data that is expected to change over time.
1 parent 0f51e0c commit 6097f9c

1 file changed

Lines changed: 107 additions & 25 deletions

File tree

tests/benchmarks/regression-guard.test.ts

Lines changed: 107 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -217,21 +217,25 @@ const SKIP_VERSIONS = new Set(['3.8.0']);
217217
* Resolution keys use: "version:resolution <lang> precision" or "version:resolution <lang> recall".
218218
*
219219
* The `version` is the release where the regression was first observed.
220-
* When the per-PR gate runs `dev` against that release as baseline, the
221-
* exemption applies via the baseline-version fallback in assertNoRegressions
222-
* (and the resolution loop) — so a single `3.11.0:Foo` entry covers both
223-
* `3.11.0 vs 3.10.0` and every subsequent `dev vs 3.11.0` comparison until
224-
* the next release clears the regression and the entry is pruned.
220+
* Any comparison whose baseline is that release gets the exemption via the
221+
* baseline-version fallback in assertNoRegressions (and the resolution
222+
* loop) — so a single `3.11.0:Foo` entry covers `3.11.0 vs 3.10.0`, every
223+
* subsequent `dev vs 3.11.0` per-PR comparison, AND the eventual `3.12.0 vs
224+
* 3.11.0` publish gate itself (the release that's supposed to fold the
225+
* drift into a fresh baseline) — until that release's own benchmark data
226+
* lands and the entry is pruned.
225227
*
226-
* Entries fire only when `latest.version` matches the prefix (or, for `dev`
227-
* latest, when `previous.version` matches via the baseline fallback). Once
228-
* a version is no longer the latest in committed history and no longer the
229-
* baseline used for `dev` comparisons, its entries become dead weight and
230-
* should be removed (last pruned: 3.9.0/3.9.1/3.9.2/3.9.6/3.10.0/3.11.0/3.11.1/
231-
* 3.11.2; the 3.12.0 and 3.13.0 entries — dataflow/no-op/full-build timing noise
232-
* and the erlang 0% drop — were pruned at the 3.15.0 release once the 3.15.0
233-
* benchmark baseline landed in PR #1702, which folds those deltas into the
234-
* baseline so dev-vs-3.15.0 comparisons no longer flag them).
228+
* Entries fire when `latest.version` matches the prefix directly, or when
229+
* `previous.version` (the baseline) matches via the baseline fallback —
230+
* regardless of whether `latest` is `dev` (per-PR gate) or a real version
231+
* (the publish gate for the next release). Once a version is no longer the
232+
* latest in committed history and no longer used as a baseline for any
233+
* comparison, its entries become dead weight and should be removed (last
234+
* pruned: 3.9.0/3.9.1/3.9.2/3.9.6/3.10.0/3.11.0/3.11.1/3.11.2; the 3.12.0 and
235+
* 3.13.0 entries — dataflow/no-op/full-build timing noise and the erlang 0%
236+
* drop — were pruned at the 3.15.0 release once the 3.15.0 benchmark
237+
* baseline landed in PR #1702, which folds those deltas into the baseline
238+
* so dev-vs-3.15.0 comparisons no longer flag them).
235239
*
236240
* NOTE: WASM *timing* noise no longer needs per-version entries here — it is
237241
* handled structurally by WASM_TIMING_THRESHOLD (see above); native keeps the
@@ -501,21 +505,33 @@ function assertNoRegressions(
501505
version?: string,
502506
baselineVersion?: string,
503507
engine?: string,
508+
// Defaults to the module-level KNOWN_REGRESSIONS (production behavior).
509+
// Tests that exercise the fallback logic itself pass a local, stable
510+
// fixture instead — so they don't break when real entries above are
511+
// pruned by a future benchmark-recording cleanup PR (see the
512+
// 'assertNoRegressions — KNOWN_REGRESSIONS baseline fallback' suite below).
513+
knownRegressions: ReadonlySet<string> = KNOWN_REGRESSIONS,
504514
) {
505515
const real = checks.filter(Boolean) as RegressionCheck[];
506516
const regressions = real.filter((c) => {
507517
if (c.pctChange <= thresholdFor(c.label, engine)) return false;
508-
if (version && KNOWN_REGRESSIONS.has(`${version}:${c.label}`)) return false;
509-
// When `latest` is the rolling 'dev' build, KNOWN_REGRESSIONS entries
510-
// are anchored to the release where the regression was first observed
511-
// (e.g. '3.9.6:No-op rebuild'), not to 'dev'. Fall back to the baseline
512-
// version so a regression introduced before release N stays exempt for
513-
// every PR comparing dev → N until release N+1 clears it.
514-
if (
515-
version === 'dev' &&
516-
baselineVersion &&
517-
KNOWN_REGRESSIONS.has(`${baselineVersion}:${c.label}`)
518-
) {
518+
if (version && knownRegressions.has(`${version}:${c.label}`)) return false;
519+
// KNOWN_REGRESSIONS entries are anchored to the release where the
520+
// regression was first observed (e.g. '3.9.6:No-op rebuild'), not to
521+
// whatever `latest` happens to be. Fall back to the baseline version so
522+
// a regression introduced before release N stays exempt for every
523+
// comparison against N — both per-PR gates (latest = 'dev') and the
524+
// actual release N+1 publish gate itself (latest = the real N+1 version,
525+
// e.g. '3.16.0') — until release N+1's own benchmark data lands as the
526+
// new baseline and the entry goes stale (see the "KNOWN_REGRESSIONS
527+
// entries are not stale" test below).
528+
//
529+
// This used to be gated on `version === 'dev'`, which meant the one run
530+
// that most needs the exemption — the publish gate for the release that
531+
// is supposed to fold the known drift into a fresh baseline — never got
532+
// it, and failed on the exact regression it was meant to be exempt from
533+
// (v3.16.0 publish, #2127; entries added by #2107 for exactly this).
534+
if (baselineVersion && knownRegressions.has(`${baselineVersion}:${c.label}`)) {
519535
return false;
520536
}
521537
return true;
@@ -532,6 +548,72 @@ function assertNoRegressions(
532548
}
533549
}
534550

551+
// Pure-logic tests for the KNOWN_REGRESSIONS baseline fallback. Unlike the
552+
// describe.runIf(RUN_REGRESSION_GUARD) suite below, these don't read real
553+
// benchmark report files — they exercise assertNoRegressions directly, so
554+
// they always run and don't need a recorded 'X.Y.Z vs baseline' pair to exist
555+
// in committed history.
556+
//
557+
// These tests use a local, test-only `knownRegressions` fixture (fictional
558+
// '9.9.9' version) rather than the module-level KNOWN_REGRESSIONS — the real
559+
// set is expected to have its '3.15.0:*' entries pruned once v3.16.0's own
560+
// benchmark data lands (see the KNOWN_REGRESSIONS docstring above), and
561+
// hardcoding these always-on tests against that live, evolving data would
562+
// break them the moment that routine cleanup PR lands.
563+
describe('assertNoRegressions — KNOWN_REGRESSIONS baseline fallback', () => {
564+
const testKnownRegressions = new Set<string>(['9.9.9:Full build']);
565+
566+
test('exempts a real-release latest (not just dev) when its baseline matches a KNOWN_REGRESSIONS entry', () => {
567+
// Reproduces the v3.16.0 publish-gate failure (#2127): a KNOWN_REGRESSIONS
568+
// entry pinned to the prior release's baseline (e.g. '3.15.0:Full build',
569+
// added by #2107 for repo-growth drift), but the publish gate labels
570+
// `latest.version` with the real new version being published, never
571+
// 'dev' — this must still hit the baseline fallback.
572+
expect(() =>
573+
assertNoRegressions(
574+
[checkRegression('Full build', 5000, 3521)], // +42%, over the 25% threshold
575+
'10.0.0',
576+
'9.9.9',
577+
'native',
578+
testKnownRegressions,
579+
),
580+
).not.toThrow();
581+
});
582+
583+
test('still exempts the per-PR dev-vs-baseline comparison', () => {
584+
expect(() =>
585+
assertNoRegressions(
586+
[checkRegression('Full build', 5000, 3521)],
587+
'dev',
588+
'9.9.9',
589+
'native',
590+
testKnownRegressions,
591+
),
592+
).not.toThrow();
593+
});
594+
595+
test('does not exempt a regression against a baseline with no matching KNOWN_REGRESSIONS entry', () => {
596+
// This describe block runs unconditionally — including under
597+
// BENCH_CANARY=1 (.github/workflows/perf-canary.yml), which widens
598+
// REGRESSION_THRESHOLD from 0.25 to 0.5. The delta here must clear the
599+
// regression threshold in *every* mode this test can run under, or the
600+
// check gets filtered out as "not a regression" before the
601+
// KNOWN_REGRESSIONS fallback is even reached — making the test flip-flop
602+
// based on an ambient env var instead of the exemption logic it's meant
603+
// to exercise. +100% clears both the 25% default and the 50% canary
604+
// threshold with margin.
605+
expect(() =>
606+
assertNoRegressions(
607+
[checkRegression('Full build', 8000, 4000)],
608+
'10.0.0',
609+
'9.8.0', // not covered by testKnownRegressions
610+
'native',
611+
testKnownRegressions,
612+
),
613+
).toThrow(/Full build/);
614+
});
615+
});
616+
535617
// ── Build benchmark data types ───────────────────────────────────────────
536618

537619
interface BuildEngine {

0 commit comments

Comments
 (0)