2026-08-31 14:44:14 -05:00
1 changed files with 448 additions and 0 deletions
@@ -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<T>(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<T>(
+  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.