Merge pull request #682 from MBombeck/fix/e2e-two-failures

Scan the painted page, and bound the waits that could not time out

e2e/a11y.spec.ts

@@ -664,12 +664,21 @@ const INSIGHTS_ROUTES: readonly RouteCase[] = [
     painted: (page) => page.locator(".recharts-wrapper").first(),
   },
   {
+    // The two states this page's OWN render produces: the list, or the
+    // no-workouts empty state. Never `[role="status"]`, because the busy
+    // skeletons carry it too (`chart-skeleton` in the streamed route shell is
+    // one), so that selector matched the loading tree the server sends before
+    // the page client has mounted anything. On a loaded runner the scan then
+    // landed on a document whose only heading had not rendered yet and blamed
+    // the app for a missing `<h1>` it does have. Same rule as the overview
+    // case above: a sentinel must prove the content, not the placeholder
+    // standing in for it.
     name: "/insights/workouts list",
     path: "/insights/workouts",
     painted: (page) =>
       page
         .locator(
-          '#main-content [data-slot="workout-list"], #main-content [role="status"]',
+          '#main-content [data-slot="workout-list"], #main-content [data-slot="empty-state"]',
         )
         .first(),
   },
@@ -679,9 +688,13 @@ const INSIGHTS_ROUTES: readonly RouteCase[] = [
     painted: (page) => page.locator('[data-slot="workout-detail-header"]'),
   },
   {
+    // `coach-page` is the sizing wrapper around a `<Suspense fallback={null}>`,
+    // so it is on screen while the conversation below it is still nothing at
+    // all. Gate on the conversation surface itself.
     name: "/coach",
     path: "/coach",
-    painted: (page) => page.locator('[data-slot="coach-page"]').first(),
+    painted: (page) =>
+      page.locator('[data-slot="coach-conversation"][data-variant="page"]'),
   },
 ];
 

e2e/utils/settle.ts

@@ -1,5 +1,23 @@
 import { expect, type Locator, type Page } from "@playwright/test";
 
+/**
+ * How long either settle wait in `settleBeforeMeasure` may take before the
+ * spec moves on.
+ *
+ * Both of those waits are written to end on their own terms; neither was
+ * bounded, and neither inherits a bound from anywhere, because the config
+ * leaves `actionTimeout` and `navigationTimeout` unset and Playwright reads
+ * that as "no limit". One request that never completes therefore ends the
+ * whole test instead of just the wait, and it ends it in the least useful way
+ * available: the run reports whichever call was still pending when the fixture
+ * tore the page down, so the message names a `page.evaluate` against a closed
+ * target rather than the surface that stalled. Six seconds is an order of
+ * magnitude above the slowest settle measured anywhere in this suite. Past
+ * that it is a stall, and a stall belongs in front of the spec's own
+ * assertions, which can say what is actually wrong.
+ */
+const SETTLE_BUDGET_MS = 6_000;
+
 /**
  * Gate a one-shot DOM read on the element it is about to measure.
  *
@@ -45,6 +63,30 @@ export async function settleBeforeMeasure(
     .catch(() => {});
   // Fonts and late CSS change geometry after the element is visible, and
   // every measurement in this suite is geometric.
-  await page.waitForLoadState("networkidle").catch(() => {});
-  await page.evaluate(() => document.fonts?.ready);
+  await page
+    .waitForLoadState("networkidle", { timeout: SETTLE_BUDGET_MS })
+    .catch(() => {});
+  await waitForFonts(page);
+}
+
+/**
+ * Wait for webfonts to settle, bounded inside the page.
+ *
+ * `document.fonts.ready` belongs to the document, so awaiting it from a
+ * `page.evaluate` hands the test an unbounded wait on a promise no timeout
+ * reaches. Race it against a timer in the page instead, so a font that never
+ * arrives costs this wait and nothing else.
+ */
+export async function waitForFonts(
+  page: Page,
+  timeout = SETTLE_BUDGET_MS,
+): Promise<void> {
+  await page.evaluate(async (budget: number) => {
+    const ready = document.fonts?.ready;
+    if (!ready) return;
+    await Promise.race([
+      ready,
+      new Promise((resolve) => window.setTimeout(resolve, budget)),
+    ]);
+  }, timeout);
 }

e2e/v1341-ui-evidence.spec.ts

@@ -11,7 +11,7 @@ import {
   mockDashboardSnapshot,
 } from "./utils/mock-dashboard-snapshot";
 import { mockPopulatedInsights } from "./utils/mock-populated-insights";
-import { settleBeforeMeasure } from "./utils/settle";
+import { settleBeforeMeasure, waitForFonts } from "./utils/settle";
 
 const EVIDENCE_DIR = join(process.cwd(), "test-results", "v1341");
 
@@ -46,7 +46,9 @@ async function settleAndCapture(
     content:
       "*,*::before,*::after{animation:none!important;transition:none!important;caret-color:transparent!important}",
   });
-  await page.evaluate(() => document.fonts?.ready);
+  // The injected stylesheet can pull a font the page had not needed yet, so
+  // the wait repeats after it. Bounded, for the reason `waitForFonts` gives.
+  await waitForFonts(page);
   const path = join(EVIDENCE_DIR, filename);
   await page.screenshot({ path, animations: "disabled" });
   await testInfo.attach(filename, { path, contentType: "image/png" });