bermudalamb
f8b2b68f0d
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
81 lines
6.1 KiB
Markdown
81 lines
6.1 KiB
Markdown
# 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.
|