diff --git a/frontend/playwright.config.ts b/frontend/playwright.config.ts index 889af55..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: { @@ -14,6 +35,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();