From d16aba1647df2fafecdb70098c0e032dbb404415 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Sun, 30 Aug 2026 16:52:05 -0500 Subject: [PATCH] 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.