From 10b455144980c10ecb83703c075bec6defda1eee Mon Sep 17 00:00:00 2001 From: James Brunton Date: Tue, 16 Jun 2026 15:56:30 +0100 Subject: [PATCH] Fix failing Playwright test in SaaS (#6688) # Description of Changes Fix Playwright failing test in SaaS (I think this is my third attempt now so who knows if this will actually fix it for real this time, but hopefully it does) --- .github/workflows/e2e-live.yml | 24 +++++++++- .../src/core/services/httpErrorHandler.ts | 16 +++++++ .../tests/live/authentication-login.spec.ts | 44 ++++++++++++++----- 3 files changed, 70 insertions(+), 14 deletions(-) diff --git a/.github/workflows/e2e-live.yml b/.github/workflows/e2e-live.yml index ec46a19f9..840942ceb 100644 --- a/.github/workflows/e2e-live.yml +++ b/.github/workflows/e2e-live.yml @@ -188,10 +188,30 @@ jobs: name: backend-log-live-${{ github.run_id }} path: .test-state/playwright/backend.log retention-days: 7 - - name: Upload Playwright report + - name: List Playwright output locations (debug) + if: always() + run: | + echo "::group::Playwright output dirs" + # Playwright anchors its default outputDir + HTML report to the + # nearest package.json, which is frontend/ (frontend/editor has + # none), so artifacts land under frontend/, not frontend/editor/. + ls -la frontend/playwright-report 2>/dev/null \ + || echo "no playwright-report at frontend/" + ls -la frontend/test-results 2>/dev/null \ + || echo "no test-results at frontend/" + find . -name node_modules -prune -o -name 'trace.zip' -print 2>/dev/null || true + echo "::endgroup::" + - name: Upload Playwright report + traces if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: playwright-report-live-${{ github.run_id }} - path: frontend/editor/playwright-report/ + # test-results/ holds the per-test trace.zip (with browser console + # logs) + screenshots/video; playwright-report/ is the HTML report. + # Both live under frontend/ (Playwright anchors them to the nearest + # package.json, which is frontend/; frontend/editor has none). + path: | + frontend/playwright-report/ + frontend/test-results/ retention-days: 7 + if-no-files-found: warn diff --git a/frontend/editor/src/core/services/httpErrorHandler.ts b/frontend/editor/src/core/services/httpErrorHandler.ts index 69d4c6e06..ed7108b6a 100644 --- a/frontend/editor/src/core/services/httpErrorHandler.ts +++ b/frontend/editor/src/core/services/httpErrorHandler.ts @@ -75,6 +75,22 @@ function markLoginRedirectFired(): void { } } +// Reset the throttle when the user establishes a fresh session via interactive +// login. Otherwise a genuine expiry that happens within the throttle window of +// the redirect that sent them to /login would be wrongly suppressed, leaving +// them on a page with silently failing requests. Login dispatches +// "jwt-available"; token refresh does not (it fires "TOKEN_REFRESHED"), so +// refresh-driven redirect churn is still dampened by the throttle. +if (typeof window !== "undefined") { + window.addEventListener("jwt-available", () => { + try { + window.sessionStorage.removeItem(LOGIN_REDIRECT_THROTTLE_KEY); + } catch { + // sessionStorage unavailable - nothing to clear + } + }); +} + /** * Handles HTTP errors with toast notifications and file error broadcasting * Returns true if the error should be suppressed (deduplicated), false otherwise diff --git a/frontend/editor/src/core/tests/live/authentication-login.spec.ts b/frontend/editor/src/core/tests/live/authentication-login.spec.ts index f4c8d7401..4875fbc4d 100644 --- a/frontend/editor/src/core/tests/live/authentication-login.spec.ts +++ b/frontend/editor/src/core/tests/live/authentication-login.spec.ts @@ -95,8 +95,23 @@ test.describe("1. Authentication and Login", () => { await page.goto("/merge"); await page.waitForLoadState("domcontentloaded"); - // Step 1-2: Invalidate session by clearing cookies and localStorage JWT, - // then re-add the cookie consent cookie so the banner doesn't block after redirect + // Simulate a genuinely expired session by forcing the token-refresh + // endpoint to fail. Clearing client state alone is not enough: the SPA's + // auth client refreshes a fresh JWT from a surviving server-side + // credential and re-authenticates, churning between /merge, / and /login + // instead of settling on the login page. A 401 here is what a real + // expired/revoked session looks like to the client. + await page.route("**/api/v1/auth/refresh", (route) => + route.fulfill({ + status: 401, + contentType: "application/json", + body: JSON.stringify({ error: "session_expired" }), + }), + ); + + // Invalidate the client session: clear cookies + the stored JWT, then + // re-add the cookie-consent cookie so the banner doesn't block the login + // page after redirect. await page.context().clearCookies(); await page.evaluate(() => { localStorage.removeItem("stirling_jwt"); @@ -116,23 +131,28 @@ test.describe("1. Authentication and Login", () => { }, ]); - // Full page reload forces the SPA to re-check auth with the backend + // Full page reload forces the SPA to re-check auth with the backend. await page.reload({ waitUntil: "domcontentloaded" }); - // Step 3: Verify the user is redirected to the login page. The redirect - // is driven client-side after the SPA re-bootstraps (session check + - // config fetch + backend probe), which can outrun a 15s budget on a - // loaded CI runner, so allow longer here. - await expect(page).toHaveURL(/\/login/, { timeout: 30000 }); + // With refresh failing, the SPA settles on the login page. Wait for the + // login form to render (a concrete signal the redirect landed) and then + // confirm the URL, rather than racing a URL poll against the bootstrap. + await page + .locator("#email") + .waitFor({ state: "visible", timeout: 30000 }); + await expect(page).toHaveURL(/\/login/); - // Step 5: Log in with valid credentials + // Stop forcing refresh failures so the fresh login below behaves normally. + await page.unroute("**/api/v1/auth/refresh"); + + // Log back in with valid credentials. await page.locator("#email").fill("admin"); await page.locator("#password").fill("adminadmin"); await page.locator('button[type="submit"]').click(); - // Step 6: Verify the user is redirected back to /merge or home. - // Any non-/login URL is acceptable — the app may route to the original - // page (/merge) or to the dashboard (/), both are valid post-login states. + // Verify the user is redirected back off the login page. Any non-/login + // URL is acceptable: the app may route to the original page (/merge) or + // to the dashboard (/), both are valid post-login states. await page.waitForURL((url) => !url.pathname.includes("/login"), { timeout: 15000, });