From 63a2923272a48467de22d6356313579944914f9d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 17:37:57 +0000 Subject: [PATCH] fix(map): select capture tests with a precise grep, not a loose substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `styleproof-map` selected the capture with `--grep 'styleproof capture'`. Playwright compiles a plain `--grep` string as `new RegExp(pattern, 'gi')` — case-INSENSITIVE — and matches it against the whole grep title path: file path, every enclosing describe, the test title, and tags. So the selector matched any consumer test whose title merely MENTIONED StyleProof capture in prose. That is not hypothetical. A consumer spec titled "ci runners: the StyleProof capture fixture actually contains the lane panel" was selected as a capture test. `styleproof-ci --spec-ref` overlays the head harness onto the BASE checkout, so it then ran against the base application, asserted head-only UI, failed, and took the whole base capture down with it: base capture failed (exit 1) — continuing with a bare baseline 230 new surface(s) with no baseline The head capture passed, the run looked green, and the gate certified nothing. The selector is now the regex literal `/(?:^|\s)styleproof capture(?:\s|$)/`. The `/.../` form makes Playwright honour it as a regex with no flags, so matching is case-SENSITIVE; the boundaries stop it matching a longer word. It still selects both capture blocks and the `styleproof browser build` test nested inside them, since a describe title is part of every descendant's grep title. It also now lives in `src/runner.ts` beside the `test.describe` titles it must agree with, and `bin/styleproof-map.mjs` imports it. The selector and the titles were two independent string literals with nothing holding them together; a test pins that the CLI no longer carries its own copy. Not exported from `src/index.ts` — this is an internal contract between the runner and the map CLI, so the public API is unchanged. Considered and rejected: - Restricting selection to `--spec`'s file. A consumer may legitimately split captures across files, and dropping those would silently reduce the captured surface set — the one failure this tool exists to prevent. - Playwright tags (`{ tag: '@styleproof' }`). They need >= 1.42 and the declared peer floor is >= 1.40. Residual, stated plainly: a consumer title containing the exactly-lowercase phrase "styleproof capture" would still be selected. Case-sensitivity removes the realistic collision — prose naturally capitalises the product name — without a peer bump or a silent coverage change. Verified: `npm test` 635 passed; typecheck, lint and format:check clean. The new suite asserts against Playwright's own `forceRegExp` semantics, reproduced from `playwright/lib/util.js`, rather than a hand-written regex — including a case proving the OLD bare-string selector did match the consumer title. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EFwLkpuAEXYKNNDNQqy4uc --- CHANGELOG.md | 13 ++++++ bin/styleproof-map.mjs | 5 ++- src/runner.ts | 35 ++++++++++++++- test/capture-grep.test.mjs | 91 ++++++++++++++++++++++++++++++++++++++ test/cli.test.mjs | 7 ++- 5 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 test/capture-grep.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index f2cb1f4..31579bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixed + +- **`styleproof-map` no longer selects consumer tests that merely mention "styleproof + capture".** Playwright compiles a plain `--grep` string as `new RegExp(pattern, 'gi')` + — case-insensitive — and matches it against the whole title path, so the bare + `styleproof capture` selector swept in any spec whose title mentioned StyleProof + capture in prose. Combined with `styleproof-ci --spec-ref`, which overlays the head + harness onto the base checkout, such a spec then ran against the base application, + failed, and took the entire base capture down with it — leaving every surface without + a baseline while the head capture reported success. The selector is now a + case-sensitive, word-bounded regex literal, and it lives beside the `test.describe` + titles it has to agree with rather than being a second independent string literal. + ## [4.7.4] - 2026-08-01 ### Fixed diff --git a/bin/styleproof-map.mjs b/bin/styleproof-map.mjs index fe40a8b..909f6cf 100755 --- a/bin/styleproof-map.mjs +++ b/bin/styleproof-map.mjs @@ -42,6 +42,9 @@ import { workingTreeDirty, writeMapManifest, } from '../dist/map-store.js'; +// The capture selector lives beside the `test.describe` titles it must agree with. +// Importing it keeps one source of truth instead of two string literals that drift. +import { CAPTURE_TEST_GREP } from '../dist/runner.js'; const STYLEPROOF_PLAYWRIGHT_CONFIG = 'playwright.styleproof.config.ts'; const STYLEPROOF_VARIANTS_SCRIPT = path.join(path.dirname(fileURLToPath(import.meta.url)), 'styleproof-variants.mjs'); @@ -384,7 +387,7 @@ const env = { ...(tolerateSurfaceFailures ? { STYLEPROOF_TOLERATE_SURFACE_FAILURES: '1' } : {}), }; runVariantCrawl(env); -const result = spawnSync(command, ['test', '--grep', 'styleproof capture', ...configArgs, ...playwrightArgs], { +const result = spawnSync(command, ['test', '--grep', CAPTURE_TEST_GREP, ...configArgs, ...playwrightArgs], { stdio: 'inherit', env, }); diff --git a/src/runner.ts b/src/runner.ts index dd0bf53..a0d9e0c 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1149,6 +1149,36 @@ function writeBrowserBuildTest(settings: Settings, dir: string): void { }); } +/** + * The `--grep` `styleproof-map` selects the capture tests with. + * + * A REGEX LITERAL, deliberately, not the bare phrase. Playwright compiles a plain + * `--grep` string as `new RegExp(pattern, 'gi')` — **case-insensitive** — and matches + * it against the whole grep title path: file path, every enclosing describe, the test + * title, and any tags. A bare `styleproof capture` therefore selects any consumer test + * whose title merely MENTIONS StyleProof capture in prose. + * + * That is not hypothetical. A consumer spec titled + * `"ci runners: the StyleProof capture fixture actually contains the lane panel"` + * was swept into the capture run. Because `styleproof-ci --spec-ref` overlays the head + * harness onto the BASE checkout, it then ran against the base application, asserted + * head-only UI, failed, and took the entire base capture down with it — leaving 230 + * surfaces with no baseline to diff against while the head capture reported success. + * A gate that certifies nothing while looking green is the worst failure this tool has. + * + * The `/.../` form makes Playwright honour it as a regex with no flags, so matching is + * case-SENSITIVE, and the `(?:^|\s)` / `(?:\s|$)` boundaries stop it matching a longer + * word. It still matches both capture blocks below, including the nested + * `styleproof browser build` test, because the describe title is part of every + * descendant's grep title. + * + * Kept here, beside the `test.describe` titles it has to agree with, because the + * selector and the titles were two independent string literals and nothing held them + * together. Not exported from `index.ts`: this is an internal contract between the + * runner and `bin/styleproof-map.mjs`, not public API. + */ +export const CAPTURE_TEST_GREP = '/(?:^|\\s)styleproof capture(?:\\s|$)/'; + export function defineStyleMapCapture(options: DefineOptions): void { const { surfaces, expected, exclude = {}, dir } = options; const captureSurfaces = surfaces.flatMap(expandSurfaceVariants); @@ -1421,8 +1451,9 @@ export function defineCrawlCapture(options: CrawlOptions): void { const settings = resolveSettings(options); - // Title contains "styleproof capture" so the same `--grep 'styleproof capture'` - // that styleproof-map uses to select capture tests picks up crawl specs too. + // Title opens with "styleproof capture" so {@link CAPTURE_TEST_GREP}, which + // styleproof-map selects capture tests with, picks up crawl specs too. The trailing + // " (crawl)" keeps the space the selector's closing boundary needs. test.describe('styleproof capture (crawl)', () => { // Record the completeness basis. Without `expected` a crawl has no registry to // check against, so it records `expected: null` (honestly "not asserted": it diff --git a/test/capture-grep.test.mjs b/test/capture-grep.test.mjs new file mode 100644 index 0000000..c57bd51 --- /dev/null +++ b/test/capture-grep.test.mjs @@ -0,0 +1,91 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import { CAPTURE_TEST_GREP } from '../dist/runner.js'; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + +/** + * Playwright's own `--grep` compilation, copied from `forceRegExp` in + * `playwright/lib/util.js`: a `/pattern/flags` string becomes that regex, and + * ANY OTHER string becomes `new RegExp(pattern, 'gi')` — case-insensitive. + * + * That fallback is the whole bug. Reproducing it here rather than asserting + * against a hand-written regex keeps the test honest about what Playwright + * actually does with the string we hand it. + */ +function compileGrepLikePlaywright(pattern) { + const asRegexLiteral = pattern.match(/^\/(.*)\/([gi]*)$/); + if (asRegexLiteral) return new RegExp(asRegexLiteral[1], asRegexLiteral[2]); + return new RegExp(pattern, 'gi'); +} + +/** + * Playwright matches the grep against the joined title path — file, every enclosing + * describe, the test title, then tags — not against the test title alone. + */ +function grepTitle(...titlePathSegments) { + return titlePathSegments.join(' '); +} + +function matchesCaptureGrep(...titlePathSegments) { + const compiled = compileGrepLikePlaywright(CAPTURE_TEST_GREP); + compiled.lastIndex = 0; + return compiled.test(grepTitle(...titlePathSegments)); +} + +test('the capture selector still selects every capture test', () => { + // The two describes in runner.ts, and the browser-build test nested inside them — + // it inherits the describe title, which is what the selector matches on. + assert.ok(matchesCaptureGrep('e2e/styleproof.spec.ts', 'styleproof capture', 'home @ 1440')); + assert.ok(matchesCaptureGrep('e2e/styleproof.spec.ts', 'styleproof capture (crawl)', 'settings @ 768')); + assert.ok(matchesCaptureGrep('e2e/styleproof.spec.ts', 'styleproof capture', 'styleproof browser build')); +}); + +test('the capture selector ignores a consumer test that merely mentions StyleProof capture', () => { + // The real title that broke a consumer: it asserts head-only UI, and because + // `styleproof-ci --spec-ref` overlays the head harness onto the BASE checkout, being + // swept into the capture run made it run against the base application, fail, and take + // the whole base capture down — 230 surfaces left with no baseline, head green. + assert.equal( + matchesCaptureGrep( + 'tests/e2e/view-behaviours.spec.ts', + 'ci runners: the StyleProof capture fixture actually contains the lane panel', + ), + false, + 'a prose mention of "StyleProof capture" must not be selected as a capture test', + ); + + // The same title through the OLD bare-string selector, to prove the bug was real and + // that this test would not have caught it before. + const bareStringSelector = compileGrepLikePlaywright('styleproof capture'); + assert.ok( + bareStringSelector.test( + grepTitle( + 'tests/e2e/view-behaviours.spec.ts', + 'ci runners: the StyleProof capture fixture actually contains the lane panel', + ), + ), + 'the bare-string selector DID match it — that is the defect being fixed', + ); +}); + +test('the capture selector does not match a longer word', () => { + assert.equal(matchesCaptureGrep('spec.ts', 'restyleproof capturely', 'x'), false); +}); + +test('styleproof-map selects with the shared constant, not its own literal', () => { + // The selector and the `test.describe` titles used to be two independent string + // literals with nothing holding them together; either could drift silently. + const mapCli = fs.readFileSync(path.join(REPO_ROOT, 'bin/styleproof-map.mjs'), 'utf8'); + + assert.match(mapCli, /--grep',\s*CAPTURE_TEST_GREP/); + assert.doesNotMatch( + mapCli, + /'--grep',\s*'styleproof capture'/, + 'the CLI must not carry its own copy of the selector', + ); +}); diff --git a/test/cli.test.mjs b/test/cli.test.mjs index 3e0ab17..6001fcb 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -84,7 +84,10 @@ test('styleproof-map runs Playwright with local cache defaults', () => { env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` }, }); assert.equal(r.status, 0, r.stderr); - assert.match(r.stdout, /current\|\.styleproof\/maps\|1\|test --grep styleproof capture/); + assert.match( + r.stdout, + /current\|\.styleproof\/maps\|1\|test --grep \/\(\?:\^\|\\s\)styleproof capture\(\?:\\s\|\$\)\//, + ); assert.equal(fs.existsSync(path.join(root, '.styleproof/maps/current/home@1280.har')), false); assert.ok(fs.existsSync(path.join(root, '.styleproof/maps/current', MAP_MANIFEST))); } finally { @@ -276,7 +279,7 @@ test('styleproof-map runs configured variant crawl before Playwright capture', ( assert.equal(r.status, 0, r.stderr); assert.deepEqual(fs.readFileSync(log, 'utf8').trim().split('\n'), [ 'crawl:--base-url http://127.0.0.1:3000 --out styleproof.variants.generated.json --route / --route settings=/settings --strict', - 'map:test --grep styleproof capture', + 'map:test --grep /(?:^|\\s)styleproof capture(?:\\s|$)/', ]); } finally { rmTmp(root);