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
6.1 KiB
E2E Suite Trustworthiness — Design
Issue: #241 — specs are not isolated 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.