Merge pull request 'test(e2e): design, plan, and the find-or-fail helper (#241)' (#247) from fix/241-e2e-isolation into main
Reviewed-on: #247
This commit was merged in pull request #247.
This commit is contained in:
@@ -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.
|
||||
@@ -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.
|
||||
@@ -1,5 +1,12 @@
|
||||
import { APIRequestContext, expect } from '@playwright/test';
|
||||
|
||||
// Re-exported so specs get it from './fixtures' with everything else, rather
|
||||
// than reaching into support/ directly — fixtures.ts does `export * from
|
||||
// './support/api'`. It lives in its own module because that one imports
|
||||
// nothing, which is what lets the Vitest unit suite cover it without loading
|
||||
// @playwright/test. See findOrFail.ts.
|
||||
export { findOrFail } from './findOrFail';
|
||||
|
||||
/**
|
||||
* Where the app under test is served.
|
||||
*
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
/**
|
||||
* 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;
|
||||
}
|
||||
@@ -0,0 +1,42 @@
|
||||
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/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user