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.