diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 25bbc75c8999..70a7418c0e34 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -338,12 +338,29 @@ pnpm exec playwright test tests/workspace-tabs.spec.ts # one file pnpm exec playwright test -g "switching workspace tabs" # one test by title ``` -A run opens a real Bloom window; that is expected. It needs a built `Bloom.exe` under -`output/{Debug,Release}/{x64,AnyCPU,}/` (build it yourself; see "Build Bloom whenever it -helps") and the inputs at `output/testing-inputs`. Point +A run needs a built `Bloom.exe` under `output/{Debug,Release}/{x64,AnyCPU,}/` (build it yourself; +see "Build Bloom whenever it helps") and the inputs at `output/testing-inputs`. Point `BLOOM_TESTING_INPUTS_DIR` at a bloom-testing-inputs checkout to use your own in-progress collections instead of the pinned ones. +The launched Bloom serves its React UI from the built `output/browser`, so **an edit to a `.tsx` +file does not reach a run until that bundle is rebuilt.** To test the working tree instead, start +a dev server and name its port in `BLOOM_E2E_VITE_PORT`; the fixture passes `--vite-port` and +Bloom loads every React control from it. Set `PORT` as well as `--port`, or the dev server's +HMR and React-Refresh URLs still point at 5173 and the page fails to load its entry module. + +```bash +PORT=5173 pnpm exec vite --port 5173 --strictPort # in src/BloomBrowserUI +BLOOM_E2E_VITE_PORT=5173 pnpm test # in src/BloomE2E +``` + +**Use 5173, and set the variable.** The page list and the toolbox write `http://localhost:5173` +into their own imports, so on any other port those two frames load nothing and come up empty, +which reads as the feature being missing. And leaving the variable unset does not mean "no dev +server": a dev build of Bloom probes 5173 by itself, so an unset variable and a server elsewhere +means the run quietly tests the built bundle, however old it is. Stop a Bloom that already holds +5173 rather than moving the dev server. See AUTOMATION-DEBT.md. + `.github/workflows/nightly.yml` does not run this suite yet. The step it will need is the same `pnpm test` in that folder, after the Release build and the testing-inputs fetch that the visual-regression job already does. diff --git a/.github/skills/bloom-automation/bloomProcessCommon.mjs b/.github/skills/bloom-automation/bloomProcessCommon.mjs index 21d2784acc66..db8d7735ee7b 100644 --- a/.github/skills/bloom-automation/bloomProcessCommon.mjs +++ b/.github/skills/bloom-automation/bloomProcessCommon.mjs @@ -42,6 +42,28 @@ export const requireTcpPortOption = (optionName, value) => { return port; }; +/** + * A process id given on the command line, as a positive integer. + * + * This throws rather than returning undefined, because the caller that wants a process id wants + * to act on exactly that process. A killer script that read a malformed id as "no id given" would + * fall through to whatever its no-target default is, and killBloomProcess.mjs's default is to kill + * every Bloom the worktree owns. + */ +export const requireProcessIdOption = (optionName, value) => { + const normalized = value === undefined ? "" : String(value).trim(); + const processId = /^\d+$/.test(normalized) + ? toPositiveInteger(normalized) + : undefined; + if (!processId) { + throw new Error( + `${optionName} must be a positive integer process id. Received: ${value}`, + ); + } + + return processId; +}; + export const requireOptionValue = (args, index, optionName) => { const value = args[index + 1]; if (!value || value.startsWith("--")) { diff --git a/.github/skills/bloom-automation/bloomProcessStatus.mjs b/.github/skills/bloom-automation/bloomProcessStatus.mjs index cc3df6e285e3..b6b1345c1800 100644 --- a/.github/skills/bloom-automation/bloomProcessStatus.mjs +++ b/.github/skills/bloom-automation/bloomProcessStatus.mjs @@ -13,6 +13,18 @@ import { toWorkspaceTabsEndpoint, } from "./bloomProcessCommon.mjs"; +const usage = `Report the Bloom and launcher processes this machine is running. + + node bloomProcessStatus.mjs [options] + + --help, -h Print this and exit. + --json Report as JSON. + --running-bloom Also probe each running Bloom's own HTTP server. + --repo-root The worktree to judge instances against (default: this checkout). + --http-port Report the instance whose server answers on this port. + +This script only reads; it never kills anything.`; + const parseArgs = () => { const args = process.argv.slice(2); const options = { @@ -25,6 +37,11 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + if (arg === "--help" || arg === "-h") { + console.log(usage); + process.exit(0); + } + if (arg === "--json") { options.json = true; continue; @@ -36,7 +53,11 @@ const parseArgs = () => { } if (arg === "--repo-root") { - options.repoRoot = args[i + 1] || options.repoRoot; + // A required value, checked the same way as every other option's. Taking + // `args[i + 1]` and falling back to the default would swallow the next flag as + // this option's value. killBloomProcess.mjs has the same parser, where that + // mistake is destructive. + options.repoRoot = requireOptionValue(args, i, "--repo-root"); i++; continue; } @@ -57,6 +78,10 @@ const parseArgs = () => { ); continue; } + + // An unknown flag is a mistake, and silently ignoring it hides it. + console.error(`Unknown option ${arg}.\n\n${usage}`); + process.exit(2); } return options; diff --git a/.github/skills/bloom-automation/killBloomProcess.mjs b/.github/skills/bloom-automation/killBloomProcess.mjs index 7c97dde452bc..20b483ae161c 100644 --- a/.github/skills/bloom-automation/killBloomProcess.mjs +++ b/.github/skills/bloom-automation/killBloomProcess.mjs @@ -7,9 +7,37 @@ import { killProcessIds, normalizeBloomInstanceInfo, requireOptionValue, + requireProcessIdOption, requireTcpPortOption, } from "./bloomProcessCommon.mjs"; +const usage = `Kill the Bloom.exe (and dotnet.exe BloomExe.csproj) processes of this worktree. + + node killBloomProcess.mjs [options] + + --help, -h Print this and exit without killing anything. + --json Report what was killed as JSON. + --only-mismatched Kill only instances whose repo root is not this worktree. + --repo-root The worktree to judge instances against (default: this checkout). + --http-port Kill the instance whose server answers on this port. + --pid Kill this process and the Bloom processes in its chain. + --watch-pid Kill this launcher/watch process and its Bloom processes. + +With no --http-port, --pid or --watch-pid, this kills EVERY Bloom this worktree owns.`; + +// Print the usage and exit 0 without killing anything, or reject an unknown flag with a non-zero +// exit. Reading the usage first must never be the dangerous move: this script used to ignore +// --help and go straight to its destructive default (AUTOMATION-DEBT.md, "Automation helper +// scripts run destructive defaults on unknown flags"). +const exitWithUsage = (unknownArgument) => { + if (unknownArgument) { + console.error(`Unknown option ${unknownArgument}.\n\n${usage}`); + process.exit(2); + } + console.log(usage); + process.exit(0); +}; + const parseArgs = () => { const args = process.argv.slice(2); const options = { @@ -24,6 +52,10 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + if (arg === "--help" || arg === "-h") { + exitWithUsage(); + } + if (arg === "--json") { options.json = true; continue; @@ -35,7 +67,11 @@ const parseArgs = () => { } if (arg === "--repo-root") { - options.repoRoot = args[i + 1] || options.repoRoot; + // A required value, checked the same way as every other option's. Taking + // `args[i + 1]` and falling back to the default would swallow the NEXT FLAG as + // this option's value, so `--repo-root --pid 123` would name no target at all and + // reach the default that kills every Bloom this worktree owns. + options.repoRoot = requireOptionValue(args, i, "--repo-root"); i++; continue; } @@ -58,25 +94,40 @@ const parseArgs = () => { } if (arg === "--pid") { - options.pid = Number(args[i + 1]); + options.pid = requireProcessIdOption( + "--pid", + requireOptionValue(args, i, "--pid"), + ); i++; continue; } if (arg.startsWith("--pid=")) { - options.pid = Number(arg.slice("--pid=".length)); + options.pid = requireProcessIdOption( + "--pid", + arg.slice("--pid=".length), + ); continue; } if (arg === "--watch-pid") { - options.watchPid = Number(args[i + 1]); + options.watchPid = requireProcessIdOption( + "--watch-pid", + requireOptionValue(args, i, "--watch-pid"), + ); i++; continue; } if (arg.startsWith("--watch-pid=")) { - options.watchPid = Number(arg.slice("--watch-pid=".length)); + options.watchPid = requireProcessIdOption( + "--watch-pid", + arg.slice("--watch-pid=".length), + ); + continue; } + + exitWithUsage(arg); } return options; @@ -85,8 +136,14 @@ const parseArgs = () => { const options = parseArgs(); const processState = classifyProcesses(options.repoRoot); const processIds = new Set(); +// Whether the caller named a target, not whether the value we parsed from it is usable. The +// two are the same now that every target option is validated at parse time, and this says the +// intended thing: a caller who asked for one process must never reach the default that kills +// every Bloom this worktree owns. const exactTargetRequested = - !!options.httpPort || !!options.pid || !!options.watchPid; + options.httpPort !== undefined || + options.pid !== undefined || + options.watchPid !== undefined; let targetedInstance; let exactTargetResolutionError; diff --git a/.github/skills/bloom-automation/launcherControl.mjs b/.github/skills/bloom-automation/launcherControl.mjs index c614f481079a..fcfa614ccba1 100644 --- a/.github/skills/bloom-automation/launcherControl.mjs +++ b/.github/skills/bloom-automation/launcherControl.mjs @@ -43,6 +43,17 @@ const actionNames = [ "--ensure-running", ]; +const usage = `Command the go.sh launcher for this worktree. + + node launcherControl.mjs [options] + + actions: ${actionNames.join(", ")} + options: --json, --wait-ready, --repo-root , --timeout-ms + --help, -h Print this and exit without commanding anything. + + --restart rebuilds and relaunches; --quit-bloom stops Bloom and leaves the launcher; + --shutdown stops Bloom, the launcher, and Vite.`; + const parseArgs = () => { const args = process.argv.slice(2); const options = { @@ -56,6 +67,13 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + // Print the usage and stop, before any action can run. Asking a destructive script how to + // use it must never be the destructive move. + if (arg === "--help" || arg === "-h") { + console.log(usage); + process.exit(0); + } + if (actionNames.includes(arg)) { if (options.action) { throw new Error( diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 1580914ccfd6..d0c53b212d0b 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -1,10 +1,10 @@ # Nightly build + full test run of everything on master. # # This is a health check, not a release: it builds the front-end and the C# solution and -# runs all four test suites — front-end vitest, C# NUnit, the BloomE2E suite and the visual -# regression suite — but produces no installer, does no signing, and publishes nothing. It -# exists to catch breakage that the PR checks miss — e.g. tests excluded from PR runs, or rot -# from dependency/runner drift — on a predictable cadence. +# runs all five test suites — front-end vitest, C# NUnit, the BloomE2E suite, the visual +# regression suite, and the component-tester Playwright suite — but produces no installer, does +# no signing, and publishes nothing. It exists to catch breakage that the PR checks miss — e.g. +# tests excluded from PR runs, or rot from dependency/runner drift — on a predictable cadence. # # Each suite publishes its own check run / job-summary section, so the commit shows four # independent results rather than one merged total. See the "Test reports" steps at the end. @@ -54,6 +54,10 @@ on: description: "Visual-regression suite (drives a real Bloom)" type: boolean default: true + run_component_tests: + description: "Component-tester suite (Playwright against the Vite harness)" + type: boolean + default: true # Don't stack nightlies: if a manual run overlaps the scheduled one, let the first finish. concurrency: @@ -81,6 +85,7 @@ jobs: RUN_CSHARP_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_csharp_tests }} RUN_E2E_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_e2e_tests }} RUN_VISUAL_REGRESSION: ${{ github.event_name != 'workflow_dispatch' || inputs.run_visual_regression }} + RUN_COMPONENT_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_component_tests }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -110,6 +115,7 @@ jobs: src/content/pnpm-lock.yaml src/BloomE2E/pnpm-lock.yaml src/BloomVisualRegressionTests/pnpm-lock.yaml + src/BloomBrowserUI/react_components/component-tester/pnpm-lock.yaml # ----- Dependencies (mirrors init.sh) ----- @@ -447,6 +453,50 @@ jobs: retention-days: 4 if-no-files-found: ignore + # ----- Component-tester tests ----- + # src/BloomBrowserUI/react_components/component-tester renders one React component at a + # time in a Vite dev server and drives it with Playwright (the *.uitest.ts files beside + # each component). Nothing ran these until now, which is how the harness sat broken for + # weeks — a React 17 pin plus a config bug — and it would rot again silently + # (AUTOMATION-DEBT.md, "The component-tester Playwright suites are not in CI"). + # + # This is the component config only (playwright.config.ts, which the package's own + # `pnpm test` uses). The sibling playwright.bloom-exe.config.ts attaches over CDP to a + # Bloom the developer already has running, so it needs the src/BloomE2E launch fixture + # before it can run unattended; its specs are excluded by that config's testIgnore. + # + # It needs neither build: the harness serves the components from its own Vite dev + # server, which playwright.config.ts starts as its webServer. So this group depends on + # nothing above it, and runs even when the builds failed. + - name: Set up component-tester tests + id: setup_component_tests + if: ${{ !cancelled() && env.RUN_COMPONENT_TESTS == 'true' }} + working-directory: src/BloomBrowserUI/react_components/component-tester + shell: bash + run: | + pnpm install --frozen-lockfile + pnpm exec playwright install chromium + + # Playwright takes its junit path from PLAYWRIGHT_JUNIT_OUTPUT_NAME, not from a CLI + # flag: it has no --outputFile (that is vitest's). The path is relative to the working + # directory, so it climbs back to the repo root's output/Tests like the other suites. + # + # --timeout raises the per-test 30s of playwright.config.ts, which is a developer's + # number: it assumes a dev server that has already transformed the module graph. Every + # CI run starts cold, and the first request for a component pays for transforming that + # whole graph. Locally, a cold first run failed 12 tests on `page.goto` timing out + # while the warm re-run of the same suite passed 142 in 58 seconds. So give the runner + # room rather than reporting a cold start as a broken component. A passing test still + # returns as soon as it passes, so this costs a green run nothing. + - name: Run component-tester tests + id: component_tests + if: ${{ !cancelled() && steps.setup_component_tests.outcome == 'success' }} + working-directory: src/BloomBrowserUI/react_components/component-tester + shell: bash + env: + PLAYWRIGHT_JUNIT_OUTPUT_NAME: ../../../../output/Tests/component-tester-junit.xml + run: pnpm test --timeout=120000 --reporter=list,junit + # ----- Test reports, one per suite ----- # Each suite gets its OWN invocation of the publish action, and therefore its own # check run on the commit and its own section in the job summary: pass/fail/skip @@ -512,6 +562,15 @@ jobs: action_fail_on_inconclusive: true files: output/Tests/visual-regression-junit.xml + - name: Publish component-tester test results + if: ${{ !cancelled() && steps.component_tests.outcome != 'skipped' }} + uses: EnricoMi/publish-unit-test-result-action/windows@d0a4676d0e0b938bc201470d88276b7c74c712b3 # v2.24.0 + with: + check_name: "Nightly tests: component-tester (Playwright)" + comment_mode: "off" + action_fail_on_inconclusive: true + files: output/Tests/component-tester-junit.xml + - name: Upload test results if: ${{ always() }} uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -525,4 +584,5 @@ jobs: output/Tests/vitest-junit.xml output/Tests/e2e-junit.xml output/Tests/visual-regression-junit.xml + output/Tests/component-tester-junit.xml retention-days: 14 diff --git a/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx b/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx index b4bc47669e94..7b7f317dc699 100644 --- a/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx +++ b/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx @@ -46,7 +46,9 @@ export const StyleAndFontTable: React.FunctionComponent<{ function closeDialogAndJumpToPage(pageId: string) { props.closeDialog(); - postString("editView/jumpToPage", pageId); + // report: false — the edit tab declines a jump it cannot do (it is mid-save), and that + // is not something to raise a problem report about. See EditingModel.JumpToPage. + postString("editView/jumpToPage", pageId, false); } return ( diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx index 23a110a078eb..37004f382a44 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx @@ -567,9 +567,13 @@ function renderPageLayoutMenu(page: HTMLElement): void { ); // Persist the newly created custom layout state so a later switch back // to standard has matching server-side state to work from. + // report: false — the edit tab declines a jump it cannot do (it is mid-save), and + // that is not something to raise a problem report about. See + // EditingModel.JumpToPage. await postString( "editView/jumpToPage", page.getAttribute("id")!, + false, ); renderPageLayoutMenu(page); } else if (selection === "custom" && response) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts new file mode 100644 index 000000000000..403924d6a7b8 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts @@ -0,0 +1,89 @@ +// The one list of the tools that belong in the Edit tab's toolbox. +// +// ToolboxRoot renders a section only for a tool that is in the master tool list, and a tool gets +// there by being registered. In the running app that used to happen as a side effect of loading +// toolboxBootstrap.ts, which also renders its own toolbox root and assigns window.toolboxBundle. +// A test harness cannot afford those side effects, so it duplicated the list with a "keep in +// sync" comment. This module is the list, and nothing else: importing it registers nothing, and +// both toolboxBootstrap.ts and react_components/ToolboxRootTestHarness call the function below. +// (AUTOMATION-DEBT.md: "Toolbox tool registration is a side effect of toolboxBootstrap".) + +import { ITool, ToolBox, getMasterToolList } from "./toolbox"; +import { + kCanvasToolId, + kGameToolId, + kMotionToolId, + kMusicToolId, +} from "./toolIds"; +import { DecodableReaderTool } from "./readers/decodableReader/decodableReaderTool"; +import { LeveledReaderTool } from "./readers/leveledReader/leveledReaderTool"; +import { MusicToolAdaptor } from "./music/musicToolControls"; +import { ImpairmentVisualizerAdaptor } from "./impairmentVisualizer/impairmentVisualizer"; +import { MotionTool } from "./motion/motionTool"; +import TalkingBookTool from "./talkingBook/talkingBookTool"; +import { SignLanguageTool } from "./signLanguage/signLanguageTool"; +import { ImageDescriptionAdapter } from "./imageDescription/imageDescription"; +import { CanvasTool } from "./canvas/canvasTool"; +import { GameTool } from "./games/GameTool"; +import { SettingsTool } from "./settings/settingsTool"; + +/** + * Make the one instance of each toolbox class and register it with the master toolbox. The + * imports above also serve to ensure that each tool's code is part of the bundle. + * + * Calling this twice registers nothing the second time, and makes no second instance either. See + * registerOnce for both. + */ +export function registerAllToolboxTools(): void { + registerOnce("decodableReader", () => new DecodableReaderTool()); + registerOnce("leveledReader", () => new LeveledReaderTool()); + registerOnce(kMusicToolId, () => new MusicToolAdaptor()); + registerOnce( + "impairmentVisualizer", + () => new ImpairmentVisualizerAdaptor(), + ); + registerOnce(kMotionToolId, () => new MotionTool()); + registerOnce("talkingBook", () => new TalkingBookTool()); + registerOnce("signLanguage", () => new SignLanguageTool()); + registerOnce( + ImageDescriptionAdapter.kToolID, + () => new ImageDescriptionAdapter(), + ); + registerOnce(kCanvasToolId, () => new CanvasTool()); + registerOnce(kGameToolId, () => new GameTool()); + registerOnce("settings", () => new SettingsTool()); +} + +/** + * Register one tool, unless the master list already has a tool of that id. + * + * ToolBox.registerTool is a bare push with no check for duplicates, so something has to do the + * check. It reads the shared master list rather than a flag in this module because a caller that + * is a React-Refresh boundary re-executes its own module during `pnpm dev` while masterToolList, + * which lives in toolbox.ts, keeps its entries. A flag here would reset and we would get eleven + * duplicate tools and duplicate accordion sections. + * + * The check is per tool, not "is the list empty": a list holding some other tool is not evidence + * that these eleven are registered, and skipping all of them on that evidence would leave the + * toolbox missing every section. + * + * The caller passes the id and a factory rather than a tool, so that a tool the list already has + * is never constructed. CanvasTool and GameTool each point a static field at the instance being + * constructed (CanvasTool.theOneCanvasTool, GameTool.theOneDragActivityTool), so constructing one + * only to discard it as a duplicate leaves every reader of that field holding a tool the toolbox + * does not know about, and a canvas refresh or a game's state then goes nowhere. + */ +function registerOnce(id: string, makeTool: () => ITool): void { + if (getMasterToolList().some((registered) => registered.id() === id)) + return; + const tool = makeTool(); + // The id above has to be the tool's own, or the duplicate check reads the wrong entry and a + // second call registers the tool again. Some of these tools name their id in a constant and + // some in a string literal, so nothing but this check ties the two together. + if (tool.id() !== id) + throw new Error( + `registerAllToolboxTools names the tool "${id}", but the tool calls itself ` + + `"${tool.id()}".`, + ); + ToolBox.registerTool(tool); +} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts index d7db8cc854c8..71d36a22eaed 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts @@ -10,18 +10,10 @@ import { import { simulateBlurOnPageFrameMouseDown } from "../../utils/menuCloseOnBlur"; import { getTheOneReaderToolsModel } from "./readers/readerToolsModel"; import { ToolBox } from "./toolbox"; -import { DecodableReaderTool } from "./readers/decodableReader/decodableReaderTool"; -import { LeveledReaderTool } from "./readers/leveledReader/leveledReaderTool"; -import { MusicToolAdaptor } from "./music/musicToolControls"; -import { ImpairmentVisualizerAdaptor } from "./impairmentVisualizer/impairmentVisualizer"; -import { MotionTool } from "./motion/motionTool"; import TalkingBookTool from "./talkingBook/talkingBookTool"; -import { SignLanguageTool } from "./signLanguage/signLanguageTool"; -import { ImageDescriptionAdapter } from "./imageDescription/imageDescription"; import "errorHandler"; -import { CanvasTool } from "./canvas/canvasTool"; -import { GameTool, setActiveDragActivityTab } from "./games/GameTool"; -import { SettingsTool } from "./settings/settingsTool"; +import { setActiveDragActivityTab } from "./games/GameTool"; +import { registerAllToolboxTools } from "./registerAllToolboxTools"; // Explicit imports needed so that these symbols are in local scope for the window.toolboxBundle object import { addWordListChangedListener, @@ -120,20 +112,10 @@ $(document).ready(() => { getTheOneToolbox().initialize(); }); -// Make the one instance of each Toolbox class and register it with the master toolbox. -// The imports we need to make these calls possible also serve to ensure that each -// toolbox's code is made part of the bundle. -ToolBox.registerTool(new DecodableReaderTool()); -ToolBox.registerTool(new LeveledReaderTool()); -ToolBox.registerTool(new MusicToolAdaptor()); -ToolBox.registerTool(new ImpairmentVisualizerAdaptor()); -ToolBox.registerTool(new MotionTool()); -ToolBox.registerTool(new TalkingBookTool()); -ToolBox.registerTool(new SignLanguageTool()); -ToolBox.registerTool(new ImageDescriptionAdapter()); -ToolBox.registerTool(new CanvasTool()); -ToolBox.registerTool(new GameTool()); -ToolBox.registerTool(new SettingsTool()); +// Make the one instance of each Toolbox class and register it with the master toolbox. The list +// lives in registerAllToolboxTools.ts, which the test harness imports as well, so there is only +// one list to keep right. +registerAllToolboxTools(); const toolboxBundle: ToolboxBundleApi = { getTheOneToolbox, diff --git a/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts b/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts index 0661561e390f..be7bea07c53d 100644 --- a/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts +++ b/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts @@ -7,6 +7,12 @@ import { test, expect } from "../../component-tester/playwrightTest"; import { setupLinkTargetChooser } from "./test-helpers"; +// How long to let the error message appear or clear. It was 1000ms, which was not enough on a +// loaded machine: two of these tests failed about once per full-suite run and passed when run on +// their own. These are waits for a state rather than sleeps, so the larger number costs a passing +// run nothing. +const kErrorAppearsTimeoutMs = 10000; + test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () => { test("Shows error when URL points to missing book", async ({ page }) => { const context = await setupLinkTargetChooser(page, { @@ -17,7 +23,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = // Wait for the error message to appear const errorMsgElement = await context.errorDisplay.getErrorMessage(); - await errorMsgElement.waitFor({ state: "visible", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "visible", + timeout: kErrorAppearsTimeoutMs, + }); // Error message should be visible const isErrorVisible = await context.errorDisplay.isVisible(); @@ -42,7 +51,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = // Wait for the error message to appear const errorMsgElement = await context.errorDisplay.getErrorMessage(); - await errorMsgElement.waitFor({ state: "visible", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "visible", + timeout: kErrorAppearsTimeoutMs, + }); // Error message should be visible const isErrorVisible = await context.errorDisplay.isVisible(); @@ -114,7 +126,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = // Wait for the error message to appear const errorMsgElement = await context.errorDisplay.getErrorMessage(); - await errorMsgElement.waitFor({ state: "visible", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "visible", + timeout: kErrorAppearsTimeoutMs, + }); // Verify error appears let isErrorVisible = await context.errorDisplay.isVisible(); @@ -124,7 +139,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = await context.urlEditor.setValue("/book/book1"); // Wait for error to disappear - await errorMsgElement.waitFor({ state: "hidden", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "hidden", + timeout: kErrorAppearsTimeoutMs, + }); isErrorVisible = await context.errorDisplay.isVisible(); expect(isErrorVisible).toBe(false); diff --git a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx index a9ce390547f8..53347d60f2f5 100644 --- a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx +++ b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx @@ -1,47 +1,15 @@ import * as React from "react"; import { ToolboxRoot } from "../../bookEdit/toolbox/ToolboxRoot"; -import { ToolBox, getMasterToolList } from "../../bookEdit/toolbox/toolbox"; -import { DecodableReaderTool } from "../../bookEdit/toolbox/readers/decodableReader/decodableReaderTool"; -import { LeveledReaderTool } from "../../bookEdit/toolbox/readers/leveledReader/leveledReaderTool"; -import { MusicToolAdaptor } from "../../bookEdit/toolbox/music/musicToolControls"; -import { ImpairmentVisualizerAdaptor } from "../../bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer"; -import { MotionTool } from "../../bookEdit/toolbox/motion/motionTool"; -import TalkingBookTool from "../../bookEdit/toolbox/talkingBook/talkingBookTool"; -import { SignLanguageTool } from "../../bookEdit/toolbox/signLanguage/signLanguageTool"; -import { ImageDescriptionAdapter } from "../../bookEdit/toolbox/imageDescription/imageDescription"; -import { CanvasTool } from "../../bookEdit/toolbox/canvas/canvasTool"; -import { GameTool } from "../../bookEdit/toolbox/games/GameTool"; -import { SettingsTool } from "../../bookEdit/toolbox/settings/settingsTool"; +import { registerAllToolboxTools } from "../../bookEdit/toolbox/registerAllToolboxTools"; // ToolboxRoot only renders a section for a tool that is in the master tool list, and tools -// put themselves there by being registered. In the running app that happens as a side effect -// of loading toolboxBootstrap. We deliberately do NOT import that module here: besides -// registering tools it also renders its own toolbox root on $(document).ready and assigns -// window.toolboxBundle, which would both duplicate the root this harness renders and -// overwrite the toolboxBundle stub some tests install. So we register the same set of tools -// ourselves. Keep this list in sync with toolboxBootstrap.ts. -// The guard keys off the shared master list rather than a module-local flag on purpose. -// This file is a valid React-Refresh boundary (its only export is a component), so editing -// it during `pnpm dev` re-executes this module without reloading the page. A module-local -// flag would reset to false while masterToolList — which lives in toolbox.ts and is not -// invalidated — kept its entries, and ToolBox.registerTool is a bare push with no dedupe, -// so we would end up with 11 duplicate tools and duplicate accordion sections. -function registerToolsOnce() { - if (getMasterToolList().length > 0) return; - ToolBox.registerTool(new DecodableReaderTool()); - ToolBox.registerTool(new LeveledReaderTool()); - ToolBox.registerTool(new MusicToolAdaptor()); - ToolBox.registerTool(new ImpairmentVisualizerAdaptor()); - ToolBox.registerTool(new MotionTool()); - ToolBox.registerTool(new TalkingBookTool()); - ToolBox.registerTool(new SignLanguageTool()); - ToolBox.registerTool(new ImageDescriptionAdapter()); - ToolBox.registerTool(new CanvasTool()); - ToolBox.registerTool(new GameTool()); - ToolBox.registerTool(new SettingsTool()); -} - -registerToolsOnce(); +// put themselves there by being registered. In the running app that happens when +// toolboxBootstrap.ts calls registerAllToolboxTools. We deliberately do NOT import +// toolboxBootstrap here: besides registering tools it also renders its own toolbox root on +// $(document).ready and assigns window.toolboxBundle, which would both duplicate the root this +// harness renders and overwrite the toolboxBundle stub some tests install. So we call the shared +// registration function, which is side-effect-free to import and safe to call twice. +registerAllToolboxTools(); export const ToolboxRootTestHarness: React.FunctionComponent = () => { return ; diff --git a/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts b/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts index 3d955de146cf..bf34d6f1ce96 100644 --- a/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts +++ b/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts @@ -10,7 +10,7 @@ test.describe("CollectionTopBarControls on Bloom.exe", () => { const connection = await connectToBloomExe(); try { - await clickWorkspaceTab(connection.page, "Collections"); + await clickWorkspaceTab(connection.page, "collection"); await waitForActiveWorkspaceTab("collection"); await expect( diff --git a/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx b/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx index 6fea416fba82..6ca83547343e 100644 --- a/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx +++ b/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx @@ -130,6 +130,10 @@ export const TopBar: React.FunctionComponent = () => {
{ const connection = await connectToBloomExe(); try { - await clickWorkspaceTab(connection.page, "Collections"); + await clickWorkspaceTab(connection.page, "collection"); await waitForActiveWorkspaceTab("collection"); await expect(connection.page.locator("body")).toHaveClass( /collection-mode/, ); - await clickWorkspaceTab(connection.page, "Publish"); + await clickWorkspaceTab(connection.page, "publish"); await waitForActiveWorkspaceTab("publish"); await expect(connection.page.locator("body")).toHaveClass( /publish-mode/, ); - await clickWorkspaceTab(connection.page, "Edit"); + await clickWorkspaceTab(connection.page, "edit"); await waitForActiveWorkspaceTab("edit"); await expect(connection.page.locator("body")).toHaveClass( /edit-mode/, @@ -56,7 +56,7 @@ test.describe("Bloom exe CDP top bar", () => { ) .toBe(true); - await clickWorkspaceTab(connection.page, "Publish"); + await clickWorkspaceTab(connection.page, "publish"); await waitForActiveWorkspaceTab("publish"); await expect @@ -67,7 +67,7 @@ test.describe("Bloom exe CDP top bar", () => { ) .toBe(true); - await clickWorkspaceTab(connection.page, "Edit"); + await clickWorkspaceTab(connection.page, "edit"); await waitForActiveWorkspaceTab("edit"); } finally { await connection.browser.close(); diff --git a/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts b/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts index beac61f2d2f6..3efa65c53db8 100644 --- a/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts +++ b/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts @@ -70,23 +70,20 @@ export const connectToBloomExe = async (): Promise<{ return { browser, page }; }; +/** + * Click a workspace tab in the real top bar. The tab is found by the test id that + * react_components/TopBar/TopBar.tsx puts on it, so this works in any UI language. + * + * Bloom hides the Edit and Publish tabs until a book is selected, so a caller that wants either + * of them must select a book first. + */ export const clickWorkspaceTab = async ( page: Page, - name: WorkspaceTabId extends infer _T - ? "Collections" | "Edit" | "Publish" - : never, + tab: WorkspaceTabId, ): Promise => { - await page.waitForSelector("#main-tabs button", { - timeout: 10000, - }); - - await page.locator("#main-tabs button").filter({ hasText: name }).first(); - - await page - .locator("#main-tabs button") - .filter({ hasText: name }) - .first() - .click(); + const target = page.getByTestId(`workspace-tab-${tab}`); + await target.waitFor({ state: "visible", timeout: 10000 }); + await target.click(); }; export const getWorkspaceTabs = async (): Promise<{ diff --git a/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts b/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts index 70d70241959f..2ca871ca3f03 100644 --- a/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts +++ b/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts @@ -11,7 +11,12 @@ import { // Test timing constants export const kTestOptOutDelaySeconds = 2; -export const kTestOptOutTimeoutMs = kTestOptOutDelaySeconds * 1000 + 2000; // delay + buffer +// The delay the component waits before offering the opt-out button, plus a buffer for the machine +// running the suite. The buffer was 2000ms, which was not enough on a loaded machine: the last +// worker to start would miss the button by a fraction of a second, so one of these tests failed +// about once per full-suite run and passed when run on its own. This is a wait for a state, not a +// sleep, so a longer buffer costs a passing run nothing. +export const kTestOptOutTimeoutMs = kTestOptOutDelaySeconds * 1000 + 8000; // Field helper type for registration form type FieldHelper = { diff --git a/src/BloomBrowserUI/utils/bloomApi.ts b/src/BloomBrowserUI/utils/bloomApi.ts index 82ec01847c26..081ef14c56f4 100644 --- a/src/BloomBrowserUI/utils/bloomApi.ts +++ b/src/BloomBrowserUI/utils/bloomApi.ts @@ -580,7 +580,14 @@ export async function getWithConfigAsync( return wrapAxios(axios.get(getBloomApiPrefix() + urlSuffix, config)); } -export function postString(urlSuffix: string, value: string) { +// Pass report: false for an endpoint whose failure is not worth a problem report, in the same +// spirit as postThatMightNavigate: a request that asks Bloom to do something it may decline for +// reasons of timing, where the user has nothing to fix and nothing to be told. +export function postString( + urlSuffix: string, + value: string, + report: boolean = true, +) { // Match post(): unit tests should not hit Bloom backend endpoints. const isTest = typeof process !== "undefined" && process.env.NODE_ENV === "test"; @@ -594,6 +601,7 @@ export function postString(urlSuffix: string, value: string) { "Content-Type": "text/plain", }, }), + report, ); } diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index f489f060e2d8..ad5b85519731 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -26,14 +26,13 @@ force-foreground trick. Fix direction: move these surfaces to the web UI (the te direction anyway), or expose each dialog's WebView2 on a discoverable CDP port. (Promoted from PAPERCUTS 2026-07-11.) -seen again 2026-09-01 (Test Case ID 169, `publish-text-languages.spec.ts`): the case -turns on which languages the collection has, and there is no API for that either. -`collectionSettings/changeLanguage` is not one: its only listener is the open WinForms -`CollectionSettingsDialog` (`CollectionSettingsDialog.cs:368`), so a POST to it while -the dialog is closed does nothing. The test therefore changes a collection language by -stopping Bloom, rewriting the `.bloomCollection`, and starting again, which is what the -new `bloomApp.restart(betweenStopAndStart)` fixture method is for. Each restart costs -about six seconds and loses whatever the editor had not yet saved. +seen again 2026-09-01, and the collection-languages half of it is now fixed: tests set +the collection's languages through the `e2e/setCollectionLanguages` hook, which does +the same work as clicking OK in the Collection Settings dialog, and +`helpers/collection.ts setCollectionLanguages` wraps it with the restart Bloom still +needs (about six seconds, and it loses whatever the editor had not yet saved). No test +composes `.bloomCollection` XML any more. What remains is the dialog itself: nothing +can drive or screenshot the Settings UI, so the journey test for it cannot be written. seen again 2026-09-01 (Test Case ID 349, `duplicate-page.spec.ts`): "Duplicate Page Many Times..." asks how many copies in `DuplicateManyDialog`, which is `WireUpForWinforms`, so the @@ -86,29 +85,34 @@ refuses to upload at all rather than let an automated click publish under the de account. (Found 2026-09-02 automating Test Case ID 606, `upload-required-items.spec.ts`.) -## Visual-regression cases stop at the first failed comparison - -Each case in `src/BloomVisualRegressionTests/index.spec.ts` throws on the first -mismatch, so later comparisons never capture their images; stale baselines surface one -layer per ~3-minute run (BL-16638 took three accept-and-rerun rounds). Fix direction: -accumulate per-comparison failures and fail once at the end — proven during BL-16638 -(~20–30 lines, confined to the spec). Loop at `index.spec.ts:426`, assertion at -`index.spec.ts:486` (pre-rewire line numbers). (Promoted from PAPERCUTS 2026-07-30.) - -## The top bar has no stable test ids, so tests match on localized text - -`TopBar.tsx` renders the workspace tabs as `` with a localized `` -label and no id, class, or `data-testid`. Two costs, both already paid: the -component-tester's `bloomExeCdp.ts` drives `#main-tabs button`, a selector that exists -nowhere in the source, so `bloom-exe-tabs.uitest.ts` cannot have worked for some time -(it needs a developer's Bloom already running, and nothing runs it in CI — see the entry -below); and `src/BloomE2E/helpers/workspace.ts` has to map tab ids to the English labels -"Collections"/"Edit"/"Publish", so the suite silently only works in an English UI — -which rules out automating the UI-language cases. The same gap makes the fixture -identify Bloom's shell document among the CDP page targets by `[role="tablist"]`, the -only stable marker available. Fix direction: `data-testid="workspace-tab-collection"` -(etc.) on each tab and one on the shell root, and drop the label matching. -(Found 2026-09-01 while scaffolding src/BloomE2E.) +## Which front end the e2e suite tests depends on what else is running + +A launched Bloom serves its React front end either from the built `output/browser` or from a Vite +dev server, and until the fixture is told which, the answer depends on the machine. Three facts, +established 2026-09-01: + +- **There is no way to point Bloom at another folder.** `BloomFileLocator.BrowserRoot` computes + `output/browser` (or `browser`) from where the app sits, with no environment variable and no + command-line option, so the isolated bundle that `build/agent-vite.ps1` writes under + `output/agent//browser` cannot be used by a launched Bloom. +- **A dev server is the supported route, and the fixture now takes it.** Set + `BLOOM_E2E_VITE_PORT=` and `fixtures/launchBloom.ts` passes `--vite-port `, so the suite + tests the working tree with no build at all. Start the server with `PORT` set as well as + `--port`: the port in `vite.config.mts` comes from `process.env.PORT`, so `--port` alone moves + the server but leaves its HMR and React-Refresh URLs pointing at 5173, and the page then fails + to load its entry module. +- **Leaving the variable unset does not mean "no dev server".** A dev build probes port 5173 by + itself (`ReactControl.TryGetActiveViteDevPort`), so a developer's own dev server silently + decides what the suite tests, and Bloom offers no option that means "ignore any dev server" + (`--vite-port` rejects 0, and `ValidateStartupVitePort` requires the port to answer). + +What remains: the fixture neither starts a dev server of its own nor checks that `output/browser` +is newer than `src/BloomBrowserUI`, so a run with the variable unset can still test a stale +bundle without saying so. Fix direction: have the fixture own the choice, either by starting a +dev server on a port of its own choosing, or by refusing to run against an `output/browser` older +than the source and naming the file that is newer. Bloom needs an explicit "no dev server" +option before the second half of that can be trusted. +(Found 2026-09-01 while fixing the top-bar test ids.) seen again 2026-09-01, in the Edit tab's page thumbnail menu: the items `pageThumbnailList.tsx` renders carry no id, class or `data-testid` (all their styling is @@ -124,18 +128,27 @@ taken from the `commandId` the menu already has. rot again silently. Fix direction: a nightly job mirroring the visual-regression one (component config only; the bloom-exe config needs the e2e launch fixture first). (Promoted from PAPERCUTS 2026-07-27.) - -## Toolbox tool registration is a side effect of toolboxBootstrap - -`ToolboxRoot` only renders tools registered via importing `toolboxBootstrap.ts`, which -also renders and clobbers globals, so the test harness duplicates the 11 -`ToolBox.registerTool(...)` calls with a "keep in sync" comment. Fix direction: extract -a side-effect-free `registerAllToolboxTools()` both import — probably folded into the -toolbox React refactor (BL-16608 / PR #8109). Related: one harness test is `test.fixme` -because it asserts on `.subscription-badge` (legacy-toolbox-only) and -`.toolbox-react-header-icon` (never existed); re-enabling it needs a decision on whether -the React header renders badges/icons and what classes to expose. -(Promoted from PAPERCUTS 2026-07-27.) +## One test's tab is the next test's starting state + +`fixtures/bloomTest.ts` launches one Bloom per worker, and Playwright gives every test with the +same fixture options that same worker. So a test that ends on the Edit tab makes the next one +start there, and `tests/workspace-tabs.spec.ts` fails its opening sanity check with +`collection: "enabled"` rather than for any reason to do with tabs. `tests/capture-book-page.spec.ts` +switches back to the collection tab at its end to avoid exactly this, which is a convention no +helper enforces and nothing reminds a new test about. Fix direction: reset the workspace in the +fixture's per-test setup, so the tab a test starts on is not a matter of file order. +(Found 2026-09-01, when adding capture-book-page.spec.ts broke workspace-tabs.spec.ts.) + +## One toolbox harness test asserts on classes that do not exist + +`react_components/ToolboxRootTestHarness`'s suite has one `test.fixme` because it asserts on +`.subscription-badge` (which only the legacy toolbox has) and `.toolbox-react-header-icon` (which +never existed). Re-enabling it needs a decision on whether the React toolbox header renders +badges and icons at all, and what classes to expose for them. Fix direction: make that decision +as part of the toolbox React refactor (BL-16608 / PR #8109), then rewrite the assertions against +what the header really renders. +(Was part of a larger entry about toolbox registration, whose other half was fixed 2026-09-01 by +extracting `bookEdit/toolbox/registerAllToolboxTools.ts`.) ## AI-image-editor selectors are an untested cross-repo contract @@ -147,21 +160,19 @@ did NOT drift. Fix direction: stable `data-testid`s on tool tiles and category h in the editor repo, or have it publish its host-harness selectors for import. (Promoted from PAPERCUTS 2026-07-30, BL-16603.) -## Driver-level CDP footguns that the automation library must absorb +## Driver-level CDP footguns the helper layer does not cover yet -Known WebView2/CDP behaviors that every ad-hoc script rediscovers the hard way; the -`src/BloomE2E` helper layer should encode them once: +Known WebView2/CDP behaviors that every ad-hoc script rediscovers the hard way. The screenshot +one is now absorbed by `helpers/screenshot.ts` (enlarge the window, clip, clear the override, +and time out every CDP request); these two are not, because they are about the scripts around a +capture rather than the capture itself: -- `Page.captureScreenshot` with `captureBeyondViewport:true` hangs (no response, no - error). Working pattern: `Emulation.setDeviceMetricsOverride` large enough for the - whole `.bloom-page`, screenshot with a `clip`, then `clearDeviceMetricsOverride`; - give every CDP request a timeout. - Never `taskkill //IM node.exe //F` to clean up a hung capture — it kills the go.sh vite/dotnet-watch flow and takes Bloom's server down. Kill only the script's own PID. - Reopening a book re-stamps it with freshly compiled xmatter CSS from `output/`, so "before" captures taken after a restart already show the new layout. -(Promoted from PAPERCUTS 2026-07-22.) +(Promoted from PAPERCUTS 2026-07-22; the screenshot item removed 2026-09-01.) ## Visual-regression baselines only match the CI runner @@ -175,15 +186,6 @@ authoring painful. Fix direction: a small per-comparison pixel tolerance, or machine-profile baselines, or render fonts only from Bloom's own WOFF2 set in --e2e mode. (Found 2026-09-01 while verifying the bloom-testing-inputs rewire.) -## Automation helper scripts run destructive defaults on unknown flags - -`node .github/skills/bloom-automation/killBloomProcess.mjs --help` killed the running -Bloom: unknown flags are ignored and the destructive default runs, so "read the usage -first" is itself the dangerous move; sibling scripts may share the shape. Fix -direction: recognize `--help`/`-h` and reject unknown flags in every script that kills -processes — and fold these helpers' jobs into the library's audited launch/teardown -fixture over time. (Promoted from PAPERCUTS 2026-07-24.) - ## Adding a page needs the Add Page dialog, which offers nothing to automate against A book made from a template starts with front and back matter only, because every page @@ -217,6 +219,11 @@ would look like the same flake. Fix direction: have `jumpToPage` queue the reque until the Edit tab is ready, or report that it refused it. (Found 2026-09-01 automating Test Case ID 169.) +Answered for tests, 2026-09-02: `editView/jumpToPage` no longer replies success to a jump +it drops. It refuses the jump and says why, and every helper that changes the page waits for +`waitForEditTabSettled` first. What remains is the Bloom defect itself, in the section "A page +change asked for while the Edit tab is still loading a page can be lost" below. + seen again 2026-09-02 (Test Case ID 72, `derivative-keeps-template-pages.spec.ts`): the same drop hits `addPage`. Every action that saves the page first goes through `EditingModel.SaveThen`, whose "not in the right state" branch does nothing and still @@ -226,15 +233,39 @@ shows a `.bloom-page`. Two page adds in a row therefore lost the second one. helpers no longer act early; the production endpoints still reply success to a request they dropped. +## Bloom names a dropped language differently on CI and on a developer machine (BL-16806) + +`publish-text-languages.spec.ts` used to drop a language by rewriting the `.bloomCollection`, +and it then expected the publish list to show that language's own name for itself, "español". +On CI that assertion failed every time, with `Expected: español Received: espagnol` — French +for Spanish. Everything else about the row was right. + +The cause is known, and BL-16806 is the card that fixes it. Bloom asks LibPalaso for the name of +the dropped language "in" the collection's metadata language, French here. LibPalaso honors such +a request only where it can find a native ICU library, and Bloom ships icu.net but no +`icuuc.dll`, so the answer depends on the machine, not on the run: the CI runner answers +"espagnol", and a developer machine ignores the request and gives the autonym "español". It is +a real difference in what Bloom shows a user, not a test problem. + +No test on this branch covers it any more. The test now drops the language through +`e2e/setCollectionLanguages`, the code the Settings dialog's OK button runs, which keeps the +language's collection name, so the list reads "Spanish" on every machine and the lookup that +differs is never reached. Recorded here so the coverage loss is visible; the defect itself +belongs to BL-16806. (Found 2026-09-01, diagnosed on master 2026-09-02.) + ## Filling a text box directly leaves part of the old text behind A `.bloom-editable` is a CKEditor surface, and Playwright's `fill()` on one leaves a tail of what was there ("Deux" became "eux"), so `typeInGroup` clicks in, selects all, -deletes, and types the new text one key at a time. That is closer to what a person does -and it is reliable, but it is also slow for anything longer than a few words, and no -test can currently clear a box by any faster route. Fix direction: understand what -CKEditor does with a programmatic value change; a supported "set the text of this box" -path would let long text be set at once. +deletes, and then puts the new text in. + +Partly fixed 2026-09-01: the typing half is no longer a key press per character. +`typeInGroup` now inserts the whole string in one call (`keyboard.insertText`), which +CKEditor and Bloom's markup code both handle through the input event it raises, so the +cost of typing no longer grows with the length of the text. What remains is clearing a +box: that still needs a click, Control+A and Delete, because neither `fill()` nor +setting the value leaves CKEditor in a state Bloom then saves correctly. Fix direction: +an `e2e/` hook, or a supported CKEditor path, that sets the text of one box outright. (Found 2026-09-01 automating Test Case ID 169.) ## The page menu offers commands that silently do nothing while a page is loading @@ -287,3 +318,117 @@ the file said `en` again a moment later. The same test has to restore the zoom i that setting is shared too. Fix direction: under `--e2e`, point the settings provider at a per-instance folder (a sibling of the temp collection would do), so a test's Bloom starts from defaults and its changes die with it. +## Typing in a text box raises no key events + +`typeInGroup` puts the whole string in with `keyboard.insertText`, which raises `input` +and nothing else. So no test that types exercises anything in Bloom that listens for +`keydown`, `keypress` or `keyup`, and the `toHaveText` check that follows cannot tell the +difference: the text arrives either way. The pieces of Bloom that watch for a particular +key, rather than for a change to the text, are therefore not covered by any test that +types. + +This is a deliberate trade for speed, taken because a key press per character made every +test that fills a book slower in proportion to how much it typed. Fix direction: a helper +that presses one named key in a box, for the tests whose subject is the key press itself +(Enter splitting a paragraph, Tab moving between boxes, a shortcut), and a note in that +helper that `typeInGroup` is not the way to test those. (Found 2026-09-02, during the +review of the headless work.) + +## A test can attach to a shell document Bloom does not drive + +More than one document in a run carries the workspace root's markup, and therefore the +top bar's `data-testid`, so `fixtures/bloomTest.ts findShellPage` returns whichever the +debugging protocol lists first. When that is not the document Bloom drives, the test is +silently broken rather than failing: its own clicking and typing work, `expect` on what +it typed passes, and every page Bloom loads goes into the document it cannot see. The +symptom is a 60-second wait in `goToPage` for a page Bloom's own log says it showed. +This is why `publish-text-languages.spec.ts` fails perhaps one run in three. + +Fixed for tests, 2026-09-01. `e2e/shellUrl` reports the URL of the document Bloom drives, +and `findShellPage` now takes the page whose URL has the same file name (Bloom and the +debugging protocol escape the rest of the URL differently), re-resolving after +`bloomApp.restart`. It falls back to the first page carrying the marker only when the +endpoint never answers, which is what an old `Bloom.exe` in `output/Debug` does, and says +so. `goToPage`'s failure message names both URLs. Also, under `--e2e` every browser built +on the UI thread shares one CoreWebView2Environment, so those documents live in one +browser process with one debugging listener; before that, each environment was given the +same port number and only the first process to start could listen on it. That sharing is +deliberately limited to the UI thread: an environment belongs to the thread that created +it, and handing it to a browser built on the thread serving an API call hangs that thread. +Publishing a BloomPUB does exactly that, and its preview never appeared. + +What remains: nobody knows why a run has a second workspace root document at all. Bloom +creates one `_workspaceReactControl`. Worth finding, because the duplicate is what makes +the test-side check necessary. +(Found 2026-09-01 while making `jumpToPage` queue a jump.) + +## A page change asked for while the Edit tab is still loading a page can be lost + +A page change that reaches the Edit tab between two announcements of one page load wedges +that tab: it stays in SavePending and refuses every later page change. The caller waits 60 +seconds for a page that never comes, and a user who clicks a page thumbnail in that window +cannot change pages at all until they leave the tab. + +The cause is a page that announces itself to Bloom twice. Bloom's own log, from the run +that found this on 2026-09-02: + +``` +Navigating(f45a2ef8) --> editing(f45a2ef8) +Editing(f45a2ef8) --> savePending() +Ignoring edit() request while in SavePending(f45a2ef8) +``` + +The first announcement moves the tab to Editing. The page change then asks the browser for +the page content so it can save the page it is leaving. The second announcement arrives, is +refused because a save is in flight, and the browser never answers the save request. The +tab stays in SavePending. The same three lines in the order that works read `--> editing`, +`Ignoring edit()`, then `--> savePending`, and the save completes. + +Any request that changes the page does this, not only a jump. The run that found it lost a +language change from `setContentLanguages`, and the Edit tab then held the tab switch that +followed, so a Publish test failed instead. + +A first attempt at this, on 2026-09-02, had `JumpToPage` queue a jump that arrived while +the tab was navigating and release it on the next page-load announcement. That made the +wedge easier to reach rather than harder, because the released jump is itself a page change +arriving in exactly the window above. `JumpToPage` now refuses such a jump and says so, so +the caller can wait and ask again. + +Worked around for tests, 2026-09-02: `e2e/editState` reports what the Edit tab is doing and +how many times the page it shows has announced itself, and every helper that changes the +page waits for `waitForEditTabSettled` first. That keeps the request out of the window. The +wait reads the state twice, 1500 ms apart, and needs Editing both times with the count +unchanged: the state alone reads Editing between the two announcements, when a request +would still be lost, and the gap between them has been seen to reach a second. No test can +see any of this from the DOM, because Bloom leaves the previous page in the frame while it +loads the next one. + +What remains: the Bloom defect itself, which is older than this suite. A user hits it +whenever something asks for a page change in that window, and the Edit tab then stops +responding to page changes altogether. Fix direction: either stop the browser announcing a +page twice, or make the state machine treat a second announcement of the page it is saving +as a reason to discard that save (`DiscardInFlightSave` already exists for BL-16766). Both +change production save behavior, so this needs a decision rather than a quiet fix. + +## A Vite dev server only reaches the whole UI on port 5173 + +`--vite-port` tells Bloom's shell which dev server to load the front end from, but two of the +Edit tab's frames ignore it. `bookEdit/pageThumbnailList/pageThumbnailList.vite-dev.pug` and +`bookEdit/toolbox/toolbox.vite-dev.pug` write `http://localhost:5173/...` into every import +they emit, so on any other port the page list and the toolbox load nothing and come up empty. + +That failure looks like the feature being missing, not like a port problem. A run on port 5199 +failed `duplicate-page.spec.ts` on 2026-09-02 with "waiting for +getByTestId('duplicate-page-button') to be visible", 30 seconds, because `#PageControls` had +never been filled. Nothing in the message points at the dev server. + +The same run showed the second half of it: `BLOOM_E2E_VITE_PORT` was unset, so Bloom fell back +to probing 5173 by itself, found nothing there, and served the built `output/browser` instead. +That bundle was a day old, so the suite silently tested yesterday's front end and reported the +new test id as absent. + +So both halves say the same thing: **serve the dev server on 5173 and set +`BLOOM_E2E_VITE_PORT=5173`.** Fix direction: emit the port into those two pug files the way the +shell gets it, so `--vite-port` means what it says; and give Bloom an option that means "ignore +any dev server", so a run can state which front end it is testing rather than inherit it from +the machine. (Found 2026-09-02.) diff --git a/src/BloomE2E/README.md b/src/BloomE2E/README.md index a21850ea1240..ffd63c26ed84 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -58,12 +58,11 @@ and whatever tab is showing. Most tests need nothing else. | `restart` | Stop Bloom, run an optional callback, start it again on the same collection, and return the new page. | `restart(betweenStopAndStart)` is how a test changes something Bloom only reads at startup. The -collection's languages are the case that needed it: the collection Settings dialog is a WinForms -surface CDP cannot reach, and `collectionSettings/changeLanguage` only answers while that dialog -is open, so the way to change a language is to stop Bloom, rewrite the `.bloomCollection` with -`makeCollectionXml`, and start again. Bloom is killed rather than asked to quit, so leave the -page being edited before restarting or what was typed on it is lost. Use the page `restart` -returns; the old one is closed. +collection's languages are the case that needed it, and a test does not call `restart` itself for +that: `setCollectionLanguages(bloomApp, tags)` posts the new languages to the `e2e/` hook that +writes the `.bloomCollection`, then restarts Bloom and returns the new page. Bloom is killed +rather than asked to quit, so leave the page being edited before restarting or what was typed on +it is lost. Use the page `restart` returns; the old one is closed. Teardown kills the process tree, waits for the HTTP port to go dark, and deletes the temp copy. @@ -93,7 +92,7 @@ real bug in the code under test; read the message and fix it rather than working dialog it opens: `openFormatDialog`, `clickOutsideFormatDialog`, `dragFormatDialog`, `getFormatDialogPlacement`, and the scrolling and zooming that put the gear at the edge of the screen. -- `helpers/collection.ts` — `selectBook`, `waitForCollectionReady`. +- `helpers/collection.ts` — `selectBook`, `waitForCollectionReady`, `setCollectionLanguages`. - `helpers/bookMaking.ts` — make a book, add pages, type into it, read its pages. - `helpers/addPageDialog.ts` — open, read, scroll and close the real Add Page dialog, and add a page through it. Tests that only need a page in their book call `addPage` instead. @@ -104,6 +103,10 @@ real bug in the code under test; read the message and fix it rather than working - `helpers/realClick.ts` — `realClick`, `realClickAt`. Book tiles, Settings, and PREVIEW ignore a synthetic `element.click()`. Never hand-roll `Input.dispatchMouseEvent` in a test; add the gesture here. +- `helpers/screenshot.ts` — `captureCurrentBookPage`, `captureElement`, `readPngSize`. Captures an + element taller than the window. `Page.captureScreenshot` with `captureBeyondViewport` hangs in + WebView2, so this enlarges the window, clips, clears the override, and times out every CDP + request. Never open a CDP session in a test; add the capture here. Two things a test must never do: trigger a native OS dialog (file pickers, the WinForms Image Toolbox, video capture), because Playwright cannot dismiss one and the run hangs; and wait on a @@ -153,6 +156,32 @@ BLOOM_TESTING_INPUTS_DIR=D:/bloom-testing-inputs pnpm test Either way the fixture copies the collection before Bloom opens it, so a run never modifies your inputs. +## Testing a front-end change + +The launched Bloom serves its React UI from the built `output/browser`, so an edit to a `.tsx` +file does not reach a run until somebody rebuilds that bundle. To test the working tree instead, +start a Vite dev server and name its port in `BLOOM_E2E_VITE_PORT`; the fixture then passes +`--vite-port` to Bloom, which loads every React control from the dev server. + +```bash +# In one terminal, in src/BloomBrowserUI. Set PORT as well as --port: the port in +# vite.config.mts comes from process.env.PORT, and --port alone leaves the HMR and +# React-Refresh URLs pointing at 5173, which makes the page fail to load its entry module. +PORT=5173 pnpm exec vite --port 5173 --strictPort + +# In another, in src/BloomE2E +BLOOM_E2E_VITE_PORT=5173 pnpm exec playwright test +``` + +**Use 5173, and set the variable.** The port is not free to choose: the page list and the toolbox +write `http://localhost:5173` into their own imports, so on any other port those two frames load +nothing and come up empty, which reads as the feature being missing rather than as a port +problem. And leaving `BLOOM_E2E_VITE_PORT` unset does not mean "no dev server": a dev build of +Bloom probes 5173 by itself, so an unset variable and a server somewhere else means the run +quietly tests the built bundle, however old it is. Both halves are in AUTOMATION-DEBT.md under +"A Vite dev server only reaches the whole UI on port 5173". + +So stop a Bloom that is already using 5173 before a run, rather than moving the dev server. ## In CI `.github/workflows/nightly.yml` runs the whole suite every night against the Release build it has diff --git a/src/BloomE2E/fixtures/bloomTest.ts b/src/BloomE2E/fixtures/bloomTest.ts index c0fef719f10d..7668f16d492c 100644 --- a/src/BloomE2E/fixtures/bloomTest.ts +++ b/src/BloomE2E/fixtures/bloomTest.ts @@ -86,8 +86,27 @@ const SHELL_READY_TIMEOUT_MS = 90000; const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); -// How we recognize Bloom's shell document: it is the page holding the top bar's tab strip. -const SHELL_MARKER = '[role="tablist"]'; +// How we recognize a candidate for Bloom's shell document: it is a page holding the top bar, which +// carries this test id (react_components/TopBar/TopBar.tsx). +const SHELL_MARKER = '[data-testid="workspace-top-bar"]'; + +// Bloom's own answer to "which document are you driving?". The endpoint exists only under --e2e, +// and reports the URL of the shell browser the C# side sends its commands to +// (E2eTestingApi.HandleGetShellUrl). +const SHELL_URL_ENDPOINT = "e2e/shellUrl"; + +/** + * The file name part of a shell URL, e.g. "bloom45mgnfsl.htm" from + * "http://localhost:8095/bloom/C$3A/.../bloom45mgnfsl.htm?x=1". Bloom names each shell document + * after a temp file, so the file name identifies the document while the query string does not: + * the workspace rewrites its own query as the user moves around (updateWorkspaceUrlParam). + */ +function shellDocumentName(url: string): string { + const withoutQuery = url.split(/[?#]/)[0]; + return withoutQuery + .substring(withoutQuery.lastIndexOf("/") + 1) + .toLowerCase(); +} /** * Connect to the WebView2's CDP endpoint, retrying while it comes up. Bloom's HTTP API reports @@ -113,34 +132,90 @@ async function connectOverCdpWithRetry(cdpPort: number): Promise { } /** - * Find Bloom's shell document among the CDP page targets. We identify it by the top bar's tab - * strip rather than by URL, which also excludes the separately-hosted problem dialog and any - * DevTools target. Polling matters: the WebView2 target exists, as about:blank, for a second or - * two before Bloom navigates it to the shell, and the React top bar mounts later still. + * Find the shell document Bloom is actually driving, among the CDP page targets. + * + * Two tests are applied, and both are needed. The top bar's test id finds the candidates, which + * also excludes the separately-hosted problem dialog and any DevTools target; then Bloom itself is + * asked which document it drives, and only that one is returned. The marker alone is not enough: + * a run can expose more than one workspace-root document, and attaching to an undriven one costs + * an hour, because the test's own clicks work while nothing Bloom loads ever appears (see + * AUTOMATION-DEBT.md). The hook alone is not enough either: it answers "" until the workspace + * view has built its browser, and an older Bloom.exe in output/Debug does not have the endpoint at + * all, so the marker match stays as the fallback for a hook that never answers. + * + * Polling matters: the WebView2 target exists, as about:blank, for a second or two before Bloom + * navigates it to the shell, and the React top bar mounts later still. */ -async function findShellPage(browser: Browser): Promise { +async function findShellPage( + browser: Browser, + httpPort: number, +): Promise { const deadline = Date.now() + SHELL_READY_TIMEOUT_MS; let lastUrls: string[] = []; + let markerOnlyMatch: Page | undefined; + let hookEverAnswered = false; while (Date.now() < deadline) { const pages = browser .contexts() .flatMap((context) => context.pages()) - .filter((page) => !page.url().startsWith("devtools://")); + .filter((page) => !page.url().startsWith("devtools://")) + // Keep only this Bloom's own documents. Another Bloom (another worktree, or the + // developer's) can answer on this CDP port, and its pages carry the same marker and + // answer the same hook, so a page from a different HTTP port must not win. + .filter( + (page) => + !page.url().startsWith("http") || + page.url().includes(`:${httpPort}/`), + ); lastUrls = pages.map((page) => page.url()); for (const page of pages) { - const hasTabs = await page + const hasTopBar = await page .evaluate( (marker) => !!document.querySelector(marker), SHELL_MARKER, ) .catch(() => false); - if (hasTabs) return page; + if (!hasTopBar) continue; + if (!markerOnlyMatch) markerOnlyMatch = page; + const drivenUrl = await page + .evaluate(async (endpoint) => { + const response = await fetch(`/bloom/api/${endpoint}`); + return response.ok ? await response.text() : ""; + }, SHELL_URL_ENDPOINT) + .catch(() => ""); + if (!drivenUrl) continue; + hookEverAnswered = true; + if (shellDocumentName(drivenUrl) === shellDocumentName(page.url())) + return page; } await delay(500); } + // Re-check it before handing it back. It was found on some earlier turn of the loop, possibly + // ninety seconds ago, and Bloom navigates the shell target while it starts up, so by now the + // page may be gone. + if (markerOnlyMatch) { + const stillThere = await markerOnlyMatch + .evaluate( + (marker) => !!document.querySelector(marker), + SHELL_MARKER, + ) + .catch(() => false); + if (!stillThere) markerOnlyMatch = undefined; + } + if (markerOnlyMatch && !hookEverAnswered) { + console.warn( + `Bloom never answered ${SHELL_URL_ENDPOINT}, so the shell document was chosen by ` + + `${SHELL_MARKER} alone. If this test fails oddly, check that output/Debug holds a ` + + `Bloom.exe new enough to have that endpoint.`, + ); + return markerOnlyMatch; + } throw new Error( - `Bloom's WebView2 never exposed a page containing ${SHELL_MARKER} within ` + - `${SHELL_READY_TIMEOUT_MS / 1000}s. Targets seen: ${lastUrls.join(", ") || "none"}.`, + `Bloom's WebView2 never exposed the shell document it is driving within ` + + `${SHELL_READY_TIMEOUT_MS / 1000}s. Pages carrying ${SHELL_MARKER} were ` + + `${markerOnlyMatch ? "found" : "not found"}; ${SHELL_URL_ENDPOINT} ` + + `${hookEverAnswered ? "answered, but named a different document" : "never answered"}. ` + + `Targets seen: ${lastUrls.join(", ") || "none"}.`, ); } @@ -165,7 +240,7 @@ export const test = base.extend({ // rejects a 127.0.0.1 Host header. See helpers/api.ts.) browser = await connectOverCdpWithRetry(launched.cdpPort); const bloomApp: IBloomApp = { - page: await findShellPage(browser), + page: await findShellPage(browser, launched.httpPort), httpPort: launched.httpPort, cdpPort: launched.cdpPort, bloomPid: launched.bloomPid, @@ -179,7 +254,12 @@ export const test = base.extend({ browser = await connectOverCdpWithRetry( launched!.cdpPort, ); - bloomApp.page = await findShellPage(browser); + // Resolve the shell again: the restarted Bloom has a new shell document, + // and the old page object points at a dead target. + bloomApp.page = await findShellPage( + browser, + launched!.httpPort, + ); bloomApp.httpPort = launched!.httpPort; bloomApp.cdpPort = launched!.cdpPort; bloomApp.bloomPid = launched!.bloomPid; diff --git a/src/BloomE2E/fixtures/launchBloom.ts b/src/BloomE2E/fixtures/launchBloom.ts index 7b8e6c566e8c..8f5aad836518 100644 --- a/src/BloomE2E/fixtures/launchBloom.ts +++ b/src/BloomE2E/fixtures/launchBloom.ts @@ -158,6 +158,24 @@ function samePath(a: string, b: string): boolean { const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +/** + * The Vite dev server port the launched Bloom should load its React front end from, or undefined + * to leave the choice to Bloom. + * + * A launched Bloom serves its UI from the built output/browser unless it is told about a dev + * server, so an edit to a .tsx file does not reach the suite until somebody rebuilds the bundle, + * which AGENTS.md reserves for a developer or CI. Set BLOOM_E2E_VITE_PORT= and Bloom loads the + * front end from that dev server instead, so the suite tests the working tree. + * + * Leaving the variable unset is NOT the same as "no dev server". A dev build of Bloom probes port + * 5173 by itself (ReactControl.TryGetActiveViteDevPort), so a developer's own dev server silently + * changes what the suite tests, and Bloom has no option that means "ignore any dev server" + * (--vite-port rejects 0). See AUTOMATION-DEBT.md. + */ +function getViteDevPort(): string | undefined { + return process.env.BLOOM_E2E_VITE_PORT || undefined; +} + /** What common/instanceInfo tells us about a running Bloom. Only the fields we use. */ interface IInstanceInfo { editableCollectionFolder?: string; @@ -363,11 +381,12 @@ async function startBloomOn( // --e2e: skip the DEBUG "attach debugger now" prompt and suppress modal error dialogs. // --automation: let this instance run alongside a Bloom the developer already has open. - const bloomProcess: ChildProcess = execFile(exe, [ - findCollectionFile(collectionDir), - "--e2e", - "--automation", - ]); + const args = [findCollectionFile(collectionDir), "--e2e", "--automation"]; + // --vite-port: serve the React front end from a dev server, so the suite tests the working + // tree rather than a stale output/browser (see getViteDevPort). + const vitePort = getViteDevPort(); + if (vitePort) args.push("--vite-port", vitePort); + const bloomProcess: ChildProcess = execFile(exe, args); let exitStatus: { code: number | null; signal: string | null } | undefined; bloomProcess.stdout?.on("data", (d) => recordOutput(String(d))); bloomProcess.stderr?.on("data", (d) => recordOutput(String(d))); diff --git a/src/BloomE2E/helpers/bookMaking.ts b/src/BloomE2E/helpers/bookMaking.ts index 7a2830f9ae2b..360cf7af56ee 100644 --- a/src/BloomE2E/helpers/bookMaking.ts +++ b/src/BloomE2E/helpers/bookMaking.ts @@ -279,6 +279,9 @@ export async function setContentLanguages( page: Page, tags: string[], ): Promise { + // Each change makes Bloom reload the page, so this is a page-changing request and must not + // arrive while the Edit tab is still loading one. See waitForEditTabSettled. + await waitForEditTabSettled(page); const usage = await apiGetJson( page, "editView/topBar/contentLanguageUsage", @@ -312,7 +315,7 @@ export async function setContentLanguages( ) .toBe(wanted); } - await waitForEditablePage(page); + await waitForEditTabSettled(page); } /** The Edit tab's frame holding the page being edited. Throws if the Edit tab is not showing. */ @@ -368,6 +371,68 @@ export async function waitForEditablePage( .toBe("true"); } +/** What e2e/editState replies with: what the Edit tab is doing. */ +interface IEditTabState { + /** A name from Bloom's State enum: NoPage, Navigating, Editing, SavePending, SavedAndStripped. */ + state: string; + /** The page that state is about, or "". */ + pageId: string; + /** Whether the Edit tab is the tab being shown. */ + visible: boolean; + /** How many times the page now shown has told Bloom its DOM had loaded. */ + pageLoadAnnouncements: number; +} + +/** + * Wait until the Edit tab is settled on a page: showing it, in the Editing state, and done + * announcing that the page has loaded. + * + * Ask this before any request that changes the page. The Edit tab refuses such a request while it + * navigates, and a page announces itself to Bloom more than once, so a request that arrives + * between two of those announcements starts a save that the second announcement leaves unanswered + * (AUTOMATION-DEBT.md: "A page change asked for while the Edit tab is still loading a page can be + * lost"). Waiting for + * the Editing state alone is not enough. Hence the second reading: the tab is settled once the + * count of announcements has stopped rising. + * + * The DOM is no help here. Bloom leaves the previous page in the frame while it loads the next + * one, so a test looking at the frame sees a settled Edit tab throughout. + */ +export async function waitForEditTabSettled( + page: Page, + timeoutMs = 90000, +): Promise { + const editState = () => apiGetJson(page, "e2e/editState"); + await expect + .poll( + async () => { + const first = await editState(); + if (!first.visible || first.state !== "Editing") + return first.state; + // Long enough to cover the gap between two announcements of one page load, which + // has been seen to reach a second. + await page.waitForTimeout(1500); + const second = await editState(); + if ( + second.state !== "Editing" || + second.pageId !== first.pageId + ) + return second.state; + if ( + second.pageLoadAnnouncements !== first.pageLoadAnnouncements + ) + return "Navigating"; + return "Editing"; + }, + { + timeout: timeoutMs, + message: + "The Edit tab never settled on a page, so a request to change pages could be lost.", + }, + ) + .toBe("Editing"); +} + /** One page of the selected book, as e2e/pages reports it. */ export interface IBookPage { /** The page's id, which is what editView/jumpToPage takes. */ @@ -467,7 +532,7 @@ export async function addPage( message: `Bloom never added the "${templatePageLabel}" page(s).`, }) .toBe(before + times); - await waitForEditablePage(page); + await waitForEditTabSettled(page); } /** @@ -491,31 +556,75 @@ export async function getShownPageId(page: Page): Promise { * it is leaving, so text typed into a box reaches the file only once the book moves off that page. */ export async function goToPage(page: Page, pageId: string): Promise { - // The Edit tab drops a jump that arrives while it is still loading a page, so wait for it to - // be showing one before asking for another. - await waitForEditablePage(page); - - // Ask up to three times. Coming back from the Publish tab, the Edit tab can still swallow a - // jump after it looks ready, and asking again costs a few seconds where failing costs the run. + // Wait for the Edit tab first. A jump asked for while it is still loading a page is refused, + // and one asked for between two announcements of one page load wedges it (see + // waitForEditTabSettled). Asking at a moment when the tab acts on the jump at once avoids + // both. + // Then ask, and ask again if the tab was busy after all. The tab answers with an error when + // it cannot jump (EditingModel.JumpToPage), rather than dropping the jump and reporting + // success as it used to, so a refusal is visible here instead of turning into a 60-second + // wait for a page that is not coming. Three tries, because the wait above closes the window + // for a refusal without quite shutting it: the tab can start loading the page list in the + // moment between the wait finishing and this request arriving. + let refusal: unknown; + for (let attempt = 0; attempt < 3; attempt++) { + if (attempt > 0) await waitForEditTabSettled(page); + refusal = undefined; + await apiPost(page, "editView/jumpToPage", pageId, "text/plain").catch( + (error) => { + refusal = error; + }, + ); + if (refusal === undefined) break; + } + if (refusal !== undefined) + throw new Error( + `The Edit tab refused to show page ${pageId} three times running: ${refusal}`, + ); const showing = async () => (await page .frame({ name: "page" }) ?.locator(`.bloom-page[id="${pageId}"]`) .count() .catch(() => 0)) ?? 0; - for (let attempt = 1; attempt <= 3; attempt++) { - await apiPost(page, "editView/jumpToPage", pageId, "text/plain"); - try { - await expect.poll(showing, { timeout: 20000 }).toBe(1); - await waitForEditablePage(page); - return; - } catch { - // fall through and ask again - } + try { + await expect.poll(showing, { timeout: 60000 }).toBe(1); + } catch { + // Say what the frame does hold. "Bloom never showed the page" on its own cannot tell a + // page that never arrived from a page that arrived and was replaced by another one. + const frame = page.frame({ name: "page" }); + const heldPageIds = frame + ? await frame + .locator(".bloom-page") + .evaluateAll((pages) => pages.map((p) => p.id)) + .catch(() => ["(could not read the frame)"]) + : []; + // Read this the forgiving way. A shell document Bloom is not driving is one of the + // failures this message exists to explain, and in exactly that case the request can + // fail. Letting it throw here would replace the whole diagnostic with its own error. + const drivenShell = await apiGet(page, "e2e/shellUrl") + .then((response) => response.body) + .catch((error) => `(could not be read: ${error})`); + // The state the Edit tab is stuck in says which of the two failures this is: a jump that + // wedged the tab leaves it in SavePending or Navigating on the page it already had. + const editState = await apiGet(page, "e2e/editState") + .then((response) => response.body) + .catch((error) => `(could not be read: ${error})`); + throw new Error( + `Bloom never showed page ${pageId} in the Edit tab. ` + + `The Edit tab is at ${editState}. ` + + (frame + ? `The 'page' frame is at ${frame.url()} and holds [${heldPageIds.join(", ")}].` + : `There is no 'page' frame.`) + + ` Bloom drives the shell at ${drivenShell}; ` + + `this test is watching ${page.url()}. If those name different documents, the test ` + + `attached to the wrong one and nothing Bloom does will ever appear (see AUTOMATION-DEBT.md).`, + ); } - throw new Error( - `Bloom never showed page ${pageId} in the Edit tab, after three attempts.`, - ); + // The page being in the frame is not the whole of arriving: Bloom is still setting it up, and + // it re-navigates the page list too. Leave the Edit tab settled, so whatever the test does + // next is not working against a frame that is about to be replaced. + await waitForEditTabSettled(page); } /** @@ -560,7 +669,16 @@ export async function typeInGroup( const box = await clickInGroup(page, groupSelector, languageTag); await box.press("Control+a"); await box.press("Delete"); - if (text) await box.pressSequentially(text); + // One insertion rather than a key press per character: the box has focus, and CKEditor and + // Bloom's own markup code both work from the input event this raises, so the result is the + // same and the cost does not grow with the length of the text. + // + // What this does NOT do is raise keydown, keypress or keyup. So a test that types here does + // not exercise anything in Bloom that listens for a key rather than for input, and the + // assertion below cannot tell the difference. A test whose subject IS a key press needs a + // helper of its own that presses that key. (AUTOMATION-DEBT.md: "Typing in a text box raises + // no key events".) + if (text) await page.keyboard.insertText(text); // Bloom's editor reacts to typing; confirm the box holds what we meant before moving on, so a // later failure cannot be blamed on text that never arrived. await expect(box).toHaveText(text, { timeout: 15000 }); diff --git a/src/BloomE2E/helpers/collection.ts b/src/BloomE2E/helpers/collection.ts index c5abfcbb1351..2380fdfa73f3 100644 --- a/src/BloomE2E/helpers/collection.ts +++ b/src/BloomE2E/helpers/collection.ts @@ -7,6 +7,7 @@ import { expect, type Page } from "@playwright/test"; import { apiGet, apiPost } from "./api"; +import type { IBloomApp } from "../fixtures/bloomTest"; /** * Wait until the editable collection is loaded and its books can be enumerated. Switching to the @@ -45,3 +46,35 @@ export async function selectBook( `&collection-id=${encodeURIComponent(collectionId)}`, ); } + +/** + * Set the collection's languages, and give Bloom back to the test with the change in effect. + * + * `tags` holds one to three language tags, for Language 1, Language 2 and Language 3. Fewer than + * three leaves the rest empty, which is how a collection ends up with no Language 3. + * + * Bloom reads a collection's languages when it opens the collection, so this restarts it, exactly + * as clicking OK in the Collection Settings dialog makes a user reopen the collection. The restart + * kills the process, so leave the page being edited first or what was typed on it is lost, and use + * the page this returns: the old one points at a dead target. + * + * The `e2e/setCollectionLanguages` hook writes the .bloomCollection with Bloom's own code, so + * everything else in the collection's settings survives. See AUTOMATION-DEBT.md for why there is + * no production API for this. + */ +export async function setCollectionLanguages( + bloomApp: IBloomApp, + tags: string[], +): Promise { + if (tags.length < 1 || tags.length > 3) + throw new Error( + `A collection has one to three languages; setCollectionLanguages was given ${tags.length}.`, + ); + await apiPost( + bloomApp.page, + "e2e/setCollectionLanguages", + JSON.stringify(tags), + "application/json", + ); + return bloomApp.restart(); +} diff --git a/src/BloomE2E/helpers/pageList.ts b/src/BloomE2E/helpers/pageList.ts index e64aac438db8..9f0898374d03 100644 --- a/src/BloomE2E/helpers/pageList.ts +++ b/src/BloomE2E/helpers/pageList.ts @@ -11,7 +11,7 @@ import { expect, type Frame, type Page } from "@playwright/test"; import { apiPost } from "./api"; -import { getPages, waitForEditablePage, type IBookPage } from "./bookMaking"; +import { getPages, waitForEditTabSettled, type IBookPage } from "./bookMaking"; /** The Edit tab's frame holding the page thumbnails. Throws if the Edit tab is not showing. */ export function pageListFrame(page: Page): Frame { @@ -57,7 +57,7 @@ async function waitForOneNewPage( }, ) .toBe(before.length + 1); - await waitForEditablePage(page); + await waitForEditTabSettled(page); return added!; } @@ -66,6 +66,9 @@ async function waitForOneNewPage( * return the new page. The copy lands right after the page it was made from. */ export async function duplicatePageWithButton(page: Page): Promise { + // Duplicating saves the page being shown, so this must not be asked for while the Edit tab is + // still loading one. See waitForEditTabSettled. + await waitForEditTabSettled(page); const before = await getPages(page); const button = pageListFrame(page).getByTestId("duplicate-page-button"); await button.waitFor({ state: "visible", timeout: 30000 }); @@ -85,6 +88,9 @@ export async function duplicatePageWithContextMenu( page: Page, pageId: string, ): Promise { + // Duplicating saves the page being shown, so this must not be asked for while the Edit tab is + // still loading one. See waitForEditTabSettled. + await waitForEditTabSettled(page); const before = await getPages(page); const target = thumbnail(page, pageId); await target.waitFor({ state: "visible", timeout: 30000 }); @@ -119,6 +125,9 @@ export async function movePageToSlotOf( pageId: string, targetPageId: string, ): Promise { + // Moving a page saves the page being shown, so this must not be asked for while the Edit tab + // is still loading one. See waitForEditTabSettled. + await waitForEditTabSettled(page); const before = await getPages(page); const targetIndex = before.findIndex((p) => p.id === targetPageId); if (targetIndex < 0 || !before.some((p) => p.id === pageId)) @@ -158,7 +167,7 @@ export async function movePageToSlotOf( message: `Bloom never listed page ${pageId} in the slot of ${targetPageId}.`, }) .toBe(pageId); - await waitForEditablePage(page); + await waitForEditTabSettled(page); } /** @@ -172,6 +181,9 @@ export async function duplicateCurrentPage( page: Page, times = 1, ): Promise { + // Duplicating saves the page being shown, so this must not be asked for while the Edit tab is + // still loading one. See waitForEditTabSettled. + await waitForEditTabSettled(page); const before = (await getPages(page)).length; await apiPost( page, @@ -185,5 +197,5 @@ export async function duplicateCurrentPage( message: "Bloom never added the duplicated page(s).", }) .toBe(before + times); - await waitForEditablePage(page); + await waitForEditTabSettled(page); } diff --git a/src/BloomE2E/helpers/publish.ts b/src/BloomE2E/helpers/publish.ts index f66f7e73f20c..5e846947e8d6 100644 --- a/src/BloomE2E/helpers/publish.ts +++ b/src/BloomE2E/helpers/publish.ts @@ -175,24 +175,36 @@ export async function clickTextLanguage( export async function showBloomPubPreview(page: Page): Promise { await page.locator('[aria-label="refresh preview"]').click(); let player: Frame | undefined; - await expect - .poll( - async () => { - player = page - .frames() - .find((f) => f.url().includes("bloomplayer.htm")); - if (!player) return 0; - return player - .locator('[aria-label="Choose Language"]') - .count() - .catch(() => 0); - }, - { - timeout: 120000, - message: "The BloomPUB preview never showed the book.", - }, - ) - .toBeGreaterThan(0); + try { + await expect + .poll( + async () => { + player = page + .frames() + .find((f) => f.url().includes("bloomplayer.htm")); + if (!player) return 0; + return player + .locator('[aria-label="Choose Language"]') + .count() + .catch(() => 0); + }, + { timeout: 120000 }, + ) + .toBeGreaterThan(0); + } catch { + // Say which of the two steps did not happen: Bloom never gave the player a book to show, + // or the player has one and never finished loading it. Without this the failure looks the + // same either way, and the two have nothing to do with each other. + const frameUrls = page.frames().map((frame) => frame.url()); + throw new Error( + `The BloomPUB preview never showed the book. ` + + (player + ? `bloom-player is loaded at ${player.url()} but never offered the language ` + + `menu, so it never finished showing a book.` + : `No frame of this page is bloom-player, so Bloom never staged the ` + + `publication. Frames: ${frameUrls.join(", ")}.`), + ); + } return player!; } diff --git a/src/BloomE2E/helpers/screenshot.ts b/src/BloomE2E/helpers/screenshot.ts new file mode 100644 index 000000000000..2d1addff2b21 --- /dev/null +++ b/src/BloomE2E/helpers/screenshot.ts @@ -0,0 +1,306 @@ +// Capture an image of one element in Bloom's WebView2, including an element taller than the +// window. +// +// A book page is the case that forces this. A `.bloom-page` in the Edit tab is usually taller than +// the WebView2's window, and the obvious route, `Page.captureScreenshot` with +// `captureBeyondViewport: true`, hangs in WebView2: no response, no error, and the run dies on a +// timeout with nothing to read. The pattern that works, and the one this module encodes, is: +// +// 1. Enlarge the window with Emulation.setDeviceMetricsOverride, big enough for the whole +// element. +// 2. Measure the element only after that, because enlarging the window re-lays out the page. +// 3. Screenshot with a `clip` for the element's box. +// 4. Emulation.clearDeviceMetricsOverride, always, even when the capture failed. A left-over +// override leaves the rest of the run driving a Bloom of the wrong size. +// +// Every CDP request also gets a timeout, because a WebView2 CDP call that never answers otherwise +// stops the run rather than failing it. (AUTOMATION-DEBT.md: "Driver-level CDP footguns that the +// automation library must absorb".) +// +// Tests do not open CDP sessions of their own. If you need a capture this file does not do, add it +// here. + +import type { CDPSession, Locator, Page } from "@playwright/test"; +import { editablePageFrame, waitForEditablePage } from "./bookMaking"; + +/** An element's image, and the size of that image in pixels. */ +export interface IElementImage { + /** The PNG bytes. */ + png: Buffer; + /** The image's width in pixels, read from the PNG itself. */ + width: number; + /** The image's height in pixels, read from the PNG itself. */ + height: number; + /** + * The element's own box, in CSS pixels, at the moment we captured it. The image should be this + * size (to within the rounding CDP does), so a caller can check that it captured the element + * rather than the window. + */ + elementWidth: number; + /** The element's own height in CSS pixels at the moment we captured it. */ + elementHeight: number; +} + +/** The CDP method names Playwright's session accepts. */ +type CdpMethod = Parameters[0]; + +/** How long any one CDP request may take before we call the driver stuck. */ +const CDP_TIMEOUT_MS = 30000; + +// The largest window we will pretend to have. A book page cannot legitimately need more than this, +// and an absurd number here would make WebView2 try to allocate an absurd surface. +const MAX_OVERRIDE_PIXELS = 8000; + +/** + * Capture the `.bloom-page` the Edit tab is showing. + * + * This waits for the Edit tab to have a page with editable text in it first, so a caller does not + * have to; call goToPage (helpers/bookMaking.ts) first to choose which page. + */ +export async function captureCurrentBookPage( + page: Page, + timeoutMs = 90000, +): Promise { + await waitForEditablePage(page, timeoutMs); + const bloomPage = editablePageFrame(page).locator(".bloom-page").first(); + return captureElement(bloomPage, timeoutMs); +} + +/** + * Capture one element as a PNG, whatever its size, and return the bytes with the image's real + * dimensions. + * + * The element may be inside an iframe: the box is measured in the top document's coordinates, + * which is what CDP's clip wants. Fails with a message naming the locator when the element has no + * on-screen box, which is what a collapsed or zero-size container looks like from here. + */ +export async function captureElement( + locator: Locator, + timeoutMs = 30000, +): Promise { + const page = locator.page(); + await locator.waitFor({ state: "visible", timeout: timeoutMs }); + + const session = await page.context().newCDPSession(page); + // The capture's own failure, kept so that a cleanup failure on top of it can carry it as a + // cause instead of hiding it. + let captureError: unknown; + try { + // How big the window has to be for the whole element to be laid out at once. Ask for the + // element's own scroll size, plus where it sits, rather than the document's size: the + // document includes page-list thumbnails and other chrome we are not capturing. + const wanted = await elementExtent(locator, timeoutMs); + // An element bigger than the cap would be clipped to its full box against a window that + // was never made large enough, so the image would be a truncated element rather than a + // failure, and nothing downstream could tell: the box we measure afterwards is measured + // inside the too-small window, so it agrees with the truncated image. Say so instead. + if ( + wanted.right > MAX_OVERRIDE_PIXELS || + wanted.bottom > MAX_OVERRIDE_PIXELS + ) { + throw new Error( + `captureElement cannot capture this element: laying all of it out needs a window ` + + `${Math.ceil(wanted.right)}x${Math.ceil(wanted.bottom)} pixels, and the ` + + `largest this helper will ask WebView2 for is ${MAX_OVERRIDE_PIXELS}. ` + + `Capture a smaller part of it, or raise MAX_OVERRIDE_PIXELS if WebView2 can ` + + `still allocate that.`, + ); + } + const viewport = page.viewportSize(); + const overrideWidth = clampOverride( + Math.max(wanted.right, viewport?.width ?? 0), + ); + const overrideHeight = clampOverride( + Math.max(wanted.bottom, viewport?.height ?? 0), + ); + + await sendWithTimeout(session, "Emulation.setDeviceMetricsOverride", { + width: overrideWidth, + height: overrideHeight, + deviceScaleFactor: 1, + mobile: false, + }); + + // Measure only now. Enlarging the window re-lays out the page, and on a book page it + // genuinely moves things: the Edit tab centres the page in the space it has. + const box = await documentBox(locator, timeoutMs); + + const result = (await sendWithTimeout( + session, + "Page.captureScreenshot", + { + format: "png", + // No captureBeyondViewport: it hangs in WebView2. The override above is what makes + // the whole element fit inside the window instead. + clip: { + x: box.x, + y: box.y, + width: box.width, + height: box.height, + scale: 1, + }, + }, + )) as { data: string }; + + const png = Buffer.from(result.data, "base64"); + const size = readPngSize(png); + return { + png, + width: size.width, + height: size.height, + elementWidth: box.width, + elementHeight: box.height, + }; + } catch (error) { + captureError = error; + throw error; + } finally { + // Always, including after a failed capture: the next test in this worker drives the same + // Bloom, and an emulated 8000-pixel window would make everything it sees wrong. + let clearError: unknown; + await sendWithTimeout( + session, + "Emulation.clearDeviceMetricsOverride", + {}, + ).catch((error) => { + clearError = error; + }); + await session.detach().catch(() => undefined); + + if (clearError !== undefined) { + // The window is still the size we made it, and the rest of this worker's tests would + // measure that Bloom rather than the real one. Fail rather than warn, whether or not + // the capture itself got that far: a warning on standard error is easy to miss in a + // long run, and every later test in this worker is then wrong. + // + // When the capture failed too, that error is the one the test author needs to read, + // so hand it on as the cause rather than let this one replace it. + throw new Error( + `captureElement could not clear the window size override, so the rest of this worker's tests would drive a Bloom of the wrong size: ${clearError}`, + captureError === undefined + ? undefined + : { cause: captureError }, + ); + } + } +} + +/** + * Keep an override within what WebView2 can reasonably allocate, and never ask for zero. + * + * The caller checks the element against MAX_OVERRIDE_PIXELS before calling this, so the cap here + * only ever applies to the viewport's own size. A silently capped element would be captured + * truncated rather than reported. + */ +function clampOverride(pixels: number): number { + return Math.max(1, Math.min(MAX_OVERRIDE_PIXELS, Math.ceil(pixels))); +} + +/** + * How far right and down the element reaches in the top document, counting its own scrollable + * content. This is what the window has to be enlarged to before the element is fully laid out. + */ +async function elementExtent( + locator: Locator, + timeoutMs: number, +): Promise<{ right: number; bottom: number }> { + const box = await requireBoundingBox(locator, timeoutMs); + const scroll = await locator.evaluate((element) => ({ + width: element.scrollWidth, + height: element.scrollHeight, + })); + return { + right: box.x + Math.max(box.width, scroll.width), + bottom: box.y + Math.max(box.height, scroll.height), + }; +} + +/** + * The element's box in the TOP document's coordinates, which is the space CDP's clip is in. + * Playwright reports a box relative to the top document's window, so add that window's scroll. + */ +async function documentBox( + locator: Locator, + timeoutMs: number, +): Promise<{ x: number; y: number; width: number; height: number }> { + const box = await requireBoundingBox(locator, timeoutMs); + const scroll = await locator.page().evaluate(() => ({ + x: window.scrollX, + y: window.scrollY, + })); + return { + x: box.x + scroll.x, + y: box.y + scroll.y, + width: box.width, + height: box.height, + }; +} + +/** The element's on-screen box, or an error saying which element had none. */ +async function requireBoundingBox( + locator: Locator, + timeoutMs: number, +): Promise<{ x: number; y: number; width: number; height: number }> { + const box = await locator.boundingBox({ timeout: timeoutMs }); + if (!box) + throw new Error( + `${locator} is visible but has no bounding box, so there is nothing to capture. ` + + `It may be inside a collapsed or zero-size container.`, + ); + if (box.width < 1 || box.height < 1) + throw new Error( + `${locator} measures ${box.width}x${box.height}, which is too small to capture.`, + ); + return box; +} + +/** + * Send one CDP request, and fail rather than hang when WebView2 never answers. A CDP call with no + * reply is the failure mode this whole module exists to absorb, so it gets its own deadline here + * instead of relying on Playwright's test timeout to notice. + */ +async function sendWithTimeout( + session: CDPSession, + method: TMethod, + params: object, +): Promise { + let timer: NodeJS.Timeout | undefined; + const expired = new Promise((_resolve, reject) => { + timer = setTimeout( + () => + reject( + new Error( + `The CDP request ${method} got no reply within ${CDP_TIMEOUT_MS / 1000}s. ` + + `WebView2 stops answering rather than failing, so treat this as the ` + + `driver being stuck.`, + ), + ), + CDP_TIMEOUT_MS, + ); + }); + try { + // `as never` only because the two arguments are typed as one pair per method, and this + // wrapper is deliberately method-agnostic; the method name itself is still checked. + return await Promise.race([ + session.send(method, params as never), + expired, + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + +/** + * Read a PNG's pixel dimensions out of its header, so a caller can assert on the size of what it + * captured without this package depending on an image library. + */ +export function readPngSize(png: Buffer): { width: number; height: number } { + // 8-byte signature, then a 4-byte length and the "IHDR" tag, then width and height as 32-bit + // big-endian integers. + const signature = "89504e470d0a1a0a"; + if (png.length < 24 || png.subarray(0, 8).toString("hex") !== signature) + throw new Error( + `That is not a PNG: ${png.length} bytes starting ${png.subarray(0, 8).toString("hex")}.`, + ); + return { width: png.readUInt32BE(16), height: png.readUInt32BE(20) }; +} diff --git a/src/BloomE2E/helpers/workspace.ts b/src/BloomE2E/helpers/workspace.ts index 6d6464c7e49d..4a3d1d13a7ba 100644 --- a/src/BloomE2E/helpers/workspace.ts +++ b/src/BloomE2E/helpers/workspace.ts @@ -23,14 +23,11 @@ export interface IWorkspaceTabs { navigationLocked: boolean; } -// The visible label on each tab in the top bar. These differ from the API's tab ids, which is why -// a test names the id and this map does the translating. The labels are localized, so a run in -// another UI language would need this to come from the l10n data instead. -const TAB_LABEL: Record = { - collection: "Collections", - edit: "Edit", - publish: "Publish", -}; +// The test id on each tab in the top bar, set in react_components/TopBar/TopBar.tsx. The tab ids +// here are Bloom's own API names, and the test ids are built from them, so this needs no map. +function tabTestId(tab: WorkspaceTabId): string { + return `workspace-tab-${tab}`; +} /** Ask Bloom which workspace tab is active and what state the others are in. */ export async function getTabs(page: Page): Promise { @@ -60,13 +57,15 @@ export async function waitForActiveTab( * * Bloom hides the Edit and Publish tabs entirely until a book is selected, so a test that wants * either of them must select a book first (see helpers/collection.ts). + * + * The tab is found by its test id, not by its label, so this works in any UI language. */ export async function switchTab( page: Page, tab: WorkspaceTabId, timeoutMs = 30000, ): Promise { - const target = page.getByRole("tab", { name: TAB_LABEL[tab] }); + const target = page.getByTestId(tabTestId(tab)); await target.waitFor({ state: "visible", timeout: timeoutMs }); await target.click(); await waitForActiveTab(page, tab, timeoutMs); diff --git a/src/BloomE2E/tests/capture-book-page.spec.ts b/src/BloomE2E/tests/capture-book-page.spec.ts new file mode 100644 index 000000000000..2f7a2f7fe729 --- /dev/null +++ b/src/BloomE2E/tests/capture-book-page.spec.ts @@ -0,0 +1,53 @@ +// Proves the capture helper works against a real WebView2, on a page taller than the window. +// +// This is the test for helpers/screenshot.ts rather than a test of a Bloom behavior. It exists +// because the driver-level footgun it absorbs is invisible from the outside: the obvious CDP route +// to this image (captureBeyondViewport) hangs with no error, and nothing else in the suite would +// notice if the safe pattern stopped working. See AUTOMATION-DEBT.md, "Driver-level CDP footguns +// that the automation library must absorb". +// +// This test has no "[Test Case ID N]" tag because it covers the automation library, not a case in +// the Notion test inventory. + +import * as Path from "node:path"; +import { expect, test } from "../fixtures/bloomTest"; +import { selectBook } from "../helpers/collection"; +import { getPages } from "../helpers/bookMaking"; +import { captureCurrentBookPage } from "../helpers/screenshot"; +import { switchTab } from "../helpers/workspace"; + +test.use({ collectionName: "basic" }); + +test("capturing the cover page gives a PNG the size of the page", async ({ + page, + bloomApp, +}) => { + await selectBook(page, Path.join(bloomApp.collectionDir, "A5 Portrait")); + await switchTab(page, "edit"); + + // Sanity check: the Edit tab opens on the cover, so there is a page to capture and it is the + // one this test says it captures. + const pages = await getPages(page); + expect(pages.length).toBeGreaterThan(0); + + const image = await captureCurrentBookPage(page); + + // Non-empty, and a PNG: readPngSize inside the helper already rejects anything else, so a + // plausible byte count is what is left to check. + expect(image.png.length).toBeGreaterThan(1000); + + // The image is the page, not the window: its pixels match the element's own box. CDP rounds + // the clip, so allow a pixel either way. + expect(image.width).toBeGreaterThan(100); + expect(image.height).toBeGreaterThan(100); + expect(Math.abs(image.width - image.elementWidth)).toBeLessThanOrEqual(1); + expect(Math.abs(image.height - image.elementHeight)).toBeLessThanOrEqual(1); + + // A5 Portrait is taller than it is wide, which is the case that needs the window override. + expect(image.height).toBeGreaterThan(image.width); + + // Put the workspace back on the collection tab. The launched Bloom is worker-scoped, so every + // test with these same options shares it: a test that ends on the Edit tab makes the next one + // start there. See AUTOMATION-DEBT.md, "One test's tab is the next test's starting state". + await switchTab(page, "collection"); +}); diff --git a/src/BloomE2E/tests/publish-text-languages.spec.ts b/src/BloomE2E/tests/publish-text-languages.spec.ts index 5d14e7d13afc..8a4f458580f7 100644 --- a/src/BloomE2E/tests/publish-text-languages.spec.ts +++ b/src/BloomE2E/tests/publish-text-languages.spec.ts @@ -22,8 +22,7 @@ import { setContentLanguages, typeInGroup, } from "../helpers/bookMaking"; -import { selectBook } from "../helpers/collection"; -import { restartWithCollectionSettings } from "../helpers/collectionSettings"; +import { selectBook, setCollectionLanguages } from "../helpers/collection"; import { clickTextLanguage, expectTextLanguageRows, @@ -95,12 +94,9 @@ test.describe("the Text Languages publish list", () => { ); await goToPage(page, coverBeforeRestart!.id); - // Now swap German out for Spanish. Collection settings have no API and their dialog is a - // WinForms surface CDP cannot reach, so the way to change them is to quit Bloom, rewrite - // the .bloomCollection, and start again. See AUTOMATION-DEBT.md. - const newPage = await restartWithCollectionSettings(bloomApp, { - languages: FINAL_LANGUAGES, - }); + // Now swap German out for Spanish. Bloom reads the collection's languages when it opens + // the collection, so this restarts it; see setCollectionLanguages. + const newPage = await setCollectionLanguages(bloomApp, FINAL_LANGUAGES); bookFolder = await findBookFolder(newPage, BOOK_TITLE); await selectBook(newPage, bookFolder); await switchTab(newPage, "edit"); @@ -347,36 +343,34 @@ test.describe("the Text Languages publish list", () => { await setContentLanguages(page, ["en"]); }); - // This test fails on CI every time, on the language NAME, and BL-16806 is the card that fixes - // it -- so if you are here because a nightly went red on this test, that is the cause and - // there is a fix in flight; nothing new to chase. - // - // Expected: español Received: espagnol - // - // "espagnol" is French for Spanish, and the answer depends on the machine, not on the run. - // Bloom asks LibPalaso for the name of the dropped language "in" the collection's metadata - // language, which is French here; LibPalaso honors that request only where a native ICU - // library is findable, and Bloom ships icu.net but no icuuc.dll. So the CI runner gives - // "espagnol" (nightly runs 33665790357 and 33685669405) while a developer machine ignores the - // request and gives the autonym "español" (checked in the real Publish tab). Everything else - // about the row -- unchecked, not incomplete, enabled -- is right. + // This test no longer meets the difference BL-16806 is about, and master's version of it + // does, so do not copy master's comment back here. Master drops the language by rewriting the + // .bloomCollection, and Bloom then asks LibPalaso for the name of the dropped language "in" + // the collection's metadata language. LibPalaso honors that only where a native ICU library + // is findable, and Bloom ships icu.net but no icuuc.dll, so the CI runner answers "espagnol", + // French for Spanish, every time, and a developer machine answers "español" every time. It is + // a real difference in what Bloom shows a user, tracked on BL-16806. // - // A local failure of this test is usually something else: it has other steps that time out on - // a loaded machine, and dies before reaching this assertion. - // - // Left running deliberately: it is a real difference in what Bloom shows a user, and the one - // test that catches it. - test("keeps a language that the collection no longer has, under its own name [Test Case ID 169]", async ({ + // This version drops the language through e2e/setCollectionLanguages, the code the Collection + // Settings dialog's OK button runs, which keeps the language's collection name. So the list + // reads "Spanish" on every machine, and the lookup that differs is never reached. The + // coverage that costs is recorded in src/BloomE2E/AUTOMATION-DEBT.md. + test("keeps a language that the collection no longer has, under the name the collection remembers [Test Case ID 169]", async ({ bloomApp, }) => { test.setTimeout(180000); // Drop Spanish from the collection. The book still has Spanish text, so the language stays - // in the list; but the collection no longer supplies a name for it, so Bloom falls back to - // the name the language calls itself. - const withoutSpanish = await restartWithCollectionSettings(bloomApp, { - languages: ["en", "fr"], - }); + // in the list, named "Spanish". Removing a language does not throw its settings away: + // CollectionSettingsDialog.UpdateLanguageSettings moves the displaced language to the end + // of the collection's list, name and font and all, so the name outlives the removal. (This + // test used to expect "español", the language's own name for itself, because it dropped + // the language by rewriting the .bloomCollection, which no part of Bloom does. Going + // through the same code the dialog's OK button runs is what changed the answer.) + const withoutSpanish = await setCollectionLanguages(bloomApp, [ + "en", + "fr", + ]); await selectBook(withoutSpanish, bookFolder); await openPublishDestination(withoutSpanish, "Web"); // In any order: where a language the collection no longer names sits in the list is not @@ -397,19 +391,20 @@ test.describe("the Text Languages publish list", () => { disabled: false, }, { - name: "español", + name: "Spanish", incomplete: false, checked: false, disabled: false, }, ], - "Spanish did not stay in the list, unchecked and under its own name.", + "Spanish did not stay in the list, unchecked and under the name the collection remembers.", ); // Put Spanish back, for the test that follows. - const withSpanish = await restartWithCollectionSettings(bloomApp, { - languages: FINAL_LANGUAGES, - }); + const withSpanish = await setCollectionLanguages( + bloomApp, + FINAL_LANGUAGES, + ); await selectBook(withSpanish, bookFolder); }); diff --git a/src/BloomE2E/tests/workspace-tabs.spec.ts b/src/BloomE2E/tests/workspace-tabs.spec.ts index 0079e3ac5405..322e933621ac 100644 --- a/src/BloomE2E/tests/workspace-tabs.spec.ts +++ b/src/BloomE2E/tests/workspace-tabs.spec.ts @@ -22,7 +22,9 @@ test("switching workspace tabs through the real top bar", async ({ bloomApp, }) => { // Sanity check the start state, so a failure below means a click failed rather than that we - // were already on the tab we were about to click. + // were already on the tab we were about to click. This reads the state once rather than waiting + // for it: a Bloom that is not on the collection tab here has been left that way by an earlier + // test sharing this worker's Bloom, and waiting would only turn that into a slow failure. expect((await getTabs(page)).tabStates.collection).toBe("active"); // Setup, not the behavior under test: Bloom hides the Edit and Publish tabs until a book is diff --git a/src/BloomExe/Edit/EditingModel.cs b/src/BloomExe/Edit/EditingModel.cs index 86a906d6a121..73d0293bb479 100644 --- a/src/BloomExe/Edit/EditingModel.cs +++ b/src/BloomExe/Edit/EditingModel.cs @@ -47,6 +47,10 @@ public class EditingModel private bool _reloadFromDiskOnLeavingEditTab; public bool Visible; + + // A page a JumpToPage call asked for while the Edit tab was not showing. OnBecomeVisible + // displays this page instead of the one it would otherwise choose. See JumpToPage. + private string _pageIdToShowWhenVisible; private Book.Book _currentlyDisplayedBook; private Book.Book _bookForToolboxContent; private EditingView _view; @@ -113,6 +117,11 @@ ITemplateFinder sourceCollectionsList _server = server; _webSocketServer = webSocketServer; _sourceCollectionsList = sourceCollectionsList; + // A run has one EditingModel, and E2eTestingApi has no way to be handed it: the + // container builds that api before any book is selected. So hand it over here, the + // same way WorkspaceView hands over its browser for e2e/shellUrl. + if (Program.RunningE2eTests) + ModelForE2eTests = this; _stateMachine = new EditingStateMachine( // navigate, @@ -515,6 +524,8 @@ BookSelectionChangedEventArgs bookSelectionChangedEventArgs // This edit tab can ignore changes that don't actually involve selecting a different book. if (_bookSelection.CurrentSelection == _currentlyDisplayedBook) return; + // A jump queued for a page of the book we are leaving means nothing in the new one. + _pageIdToShowWhenVisible = null; //prevent trying to save this page in whatever comes next var hadPageToSave = _havePageToSave; _havePageToSave = false; @@ -996,11 +1007,23 @@ public void OnBecomeVisible() ErrorReportUtils.CheckForFakeTestErrorsIfNotRealUser(_currentlyDisplayedBook.Title); - // BL-2339: try to choose the last edited page - var page = - _currentlyDisplayedBook.GetPageByIndex( - _currentlyDisplayedBook.UserPrefs.MostRecentPage - ) ?? _currentlyDisplayedBook.FirstPage; + // A jump asked for while this tab was not showing wins over the remembered page: it is + // the more recent request. See JumpToPage. + var requestedPageId = _pageIdToShowWhenVisible; + _pageIdToShowWhenVisible = null; + IPage page = null; + if (requestedPageId != null) + page = _currentlyDisplayedBook + .GetPages() + .FirstOrDefault(p => p.Id == requestedPageId); + if (page == null) + { + // BL-2339: try to choose the last edited page + page = + _currentlyDisplayedBook.GetPageByIndex( + _currentlyDisplayedBook.UserPrefs.MostRecentPage + ) ?? _currentlyDisplayedBook.FirstPage; + } if (page != null) _view.GoToPage(page); @@ -1687,6 +1710,93 @@ internal void SavePageAndReloadIt(ApiRequest request) request.PostSucceeded(); } + /// + /// The one EditingModel of an e2e run, or null outside such a run. See the constructor. + /// + public static EditingModel ModelForE2eTests; + + /// + /// What the Edit tab is doing, for the e2e suite to wait on (see e2e/editState). Editing + /// is the only state in which the tab accepts a request without deferring it. + /// + public State EditTabState => _stateMachine.CurrentState; + + /// + /// The page EditTabState is about, or null. + /// + public string EditTabStatePageId => _stateMachine.CurrentPageId; + + // How many times the page named by _announcedPageId has told us its DOM had loaded. See + // HandlePageDomLoadedEvent. + private string _announcedPageId; + private int _pageLoadAnnouncements; + + /// + /// How many times the page now being shown has announced that its DOM had loaded. A page + /// does that more than once, and a request that changes the page is lost if it arrives + /// between two of those announcements (see src/BloomE2E/AUTOMATION-DEBT.md). So the e2e + /// suite waits for this to stop rising. For automation only. + /// + public int PageLoadAnnouncements => _pageLoadAnnouncements; + + /// + /// Show the page with this id in the Edit tab. The page being left is saved on the way, + /// which is how anything typed on it reaches the file. + /// + /// A jump that arrives before the Edit tab is showing is remembered, and OnBecomeVisible + /// displays that page instead of the one it would otherwise choose. + /// + /// Otherwise the jump either happens at once or is refused: this returns false when the + /// Edit tab is busy with a page (loading one, or saving one), when there is no book, and + /// when the tab is in the momentary state after a save in which no transition is allowed. + /// Bloom used to drop such a jump and report success, which left the caller waiting for a + /// page that was never coming (see src/BloomE2E/AUTOMATION-DEBT.md). + /// + public bool JumpToPage(string pageId) + { + if (CurrentBook == null || string.IsNullOrEmpty(pageId)) + return false; + + if (!Visible) + { + // The Edit tab is not showing, so there is nothing to save and nowhere to + // navigate. OnBecomeVisible chooses the page to display, so hand it this one. + _pageIdToShowWhenVisible = pageId; + return true; + } + + // The Edit tab is busy with a page: saving one, or loading one. Say so rather than + // queue the jump. A queued jump has to be released by some signal that the tab is + // ready, and the only signal available is a page announcing that its DOM has loaded, + // which a page does more than once. A jump released by the first announcement starts + // a save, the second announcement is then refused because a save is in flight, and + // the browser never answers the save request, which leaves the tab unable to change + // pages at all. See src/BloomE2E/AUTOMATION-DEBT.md. + if (_stateMachine.SavePending || _stateMachine.Navigating) + { + Logger.WriteEvent( + $"EditingModel.JumpToPage({pageId}): the edit tab was busy with a page, so the jump was refused." + ); + return false; + } + + var jumped = true; + SaveThen( + () => pageId, + doIfNotInRightStateToSave: () => + { + // We ruled out the two states that refuse a save above, so we are in the + // momentary SavedAndStripped state, which an API call should not be able to + // observe. Say so rather than drop the request silently. + Logger.WriteEvent( + $"EditingModel.JumpToPage({pageId}): the edit tab was not in a state to save, so the jump was refused." + ); + jumped = false; + } + ); + return jumped; + } + private bool CannotSavePage() { return _bookSelection == null @@ -2186,6 +2296,16 @@ public UrlPathString AddWidgetFilesToBookFolder(string fullWidgetPath) public void HandlePageDomLoadedEvent(string pageId) { + // Count the announcement before acting on it. A page announces itself more than once, + // and the e2e suite has to know that the last one has been and gone before it asks + // this tab for anything (see the editState endpoint and PageLoadAnnouncements). + if (pageId != _announcedPageId) + { + _announcedPageId = pageId; + _pageLoadAnnouncements = 0; + } + _pageLoadAnnouncements++; + var nowEditing = _stateMachine.ToEditing(pageId); if (nowEditing) { diff --git a/src/BloomExe/Edit/EditingStateMachine.cs b/src/BloomExe/Edit/EditingStateMachine.cs index 6272e4168d8d..e5a1e51f3990 100644 --- a/src/BloomExe/Edit/EditingStateMachine.cs +++ b/src/BloomExe/Edit/EditingStateMachine.cs @@ -50,6 +50,7 @@ public class EditingStateMachine // save has completed and we are back in a state that allows transitions. // See DeferUntilSaveCompletes. private Action _workToDoAfterInFlightSave; + private Action _hidePage; private Action _enableStateTransitions; // arg is (enabled) @@ -149,6 +150,18 @@ public bool ToNoPage() /// public bool Editing => _currentState == State.Editing; + /// + /// The state we are in, and the page it is about. These exist for automation: the e2e suite + /// waits until the Edit tab is settled before it asks that tab for anything, and the DOM + /// cannot tell it that (see E2eTestingApi's editState endpoint). Read only. + /// + public State CurrentState => _currentState; + + /// + /// The page the current state is about, or null. See CurrentState. + /// + public string CurrentPageId => _pageId; + /// /// Called to initiate navigation to a new page (or the same one again). /// Should not be called when there are unsaved (or incompletely saved) changes. @@ -511,6 +524,11 @@ public bool ToSavedAndStripped(Func postSaveAction, string pageContentOr private void Log(string message) { Debug.WriteLine("[EditingStateMachine] " + message); + // Under --e2e these go into Bloom's log as well. A test that waits for a page the edit tab + // never shows is otherwise a mystery: nothing else records which state the tab was in or + // which request it turned down. + if (Bloom.Program.RunningE2eTests) + Logger.WriteEvent("[EditingStateMachine] " + message); } private void LogTransition(string nextState, string nextPageId) diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index e47eb7f19b2c..c90fbffb0111 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -309,6 +309,12 @@ CoreWebView2ContextMenuRequestedEventArgs e private static bool _useSharedEnvironment; private static CoreWebView2Environment _sharedEnvironment; + // The one environment every browser of an e2e run shares, so the run has a single browser + // process and therefore a single remote-debugging listener. See where it is used in + // InitWebView. Like the statics above it is unsynchronized, which is safe for the same + // reason: browser construction is marshalled to the UI thread. + private static CoreWebView2Environment _environmentForE2eTests; + public static void BeginSharedEnvironmentBatch() { AssertSharedEnvironmentStaticsAreUiThreadOnly(); @@ -475,6 +481,21 @@ private async Task InitWebView() // - _sharedEnvironment: the legacy on-UI-thread shared-environment batch (BookProcessor's old path). // Otherwise we fall through and create a fresh one. var env = _injectedEnvironment ?? (_useSharedEnvironment ? _sharedEnvironment : null); + // An e2e run attaches a test to ONE of these browser processes over the remote debugging + // port, and every environment we create is given that same port number, so only the + // process that starts first can listen on it. Which one that is depends on startup + // timing, so a test could attach to a browser Bloom is not driving: its scripts appeared + // to run (ExecuteScriptAsync reported success against the browser Bloom does drive) + // while the document the test was watching never changed. One environment for the whole + // run means one browser process, one listener, and every document visible to the test. + // + // Only for browsers built on the UI thread, which is every browser a test can see. A + // CoreWebView2Environment belongs to the thread that created it, so handing this one to + // a browser built on a server thread hangs that thread: publishing a BloomPUB, which + // makes its browsers on the thread serving the API call, waited forever and the preview + // never appeared. + if (env == null && Program.RunningE2eTests && Program.RunningOnUiThread) + env = _environmentForE2eTests; if (env == null) { string dataFolder; @@ -494,6 +515,18 @@ private async Task InitWebView() ); if (_useSharedEnvironment) _sharedEnvironment = env; + // Only keep it when it actually carries a debugging port. The port lives in the + // options, which are fixed when the environment is made, so an environment built + // before BloomServer had its port would have none, and every UI-thread browser + // after it would inherit that: no browser in the run would ever listen, and the + // suite would report a startup timeout rather than a reason. No browser is built + // that early today, and this keeps it that way if one ever is. + if ( + Program.RunningE2eTests + && Program.RunningOnUiThread + && RemoteDebuggingPort.HasValue + ) + _environmentForE2eTests = env; } await _webview.EnsureCoreWebView2Async(env); // Added as a footnote to BL-15466 to prevent popups generated from title diff --git a/src/BloomExe/Workspace/WorkspaceView.cs b/src/BloomExe/Workspace/WorkspaceView.cs index 8707ea3666c2..f713289c5e5f 100644 --- a/src/BloomExe/Workspace/WorkspaceView.cs +++ b/src/BloomExe/Workspace/WorkspaceView.cs @@ -179,6 +179,8 @@ TeamCollectionApi teamCollectionApi _workspaceReactControl.BrowserCreated += (unused, args) => { _mainBrowser = _workspaceReactControl.Browser; + if (Program.RunningE2eTests) + MainBrowserForE2eTests = _mainBrowser; _mainBrowser?.SetBuiltInBrowserZoomEnabled(false); _editingView.InitializeMainBrowserForEditMode(); MaybeOpenMainBrowserDevTools(); @@ -281,6 +283,14 @@ protected override void OnLoad(EventArgs e) ); // possibility of error message boxes (BL-12155) } + /// + /// The browser holding the workspace root document that Bloom drives: the one whose page + /// iframe the Edit tab navigates. Set only under --e2e, for the e2e/shellUrl endpoint. + /// More than one document in a run carries the workspace root's markup, and a test that + /// attaches to the wrong one sees its own clicks work while nothing Bloom does arrives. + /// + internal static Browser MainBrowserForE2eTests { get; private set; } + internal void ReloadWorkspaceRootDocument() { _workspaceReactControl?.Reload(); diff --git a/src/BloomExe/web/controllers/E2eTestingApi.cs b/src/BloomExe/web/controllers/E2eTestingApi.cs index 0d038182dde3..1f4bdb30ad75 100644 --- a/src/BloomExe/web/controllers/E2eTestingApi.cs +++ b/src/BloomExe/web/controllers/E2eTestingApi.cs @@ -1,3 +1,4 @@ +using System; using System.Linq; using Bloom.Api; using Bloom.Book; @@ -123,6 +124,21 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) false // does not need the UI thread ); + // POST body is a JSON array of one to three language tags, e.g. ["en","fr","es"], for + // Language1, Language2 and Language3. Sets the collection's languages and writes the + // .bloomCollection file, which a test otherwise has to compose by hand: the Collection + // Settings dialog is a WinForms surface CDP cannot reach, and its own + // collectionSettings/changeLanguage endpoint only answers while that dialog is open. + // Changing a collection's languages still needs the collection to be reopened, exactly + // as it does for a user who clicks OK in that dialog, so the caller must restart Bloom + // (src/BloomE2E/helpers/collection.ts setCollectionLanguages does both). + // Must run on the UI thread because it changes the settings the UI is showing. + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "setCollectionLanguages", + HandleSetCollectionLanguages, + true + ); + // GET returns the pages the Add Page dialog would offer for the selected book: the // path of its template book, and the id and label of each template page. A test needs // these to call the production "addPage" endpoint, and the dialog itself reads them @@ -133,6 +149,31 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) false // does not need the UI thread ); + // GET returns the URL of the workspace root document Bloom drives, or an empty string + // before that browser exists. A run has more than one document carrying the workspace + // root's markup, so the top bar's test id alone does not identify the right one, and a + // test that attaches to the wrong one is silently broken: its own typing and clicking + // work, while every page Bloom loads goes somewhere it cannot see. Compare on the file + // name, which is unique per document; the rest of the URL is escaped differently by + // Bloom and by the debugging protocol. Needs the UI thread to read the browser. + apiHandler.RegisterEndpointHandler(kApiUrlPart + "shellUrl", HandleGetShellUrl, true); + + // GET returns what the Edit tab is doing, as + // {state, pageId, visible, pageLoadAnnouncements}. A test must not ask that tab to + // change pages while it is still loading one: the request is queued until the page + // loads, and a page announces itself more than once, so a request released by the + // first announcement starts a save that the second one leaves unanswered. Nothing + // happens after that (see src/BloomE2E/AUTOMATION-DEBT.md). The DOM cannot tell a test + // any of this, because the frame still holds the page from before the tab switch, so + // the count of announcements is how a test knows the last one has been and gone. Off + // the UI thread on purpose: this reads three fields, and a test asks for them exactly + // when the UI thread is busy. + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "editState", + HandleGetEditState, + false // does not need the UI thread + ); + // POST {"email": ...}: which Bloom Library login state Bloom should REPORT. A test // needs this because the real login lives in machine-wide settings shared with the // developer's own Bloom: signing out for real would sign the developer out, and @@ -147,6 +188,38 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) ); } + /// + /// Reply with what the Edit tab is doing (see the registration above). Before the first + /// book is selected there is no EditingModel, which reads as NoPage. + /// + private void HandleGetEditState(ApiRequest request) + { + var model = Bloom.Edit.EditingModel.ModelForE2eTests; + // The UI thread can change these fields between one read and the next, so keep + // pageLoadAnnouncements last. A count read after the state can only be the same or + // higher, which makes a test see the count still rising and wait again. Read first, it + // could pair a stale count with a settled state, and a test would stop waiting too + // soon. + request.ReplyWithJson( + new + { + state = (model?.EditTabState ?? State.NoPage).ToString(), + pageId = model?.EditTabStatePageId ?? "", + visible = model?.Visible ?? false, + pageLoadAnnouncements = model?.PageLoadAnnouncements ?? 0, + } + ); + } + + /// + /// Reply with the URL of the workspace root document Bloom drives (see the registration + /// above), or an empty string if the main browser is not up yet. + /// + private void HandleGetShellUrl(ApiRequest request) + { + request.ReplyWithText(Workspace.WorkspaceView.MainBrowserForE2eTests?.Url ?? ""); + } + /// /// What POST e2e/loginState takes: the email to report as signed in, the empty string to /// report as signed out, or null (an absent member) to stop pretending altogether and @@ -255,6 +328,76 @@ private void HandleGetTemplatePages(ApiRequest request) request.ReplyWithJson(pages); } + /// + /// Set the collection's Language1, Language2 and Language3 to the tags in the POST body (a + /// JSON array of one to three tags), and save the .bloomCollection file. Fewer than three + /// tags leaves the languages that were not named empty, except that a collection naming + /// only one language repeats it as Language2, which is what Bloom's own new-collection code + /// writes. + /// + /// This does the same work as clicking OK in the Collection Settings dialog, including + /// keeping a language that is no longer one of the first three in the collection's list of + /// languages. Like that dialog, it needs the collection reopened before the change is + /// everywhere it should be; the caller restarts Bloom. + /// + private void HandleSetCollectionLanguages(ApiRequest request) + { + var tags = request.RequiredPostObject(); + if (tags == null || tags.Length < 1 || tags.Length > 3) + throw new ArgumentException( + "e2e/setCollectionLanguages takes a JSON array of one to three language tags." + ); + if (string.IsNullOrWhiteSpace(tags[0])) + throw new ArgumentException( + "e2e/setCollectionLanguages needs a tag for Language1." + ); + + // Start from the collection's own writing systems, so that everything about each + // language except its tag (the font, the line height, the writing direction) keeps the + // value it had, then put the requested tag on each one. + // + // Each language is named in English ("French", not "français"), which is what a person + // gets by keeping the English name Bloom's language chooser offers. Bloom calls a name + // that is not the language's own name for itself a custom name, and a custom name is + // the one thing it shows verbatim everywhere; leaving the name uncustomized would make + // each screen name the language in whatever language it liked. + var pending = new WritingSystem[3]; + for (var i = 0; i < 3; i++) + { + pending[i] = _collectionSettings.AllLanguages[i].Clone(); + var tag = i < tags.Length ? tags[i].Trim() : string.Empty; + pending[i].ChangeTag(tag); + if (!string.IsNullOrEmpty(tag)) + { + // ChangeTag has already replaced the name with the one the language uses for + // itself, but it leaves IsCustomName alone. So clear that flag before asking + // for the English name: a language that arrived here with a custom name would + // otherwise be told "you have a name a person chose", and hand back the name + // ChangeTag just computed instead of the English one. + pending[i].SetName(pending[i].Name, false); + pending[i].SetName(pending[i].GetNameInLanguage("en"), true); + } + } + if (string.IsNullOrEmpty(pending[1].Tag)) + { + pending[1].ChangeTag(pending[0].Tag); + // Same reason as the loop above: ChangeTag leaves IsCustomName alone, so a + // Language2 that arrived here with a custom name would keep the name of the + // language it just replaced. + pending[1].SetName(pending[1].Name, false); + pending[1].SetName(pending[1].GetNameInLanguage("en"), true); + } + + CollectionSettingsDialog.UpdateLanguageSettings( + _collectionSettings.AllLanguages, + pending, + pending.Select(language => language.FontName).ToArray() + ); + _collectionSettings.Save(); + + request.PostSucceeded(); + } + /// /// Stage the currently selected book as a BloomPUB and reply with the localhost URL of the /// staged .htm file, which a test can load in bloom-player. diff --git a/src/BloomExe/web/controllers/EditingViewApi.cs b/src/BloomExe/web/controllers/EditingViewApi.cs index 0935efc49de0..858109699605 100644 --- a/src/BloomExe/web/controllers/EditingViewApi.cs +++ b/src/BloomExe/web/controllers/EditingViewApi.cs @@ -320,11 +320,22 @@ private void HandleSetCustomPageLayout(ApiRequest request) ); } + /// + /// Show the page whose id is in the POST body. The reply comes after asking the model, so + /// that a jump the Edit tab cannot do is reported as a failure rather than as a success + /// that shows nothing. EditingModel.JumpToPage queues a jump that arrives at an awkward + /// moment, so a caller does not have to ask twice. + /// + /// A failure here is a matter of timing, not of anything a user could put right, so the + /// front end posts to this endpoint with error reporting turned off. + /// private void HandleJumpToPage(ApiRequest request) { var pageId = request.GetPostStringOrNull(); - request.PostSucceeded(); - View.Model.SaveThen(() => pageId, () => { }); + if (View.Model.JumpToPage(pageId)) + request.PostSucceeded(); + else + request.Failed($"The edit tab could not show page {pageId}."); } /// diff --git a/src/BloomVisualRegressionTests/index.spec.ts b/src/BloomVisualRegressionTests/index.spec.ts index b7a260b9bdda..dfe130d1030c 100644 --- a/src/BloomVisualRegressionTests/index.spec.ts +++ b/src/BloomVisualRegressionTests/index.spec.ts @@ -208,6 +208,12 @@ describe("All books", () => { let playerPage: Page; let browser: Browser; let context: BrowserContext; + // Every image comparison the CURRENT case has failed. A case compares one book-preview image + // and one image per bloom-player page, and a stale baseline usually affects several of them. + // Throwing on the first mismatch meant the later comparisons never even captured their + // images, so accepting a real layout change took one ~3-minute run per image (BL-16638 took + // three rounds). So collect them all and fail once, at the end of the case. + let comparisonFailures: string[] = []; beforeAll(async () => { await launchDedicatedBloom(); @@ -297,6 +303,7 @@ describe("All books", () => { }); test.each(cases)("$title", async (testCase) => { + comparisonFailures = []; // Park the capture pages before we mutate this book. Otherwise the previous case's still-open // book-preview / bloom-player page keeps requesting book and staged-BloomPUB files while this // case rewrites them, which caused mid-run "file is being used by another process" and @@ -334,6 +341,14 @@ describe("All books", () => { await selectTab("publish"); const stagedUrl = await makeBloomPubPreview(); await capturePlayerPages(stagedUrl, testCase.label, screenshotsDir); + + // One failure for the whole case, listing every image that did not match, so a single run + // shows all the baselines that need looking at. + if (comparisonFailures.length > 0) + throw new Error( + `${comparisonFailures.length} of this case's images did not match their ` + + `reference:\n ${comparisonFailures.join("\n ")}`, + ); }); // Create the reference image if it does not exist yet; otherwise capture a current image and @@ -694,6 +709,11 @@ describe("All books", () => { } } + // Compare one captured image against its reference. This does NOT throw on a mismatch: it + // appends a description to comparisonFailures, and the test body fails the case once it has + // compared every image. Anything thrown while comparing (notably Pixelmatch's "Image sizes do + // not match", which is itself a real failure) is recorded the same way. + // // `likelyCause`, when given, is an explanation to attach to a mismatch (see andikaIsInstalled): // something we know about this machine that makes the difference expected rather than a regression. async function comparePreviewImage( @@ -701,6 +721,27 @@ describe("All books", () => { testPath: string, diffPath: string, likelyCause?: string, + ) { + try { + await compareOrThrow( + referencePath, + testPath, + diffPath, + likelyCause, + ); + } catch (error) { + comparisonFailures.push( + `${testPath}: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + + // The comparison itself: write a diff image and throw when the two images differ at all. + async function compareOrThrow( + referencePath: string, + testPath: string, + diffPath: string, + likelyCause?: string, ) { const referenceImage = PNG.sync.read(fs.readFileSync(referencePath)); const testImage = PNG.sync.read(fs.readFileSync(testPath)); @@ -732,9 +773,9 @@ describe("All books", () => { ); // A thrown Error rather than expect(...).toBe(0), so the failure itself carries the // diff path and, when we know one, the likely cause; the console lines above are lost in - // a long run's output. + // a long run's output. comparePreviewImage catches this and records it. throw new Error( - `${testPath} differed from the reference by ${numberOfDifferentPixels} pixels. ` + + `${testPath} differed from ${referencePath} by ${numberOfDifferentPixels} pixels. ` + `The diff image is at ${diffPath}.` + (likelyCause ? `\n\n${likelyCause}` : ""), );