From f8b2b68f0d237519e45a2636b459a71cb60d1831 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Sun, 30 Aug 2026 16:45:22 -0500 Subject: [PATCH 1/3] docs(test): design for making the e2e suite trustworthy (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five distinct specs failed across two runs of identical code with no overlap between the sets, so which test fails is decided by the scheduler. The cost is already being paid: #245 skipped the only end-to-end check that adding an item reaches the database, purely to get main green. The design separates two mechanisms that had been treated as one. Unchecked lookups into shared collections — `collection.find(...)` dereferenced immediately, found at eight or more sites — are a defect regardless of concurrency: when the row is missing the test dies with "Cannot read properties of undefined" naming test plumbing rather than failing an assertion that says what it wanted. Load-induced timing is the other, and is the larger share of what has actually been observed: three of four local failures and the CI one are assertions in a spec's own browser context that nothing else can touch. Two claims from earlier in this investigation are retracted in the document rather than quietly dropped. The verification-resend limiter is not a shared axis — it is keyed per customer and every test registers its own — and the suite contains no snapshot-style assertions, so there is nothing to convert to web-first. Both were stated as fact on the issue, and both would have justified work that was not needed. Per-worker databases are ruled out structurally: every worker talks to one backend on :3000, so isolation there means N backends, not N databases. Worker-count reduction is deliberately deferred rather than taken now, because applying it at the same time would mask whether fixing the defects worked. The honest limit is recorded too. CI's load cannot be reproduced here on demand, so the timing changes rest on reasoning rather than a red-to-green demonstration, and the success criterion is several consecutive green runs rather than one. Ref #241 --- .../specs/2026-08-30-e2e-isolation-design.md | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-30-e2e-isolation-design.md diff --git a/docs/superpowers/specs/2026-08-30-e2e-isolation-design.md b/docs/superpowers/specs/2026-08-30-e2e-isolation-design.md new file mode 100644 index 0000000..8f4559a --- /dev/null +++ b/docs/superpowers/specs/2026-08-30-e2e-isolation-design.md @@ -0,0 +1,80 @@ +# E2E Suite Trustworthiness — Design + +**Issue:** [#241 — specs are not isolated](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/241) +**Date:** 2026-08-30 +**Status:** Draft + +## Goal + +A red e2e run means something changed. Today it might mean that, or it might mean the scheduler interleaved differently. + +## Where this starts from + +Across two runs of identical code on `feb714c`, **five distinct specs failed with no overlap between the two sets**: + +| Run | Failed | +| --- | --- | +| Local, parallel | `admin-inventory-filters:133`, `auth:176`, `favorites-filter:169`, `resend-verification:25` | +| CI | `admin-save-failures` — "saves an item successfully when the server accepts it" | + +Which test fails is decided by scheduling. The consequence is already being paid: #245 skipped the admin-save happy path — the only end-to-end check that adding an item reaches the database — purely to get `main` green. + +## The mechanisms + +Two, and only one of them is isolation. + +**Data collision on unscoped reads.** `favorites-filter:169` fetches `/api/items`, does `items.find(...)` across the entire catalogue, and dereferences the result. Another spec's activity makes that lookup miss, and it *throws* rather than failing an assertion. This is a defect in the test regardless of concurrency: a lookup that assumes a shared list contains its row is wrong even single-threaded, and it fails in the least informative way possible. + +**Load-induced timing.** `admin-inventory-filters:133` toggles three statuses inside its own browser context and asserts `Filters (3)`; no other spec can touch that state. `resend-verification:25` clicks four times and depends on the button settling between clicks. `admin-save-failures` waits on a toast. These fail because the machine is saturated. + +The config sets no `expect` timeout, so the default 5 s applies — and the sold-filter failure in #239 reported exactly `Timeout: 5000ms`. Five seconds is generous on an idle laptop and tight on a runner doing several things at once. + +**Timing is the larger share of what has actually been observed**: three of four local failures, and the CI one. + +### Corrected: the rate limiter is not a shared axis + +An earlier claim on #241 said `resend-verification` fails because the verification-resend limiter's store is shared across concurrent specs. That was wrong. `verificationResendLimiter` is keyed `customer:${req.customerId}`, and the `customer` fixture creates a `uniqueEmail()` per test, so each test has its own customer and its own allowance. The store is process-wide; the keys are not. + +The export exists for the *integration* suite, where `TRUNCATE ... RESTART IDENTITY` recycles customer ids and the allowance genuinely leaks. Its comment was read as applying here. + +It matters because believing it would have justified per-worker isolation to fix something that is not shared. + +## Decisions + +| Question | Decision | +| --- | --- | +| Keep `fullyParallel`? | Yes. CI time is already the binding constraint | +| Per-worker databases? | No — every worker hits one backend on `:3000`, so this needs N backends, not N databases | +| Serialise the suite? | No. Correct, but it lengthens the slowest job on the only runner | +| Reduce worker count? | Only if evidence says so after the real defects are fixed | +| Order of work | Fix the defects first; treat contention as a separate, later question | + +## Approach + +Fix both mechanisms where they occur, then reassess. + +**Unchecked lookups into shared collections.** The pattern is `collection.find(...)` followed by an immediate dereference. A survey found it at eight or more sites, including `admin-disable-customer:37` and `:56` (`.find(...).id`), `admin-inline-category:35`, `admin-taxonomy:44`, `email-templates:54/72/91/116` (`.find(...).body`), and `favorites-filter:169`. + +Most search by something unique to the test — the customer's own email, `vintage-${RUN}` — so an actual collision is unlikely for them. **The defect is the dereference, not the search.** When the row is missing for any reason, the test dies with `Cannot read properties of undefined` naming a line of test plumbing, instead of failing an assertion that says which row was expected. That is a legibility bug independent of concurrency, and it is why `favorites-filter:169` produced no useful signal. + +The fix is a helper that finds-or-fails with a message naming what it looked for, used at every such site. `uniqueSuffix()` and `uniqueEmail()` already exist and 16 of 29 specs use them, so naming data is not the gap — reading it back is. + +**Timing.** Raise the `expect` timeout in `playwright.config.ts` from Playwright's default 5 s to **10 s**. Nothing else sets it today, and the #239 failure reported exactly `Timeout: 5000ms`. Doubling it costs nothing on a green run — it changes how long a *failing* assertion waits before giving up, not how long a passing one takes. + +Verified while surveying: the suite contains **no** snapshot-style assertions (`expect(await locator.isVisible())` and similar). Web-first assertions are already used throughout, so there is nothing to correct there. An earlier draft of this design assumed otherwise. + +**Then, and only then**, un-skip the admin-save test from #245. + +## Not doing + +Worker-count reduction. It is a real lever and may still be needed, but applying it now would mask whether the defect fixes worked. It stays in #241 as the next step if flakes persist. + +Per-worker databases, for the structural reason above. + +## Testing, and an honest limit + +Data-collision fixes are verifiable locally: the failing spec throws today and should stop. + +**The timing half cannot be demonstrated the same way.** CI's load is not reproducible on demand here, so those changes rest on reasoning — a retrying assertion is strictly better than a snapshot one, and 5 s is demonstrably too tight — rather than on a red-to-green transition. + +The success criterion is therefore **several consecutive green CI runs**, not one. A single green run proves very little about a problem whose defining symptom is intermittency. If flakes continue, the next move is capping workers, and that will then be an evidence-backed decision rather than a guess. From d16aba1647df2fafecdb70098c0e032dbb404415 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Sun, 30 Aug 2026 16:52:05 -0500 Subject: [PATCH 2/3] docs(test): plan the e2e trustworthiness work (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tasks. A pure findOrFail helper with Vitest coverage, the nine unchecked lookups converted to use it, the expect timeout raised from Playwright's unset default of 5s to 10s, and the test #245 skipped brought back. The helper deliberately imports nothing and lives apart from support/api.ts. api.ts imports @playwright/test, and vitest.config.ts runs with environment: 'node' over tests/unit only — putting the helper there would drag a browser harness into the unit suite to test six lines of pure logic. Two things the survey changed. filters.spec.ts:216 is excluded: its .find() searches CSS class names on a string array rather than test data, so it has no missing-row failure mode. And three `expect(row).toBeTruthy()` assertions are deleted rather than kept, because findOrFail has already thrown by then — leaving them would tell the next reader the value might be falsy, which is the confusion the change exists to remove. Worker-count reduction is explicitly not in this plan. It is a real lever and may still be needed, but applying it at the same time would make it impossible to tell which change fixed anything. The plan states the criterion it cannot check: CI's load is not reproducible here, so two consecutive green local runs mean the refactor is sound, not that the flakiness is gone. Several consecutive green CI runs are the real bar, and #241 stays open until then. Ref #241 --- .../plans/2026-08-30-e2e-isolation.md | 448 ++++++++++++++++++ 1 file changed, 448 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-30-e2e-isolation.md diff --git a/docs/superpowers/plans/2026-08-30-e2e-isolation.md b/docs/superpowers/plans/2026-08-30-e2e-isolation.md new file mode 100644 index 0000000..f889c7b --- /dev/null +++ b/docs/superpowers/plans/2026-08-30-e2e-isolation.md @@ -0,0 +1,448 @@ +# E2E Suite Trustworthiness Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** A red e2e run means something changed, rather than that the scheduler interleaved differently. + +**Architecture:** Two independent fixes. A pure `findOrFail` helper replaces nine unchecked `collection.find(...)` dereferences so a missing row fails as a named assertion instead of `Cannot read properties of undefined`. The `expect` timeout doubles from Playwright's default 5 s to 10 s, because the runner is saturated and 5 s is where #239 died. Then the test #245 skipped comes back. + +**Tech Stack:** TypeScript, Playwright (e2e), Vitest (unit). + +**Spec:** `docs/superpowers/specs/2026-08-30-e2e-isolation-design.md` + +## Global Constraints + +- **`fullyParallel` stays true.** CI time is the binding constraint — a full pass is ~21.5 minutes on one serialised runner. +- **Worker count is NOT reduced in this plan.** It is a real lever, deliberately held back so the next CI results show whether these fixes worked rather than being masked. It stays in #241 as the follow-up if flakes persist. +- **`findOrFail` must not import from `@playwright/test`.** It lives in its own module so Vitest can test it without pulling a browser harness into the unit suite. `vitest.config.ts` includes only `tests/unit/**/*.test.ts`. +- **Playwright needs Node 20+.** The machine default here is 18.16.1, which cannot run it at all. Use `scripts/run-tests.ps1`, or put a newer Node first on `PATH` for the command only — never `nvm use`, which rewrites a machine-global symlink and needs elevation. +- **The e2e suite needs the app stack.** Backend on `:3000` and a database matching `frontend/tests/e2e/support/db.ts` — user/password/database `redefined_local` on port 55500 unless `TEST_PGPORT` and friends override. +- **Commit style:** Conventional Commits, subject ending `(#241)`, no hard wrapping in bodies. + +## File Structure + +**Created:** +- `frontend/tests/e2e/support/findOrFail.ts` — the helper, pure, no imports +- `frontend/tests/unit/findOrFail.test.ts` — its Vitest tests + +**Modified:** +- `frontend/tests/e2e/support/api.ts` — re-export, so specs reach it through `fixtures.ts` +- `frontend/playwright.config.ts` — add `expect.timeout` +- `frontend/tests/e2e/admin-disable-customer.spec.ts:37,56` +- `frontend/tests/e2e/admin-inline-category.spec.ts:35` +- `frontend/tests/e2e/admin-taxonomy.spec.ts:44` +- `frontend/tests/e2e/email-templates.spec.ts:54,72,91,116` +- `frontend/tests/e2e/favorites-filter.spec.ts:181` +- `frontend/tests/e2e/admin-save-failures.spec.ts` — remove the #245 skip + +**Deliberately untouched:** `filters.spec.ts:216`. Its `.find()` searches CSS class names on a string array, not test data, and has no missing-row failure mode. + +--- + +### Task 1: The find-or-fail helper + +**Files:** +- Create: `frontend/tests/e2e/support/findOrFail.ts` +- Create: `frontend/tests/unit/findOrFail.test.ts` +- Modify: `frontend/tests/e2e/support/api.ts` + +**Interfaces:** +- Produces: `findOrFail(items: T[], predicate: (item: T) => boolean, description: string): T` — returns the match, throws an `Error` naming `description` and the row count when there is none. + +- [ ] **Step 1: Write the failing test** + +Create `frontend/tests/unit/findOrFail.test.ts`: + +```ts +import { describe, it, expect } from 'vitest'; +import { findOrFail } from '../e2e/support/findOrFail'; + +interface Row { + id: number; + name: string; +} + +const rows: Row[] = [ + { id: 1, name: 'first' }, + { id: 2, name: 'second' } +]; + +describe('findOrFail', () => { + it('returns the matching row', () => { + expect(findOrFail(rows, (r) => r.name === 'second', 'the second row')).toEqual({ + id: 2, + name: 'second' + }); + }); + + it('returns the first match when several qualify', () => { + expect(findOrFail(rows, () => true, 'anything').id).toBe(1); + }); + + // The whole point. Dereferencing a missed `.find()` gives "Cannot read + // properties of undefined" pointing at test plumbing; this says what was + // wanted and how many rows were searched. + it('throws naming what it looked for', () => { + expect(() => findOrFail(rows, (r) => r.name === 'absent', 'the absent row')).toThrow( + /the absent row/ + ); + }); + + it('reports how many rows it searched', () => { + expect(() => findOrFail(rows, () => false, 'nothing')).toThrow(/2 rows/); + }); + + it('says so when the collection was empty, which reads differently', () => { + expect(() => findOrFail([], () => true, 'anything')).toThrow(/0 rows/); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd frontend +npm run test:unit +``` + +Expected: FAIL — cannot resolve `../e2e/support/findOrFail`. + +- [ ] **Step 3: Write the implementation** + +Create `frontend/tests/e2e/support/findOrFail.ts`: + +```ts +/** + * Picks one row out of a collection, failing loudly when it is not there. + * + * The pattern this replaces is `collection.find(...)` dereferenced straight + * away — `customers.find(c => c.email === x).id`. When the row is missing the + * test dies with "Cannot read properties of undefined" pointing at a line of + * test plumbing, which says nothing about what was expected. The suite runs + * fullyParallel against one shared database, so a lookup over a collection + * other specs also write to can miss for reasons that have nothing to do with + * the behaviour under test (#241). + * + * Deliberately imports nothing. It lives apart from `api.ts` so the Vitest + * unit suite can cover it without pulling `@playwright/test` — and therefore a + * browser harness — into a run configured for `environment: 'node'`. + * + * Throws rather than returning null: every caller wants the row, and an + * Error at the point of the miss beats a null threaded through three more + * lines before something else fails. + */ +export function findOrFail( + items: T[], + predicate: (item: T) => boolean, + description: string +): T { + const found = items.find(predicate); + + if (found === undefined) { + // The count matters as much as the description: "0 rows" means the + // fixture never landed, while "37 rows" means it landed and the predicate + // is wrong. Those are different bugs and the message should tell them + // apart without a re-run. + throw new Error( + `expected to find ${description}, but none of the ${items.length} rows matched` + ); + } + + return found; +} +``` + +- [ ] **Step 4: Re-export it so specs can reach it** + +Add to the top of `frontend/tests/e2e/support/api.ts`, below the existing imports: + +```ts +// Re-exported so specs get it from './fixtures' with everything else, rather +// than reaching into support/ directly. fixtures.ts does `export * from +// './support/api'`. +export { findOrFail } from './findOrFail'; +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +```bash +cd frontend +npm run test:unit +``` + +Expected: PASS, 5 new tests, plus the existing `filterDimensions` tests. + +- [ ] **Step 6: Commit** + +```bash +git add frontend/tests/e2e/support/findOrFail.ts frontend/tests/unit/findOrFail.test.ts frontend/tests/e2e/support/api.ts +git commit -m "test(e2e): add a find-or-fail helper for collection lookups (#241)" +``` + +--- + +### Task 2: Replace the nine unchecked lookups + +**Files:** +- Modify: `frontend/tests/e2e/admin-disable-customer.spec.ts:37,56` +- Modify: `frontend/tests/e2e/admin-inline-category.spec.ts:35` +- Modify: `frontend/tests/e2e/admin-taxonomy.spec.ts:44` +- Modify: `frontend/tests/e2e/email-templates.spec.ts:54,72,91,116` +- Modify: `frontend/tests/e2e/favorites-filter.spec.ts:181` + +**Interfaces:** +- Consumes: `findOrFail` from Task 1, imported from `./fixtures` + +- [ ] **Step 1: Add the import to each of the five spec files** + +Each already imports from `'./fixtures'`. Add `findOrFail` to that existing import list rather than adding a second import statement — for example, `import { test, expect, findOrFail } from './fixtures';`. Check each file's current import line and extend it. + +- [ ] **Step 2: Replace both lookups in `admin-disable-customer.spec.ts`** + +Lines 37 and 56 are identical. Replace each: + +```ts + const id = customers.find((c: { email: string }) => c.email === customer.email).id; +``` + +with: + +```ts + const id = findOrFail( + customers as { id: number; email: string }[], + (c) => c.email === customer.email, + `the customer ${customer.email}` + ).id; +``` + +- [ ] **Step 3: Replace the lookup in `admin-inline-category.spec.ts`** + +Lines 35-37 currently read: + +```ts + const saved = items.find((item: { name: string }) => item.name === itemName); + expect(saved).toBeTruthy(); + expect(saved.category_name).toBe(categoryName); +``` + +Replace all three with: + +```ts + const saved = findOrFail( + items as { name: string; category_name: string }[], + (item) => item.name === itemName, + `the item ${itemName}` + ); + expect(saved.category_name).toBe(categoryName); +``` + +The `expect(saved).toBeTruthy()` goes deliberately. `findOrFail` has already thrown if the row is absent, so the assertion can no longer fail — and leaving it implies to the next reader that `saved` might be falsy here, which is exactly the confusion this change removes. + +- [ ] **Step 4: Replace the lookup in `admin-taxonomy.spec.ts`** + +Lines 44-46 currently read: + +```ts + const created = tags.find((tag: { name: string }) => tag.name === `vintage-${RUN}`); + expect(created).toBeTruthy(); + expect(created.color).toBeTruthy(); +``` + +Replace all three with: + +```ts + const created = findOrFail( + tags as { name: string; color: string }[], + (tag) => tag.name === `vintage-${RUN}`, + `the tag vintage-${RUN}` + ); + expect(created.color).toBeTruthy(); +``` + +Same reasoning: the existence check is now `findOrFail`'s job. `expect(created.color).toBeTruthy()` stays — that one asserts a real property of the tag, which is what the test is about. + +- [ ] **Step 5: Replace the four lookups in `email-templates.spec.ts`** + +Lines 54, 72 and 91 share a shape: + +```ts + const reset = stored.find((t: { key: string }) => t.key === 'passwordReset'); +``` + +becomes: + +```ts + const reset = findOrFail( + stored as { key: string; subject: string | null; body: string | null }[], + (t) => t.key === 'passwordReset', + 'the passwordReset template' + ); +``` + +Line 116 dereferences inline and needs restructuring so the failure is legible: + +```ts + expect(stored.find((t: { key: string }) => t.key === 'passwordReset').body).toBeNull(); +``` + +becomes: + +```ts + const reset = findOrFail( + stored as { key: string; body: string | null }[], + (t) => t.key === 'passwordReset', + 'the passwordReset template' + ); + expect(reset.body).toBeNull(); +``` + +- [ ] **Step 6: Replace the lookup in `favorites-filter.spec.ts`** + +This is the one with a genuine collision risk — it reads the whole catalogue, which every other spec also writes to. + +```ts + const sells = items.find((item: { name: string }) => item.name === SELLS); +``` + +becomes: + +```ts + const sells = findOrFail( + items as { id: number; name: string }[], + (item) => item.name === SELLS, + `the item ${SELLS}` + ); +``` + +- [ ] **Step 7: Confirm nothing was missed** + +```bash +cd frontend/tests/e2e +grep -rn "\.find(" *.spec.ts +``` + +Expected: exactly one line, `filters.spec.ts:216`. That one searches CSS class names on a string array rather than test data and is deliberately left alone. + +- [ ] **Step 8: Lint and type-check** + +```bash +cd frontend +npm run lint +npm run build +``` + +Expected: `build` clean. `lint` reports 2 warnings, both pre-existing in `src/filters.ts` — no errors, and no new warnings. + +- [ ] **Step 9: Run the changed specs** + +Bring up the stack first (see Global Constraints), then: + +```bash +cd frontend +npx playwright test admin-disable-customer admin-inline-category admin-taxonomy email-templates favorites-filter --workers=1 --reporter=list +``` + +Expected: all pass. Serial deliberately — this step checks the refactor did not change behaviour, and parallelism would confound that with the flakiness being fixed. + +- [ ] **Step 10: Commit** + +```bash +git add frontend/tests/e2e +git commit -m "test(e2e): fail with a named row instead of dereferencing a missed lookup (#241)" +``` + +--- + +### Task 3: Give assertions room on a loaded runner + +**Files:** +- Modify: `frontend/playwright.config.ts` + +- [ ] **Step 1: Add the expect timeout** + +`playwright.config.ts` currently sets no `expect` block, so Playwright's default 5 s applies. Add one after `use`: + +```ts + // Playwright's default is 5s, and nothing overrode it. That is generous on an + // idle laptop and tight on this runner: a full CI pass takes ~21.5 minutes on + // a single machine that also builds, migrates and runs three other suites, + // and the #239 failure reported exactly `Timeout: 5000ms`. + // + // Costs nothing on a green run. This bounds how long a *failing* assertion + // waits before giving up, not how long a passing one takes — a locator that + // resolves in 200ms still resolves in 200ms. + expect: { + timeout: 10000 + }, +``` + +- [ ] **Step 2: Confirm the config still parses and the suite still runs** + +```bash +cd frontend +npm run build +npx playwright test storefront.spec.ts --reporter=list +``` + +Expected: `build` clean, and `storefront.spec.ts` passes. A malformed config fails immediately with a config error rather than a test failure. + +- [ ] **Step 3: Commit** + +```bash +git add frontend/playwright.config.ts +git commit -m "test(e2e): raise the expect timeout for a saturated runner (#241)" +``` + +--- + +### Task 4: Restore the skipped admin-save test + +**Files:** +- Modify: `frontend/tests/e2e/admin-save-failures.spec.ts` + +- [ ] **Step 1: Remove the skip and its explanation** + +Delete the comment block added by #245 and change `test.skip(` back to `test(` for `'saves an item successfully when the server accepts it'`. The whole block from `// SKIPPED, temporarily, ...` down to and including the `test.skip(` line goes; the test body is unchanged. + +- [ ] **Step 2: Run the spec** + +```bash +cd frontend +npx playwright test admin-save-failures --reporter=list +``` + +Expected: 3 passed, 0 skipped. + +- [ ] **Step 3: Run the whole suite in parallel, twice** + +```bash +cd frontend +npx playwright test --reporter=list +npx playwright test --reporter=list +``` + +Expected: both green. Twice, because one green run of an intermittent problem proves very little — and if a run fails, record **which** spec failed rather than only that it did. That name is the evidence for whether Task 2 helped, and for whether worker-count reduction is the next move. + +- [ ] **Step 4: Commit** + +```bash +git add frontend/tests/e2e/admin-save-failures.spec.ts +git commit -m "test(e2e): restore the admin save happy-path test (#245)" +``` + +--- + +## Done when + +- `findOrFail` is covered by 5 Vitest tests and used at all nine former dereference sites. +- `grep -rn "\.find(" frontend/tests/e2e/*.spec.ts` returns only `filters.spec.ts:216`. +- `expect.timeout` is 10 s. +- `admin-save-failures` runs 3 tests, none skipped. +- Two consecutive full parallel local runs are green. +- `npm run lint` and `npm run build` are clean in `frontend/`. + +## The success criterion this plan cannot check + +**Several consecutive green CI runs.** CI's load is not reproducible here on demand, so local green says the refactor is sound — not that the flakiness is gone. Watch the next few runs on `main`. + +If flakes continue, the next step is capping worker count in `playwright.config.ts`, and #241 stays open until that decision is made on evidence. Do not cap it as part of this plan: doing both at once makes it impossible to tell which one worked. From 64324609e120610439971021b5021988010293b9 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Sun, 30 Aug 2026 17:02:07 -0500 Subject: [PATCH 3/3] test(e2e): add a find-or-fail helper for collection lookups (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal. The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase. It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures. Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails. Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean. Ref #241 --- frontend/tests/e2e/support/api.ts | 7 ++++ frontend/tests/e2e/support/findOrFail.ts | 38 +++++++++++++++++++++ frontend/tests/unit/findOrFail.test.ts | 42 ++++++++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 frontend/tests/e2e/support/findOrFail.ts create mode 100644 frontend/tests/unit/findOrFail.test.ts diff --git a/frontend/tests/e2e/support/api.ts b/frontend/tests/e2e/support/api.ts index 282ca15..2f15b9f 100644 --- a/frontend/tests/e2e/support/api.ts +++ b/frontend/tests/e2e/support/api.ts @@ -1,5 +1,12 @@ import { APIRequestContext, expect } from '@playwright/test'; +// Re-exported so specs get it from './fixtures' with everything else, rather +// than reaching into support/ directly — fixtures.ts does `export * from +// './support/api'`. It lives in its own module because that one imports +// nothing, which is what lets the Vitest unit suite cover it without loading +// @playwright/test. See findOrFail.ts. +export { findOrFail } from './findOrFail'; + /** * Where the app under test is served. * diff --git a/frontend/tests/e2e/support/findOrFail.ts b/frontend/tests/e2e/support/findOrFail.ts new file mode 100644 index 0000000..d971086 --- /dev/null +++ b/frontend/tests/e2e/support/findOrFail.ts @@ -0,0 +1,38 @@ +/** + * Picks one row out of a collection, failing loudly when it is not there. + * + * The pattern this replaces is `collection.find(...)` dereferenced straight + * away — `customers.find(c => c.email === x).id`. When the row is missing the + * test dies with "Cannot read properties of undefined" pointing at a line of + * test plumbing, which says nothing about what was expected. The suite runs + * fullyParallel against one shared database, so a lookup over a collection + * other specs also write to can miss for reasons that have nothing to do with + * the behaviour under test (#241). + * + * Deliberately imports nothing. It lives apart from `api.ts` so the Vitest + * unit suite can cover it without pulling `@playwright/test` — and therefore a + * browser harness — into a run configured for `environment: 'node'`. + * + * Throws rather than returning null: every caller wants the row, and an + * Error at the point of the miss beats a null threaded through three more + * lines before something else fails. + */ +export function findOrFail( + items: T[], + predicate: (item: T) => boolean, + description: string +): T { + const found = items.find(predicate); + + if (found === undefined) { + // The count matters as much as the description: "0 rows" means the + // fixture never landed, while "37 rows" means it landed and the predicate + // is wrong. Those are different bugs and the message should tell them + // apart without a re-run. + throw new Error( + `expected to find ${description}, but none of the ${items.length} rows matched` + ); + } + + return found; +} diff --git a/frontend/tests/unit/findOrFail.test.ts b/frontend/tests/unit/findOrFail.test.ts new file mode 100644 index 0000000..1f9551d --- /dev/null +++ b/frontend/tests/unit/findOrFail.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect } from 'vitest'; +import { findOrFail } from '../e2e/support/findOrFail'; + +interface Row { + id: number; + name: string; +} + +const rows: Row[] = [ + { id: 1, name: 'first' }, + { id: 2, name: 'second' } +]; + +describe('findOrFail', () => { + it('returns the matching row', () => { + expect(findOrFail(rows, (r) => r.name === 'second', 'the second row')).toEqual({ + id: 2, + name: 'second' + }); + }); + + it('returns the first match when several qualify', () => { + expect(findOrFail(rows, () => true, 'anything').id).toBe(1); + }); + + // The whole point. Dereferencing a missed `.find()` gives "Cannot read + // properties of undefined" pointing at test plumbing; this says what was + // wanted and how many rows were searched. + it('throws naming what it looked for', () => { + expect(() => findOrFail(rows, (r) => r.name === 'absent', 'the absent row')).toThrow( + /the absent row/ + ); + }); + + it('reports how many rows it searched', () => { + expect(() => findOrFail(rows, () => false, 'nothing')).toThrow(/2 rows/); + }); + + it('says so when the collection was empty, which reads differently', () => { + expect(() => findOrFail([], () => true, 'anything')).toThrow(/0 rows/); + }); +});