MetaMask / MetaMask/metamask-extension
[P1] PRs that change only the benchmark harness never run the benchmarks
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 13.2k
- Forks
- 5.6k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 451
Description
**Parent Epic:** [#6944 Performance Quality Gates](https://github.com/MetaMask/MetaMask-planning/issues/6944) → [#7182 Benchmarks: Reliability](https://github.com/MetaMask/MetaMask-planning/issues/7182) → [#7206 CI Reliability Improvements](https://github.com/MetaMask/MetaMask-planning/issues/7206)
**File:** `.github/workflows/get-requirements.yml`
**Size:** S | **Hours:** ~3-4
---
## Problem
**A PR that changes only the benchmark harness never runs the benchmarks.**
`needs-benchmarks` is downstream of build reuse. `find-reusable-builds` hashes the *application* sources; when that hash matches the base branch, the PR reuses the base branch's builds and `run-benchmarks` is skipped. Benchmark-owned code — the mocks, the harness, the thresholds, the gate allowlist — is not in that hash by construction, so a PR touching only those files always matches, always reuses, and never benchmarks.
The code that decides what the gate measures is precisely the code the gate never exercises on the PR that changes it.
### Observation
Four PRs open on 2026-08-12, all touching benchmark or CI code:
| PR | changes | `builds-from-run` | `run-benchmarks` |
|---|---|---|---|
| #45443 | `test/e2e/benchmarks/mocks/` | `31579703000` (a **main** run) | **skipped** |
| #45444 | `gated-metrics.ts` + test | `31579703000` (**main**) | **skipped** |
| #45445 | `.github/scripts/`, `.github/workflows/` | `31579703000` (**main**) | **skipped** |
| #45455 | `.github/scripts/`, `.github/workflows/` | `31585190838` (**its own**) | **ran** |
As of 2026-09-02: #45444 merged 2026-08-28; #45445 closed unmerged 2026-09-01; #45443 and #45455 are still open. The table describes CI behavior observed at filing time and is unaffected by those PRs' later disposition.
#45455 is the control, and it discriminates cleanly: it touches the same file types as #45445 but is based on a feature branch rather than `main`, so no reusable build was found, it built its own, and its benchmarks ran. The variable is build reuse, not file type.
From the `Filter files changed and decide which jobs to run` job of [run 31582455153](https://github.com/MetaMask/metamask-extension/actions/runs/31582455153):
```
Searching branch 'jongsun/test/benchmark-solana-discovery-mocks' for matching build hash...
No match on 'jongsun/test/benchmark-solana-discovery-mocks', trying 'main'...
```
It matched `main`, reused those builds, and skipped. The head SHA carries `build-source-hash` and `builds-from-run` commit statuses but no `benchmarks-required` status. That status does not exist anywhere in `get-requirements.yml` on `main`; the file is unchanged since `b3a1226` (2026-06-25) and contains no `benchmarks-required` string. An earlier revision of #45352 proposed adding it as a per-commit pin; the revision of #45352 open now (read 2026-09-02) does not add it, so there is currently no mechanism anywhere in the codebase, merged or in flight, that sets this status.
### Why it matters beyond convenience
- **#45443's falsifier cannot be read from its own CI.** That falsifier — the ~8.4–10.2s cluster in `doneButtonToAssetList` disappearing — needs a benchmark run on the mocked population, and a PR run *is* that population. The run is skipped, so the change would merge with nothing to check it against.
- **The reuse logic is correct about builds and wrong about benchmarks.** Reusing a build when app sources are unchanged is right. Concluding "therefore nothing to measure" is not: the measurement apparatus changed even though the thing measured did not.
- **A skipped job counts as passing** in [`ci-status-gate.yml:66`](https://github.com/MetaMask/metamask-extension/blob/0350a0187/.github/workflows/ci-status-gate.yml#L66) — `} else if (result !== 'success' && result !== 'skipped') {` routes to the failure branch, so a skipped result never reaches it. Same shape as #45351 (a stale build-listing page causes an unnecessary rebuild) and #45450 (a benchmark that never ran scores as a gate pass). All three are a missing input scoring as a pass.
---
## Solution
`needs-benchmarks` must be true when benchmark-owned paths change, independent of build reuse. A `dorny/paths-filter` clause forcing it on:
```
test/e2e/benchmarks/**
development/metamaskbot-build-announce/**
shared/constants/benchmarks.ts
.github/workflows/run-benchmarks.yml
.github/scripts/benchmark-stats-commit.sh
```
The forced path must be evaluated **before** the reuse short-circuit.
Whether the decision should also be pinned per commit, so a later re-run can't silently flip it, is open: the pinning approach described above was dropped from #45352's current revision, so nothing in flight provides it. If this ticket still needs that guarantee, it has to add the pin itself rather than relying on #45352.
Open question for whoever takes this: a forced benchmark run still needs a build. Either the PR builds its own (cost) or it benchmarks the reused base build (measuring the base's app with the PR's harness — which is actually the correct comparison for a harness-only change, and cheaper). The second is probably right and should be stated explicitly rather than fallen into.
---
## Acceptance Criteria
- [ ] A PR touching only `test/e2e/benchmarks/**` runs `run-benchmarks`
- [ ] The head SHA of such a PR carries a `benchmarks-required` commit status — note that as of 2026-09-02 no open PR provides this status, so it falls to whichever PR closes this ticket to add it
- [ ] A PR touching no benchmark paths and reusing base builds still skips, unchanged
- [ ] Which build a forced run measures against is documented in the workflow
- [ ] #45443's falsifier is readable from its own CI
---
## Labels
`team-extension-platform`, `area-CI`, `area-testSuite`
---
## Dependencies
**Addressed by, not yet merged:** #45460 (open, stacked on #45352 — implements the `dorny/paths-filter` clause this ticket specifies, and its own decision-table verification covers the same four PRs listed above)
**Related:** #45352 (open, unmerged; adds the reusable-build-listing retry from #45351, not the per-commit pinning an earlier revision proposed), #45351 (the same missing-input-scores-as-a-pass shape, from a stale-listing angle), #45450 (a gated benchmark with no result scores as a pass), #45443 (the PR whose falsifier this issue blocks), #45266 (the bimodal-onboarding fix #45443 carries)
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in .github/workflows/get-requirements.yml, especially the “Filter files changed and decide which jobs to run” job and its build-reuse short-circuit. Review .github/workflows/run-benchmarks.yml and ci-status-gate.yml to determine how a forced benchmark run and benchmarks-required status should be represented. Done means benchmark-only changes run benchmarks, non-benchmark reuse still skips, and the measured build choice is documented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, typescript
- Domain
- ci-cd, devops
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- Half a day
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 62/100