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, - }); - }); -});