Fix docs search regression by testing against preview build and adding responsive search-modal coverage #43338
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,9 +45,9 @@ export default defineConfig({ | |
| // Ensure the command always runs relative to the docs folder, regardless | ||
| // of the caller's current working directory. | ||
| cwd: configDir, | ||
| // Search is only available in production builds, so run tests against a | ||
| // built preview server. | ||
| command: 'npm run build && npm run preview -- --host 127.0.0.1 --port 4321', | ||
|
Contributor
There was a problem hiding this comment. webServer timeout of 180 s will be breached by a full production build — tests will never start. 💡 Explanation and fixThe old command was Increase the timeout: timeout: 360 * 1000, // 6 min — covers full Astro prebuild + preview startupIf build time is a concern, a |
||
| url: 'http://localhost:4321/gh-aw/', | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 180 * 1000, | ||
|
Contributor
There was a problem hiding this comment. The Consider increasing the timeout or adding a step in CI to cache the build output: timeout: 300 * 1000, // full production build can take 3–5 min in CI@copilot please address this. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { test, expect } from '@playwright/test'; | ||
|
|
||
| const VIEWPORTS = [ | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The PR title mentions "responsive" coverage, but only desktop (1366px) and tablet (834px) are tested — no mobile viewport. The original false-critical failure was a desktop issue; a mobile breakpoint would round out the regression suite. 💡 Suggested additionconst VIEWPORTS = [
{ name: 'desktop', width: 1366, height: 768 },
{ name: 'tablet', width: 834, height: 1194 },
{ name: 'mobile', width: 390, height: 844 },
];On narrow viewports, Starlight typically hides or replaces the desktop search button with a mobile-specific trigger — worth asserting the modal is still reachable. @copilot please address this.
Contributor
There was a problem hiding this comment. Missing mobile viewport coverage directly contradicts the PR's claim of 'responsive search-modal coverage'. 💡 Explanation and fix
Add at least one mobile breakpoint: const VIEWPORTS = [
{ name: 'mobile', width: 390, height: 844 },
{ name: 'tablet', width: 834, height: 1194 },
{ name: 'desktop', width: 1366, height: 768 },
];On mobile, the search button may need a different locator (e.g., a menu must be opened first). Test that path explicitly rather than assuming desktop and tablet selectors will work. |
||
| { name: 'desktop', width: 1366, height: 768 }, | ||
| { name: 'tablet', width: 834, height: 1194 }, | ||
| ]; | ||
|
Contributor
There was a problem hiding this comment. The spec loops over only desktop (1366×768) and tablet (834×1194) viewports, but the original bug report was a multi-device regression. A mobile viewport (e.g., 390×844 — iPhone 14) would complete the coverage story. const VIEWPORTS = [
{ name: 'desktop', width: 1366, height: 768 },
{ name: 'tablet', width: 834, height: 1194 },
{ name: 'mobile', width: 390, height: 844 },
];Without a mobile entry, a breakpoint-specific regression on small screens would go undetected by this suite. @copilot please address this. |
||
|
|
||
| for (const viewport of VIEWPORTS) { | ||
| test(`search dialog is usable and dismissible on ${viewport.name}`, async ({ page }) => { | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The 💡 Suggested structuretest(`search dialog opens with visible input on ${viewport.name}`, async ({ page }) => { /* open only */ });
test(`search dialog dismisses on Escape on ${viewport.name}`, async ({ page }) => { /* open + Escape */ });
test(`search dialog can be reopened on ${viewport.name}`, async ({ page }) => { /* open, close, reopen */ });
test(`CTA is actionable after closing dialog on ${viewport.name}`, async ({ page }) => { /* open, close, click CTA */ });Each test name reads as a spec statement and the failure message points directly to the broken behaviour. @copilot please address this. |
||
| await page.setViewportSize({ width: viewport.width, height: viewport.height }); | ||
| await page.goto('/gh-aw/'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
Contributor
There was a problem hiding this comment. Flakiness risk: 💡 Explanation and fixA full Astro production build registers deferred hydration callbacks, analytics pings, and background fetches that keep open network requests well past Playwright's 500 ms idle window. On a cold CI runner this reliably causes Replace: await page.waitForLoadState('networkidle');With: await page.waitForLoadState('domcontentloaded');For a static Astro site, |
||
|
|
||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] 💡 SuggestionDrop the explicit await page.goto('/gh-aw/');
// Auto-wait: Playwright retries until the element is visible
await expect(page.getByRole('button', { name: 'Search' })).toBeVisible();The workflow guidance already recommends @copilot please address this. |
||
| await page.getByRole('button', { name: 'Search' }).click(); | ||
|
Contributor
There was a problem hiding this comment. Using Consider replacing with: await page.goto('/gh-aw/', { waitUntil: 'domcontentloaded' });
// remove the separate waitForLoadState call
@copilot please address this.
Contributor
There was a problem hiding this comment.
💡 Explanation and fixStarlight search implementations commonly render multiple elements whose accessible name includes the word "Search" — for example, the visible icon button in the top bar, a keyboard-shortcut hint button (⌘K), and potentially an aria-label on the search container itself. Playwright's Use await page.getByRole('button', { name: 'Search', exact: true }).click();Or, if there are still multiple matches, scope the query to the header: await page.locator('header').getByRole('button', { name: 'Search', exact: true }).click(); |
||
|
|
||
| const openDialog = page.locator('dialog[open]'); | ||
| await expect(openDialog).toBeVisible(); | ||
| await expect(openDialog.getByRole('search').getByRole('textbox', { name: 'Search' })).toBeVisible(); | ||
|
|
||
| await page.keyboard.press('Escape'); | ||
| await expect(openDialog).toHaveCount(0); | ||
|
|
||
| await page.getByRole('button', { name: 'Search' }).click(); | ||
| const reopenedDialog = page.locator('dialog[open]'); | ||
| await expect(reopenedDialog).toBeVisible(); | ||
| await expect( | ||
| reopenedDialog.getByRole('search').getByRole('textbox', { name: 'Search' }) | ||
| ).toBeVisible(); | ||
| await page.keyboard.press('Escape'); | ||
| await expect(reopenedDialog).toHaveCount(0); | ||
|
|
||
| await page.getByRole('link', { name: 'Quick Start with CLI' }).click(); | ||
| await expect(page).toHaveURL(/\/gh-aw\/setup\/quick-start\//); | ||
| }); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnosing-bugs] The
timeoutforwebServeris still180 * 1000ms but the startup command now includes a fullnpm run buildbefore preview. If the build takes longer than ~3 min on a cold CI runner, Playwright will kill the server before tests start and produce confusing timeout errors.💡 Suggestion
Consider raising the timeout to accommodate the build step, or separating it:
Alternatively, document the expected build time in a comment so future editors know the limit isn't arbitrary.
@copilot please address this.