From f881828cd8135bbe8ad303a95d4dae84b906b2d8 Mon Sep 17 00:00:00 2001 From: James Brunton Date: Mon, 6 Jul 2026 22:37:21 +0100 Subject: [PATCH] Fix intermittently failing Playwright tests (#6886) # Description of Changes Fixes intermittently failing tests (and replaces one that wasn't useful in its previous state) and also adds a CI check to warn if there are any Playwright tests which failed on their first go and succeeded on retries, to hopefully help find intermittently failing tests more quickly and avoid them being merged in the first place. --- .github/workflows/build-enterprise.yml | 19 +++ .github/workflows/e2e-live.yml | 10 ++ .github/workflows/e2e-stubbed.yml | 11 ++ frontend/editor/playwright.config.ts | 13 +- .../editor/scripts/report-flaky-tests.mts | 127 ++++++++++++++++++ .../stubbed/file-state-across-tools.spec.ts | 38 ++++++ .../src/core/tests/stubbed/files-page.spec.ts | 9 +- .../stubbed/unsaved-changes-guard.spec.ts | 43 ------ 8 files changed, 221 insertions(+), 49 deletions(-) create mode 100644 frontend/editor/scripts/report-flaky-tests.mts delete mode 100644 frontend/editor/src/core/tests/stubbed/unsaved-changes-guard.spec.ts diff --git a/.github/workflows/build-enterprise.yml b/.github/workflows/build-enterprise.yml index 99b3b127a4..9e1a4efb83 100644 --- a/.github/workflows/build-enterprise.yml +++ b/.github/workflows/build-enterprise.yml @@ -167,6 +167,8 @@ jobs: wait_for_backend - name: Run enterprise OAuth Playwright tests id: oauth-tests + env: + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results-oauth.json run: task e2e:enterprise -- --grep "OAuth" - name: Stop backend + tear down OAuth Keycloak if: always() @@ -240,6 +242,8 @@ jobs: wait_for_backend - name: Run enterprise SAML Playwright tests id: saml-tests + env: + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results-saml.json run: task e2e:enterprise -- --grep "SAML" - name: Stop backend + tear down SAML Keycloak if: always() @@ -270,6 +274,8 @@ jobs: wait_for_backend - name: Run enterprise feature Playwright tests id: feature-tests + env: + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results-feature.json run: task e2e:enterprise -- --grep "Enterprise license" - name: Print backend log on failure if: failure() @@ -282,6 +288,19 @@ jobs: run: | source /tmp/helpers.sh stop_backend + - name: Flag flaky tests + # Runs regardless of the test outcomes: a flaky test (passed on retry) + # leaves its step green, so this is the only place it surfaces. Merges + # all three phase reports (some may be absent if an earlier phase hard- + # failed and skipped the rest). Emits ::warning:: annotations + a job + # summary; never fails the job. + if: always() + working-directory: frontend + run: > + npx tsx editor/scripts/report-flaky-tests.mts + "${{ github.workspace }}/frontend/playwright-report/results-oauth.json" + "${{ github.workspace }}/frontend/playwright-report/results-saml.json" + "${{ github.workspace }}/frontend/playwright-report/results-feature.json" - name: Upload Playwright report if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/.github/workflows/e2e-live.yml b/.github/workflows/e2e-live.yml index 44d5443a73..57a4357dad 100644 --- a/.github/workflows/e2e-live.yml +++ b/.github/workflows/e2e-live.yml @@ -62,7 +62,17 @@ jobs: # .test-state/playwright/coverage-pw/ for the post-process step # to aggregate. Chromium-only - other engines silently skip. PW_COVERAGE: "1" + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results.json run: task e2e:live + - name: Flag flaky tests + # Runs regardless of the test outcome: a flaky test (passed on retry) + # leaves the step green, so this is the only place it surfaces. Emits + # ::warning:: annotations + a job summary; never fails the job. + if: always() + working-directory: frontend + run: npx tsx editor/scripts/report-flaky-tests.mts "$PLAYWRIGHT_JSON_OUTPUT_FILE" + env: + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results.json - name: Generate JaCoCo report from e2e:live .exec if: always() id: live-coverage diff --git a/.github/workflows/e2e-stubbed.yml b/.github/workflows/e2e-stubbed.yml index 33bb24ee4c..ccfdf0052f 100644 --- a/.github/workflows/e2e-stubbed.yml +++ b/.github/workflows/e2e-stubbed.yml @@ -44,7 +44,18 @@ jobs: VITE_BUILD_FOR_PREVIEW: "1" run: task frontend:build - name: Run stubbed E2E tests (chromium) + env: + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results.json run: task e2e:stubbed -- --workers=3 + - name: Flag flaky tests + # Runs regardless of the test outcome: a flaky test (passed on retry) + # leaves the step green, so this is the only place it surfaces. Emits + # ::warning:: annotations + a job summary; never fails the job. + if: always() + working-directory: frontend + run: npx tsx editor/scripts/report-flaky-tests.mts "$PLAYWRIGHT_JSON_OUTPUT_FILE" + env: + PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results.json - name: Upload Playwright report if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/frontend/editor/playwright.config.ts b/frontend/editor/playwright.config.ts index 7a049e2632..93e6572392 100644 --- a/frontend/editor/playwright.config.ts +++ b/frontend/editor/playwright.config.ts @@ -30,7 +30,18 @@ export default defineConfig({ forbidOnly: !!process.env.CI, retries: process.env.CI ? 2 : 0, workers: process.env.CI ? 1 : "50%", - reporter: [["html", { open: "never" }], ["list"]], + // In CI, add a JSON report alongside the HTML/list output so the workflow + // can flag flaky tests (passed only on retry) as warnings without failing + // the job. Path is pinned via PLAYWRIGHT_JSON_OUTPUT_FILE in the workflow; + // the outputFile here is just a sane default. Omitted locally to keep dev + // runs' terminal output clean. + reporter: process.env.CI + ? [ + ["html", { open: "never" }], + ["list"], + ["json", { outputFile: "playwright-report/results.json" }], + ] + : [["html", { open: "never" }], ["list"]], timeout: 60_000, expect: { timeout: 10_000 }, diff --git a/frontend/editor/scripts/report-flaky-tests.mts b/frontend/editor/scripts/report-flaky-tests.mts new file mode 100644 index 0000000000..1b1039b3ae --- /dev/null +++ b/frontend/editor/scripts/report-flaky-tests.mts @@ -0,0 +1,127 @@ +// Reads a Playwright JSON report and surfaces "flaky" tests (tests that +// failed at least once, then passed on retry) in GitHub Actions WITHOUT +// failing the job: +// - emits one ::warning:: workflow command per flaky test, so the run and +// PR show a yellow warning triangle + count, and the annotation links to +// the test's source line +// - appends a summary table to the job summary ($GITHUB_STEP_SUMMARY) +// +// A green-but-flaky job is otherwise invisible (Playwright exits 0 once a +// retry passes), which lets flakes accrete unnoticed. This makes them visible +// without turning them into hard failures. +// +// Run: `npx tsx editor/scripts/report-flaky-tests.mts [more.json...]` +// (a single path is also read from PLAYWRIGHT_JSON_OUTPUT_FILE). Multiple +// reports are merged + de-duplicated, so a job that runs Playwright in +// several segments (e.g. the enterprise OAuth/SAML/license phases) can +// pass one report per phase. A missing report or zero flaky tests is a +// silent no-op, so it is safe to run with `if: always()` after any +// Playwright step. + +import { appendFileSync, existsSync, readFileSync } from "fs"; +import { isAbsolute, join, relative } from "path"; +import type { JSONReport, JSONReportSuite } from "@playwright/test/reporter"; + +interface FlakyTest { + file: string; + line: number; + title: string; +} + +// Playwright records each test's outcome as expected|unexpected|flaky|skipped. +// "flaky" means it needed a retry to pass, which is exactly what we surface. +function collectFlaky( + report: JSONReport, + workspace: string, + rootDir: string, +): FlakyTest[] { + const flaky: FlakyTest[] = []; + const walk = (suite: JSONReportSuite, trail: string[], depth: number) => { + // The outermost suite per file has title === the file path; skip it so the + // human-readable title is just "describe > test" (the path is shown + // separately as the location). Nested suites are the describe() blocks. + const titles = depth > 0 && suite.title ? [...trail, suite.title] : trail; + for (const spec of suite.specs ?? []) { + if ((spec.tests ?? []).some((t) => t.status === "flaky")) { + const abs = spec.file + ? isAbsolute(spec.file) + ? spec.file + : join(rootDir, spec.file) + : ""; + const rel = abs ? relative(workspace, abs) : ""; + flaky.push({ + // Drop the path from the annotation if it escapes the workspace, so + // we never emit a broken file= link (the warning still shows). + file: rel && !rel.startsWith("..") ? rel : "", + line: spec.line || 0, + title: [...titles, spec.title].filter(Boolean).join(" > "), + }); + } + } + for (const child of suite.suites ?? []) walk(child, titles, depth + 1); + }; + for (const suite of report.suites ?? []) walk(suite, [], 0); + return flaky; +} + +// Deliberately no process.exit() calls: every path falls through to a natural +// exit(0). This step must never fail the job, and it keeps CI green even when +// the report is missing or clean. +function main(): void { + // Accept one or more report paths: a job may run Playwright in several + // segments, each writing its own report (the enterprise job does this for + // OAuth / SAML / license phases). Fall back to the env var when no paths are + // passed. Missing files are skipped, not fatal. + const reportPaths = process.argv.slice(2); + const envPath = process.env.PLAYWRIGHT_JSON_OUTPUT_FILE; + if (reportPaths.length === 0 && envPath) { + reportPaths.push(envPath); + } + + const workspace = process.env.GITHUB_WORKSPACE || process.cwd(); + const seen = new Set(); + const flaky: FlakyTest[] = []; + for (const reportPath of reportPaths) { + if (!reportPath || !existsSync(reportPath)) { + // No report (e.g. the build failed before this segment ran). + continue; + } + const report = JSON.parse(readFileSync(reportPath, "utf8")) as JSONReport; + const rootDir = report.config?.rootDir || process.cwd(); + for (const test of collectFlaky(report, workspace, rootDir)) { + const key = `${test.file}:${test.line}:${test.title}`; + if (!seen.has(key)) { + seen.add(key); + flaky.push(test); + } + } + } + if (flaky.length === 0) { + return; + } + + for (const f of flaky) { + const loc = f.file ? `file=${f.file},line=${f.line},` : ""; + process.stdout.write( + `::warning ${loc}title=Flaky test::${f.title} passed only on retry\n`, + ); + } + + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath) { + const plural = flaky.length === 1 ? "" : "s"; + const lines = [ + `### :warning: ${flaky.length} flaky test${plural} (passed on retry)`, + "", + "These passed, but not on the first attempt. Worth fixing before they turn into hard failures.", + "", + "| Test | Location |", + "| --- | --- |", + ...flaky.map((f) => `| ${f.title} | \`${f.file || "?"}:${f.line}\` |`), + "", + ]; + appendFileSync(summaryPath, lines.join("\n") + "\n"); + } +} + +main(); diff --git a/frontend/editor/src/core/tests/stubbed/file-state-across-tools.spec.ts b/frontend/editor/src/core/tests/stubbed/file-state-across-tools.spec.ts index e4b9a30c3e..6c84f6020a 100644 --- a/frontend/editor/src/core/tests/stubbed/file-state-across-tools.spec.ts +++ b/frontend/editor/src/core/tests/stubbed/file-state-across-tools.spec.ts @@ -9,6 +9,11 @@ const SAMPLE_PDF = path.join(FIXTURES_DIR, "sample.pdf"); * Files uploaded on one tool page should remain in the workbench when the * user navigates to a different tool. This is FileContext behaviour and * easy to break with a stale-effect or unmount-clear bug. + * + * Covered for both navigation mechanisms, which take different code paths: + * - a full page reload (page.goto) -> FileContext re-hydrates from IndexedDB + * - an in-app tool-link click -> client-side nav, FileContext stays + * in memory */ test.describe("File state persists across tool navigation", () => { test("file uploaded on /merge survives navigation to /split", async ({ @@ -32,4 +37,37 @@ test.describe("File state persists across tool navigation", () => { timeout: 5_000, }); }); + + test("file uploaded on /merge survives an in-app tool-link navigation", async ({ + page, + }) => { + await page.goto("/merge"); + await page.waitForLoadState("domcontentloaded"); + await uploadFiles(page, SAMPLE_PDF); + + // Navigate via the in-app tool link (client-side React Router nav) rather + // than a full reload, so this exercises the in-memory FileContext path the + // page.goto test above doesn't. Fall back to a direct visit if the nav + // rail isn't showing the link yet. + const splitNav = page.getByRole("link", { name: /^Split$/i }).first(); + if (await splitNav.isVisible({ timeout: 1_000 }).catch(() => false)) { + await splitNav.click(); + } else { + await page.goto("/split"); + } + + // A client-side nav has no document load event, so waitForLoadState is a + // no-op here. Wait for the route to actually commit before opening the + // file manager; otherwise the my-files click fires mid-transition and + // opens it against a not-yet-settled workbench, which renders a permanent + // empty state (the flaky "0 items" that then passes on retry). + await expect(page).toHaveURL(/\/split(?:$|[/?#])/); + + // The upload must still be listed after the tool switch. A "no files" + // empty state here would mean the client-side nav silently dropped it. + await page.getByTestId("my-files-button").click(); + await expect(page.getByText(/sample\.pdf/i).first()).toBeVisible({ + timeout: 10_000, + }); + }); }); diff --git a/frontend/editor/src/core/tests/stubbed/files-page.spec.ts b/frontend/editor/src/core/tests/stubbed/files-page.spec.ts index 7c08d943bb..56687b2217 100644 --- a/frontend/editor/src/core/tests/stubbed/files-page.spec.ts +++ b/frontend/editor/src/core/tests/stubbed/files-page.spec.ts @@ -825,11 +825,10 @@ test.describe("Files page", () => { // and direct user shares.) await page.locator("#filesPage-tab-sharedByMe").click(); const sharedByMeCards = page.locator(".files-page-card:not(.is-folder)"); - await expect(sharedByMeCards).toHaveCount(2, { timeout: 3_000 }); - await expect(sharedByMeCards).toContainText([ - "link-shared.pdf", - "user-shared.pdf", - ]); + await expect(sharedByMeCards).toHaveCount(2, { timeout: 5_000 }); + for (const name of ["link-shared.pdf", "user-shared.pdf"]) { + await expect(sharedByMeCards.filter({ hasText: name })).toHaveCount(1); + } // "Shared with me" -> only from-someone-else.pdf await page.locator("#filesPage-tab-shared").click(); diff --git a/frontend/editor/src/core/tests/stubbed/unsaved-changes-guard.spec.ts b/frontend/editor/src/core/tests/stubbed/unsaved-changes-guard.spec.ts deleted file mode 100644 index fc3a1240d1..0000000000 --- a/frontend/editor/src/core/tests/stubbed/unsaved-changes-guard.spec.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { test, expect } from "@app/tests/helpers/stub-test-base"; -import { uploadFiles } from "@app/tests/helpers/ui-helpers"; -import path from "path"; - -const SAMPLE_PDF = path.join(__dirname, "../test-fixtures/sample.pdf"); - -/** - * The NavigationGuard context warns the user when they have unsaved work - * (uploaded files or modified config) and try to navigate away. The guard - * surface is a Mantine modal asking to confirm or cancel the navigation. - * - * Today the guard logic exists but is silently bypassed by tests that go - * through the workbench. This spec asserts the modal appears and that - * cancelling keeps the user on the current tool. - */ -test.describe("Unsaved changes navigation guard", () => { - test("uploading then navigating away surfaces the guard prompt", async ({ - page, - }) => { - await page.goto("/merge"); - await page.waitForLoadState("domcontentloaded"); - await uploadFiles(page, SAMPLE_PDF); - - // Triggering a tool-level navigation while files are loaded should - // either prompt or clear-and-navigate cleanly. A regression that - // discards files silently is the failure we want to catch. - const splitNav = page.getByRole("link", { name: /^Split$/i }).first(); - if (await splitNav.isVisible({ timeout: 1_000 }).catch(() => false)) { - await splitNav.click(); - } else { - await page.goto("/split"); - } - - // After arriving at /split the My Files page should still list the - // previously uploaded sample (NavigationGuard either kept us on - // /merge or moved us with state intact). A "no files" empty state - // here would indicate the guard silently dropped the workbench. - await page.getByTestId("my-files-button").click(); - await expect(page.getByText(/sample\.pdf/i).first()).toBeVisible({ - timeout: 5_000, - }); - }); -});