diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 13d4c87684c7..0095f5e3b346 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -323,7 +323,11 @@ jobs: run: pnpm test --reporter=list,junit,html # What explains an e2e failure: Playwright's HTML report, plus the trace and failure - # screenshot its config keeps (trace: retain-on-failure, screenshot: only-on-failure). + # screenshot its config keeps (trace: retain-on-failure, screenshot: only-on-failure), + # and what the fixture keeps beside them for a failed test (fixtures/bloomTest.ts, + # keepEvidenceOnFailure): Bloom's Log.txt and a copy of the collection folder as Bloom + # left it. Not the user-settings folder: this artifact is public, and user.config holds + # the Bloom Library login of a Bloom that signed in for real. # On failure only: a green run's report says nothing a green check has not already said, # the same reason the visual-regression screenshots below go up only on failure. # diff --git a/src/BloomE2E/fixtures/bloomTest.ts b/src/BloomE2E/fixtures/bloomTest.ts index e67e030a1ad7..cfd57c4740da 100644 --- a/src/BloomE2E/fixtures/bloomTest.ts +++ b/src/BloomE2E/fixtures/bloomTest.ts @@ -23,6 +23,9 @@ import { type ICollectionSpec, } from "./launchBloom"; import { chromium } from "@playwright/test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as Path from "node:path"; import { describeProblems, startProblemDialogWatcher, @@ -90,8 +93,21 @@ interface IBloomWorkerFixtures { interface IBloomTestFixtures { /** Fails the test when Bloom raised a problem dialog while it ran. Runs automatically. */ failOnBloomProblem: void; + /** + * When a test fails, keeps what explains it beside Playwright's own trace and screenshot: Bloom's + * log, and a copy of the collection as Bloom left it. Runs automatically. + */ + keepEvidenceOnFailure: void; } +/** + * Where the Bloom under test writes its log. Bloom logs to %TEMP%\SIL\Bloom\Log.txt whatever + * folder its settings are in, so this is the developer's or the runner's temp folder, and a run's + * successive Blooms overwrite one another there: what is there when a test fails is the log of the + * Bloom that was running. + */ +const BLOOM_LOG_PATH = Path.join(os.tmpdir(), "SIL", "Bloom", "Log.txt"); + // How long we wait for Bloom's WebView2 to expose the shell document after the HTTP server is up. // The first navigation after launch is slow: WebView2 starts, the bundle loads, and React mounts. const SHELL_READY_TIMEOUT_MS = 90000; @@ -321,8 +337,71 @@ export const test = base.extend({ await use(bloomApp.page); }, + // Torn down after every other test-scoped fixture (failOnBloomProblem depends on it, so it is + // set up first), so a failure raised by one of them is still seen here. Everything goes under + // testInfo.outputDir, which is test-results//, the folder CI already uploads on failure. + // + // The collection folder is copied rather than described because the failures this is for are + // the ones where Bloom's state on disk disagrees with what the test saw: a title typed on the + // cover that the collection never learned (AUTOMATION-DEBT.md). The book's HTML and meta.json + // say which side lost it. + // + // The user-settings folder is deliberately NOT kept. These artifacts are public (the repository + // is), and user.config is where a Bloom that signed in to Bloom Library for real + // (helpers/bloomLibraryAccount.ts) saves its session token and account, and where the next + // secret-shaped setting would land too. Redacting known names would protect only against the + // ones we thought of. helpers/userSettings.ts reads that file for a test while it runs instead. + keepEvidenceOnFailure: [ + async ({ bloomApp }, use, testInfo) => { + await use(); + if (testInfo.status === testInfo.expectedStatus) return; + // Bloom may still be writing; a moment lets its last save land in the copy. + await delay(1000); + // Best effort throughout: this runs on a test that has already failed, and a file + // Bloom still holds open must cost only that file, never the rest of the evidence or + // a second error on top of the real one. Each step is contained on its own, and the + // copy filter really opens each file, because on Windows accessSync checks attributes, + // not whether another process has the file locked. + try { + if (fs.existsSync(BLOOM_LOG_PATH)) + await testInfo.attach("bloom-log", { + body: fs.readFileSync(BLOOM_LOG_PATH), + contentType: "text/plain", + }); + } catch (error) { + console.warn(`Could not keep Bloom's log: ${error}`); + } + try { + fs.cpSync( + bloomApp.collectionDir, + testInfo.outputPath("collection"), + { + recursive: true, + errorOnExist: false, + filter: (source) => { + if (fs.statSync(source).isDirectory()) return true; + try { + fs.closeSync(fs.openSync(source, "r")); + return true; + } catch { + console.warn( + `Left out of the collection copy (locked?): ${source}`, + ); + return false; + } + }, + }, + ); + } catch (error) { + console.warn(`Could not copy the collection folder: ${error}`); + } + }, + { auto: true }, + ], + failOnBloomProblem: [ - async ({ problemDialogWatcher }, use) => { + async ({ problemDialogWatcher, keepEvidenceOnFailure }, use) => { + void keepEvidenceOnFailure; // Discard anything raised before this test started, so one test's problem is not // reported against the next. problemDialogWatcher.takeProblems(); diff --git a/src/BloomE2E/helpers/fontChooser.ts b/src/BloomE2E/helpers/fontChooser.ts index f819fa1c2d19..5f883acb719d 100644 --- a/src/BloomE2E/helpers/fontChooser.ts +++ b/src/BloomE2E/helpers/fontChooser.ts @@ -403,7 +403,12 @@ export async function showFontDetails(page: Page): Promise { resolve(text); }); }); - await icon.click({ timeout: 30000 }); + // The pane re-renders while the mouse rests on the list (on the nightly runner the icon was + // "not stable", then detached, for the whole 30 seconds of one click attempt), so try the click + // afresh, against the icon as it is now, rather than waiting on one that is being replaced. + await expect(async () => { + await icon.click({ timeout: 3000 }); + }).toPass({ timeout: 30000 }); return message; } diff --git a/src/BloomE2E/helpers/userSettings.ts b/src/BloomE2E/helpers/userSettings.ts index 41d58522d442..9cf1f7bc8b09 100644 --- a/src/BloomE2E/helpers/userSettings.ts +++ b/src/BloomE2E/helpers/userSettings.ts @@ -29,14 +29,29 @@ export async function getUserSettingsFolder(page: Page): Promise { * This reads the disk rather than asking Bloom, because what a test usually wants to know is what * reached the file: that is what the next Bloom to use the folder starts from. Bloom writes some * settings a moment after they change (the zoom two seconds after the last change), so poll. + * + * Bloom holds the file exclusively for a moment while it saves, and reading it then fails with + * EBUSY (seen on the nightly runner, 2026-09-10), so a busy file is retried for a few seconds. */ -export function readSavedUserSetting( +export async function readSavedUserSetting( folder: string, name: string, -): string | undefined { +): Promise { const file = Path.join(folder, "user.config"); if (!fs.existsSync(file)) return undefined; - const xml = fs.readFileSync(file, "utf8"); + let xml = ""; + const deadline = Date.now() + 5000; + for (;;) { + try { + xml = fs.readFileSync(file, "utf8"); + break; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if ((code !== "EBUSY" && code !== "EPERM") || Date.now() > deadline) + throw error; + await new Promise((resolve) => setTimeout(resolve, 200)); + } + } const match = new RegExp( `]*>\\s*([^<]*)`, ).exec(xml); diff --git a/src/BloomE2E/tests/user-settings-isolation.spec.ts b/src/BloomE2E/tests/user-settings-isolation.spec.ts index 2a7b88e0cfea..aa36b0659230 100644 --- a/src/BloomE2E/tests/user-settings-isolation.spec.ts +++ b/src/BloomE2E/tests/user-settings-isolation.spec.ts @@ -42,7 +42,10 @@ test.describe("a test's Bloom keeps its user settings to itself", () => { // Bloom accepted the license for us at startup (there is nobody to click Accept) and saved // that, so the folder already holds a user.config, and this is what is in it. expect( - readSavedUserSetting(bloomApp.userSettingsDir, "LicenseAccepted"), + await readSavedUserSetting( + bloomApp.userSettingsDir, + "LicenseAccepted", + ), ).toBe("True"); }); @@ -79,7 +82,7 @@ test.describe("a test's Bloom keeps its user settings to itself", () => { bloomApp, }) => { test.setTimeout(300000); - const savedZoom = readSavedUserSetting( + const savedZoom = await readSavedUserSetting( bloomApp.userSettingsDir, "PageZoom", ); @@ -96,8 +99,8 @@ test.describe("a test's Bloom keeps its user settings to itself", () => { bloomApp.userSettingsDir, ), ).toBe(true); - expect(readSavedUserSetting(bloomApp.userSettingsDir, "PageZoom")).toBe( - savedZoom, - ); + expect( + await readSavedUserSetting(bloomApp.userSettingsDir, "PageZoom"), + ).toBe(savedZoom); }); });