diff --git a/.claude/skills/debug-e2e/SKILL.md b/.claude/skills/debug-e2e/SKILL.md index 253b2900f0..b31ee0223b 100644 --- a/.claude/skills/debug-e2e/SKILL.md +++ b/.claude/skills/debug-e2e/SKILL.md @@ -69,7 +69,7 @@ Prefer narrowing with `-g` and `--project` first, then increase `--repeat-each` ### 3. Analyze failure artifacts -When tests fail, Playwright saves traces and error context: +When tests fail, Playwright saves error context. Traces are off by default in local runs because recording them slows the suite; add `--trace retain-on-failure` to the test command to keep a trace for each failed test. ```bash # View error context (page snapshot at failure time) diff --git a/AGENTS.md b/AGENTS.md index b67c800bc7..e4d5e39546 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ - Treat Vitest browser specs as small e2e tests: query by accessible role, label, or visible text and use retrying browser matchers. Avoid selectors coupled to CSS classes or internal DOM structure; inspect layout or computed styles only when the behavior has no semantic representation. - For sweeping styling changes, coordinate with the visual regression harness and follow `test/visual/README.md` for the workflow. - Fix root causes of flaky timing rather than adding `sleep()` workarounds in tests. -- When an intermittent browser or E2E failure does not reproduce under normal repetition, retry under controlled CPU contention before concluding it is CI-only. Keep the load bounded so the browser and test runner can still make progress, enable failure-only traces when available, and use a shell trap to clean up every churn process. +- When an intermittent browser or E2E failure does not reproduce under normal repetition, retry under controlled CPU contention before concluding it is CI-only. Keep the load bounded so the browser and test runner can still make progress, enable failure-only traces (`--trace retain-on-failure`, off by default locally), and use a shell trap to clean up every churn process. - Local Playwright runs write a compact plain-text report to `.e2e-logs/` (gitignored, one timestamped `.log` per run, last 10 kept) via the custom reporter at `test/e2e/compact-reporter.ts`. Top line is `status: ... total=N passed=N ...`; each failure is a `── UNEXPECTED|FLAKY file:line title` block followed by the error (ANSI stripped). Latest run: `ls .e2e-logs | tail -1` — Read it directly, no parsing needed. # Data fetching pattern diff --git a/playwright.config.ts b/playwright.config.ts index adaa0fe63a..a399eb8d69 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -33,7 +33,10 @@ export default { // an LLM agent can read the failures. reporter: process.env.CI ? 'list' : [['list'], ['./test/e2e/compact-reporter.ts']], use: { - trace: process.env.CI ? 'on-first-retry' : 'retain-on-failure', + // Recording a trace for every test makes local runs much slower, so it's + // off by default. Pass `--trace retain-on-failure` to keep traces of + // failed tests, e.g. when chasing a flake. + trace: process.env.CI ? 'on-first-retry' : 'off', baseURL: 'http://localhost:4009', }, projects: [ diff --git a/test/e2e/route-announcer.e2e.ts b/test/e2e/route-announcer.e2e.ts index 9570f084f1..4e41d39d76 100644 --- a/test/e2e/route-announcer.e2e.ts +++ b/test/e2e/route-announcer.e2e.ts @@ -7,22 +7,46 @@ */ import { expect, test, type Page } from '@playwright/test' +declare global { + interface Window { + announcements: string[] + } +} + // react-aria appends each announcement as a child of a hidden polite live -// region shared with toasts. CSS locator because there's a second region for +// region shared with toasts, and removes it 7 seconds later. A slow CI runner +// can take longer than that to get to an assertion, so record announcements as +// they're added. Match on the polite region because there's a second one for // assertive announcements and we only want the messages from this one. -const announcements = (page: Page) => - page.locator('[data-live-announcer] [aria-live="polite"] > div') +test.beforeEach(async ({ page }) => { + await page.addInitScript(() => { + window.announcements = [] + new MutationObserver((records) => { + for (const { target, addedNodes } of records) { + if ( + target instanceof Element && + target.matches('[data-live-announcer] [aria-live="polite"]') + ) { + for (const node of addedNodes) window.announcements.push(node.textContent ?? '') + } + } + }).observe(document, { childList: true, subtree: true }) + }) +}) + +const expectAnnouncements = (page: Page) => + expect.poll(() => page.evaluate(() => window.announcements)) test('route announcer', async ({ page }) => { await page.goto('/projects') await expect(page.getByRole('heading', { name: 'Projects' })).toBeVisible() // nothing on first load — the browser already announced the page - await expect(announcements(page)).toHaveCount(0) + await expectAnnouncements(page).toEqual([]) // nav by a link inside the content, which unmounts along with the page await page.getByRole('link', { name: 'mock-project' }).click() - await expect(announcements(page)).toHaveText(['Instances, mock-project, Projects']) + await expectAnnouncements(page).toEqual(['Instances, mock-project, Projects']) // focus goes to the new page's heading, like (better than) a real page load await expect(page.getByRole('heading', { name: 'Instances' })).toBeFocused() @@ -32,7 +56,7 @@ test('route announcer', async ({ page }) => { .getByRole('navigation', { name: 'Sidebar navigation' }) .getByRole('link', { name: 'Disks' }) await disksLink.click() - await expect(announcements(page)).toHaveText([ + await expectAnnouncements(page).toEqual([ 'Instances, mock-project, Projects', 'Disks, mock-project, Projects', ]) @@ -42,12 +66,12 @@ test('route announcer', async ({ page }) => { // than replacing it, so it doesn't announce or take focus from the dialog await page.getByRole('link', { name: 'New Disk' }).click() await expect(page.getByRole('dialog', { name: 'Create disk' })).toBeVisible() - await expect(announcements(page)).toHaveCount(2) + await expectAnnouncements(page).toHaveLength(2) // ...and neither does dismissing it await page.getByRole('button', { name: 'Cancel' }).click() await expect(page.getByRole('dialog', { name: 'Create disk' })).toBeHidden() - await expect(announcements(page)).toHaveCount(2) + await expectAnnouncements(page).toHaveLength(2) }) // tabs that live in the query param rather than the path aren't navigations as @@ -56,7 +80,7 @@ test('route announcer ignores query param tabs', async ({ page }) => { await page.goto('/system/utilization') await page.getByRole('tab', { name: 'Metrics' }).click() await expect(page).toHaveURL(/tab=metrics/) - await expect(announcements(page)).toHaveCount(0) + await expectAnnouncements(page).toEqual([]) }) // VPC tabs, on the other hand, are real routes. Routes like these share a crumb @@ -65,7 +89,7 @@ test('route announcer on tab routes', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/default/firewall-rules') const tab = page.getByRole('tab', { name: 'Routers' }) await tab.click() - await expect(announcements(page)).toHaveText([ + await expectAnnouncements(page).toEqual([ 'Routers, default, VPCs, mock-project, Projects', ]) // the tab is still there, so it keeps focus @@ -78,5 +102,5 @@ test('route announcer ignores detail side modals', async ({ page }) => { await page.goto('/images') await page.getByRole('link', { name: 'ubuntu-22-04' }).click() await expect(page.getByRole('dialog', { name: 'Image details' })).toBeVisible() - await expect(announcements(page)).toHaveCount(0) + await expectAnnouncements(page).toEqual([]) }) diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 0aca22ede9..f14f1cde7a 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -179,11 +179,20 @@ export async function stopInstance(page: Page) { } /** - * Assert that a toast with text matching `expectedText` is visible. + * Assert that a toast with text matching `expectedText` is visible, then + * dismiss that toast and wait for it to go away. */ export async function expectToast(page: Page, expectedText: string | RegExp) { await expect(page.getByTestId('Toasts')).toContainText(expectedText) - await closeToast(page) + // Dismiss the toast we matched, not whichever is first. An error test often + // has a "creating..." toast followed closely by a result toast, and which one + // is first depends on how fast the mock answers. + const toast = page + .getByTestId('Toasts') + .locator(':scope > div') + .filter({ hasText: expectedText }) + .first() + await dismissToast(toast.getByRole('button', { name: 'Dismiss notification' })) } /** @@ -193,18 +202,25 @@ export async function expectNoToast(page: Page, expectedText: string | RegExp) { await expect(page.getByTestId('Toasts')).not.toContainText(expectedText) } +/** Close the first toast and wait for it to go away. */ +export async function closeToast(page: Page) { + await dismissToast(page.getByRole('button', { name: 'Dismiss notification' }).first()) +} + /** - * Close first toast and wait for it to fade out. For some reason it prevents - * things from working, but only in tests as far as we can tell. + * Click a toast's dismiss button and wait until the toast has left the DOM + * (after its exit animation), so it can't cover what the test clicks next. + * Hold the element, not the locator: a toast arriving in between would change + * what the locator points to. */ -export async function closeToast(page: Page) { - // first() is a hack aimed at situations where we're testing an error - // response, which usually means we have an initial "creating..." toast - // followed by an error toast. Sometimes the error toast shows up so fast that - // we don't have time to close the first one. Without first(), this errors out - // because there are two toasts. - await page.getByRole('button', { name: 'Dismiss notification' }).first().click() - await sleep(1000) +async function dismissToast(button: Locator) { + // elementHandle waits for a match and throws on timeout, so it's never null + const handle = (await button.elementHandle())! + await handle.click().catch(async (e) => { + // the toast timed out and left on its own before the click landed + if (await handle.evaluate((el) => el.isConnected)) throw e + }) + await handle.waitForElementState('hidden') } /** diff --git a/test/e2e/vpcs.e2e.ts b/test/e2e/vpcs.e2e.ts index 36d42894ee..609833850b 100644 --- a/test/e2e/vpcs.e2e.ts +++ b/test/e2e/vpcs.e2e.ts @@ -13,7 +13,9 @@ test('can nav to VpcPage from /', async ({ page }) => { await page.goto('/') await page.getByRole('table').getByRole('link', { name: 'mock-project' }).click() await page.waitForURL('**/projects/mock-project/**') - await page.getByRole('link', { name: 'VPCs' }).click() + // exact because the URL changes before the projects table goes away, and on + // a slow machine a substring match clicks the project-no-vpcs link in it + await page.getByRole('link', { name: 'VPCs', exact: true }).click() await page.waitForURL('**/vpcs**') await expectRowVisible(page.getByRole('table'), {