From 58c3c52a2290ca1228dbc3949bce2c5f83ccbc0f Mon Sep 17 00:00:00 2001 From: Kalle <38327916+Sendouc@users.noreply.github.com> Date: Fri, 31 Jul 2026 11:16:56 +0300 Subject: [PATCH] Deflake --- .claude/skills/e2e/SKILL.md | 4 +- app/root.tsx | 17 ++++- e2e/comp-analyzer.spec.ts | 4 +- e2e/helpers/playwright.ts | 123 ++++++++++++------------------------ e2e/params.spec.ts | 14 ++-- 5 files changed, 68 insertions(+), 94 deletions(-) diff --git a/.claude/skills/e2e/SKILL.md b/.claude/skills/e2e/SKILL.md index 66c4cc3df..88afdf070 100644 --- a/.claude/skills/e2e/SKILL.md +++ b/.claude/skills/e2e/SKILL.md @@ -81,8 +81,8 @@ Playwright is configured with `trace: "retain-on-failure"`. After a failure, che pnpm exec playwright show-trace test-results//trace.zip ``` -### Cross-worker flakiness -All workers' servers share one websocket (skalop) server, so another worker's actions can revalidate your page mid-interaction — a click on a React Aria button can register the press start but never complete it. The helpers (`waitForPOSTResponse`, `UserCard.open`) retry for this; a spec failing this way under full-suite load but passing alone usually needs its flow routed through such a retrying helper, not a sleep. +### Re-render races +Skalop (websocket) is fully disconnected in e2e — the build has an empty `VITE_SKALOP_WS_URL` and worker servers get empty `SKALOP_SYSTEM_MESSAGE_URL`/`SKALOP_TOKEN` (see `e2e/global-setup.ts`), so cross-worker websocket crosstalk cannot cause flakes. Google Fonts are also blocked at the context level so font swaps never reflow the page mid-test. Re-renders from the test's own action revalidations can still swallow a React Aria press (press start registers, press end never fires — no POST); `waitForPOSTResponse` retries for this, so route flows through it rather than adding sleeps. When e2e tests for chat/websocket features are added, skalop needs a per-worker instance or stub with a runtime-derived WS URL. ## Test pattern reference diff --git a/app/root.tsx b/app/root.tsx index 5556911cd..2aa9daebc 100644 --- a/app/root.tsx +++ b/app/root.tsx @@ -19,6 +19,7 @@ import { Scripts, ScrollRestoration, type ShouldRevalidateFunction, + useFetchers, useHref, useLoaderData, useMatches, @@ -433,10 +434,24 @@ export const ErrorBoundary = () => { function HydrationTestIndicator() { const isHydrated = useHydrated(); + const navigation = useNavigation(); + const revalidator = useRevalidator(); + const fetchers = useFetchers(); if (!isHydrated) return null; - return
; + const routerIdle = + navigation.state === "idle" && + revalidator.state === "idle" && + fetchers.every((fetcher) => fetcher.state === "idle"); + + return ( +
+ ); } function Fonts() { diff --git a/e2e/comp-analyzer.spec.ts b/e2e/comp-analyzer.spec.ts index 4a2ef7f71..3a497ae25 100644 --- a/e2e/comp-analyzer.spec.ts +++ b/e2e/comp-analyzer.spec.ts @@ -29,7 +29,7 @@ test.describe("Composition Analyzer", () => { await expect(compAnalyzer.locators.categorizationToggle).not.toBeVisible(); - expect(page.url()).toContain("weapons="); + await expect(page).toHaveURL(/weapons=/); // Weapons should still be selected after reload await page.reload(); @@ -63,7 +63,7 @@ test.describe("Composition Analyzer", () => { // Switch categorization and test URL persistence await compAnalyzer.selectCategorization("sub"); - expect(page.url()).toContain("categorization=sub"); + await expect(page).toHaveURL(/categorization=sub/); await page.reload(); diff --git a/e2e/helpers/playwright.ts b/e2e/helpers/playwright.ts index a762e83a0..c8229b085 100644 --- a/e2e/helpers/playwright.ts +++ b/e2e/helpers/playwright.ts @@ -32,6 +32,17 @@ type TestFixtures = { }; export const test = base.extend({ + context: async ({ context }, use) => { + // Google Fonts load with display=swap and every test context re-fetches + // them, so the swap reflows the page mid-test (e.g. re-collapsing the + // tournament nav between a visibility check and a click). Block them so + // layout settles at first paint and stays put. + await context.route( + /^https:\/\/fonts\.(googleapis|gstatic)\.com\//, + (route) => route.abort(), + ); + await use(context); + }, workerPort: [ // biome-ignore lint/correctness/noEmptyPattern: Playwright requires object destructuring async ({}, use, workerInfo) => { @@ -201,80 +212,13 @@ async function retryPost( } export async function submit(page: Page, testId?: string) { - // Started before the click because the data GET can land before awaiting the - // POST response hands control back to us, and `page.waitForResponse` only - // sees responses arriving after it is called. - const dataGet = watchForDataGetAfterPOST(page); - - try { - const postRes = await waitForPOSTResponse(page, async () => { - await page.getByTestId(testId ?? "submit-button").click(); - }); - - // Remix returns 202 from action endpoints when the action threw/returned a - // redirect. The fetcher then drives a client-side navigation and, once - // that completes, fires a GET against the new route's data. If we return - // before that GET fires, a subsequent Link click can be aborted mid-flight - // by the queued navigation (ERR_ABORTED on the new route's .data fetch), - // leaving the test on the old page. - if (postRes.status() !== 202) return; - - await dataGet.fired; - // Toast flash params are stripped right after via a replace navigation - // (without revalidation); wait for it so it can't abort a later click. - await expect(page).not.toHaveURL(/__(?:success|error)=/); - } finally { - dataGet.stop(); - } -} - -/** - * Resolves once a route data GET follows the POST, without missing one that - * arrives while the caller is still awaiting the POST response. - */ -function watchForDataGetAfterPOST(page: Page) { - const TIMEOUT = 15_000; - - let postSeen = false; - let resolveFired: () => void = () => {}; - let rejectFired: (error: Error) => void = () => {}; - - const fired = new Promise((resolve, reject) => { - resolveFired = resolve; - rejectFired = reject; + await waitForPOSTResponse(page, async () => { + await page.getByTestId(testId ?? "submit-button").click(); }); - const onResponse = (res: Response) => { - if (!postSeen) { - postSeen = res.request().method() === "POST"; - return; - } - - if (res.request().method() === "GET" && res.url().includes(".data")) { - resolveFired(); - } - }; - page.on("response", onResponse); - - const timeout = setTimeout( - () => - rejectFired( - new Error( - `submit: no route data GET followed the redirecting POST within ${TIMEOUT}ms`, - ), - ), - TIMEOUT, - ); - - return { - fired, - stop: () => { - clearTimeout(timeout); - page.off("response", onResponse); - // Nothing awaits `fired` when the POST wasn't a redirect - resolveFired(); - }, - }; + // Toast flash params are stripped right after via a replace navigation + // (without revalidation); wait for it so it can't abort a later click. + await expect(page).not.toHaveURL(/__(?:success|error)=/); } export async function waitForPOSTResponse(page: Page, cb: () => Promise) { @@ -288,6 +232,7 @@ export async function waitForPOSTResponse(page: Page, cb: () => Promise) { // completes into a submit, so no POST fires — e.g. when a re-render lands // mid-press. Re-issue the action when the expected POST doesn't arrive // within the per-attempt window. + let response: Response | undefined; for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { const responsePromise = page.waitForResponse( (res) => res.request().method() === "POST", @@ -295,13 +240,30 @@ export async function waitForPOSTResponse(page: Page, cb: () => Promise) { ); await cb(); try { - return await responsePromise; + response = await responsePromise; + break; } catch (error) { if (attempt === MAX_ATTEMPTS) throw error; } } - throw new Error("waitForPOSTResponse: unreachable"); + // The POST's revalidation (and any redirect it drives) is still in flight; + // an interaction landing mid-flight aborts it, and routes that opt out of + // revalidation on navigation (e.g. to.$id) then keep the stale data. + await expectRouterIdle(page); + + return response!; +} + +/** Waits until no navigation, revalidation or fetcher is in flight. */ +async function expectRouterIdle(page: Page) { + // A submit's redirect plus the target page's loaders can exceed the default + // expect timeout when the full suite is loading all workers. + await expect(page.getByTestId("hydrated")).toHaveAttribute( + "data-router-idle", + "true", + { timeout: 15_000 }, + ); } export function isNotVisible(locator: Locator) { @@ -314,15 +276,12 @@ export function modalClickConfirmButton(page: Page) { /** * Clicks a tournament nav tab by its testId, opening the overflow ("More") menu - * first when the tab has collapsed into it on the current viewport. Retried as a - * whole because the nav can re-collapse between the visibility check and the click. + * first when the tab has collapsed into it on the current viewport. */ export async function clickNavTab(page: Page, testId: string) { const visibleTab = page.locator(`[data-testid="${testId}"]:visible`); - await expect(async () => { - if ((await visibleTab.count()) === 0) { - await page.getByRole("button", { name: "More" }).click(); - } - await visibleTab.click({ timeout: 2_000 }); - }).toPass({ timeout: 15_000 }); + if ((await visibleTab.count()) === 0) { + await page.getByRole("button", { name: "More" }).click(); + } + await visibleTab.click(); } diff --git a/e2e/params.spec.ts b/e2e/params.spec.ts index 6a7f19df5..19f039bc6 100644 --- a/e2e/params.spec.ts +++ b/e2e/params.spec.ts @@ -27,11 +27,11 @@ test.describe("Weapon parameters", () => { await expect(showAllWeaponsButton).toBeVisible(); await expect(weaponHeaders).toHaveCount(initialColumnCount - 1); - expect(page.url()).toMatch(/hidden=\d/); + await expect(page).toHaveURL(/hidden=\d/); // Refresh keeps the hidden selection await page.reload(); - expect(page.url()).toMatch(/hidden=\d/); + await expect(page).toHaveURL(/hidden=\d/); await expect(showAllWeaponsButton).toBeVisible(); await expect(weaponHeaders).toHaveCount(initialColumnCount - 1); @@ -39,7 +39,7 @@ test.describe("Weapon parameters", () => { await weaponParams.showAllWeapons(); await expect(showAllWeaponsButton).not.toBeVisible(); await expect(weaponHeaders).toHaveCount(initialColumnCount); - expect(page.url()).not.toMatch(/hidden=\d/); + await expect(page).not.toHaveURL(/hidden=\d/); // Comparison bar graph await weaponParams.openParamComparison(); @@ -81,11 +81,11 @@ test.describe("Weapon parameters", () => { weaponParams.locators; await weaponParams.openPatchHistoryTab(); - expect(page.url()).toContain("tab=patches"); + await expect(page).toHaveURL(/tab=patches/); // Refresh keeps the selected tab await page.reload(); - expect(page.url()).toContain("tab=patches"); + await expect(page).toHaveURL(/tab=patches/); await expect(patchHistoryTab).toHaveAttribute("aria-selected", "true"); // Either patch columns are shown or the empty state @@ -94,10 +94,10 @@ test.describe("Weapon parameters", () => { // Toggle "Show sub & special changes" and verify it persists on refresh await expect(subAndSpecialChangesSwitch).toBeChecked(); await weaponParams.toggleSubAndSpecialChanges(); - expect(page.url()).toContain("kitExtras=false"); + await expect(page).toHaveURL(/kitExtras=false/); await page.reload(); - expect(page.url()).toContain("kitExtras=false"); + await expect(page).toHaveURL(/kitExtras=false/); await expect(subAndSpecialChangesSwitch).not.toBeChecked(); }); });