fix(ci): run benchmarks when benchmark-owned files change on reused builds - #45460
Open
MajorLift wants to merge 1 commit into
Open
fix(ci): run benchmarks when benchmark-owned files change on reused builds#45460MajorLift wants to merge 1 commit into
MajorLift wants to merge 1 commit into
Conversation
The build hash covers application sources, so a PR touching only the harness, the mocks, the thresholds or the gate allowlist always matches its base branch, always reuses those builds, and never runs the benchmarks it changes. The app is unchanged; what the gate measures about it is not. Adds a `benchmark_files` filter and ORs it into the decision, mirroring the existing `unit_integration_test_files` rule. Such a run measures the reused base build with the new apparatus, which isolates the apparatus change and costs no extra build.
MajorLift
requested review from
a team,
HowardBraham and
itsyoboieltr
as code owners
August 12, 2026 10:39
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
metamask-ci
Bot
added
team-extension-platform
Contributor
Builds ready [1e6ed43]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 8 warn · 🔴 2 fail)
Bundle size diffs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
CHANGELOG entry: null
Description
A PR that changes only the benchmark harness never runs the benchmarks.
find-reusable-buildshashes application sources. Benchmark-owned code — the harness, the mocks, the thresholds, the gate allowlist — is not in that hash by construction, so such a PR always matches its base branch, always reuses those builds, andneeds-benchmarksresolvesfalse. The application is unchanged; what the gate measures about it is not.Observed across four PRs open on 2026-08-12:
builds-from-runrun-benchmarkstest/e2e/benchmarks/mocks/31579703000(a main run)gated-metrics.ts+ test31579703000(main).github/scripts/,.github/workflows/31579703000(main).github/scripts/,.github/workflows/31585190838(its own)#45455 is the control: same file types as #45445, but based on a feature branch, so no reusable build was found and its benchmarks ran. The variable is build reuse, not file type.
From the requirements job of run 31582455153:
It matched
mainand skipped. That head SHA carriesbuild-source-hashandbuilds-from-runstatuses but nobenchmarks-requiredstatus — #45352's pin never fires, because the decision short-circuits before it.What changed
A
benchmark_filesfilter, ORed into the decision. This mirrors the existingunit_integration_test_filesrule, which already carries exactly this reasoning — "changes require running unit/integration even when builds are reused" — for test files.The forced run measures the reused base build with the new apparatus. That is deliberate and is the cheaper option: for a harness-only change it is also the comparison that isolates the change, since holding the application constant is what makes the apparatus difference legible. A fresh build would vary both at once.
The decision still flows into #45352's
benchmarks-requiredpinning, so it stays fixed per commit and a re-run cannot flip it.Verification
Decision table, logic extracted verbatim from the step, run against the four PRs above plus two controls:
bench-decision-check.txtThe first control is the one that matters: an ordinary PR that touches no benchmark paths and reuses base builds still skips. This widens the trigger, it does not remove it.
Both files parse (
yaml.safe_loadonfilter-rules.ymlandget-requirements.yml).What this does not prove: that a forced run produces usable numbers against a reused build. The decision logic is checked here; the end-to-end behaviour is only observable once this lands and a harness-only PR runs. The falsifier is direct — #45443 should stop showing
run-benchmarks: skipped.Related issues
Fixes: #45456
needs-benchmarksdecision to the head SHA #45352 — pins the decision per commit; this is why that pin never fires on harness-only PRsManual testing steps
filter-rules.ymlandget-requirements.yml, neither of which is inbenchmark_files—get-requirements.ymlis touched by many unrelated CI PRs, and listing it would run the full benchmark matrix on all of them. (An earlier revision of this section claimed the opposite; that was wrong.) Verify via step 2 instead.run-benchmarksis no longer skipped, and that its head SHA gains abenchmarks-requiredcommit status.Pre-merge author checklist
Note
Medium Risk
Changes CI gate logic that decides when the benchmarks job runs, which can affect merge gating. Scope is small and widens the trigger rather than narrowing it.
Overview
Fixes a CI gap where PRs that only change the benchmark harness reused base builds and skipped benchmarks entirely, so apparatus changes were never measured.
Adds a
benchmark_filespath filter and ORs it into theneeds-benchmarksdecision: benchmarks now run on fresh builds or benchmark-owned file changes. App-only PRs that reuse builds still skip. Forced runs intentionally measure the reused base build with the new apparatus.Reviewed by Cursor Bugbot for commit 1e6ed43. Bugbot is set up for automated code reviews on this repo. Configure here.