diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index acde9174d..dcd665f33 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -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. diff --git a/.claude/agent-memory/e2e-test-engineer/e2e-parallel-isolation.md b/.claude/agent-memory/e2e-test-engineer/e2e-parallel-isolation.md index caa1a4903..7b38a2093 100644 --- a/.claude/agent-memory/e2e-test-engineer/e2e-parallel-isolation.md +++ b/.claude/agent-memory/e2e-test-engineer/e2e-parallel-isolation.md @@ -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: diff --git a/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md b/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md index daaf718e2..53aad8699 100644 --- a/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md +++ b/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md @@ -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=` 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) diff --git a/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md b/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md new file mode 100644 index 000000000..a266de578 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md @@ -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 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. diff --git a/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md b/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md index 016f4639c..902bb9655 100644 --- a/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md +++ b/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md @@ -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=`) 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. diff --git a/e2e/fixtures/isolatedUser.ts b/e2e/fixtures/isolatedUser.ts new file mode 100644 index 000000000..1e4ea3f95 --- /dev/null +++ b/e2e/fixtures/isolatedUser.ts @@ -0,0 +1,416 @@ +/** + * Dedicated-user isolation for preference-mutating E2E specs (Issue #1957). + * + * ───────────────────────────────────────────────────────────────────────────── + * WHY THIS EXISTS + * ───────────────────────────────────────────────────────────────────────────── + * `playwright.config.ts` runs with `fullyParallel: true` and every project + * authenticates as the SAME shared admin user (`test-results/.auth/admin.json`). + * Per-user preference rows (`user_preferences`, keyed by user id) are therefore a + * single mutable row shared by every concurrently running test in the suite. + * + * This is not merely "a test reads stale data" — it actively corrupts a running + * test's UI. `LocaleContext.syncWithServer` (client/src/contexts/LocaleContext.tsx) + * treats the SERVER value as authoritative: on every page load it fetches + * `/api/users/me/preferences` and, when a `locale` row exists, applies the server + * value AND deletes the `locale` key from `localStorage`. So if test A is asserting + * English text and test B (different file, different worker) PATCHes `locale='de'` + * on the shared admin, test A's next navigation flips to German and the + * `localStorage` override that would otherwise have protected it has just been + * deleted by the same sync call. Same class of failure for + * `dashboard.hiddenCards` (cards vanish/reappear mid-test) and + * `table..columns` (columns appear/disappear mid-test). + * + * `test.describe.configure({ mode: 'serial' })` cannot fix this: it only + * serializes a file against ITSELF and has no knowledge of writes coming from + * other files running concurrently in other workers. + * + * The remedy is to give preference-mutating tests their own disposable user, so + * the row they write is unreachable by any other test in the suite. This module + * packages that pattern (previously hand-rolled in `dashboard.spec.ts`, + * `i18n-categories.spec.ts` and `change-password.spec.ts`) as a fixture, so the + * spec bodies keep using the plain `page` fixture and keep Playwright's automatic + * trace/video/screenshot instrumentation (a hand-made `browser.newContext()` does + * not get those artifacts). + * + * ───────────────────────────────────────────────────────────────────────────── + * AUDIT — every spec that writes /api/users/me/preferences (Issue #1957, AC1) + * ───────────────────────────────────────────────────────────────────────────── + * Reproduce the file list with: `grep -rl "users/me/preferences" e2e/tests/` + * (`e2e/pages/DashboardPage.ts` and `e2e/pages/InvoicesPage.ts` also match that + * string, but only inside `waitForResponse()` predicates — they observe the + * app's own requests, they never issue one, so they are not audit entries.) + * + * 1. e2e/tests/navigation/dashboard.spec.ts + * Keys: `dashboard.hiddenCards`, `locale` (top-level `beforeEach` reset, ran + * for all ~34 tests) + `dashboard.hiddenCards` written by the app itself when + * Scenario 6/7 click a card's dismiss/re-enable button. + * Admin-gated? No. The dashboard, `/project/work-items`, `/budget/invoices`, + * `/diary/new` and the Add/Customize dropdowns have no role checks. The complete + * role-gating inventory in the app: server — `requireRole('admin')` on + * `/api/users` mutations and `/api/backups/*`, plus work-item note + * update/delete, which pass `request.user.role === 'admin'` into `noteService` + * as an ownership override (`server/src/routes/notes.ts`); client — the Settings + * sub-nav "User Management" and "Backups" tabs, and work-item note edit/delete + * on other users' notes. Nothing else in `client/src` or `server/src` branches + * on role. + * Resolution (AC2): `isolatedUserPerWorker` at file scope. + * + * 2. e2e/tests/i18n/i18n.spec.ts + * Keys: `locale` (PATCH in `setLanguage()`/`resetToEnglish()`, plus + * `DELETE /api/users/me/preferences/locale` in "DELETE preference resets to + * system locale"). + * Admin-gated? No — profile, dashboard, budget, schedule, diary, work items + * and the Settings→Vendors tab are all member-visible. + * Resolution (AC2, AC4): `isolatedUserPerWorker` at file scope. The file-level + * `serial` guard added in PR #1956 is kept only as defence-in-depth against CPU + * contention between two slow German-locale tests; it is no longer the + * isolation mechanism. + * + * 3. e2e/tests/i18n/i18n-categories.spec.ts + * Key: `locale`. ALREADY ISOLATED before this issue — creates a dedicated user + * per test and logs it into its own browser context. No change needed; listed + * here because AC1 requires the audit to cover every file in the grep output. + * + * 4. e2e/tests/diary/diary-uat-fixes.spec.ts + * Key: `dashboard.hiddenCards` (Scenario 3 "Dashboard shows a Recent Diary + * card" and Scenario 7 "Recent Diary View All link", which reset the key so + * the card is guaranteed visible). Same key as dashboard.spec.ts's reset — + * the second confirmed cross-file collision pair. + * Admin-gated? No — both tests only read the dashboard with mocked + * `/api/diary-entries`. + * Resolution (AC2, AC5): `isolatedUserPerTest` at describe scope for those two + * describes; the other six tests in the file touch no preferences and stay on + * the shared admin. + * + * 5. e2e/tests/invoices/invoices.spec.ts + * Key: `table.invoices.columns` (two DELETEs, both inside the single test + * "Toggling Effective Amount ..."), plus the app's own debounced PATCH of that + * key when `enableColumn()` toggles a column. + * Admin-gated? No — the invoices list/detail pages and vendor/invoice/work-item/ + * budget-source creation have no role checks. + * Resolution (AC2): `isolatedUserPerTest` at describe scope for the + * "Effective Amount" describe only. + * + * No audited spec needs the admin role, so no spec required the AC3 treatment. + * `role: 'admin'` is supported below for any future case that does. + * + * Related shared-state hazards deliberately OUT of scope here (they do not write + * `/api/users/me/preferences` and so are outside AC1's grep, but the next person + * should know): the app persists `table..columns` for every DataTable, so + * any spec that toggles columns as the shared admin writes a preference row for + * that table — only a spec asserting the same table's columns can be a victim, + * and `invoices.spec.ts` is currently the only such spec. Dark-mode specs set + * `data-theme` on the document instead of persisting a preference, so they are + * not affected. + * + * ───────────────────────────────────────────────────────────────────────────── + * USAGE + * ───────────────────────────────────────────────────────────────────────────── + * File scope — one dedicated user per worker, shared by every test in the file: + * + * import { test, expect } from '../../fixtures/isolatedUser.js'; + * test.use({ isolatedUserPerWorker: { emailPrefix: 'dash' } }); + * + * Describe scope — a fresh dedicated user for each test in that describe: + * + * test.describe('...', () => { + * test.use({ isolatedUserPerTest: { emailPrefix: 'diary-dash' } }); + * }); + * + * Either way the built-in `page`/`context` fixtures are already authenticated as + * the dedicated user, so `page.request.patch('/api/users/me/preferences', ...)` + * inside a test writes that user's row and nothing else's. Tests that need the + * user's identity can read the `isolatedUserSession` fixture. + * + * WHICH SCOPE TO PICK + * - `isolatedUserPerWorker` is the default choice. A Playwright worker runs one + * test at a time, and no other worker shares the user, so the row still cannot + * be written concurrently by anything else in the suite — while costing one user + * per worker instead of one per test. State CAN carry over between the + * sequential tests of one worker, so a file using it must still reset any key it + * dirties (see dashboard.spec.ts's `beforeEach` / i18n.spec.ts's `afterEach`). + * - `isolatedUserPerTest` gives a guaranteed-pristine row per test, at the cost of + * one user creation + login per test (~0.5-1s, which counts against the test + * timeout). Use it for a handful of tests inside an otherwise shared-admin file, + * because a worker-scoped option cannot be set from inside a `describe` + * (Playwright rejects it: "Cannot use({...}) in a describe group, because it + * forces a new worker"). + * + * !! EXPECT AN UNRELATED SHARD TO GO RED WHEN YOU OPT A FILE IN !! + * Adding `test.use()` for either option changes the affected tests' `_workerHash`. + * `createTestGroups` (node_modules/playwright/lib/runner/index.js) buckets tests by + * worker hash FIRST and emits groups in bucket insertion order; `filterForShard` + * then slices that list by cumulative test count. So opting one file in + * redistributes shard MEMBERSHIP across the whole suite — not just this file's — + * even when the total test count is unchanged. Any latent cross-file shared-state + * assumption in a newly co-located pair surfaces as a failure that looks unrelated + * to your change. #1957 hit exactly this: `no-area-filter.spec.ts` moved from shard + * 15 to shard 14 next to `area-filter.spec.ts` and its suite-global "no area-less + * household item exists" precondition broke. Diagnose before blaming the change + * itself, by diffing shard membership between base and head: + * git archive e2e | tar -x -C /tmp/base && ln -s /node_modules /tmp/base/node_modules + * (cd /tmp/base && npx playwright test --list --shard=N/16) # vs the same in the worktree + * Do NOT respond by pinning or reordering shards (#1957's Notes rule that out) — + * fix the test that assumes suite-global state. + * + * Users are created via `POST /api/users` (admin-only) and removed in fixture + * teardown via `DELETE /api/users/:id`. Note that DELETE is a SOFT delete + * (`deactivateUser` + `destroyUserSessions`): the preference row survives, but the + * account can never be logged into again and its e-mail is never reused, so + * nothing can read or write that row afterwards. 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 `/settings/users`' 100-row page + * is never in reach. The reason to prefer per-worker is the login rate limit + * (`POST /api/auth/login`: 20 requests / 15 min, keyed on `request.ip`, one bucket + * per shard), not user-table size. + */ + +import type { APIRequestContext, PlaywrightWorkerArgs } from '@playwright/test'; +import { test as authTest } from './auth.js'; +import { API } from './testData.js'; + +/** + * Shared-admin storage state written by `auth.setup.ts`; mirrors `use.storageState` + * in playwright.config.ts. Used here to authenticate the admin API context that + * provisions and deactivates dedicated users. (Non-opted-in tests get their state + * from `testInfo.project.use.storageState`, not from this constant.) + */ +export const ADMIN_STORAGE_STATE = 'test-results/.auth/admin.json'; + +/** Password given to every dedicated user (>= 8 chars per createUserSchema). */ +const ISOLATED_USER_PASSWORD = 'e2e-isolated-pw-123!'; + +/** Opt-in configuration for a dedicated user. */ +export interface IsolatedUserSpec { + /** + * Short slug used in the generated e-mail address, for readability in failure + * output. Keep the string "admin" out of it — `search-users.spec.ts` asserts + * that every row matching a search for "Admin" has "admin" in its name column. + */ + emailPrefix: string; + /** Defaults to 'E2E Isolated User'. Must not contain "admin" (see above). */ + displayName?: string; + /** Defaults to 'member'. Only set 'admin' for an actual admin-gated dependency. */ + role?: 'admin' | 'member'; + /** + * `locale` preference to seed on the dedicated user. Defaults to 'en' so specs + * asserting English strings do not depend on the CI browser's default locale. + */ + locale?: 'en' | 'de' | 'system'; +} + +/** Identity of the dedicated user backing the current test's browser context. */ +export interface IsolatedUserSession { + id: string; + email: string; + password: string; + storageState: StorageStateValue; +} + +type StorageStateValue = Awaited>; + +/** + * Creates a dedicated user, logs it in, seeds its `locale` preference and returns + * both its identity and a storage state carrying its session cookie. + * + * `baseURL` is passed in explicitly: Playwright only merges the project's context + * options (including `baseURL`) into contexts created *inside a test*, and a + * worker-scoped fixture runs before that instrumentation is installed. + */ +async function provisionIsolatedUser( + playwright: PlaywrightWorkerArgs['playwright'], + baseURL: string, + spec: IsolatedUserSpec, + uniqueSuffix: string, +): Promise<{ session: IsolatedUserSession; dispose: () => Promise }> { + const adminApi = await playwright.request.newContext({ + baseURL, + storageState: ADMIN_STORAGE_STATE, + }); + + const email = `${spec.emailPrefix}-${uniqueSuffix}@e2e-test.local`; + let userId: string | null = null; + + try { + const createResponse = await adminApi.post(API.users, { + data: { + email, + displayName: spec.displayName ?? 'E2E Isolated User', + password: ISOLATED_USER_PASSWORD, + role: spec.role ?? 'member', + }, + }); + if (!createResponse.ok()) { + throw new Error( + `isolatedUser: POST ${API.users} for "${email}" failed with ${createResponse.status()}: ${await createResponse.text()}`, + ); + } + const created = (await createResponse.json()) as { user: { id: string } }; + userId = created.user.id; + + const userApi = await playwright.request.newContext({ baseURL }); + let storageState: StorageStateValue; + try { + const loginResponse = await userApi.post(API.login, { + data: { email, password: ISOLATED_USER_PASSWORD }, + }); + if (!loginResponse.ok()) { + throw new Error( + `isolatedUser: login as "${email}" failed with ${loginResponse.status()}: ${await loginResponse.text()}`, + ); + } + const localeResponse = await userApi.patch('/api/users/me/preferences', { + data: { key: 'locale', value: spec.locale ?? 'en' }, + }); + if (!localeResponse.ok()) { + throw new Error( + `isolatedUser: seeding locale for "${email}" failed with ${localeResponse.status()}`, + ); + } + storageState = await userApi.storageState(); + } finally { + // The session lives on the server; disposing the client is safe. + await userApi.dispose(); + } + + return { + session: { id: userId, email, password: ISOLATED_USER_PASSWORD, storageState }, + dispose: async () => { + // Soft-delete (deactivate) the user and drop its sessions, then release + // the admin client. Failures are non-fatal: the account is never reused. + await adminApi.delete(`${API.users}/${userId}`); + await adminApi.dispose(); + }, + }; + } catch (error) { + if (userId) await adminApi.delete(`${API.users}/${userId}`); + await adminApi.dispose(); + throw error; + } +} + +/** + * Base URL of the app under test. Mirrors playwright.config.ts's + * `use.baseURL`; `containers/setup.ts` sets APP_BASE_URL to the proxy URL in + * globalSetup, which worker processes inherit. Needed because a worker-scoped + * fixture cannot depend on the test-scoped `baseURL` fixture. + */ +function resolveBaseURL(): string { + return process.env.APP_BASE_URL || 'http://localhost:3000'; +} + +export const test = authTest.extend< + { + isolatedUserPerTest: IsolatedUserSpec | null; + isolatedUserSession: IsolatedUserSession | null; + isolatedUserGuard: void; + }, + { + isolatedUserPerWorker: IsolatedUserSpec | null; + workerIsolatedUserSession: IsolatedUserSession | null; + } +>({ + // ── Options ─────────────────────────────────────────────────────────────── + isolatedUserPerWorker: [null, { scope: 'worker', option: true }], + isolatedUserPerTest: [null, { option: true }], + + // ── One dedicated user per worker (file-scope opt-in) ────────────────────── + workerIsolatedUserSession: [ + async ({ playwright, isolatedUserPerWorker }, use, workerInfo) => { + if (!isolatedUserPerWorker) { + await use(null); + return; + } + const { session, dispose } = await provisionIsolatedUser( + playwright, + resolveBaseURL(), + isolatedUserPerWorker, + `${workerInfo.project.name}-w${workerInfo.workerIndex}-${Date.now()}`, + ); + try { + await use(session); + } finally { + await dispose(); + } + }, + { scope: 'worker' }, + ], + + // ── The session backing this test's browser context ─────────────────────── + isolatedUserSession: async ( + { playwright, baseURL, isolatedUserPerTest, workerIsolatedUserSession }, + use, + testInfo, + ) => { + if (isolatedUserPerTest && workerIsolatedUserSession) { + throw new Error( + 'isolatedUser: set either isolatedUserPerWorker (file scope) or isolatedUserPerTest (file/describe scope), not both.', + ); + } + if (!isolatedUserPerTest) { + await use(workerIsolatedUserSession); + return; + } + const { session, dispose } = await provisionIsolatedUser( + playwright, + baseURL ?? resolveBaseURL(), + isolatedUserPerTest, + `${testInfo.project.name}-w${testInfo.workerIndex}-${Date.now()}`, + ); + try { + await use(session); + } finally { + await dispose(); + } + }, + + // ── Point the built-in context/page fixtures at that session ────────────── + // + // The `testInfo.project.use.storageState` fallback is load-bearing, not + // defensive: playwright.config.ts sets `storageState` per project, and once the + // option is overridden here the config value is no longer consulted — without it, + // non-opted-in tests in an importing file would lose admin auth entirely. + // Deliberately resolves to `undefined` rather than forcing ADMIN_STORAGE_STATE if + // a project ever sets no storageState, so a future unauthenticated project keeps + // its intended state. + storageState: async ({ isolatedUserSession }, use, testInfo) => { + await use(isolatedUserSession?.storageState ?? testInfo.project.use.storageState); + }, + + // ── Fail loudly if the override above ever stops taking effect ──────────── + // + // The whole point of this module is that `page` belongs to the dedicated user. + // If that silently regressed (e.g. a Playwright change to how the `storageState` + // option feeds `_combinedContextOptions`), the specs would quietly go back to + // mutating the shared admin and the collisions this fixture prevents would + // return unnoticed — every test would still pass. One cheap request per test + // turns that into an immediate, explicit failure. + // + // Two limits, both deliberate: + // (a) It validates the `page` fixture only. A context a test builds itself via + // `browser.newContext()` is outside its reach — such a context inherits + // this override unless it passes an explicit `storageState` (as + // invoices.spec.ts's Dark mode describe does, which is why that + // non-converted describe is unaffected). + // (b) It is `auto` and depends on `page`, so every test in an importing file + // gets a browser context — including one a `beforeEach` immediately + // `test.skip()`s, since fixtures resolve before hooks run. + isolatedUserGuard: [ + async ({ page, isolatedUserSession }, use) => { + if (isolatedUserSession) { + const response = await page.request.get(API.authMe); + const body = (await response.json()) as { user: { email: string } | null }; + if (body.user?.email !== isolatedUserSession.email) { + throw new Error( + `isolatedUser: expected the test's browser context to be authenticated as "${isolatedUserSession.email}", but /api/auth/me reports "${body.user?.email ?? 'nobody'}". The storageState override is not taking effect — preference writes would hit the shared admin user.`, + ); + } + } + await use(); + }, + { auto: true }, + ], +}); + +export { expect } from '@playwright/test'; diff --git a/e2e/tests/diary/diary-uat-fixes.spec.ts b/e2e/tests/diary/diary-uat-fixes.spec.ts index 591380a96..38f5918f6 100644 --- a/e2e/tests/diary/diary-uat-fixes.spec.ts +++ b/e2e/tests/diary/diary-uat-fixes.spec.ts @@ -21,7 +21,7 @@ * 8. Diary detail page has no print button */ -import { test, expect } from '../../fixtures/auth.js'; +import { test, expect } from '../../fixtures/isolatedUser.js'; import { DiaryPage, DIARY_ROUTE } from '../../pages/DiaryPage.js'; import { DiaryEntryDetailPage } from '../../pages/DiaryEntryDetailPage.js'; import { DiaryEntryCreatePage } from '../../pages/DiaryEntryCreatePage.js'; @@ -82,6 +82,16 @@ test.describe('Back button navigates to /diary (Scenario 2)', { tag: '@responsiv // Scenario 3: Dashboard "Recent Diary" card is visible // ───────────────────────────────────────────────────────────────────────────── test.describe('Dashboard Recent Diary card (Scenario 3)', { tag: '@responsive' }, () => { + // This test resets `dashboard.hiddenCards` so the Recent Diary card is guaranteed + // to render. On the shared admin that reset collided with dashboard.spec.ts, which + // resets the same key for its own tests and asserts card visibility against it + // (Issue #1957). Run it as a dedicated user instead, so the row is unreachable by + // any other test. Per-test rather than per-worker because a worker-scoped option + // cannot be set inside a describe, and only 2 of this file's 8 tests need it. + // Nothing here is admin-gated: the test only reads the dashboard with + // `/api/diary-entries` mocked. + test.use({ isolatedUserPerTest: { emailPrefix: 'diary-dash', displayName: 'E2E Diary User' } }); + test('Dashboard page shows a "Recent Diary" card', { tag: '@smoke' }, async ({ page }) => { const dashboardPage = new DashboardPage(page); @@ -102,7 +112,10 @@ test.describe('Dashboard Recent Diary card (Scenario 3)', { tag: '@responsive' } }); try { - // Reset hidden cards to ensure "Recent Diary" is visible + // Reset hidden cards on this test's dedicated user to ensure "Recent Diary" is + // visible. A freshly created user has no preference rows at all, so this is + // belt-and-braces rather than load-bearing — but it keeps the precondition + // explicit and would still hold if a dismiss step were added here later. await page.request.patch('/api/users/me/preferences', { data: { key: 'dashboard.hiddenCards', value: '[]' }, }); @@ -322,6 +335,9 @@ test.describe('Create navigates to detail page (Scenario 6)', { tag: '@responsiv // Scenario 7: Dashboard diary card "View All" navigates to /diary // ───────────────────────────────────────────────────────────────────────────── test.describe('Recent Diary "View All" link (Scenario 7)', { tag: '@responsive' }, () => { + // Same `dashboard.hiddenCards` collision as Scenario 3 — see the comment there. + test.use({ isolatedUserPerTest: { emailPrefix: 'diary-dash', displayName: 'E2E Diary User' } }); + test('Clicking "View All" in the Recent Diary card navigates to /diary', async ({ page }) => { const dashboardPage = new DashboardPage(page); @@ -360,7 +376,7 @@ test.describe('Recent Diary "View All" link (Scenario 7)', { tag: '@responsive' }); try { - // Reset hidden cards + // Reset hidden cards on this test's dedicated user (see Scenario 3's note) await page.request.patch('/api/users/me/preferences', { data: { key: 'dashboard.hiddenCards', value: '[]' }, }); diff --git a/e2e/tests/household-items/no-area-filter.spec.ts b/e2e/tests/household-items/no-area-filter.spec.ts index c120da295..1c81ad67b 100644 --- a/e2e/tests/household-items/no-area-filter.spec.ts +++ b/e2e/tests/household-items/no-area-filter.spec.ts @@ -15,7 +15,9 @@ * 1. Sentinel renders at top of popover (desktop/tablet; mobile skip) * 2. ?areaId=__none__ shows only unassigned items * 3. ?areaId=__none__, shows union of unassigned + named area items - * 4. Empty state when no unassigned items exist and ?areaId=__none__ applied + * 4. Empty state when none of this test's own items are unassigned and + * ?areaId=__none__ applied — see the note on Scenario 4 below for why the + * assertion is scoped with `&q=` instead of assuming a suite-global state. */ import { test, expect } from '../../fixtures/auth.js'; @@ -183,15 +185,41 @@ test.describe( ); // ───────────────────────────────────────────────────────────────────────────── -// Scenario 4: Empty state when no unassigned items and ?areaId=__none__ applied +// Scenario 4: Empty state when none of this test's items are unassigned and +// ?areaId=__none__ applied +// +// This assertion must NOT be phrased as "no unassigned household item exists +// anywhere", which is what it used to do: `?areaId=__none__` alone lists every +// area-less item in the shared DB, and 48 of the suite's 62 +// `createHouseholdItemViaApi()` calls — spread over 12 spec files — pass no areaId +// (area-filter.spec.ts:185, :318 and :377 each hold one for the length of a long +// scenario). Under `fullyParallel: true` a single foreign area-less item makes the +// empty state unreachable, so the test only ever passed while no such spec happened +// to share its shard — it broke the moment #1957's worker-hash change moved this +// file from shard 15 into shard 14, whose other members are area-filter.spec.ts, +// household-items-list.spec.ts, household-item-create/-detail/-edit.spec.ts. +// +// The sibling `e2e/tests/work-items/no-area-filter.spec.ts:223` already solves this +// the same way and has been green in shards 7/12/16 while co-resident with five +// area-less work-item creators — this file was simply never given the same +// treatment. +// +// Fix: AND the sentinel filter with a `q=` search for this scenario's own name +// token (`useTableState` hydrates `q` from the URL and householdItemService ANDs +// it with the areaId condition), so only items this test created can satisfy the +// list. `q` also counts towards DataTable's `hasActiveFilters`, so the filtered +// empty-state message and Clear Filters button are still the correct expectations. +// A positive control runs first: the same `q` without the sentinel must list the +// area-assigned item, which rules out the empty state passing vacuously because +// the search matched nothing at all. // ───────────────────────────────────────────────────────────────────────────── test.describe( - '?areaId=__none__ shows filtered empty state when no unassigned household items exist (Scenario 4)', + '?areaId=__none__ shows filtered empty state when no matching household item is unassigned (Scenario 4)', { tag: '@responsive' }, () => { test.describe.configure({ timeout: 90_000 }); - test('Empty state with Clear Filters button when all items are area-assigned', async ({ + test('Empty state with Clear Filters button when the only matching item is area-assigned', async ({ page, testPrefix, }) => { @@ -199,16 +227,43 @@ test.describe( const areaIds: string[] = []; const itemIds: string[] = []; - const areaName = `${testPrefix} HI NoArea Sc4 Area`; - const itemAssignedName = `${testPrefix} HI NoArea Sc4 Assigned`; + // Search token that scopes every assertion below to this test's own data. + // Unique per worker+project via testPrefix, and no other test uses this + // scenario suffix. + const scopeToken = `${testPrefix} HI NoArea Sc4`; + const areaName = `${scopeToken} Area`; + const itemAssignedName = `${scopeToken} Assigned`; try { const areaId = await createAreaViaApi(page, { name: areaName }); areaIds.push(areaId); itemIds.push(await createHouseholdItemViaApi(page, { name: itemAssignedName, areaId })); - await page.goto(`${HOUSEHOLD_ITEMS_ROUTE}?areaId=__none__`); + // Positive control: the search token alone must find the area-assigned item. + // Without this, an empty result below would be indistinguishable from "the + // search matched nothing", and the empty-state assertion would pass even if + // the sentinel filter did nothing. + await page.goto(`${HOUSEHOLD_ITEMS_ROUTE}?q=${encodeURIComponent(scopeToken)}`); await listPage.heading.waitFor({ state: 'visible' }); + await listPage.waitForLoaded(); + await expect(async () => { + const names = await listPage.getItemNames(); + expect(names).toContain(itemAssignedName); + }).toPass({ timeout: 30_000 }); + + // Now add the sentinel: the one matching item has an area, so the filtered + // result must be empty regardless of what other specs are doing concurrently. + await page.goto( + `${HOUSEHOLD_ITEMS_ROUTE}?areaId=__none__&q=${encodeURIComponent(scopeToken)}`, + ); + await listPage.heading.waitFor({ state: 'visible' }); + + // Both filters must survive the SPA's URL round-trip (handleStateChange + // rewrites the query string from table state), otherwise the empty state + // below could be caused by a different filter set than the one under test. + const url = new URL(page.url()); + expect(url.searchParams.get('areaId')).toBe('__none__'); + expect(url.searchParams.get('q')).toBe(scopeToken); // Wait for the filter empty state await expect(async () => { diff --git a/e2e/tests/i18n/i18n-categories.spec.ts b/e2e/tests/i18n/i18n-categories.spec.ts index 92182eed3..205bd24b0 100644 --- a/e2e/tests/i18n/i18n-categories.spec.ts +++ b/e2e/tests/i18n/i18n-categories.spec.ts @@ -9,7 +9,11 @@ * Test strategy: * - Each test creates a dedicated local user so that PATCH /api/users/me/preferences * never mutates the shared TEST_ADMIN user, eliminating locale-state leakage across - * parallel workers. + * parallel workers. (This file is listed in the preference-write audit in + * e2e/fixtures/isolatedUser.ts, Issue #1957, as already-isolated. The hand-rolled + * helper below predates that fixture and is equivalent to + * `test.use({ isolatedUserPerTest: ... })`; it is left as-is because it also needs + * to write localStorage on the scoped page before the first navigation.) * - A fresh browser context (no storageState) is created per test, logged in as the * dedicated user, and closed in a finally block. * - Scoped to desktop only — language state changes involve localStorage + API diff --git a/e2e/tests/i18n/i18n.spec.ts b/e2e/tests/i18n/i18n.spec.ts index 24e587d36..dfdec5775 100644 --- a/e2e/tests/i18n/i18n.spec.ts +++ b/e2e/tests/i18n/i18n.spec.ts @@ -16,28 +16,38 @@ * Scoped to desktop only: language switching involves a form that can be unreliable * on WebKit tablet — and language correctness is not viewport-specific. * - * Serial mode (file scope): every test here mutates the SAME row — - * user_preferences(locale) of the shared TEST_ADMIN user. Under `fullyParallel: true` - * two of these tests run concurrently in different workers, so one test's - * `setLanguage()`/`resetToEnglish()` PATCH lands inside another's assertions. That is - * not merely "stale data": LocaleContext.syncWithServer treats the server value as - * authoritative, so it applies the other test's locale AND deletes the victim's - * 'locale' localStorage key, permanently flipping the victim's UI language mid-test. - * Observed in CI run 30790367863 shard 4, where "Key page headings render in German" - * asserted 'Projekt' successfully at 06:35:35.49 and then found an English sidebar - * ('Main navigation', 'Schedule') at 06:35:36.11 — the window in which the - * concurrently running "Language can be switched back to English from German" - * (06:35:34.41–35.88, worker 1) PATCHed locale='en'. - * There is only one admin user, so `testPrefix` cannot isolate this; serial mode is - * the established remedy for shared-admin-mutating specs (cf. change-password.spec.ts, - * edit-user.spec.ts). It must be file-scoped, not describe-scoped: the interference - * observed above crossed describe boundaries. + * Preference isolation (Issue #1957): every test here mutates the `locale` + * preference row of whichever user it is authenticated as. That used to be the one + * shared TEST_ADMIN user, which under `fullyParallel: true` made this file both a + * victim and a cause of cross-file corruption — LocaleContext.syncWithServer treats + * the server value as authoritative, so a concurrent PATCH from another worker + * applies its locale AND deletes the victim's 'locale' localStorage key, flipping + * the victim's UI language mid-test. Observed in CI run 30790367863 shard 4, where + * "Key page headings render in German" asserted 'Projekt' successfully at + * 06:35:35.49 and then found an English sidebar ('Main navigation', 'Schedule') at + * 06:35:36.11 — the window in which the concurrently running "Language can be + * switched back to English from German" (06:35:34.41–35.88, worker 1) PATCHed + * locale='en'. `dashboard.spec.ts` was the mirror-image victim: its top-level + * `beforeEach` PATCHed locale='en' on the same shared row. + * + * The isolation mechanism is now `isolatedUserPerWorker` (see + * e2e/fixtures/isolatedUser.ts): every test runs as a dedicated user, so the row + * `setLanguage()` writes is unreachable by any other test in the suite. Serial mode + * is KEPT, but only as defence in depth: it no longer provides isolation (it never + * could across files), it just avoids running two slow German cold-start tests + * concurrently on a 2-vCPU CI runner. `resetToEnglish()` in afterEach is also kept + * and is still load-bearing — a worker's dedicated user is shared by that worker's + * sequentially-executed tests, so locale state must not carry over between them. */ import type { Page } from '@playwright/test'; -import { test, expect } from '../../fixtures/auth.js'; +import { test, expect } from '../../fixtures/isolatedUser.js'; import { ROUTES } from '../../fixtures/testData.js'; +test.use({ + isolatedUserPerWorker: { emailPrefix: 'i18n-switch', displayName: 'E2E i18n User' }, +}); + test.describe.configure({ mode: 'serial' }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/e2e/tests/invoices/invoices.spec.ts b/e2e/tests/invoices/invoices.spec.ts index c133d3855..a9ca9c457 100644 --- a/e2e/tests/invoices/invoices.spec.ts +++ b/e2e/tests/invoices/invoices.spec.ts @@ -19,7 +19,7 @@ * - Dark mode */ -import { test, expect } from '../../fixtures/auth.js'; +import { test, expect } from '../../fixtures/isolatedUser.js'; import type { Page } from '@playwright/test'; import { InvoicesPage } from '../../pages/InvoicesPage.js'; import { InvoiceDetailPage } from '../../pages/InvoiceDetailPage.js'; @@ -838,6 +838,19 @@ test.describe('Dark mode', () => { // ───────────────────────────────────────────────────────────────────────────── test.describe('"Effective Amount" column (Issue #1876)', { tag: '@responsive' }, () => { + // This is the only test in the suite that asserts against `table.invoices.columns`, + // a per-user singleton preference that `useColumnPreferences` writes (debounced) for + // every visitor of the invoices table. On the shared admin, any concurrently running + // test that renders that table could move the column set under this test's feet, and + // this test's own DELETEs could equally disturb others (Issue #1957). Run it as a + // dedicated user so the row belongs to nobody else. Per-test rather than per-worker + // because a worker-scoped option cannot be set inside a describe. Nothing here is + // admin-gated: the invoices list/detail pages and vendor/invoice/work-item/ + // budget-source creation have no role checks. + test.use({ + isolatedUserPerTest: { emailPrefix: 'inv-columns', displayName: 'E2E Invoices User' }, + }); + test('Toggling "Effective Amount" shows a deposit/refund-aware value distinct from "Remaining Amount"', async ({ page, testPrefix, @@ -908,9 +921,10 @@ test.describe('"Effective Amount" column (Issue #1876)', { tag: '@responsive' }, // Effective Amount = 1000 − 150 (paid refund) = 850 // Reset the "table.invoices.columns" preference before asserting the - // hidden-by-default baseline — a prior run's debounced save (or this test's - // own retry) can otherwise leave "Effective Amount"/"Remaining Amount" - // already-visible for this account before we even start. + // hidden-by-default baseline. Since this test now runs as a freshly created + // dedicated user (see test.use above), the row cannot pre-exist — not even on a + // retry, which provisions a new user. Kept as an explicit precondition so the + // baseline assertion below cannot silently depend on account history. await page.request.delete('/api/users/me/preferences/table.invoices.columns'); await invoicesPage.goto(); @@ -964,10 +978,10 @@ test.describe('"Effective Amount" column (Issue #1876)', { tag: '@responsive' }, } finally { // Column visibility/order is a per-user server-side SINGLETON preference // (`table.invoices.columns`, see useColumnPreferences), not a per-test entity. - // Reset it so this test's "enable Remaining/Effective Amount" toggles never - // leak into a retry of this same test (the debounced save can persist after a - // failed assertion) or into any other invoices test running afterward against - // the same account. DELETE 404s if no preference was ever saved — fine either way. + // The dedicated user is deactivated in fixture teardown, so these toggles can no + // longer leak anywhere; the reset is kept so the intent survives if this describe + // is ever switched back to a shared account. + // DELETE 404s if no preference was ever saved — fine either way. await page.request.delete('/api/users/me/preferences/table.invoices.columns'); if (vendorId) await deleteVendorViaApi(page, vendorId); if (workItemId) await deleteWorkItemViaApi(page, workItemId); diff --git a/e2e/tests/navigation/dashboard.spec.ts b/e2e/tests/navigation/dashboard.spec.ts index 066912537..f14f95a63 100644 --- a/e2e/tests/navigation/dashboard.spec.ts +++ b/e2e/tests/navigation/dashboard.spec.ts @@ -17,17 +17,45 @@ * 11. No horizontal scroll on current viewport */ -import type { Browser, BrowserContext, Page } from '@playwright/test'; -import { test, expect } from '../../fixtures/auth.js'; +import { test, expect } from '../../fixtures/isolatedUser.js'; import { DashboardPage, DASHBOARD_ROUTE, CARD_TITLES } from '../../pages/DashboardPage.js'; -import { createLocalUserViaApi, deleteUserViaApi } from '../../fixtures/apiHelpers.js'; // ───────────────────────────────────────────────────────────────────────────── -// Global setup: reset dashboard preferences before every test to prevent -// state leaking from dismiss tests (Issue 1: server-side preference persistence) +// Preference isolation (Issue #1957) +// +// Every test in this file depends on two per-user preference rows: +// `dashboard.hiddenCards` (which cards render) and `locale` (English card +// headings). Scenario 6/7 additionally WRITE `dashboard.hiddenCards` by clicking +// dismiss/re-enable, and the reset hook below writes both keys. +// +// Under `fullyParallel: true` all of that used to happen on the one shared admin +// user (test-results/.auth/admin.json), so any other spec touching those keys — +// diary-uat-fixes.spec.ts resets `dashboard.hiddenCards`, i18n.spec.ts flips +// `locale` — could land a write inside a test's assertion window from another +// worker, and this file's own reset hook could wipe out Scenario 6's dismissed +// state mid-test. `mode: 'serial'` cannot fix that: it only orders a file against +// itself. +// +// This file therefore runs against a dedicated user (see e2e/fixtures/isolatedUser.ts +// for the full mechanism and the audit of every preference-writing spec). One +// dedicated user per worker is enough: a Playwright worker executes one test at a +// time and no other worker shares the user, so no concurrent write to those rows +// is possible from anywhere in the suite. Sequential carry-over inside one worker +// is still possible (Scenario 6 leaves a card hidden for whatever test runs next +// in that worker), which is exactly what the reset hook below handles. +// +// The write/read path itself is correctly ordered and durable (dismissCard() awaits +// the PATCH response; preferencesService.upsertPreference() commits synchronously) — +// this was a test-isolation gap, never a product bug. // ───────────────────────────────────────────────────────────────────────────── +test.use({ + isolatedUserPerWorker: { emailPrefix: 'dash', displayName: 'E2E Dashboard User' }, +}); + test.beforeEach(async ({ page }) => { + // Reset this worker's dedicated user back to "no cards hidden" so a preceding + // dismiss test in the same worker cannot leak into the next one. const resp = await page.request.patch('/api/users/me/preferences', { data: { key: 'dashboard.hiddenCards', value: '[]' }, }); @@ -35,9 +63,10 @@ test.beforeEach(async ({ page }) => { // from a prior test, causing downstream dismiss tests to fail. expect(resp.ok(), `beforeEach: preference reset failed with ${resp.status()}`).toBeTruthy(); - // Also reset the locale preference to English. If an i18n test in the same shard - // left the locale as 'de', the dashboard would render with German card headings - // (e.g., "Schnellaktionen" instead of "Quick Actions"), causing locator failures. + // Force English regardless of the CI browser's default locale — every assertion + // in this file matches English headings (e.g. "Quick Actions", not + // "Schnellaktionen"). The dedicated user is seeded with locale='en' on creation; + // this re-asserts it in case a test in this worker changed it. await page.request.patch('/api/users/me/preferences', { data: { key: 'locale', value: 'en' }, }); @@ -480,82 +509,22 @@ test.describe('Quick Actions card (Scenario 5)', { tag: '@responsive' }, () => { }); }); -// ───────────────────────────────────────────────────────────────────────────── -// Isolated-user helper for Scenario 6 & 7 (card dismiss / re-enable) -// -// Root cause of the intermittent "Dismissed card stays hidden after page reload" -// failure (PR #1935, shard 6): `dashboard.hiddenCards` is a single preference row -// keyed by userId. Every test in this file authenticates as the one shared admin -// user (test-results/.auth/admin.json), and playwright.config.ts runs with -// `fullyParallel: true`, so unrelated tests/describe-blocks in this same file -// (each of which runs the top-level `beforeEach` that PATCHes -// `dashboard.hiddenCards` back to "[]" on that same shared admin user) can be -// scheduled onto a different worker and land their reset in the narrow window -// between this test's own dismiss-PATCH and its post-reload GET — wiping out the -// very state under test. The previous `mode: 'serial'` guard only serialized -// Scenario 6's own two tests against each other; it could not stop the other -// ~30 tests in this file from touching the same shared-admin preference row -// concurrently, since they have no knowledge of Scenario 6's serial group. -// -// The write/read path itself (upsertPreference -> synchronous better-sqlite3 -// write -> 200 response -> dismissCard() awaits that response -> reload -> GET) -// is correctly ordered and durable — there is no persistence bug in production -// code here (see `dismissCard()` in DashboardPage.ts and -// `preferencesService.upsertPreference()`, both of which resolve/commit before -// the next step runs). This was a test-isolation gap, not a product bug. -// -// Fix: give each dismiss/re-enable test its own dedicated user + browser -// context, mirroring the established pattern in -// e2e/tests/i18n/i18n-categories.spec.ts and e2e/tests/profile/change-password.spec.ts. -// A dedicated user's `dashboard.hiddenCards` row can never be touched by any -// other concurrently running test, so the persistence contract can be verified -// without racing the rest of the suite. `ON DELETE CASCADE` on -// user_preferences.user_id means deleting the user also removes its -// preferences — no separate reset/cleanup PATCH is needed. -// ───────────────────────────────────────────────────────────────────────────── - -const DASHBOARD_ISOLATED_USER_PASSWORD = 'e2e-dashboard-pw-123!'; - -async function loginAsIsolatedDashboardUser( - browser: Browser, - adminPage: Page, - testPrefix: string, -): Promise<{ context: BrowserContext; page: Page; userId: string }> { - const email = `dash-${testPrefix}-${Date.now()}@e2e-test.local`; - const user = await createLocalUserViaApi(adminPage, { - email, - displayName: 'E2E Dashboard User', - password: DASHBOARD_ISOLATED_USER_PASSWORD, - }); - - const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }); - const scopedPage = await context.newPage(); - await scopedPage.request.post('/api/auth/login', { - data: { email, password: DASHBOARD_ISOLATED_USER_PASSWORD }, - }); - // Force English regardless of the CI browser's default locale — every assertion - // in these scenarios matches English card titles (e.g. "Quick Actions"). - await scopedPage.request.patch('/api/users/me/preferences', { - data: { key: 'locale', value: 'en' }, - }); - - return { context, page: scopedPage, userId: user.id }; -} - // ───────────────────────────────────────────────────────────────────────────── // Scenario 6: Card dismiss — clicking dismiss hides card; reload keeps it hidden +// +// These two tests are the only ones in this file that WRITE +// `dashboard.hiddenCards`. They previously provisioned a dedicated user inline +// (PR #1956) to survive the reset hook of a sibling test running in another +// worker; that is now handled file-wide by `isolatedUserPerWorker` above, so the +// plain `page` fixture is already the dedicated user's page and the inline helper +// is gone (Issue #1957). // ───────────────────────────────────────────────────────────────────────────── test.describe('Card dismiss (Scenario 6)', () => { - test('Dismissing a card hides it from the dashboard', async ({ page, browser, testPrefix }) => { - const { - context, - page: scopedPage, - userId, - } = await loginAsIsolatedDashboardUser(browser, page, testPrefix); - const dashboardPage = new DashboardPage(scopedPage); + test('Dismissing a card hides it from the dashboard', async ({ page }) => { + const dashboardPage = new DashboardPage(page); - await interceptDashboardApis(scopedPage); + await interceptDashboardApis(page); try { await dashboardPage.goto(); @@ -572,21 +541,14 @@ test.describe('Card dismiss (Scenario 6)', () => { const afterDismiss = dashboardPage.card('Quick Actions'); await expect(afterDismiss).toHaveCount(0); } finally { - await uninterceptDashboardApis(scopedPage); - await context.close(); - await deleteUserViaApi(page, userId); + await uninterceptDashboardApis(page); } }); - test('Dismissed card stays hidden after page reload', async ({ page, browser, testPrefix }) => { - const { - context, - page: scopedPage, - userId, - } = await loginAsIsolatedDashboardUser(browser, page, testPrefix); - const dashboardPage = new DashboardPage(scopedPage); + test('Dismissed card stays hidden after page reload', async ({ page }) => { + const dashboardPage = new DashboardPage(page); - await interceptDashboardApis(scopedPage); + await interceptDashboardApis(page); try { await dashboardPage.goto(); @@ -601,7 +563,7 @@ test.describe('Card dismiss (Scenario 6)', () => { // Reload the page. Use navigationTimeout (10s) for the heading waitFor since the SPA // must fully initialize after a hard reload before the Dashboard heading appears. - await scopedPage.reload(); + await page.reload(); await dashboardPage.heading.waitFor({ state: 'visible', timeout: 10000 }); // On page load, two contexts fetch preferences independently: @@ -611,14 +573,12 @@ test.describe('Card dismiss (Scenario 6)', () => { // // waitForLoadState('networkidle') ensures all pending network requests (including both // preference fetches) have completed and React has finished re-rendering before we assert. - await scopedPage.waitForLoadState('networkidle', { timeout: 15000 }); + await page.waitForLoadState('networkidle', { timeout: 15000 }); // The Quick Actions card must be absent — usePreferences applied hiddenCards: ["quick-actions"] await expect(dashboardPage.card('Quick Actions')).toHaveCount(0); } finally { - await uninterceptDashboardApis(scopedPage); - await context.close(); - await deleteUserViaApi(page, userId); + await uninterceptDashboardApis(page); } }); }); @@ -628,19 +588,10 @@ test.describe('Card dismiss (Scenario 6)', () => { // ───────────────────────────────────────────────────────────────────────────── test.describe('Card re-enable (Scenario 7)', () => { - test('Customize button appears when a card is dismissed', async ({ - page, - browser, - testPrefix, - }) => { - const { - context, - page: scopedPage, - userId, - } = await loginAsIsolatedDashboardUser(browser, page, testPrefix); - const dashboardPage = new DashboardPage(scopedPage); + test('Customize button appears when a card is dismissed', async ({ page }) => { + const dashboardPage = new DashboardPage(page); - await interceptDashboardApis(scopedPage); + await interceptDashboardApis(page); try { await dashboardPage.goto(); @@ -665,25 +616,14 @@ test.describe('Card re-enable (Scenario 7)', () => { // Customize button should now appear await expect(dashboardPage.customizeButton).toBeVisible(); } finally { - await uninterceptDashboardApis(scopedPage); - await context.close(); - await deleteUserViaApi(page, userId); + await uninterceptDashboardApis(page); } }); - test('Customize dropdown lists dismissed card and clicking re-enables it', async ({ - page, - browser, - testPrefix, - }) => { - const { - context, - page: scopedPage, - userId, - } = await loginAsIsolatedDashboardUser(browser, page, testPrefix); - const dashboardPage = new DashboardPage(scopedPage); + test('Customize dropdown lists dismissed card and clicking re-enables it', async ({ page }) => { + const dashboardPage = new DashboardPage(page); - await interceptDashboardApis(scopedPage); + await interceptDashboardApis(page); try { await dashboardPage.goto(); @@ -712,9 +652,7 @@ test.describe('Card re-enable (Scenario 7)', () => { // The button is removed from the DOM entirely when all cards are visible. await expect(dashboardPage.customizeButton).not.toBeVisible(); } finally { - await uninterceptDashboardApis(scopedPage); - await context.close(); - await deleteUserViaApi(page, userId); + await uninterceptDashboardApis(page); } }); });