Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .claude/agent-memory/e2e-test-engineer/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

- [general-e2e-patterns.md](general-e2e-patterns.md) — cross-cutting waits/timing, viewport timeouts, strict-mode anti-patterns, DataTable migration fallout, Gantt touch, breadcrumbs, print, dashboard cards, headless-shell PDF-iframe limitation → CSP header+console verification pattern (blob-fetch leg removed, connect-src pitfall), key file locations.
- [e2e-pom-patterns.md](e2e-pom-patterns.md) — Page Object Model conventions and stable-locator strategies.
- [e2e-parallel-isolation.md](e2e-parallel-isolation.md) — `testPrefix` fixture, parallel-safe data isolation, serial-mode tests.
- [e2e-parallel-isolation.md](e2e-parallel-isolation.md) — `testPrefix` fixture, parallel-safe data isolation, serial-mode tests (serial is **superseded** for preference rows — see next line).
- [isolated-user-fixture.md](isolated-user-fixture.md) — `e2e/fixtures/isolatedUser.ts` dedicated-user opt-in for preference-mutating specs (#1957); **`test.use()` reshuffles shard membership suite-wide → unrelated shards go red**; login rate limit 20/15min, soft user delete, worker-scoped-option-in-describe ban, `--list` as the only local validation.
- [known-flakes-and-regressions.md](known-flakes-and-regressions.md) — triaged log of flaky tests, pre-existing CI failures, and production regressions caught by E2E. **Check this before re-triaging a failure.**
- [flake-patterns.md](flake-patterns.md) — flake-avoidance patterns moved from the implementation checklist: Konva canvas coordinates, `test.slow()` timeout negation, locale timing after reload, shard redistribution, stale cache-warmup CI. Check when writing timing-sensitive tests or adding spec files.
- [auto-itemize-and-invoices-e2e.md](auto-itemize-and-invoices-e2e.md) — AutoItemizePage, PaperlessInvoiceReviewPage, invoice budget lines, Paperless mocking, vendor reassignment.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@ expect(countAfter).toBe(countBefore); // unchanged (not toBe(DEFAULT_CATEGORIES.

## Shared State Tests: Serial Mode

> **Superseded for per-user _preference_ rows (Issue #1957).** Serial mode only orders a file
> against itself, so it cannot protect `locale` / `dashboard.hiddenCards` / `table.*.columns`
> from writes by another file in another worker. Those specs now use a dedicated user via
> `e2e/fixtures/isolatedUser.ts` — see [isolated-user-fixture.md](isolated-user-fixture.md).
> Serial mode is still the right tool for shared-admin _identity_ mutations (display name,
> password, role) as listed below.

Tests that modify the shared admin user (display name, password, role) cannot use `testPrefix`
since there's only one admin user. Use `test.describe.configure({ mode: 'serial' })` inside the describe block:

Expand Down
27 changes: 27 additions & 0 deletions .claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,33 @@ technique that touches network/fetch is a safe workaround — check which specif
(`connect-src` vs `frame-src` vs `script-src`, etc.) governs the technique you're about to add,
since they're independently configured and a fix for one doesn't imply permission under another.**

## Never assert a suite-global DB precondition — scope list assertions to your own data (#1957, 2026-08-02)

An assertion like "the `?areaId=__none__` list is empty" is really "no area-less household item
exists anywhere in the shared DB" — and 48 of the suite's 62 `createHouseholdItemViaApi()` calls,
across 12 spec files, pass no areaId. Under `fullyParallel: true` such a test passes only while
no such spec shares its shard — pure shard luck, and it breaks the moment shard boundaries move
(which any `test.use()` addition anywhere in the suite does; see
[isolated-user-fixture.md](isolated-user-fixture.md)). It reads as a mysterious, unrelated
failure.

**Pattern:** AND the filter under test with a search scoped to the test's own entity names —
e.g. `?areaId=__none__&q=<testPrefix + scenario token>` on any DataTable page (`useTableState`
hydrates `q` from the URL, and the list services AND it with the other filters). This keeps the
empty-state expectations valid, because `q` counts towards DataTable's `hasActiveFilters`
(→ "No items match the current filters" + Clear Filters button). URL-driven, so it works on
mobile where filter popovers are CSS-hidden.

**Always pair it with a positive control**, or the fix creates a vacuous assertion: navigate
with `q` alone first and assert the seeded item IS listed. Otherwise an empty result is
indistinguishable from "the search matched nothing" and the test passes even if the filter
under test does nothing. Worked example: Scenario 4 of
`e2e/tests/household-items/no-area-filter.spec.ts`.

Sibling assertions in the same family are fine when they are per-entity rather than global —
`area-filter.spec.ts` Scenario 6 filters by a freshly created area ID it owns, so no other spec
can put an item in it. Check which shape an assertion has before "fixing" it.

## Key file locations

- Test fixtures: `e2e/fixtures/auth.ts` (testPrefix, authenticatedPage)
Expand Down
85 changes: 85 additions & 0 deletions .claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
---
name: isolated-user-fixture
description: e2e/fixtures/isolatedUser.ts — how to give a preference-mutating spec its own user; why opting a file in reshuffles shard membership suite-wide and reddens unrelated shards; plus the login rate limit and soft-delete facts that dictate per-worker vs per-test scope
metadata:
type: project
---

`e2e/fixtures/isolatedUser.ts` (added for Issue #1957) is the canonical way to stop a spec
from reading/writing per-user preference rows on the shared admin
(`test-results/.auth/admin.json`). Import `test`/`expect` from it and opt in:

- `test.use({ isolatedUserPerWorker: { emailPrefix: 'dash' } })` — **file scope only**, one
dedicated user per worker. Preferred.
- `test.use({ isolatedUserPerTest: { emailPrefix: 'x' } })` — file _or_ describe scope, fresh
user per test. Use when only a few tests inside a shared-admin file need isolation.

It works by overriding the built-in **`storageState` option**, so plain `page`/`context` are
already the dedicated user (no `scopedPage` rewrites) _and_ Playwright keeps trace/video/
screenshot artifacts — a hand-made `browser.newContext()` silently loses those. An `auto`
guard fixture checks `/api/auth/me` once per test so a silent regression of the override
fails loudly instead of quietly reverting to shared-admin writes.

**Why:** `LocaleContext.syncWithServer` treats the server as authoritative — it applies the
server locale _and_ deletes the victim's `localStorage.locale`, so a concurrent PATCH from
another file flips a running test's language permanently. Same shape for
`dashboard.hiddenCards` and `table.*.columns`. `mode: 'serial'` cannot fix it (orders a file
against itself only).

**How to apply — constraints that decide the scope, verified against the server:**

- `POST /api/auth/login` is rate-limited **20 requests / 15 min per IP**
(`server/src/routes/auth.ts`); every worker in a shard shares the bucket (`global: false`,
so only explicitly-configured routes are limited). Per-worker = 1 login per worker;
per-test multiplies fast. Budget it before converting a big file.
- `DELETE /api/users/:id` is a **soft delete** (`deactivateUser` + `destroyUserSessions`) —
the `user_preferences` row survives, so the issue's "ON DELETE CASCADE cleans up" claim is
wrong. Isolation still holds (the account can never be logged into again), but users
accumulate.
- User accumulation is bounded **per shard, not per run**: each shard is its own CI job with
its own container and DB (`containers/setup.ts` runs in globalSetup). So the 100-row
`/settings/users` page (`sortBy: null`, insertion order, scanned row-by-row by
`edit-user.spec.ts` / `deactivate-user.spec.ts`) is never remotely in reach — my original
worry was overstated. The case for per-worker rests on the login rate limit above.
- Keep the string "admin" out of generated e-mails/display names —
`search-users.spec.ts` asserts every row matching a search for "Admin" has "admin" in its
name column.
- Admin-gating is narrow: server `requireRole('admin')` only on `/api/users` mutations +
`/api/backups/*`; client only the Settings sub-nav "User Management"/"Backups" tabs and
work-item note edit/delete. So `role: 'member'` (the default) is fine for almost everything.

**Playwright mechanics learned here (all verified in `node_modules/playwright/lib`):**

- A **worker-scoped option cannot be set from inside a `describe`** — Playwright throws
"Cannot use({ x }) in a describe group, because it forces a new worker." That is the only
reason `isolatedUserPerTest` exists.
- Option values participate in the worker hash, so each distinct `test.use()` value forces
its own worker group (extra worker restarts, a few seconds per shard).
- **`test.use()` reshuffles shard MEMBERSHIP across the whole suite — expect an unrelated
shard to go red.** `createTestGroups` (`node_modules/playwright/lib/runner/index.js:2251`)
buckets tests by `test._workerHash` **first** and emits groups in bucket insertion order;
`filterForShard` (`:2321`) then walks that list and slices by cumulative test count. So
changing one file's worker hash moves **every** shard boundary in the suite, even with the
total test count unchanged. Any latent cross-file shared-state assumption in a newly
co-located pair then fails, looking completely unrelated to the change. This is what bit
#1957: `no-area-filter.spec.ts` moved from shard 15 to shard 14 next to
`area-filter.spec.ts`, whose Scenarios 3/5/6 hold area-less household items, and its
suite-global "no unassigned household item exists" precondition became unreachable.
**Diagnostic before blaming the change:** diff shard membership between base and head with
`npx playwright test --list --shard=N/16` — extract the base tree via
`git archive <base-sha> e2e | tar -x -C /tmp/base` and symlink `node_modules` into it.
Never respond by pinning or reordering shard assignment; fix the test that assumes
suite-global state (scope it to its own data — see
[general-e2e-patterns.md](general-e2e-patterns.md) and the Scenario 4 comment in
`e2e/tests/household-items/no-area-filter.spec.ts` for the `&q=`+`testPrefix` pattern).
- `_combinedContextOptions` depends on the `storageState` _fixture_, so overriding that option
really does re-point `page`/`context` (not a silent no-op).
- Project context options (incl. `baseURL`) are merged into `browser.newContext()` /
`request.newContext()` only by instrumentation installed by the test-scoped
`_setupArtifacts` fixture. A **worker-scoped** fixture runs before that, so it must pass
`baseURL` explicitly (`process.env.APP_BASE_URL || 'http://localhost:3000'`, set by
`containers/setup.ts` in globalSetup).
- `npx playwright test --list` validates the whole fixture graph (scope violations, cycles,
load errors) **without containers or a browser** — the only meaningful local check available
in this sandbox. `npx tsc --noEmit -p e2e/tsconfig.json` also works, but e2e carries ~123
pre-existing errors (it is not covered by `npm run typecheck`), so use it differentially.
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ metadata:
Note: `invoices.spec.ts` itself has **zero diff vs `origin/beta`** — these are pre-existing latent bugs from an earlier story (#1876), not introduced by #1877; they just happened to surface in PR #1883's E2E run because full E2E runs on every PR regardless of which files changed.

- **Issue #1829 (2026-07-08)** shard-3 diary flakes on main-targeted PRs: `diary-drafts.spec.ts:854` (Scenario 14) was genuinely failing on both attempt + retry — `test.slow()`'s 45s total test budget was being exhausted by 4 oversized nested step timeouts (30-45s each) before reaching cleanup, surfacing as `apiRequestContext.delete: Test timeout exceeded`. Fixed via `test.setTimeout(60_000)` + proportionate 15s step ceilings + `testInfo.setTimeout(testInfo.timeout + 15_000)` guaranteed-cleanup-time pattern in `finally`. `diary-r2-uat.spec.ts:599` (Scenario 10) had a genuine secondary race (always-empty mock defeats `waitForLoaded()`, letting a stale prior response satisfy the next `waitForResponse`) — fixed by awaiting each transition's own response explicitly and reading straight off the resolved `Response` object. `document-linking.spec.ts:369` was NOT an independent bug — confirmed via job logs it was cancelled mid-flight (1.6s runtime) as `maxFailures:1` collateral once drafts:854 exhausted its retries; no test-logic fix needed, just added the guaranteed-cleanup-time pattern defensively. Full root cause + `maxFailures`/retry-accounting proof (Playwright source citation) in [bug-1829-shard3-flakes.md](bug-1829-shard3-flakes.md). AC #3 (fail-fast retry tolerance) was already satisfied by Playwright's built-in behavior — no config change needed, only documented.
- **Issue #1957 / PR #1961 (2026-08-02)** `no-area-filter.spec.ts:194` [mobile] "empty state when no unassigned household items exist" failed on both attempt and retry, with two `household-item-edit` timeout flakes alongside — on a PR whose entire diff was `e2e/` preference isolation and which touched neither file. **Not a flake and not a timing issue:** the PR's `test.use()` opt-ins changed worker hashes, which moved shard boundaries suite-wide, relocating `no-area-filter.spec.ts` from shard 15 into shard 14 alongside `area-filter.spec.ts` — whose Scenarios 3/5/6 hold area-less household items — and the test asserted a suite-global "no area-less item exists" precondition. Fixed by scoping the assertion to its own data (`&q=<testPrefix token>`) plus a positive control; see the pattern in [general-e2e-patterns.md](general-e2e-patterns.md) and the mechanism in [isolated-user-fixture.md](isolated-user-fixture.md). **Triage rule learned: when a shard reddens on a PR that does not touch the failing file, diff shard membership (`--list --shard=N/16`) between base and head before assuming a flake.** A green rerun proves nothing here — the collision depends on shard-internal execution order.
- Diary Scenario 14 (`diary-drafts.spec.ts` draft-card-click-navigates) — was a persistent flake even after an earlier partial fix (PR #1671); root-caused and properly fixed via `fix/diary-scenario14-e2e-flake` (register `waitForResponse` before the click, not after). Recurred 2026-07-07 with a DIFFERENT failure signature (teardown-timeout, not nav-timing) — see #1829 entry above for the follow-up fix.
- `invoice-budget-line-create-and-link.spec.ts` Scenarios 1–4 — REAL production regression from PR #1566 (`eagerLinkInvoice:false`), filed as bug #1611. Tests were NOT weakened; they assert correct behavior.
- Shard 3 promotion blocker (`budget-source-filter.spec.ts`, 2026-06-12): "Rapid debounce coalesces requests" flaked on strict request-count assertions vs. CI click serialization beyond the 50ms debounce (fixed PR #1665); "Perspective toggle changes Cost value" flaked on reading `textContent()` immediately after a radio click without waiting for React re-render on WebKit (fixed PR #1666). **General fix pattern**: after any click triggering a React state update, `await expect(locator).not.toHaveText(previousValue)` before `textContent()` — never read immediately post-click on WebKit. Remove `page.on('request', ...)` listeners you add (persist for the page's lifetime otherwise); prefer state assertions (`aria-pressed`, URL params) over raw request counting.
Expand Down
Loading