fix(#934): make the required E2E aggregate consult its own shards - #935
Merged
Conversation
CI reported chromium/firefox/webkit `gen 2/6` failing on this branch, with the zero-assertion gate saying "this shard executed 0 tests (0 skipped) — a green tick here would be meaningless" (#861). The gate was right and it was my change. CI sets `fullyParallel: false` (playwright.config.ts:78) so specs sharing the PRIMARY/TERTIARY fixture users run serially. Under that setting Playwright shards by FILE — a file is indivisible — and color-contrast.spec.ts is 142 of chromium-gen's 668 tests. Adding the third house theme pushed it past a balancing threshold and left shard 2 with nothing. Measured against the parent commit, same command: shard baseline with forge with this fix 1/6 184 232 113 2/6 39 0 112 3/6 89 108 116 4/6 105 112 112 The sweep authenticates nothing and shares no fixture user, so its cases are safe to schedule individually. `test.describe.configure({ mode: 'parallel' })` makes them distributable across shards without touching `workers`, which stays 1 in CI for the specs that genuinely need it. Identical distribution on all three browsers, and better balanced than the baseline it replaces — 39 in one shard was already a symptom. The first diagnosis was wrong and worth recording: three shards failing across three browsers looked like a real forge contrast violation, which is what the PR body warns could happen. It was not. The annotation named the cause exactly, and reading it beat reasoning about it. type-check, lint clean; theme guards 14 passed.
`E2E (local) result` concluded `success` on run 32560171118, whose shard matrix concluded `failure`. Branch protection reads the named context rather than the run, so PR #928 merged with chromium/firefox/webkit `gen 2/6` red, and 95e4ff4 is on main. The aggregate reconstructed a verdict from uploaded artifacts and never asked the shards. Its own log shows why every condition still passed: shards reporting: 24/24 passed 2035 baseline 1807 failed 0 Gate passed: 24/24 shards, 0 failures, above the floor. The three shards had each executed ZERO tests. `Upload results` is `if: always()`, so a failing shard still uploads a valid results.json describing nothing; a shard with no tests has no failing tests; and Playwright redistributed the work, so the total rose to 2035 and cleared the 1700 floor. The zero-test detector was right and was the only thing that noticed. The blast radius is wider than that detector. Every anti-vacuity guard in this lane runs `if: always()` after the tests, so no verdict of theirs reaches the artifacts — including check-zero-assertions.mjs (#861), whose subject is tests that pass while measuring nothing. Those are `expected` in results.json, so the totals count them as passed. The guards built to stop vacuous green were invisible to the check that decides whether green means anything. Fix: one step reading `needs.e2e-local.result`. A STEP, not a job-level `if:`, for the reason every other condition here is a step — branch protection is never satisfied by a `skipped` job, so it must run and pass on a docs-only PR, and it is gated on `changes` so it does. Placed last, so the totals print first and a future reader sees the numbers that looked fine before the verdict that did not. The totals stay; they answer a different question and they caught #732. Verified by mutation rather than by reading, which is the only thing that finds this class: step deleted pass 4 fail 3 exit 1 -> echo pass 6 fail 1 != success inverted pass 6 fail 1 step ungated pass 6 fail 1 restored pass 7 fail 0 and the step's shell driven directly: success -> rc=0; failure, cancelled and skipped -> rc=1. The guard strips comments before matching. The workflow now carries a comment block naming these very symbols, and a raw-text match would pass with the code deleted — which has happened here four times, twice to guards written to catch this class. test:scripts 546 passed; type-check and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Closes #934. The finding, the evidence and the blast radius live in that issue body.
What merged that should not have
E2E (local) result— a required check — concluded success onrun 32560171118,
whose shard matrix concluded failure. Three shards had each executed zero tests. PR #928
merged on that green tick and
95e4ff4eis onmain.The aggregate reconstructed a verdict from uploaded artifacts and never asked the shards.
Its own log:
Upload resultsisif: always(), so a failing shard still uploads a validresults.jsondescribing nothing; a shard with no tests has no failing tests; and Playwright redistributed
the work, so the total rose to 2035 and cleared the 1700 floor. Every condition passed
because of the failure, not despite it.
Two commits, and the order matters
da2e725ccolor-contrast.spec.tsshardable, so no shard runs zero testsd322064bneeds.e2e-local.resultThey are in one PR because the gate fix cannot land on a
mainwhose shards are alreadyred —
mainis in exactly that state right now, running zero tests ingen 2/6on allthree browsers. Split across two PRs, the second would correctly block itself on the first.
#930is open against the samemainand will hit the same starved shard; it should pickthis up after merge.
Verified by mutation, not by reading
Reading a guard does not find this class — both green-through-deletion cases in this repo
were invisible on review and obvious in one mutation run.
The step's shell driven directly:
success→ rc=0;failure,cancelled,skipped→ rc=1.test:scripts546 passed ·type-checkclean ·lintclean · pre-push gate green.What is deliberately unchanged
and they caught e2e-local.yml cannot fail: 21 failed tests concluded success #732. This adds the question nobody was asking.
if:. Branch protection is never satisfied by askippedjob,which is why every other condition in this job is already a step. It is gated on
changesso a docs-only PR still passes, and there is a test for that direction too.
green — and note it would not have helped here anyway: the total went up.
The part worth keeping after this merges
Every anti-vacuity guard in this lane runs
if: always()after the tests, so none of theirverdicts reach the artifacts — including
check-zero-assertions.mjs(#861), whose entiresubject is tests that pass while measuring nothing. Those show as
expectedinresults.json, so the totals count them as passed. The guards built to stop vacuous greenwere invisible to the check that decides whether green means anything.
🤖 Generated with Claude Code