diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 078e9ad00c33..a7dfed1e8bf6 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -87,27 +87,43 @@ carries the API mechanics. `test("change UI language repeatedly [Test Case ID 69]", ...)` — so the code and the inventory stay tied. Read the card's Test Steps checkboxes; they are the behavior contract. When the automated test lands, set the card's `Automation` property to - `Automated` — or to `Partial` when the automated test covers only part of the steps, - and say which part in `Automation Notes`. While the test is still in an open PR, the - card belongs in `PR Pending` instead, with the PR URL in `Automation Notes`. The title string is the whole mechanism; + `Automated`. While the test is still in an open PR, the card belongs in `PR Pending` + instead, with the PR URL in `Automation Notes`. The title string is the whole mechanism; the library provides no helper or annotation for it, deliberately, so that grepping `Test Case ID` across `src/BloomE2E/tests/` finds every tie. +- **When the test covers only part of the card's steps, split the card.** A card marked + `Automated` while some of its steps are still human-run hides those steps: nobody reads + `Automation Notes` when planning a manual run. So a card is never half automated. + 1. Rename the original to ` [Automated portion]`. It keeps its `Test Case ID`, + because the test source carries that id, and keeps only the steps the test covers. + 2. Add a row `<title> [Manual portion]` with the next free `Test Case ID` and the same + `Test Suite Run`, `Areas`, `Priority`, and `Dokimion ID`. Move the uncovered steps into + it. Start its body with a callout that names the automated card and says, per step, + why it is not automated (microphone, native dialog, WinForms surface) and which + `AUTOMATION-DEBT.md` entry covers it. Its `Automation` is `Manual`, or `Keep manual` + when the steps can never be automated. + 3. Link the two rows through the `Related Cases` relation property, in both directions, + and name the other card's id in each `Summary`. + Do this when you set `PR Pending`, not after the merge. Example: Test Case ID 349, + "Duplicate Page [Automated portion]", and its manual portion, Test Case ID 810. - **Writing a new e2e test that has no manual card:** add a row to the inventory so it remains the inventory of ALL tests, not only human-run ones. Allocate the next free `Test Case ID`, fill in the title, Summary, and Areas, and set `Automation` to `Automated`. - **The `Automation` select property** holds the case's automation lifecycle: - `Manual` → `Planned` → `Building` → `PR Pending` → `Automated` (or `Partial`), with - `Keep manual` as the deliberate opt-out. + `Manual` → `Planned` → `Building` → `PR Pending` → `Automated`, with `Keep manual` as + the deliberate opt-out. - Empty means the same as `Manual` — the legacy rows were not bulk-stamped. - `Planned` marks a case the team judged a good automation candidate. To find work, filter the current suite run on `Automation = Planned`. - `Building` means someone is automating it right now. Set it when you start, so two - people or agents do not automate the same case; set `Automated` (or `Partial`, - with the covered part named in `Automation Notes`) when the test lands. + people or agents do not automate the same case; set `Automated` when the test lands, + after splitting the card if the test covers only part of its steps. - `PR Pending` means the test exists in an open PR that has not merged. Put the PR URL in `Automation Notes`. The `improve-test-automation-coverage` skill leaves cards here; - a human (or a later sweep) moves them to `Automated` or `Partial` after the merge. + a human (or a later sweep) moves them to `Automated` after the merge. + - `Partial` is retired. A card that would have been `Partial` is split instead (see + above). A card still marked `Partial` is one that still needs the split. - `Has automation problems` means an automation attempt found the card not automatable as written. `Automation Notes` says which step blocks it and what the card, or Bloom, needs. The developer who owns the card fixes that and sets `Planned` again. diff --git a/.github/skills/improve-test-automation-coverage/SKILL.md b/.github/skills/improve-test-automation-coverage/SKILL.md index b3fb4afafafe..d1138d4a14cb 100644 --- a/.github/skills/improve-test-automation-coverage/SKILL.md +++ b/.github/skills/improve-test-automation-coverage/SKILL.md @@ -49,8 +49,11 @@ forbidden: marking a PR ready for review, moving any Orca card to Peer Review, s ## The Automation lifecycle this skill drives `Planned` → `Building` (you, at claim time) → `PR Pending` (worker, when the draft PR exists, -with the PR URL in `Automation Notes`) → `Automated` or `Partial` (a human, after merge; a -separate sweep of `PR Pending` cards is planned). A worker that finds a case not feasible as +with the PR URL in `Automation Notes`) → `Automated` (a human, after merge; a separate sweep of +`PR Pending` cards is planned). A worker whose test covers only part of a card's steps splits the +card when it sets `PR Pending`, as `add-e2e-test` describes: the original becomes the +`[Automated portion]` and keeps its id, the uncovered steps move to a new `[Manual portion]` row. +A card is never left half automated. A worker that finds a case not feasible as written sets it to `Has automation problems` with a dated note that says what the card, or Bloom, needs; that is the queue for the developer who wrote the card. The developer telling the worker that the card is not ready counts as such a finding, as much as a technical block does. Such a card is out of @@ -183,7 +186,8 @@ The worker has asked you to review its test, and is blocked until you reply. `add-e2e-test`: the title carries `[Test Case ID <id>]`; the test builds its own collection unless a fixture is justified; the behavior under test goes through the real UI, setup may use the API; waits are state-based; no native dialogs; helpers reused rather than re-implemented; - the covered and uncovered Test Steps match what the worker says; `AUTOMATION-DEBT.md` records + the covered and uncovered Test Steps match what the worker says, and any uncovered step means + the card was split into an automated and a manual portion; `AUTOMATION-DEBT.md` records anything the worker could not automate cleanly. Run the `code-review` skill on the worktree for a second opinion when the diff touches C# or the shared helpers. 2. If you want to see it run, run it yourself through the lock, from that worktree's @@ -215,6 +219,27 @@ in the state the outcome implies (`PR Pending` with a PR URL; `Has automation pr dated note; or `Planned` with a `Blocked:` note) and fix it with `notion_automation.py set` if the worker forgot. Do not delete the worktree: the PR lives on that branch. +## Resuming a stalled run + +A run stalls when the controller or a worker stops for a reason outside the work: a Claude +usage limit, a machine sleep, an Orca restart. Symptoms: `check` shows an `escalation` +"Agent exited unexpectedly", or heartbeats "rejected ... capability is revoked", and the cards +stay `Building`. A new controller can take the run over: + +1. `orca orchestration worker-list --json` filtered on the run id gives every dispatch, its + task, and its worktree. `git -C <worktree> status --short` shows what the dead worker left. + Nothing is lost: the work is uncommitted in the worktree. +2. For each task whose dispatch is `failed` or `abandoned`, start a replacement in the SAME + worktree: `worker-start --task <task_id> --retry-of <old dispatch> --worktree id:<worktree id> + --agent claude --model claude-fable-5-1`. Then send the new dispatch a follow-up that says + the predecessor died, that its work is in the worktree, to read `git status` and `git diff` + first and continue from it, and where the brief file is. Restate any review fixes you had + already sent the dead worker. +3. Acknowledge the stale inbox messages, then continue Step 3 as usual. + +Do not reset a card to `Planned` because its worker died; the claim and the worktree are +still good. + ## Step 4 — Report One message to the developer, in this order: diff --git a/.github/skills/improve-test-automation-coverage/worker-brief.md b/.github/skills/improve-test-automation-coverage/worker-brief.md index 111d812295ec..89171092cc4d 100644 --- a/.github/skills/improve-test-automation-coverage/worker-brief.md +++ b/.github/skills/improve-test-automation-coverage/worker-brief.md @@ -94,8 +94,7 @@ Use the task id and dispatch id from the dispatch preamble at the top of your pr ### 3. Implement the test Follow the add-e2e-test skill. Put `[Test Case ID {{TEST_CASE_ID}}]` in the test title. Prefer -`collectionSpec` (the test builds its own collection). Cover every to_do step you can; if you -cover only part, remember which part for the `Partial` note later. +`collectionSpec` (the test builds its own collection). If you cannot implement each step, that is a problem. If a substantial portion can be automated and there is a clean split, then the notion test must be split into manual vs. automated. Otherwise,  you can just fail the implementation of this test and set the Automation property to "Has automation problems". ### 4. Run it through the lock, three times @@ -131,8 +130,11 @@ proceed without a `ship`. Also say which Test Steps the test covers and which it does not, and give any change outside `src/BloomE2E` its own **Bloom production code changes** heading, as the add-e2e-test summary does. -2. Set the card to `PR Pending` with the PR URL in the note. If the test covers only part of the - steps, say which part in the same note: +2. If the test covers only part of the card's steps, split the card first, as `add-e2e-test` + describes: the original keeps its `Test Case ID` and becomes `<title> [Automated portion]`; + the uncovered steps move to a new `<title> [Manual portion]` row. A card is never left half + automated. Then set the card to `PR Pending` with the PR URL in the note, and say in the same + note which steps the test covers: ```powershell py {{SKILL_DIR}}/notion_automation.py set {{TEST_CASE_ID}} "PR Pending" --note "[improve-test-automation-coverage {{TODAY}}] PR: <pr url>. Covers steps: <which>. Not covered: <which, or 'none'>." diff --git a/build/testing-inputs.pin b/build/testing-inputs.pin index 647be254c0a8..cb175131e318 100644 --- a/build/testing-inputs.pin +++ b/build/testing-inputs.pin @@ -18,4 +18,4 @@ # Format: one `key=value` per line. Blank lines and lines starting with # are ignored. repo=https://github.com/BloomBooks/bloom-testing-inputs.git -commit=3b69cfb30892202c9564f58290fa05a00f80f2ee +commit=9b58f6be71491201b121debd184bb38ec14aa43f diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index d94d5c8bd396..204e3978a070 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -82,6 +82,12 @@ only stable marker available. Fix direction: `data-testid="workspace-tab-collect (etc.) on each tab and one on the shell root, and drop the label matching. (Found 2026-09-01 while scaffolding src/BloomE2E.) +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 +inline), so `src/BloomE2E/helpers/pageThumbnails.ts` has to find "Copy Page" and "Paste Page" +by their English labels, exactly as the top bar does. Same fix: a `data-testid` per command, +taken from the `commandId` the menu already has. + ## The component-tester Playwright suites are not in CI `nightly.yml` runs vitest, C#, and visual-regression only; nothing runs @@ -187,6 +193,40 @@ CKEditor does with a programmatic value change; a supported "set the text of thi path would let long text be set at once. (Found 2026-09-01 automating Test Case ID 169.) +## The page menu offers commands that silently do nothing while a page is loading + +Copy Page and Paste Page go through `EditingModel.SaveThen`, which quietly gives up when the +editing state machine is not in Editing or NoPage (`EditingStateMachine.ToSavePending` returns +false and `CopyPage` passes `() => { }` as its "wrong state" action). The menu does not know +this: `PageThumbnailList.IsContextMenuCommandEnabled` disables commands during SavePending, but +NOT during Navigating, so while a page is still loading both commands look available and both +do nothing at all, with no error and no message. Copy Page itself then saves and reloads the +page, which reopens the same window for the very next click. + +Cost, twice over. For a person: click Copy Page and then Paste Page quickly and the paste is +lost with no feedback. For a test: `src/BloomE2E/helpers/pageThumbnails.ts` has to carry +`markEditablePage` / `waitForEditablePageReload`, which stamp the page's document and wait for +Bloom to replace it, purely to know when the model has come back to Editing — the page url +cannot answer it, because Bloom reloads a page to the same in-memory url. Fix direction: make +the enabled test cover the Navigating state too, so a command that cannot run is greyed out; +or, better, queue the command instead of dropping it. Either would let the helper drop the +document-marking dance. +(Found 2026-09-01 while automating Test Case ID 348, copy page preserves everything.) + +## Copying a page between two Bloom instances cannot be tested at all + +The manual case "Copy Page Preserves Everything" (Test Case ID 348) ends by copying a page from +one running Bloom into a second one. Bloom's page clipboard is a pair of fields on the one +`EditingModel` instance (`_pageDivFromCopyPage`, `_bookPathFromCopyPage`), not the Windows +clipboard, so nothing crosses a process boundary; the feature is known not to work in 6.5. The +e2e fixture is also built around one Bloom per worker, so a test could not stage it today even +if the feature worked. The automated test therefore covers the within-book and between-books +cases only, so the Notion card splits: the cross-instance step belongs on a manual portion +row, per the card-splitting rule in `add-e2e-test`. Fix direction: decide whether cross-instance +copy is a feature we want; if it is, put the page on the real clipboard, and give the launch +fixture a way to run a second instance. +(Found 2026-09-01 while automating Test Case ID 348.) + ## Every Bloom of one build shares one user.config, so a run inherits another Bloom's settings Bloom keeps its user settings (UI language, page zoom, and the rest of `Settings.Default`) in diff --git a/src/BloomE2E/README.md b/src/BloomE2E/README.md index 8e527b9496c7..38ac655fcd85 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -115,10 +115,11 @@ a case, put that id in the test title so the code and the inventory stay tied: test("change UI language repeatedly [Test Case ID 69]", async ({ page }) => { ... }); ``` -Then set the card's `Automation` property to `Automated`, or to `Partial` when the test covers only -part of the steps. A new test with no manual card gets a new inventory row, so the inventory stays -the inventory of all tests rather than only the human-run ones. `.github/skills/add-e2e-test/SKILL.md` -has the details. +Then set the card's `Automation` property to `Automated`. If the test covers only part of the card's +steps, split the card first into an `[Automated portion]` that keeps the id and a `[Manual portion]` +with a new id, so no human-run step hides behind an automated card. A new test with no manual card +gets a new inventory row, so the inventory stays the inventory of all tests rather than only the +human-run ones. `.github/skills/add-e2e-test/SKILL.md` has the details. ## Running diff --git a/src/BloomE2E/helpers/bookHtml.ts b/src/BloomE2E/helpers/bookHtml.ts new file mode 100644 index 000000000000..197ff9ba6034 --- /dev/null +++ b/src/BloomE2E/helpers/bookHtml.ts @@ -0,0 +1,155 @@ +// Read a book's saved .htm from disk and describe what each of its pages contains. +// +// Bloom writes the book to disk as it edits, so the file is the product's own record of what a +// page holds — a better subject for "did the copy preserve everything?" than the editing DOM, +// which shows only the one page on screen and decorates it with editing-only markup. +// +// Parsing happens INSIDE Bloom's own page, with DOMParser, rather than in Node: this package +// has no HTML parser among its dependencies, and adding one to read a file we already have is +// not worth it. Nothing is written back; the page is only borrowed as a parser. + +import * as fs from "node:fs"; +import * as Path from "node:path"; +import { expect, type Page } from "@playwright/test"; + +/** What one page of a book holds, reduced to the things the copy-page test measures. */ +export interface IPageContents { + /** The page div's own id. Bloom gives a pasted page a fresh one. */ + id: string; + /** The template the page came from, e.g. the Custom layout's id. */ + lineage: string; + /** Every user-defined style class (`Foo-style`) on the page's editable text. */ + styleClasses: string[]; + /** The `src` of every image on the page, relative to the book folder. */ + imageSources: string[]; + /** The id of every Talking Book recorded span; each names a file in `audio/`. */ + audioSentenceIds: string[]; + /** The `src` of every video source on the page, `#t=` trim fragment included. */ + videoSources: string[]; + /** + * The page's origami layout, as one string per split: the orientation and the two + * component sizes. Comparing these says whether a custom layout survived the copy. + */ + layout: string[]; +} + +/** A book as read from disk: its pages, plus the user-defined styles its head carries. */ +export interface IBookContents { + pages: IPageContents[]; + /** The text of the book's `userModifiedStyles` block, where Bloom keeps custom styles. */ + userModifiedStyles: string; +} + +/** The path of a book folder's own .htm file, which Bloom names after the folder. */ +export function bookHtmlPath(bookFolder: string): string { + return Path.join(bookFolder, `${Path.basename(bookFolder)}.htm`); +} + +/** + * Read the book at `bookFolder` and describe its numbered (non-front/back-matter) pages, in + * order. `page` is used only as a DOM parser. + */ +export async function readBook( + page: Page, + bookFolder: string, +): Promise<IBookContents> { + const html = fs.readFileSync(bookHtmlPath(bookFolder), "utf8"); + return page.evaluate((source) => { + const document = new DOMParser().parseFromString(source, "text/html"); + const styleElement = document.querySelector( + 'style[title="userModifiedStyles"]', + ); + const pages = [ + ...document.querySelectorAll("div.bloom-page.numberedPage"), + ].map((pageDiv) => ({ + id: pageDiv.id, + lineage: pageDiv.getAttribute("data-pagelineage") ?? "", + styleClasses: [ + ...new Set( + [...pageDiv.querySelectorAll(".bloom-editable")].flatMap( + (editable) => + [...editable.classList].filter((c) => + c.endsWith("-style"), + ), + ), + ), + ].sort(), + imageSources: [...pageDiv.querySelectorAll("img")].map( + (img) => img.getAttribute("src") ?? "", + ), + audioSentenceIds: [ + ...pageDiv.querySelectorAll(".audio-sentence"), + ].map((span) => span.id), + videoSources: [...pageDiv.querySelectorAll("video source")].map( + (source) => source.getAttribute("src") ?? "", + ), + layout: [...pageDiv.querySelectorAll(".split-pane")].map( + (split) => { + const orientation = split.classList.contains( + "horizontal-percent", + ) + ? "horizontal" + : "vertical"; + // The inline style is where origami records the split percentage. + const sizeOf = (position: string) => + split + .querySelector( + `:scope > .split-pane-component.position-${position}`, + ) + ?.getAttribute("style") ?? ""; + const [first, second] = + orientation === "horizontal" + ? ["top", "bottom"] + : ["left", "right"]; + return `${orientation} ${sizeOf(first)} | ${sizeOf(second)}`; + }, + ), + })); + return { + pages, + userModifiedStyles: styleElement?.textContent ?? "", + }; + }, html); +} + +/** + * Wait until the book on disk has `count` numbered pages, then return it. Bloom saves after the + * edit, not with it, so a test that reads the file the moment a click returns can read the old + * one. This polls the file rather than sleeping. + */ +export async function waitForBookWithPageCount( + page: Page, + bookFolder: string, + count: number, + timeoutMs = 60000, +): Promise<IBookContents> { + // Return the very read that satisfied the check. A second read after the poll could catch + // Bloom mid-write and hand back a different, half-written file. + let book: IBookContents | undefined; + await expect + .poll( + async () => { + book = await readBook(page, bookFolder); + return book.pages.length; + }, + { + timeout: timeoutMs, + message: + `${bookHtmlPath(bookFolder)} never came to have ${count} numbered pages. ` + + `Bloom may not have saved the change.`, + }, + ) + .toBe(count); + return book!; +} + +/** True if `relativePath` (as a page's markup names it) exists inside the book folder. */ +export function bookFileExists( + bookFolder: string, + relativePath: string, +): boolean { + // A video source carries a trim fragment, e.g. "video/x.mp4#t=0.0,2.0"; the file is the + // part before it. + const file = relativePath.split("#")[0]; + return fs.existsSync(Path.join(bookFolder, file)); +} diff --git a/src/BloomE2E/helpers/pageThumbnails.ts b/src/BloomE2E/helpers/pageThumbnails.ts new file mode 100644 index 000000000000..adabd90e8195 --- /dev/null +++ b/src/BloomE2E/helpers/pageThumbnails.ts @@ -0,0 +1,290 @@ +// Drive the Edit tab's page thumbnail list: select a page, open its menu, and run a command. +// +// Three facts about this pane shape everything here, and each one has cost someone time: +// +// 1. The list lives in the Edit tab's `#pageList` iframe, but React renders it into +// `#pageGridWrapper`, replacing the `#pageGrid` div the pug file declares. So the +// thumbnails are under `#pageGridWrapper`, not `#pageGrid`. +// 2. The menu itself is rendered with a portal into the PARENT document, so that it can +// extend past the narrow iframe. A test therefore clicks in the iframe to open it and +// then queries the shell page for the items. +// 3. Opening the menu on a thumbnail that is not already selected does nothing at all +// (pageThumbnailList.tsx bails out). Selection is a round trip through C# and back over +// a websocket, so a test has to wait for the selection to arrive before it opens the menu. +// +// The menu items are matched by their English labels because they carry no test ids; see the +// entry in AUTOMATION-DEBT.md. + +import { + expect, + type FrameLocator, + type Locator, + type Page, +} from "@playwright/test"; +import { apiGet } from "./api"; + +/** A command in a page thumbnail's context menu, named as the menu shows it in English. */ +export type PageMenuCommand = + | "Copy Page" + | "Paste Page" + | "Duplicate Page" + | "Choose Different Layout" + | "Remove Page"; + +// Bloom's own name for each command, which is what the enabled/clicked APIs speak. +const COMMAND_ID: Record<PageMenuCommand, string> = { + "Copy Page": "copyPage", + "Paste Page": "pastePage", + "Duplicate Page": "duplicatePage", + "Choose Different Layout": "chooseDifferentLayout", + "Remove Page": "removePage", +}; + +/** The page-thumbnail pane's iframe inside the Edit tab. */ +export function pageListFrame(page: Page): FrameLocator { + return page.frameLocator("#pageList"); +} + +/** Every real page thumbnail, in the order the pane shows them. */ +function thumbnails(page: Page): Locator { + return pageListFrame(page).locator( + "#pageGridWrapper .gridItem:not(.placeholder)", + ); +} + +/** + * Wait until the thumbnail pane has finished loading and report the page ids it shows, in + * order. A thumbnail's element id is the page's own id, which is what the saved book HTML + * uses too, so this is how a test ties a thumbnail to a page in the file. + */ +export async function getPageIds( + page: Page, + timeoutMs = 60000, +): Promise<string[]> { + await thumbnails(page).first().waitFor({ timeout: timeoutMs }); + return thumbnails(page).evaluateAll((elements) => + elements.map((element) => element.id), + ); +} + +/** Wait until the thumbnail pane shows exactly `count` pages. */ +export async function waitForPageCount( + page: Page, + count: number, + timeoutMs = 60000, +): Promise<void> { + await expect + .poll(async () => (await getPageIds(page, timeoutMs)).length, { + timeout: timeoutMs, + message: `The page thumbnail list never showed ${count} pages.`, + }) + .toBe(count); +} + +/** + * Click a page's thumbnail and wait for Bloom to report it selected. The click target is the + * transparent cover over the thumbnail, which is what a person hits; the thumbnail's own + * content ignores clicks. + */ +export async function selectPage( + page: Page, + pageId: string, + timeoutMs = 60000, +): Promise<void> { + // An attribute selector, not `#id`: Bloom's page ids are GUIDs that may start with a + // digit, which a bare id selector cannot express. + const thumbnail = pageListFrame(page).locator( + `#pageGridWrapper .gridItem[id="${pageId}"]`, + ); + await thumbnail.waitFor({ timeout: timeoutMs }); + await thumbnail + .locator(".invisibleThumbnailCover") + .click({ timeout: timeoutMs }); + // Selection goes to C# and comes back over a websocket, so it is not done when the click is. + await expect(thumbnail).toHaveClass(/gridSelected/, { timeout: timeoutMs }); + await waitForEditablePage(page, pageId, timeoutMs); +} + +/** + * Wait until the Edit tab is actually showing `pageId` and has finished loading it. + * + * This matters more than it looks. While the page is still loading, Bloom's editing model is in + * its Navigating state, and several commands — Copy Page among them — quietly do nothing at all + * in that state rather than failing. A test that clicks Copy Page too early gets no error and an + * empty clipboard. + * + * So this waits on two things: the page's own document, and Bloom's editing state, which it reads + * through the e2e/isEditingPage hook. The document reaches its final state first. + */ +export async function waitForEditablePage( + page: Page, + pageId: string, + timeoutMs = 60000, +): Promise<void> { + await page + .frameLocator("#page") + .locator(`.bloom-page[id="${pageId}"]`) + .waitFor({ state: "attached", timeout: timeoutMs }); + // The page's own script tells Bloom it is ready to edit once its DOM has loaded, so wait for + // the document to be fully loaded rather than merely parsed. + await expect + .poll( + () => + page.evaluate(() => { + const frame = document.querySelector( + "#page", + ) as HTMLIFrameElement | null; + return frame?.contentDocument?.readyState ?? "none"; + }), + { + timeout: timeoutMs, + message: `The Edit tab never finished loading page ${pageId}.`, + }, + ) + .toBe("complete"); + // The document can be complete a moment before Bloom is, so ask Bloom itself: until its + // editing model leaves Navigating, it silently ignores a command that starts by saving the + // page, and Copy Page is one of those. + await expect + .poll(async () => (await apiGet(page, "e2e/isEditingPage")).body, { + timeout: timeoutMs, + message: `Bloom never finished loading page ${pageId} in the Edit tab (its editing state never became Editing).`, + }) + .toBe("true"); +} + +// Property name put on the editable page's document so a later poll can tell whether it is still +// the same document or a reload has replaced it. Bloom reloads a page to the SAME url (the +// in-memory file is named after the page id), so the url cannot answer that question. +const RELOAD_MARKER = "__bloomE2eEditablePageMarker"; + +/** + * Mark the document now showing in the Edit tab, so waitForEditablePageReload can tell when + * Bloom has replaced it. Call this before a command that reloads the page. + */ +export async function markEditablePage(page: Page): Promise<void> { + await page.evaluate((marker) => { + const document_ = ( + document.querySelector("#page") as HTMLIFrameElement | null + )?.contentDocument; + if (!document_) + throw new Error( + "The Edit tab is not showing a page, so there is nothing to mark.", + ); + (document_ as unknown as Record<string, boolean>)[marker] = true; + }, RELOAD_MARKER); +} + +/** + * Wait out the page reload that follows a command which saves the book, and for the reloaded + * page to finish loading. Copy Page is one such command: it saves first, so that unsaved typing + * is copied too, and Bloom then navigates back to the page. Until that navigation finishes the + * editing model is in its Navigating state, in which Paste Page silently does nothing while the + * menu still offers it. + * + * Call markEditablePage() before the command. + */ +export async function waitForEditablePageReload( + page: Page, + pageId: string, + timeoutMs = 60000, +): Promise<void> { + await expect + .poll( + () => + page.evaluate( + (options) => { + const document_ = ( + document.querySelector( + "#page", + ) as HTMLIFrameElement | null + )?.contentDocument; + if (!document_) return "no document"; + if ( + (document_ as unknown as Record<string, boolean>)[ + options.marker + ] + ) + return "not reloaded yet"; + if ( + !document_.querySelector( + `.bloom-page[id="${options.pageId}"]`, + ) + ) + return "showing some other page"; + return document_.readyState; + }, + { marker: RELOAD_MARKER, pageId }, + ), + { + timeout: timeoutMs, + message: `The Edit tab never reloaded page ${pageId}.`, + }, + ) + .toBe("complete"); +} + +/** + * Wait until Bloom would enable `command` for `pageId`. This is the same question the menu asks + * as it opens, and it has to be settled first, because the commands run asynchronously: Copy + * Page returns long before the page is on Bloom's clipboard, so a menu opened straight after it + * shows Paste Page still greyed out. + */ +export async function waitForPageMenuCommandEnabled( + page: Page, + pageId: string, + command: PageMenuCommand, + timeoutMs = 30000, +): Promise<void> { + await expect + .poll( + async () => + ( + await apiGet( + page, + `pageList/contextMenuItemEnabled?commandId=${COMMAND_ID[command]}` + + `&pageId=${encodeURIComponent(pageId)}`, + ) + ).body, + { + timeout: timeoutMs, + message: `Bloom never enabled the page menu's "${command}" command.`, + }, + ) + .toBe("true"); +} + +/** + * Open a page's context menu with the chevron button the pane shows on it, and run one command. + * The page must already be selected (see selectPage): the menu refuses to open on any other one. + * + * This is the real user gesture for Copy Page and Paste Page, which is why the copy-page test + * goes through here rather than posting pageList/contextMenuItemClicked. + */ +export async function runPageMenuCommand( + page: Page, + pageId: string, + command: PageMenuCommand, + timeoutMs = 30000, +): Promise<void> { + await waitForPageMenuCommandEnabled(page, pageId, command, timeoutMs); + await pageListFrame(page) + .locator("#menuIconHolder") + .click({ timeout: timeoutMs }); + const item = pageMenuItem(page, command); + await item.waitFor({ timeout: timeoutMs }); + await expect( + item, + `The page menu's "${command}" command is disabled.`, + ).toBeEnabled({ timeout: timeoutMs }); + await item.click(); + await expect(page.getByRole("menu")).toHaveCount(0, { timeout: timeoutMs }); +} + +/** + * The menu item for a command, in the shell page (the menu is portaled out of the iframe). + * Exported so a test can assert on a command's enabled state without running it. + */ +export function pageMenuItem(page: Page, command: PageMenuCommand): Locator { + return page.getByRole("menuitem", { name: command, exact: true }); +} diff --git a/src/BloomE2E/tests/copy-page.spec.ts b/src/BloomE2E/tests/copy-page.spec.ts new file mode 100644 index 000000000000..c6abb13f5761 --- /dev/null +++ b/src/BloomE2E/tests/copy-page.spec.ts @@ -0,0 +1,224 @@ +// Copying a page must carry everything on it: the custom style, the image, the Talking Book +// recording, the video, and the custom origami layout — both when the page is pasted back into +// its own book and when it is pasted into another book in the same Bloom. +// +// This automates the manual case "Copy Page Preserves Everything". Copying between two separate +// Bloom instances is a third case in the manual test; it is known not to work in 6.5 and is out +// of scope here, so this test is Partial coverage. See AUTOMATION-DEBT.md. +// +// The page under test cannot be built through the UI: adding an image, a recording, or a video +// needs a native file dialog or a microphone, which an e2e test must never open. So the page +// comes ready-made from the `page-copy` collection in bloom-testing-inputs. The copy and the +// paste themselves, which are what this test measures, go through the real page menu. + +import * as Path from "node:path"; +import { expect, test } from "../fixtures/bloomTest"; +import { + bookFileExists, + readBook, + waitForBookWithPageCount, + type IPageContents, +} from "../helpers/bookHtml"; +import { selectBook } from "../helpers/collection"; +import { + getPageIds, + markEditablePage, + runPageMenuCommand, + selectPage, + waitForEditablePageReload, + waitForPageCount, +} from "../helpers/pageThumbnails"; +import { switchTab } from "../helpers/workspace"; + +test.use({ collectionName: "page-copy" }); + +// The layout the fixture page was built from. Bloom's "Custom" page template, whose id is fixed +// in src/content/templates/template books/standard-page-mixins.pug. +const CUSTOM_LAYOUT_TEMPLATE_ID = "5dcd48df-e9ab-4a07-afd4-6a24d0398386"; + +// The user-defined style the fixture's page carries. Bloom keeps the rule for it in the book's +// own userModifiedStyles block, so pasting into another book has to carry the rule across too. +const CUSTOM_STYLE_CLASS = "PageCopyMarker-style"; + +/** + * Fail unless the page really has all five things the manual test says a copy must preserve. + * Run against the ORIGINAL before anything is copied, so a later pass cannot be vacuous, and + * against each pasted page afterwards. + */ +function expectPageHasEverything( + page: IPageContents, + bookFolder: string, + what: string, +): void { + expect(page.lineage, `${what}: the page's layout template`).toContain( + CUSTOM_LAYOUT_TEMPLATE_ID, + ); + expect(page.styleClasses, `${what}: the custom style class`).toContain( + CUSTOM_STYLE_CLASS, + ); + + expect(page.imageSources, `${what}: images on the page`).toHaveLength(1); + expect( + bookFileExists(bookFolder, page.imageSources[0]), + `${what}: the image file ${page.imageSources[0]} is missing from ${bookFolder}`, + ).toBe(true); + + expect( + page.audioSentenceIds, + `${what}: recorded sentences on the page`, + ).toHaveLength(1); + expect( + bookFileExists(bookFolder, `audio/${page.audioSentenceIds[0]}.mp3`), + `${what}: the recording audio/${page.audioSentenceIds[0]}.mp3 is missing from ${bookFolder}`, + ).toBe(true); + + expect(page.videoSources, `${what}: videos on the page`).toHaveLength(1); + expect( + bookFileExists(bookFolder, page.videoSources[0]), + `${what}: the video file ${page.videoSources[0]} is missing from ${bookFolder}`, + ).toBe(true); + + // Three slots, so two splits: the image over the rest, then the video over the text. + expect(page.layout, `${what}: the page's origami layout`).toHaveLength(2); +} + +test("copying a page preserves everything, within and between books [Test Case ID 348]", async ({ + page, + bloomApp, +}) => { + const sourceBook = Path.join(bloomApp.collectionDir, "Copy Source"); + const destinationBook = Path.join( + bloomApp.collectionDir, + "Copy Destination", + ); + + // ---- The page we are about to copy really does have all five ingredients ---------------- + await selectBook(page, sourceBook); + await switchTab(page, "edit"); + + const sourceBefore = await readBook(page, sourceBook); + expect( + sourceBefore.pages, + "The source book should start with two numbered pages.", + ).toHaveLength(2); + const original = sourceBefore.pages[0]; + expectPageHasEverything(original, sourceBook, "the original page"); + expect( + sourceBefore.userModifiedStyles, + "The source book should define the custom style before anything is copied.", + ).toContain(`.${CUSTOM_STYLE_CLASS}`); + + // ---- Copy and paste it, through the page menu, inside its own book ----------------------- + const pageIdsBefore = await getPageIds(page); + expect( + pageIdsBefore, + "The page under test should be a thumbnail in the pane.", + ).toContain(original.id); + + await selectPage(page, original.id); + await markEditablePage(page); + await runPageMenuCommand(page, original.id, "Copy Page"); + // Copy Page saves the book first, which makes Bloom reload the page. Pasting before that + // finishes does nothing at all, so wait for the page to come back. + await waitForEditablePageReload(page, original.id); + await runPageMenuCommand(page, original.id, "Paste Page"); + await waitForPageCount(page, pageIdsBefore.length + 1); + + // Leaving the Edit tab makes Bloom save the book, which is what puts the pasted page in the + // file we are about to read. It is also the way back to the collection for the second half. + await switchTab(page, "collection"); + const sourceAfter = await waitForBookWithPageCount(page, sourceBook, 3); + + const originalIndex = sourceAfter.pages.findIndex( + (p) => p.id === original.id, + ); + expect( + originalIndex, + "The original page should still be in the source book after the paste.", + ).toBeGreaterThanOrEqual(0); + const copyInSameBook = sourceAfter.pages[originalIndex + 1]; + expect( + copyInSameBook, + "Bloom should have inserted the pasted page after the page that was copied.", + ).toBeDefined(); + expect( + copyInSameBook.id, + "The pasted page should be a new page, not the one that was copied.", + ).not.toBe(original.id); + + expectPageHasEverything( + copyInSameBook, + sourceBook, + "the page pasted into the same book", + ); + expect( + copyInSameBook.styleClasses, + "The pasted page should carry exactly the styles the original had.", + ).toEqual(original.styleClasses); + expect( + copyInSameBook.layout, + "The pasted page should have the original's custom layout.", + ).toEqual(original.layout); + expect( + copyInSameBook.imageSources, + "The pasted page should show the same image file.", + ).toEqual(original.imageSources); + + // ---- Paste the same page into a different book in the same Bloom ------------------------- + await selectBook(page, destinationBook); + await switchTab(page, "edit"); + + const destinationBefore = await readBook(page, destinationBook); + expect( + destinationBefore.pages, + "The destination book should start with one numbered page.", + ).toHaveLength(1); + expect( + destinationBefore.userModifiedStyles, + "The destination book should not know the custom style before the paste.", + ).not.toContain(`.${CUSTOM_STYLE_CLASS}`); + const destinationPageBefore = destinationBefore.pages[0]; + + // The thumbnail pane counts front and back matter too, so compare against what it shows now + // rather than against the book's numbered-page count. + const destinationThumbnailsBefore = (await getPageIds(page)).length; + await selectPage(page, destinationPageBefore.id); + await runPageMenuCommand(page, destinationPageBefore.id, "Paste Page"); + await waitForPageCount(page, destinationThumbnailsBefore + 1); + + await switchTab(page, "collection"); + const destinationAfter = await waitForBookWithPageCount( + page, + destinationBook, + 2, + ); + + const copyInOtherBook = destinationAfter.pages.find( + (p) => p.id !== destinationPageBefore.id, + )!; + expect( + copyInOtherBook, + "The destination book should have gained the pasted page.", + ).toBeDefined(); + + // Every file the page refers to has to have come across into this book's own folder. + expectPageHasEverything( + copyInOtherBook, + destinationBook, + "the page pasted into the other book", + ); + expect( + copyInOtherBook.styleClasses, + "The page pasted into the other book should carry the original's styles.", + ).toEqual(original.styleClasses); + expect( + copyInOtherBook.layout, + "The page pasted into the other book should keep the original's custom layout.", + ).toEqual(original.layout); + + // The class alone would render as plain text; the rule that defines it has to travel too. + expect( + destinationAfter.userModifiedStyles, + "Bloom should have copied the custom style's rule into the destination book.", + ).toContain(`.${CUSTOM_STYLE_CLASS}`); +});