From 464a0645e7b1a6792017c40a28ba3e9b10bca6c9 Mon Sep 17 00:00:00 2001 From: ethernet Date: Mon, 20 Jul 2026 17:29:09 -0400 Subject: [PATCH] fix(desktop): wire error-banner guard into e2e fixtures The guard wasn't firing because Electron tests create their own page via app.firstWindow(), not via Playwright's default page fixture. The base.afterEach's page fixture was undefined for Electron tests. Fix: export installErrorBannerGuard + collectErrorBanners from e2e/test.ts, call installErrorBannerGuard from launchDesktop() and setupPackagedApp() in fixtures.ts (both firstWindow() call sites), and use activePage (set by the guard) in afterEach instead of the default page fixture. Verified: the guard now catches the 'Resume failed' error banner ('handler error: str object has no attribute get') that appears during the large-session-reload e2e test. --- apps/desktop/e2e/fixtures.ts | 6 ++++ apps/desktop/e2e/large-session-reload.spec.ts | 6 +++- apps/desktop/e2e/test.ts | 34 ++++++++++++++----- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/apps/desktop/e2e/fixtures.ts b/apps/desktop/e2e/fixtures.ts index 27a3c0be7c0..93acc6f8623 100644 --- a/apps/desktop/e2e/fixtures.ts +++ b/apps/desktop/e2e/fixtures.ts @@ -28,6 +28,7 @@ import * as path from 'node:path' import { _electron, type ElectronApplication, type Page } from '@playwright/test' import { startMockServer } from './mock-server' +import { installErrorBannerGuard } from './test' const DESKTOP_ROOT = path.resolve(import.meta.dirname, '..') const REPO_ROOT = path.resolve(DESKTOP_ROOT, '..', '..') @@ -305,6 +306,10 @@ export async function launchDesktop( const page = await app.firstWindow() + // Install the error-banner guard so any [role="alert"] that appears + // during a test is collected and surfaced in afterEach. + installErrorBannerGuard(page) + return { app, page } } @@ -514,6 +519,7 @@ export async function setupPackagedApp(): Promise { }) const page = await app.firstWindow() + installErrorBannerGuard(page) return { app, diff --git a/apps/desktop/e2e/large-session-reload.spec.ts b/apps/desktop/e2e/large-session-reload.spec.ts index 333a94df2bc..89243f4003d 100644 --- a/apps/desktop/e2e/large-session-reload.spec.ts +++ b/apps/desktop/e2e/large-session-reload.spec.ts @@ -27,7 +27,7 @@ import { spawnSync } from 'node:child_process' import { readFileSync } from 'node:fs' import * as path from 'node:path' -import { _electron, expect, type ElectronApplication, type Page, test } from './test' +import { _electron, expect, installErrorBannerGuard, type ElectronApplication, type Page, test } from './test' import { buildAppEnv, @@ -111,6 +111,10 @@ async function setupSeededMockBackend(): Promise { const env = buildAppEnv(sandbox) const { app, page } = await launchDesktop(env) + // Install the error-banner guard on the custom page (launchDesktop + // already calls this, but call again in case the page was replaced). + installErrorBannerGuard(page) + return { app, page, diff --git a/apps/desktop/e2e/test.ts b/apps/desktop/e2e/test.ts index 1a9fb33225b..4ed4121d3e2 100644 --- a/apps/desktop/e2e/test.ts +++ b/apps/desktop/e2e/test.ts @@ -23,8 +23,13 @@ let activePage: Page | null = null * Install the error-banner guard on a page. Watches for `[role="alert"]` * elements appearing in the DOM. When one is found, records its text * content for the afterEach assertion. + * + * Exported so e2e fixture functions (which create pages via _electron.launch) + * can install the guard on their custom pages — the default Playwright `page` + * fixture override only catches pages created by Playwright itself, not + * pages created by the test's own Electron launch. */ -function installErrorBannerGuard(page: Page): void { +export function installErrorBannerGuard(page: Page): void { activePage = page // Clear any errors from a previous test when a new page is created. @@ -69,9 +74,10 @@ function installErrorBannerGuard(page: Page): void { /** * Check for error banners that appeared during the test. Called in - * afterEach via the custom fixture below. + * afterEach via the custom fixture below. Also exported so specs that + * manage their own page lifecycle can call it directly. */ -async function collectErrorBanners(page: Page | null): Promise { +export async function collectErrorBanners(page: Page | null): Promise { if (!page) { return [] } @@ -110,12 +116,18 @@ export const test = base.extend({ }) // afterEach: fail the test if any error banners appeared. -base.afterEach(async ({ page }, testInfo) => { - const errors = await collectErrorBanners(page ?? activePage) +// Always fires — even if the test already failed for another reason. +// An error banner often IS the root cause (e.g. "resume failed" from a +// backend bug), and suppressing it when the test also fails on an +// assertion hides the real problem. +// +// Uses `activePage` (set by installErrorBannerGuard) instead of the +// default `page` fixture — Electron tests create their own page via +// app.firstWindow(), so the default `page` fixture is undefined. +base.afterEach(async ({}, testInfo) => { + const errors = await collectErrorBanners(activePage) - if (errors.length > 0 && testInfo.status !== 'failed') { - // Only fail if the test didn't already fail on its own — we don't - // want to mask the original assertion error with our banner check. + if (errors.length > 0) { throw new Error( `Error banner(s) appeared during test "${testInfo.title}":\n` + errors.map(e => ` • ${e}`).join('\n'), @@ -123,4 +135,10 @@ base.afterEach(async ({ page }, testInfo) => { } }) +// Reset for the next test file. +base.afterAll(async () => { + seenErrors.length = 0 + activePage = null +}) + export { expect, type Page, type ElectronApplication, _electron }