From 895c08d1a741a822341f2cdb5eaa071668e616cf Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 20:04:43 -0500 Subject: [PATCH 1/2] test(e2e): replace unchecked lookups, widen the assertion timeout, restore the skipped test (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks 2 to 4 of the plan. Nine `collection.find(...)` dereferences become findOrFail, so a missing row fails as a named assertion naming what was wanted and how many rows were searched, rather than "Cannot read properties of undefined" pointing at test plumbing. Where the old code followed the lookup with expect(x).toBeTruthy(), that assertion is dropped: findOrFail already guarantees it, and with a better message. The expect timeout goes from Playwright's default 5s to 10s. It costs nothing on a green run — it bounds how long a failing assertion waits, not how long a passing one takes — and #239 died reporting exactly Timeout: 5000ms on a runner that also builds, migrates and runs three other suites. The admin-save happy path comes back from the #245 skip. It is the only end-to-end check that adding an item reaches the database rather than merely firing a toast, and it passed both full parallel runs and in isolation. filters.spec.ts:216 is deliberately untouched: its .find() searches CSS class names on a string array, not test data, and has no missing-row failure mode. Co-Authored-By: Claude Opus 5 --- frontend/playwright.config.ts | 11 ++++++++ .../tests/e2e/admin-disable-customer.spec.ts | 14 +++++++--- .../tests/e2e/admin-inline-category.spec.ts | 11 +++++--- .../tests/e2e/admin-save-failures.spec.ts | 22 ++++------------ frontend/tests/e2e/admin-taxonomy.spec.ts | 9 ++++--- frontend/tests/e2e/email-templates.spec.ts | 26 +++++++++++++++---- frontend/tests/e2e/favorites-filter.spec.ts | 9 +++++-- 7 files changed, 69 insertions(+), 33 deletions(-) diff --git a/frontend/playwright.config.ts b/frontend/playwright.config.ts index 889af55..38b7a1f 100755 --- a/frontend/playwright.config.ts +++ b/frontend/playwright.config.ts @@ -14,6 +14,17 @@ export default defineConfig({ // that was plainly visible and enabled. reducedMotion: 'reduce' }, + // 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 around 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 + }, webServer: { command: 'npm run dev', url: 'http://localhost:5173', diff --git a/frontend/tests/e2e/admin-disable-customer.spec.ts b/frontend/tests/e2e/admin-disable-customer.spec.ts index 22464d7..937c47e 100644 --- a/frontend/tests/e2e/admin-disable-customer.spec.ts +++ b/frontend/tests/e2e/admin-disable-customer.spec.ts @@ -1,4 +1,4 @@ -import { test, expect, createItem, uniqueSuffix } from './fixtures'; +import { test, expect, createItem, uniqueSuffix, findOrFail } from './fixtures'; test.describe('Disabling a customer account', () => { test('an admin can disable an account and the customer is told at sign-in', async ({ @@ -34,7 +34,11 @@ test.describe('Disabling a customer account', () => { await expect(accountModal.emailText(customer.email)).toBeVisible(); const customers = await (await request.get('/api/admin/customers')).json(); - const id = customers.find((c: { email: string }) => c.email === customer.email).id; + const id = findOrFail( + customers, + (c: { email: string }) => c.email === customer.email, + `the customer ${customer.email}` + ).id; await request.post(`/api/admin/customers/${id}/disable`); // The cookie is unchanged, so this proves the server rejects it rather @@ -53,7 +57,11 @@ test.describe('Disabling a customer account', () => { header }) => { const customers = await (await request.get('/api/admin/customers')).json(); - const id = customers.find((c: { email: string }) => c.email === customer.email).id; + const id = findOrFail( + customers, + (c: { email: string }) => c.email === customer.email, + `the customer ${customer.email}` + ).id; await request.post(`/api/admin/customers/${id}/disable`); await admin.open('Customers'); diff --git a/frontend/tests/e2e/admin-inline-category.spec.ts b/frontend/tests/e2e/admin-inline-category.spec.ts index 4693f1a..05997b4 100644 --- a/frontend/tests/e2e/admin-inline-category.spec.ts +++ b/frontend/tests/e2e/admin-inline-category.spec.ts @@ -1,4 +1,4 @@ -import { test, expect, uniqueSuffix, createCategory } from './fixtures'; +import { test, expect, uniqueSuffix, createCategory, findOrFail } from './fixtures'; test.describe('Inline category creation from the item form', () => { test('creates a category without leaving the item form and assigns it', async ({ @@ -32,8 +32,13 @@ test.describe('Inline category creation from the item form', () => { await expect(page.getByText('Item added')).toBeVisible(); const items = await (await page.request.get('/api/admin/items')).json(); - const saved = items.find((item: { name: string }) => item.name === itemName); - expect(saved).toBeTruthy(); + // findOrFail already fails with the row count when the item is missing, + // which the toBeTruthy assertion this replaces could not report. + const saved = findOrFail( + items, + (item: { name: string }) => item.name === itemName, + `the item named ${itemName}` + ); expect(saved.category_name).toBe(categoryName); }); diff --git a/frontend/tests/e2e/admin-save-failures.spec.ts b/frontend/tests/e2e/admin-save-failures.spec.ts index d0bb426..3e34c62 100644 --- a/frontend/tests/e2e/admin-save-failures.spec.ts +++ b/frontend/tests/e2e/admin-save-failures.spec.ts @@ -59,23 +59,11 @@ test.describe('Admin save failures', () => { await expect(adminInventory.row(name)).toBeVisible(); }); - // SKIPPED, temporarily, to get main green while #241 is outstanding. See #245 - // before deleting this comment or the skip. - // - // It fails in CI and passes locally, and which test fails moves around: a - // local parallel run of the whole suite on the same commit failed four - // *different* specs and not this one. That is #241 — fullyParallel against a - // single shared database — so fixing this test on its own would be guessing at - // a symptom that will simply reappear somewhere else. - // - // Not the image work from #226: addItem fills a name and a price and saves, - // attaching nothing, so the re-encoding path is never entered here. - // - // What this stops covering is not trivial. It is the only end-to-end check - // that adding an item actually reaches the database rather than just firing a - // toast — the happy path of the core admin action. Un-skip it as soon as #241 - // lands; if it still fails then, it is a real defect and worth chasing. - test.skip('saves an item successfully when the server accepts it', async ({ + // Un-skipped now that #241 has landed (was skipped under #245). This is the + // only end-to-end check that adding an item actually reaches the database + // rather than just firing a toast — the happy path of the core admin action — + // so it is worth having back. + test('saves an item successfully when the server accepts it', async ({ page, admin, adminInventory diff --git a/frontend/tests/e2e/admin-taxonomy.spec.ts b/frontend/tests/e2e/admin-taxonomy.spec.ts index 5858abf..6896b67 100644 --- a/frontend/tests/e2e/admin-taxonomy.spec.ts +++ b/frontend/tests/e2e/admin-taxonomy.spec.ts @@ -1,4 +1,4 @@ -import { test, expect, uniqueSuffix } from './fixtures'; +import { test, expect, uniqueSuffix, findOrFail } from './fixtures'; // The e2e database is shared and never reset, so every fixture name carries a // unique suffix and assertions are scoped to the nodes this run created. The @@ -41,8 +41,11 @@ test.describe('Admin taxonomy', () => { // The table paginates and the shared database holds many tags, so the new // row is confirmed through the API rather than hunted for across pages. const tags = await (await page.request.get('/api/admin/tags')).json(); - const created = tags.find((tag: { name: string }) => tag.name === `vintage-${RUN}`); - expect(created).toBeTruthy(); + const created = findOrFail( + tags, + (tag: { name: string }) => tag.name === `vintage-${RUN}`, + `the tag vintage-${RUN}` + ); expect(created.color).toBeTruthy(); }); diff --git a/frontend/tests/e2e/email-templates.spec.ts b/frontend/tests/e2e/email-templates.spec.ts index 9718670..416d872 100644 --- a/frontend/tests/e2e/email-templates.spec.ts +++ b/frontend/tests/e2e/email-templates.spec.ts @@ -1,4 +1,4 @@ -import { test, expect, uniqueSuffix } from './fixtures'; +import { test, expect, uniqueSuffix, findOrFail } from './fixtures'; // Each test leaves the templates as it found them, because they are stored in // admin_settings and would otherwise change the copy a later test reads. @@ -51,7 +51,11 @@ test.describe('Editing the customer emails', () => { // Persisted, not merely accepted by the form. const stored = await (await page.request.get('/api/admin/email-templates')).json(); - const reset = stored.find((t: { key: string }) => t.key === 'passwordReset'); + const reset = findOrFail( + stored, + (t: { key: string }) => t.key === 'passwordReset', + 'the passwordReset template' + ); expect(reset.subject).toBe(subject); }); @@ -69,7 +73,11 @@ test.describe('Editing the customer emails', () => { // And nothing was stored. const stored = await (await page.request.get('/api/admin/email-templates')).json(); - const reset = stored.find((t: { key: string }) => t.key === 'passwordReset'); + const reset = findOrFail( + stored, + (t: { key: string }) => t.key === 'passwordReset', + 'the passwordReset template' + ); expect(reset.body).toBeNull(); }); @@ -88,7 +96,11 @@ test.describe('Editing the customer emails', () => { await expect(page.getByText('Password reset restored to the default')).toBeVisible(); const stored = await (await page.request.get('/api/admin/email-templates')).json(); - const reset = stored.find((t: { key: string }) => t.key === 'passwordReset'); + const reset = findOrFail( + stored, + (t: { key: string }) => t.key === 'passwordReset', + 'the passwordReset template' + ); expect(reset.subject).toBeNull(); expect(reset.body).toBeNull(); }); @@ -113,7 +125,11 @@ test.describe('Previewing the customer emails', () => { await expect(adminEmails.preview('Password reset').getByText(wording)).toBeVisible(); const stored = await (await page.request.get('/api/admin/email-templates')).json(); - expect(stored.find((t: { key: string }) => t.key === 'passwordReset').body).toBeNull(); + expect(findOrFail( + stored, + (t: { key: string }) => t.key === 'passwordReset', + 'the passwordReset template' + ).body).toBeNull(); }); test('substitutes sample values rather than showing raw placeholders', async ({ page, admin, adminEmails }) => { diff --git a/frontend/tests/e2e/favorites-filter.spec.ts b/frontend/tests/e2e/favorites-filter.spec.ts index 70197a0..ed0c700 100644 --- a/frontend/tests/e2e/favorites-filter.spec.ts +++ b/frontend/tests/e2e/favorites-filter.spec.ts @@ -8,7 +8,8 @@ import { uniqueEmail, PASSWORD, StorefrontPage, - FavoritePrompt + FavoritePrompt, + findOrFail } from './fixtures'; // The storefront runs against a shared database that is never reset, so every @@ -178,7 +179,11 @@ test.describe('Filtering the storefront by favorites', () => { const api = await createAdminContext(playwright); const items = await (await api.get('/api/items')).json(); - const sells = items.find((item: { name: string }) => item.name === SELLS); + const sells = findOrFail( + items, + (item: { name: string }) => item.name === SELLS, + `the item named ${SELLS}` + ); await sellItem(api, sells.id); await api.dispose(); -- 2.54.0 From 6489486de3508e75bcecb85d5eaaf3e019c14bbe Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 20:13:37 -0500 Subject: [PATCH 2/2] test(e2e): run the suite on one worker so a red run means something (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lever the plan deliberately held back, applied now that there is evidence it is needed. Better failure messages and a wider assertion timeout were not enough. Measured on this branch, same commit, same machine. Two parallel runs each failed 3 of 155, and not the same three: password-reset and admin-inventory-filters in one, admin-email-settings, admin-inventory-filters and resend-verification in the next. All 16 tests from those three specs then passed when run serially, and the full suite passed 155 of 155 twice in a row. Failures that move between runs of identical code are contention, not defects — specs asserting over tables other specs are concurrently writing to. The cost is about three minutes, roughly one minute parallel against 3.9 and 4.0 serially. A red run that means something is worth three minutes; the previous state was a suite whose result nobody could act on, which is what #245 had to skip a real test to work around. If that time is ever needed back, the cheaper fix is giving each worker its own database rather than raising this number and reopening #241. Co-Authored-By: Claude Opus 5 --- frontend/playwright.config.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/frontend/playwright.config.ts b/frontend/playwright.config.ts index 38b7a1f..1808161 100755 --- a/frontend/playwright.config.ts +++ b/frontend/playwright.config.ts @@ -2,7 +2,28 @@ import { defineConfig, devices } from '@playwright/test'; export default defineConfig({ testDir: './tests/e2e', + // Kept true so a file's tests are independent units, but with one worker it + // no longer interleaves them. See `workers` below. fullyParallel: true, + // One worker, because the suite shares a single database. + // + // This was the lever #241 deliberately held back, to see first whether better + // failure messages and a wider assertion timeout were enough. They were not. + // Measured on this branch, same commit, same machine: parallel runs failed 3 + // of 155 twice over, and the three were not the same three — password-reset + // and admin-inventory-filters in one run, admin-email-settings, + // admin-inventory-filters and resend-verification in the next. Run serially, + // all 16 of those same tests passed; the full suite then passed 155 of 155 + // twice in a row. + // + // So the failures were contention, not defects: specs asserting over tables + // that other specs were concurrently writing to. The cost is about 3 minutes + // — roughly 1 minute parallel against 3.9 and 4.0 serially — and a red run + // that means something is worth three minutes. + // + // The cheaper fix, if that time is ever needed back, is giving each worker + // its own database rather than raising this number and reopening #241. + workers: 1, retries: process.env.CI ? 1 : 0, reporter: [['list']], use: { -- 2.54.0