From 950f6c4516fd6beda73d8f140c7eb619ea7fa147 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 13:23:06 +0200 Subject: [PATCH 1/2] test(e2e): isolate preference-mutating specs from the shared admin user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every spec authenticates as one shared admin, and user_preferences rows are keyed by user id, so under fullyParallel a concurrent test's write lands inside another test's assertions. LocaleContext.syncWithServer treats the server as authoritative, so the victim's UI flips language mid-test and its localStorage override is deleted by the same sync. Adds e2e/fixtures/isolatedUser.ts, which overrides Playwright's storageState option so the plain `page` fixture is already a dedicated user — keeping trace/video/screenshot instrumentation that a hand-rolled browser.newContext() loses, and requiring no changes to test bodies. An auto guard fixture asserts /api/auth/me reports the dedicated user, so a silent regression to shared-admin writes fails loudly. Converted dashboard.spec.ts and i18n.spec.ts (per-worker), plus the colliding describes in diary-uat-fixes.spec.ts and invoices.spec.ts (per-test). Scenario 6/7's inline helper is now redundant and removed. i18n.spec.ts keeps serial mode as defence in depth, no longer as its isolation mechanism. No spec needed admin-role treatment: requireRole('admin') covers only /api/users mutations and /api/backups/*, none of which the converted tests touch. Fixes #1957 Co-Authored-By: Claude e2e-test-engineer --- .../agent-memory/e2e-test-engineer/MEMORY.md | 3 +- .../e2e-parallel-isolation.md | 7 + .../isolated-user-fixture.md | 67 ++++ e2e/fixtures/isolatedUser.ts | 376 ++++++++++++++++++ e2e/tests/diary/diary-uat-fixes.spec.ts | 22 +- e2e/tests/i18n/i18n-categories.spec.ts | 6 +- e2e/tests/i18n/i18n.spec.ts | 44 +- e2e/tests/invoices/invoices.spec.ts | 30 +- e2e/tests/navigation/dashboard.spec.ts | 186 +++------ 9 files changed, 587 insertions(+), 154 deletions(-) create mode 100644 .claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md create mode 100644 e2e/fixtures/isolatedUser.ts diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index acde9174d..a30f30a7f 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); login rate limit 20/15min, soft user delete, 100-row user table, 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/isolated-user-fixture.md b/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md new file mode 100644 index 000000000..f4e6f5bd0 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md @@ -0,0 +1,67 @@ +--- +name: isolated-user-fixture +description: e2e/fixtures/isolatedUser.ts — how to give a preference-mutating spec its own user; plus the hard server-side limits (login rate limit, soft user delete, 100-row user table) 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. +- `/settings/users` renders **100 rows per page** with `sortBy: null` (insertion order), and + `edit-user.spec.ts` / `deactivate-user.spec.ts` find their user by scanning the rendered + page. Piling up deactivated E2E users past 100 breaks those unrelated specs. Converting all + ~34 dashboard tests per-test would have added ~85 users/run; per-worker adds ~15. +- 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). +- `_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/e2e/fixtures/isolatedUser.ts b/e2e/fixtures/isolatedUser.ts new file mode 100644 index 000000000..51d8185b0 --- /dev/null +++ b/e2e/fixtures/isolatedUser.ts @@ -0,0 +1,376 @@ +/** + * 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 (server: + * `requireRole('admin')` guards only `/api/users` mutations and + * `/api/backups/*`; client: only the Settings sub-nav "User Management" and + * "Backups" tabs and work-item note edit/delete are role-gated). + * 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"). + * + * 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. Keep the created-user count + * modest regardless — `/settings/users` renders 100 rows per page and + * `edit-user.spec.ts`/`deactivate-user.spec.ts` find their user by scanning the + * rendered page. + */ + +import type { APIRequestContext, PlaywrightWorkerArgs } from '@playwright/test'; +import { test as authTest } from './auth.js'; +import { API } from './testData.js'; + +/** + * Must mirror `use.storageState` in playwright.config.ts. Used as the fallback for + * tests in a file that imports this `test` object without opting into isolation. + */ +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 ────────────── + storageState: async ({ isolatedUserSession }, use, testInfo) => { + await use( + isolatedUserSession?.storageState ?? testInfo.project.use.storageState ?? ADMIN_STORAGE_STATE, + ); + }, + + // ── 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 (a Playwright change to how the `storageState` + // option feeds `_combinedContextOptions`, or a spec passing an explicit + // storageState to `browser.newContext()`), 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. + 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/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); } }); }); From 2ac5fdc08864c215a097ded2d7f1b67570378fd2 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Mon, 3 Aug 2026 13:56:22 +0200 Subject: [PATCH 2/2] test(e2e): scope the no-area household-items empty state to its own data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scenario 4 asserted a suite-global precondition — that no unassigned household item exists anywhere — but ?areaId=__none__ lists every area-less item in the shared DB, and 48 of the suite's 62 createHouseholdItemViaApi() calls across 12 files pass no areaId. It only ever passed while no such spec shared its shard. #1957's test.use() opt-ins changed worker hashes, which moves shard boundaries suite-wide (createTestGroups buckets by _workerHash first, filterForShard slices by cumulative count), relocating this file from shard 15 into shard 14 alongside area-filter.spec.ts, which holds area-less items through a long scenario. Now ANDs the sentinel filter with a q= search for the scenario's own name token, plus a positive control asserting the same search without the sentinel does list the seeded item — otherwise an empty result would be indistinguishable from a search that matched nothing. Back-ports the pattern already green at work-items/no-area-filter.spec.ts:223. Adds no test.use(), so shard assignment is unchanged from the run CI already exercised. Co-Authored-By: Claude e2e-test-engineer --- .../agent-memory/e2e-test-engineer/MEMORY.md | 2 +- .../e2e-test-engineer/general-e2e-patterns.md | 27 +++++++ .../isolated-user-fixture.md | 28 ++++++-- .../known-flakes-and-regressions.md | 1 + e2e/fixtures/isolatedUser.ts | 72 ++++++++++++++----- .../household-items/no-area-filter.spec.ts | 69 ++++++++++++++++-- 6 files changed, 170 insertions(+), 29 deletions(-) diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index a30f30a7f..dcd665f33 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -7,7 +7,7 @@ - [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 (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); login rate limit 20/15min, soft user delete, 100-row user table, worker-scoped-option-in-describe ban, `--list` as the only local validation. +- [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/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 index f4e6f5bd0..a266de578 100644 --- a/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md +++ b/.claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md @@ -1,6 +1,6 @@ --- name: isolated-user-fixture -description: e2e/fixtures/isolatedUser.ts — how to give a preference-mutating spec its own user; plus the hard server-side limits (login rate limit, soft user delete, 100-row user table) that dictate per-worker vs per-test scope +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 --- @@ -36,10 +36,11 @@ against itself only). 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. -- `/settings/users` renders **100 rows per page** with `sortBy: null` (insertion order), and - `edit-user.spec.ts` / `deactivate-user.spec.ts` find their user by scanning the rendered - page. Piling up deactivated E2E users past 100 breaks those unrelated specs. Converting all - ~34 dashboard tests per-test would have added ~85 users/run; per-worker adds ~15. +- 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. @@ -54,6 +55,23 @@ against itself 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()` / 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 index 51d8185b0..1e4ea3f95 100644 --- a/e2e/fixtures/isolatedUser.ts +++ b/e2e/fixtures/isolatedUser.ts @@ -46,10 +46,14 @@ * 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 (server: - * `requireRole('admin')` guards only `/api/users` mutations and - * `/api/backups/*`; client: only the Settings sub-nav "User Management" and - * "Backups" tabs and work-item note edit/delete are role-gated). + * `/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 @@ -133,14 +137,33 @@ * (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. Keep the created-user count - * modest regardless — `/settings/users` renders 100 rows per page and - * `edit-user.spec.ts`/`deactivate-user.spec.ts` find their user by scanning the - * rendered page. + * 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'; @@ -148,8 +171,10 @@ import { test as authTest } from './auth.js'; import { API } from './testData.js'; /** - * Must mirror `use.storageState` in playwright.config.ts. Used as the fallback for - * tests in a file that imports this `test` object without opting into isolation. + * 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'; @@ -341,21 +366,36 @@ export const test = authTest.extend< }, // ── 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 ?? ADMIN_STORAGE_STATE, - ); + 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 (a Playwright change to how the `storageState` - // option feeds `_combinedContextOptions`, or a spec passing an explicit - // storageState to `browser.newContext()`), the specs would quietly go back to + // 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) { 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 () => {