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