diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index 971bc2c0d..25c44bcf6 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -614,3 +614,44 @@ deactivated users). Consequences for review: `parallelIndex` would make `createLocalUserViaApi`'s `expect(response.ok())` fail and mask the real failure. Require `${testPrefix}-${Date.now()}@e2e-test.local` (precedent: `i18n-categories.spec.ts`). - `deleteUserViaApi` ignores the response status, so cleanup failures in this family are always silent. + +## `page.route()` matcher traps in e2e specs (PR #1986) + +Two independent ways a route-interception assertion becomes vacuous, both invisible to CI: + +1. **`API` is an object map**, not a string (`e2e/fixtures/testData.ts:39`). `` page.route(`${API}/users/me/preferences`) `` interpolates to the glob `[object Object]/users/me/preferences`, which matches nothing — so `expect(captured).toHaveLength(0)` passes forever. The repo convention is `` `**${API.}` `` (property access + `**` prefix); `reportWizardEditableContent.spec.ts:969,998` do it right. ESLint's `restrict-template-expressions` would flag it but **CI runs no ESLint** (`static-analysis` = `npm audit signatures` + `typecheck` + `Stylelint` only). +2. Any **negative** route assertion (`toHaveLength(0)`, `not.toHaveBeenCalled`) is indistinguishable from a broken matcher. Always require the author to prove the matcher fires once (assert `1` against a deliberate request, then invert) before accepting the guard. + +## "Runs at all three viewports" is false unless the test is `@responsive`-tagged + +`e2e/playwright.config.ts`: `tablet` (iPad gen 7, 810px, webkit) and `mobile` (iPhone 13, 390px, webkit) +projects both set `grep: /@responsive/`. An untagged test runs **desktop only** — reject any AC/docstring +claiming multi-viewport coverage without `{ tag: '@responsive' }`. + +Adding the tag is not a free fix when the component has a **dual layout in the DOM**: `ReportContentEditor` +renders both a `` and a `.mobileCardList`, CSS-gated at `@media (max-width: 767px)`. `display: none` +drops the table from the a11y tree, so `getByRole('columnheader')` is 0 at mobile *regardless of state* — +`toHaveCount(0)` passes vacuously and `toHaveCount(1)` fails. Layout-dependent assertions must branch on +viewport (assert `.mobileCardRow` captions at mobile). Scenario 1b in that spec is the precedent guard. + +Related smell from the same PR: a test **title** naming behavior the body never asserts ("reset on remount", +"`
` cells" when only `` is checked) — a coverage illusion; trim the title or add the assertions. + +### Accessible-name locators: what they are and are not immune to (#1966 round 3) + +`getByRole(..., { name })` computes the name from **DOM text**, so it is immune to CSS `text-transform` — +the opposite of `innerText`/`toHaveText` assertions, which fail on transformed labels. Prefer the role+name +form when a component may style its casing. + +Two follow-on facts worth reusing: +- An embedded control inside a name-from-content traversal contributes its **value**, not its `aria-label`. + So an `EditableField` whose `ariaLabel` interpolates a neighbouring column's text cannot inflate the + containing cell's accessible name (and `exact: true` guards even if it could). +- `role=cell` / `role=columnheader` exposure depends on the table keeping table semantics — a `display: block` + or `display: flex` on the `` strips them in Chromium and silently zeroes such locators. Before + trusting a new `cell` assertion, confirm a sibling `columnheader` assertion already passes in CI; both rest + on the same exposure. + +Absence assertions need a **positive baseline in the same test** (`toHaveCount(1)` before, `toHaveCount(0)` +after). Without it, a typo'd or mis-scoped locator makes the absence check pass on nothing. With it, every +mis-scoping fails loudly instead — that property is the review bar, not the assertion count. diff --git a/.claude/agent-memory/product-architect/story-reviews.md b/.claude/agent-memory/product-architect/story-reviews.md index 86c196455..26c2da312 100644 --- a/.claude/agent-memory/product-architect/story-reviews.md +++ b/.claude/agent-memory/product-architect/story-reviews.md @@ -615,3 +615,52 @@ keeps both `tbody tr` rows and the mobile card list in the DOM, so the loops beh viewports. Three non-blocking follow-ups (loop-vs-seeded-row discriminating power, non-worker-scoped `no-match-` email, positional cell indices vs column preferences) — all recorded in [[recurring-patterns]]. + +## #1966 + #1969 / PR #1986 — column-toggle E2E coverage + testPrefix decoupling + +Round 1 CHANGES_REQUIRED (`${API}` object-interpolation making AC3 vacuous; untagged test claiming +three-viewport coverage; `no-empty-pattern` lint error; over-claiming test title). Round 2 (`9e4b0e57`) +still CHANGES_REQUIRED — but on a gap **my own round-1 review created**, see below. + +### I told them to trim a title when the AC required the assertion (my error) + +Round 1 I wrote "neither `` (ReportContentEditor.tsx:251) ← `vendor: invoice.vendorName` +(buildReportContent.ts:200) ← `vendorName: vendors.name` join (invoiceService.ts:272). + +Remaining non-blocking: unformatted new line (Prettier, invisible to CI on e2e-only PRs), stale AC4 docstring +paragraph, hardcoded preferences glob ×3, #1969 AC2 premise error (product-owner). diff --git a/e2e/fixtures/auth.ts b/e2e/fixtures/auth.ts index 44132f1ea..d010a0a7f 100644 --- a/e2e/fixtures/auth.ts +++ b/e2e/fixtures/auth.ts @@ -22,8 +22,11 @@ export const test = base.extend<{ // Unique prefix per worker+project to prevent data collisions in shared DB. // Format: "E2E-<3-char-project>" e.g. "E2E-des0", "E2E-tab2", "E2E-mob1" + // No auth dependency: testInfo provides all needed context without forcing a shared-admin + // browser context to be constructed for tests that authenticate as their own isolated user. testPrefix: [ - async ({ authenticatedPage: _ap }, use, testInfo: TestInfo) => { + // eslint-disable-next-line no-empty-pattern -- Playwright infers fixture deps from destructuring; {} is required syntax to declare no deps + async ({}, use, testInfo: TestInfo) => { const project = testInfo.project.name.slice(0, 3); // "des", "tab", "mob" await use(`E2E-${project}${testInfo.workerIndex}`); }, diff --git a/e2e/tests/budget/reportWizardEditableContent.spec.ts b/e2e/tests/budget/reportWizardEditableContent.spec.ts index 0fcaddb96..49c9f98c7 100644 --- a/e2e/tests/budget/reportWizardEditableContent.spec.ts +++ b/e2e/tests/budget/reportWizardEditableContent.spec.ts @@ -2256,3 +2256,140 @@ test.describe('Report wizard editable content — signature field reset (Scenari } }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 24: Column-visibility toggles — local state, no persistence (#1966) +// ───────────────────────────────────────────────────────────────────────────── +// +// ReportContentEditor renders a `role="group"` labelled "Show/hide columns" above the summary +// table. Checkboxes control per-column visibility using local `useState` only — the PDF always +// includes every column regardless of toggle state. This scenario asserts: +// AC1: every column checkbox is present and locatable by accessible name; +// AC2: the rendered checkbox count equals the component-defined toggleable-column count; +// AC3: toggling fires no PATCH to /api/users/me/preferences (local state, not persisted); +// AC4: coverage runs at desktop viewport only (no `@responsive` tag) — the toggle group and +// checkboxes are always visible regardless of viewport, but the `
` cells nor remount reset is required by AC1-AC4, so the cheap fix is to trim +the title" — without re-reading AC1, which bolds "the corresponding `` **and every matching `**`… +asserts **both** return". They trimmed, as instructed, and the required assertion stayed missing. +**Rule: when a test title over-claims, re-read the AC before recommending the trim.** An over-claiming title +has two fixes and they are not interchangeable — trimming is only correct once you have confirmed no AC +demands the named behavior. Getting this backwards converts a MEDIUM cosmetic finding into a silently +dropped requirement, and costs an extra review round on top. + +### `page.route` does not intercept `page.request.*` + +`page.route` only sees requests from the **browser context**. `page.request.patch()` / any +`APIRequestContext` call bypasses it. So the positive control for a route guard must be +`page.evaluate(() => fetch(...))`, not `page.request.*` — my round-1 fix spec suggested +`page.request.patch()` for exactly this purpose, which would have failed and looked like a broken matcher. +The author correctly used `page.evaluate`. Ordering is deterministic without any wait: the Node-side handler +pushes before `route.continue()`, so the in-page `await fetch` cannot resolve until the capture has happened. + +### Other verified facts from this review + +- `--report-unused-disable-directives` is the cheap way to prove an `eslint-disable` is live rather than + cargo-culted — run it whenever a PR adds a suppression. +- `Detect Changes` **skips `Static Analysis` entirely** on `e2e/`+`.claude/`-only PRs, so on those PRs the + local lint policy is the only lint gate that exists at all (weaker even than the usual "CI runs no ESLint"). +- AC premise error in #1969 AC2: asks that `testPrefix` "values differ" between two tests in one file, but + the value is `E2E-` — identical within a worker despite `{ scope: 'test' }`. + Flagged to product-owner for amendment rather than designed around (cf. the AC-premise-error rule). +- AC4's own suggested rationale ("the mobile card list exposes no column toggles") is factually wrong for + `ReportContentEditor` — the card layout gates every row on the same `show()` predicate. The desktop-only + exclusion is a limitation of the `columnheader` locator under `display: none`, not an absence of toggles. + +### Round 3 (`4cf5a735`) — APPROVED + +The `` gap was fixed the right way: `getByRole('cell', { name: , exact: true })` with a +**baseline `toHaveCount(1)` before the toggle** and `toHaveCount(1)` again after re-checking. That baseline is +what makes the `toHaveCount(0)` non-vacuous — insist on it every time a test asserts an element's absence. +Verified chain: `{row.vendor}` removal assertion +// uses `getByRole('columnheader')` which requires elements in the accessibility tree; +// the table is CSS-hidden on mobile (`max-width: 767px → .table { display: none }`), so +// `columnheader` assertions would fail at mobile. The mobile card layout is tested in +// other scenarios that carry `@responsive`. +// +// Uses `budget-overview` (7 columns incl. Status) to exercise the `content.isOverview` branch +// in ReportContentEditor's column list — a claim report would render 6 columns. + +test.describe('Report wizard editable content — column-visibility toggles, local state (Scenario 24, #1966)', () => { + // toggleable columns for budget-overview in insertion order (matches component source) + const OVERVIEW_COLUMNS = [ + 'Vendor', + 'Invoice No.', + 'Date', + 'Status', + 'Invoice Amount', + 'Allocated Amount', + 'Usage', + ] as const; + const OVERVIEW_COLUMN_COUNT = OVERVIEW_COLUMNS.length; // 7 + + test('Column toggles show/hide column headers and data cells (desktop) and never write to /api/users/me/preferences', async ({ + page, + testPrefix, + }) => { + const wizard = new ReportWizardPage(page); + + let vendorId = ''; + let sourceId = ''; + let workItemId = ''; + try { + vendorId = await createVendorViaApi(page, { name: `${testPrefix} Toggle Vendor` }); + sourceId = await createBudgetSourceViaApi(page, { + name: `${testPrefix} Toggle Source`, + totalAmount: 5000, + // contactAddress + reference required for cover letter to auto-enable on budget-overview + contactAddress: '1 Toggle St, Testville', + reference: 'Ref-TOGGLE', + }); + workItemId = await createWorkItemViaApi(page, { title: `${testPrefix} WI Toggle` }); + await seedAllocatedInvoice(page, workItemId, vendorId, sourceId, { + invoiceNumber: `${testPrefix}-TOG-001`, + amount: 500, + date: '2026-06-01', + status: 'pending', + }); + + await reachStep5(wizard, sourceId, 'budget-overview'); + + // ── AC1 + AC2: group present, every checkbox visible and checked, count matches component ── + const columnGroup = page.getByRole('group', { name: 'Show/hide columns' }); + await expect(columnGroup).toBeVisible(); + + const checkboxes = columnGroup.getByRole('checkbox'); + // AC2: count must equal the component-defined column list length so a future column + // addition fails this test instead of silently going uncovered. + await expect(checkboxes).toHaveCount(OVERVIEW_COLUMN_COUNT); + + // AC1: each column is locatable by its label and checked by default + for (const label of OVERVIEW_COLUMNS) { + await expect(columnGroup.getByLabel(label)).toBeVisible(); + await expect(columnGroup.getByLabel(label)).toBeChecked(); + } + + // ── AC3: intercept preference writes ── + // Note: API is an object (`testData.ts`), so `${API}/...` would expand to + // `[object Object]/...` and never match. Use the glob form instead. + const prefPatches: string[] = []; + await page.route('**/api/users/me/preferences', (route) => { + if (route.request().method() === 'PATCH') prefPatches.push(route.request().url()); + void route.continue(); + }); + + // Positive control: confirm the interceptor fires before relying on "nothing fired". + // A real PATCH issued via page.evaluate() must increment the counter. + await page.evaluate(async () => { + await fetch('/api/users/me/preferences', { + method: 'PATCH', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({}), + }); + }); + expect( + prefPatches, + 'positive control: interceptor must capture a manually-triggered PATCH', + ).toHaveLength(1); + prefPatches.length = 0; // reset before the actual toggle assertions + + // ── Toggle off: "Vendor" disappears from both table header and data cells ── + // AC1 requires asserting BOTH the and the matching are absent. + // ReportContentEditor uses conditional rendering (`show('vendor') && …` and + // `show('vendor') && …`), so both are removed from the DOM entirely when hidden. + const vendorHeader = page.getByRole('columnheader', { name: 'Vendor', exact: true }); + const vendorCell = page.getByRole('cell', { + name: `${testPrefix} Toggle Vendor`, + exact: true, + }); + + // Baseline: both are present before any toggle + await expect(vendorHeader).toHaveCount(1); + await expect(vendorCell).toHaveCount(1); + + await columnGroup.getByLabel('Vendor').uncheck(); + await expect(columnGroup.getByLabel('Vendor')).not.toBeChecked(); + await expect(vendorHeader).toHaveCount(0); + await expect(vendorCell).toHaveCount(0); + + // ── Toggle back on: both column header and data cell return ── + await columnGroup.getByLabel('Vendor').check(); + await expect(columnGroup.getByLabel('Vendor')).toBeChecked(); + await expect(vendorHeader).toHaveCount(1); + await expect(vendorCell).toHaveCount(1); + + // AC3: no preference PATCH was issued during any column toggle + expect(prefPatches, 'column toggle must not write to /api/users/me/preferences').toHaveLength( + 0, + ); + } finally { + if (workItemId) await deleteWorkItemViaApi(page, workItemId); + if (sourceId) await deleteBudgetSourceViaApi(page, sourceId); + if (vendorId) await deleteVendorViaApi(page, vendorId); + } + }); +});