From 1b20b8e89163de793c204aa05572054b6651f92e Mon Sep 17 00:00:00 2001 From: TurtleWolfe Date: Thu, 20 Aug 2026 05:53:46 +0000 Subject: [PATCH] fix(#396): the zero-assertion reporter printed nothing in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #846 added a reporter that names any test finishing with zero assertions, wired it into `playwright.config.ts`, verified it locally, mutation-tested it three ways, and merged it. **It printed nothing in CI.** `playwright test --reporter=X` REPLACES the config's `reporter` array. It does not append. Every lane passes one — `line,json` in e2e-local, `list` and `blob` across e2e.yml — so the reporter ran in the two lanes that do not override (smoke, signup-mailer) and in NONE of the 25 shards on the required lane. That is the exact family #396 catalogues — a control whose presence was asserted and whose effect never was — committed by the tool built to detect it. It was caught by grepping the merged run's logs for the reporter's own output and getting nothing back, which is the only check that could have found it: everything else about the change was green. Fixed at all 8 overriding invocations (1 in e2e-local, 7 in e2e.yml), appending the reporter to each existing list rather than replacing it. `merge-reports` lines are left alone — merging a report is not running the suite. GUARDED, because the config and the workflows are different files with nothing relating them and the failure is silent in the worst way: the reporter is present, correct, unit-tested and mute, and nothing goes red. `scripts/__tests__/reporter-is-not-inert.test.js` asserts every `--reporter=` override still names it, with both halves of non-vacuity — the reporter file must exist, and several overrides must be found, since "every override includes it" is trivially true of zero overrides. Mutation-verified, mutant confirmed present first, two ways: reverting e2e-local to the exact string that shipped inert (caught, naming `e2e-local.yml:429`), and deleting the reporter file (caught by the existence check). Worth stating for the next person: the local verification was not wrong, it was incomplete. Running `playwright test --reporter=` by hand proves the reporter WORKS; it proves nothing about whether CI invokes it. Those are different questions and only the second one mattered here. test:scripts 428/428; type-check and lint clean. Refs #396 --- .github/workflows/e2e-local.yml | 2 +- .github/workflows/e2e.yml | 14 +- .../__tests__/reporter-is-not-inert.test.js | 121 ++++++++++++++++++ 3 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 scripts/__tests__/reporter-is-not-inert.test.js diff --git a/.github/workflows/e2e-local.yml b/.github/workflows/e2e-local.yml index 1095c68b..e7fbe1c5 100644 --- a/.github/workflows/e2e-local.yml +++ b/.github/workflows/e2e-local.yml @@ -426,7 +426,7 @@ jobs: --project=${{ matrix.project }} --shard=${{ matrix.shard }} \ $WORKERS_FLAG \ --grep-invert='@hosted' \ - --reporter=line,json || true + --reporter=line,json,./tests/e2e/reporters/assertion-count-reporter.ts || true env: CI: 'true' SKIP_WEBSERVER: 'true' diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index ed860f5b..3b2d0023 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -314,7 +314,7 @@ jobs: tests/e2e/auth/sign-up.spec.ts \ --project=signup \ --no-deps \ - --reporter=list \ + --reporter=list,./tests/e2e/reporters/assertion-count-reporter.ts \ --timeout=30000 env: CI: true @@ -407,7 +407,7 @@ jobs: scripts/ci/playwright-in-container.sh test \ --project=basepath \ --no-deps \ - --reporter=list \ + --reporter=list,./tests/e2e/reporters/assertion-count-reporter.ts \ --timeout=30000 env: CI: true @@ -503,7 +503,7 @@ jobs: echo "serve is responding on http://localhost:3000" - name: Run rate-limiting tests (ordered) - run: scripts/ci/playwright-in-container.sh test --project=rate-limiting --project=brute-force --project=signup --reporter=list --trace=on-first-retry + run: scripts/ci/playwright-in-container.sh test --project=rate-limiting --project=brute-force --project=signup --reporter=list,./tests/e2e/reporters/assertion-count-reporter.ts --trace=on-first-retry env: CI: true PLAYWRIGHT_BROWSER: chromium @@ -591,7 +591,7 @@ jobs: echo "serve is responding on http://localhost:3000" - name: Run auth setup - run: scripts/ci/playwright-in-container.sh test --project=setup --reporter=list --timeout=180000 + run: scripts/ci/playwright-in-container.sh test --project=setup --reporter=list,./tests/e2e/reporters/assertion-count-reporter.ts --timeout=180000 env: CI: true AUTH_SETUP_JOB: true @@ -800,7 +800,7 @@ jobs: scripts/ci/playwright-in-container.sh test \ --project="$PROJECT" \ --shard="$SHARD" \ - --reporter=blob \ + --reporter=blob,./tests/e2e/reporters/assertion-count-reporter.ts \ --trace=on-first-retry \ --no-deps \ $WORKERS_FLAG @@ -977,7 +977,7 @@ jobs: scripts/ci/playwright-in-container.sh test \ --project="$PROJECT" \ --shard="$SHARD" \ - --reporter=blob \ + --reporter=blob,./tests/e2e/reporters/assertion-count-reporter.ts \ --trace=on-first-retry \ --no-deps \ $WORKERS_FLAG @@ -1134,7 +1134,7 @@ jobs: scripts/ci/playwright-in-container.sh test \ --project="$PROJECT" \ --shard="$SHARD" \ - --reporter=blob \ + --reporter=blob,./tests/e2e/reporters/assertion-count-reporter.ts \ --trace=on-first-retry \ --no-deps \ $WORKERS_FLAG diff --git a/scripts/__tests__/reporter-is-not-inert.test.js b/scripts/__tests__/reporter-is-not-inert.test.js new file mode 100644 index 00000000..c7124e34 --- /dev/null +++ b/scripts/__tests__/reporter-is-not-inert.test.js @@ -0,0 +1,121 @@ +/** + * A reporter in `playwright.config.ts` does nothing if a lane passes `--reporter=` (#396). + * + * WHAT HAPPENED, and it is worth recording because the tool built to detect this family + * committed the family's own defect. PR #846 added a reporter that names any test + * finishing with zero assertions, and wired it into `playwright.config.ts`. It was + * verified locally, mutation-tested three ways, and merged. + * + * It printed NOTHING in CI. + * + * `playwright test --reporter=X` REPLACES the config's `reporter` array — it does not + * append. Every lane passes one (`line,json` in e2e-local, `list` and `blob` in e2e.yml), + * so the reporter ran in exactly the two lanes that do not override (smoke, + * signup-mailer) and in none of the 25 shards on the REQUIRED lane. Found by grepping + * the run logs for its own output and getting nothing back — the check that a control + * has an EFFECT, not merely a presence. + * + * WHY A TEST AND NOT JUST THE FIX. The config and the workflows are different files with + * nothing relating them, and the failure is silent in the worst way: the reporter is + * present, correct, unit-tested, and mute. Nothing goes red. This relates them. + * + * WHAT THIS CANNOT CHECK: that the reporter produces useful output, or that a lane's + * output is ever read. It asserts the wiring reaches every lane that runs the suite. + */ +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..', '..'); +const WORKFLOWS = path.join(ROOT, '.github', 'workflows'); +const REPORTER = 'assertion-count-reporter'; + +/** Lines that actually run the suite, ignoring comments and report merging. */ +function invocations() { + const out = []; + for (const file of fs + .readdirSync(WORKFLOWS) + .filter((f) => /\.ya?ml$/.test(f))) { + const src = fs.readFileSync(path.join(WORKFLOWS, file), 'utf8'); + src.split('\n').forEach((line, i) => { + if (/^\s*#/.test(line)) return; + if (/merge-reports/.test(line)) return; + if (!/--reporter=/.test(line)) return; + out.push({ file, line: i + 1, text: line.trim() }); + }); + } + return out; +} + +describe('the zero-assertion reporter actually runs (#396)', () => { + it('the reporter file exists and the config references it', () => { + // Non-vacuity: if the reporter were deleted, every assertion below about + // `--reporter=` lists would be checking a string nothing produces. + const rp = path.join( + ROOT, + 'tests', + 'e2e', + 'reporters', + 'assertion-count-reporter.ts' + ); + assert.ok(fs.existsSync(rp), 'the reporter file is gone'); + const cfg = fs.readFileSync( + path.join(ROOT, 'playwright.config.ts'), + 'utf8' + ); + assert.match( + cfg, + new RegExp(REPORTER), + 'playwright.config.ts no longer lists it' + ); + }); + + it('finds the lanes that override the reporter list', () => { + // The other half of non-vacuity. "Every override includes it" is trivially true + // of zero overrides — which is exactly the state this test would have to + // distinguish from the real one. + assert.ok( + invocations().length >= 5, + `expected several --reporter= invocations across the lanes, found ${invocations().length}` + ); + }); + + it('every lane that overrides --reporter still includes it', () => { + const missing = invocations().filter((v) => !v.text.includes(REPORTER)); + assert.deepEqual( + missing.map((v) => `${v.file}:${v.line}`), + [], + "`playwright test --reporter=X` REPLACES the config's reporter array rather " + + 'than appending to it, so these lanes run without the zero-assertion ' + + 'reporter entirely. That is how #846 shipped a reporter that printed nothing ' + + 'in the required lane. Append it to the list: ' + + `--reporter=,./tests/e2e/reporters/${REPORTER}.ts` + ); + }); + + it('the detector can actually fail', () => { + const has = (line) => + !/^\s*#/.test(line) && + /--reporter=/.test(line) && + !/merge-reports/.test(line); + + assert.equal(has(' --reporter=line,json \\'), true); + assert.equal( + has(' # --reporter=line'), + false, + 'a comment is not an invocation' + ); + assert.equal( + has(' pnpm exec playwright merge-reports --reporter=json ./blobs'), + false, + 'merging reports is not running the suite' + ); + + // The exact bug: an override that omits the reporter must be reported. + const bad = { file: 'x.yml', line: 1, text: '--reporter=line,json' }; + assert.equal(bad.text.includes(REPORTER), false); + }); +});