diff --git a/.github/workflows/e2e-local.yml b/.github/workflows/e2e-local.yml index 6fcd3f85..adc72433 100644 --- a/.github/workflows/e2e-local.yml +++ b/.github/workflows/e2e-local.yml @@ -763,7 +763,9 @@ jobs: on_baseline = os.environ.get("TESTED_SHA", "") == BASE_SHA if not on_baseline: print("") - print("Gate passed: 24/24 shards, 0 failures, above the floor.") + print("Artifact totals OK: 24/24 shards, 0 failures, above the floor.") + print("The shard matrix's OWN verdict is a separate step after this one") + print("— these totals cannot see a shard that failed after reporting (#934).") print("COUNT PARITY not evaluated — the cloud numbers describe") print(f"{BASE_SHA[:8]}; on any other commit the test set has moved and a") print("difference is uninterpretable. Re-run on that SHA to decide parity.") @@ -775,3 +777,51 @@ jobs: sys.exit(1) print("PARITY: local matches the cloud baseline exactly.") EOF + + # ── THE VERDICT THE SHARDS ALREADY REACHED (#934) ──────────────────── + # + # Everything above reconstructs a verdict from uploaded artifacts. This asks the + # shards. They are different questions, and until now only the reconstructed one + # was asked — so this REQUIRED context concluded `success` on a run whose own + # matrix concluded `failure`, and PR #928 merged with three shards red. + # + # HOW THAT PASSED, from the aggregate's own log in run 32560171118: + # + # shards reporting: 24/24 + # passed 2035 baseline 1807 + # failed 0 + # + # `chromium/firefox/webkit gen 2/6` had each executed ZERO tests and failed on the + # zero-test detector, which was doing its job. Every condition above still passed: + # `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 went UP, clearing the floor. + # + # The blast radius is wider than that one detector. EVERY anti-vacuity guard in this + # lane runs `if: always()` after the tests, so none of their verdicts reach the + # artifacts — including check-zero-assertions.mjs (#861), whose whole 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. + # + # 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 this must run and pass + # on a docs-only PR. Gated on `changes` so it does — when the matrix is skipped its + # result is `skipped`, which is correct there and must not fail. + # + # Deliberately LAST: the totals print first, so a future reader sees the numbers that + # looked fine and then the verdict that did not. + - name: The shards themselves must have passed + if: needs.changes.outputs.run == 'true' + run: | + result='${{ needs.e2e-local.result }}' + echo "shard matrix concluded: $result" + if [ "$result" != "success" ]; then + echo "::error::the E2E shard matrix concluded '$result', so this check fails." + echo "::error::The totals above may look clean — they are reconstructed from" + echo "::error::uploaded artifacts and cannot see a shard that failed AFTER" + echo "::error::writing a valid report (a zero-test shard, a zero-assertion" + echo "::error::verdict, any post-upload step). Open the red shard job; its own" + echo "::error::log names the cause. Do not relax this to make a run green." + exit 1 + fi diff --git a/scripts/__tests__/e2e-local-aggregate-consults-shards.test.js b/scripts/__tests__/e2e-local-aggregate-consults-shards.test.js new file mode 100644 index 00000000..d6e0da13 --- /dev/null +++ b/scripts/__tests__/e2e-local-aggregate-consults-shards.test.js @@ -0,0 +1,180 @@ +/** + * The required aggregate must consult the shard matrix's own verdict (#934). + * + * WHAT HAPPENED. `E2E (local) result` concluded `success` on run 32560171118, whose shard + * matrix concluded `failure` — `chromium/firefox/webkit gen 2/6` had each executed zero + * tests. Branch protection reads the named context, not the run, so PR #928 merged with + * three shards red. + * + * The aggregate reconstructed a verdict from uploaded artifacts and never asked the shards. + * Every condition it did check passed: `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 went UP and cleared + * the floor. + * + * WHY A TEST AND NOT JUST THE FIX. The fix is one `if:` and one `exit 1`. Deleting it turns + * the required check back into one that cannot fail, and nothing goes red to say so — the + * defect's entire signature is that everything looks green. That is #396, and it is exactly + * the class this repo keeps re-deriving. + * + * COMMENTS ARE STRIPPED BEFORE MATCHING. The workflow now carries a long comment block + * explaining this defect, and that prose names the very symbols asserted below. A guard that + * matched raw text would pass with the guarded code deleted — which has happened here four + * times, twice to guards written to catch this class. + */ + +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); + +const WF = path.resolve( + __dirname, + '..', + '..', + '.github', + 'workflows', + 'e2e-local.yml' +); +const RAW = fs.readFileSync(WF, 'utf8'); + +/** Whole-line YAML comments removed. Trailing `#` inside a value is not one. */ +function stripComments(text) { + return text + .split('\n') + .filter((l) => !/^\s*#/.test(l)) + .join('\n'); +} + +/** The `parity:` job block, comments removed. */ +function parityBlock(text = RAW) { + const start = text.indexOf('\n parity:'); + if (start === -1) return ''; + return stripComments(text.slice(start)); +} + +/** Does this text consult the shard matrix's conclusion? Both dot and bracket forms. */ +function consultsShards(text) { + return ( + /needs\.e2e-local\.result/.test(text) || + /needs\[\s*['"]e2e-local['"]\s*\]\.result/.test(text) + ); +} + +describe('the required aggregate consults its shards (#934)', () => { + it('the parser found the job it is asserting about', () => { + // Anti-vacuity, first. A renamed job or a moved file makes every assertion below + // pass by inspecting an empty string — the same shape as the defect itself. + assert.ok(RAW.length > 2000, `${WF} is suspiciously small — stale path?`); + const block = parityBlock(); + assert.ok(block.length > 500, 'parity: job not found, or it is empty'); + assert.match( + block, + /name: E2E \(local\) result/, + 'the required context name is gone from the parity job — if it was renamed, branch ' + + 'protection now waits on a context nobody reports' + ); + }); + + it('comment stripping actually removes comments', () => { + // The control for the control. If stripComments() silently stopped working, the + // assertion below would go back to matching the prose that explains the fix. + const stripped = stripComments(RAW); + assert.ok( + stripped.length < RAW.length - 2000, + 'stripComments removed almost nothing — this file is heavily commented, so the ' + + 'stripper is broken and the assertions below may be matching prose' + ); + assert.doesNotMatch( + stripped, + /WHY THIS EXISTS/, + 'a known comment survived stripping' + ); + }); + + it('the aggregate reads the shard matrix result, in CODE not prose', () => { + assert.ok( + consultsShards(parityBlock()), + 'the parity job no longer reads `needs.e2e-local.result`. It is then judging the ' + + 'run purely by uploaded artifacts, which cannot see a shard that failed AFTER ' + + 'writing a valid report — a zero-test shard, a zero-assertion verdict (#861), or ' + + 'any post-upload step. That is how #928 merged with three shards red.' + ); + }); + + it('and can actually fail on it', () => { + // Reading the value and printing it would satisfy the assertion above while gating + // nothing. The job must exit non-zero. + const block = parityBlock(); + const start = block.search(/needs\.e2e-local\.result|needs\[/); + const region = block.slice(start, start + 1200); + assert.match( + region, + /exit 1/, + 'the shard result is read but nothing exits non-zero on it — the check reports the ' + + 'failure and passes anyway' + ); + assert.match( + region, + /!=\s*["']?success/, + 'the comparison is not against `success`. Any other conclusion — failure, ' + + 'cancelled, timed_out — must fail this check.' + ); + }); + + it('the step is gated on `changes`, so a docs-only PR still passes', () => { + // The counterweight. When the matrix is skipped its result is `skipped`, which is + // correct there. An ungated assertion would block every docs-only PR — and a required + // check that fails on documentation is how this lane loses its unfiltered trigger. + const block = parityBlock(); + const stepStart = block.indexOf( + '- name: The shards themselves must have passed' + ); + assert.notStrictEqual( + stepStart, + -1, + 'the shard-verdict step was renamed; update this test deliberately, not reflexively' + ); + const step = block.slice(stepStart, stepStart + 400); + assert.match( + step, + /if:\s*needs\.changes\.outputs\.run == 'true'/, + 'the shard-verdict step is not gated on the changes job' + ); + }); + + it('the job itself is still ungated and still `if: always()`', () => { + // Branch protection is never satisfied by a `skipped` job. Moving the gate from the + // steps up to the job would make the required check go PENDING FOREVER on docs-only + // PRs — passing locally, blocking merges, with nothing red to explain it. + const block = parityBlock(); + const head = block.slice(0, block.indexOf('steps:')); + assert.match( + head, + /if:\s*always\(\)/, + 'parity is no longer `if: always()`' + ); + assert.match( + head, + /needs:\s*\[changes,\s*e2e-local\]/, + 'parity no longer depends on both `changes` and `e2e-local` — it cannot read a ' + + 'result it does not depend on' + ); + }); + + it('CONTROL: the matcher reports absence when the call is removed', () => { + // Without this, an always-true matcher would satisfy every assertion above. This is + // the mutation the reviewer cannot perform by reading. + const mutated = parityBlock().replace( + /\$\{\{\s*needs\.e2e-local\.result\s*\}\}/g, + "'success'" + ); + assert.ok( + !consultsShards(mutated), + 'the matcher still reports the call present after it was removed — it is matching ' + + 'something other than the code it claims to check' + ); + }); +}); diff --git a/tests/e2e/color-contrast.spec.ts b/tests/e2e/color-contrast.spec.ts index 60357825..d2d81242 100644 --- a/tests/e2e/color-contrast.spec.ts +++ b/tests/e2e/color-contrast.spec.ts @@ -233,6 +233,20 @@ console.log( .join('\n') ); +// SHARDABLE, NOT ATOMIC (#915). +// +// CI sets `fullyParallel: false` (playwright.config.ts:78) so that specs sharing the +// PRIMARY/TERTIARY test users run serially. Under that setting Playwright shards by FILE: +// a file is an indivisible unit, and this one is 142 of chromium-gen's 668 tests. Adding a +// third house theme pushed it over a balancing threshold and left `gen 2/6` with ZERO tests +// — measured: shard 2 went 39 -> 0, and the zero-assertion gate (#861) correctly refused to +// call that green. +// +// This sweep authenticates nothing and shares no fixture user, so it is safe to schedule per +// test. `mode: 'parallel'` makes its cases individually distributable across shards without +// touching `workers`, which stays 1 in CI for the specs that genuinely need it. +test.describe.configure({ mode: 'parallel' }); + test.describe('WCAG AAA color-contrast-enhanced (violations only)', () => { // Match pa11yci.json viewport. test.use({ viewport: { width: 1280, height: 1024 } });