From 4132a8d256147df8550d8a3fdccc80de1954dbb0 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 1 Sep 2026 15:51:02 -0600 Subject: [PATCH 1/8] Automate "Copy Page Preserves Everything" (Test Case ID 348) An e2e test that copies a page through the real page menu and pastes it, first back into its own book and then into a second book in the same Bloom. For each paste it checks the five things the manual case names: the user-defined style class and its rule, the image file, the Talking Book recording file, the video file, and the custom origami layout. The original page is checked first, so the test cannot pass on an empty page. The page under test comes prebuilt from the new page-copy collection in bloom-testing-inputs, because adding an image, a recording, or a video through the UI needs a native dialog or a microphone. Until that collection merges and the pin advances, run with BLOOM_TESTING_INPUTS_DIR pointed at a checkout of it. Two new helpers: pageThumbnails.ts drives the page thumbnail menu, and bookHtml.ts reads a saved book from disk and reports each page's ingredients. Copying between two Bloom instances, the manual case's last step, is not covered: the page clipboard is a field on the one EditingModel, so nothing crosses a process boundary. AUTOMATION-DEBT.md records that, the missing test ids on the page menu items, and a product gap the test exposed: Copy Page and Paste Page silently do nothing while a page is loading, though the menu shows them enabled. Co-Authored-By: Claude Fable 5.1 --- src/BloomE2E/AUTOMATION-DEBT.md | 39 ++++ src/BloomE2E/helpers/bookHtml.ts | 155 ++++++++++++++ src/BloomE2E/helpers/pageThumbnails.ts | 278 +++++++++++++++++++++++++ src/BloomE2E/tests/copy-page.spec.ts | 224 ++++++++++++++++++++ 4 files changed, 696 insertions(+) create mode 100644 src/BloomE2E/helpers/bookHtml.ts create mode 100644 src/BloomE2E/helpers/pageThumbnails.ts create mode 100644 src/BloomE2E/tests/copy-page.spec.ts diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index 3ffc24a1189f..3f8f35af1cf4 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -67,6 +67,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 @@ -171,3 +177,36 @@ test can currently clear a box by any faster route. Fix direction: understand wh CKEditor does with a programmatic value change; a supported "set the text of this box" 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, and the Notion case stays Partial. 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.) 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 { + 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 { + // 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..37889fbbc176 --- /dev/null +++ b/src/BloomE2E/helpers/pageThumbnails.ts @@ -0,0 +1,278 @@ +// 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 = { + "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 { + 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 { + 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 { + // 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. + */ +export async function waitForEditablePage( + page: Page, + pageId: string, + timeoutMs = 60000, +): Promise { + 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"); +} + +// 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 { + 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)[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 { + 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)[ + 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 { + 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 { + 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}`); +}); From 81a530ae24b22b333925338a9c74c36842414a93 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 1 Sep 2026 16:24:34 -0600 Subject: [PATCH 2/8] Advance the testing-inputs pin to the page-copy collection The copy-page e2e test (Test Case ID 348) reads its books from the page-copy collection, which merged into bloom-testing-inputs as aa2e7c2. With the pin at that commit the test runs from output/testing-inputs with no override variable. Co-Authored-By: Claude Fable 5.1 --- build/testing-inputs.pin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/testing-inputs.pin b/build/testing-inputs.pin index 9f2a83d89187..61ec65f8d2c9 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=cd0df0a7310312ff1e014716d89c39e00ef0615d +commit=aa2e7c2d31bdef05eb6211bb4ef076135ccbbb11 From 2dddd2c6eede95b4451a184cc642af0ce04d97fc Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 1 Sep 2026 17:58:17 -0600 Subject: [PATCH 3/8] Add the improve-test-automation-coverage skill A controller agent claims Notion test cases whose Automation property is Planned (setting Building at once, so parallel developers do not collide), starts one Orca worktree and one supervised Claude worker per card, reviews each worker's e2e test, and lets the worker run preflight and open a draft PR that links the Notion card. Cards end as "PR Pending" with the PR URL, or as "Has automation problems" with a note for the card's author when the case is not automatable as written. An unattended mode lets the run continue through the Planned cards with nobody answering questions. Beside the skill: notion_automation.py (list, show, claim, set, note, brief; retries on HTTP 429), e2e-lock.mjs (a machine-wide lock so worktrees take turns running Bloom e2e tests), and the worker brief template. A stub under .claude/skills makes the skill a slash command. add-e2e-test gains the two new Automation states, PR Pending and Has automation problems. Co-Authored-By: Claude Fable 5.1 --- .../improve-test-automation-coverage/SKILL.md | 11 + .github/skills/add-e2e-test/SKILL.md | 13 +- .../improve-test-automation-coverage/SKILL.md | 231 +++++++++++++++++ .../e2e-lock.mjs | 153 +++++++++++ .../notion_automation.py | 239 ++++++++++++++++++ .../worker-brief.md | 150 +++++++++++ 6 files changed, 794 insertions(+), 3 deletions(-) create mode 100644 .claude/skills/improve-test-automation-coverage/SKILL.md create mode 100644 .github/skills/improve-test-automation-coverage/SKILL.md create mode 100644 .github/skills/improve-test-automation-coverage/e2e-lock.mjs create mode 100644 .github/skills/improve-test-automation-coverage/notion_automation.py create mode 100644 .github/skills/improve-test-automation-coverage/worker-brief.md diff --git a/.claude/skills/improve-test-automation-coverage/SKILL.md b/.claude/skills/improve-test-automation-coverage/SKILL.md new file mode 100644 index 000000000000..2b134105eaeb --- /dev/null +++ b/.claude/skills/improve-test-automation-coverage/SKILL.md @@ -0,0 +1,11 @@ +--- +name: improve-test-automation-coverage +description: '"improve-test-automation-coverage N" — claim N Notion test cases whose Automation is Planned and automate each in its own Orca worktree with a supervised Claude Fable 5.1 worker. Use when the developer says "improve test automation coverage", "burn tokens on test automation", or "automate N planned tests".' +argument-hint: "N — how many Planned test cases to automate in parallel (default 3); or 'case 349,350' to name the cards" +user-invocable: true +--- + +This is the slash-command entry point only. The procedure, the helper scripts, and the worker +brief live in `.github/skills/improve-test-automation-coverage/`. Open +`.github/skills/improve-test-automation-coverage/SKILL.md` and follow it exactly, with the +argument as N. diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 22f7db7ddfe0..930980785476 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -55,7 +55,8 @@ carries the API mechanics. 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`. The title string is the whole mechanism; + 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; the library provides no helper or annotation for it, deliberately, so that grepping `Test Case ID` across `src/BloomE2E/tests/` finds every tie. - **Writing a new e2e test that has no manual card:** add a row to the inventory so it @@ -63,14 +64,20 @@ carries the API mechanics. `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` → `Automated` (or `Partial`), with `Keep manual` - as the deliberate opt-out. + `Manual` → `Planned` → `Building` → `PR Pending` → `Automated` (or `Partial`), 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. + - `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. + - `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. - `Keep manual` is a deliberate decision that a case stays human-run (e.g. installer feel, print quality); do not propose those for automation. - Do not confuse `Test Case ID` (the number, stable, ours) with `Dokimion ID` (e.g. diff --git a/.github/skills/improve-test-automation-coverage/SKILL.md b/.github/skills/improve-test-automation-coverage/SKILL.md new file mode 100644 index 000000000000..b3fb4afafafe --- /dev/null +++ b/.github/skills/improve-test-automation-coverage/SKILL.md @@ -0,0 +1,231 @@ +--- +name: improve-test-automation-coverage +description: '"improve-test-automation-coverage N" — take N test cases whose Notion Automation property is Planned, claim each one (set it to Building at once), and automate each in its own Orca worktree with a supervised Claude Fable 5.1 worker that follows add-e2e-test. The controller reviews every worker''s test, sends fixes, then lets the worker run preflight and open a draft PR that links the Notion card; the card ends as "PR Pending". Use when the developer says "improve test automation coverage", "burn tokens on test automation", or "automate N planned tests".' +argument-hint: "N — how many Planned test cases to automate in parallel (default 3); or 'case 349,350' to name the cards" +user-invocable: true +--- + +# Improve test automation coverage + +Turn N manual test cases into edge-to-edge tests, in parallel, with one Orca worktree and one +Claude Fable 5.1 worker per case. You are the **controller**: you pick and claim the cases, start +the workers, answer their review requests, and report. The workers write the tests and open the +PRs. The developer answers worker questions in Orca. + +Files beside this skill: + +- `notion_automation.py` — list, show, claim, and update cards. Run it with `py`. +- `e2e-lock.mjs` — the machine-wide lock every Playwright run goes through. Workers use it; you + use it too if you ever run a test yourself. +- `worker-brief.md` — the task brief template for a worker. + +Related skills: `.github/skills/add-e2e-test/SKILL.md` (what a worker follows), `orchestration` +and `orca-cli` (Orca mechanics; run `orca skills get orchestration` first, the guide is +version-matched), `preflight` (the worker runs it to open the PR), `personal-board`. + +## Authorization + +Invoking this skill authorizes, for this run only: setting the `Automation` and +`Automation Notes` properties of Notion test cards; creating Orca worktrees and workers; and, +inside each worker, everything `preflight` authorizes (commit, push, draft PR, bot replies). Still +forbidden: marking a PR ready for review, moving any Orca card to Peer Review, setting a card to +`Automated` or `Keep manual` (a human does both, after merge or after judging the case). + +## Constants + +- Notion database "Test Case Runs", suite run `6.5` (the current release; change `--suite` when + the team moves on). Token: `BLOOM_TESTCASE_NOTION`, Windows User scope. In PowerShell: + `$env:BLOOM_TESTCASE_NOTION = [Environment]::GetEnvironmentVariable('BLOOM_TESTCASE_NOTION','User')` +- Base branch for every worktree and PR: `master`. The BloomE2E system lives on master; pass + `--base-branch master` explicitly, do not rely on the Orca repo default. This is the + exception `AGENTS.md` allows to its "new work targets Version6.5" rule: an e2e test cannot + target a branch that has no `src/BloomE2E`. +- Worker agent: `claude`, model `claude-fable-5-1`. +- Default N: 3. N is a maximum; if fewer Planned cards exist, take what there is and say so. +- Worktree name: `tc-`, for example + `tc349-duplicate-page`. There is no YouTrack card for this work, so the `BL-` prefix rule does + not apply; `preflight` finding no ticket id is the expected outcome. + +## 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 +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 +the candidate list until that developer fixes it and sets `Planned` again. A worker blocked by the +environment (broken build, no PR possible) sets the card back to `Planned` with a `Blocked:` +note instead, because the card itself is fine. + +## Unattended mode + +The developer is away and nobody answers questions. Two things change; everything else stays the same. + +- **Workers never ask the developer.** The brief (built with `brief ... --unattended`) tells the worker + to decide for itself. A card that leaves a question of intent open, or that needs a human + judgment such as "is this too hard", goes to `Has automation problems` with the unanswered + questions in the note, so the developer can answer them on the card later. A small ambiguity that a + careful tester would resolve the same way gets the conservative reading, written down in the + PR description and the `PR Pending` note. +- **You keep N workers busy until no `Planned` card is left to take.** When a worker settles without a + PR (`HAS AUTOMATION PROBLEMS`, or a blocked card), release it, claim the next `Planned` card + by the ordering rule in Step 1, and start a new worker for it, so that N workers run at any + time. Stop claiming when `list-planned` returns nothing you may take, or when this run has + started 3 × N workers, whichever comes first; the cap keeps a broken environment from + burning through every card. A worker that opens a PR is a success and is not replaced beyond + that cap either. Report at the end as usual, with one row per card tried. + +In unattended mode the worker still asks you for review; you reply `ship` or `fixes` yourself. +`preflight` is autonomous already and batches its decisions into the PR report. + +## Step 1 — Pick and claim the cases + +**Claim first, spawn later.** Other developers run this skill on other machines. The window +between reading `Planned` and writing `Building` is the only race, so keep it short: claim each +card the moment you decide to take it, before you create anything else for it. + +1. Resolve the argument. A bare number is N (default 3). `case [,...]` names the + cards to take instead, for example `case 349` or `case 349,350`; then N is the count of ids + and the ordering rule below does not apply, but the `Planned` check and the claim do. The + word `unattended` anywhere in the argument selects unattended mode (see below); it + combines with either form, for example `3 unattended` or `case 349 unattended`. +2. List candidates: + ```powershell + py \notion_automation.py list-planned + ``` +3. Skip a card whose `automationNotes` contains `[improve-test-automation-coverage` and + `Blocked:` (an earlier run hit an environment problem). Show the developer those skips in the final + report; do not retry them without being asked. Cards with automation problems are not in + the `Planned` list at all. +4. Take the first N remaining cards, lowest `Test Case ID` first, and claim each one at once: + ```powershell + py \notion_automation.py claim + ``` + Exit code 3 means someone else got there first; take the next candidate instead. Keep going + until you hold N claims or run out of candidates. +5. Only now read each claimed card (`show `) so the brief can carry its title. + +## Step 2 — Start one worker per claimed card + +Confirm Orca is up (`orca status --json`) and read the version-matched guide +(`orca skills get orchestration`). Then: + +```powershell +orca orchestration run-create --objective "improve-test-automation-coverage: automate Notion test cases " --json +``` + +For each claimed card: + +1. Build the brief with the script, never with sed or a regex (a regex eats the backslashes in + the skill path; the first run produced a brief with bell characters in it): + ```powershell + py \notion_automation.py brief --out %LOCALAPPDATA%\Bloom\e2e-briefs\brief-.md + ``` + Add `--unattended` in unattended mode; it swaps the "ask the developer" rules for "decide yourself". + Write the brief where the worker can read it and where it outlives your scratchpad: the folder + above (the script creates it) or another durable folder such as + `\..\e2e-briefs\`. Do not paste the brief into the task spec: Orca types the spec into + the worker's terminal, and a 140-line paste has arrived truncated and unsubmitted. The spec is + two sentences: + `Read the file and follow it exactly; it is your whole task. Report with worker_done as it says.` +2. Find the Orca repo that owns this checkout: `orca repo list --json`. For BloomDesktop on this + machine that is `path:D:/bloom`. A worktree path such as `D:/automate-notion-test` is not a + repo and `worker-start` answers `repo_not_found`. +3. Create the task and start the worker in a fresh top-level worktree off master: + ```powershell + orca orchestration task-create --spec "Read the file and follow it exactly; it is your whole task. Report with worker_done as it says." --json + orca orchestration worker-start --task --worktree new-top-level --name tc- --repo path: --base-branch master --agent claude --model claude-fable-5-1 --setup run --json + ``` + Read the receipt: `ready` with setup `running` is normal. A failed start exits nonzero; read + its `stage` and `effects`, fix the cause, and start again with `--retry-of `. + If the PowerShell tool refuses the command, run the same command through the Bash tool; the + first run saw the auto-mode classifier deny it once in PowerShell and accept it in Bash. + About a minute after the start, read the worker's terminal (`orca terminal read`) and confirm + the agent is working on the brief, not sitting at an empty prompt or on an unsubmitted paste. + If it sits, the spec never arrived: send it again with + `orca terminal send --terminal --text "" --enter --json`. + Do not leave a claimed card without a worker: if you cannot start one, set the card back to + `Planned` with a `[improve-test-automation-coverage ] Blocked: ` note. +4. Expect a `status` message `Setup failed for worker ` soon after the start. The + repo setup hook (`./init.sh`) runs under `cmd.exe` with `NoDefaultCurrentDirectoryInExePath=1` + inherited from Claude Code and dies with `'.' is not recognized`. That is an agent-session + artifact, not a repo fault (see the team rules). The brief tells the worker to run + `./init.sh` itself, so you do nothing about this message except acknowledge it. +5. Record `task_id`, `dispatch_id`, worktree path, and Test Case ID in a table in your + scratchpad. You will need all four for every later message. + +Start all N workers before you wait on any of them. + +## Step 3 — Supervise: answer review requests, relay nothing else + +Workers ask the developer their own questions with `AskUserQuestion` in their terminals. You do not +relay those. You handle three message types: + +```powershell +orca orchestration check --wait --types worker_done,escalation,question --timeout-ms 900000 --json +``` + +Loop until every dispatch has settled. After each batch, acknowledge with +`check --ack --wait ...` and keep waiting; for the last acknowledgement, when no +more messages can come, `check --ack --json` without `--wait` returns at once. A +wait that times out with nothing is normal; a worker can spend an hour in preflight. Do not stop +a worker for being slow. While it waits, `check --wait` prints one `_keepalive` line every 15 +seconds; pipe the output through `Select-String -NotMatch '_keepalive'` or it floods your +context. `status` and `heartbeat` messages need no action beyond the acknowledgement. + +### A `question` whose text starts with `READY FOR REVIEW ` + +The worker has asked you to review its test, and is blocked until you reply. + +1. Review the worktree at the path you recorded. Read the whole diff against `origin/master`, + the new spec file, and any Bloom change (`data-testid`, `E2eTestingApi`). Check it against + `add-e2e-test`: the title carries `[Test Case 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 + 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 + `src/BloomE2E`: + `node \e2e-lock.mjs -- pnpm exec playwright test tests/.spec.ts` +3. Reply once: + - Nothing to fix: `orca orchestration reply --id --body "ship" --json` + - Otherwise: `orca orchestration reply --id --body "fixes: 1. ... 2. ..." --json`. + Number the fixes; say what and why, not how. Expect a second `READY FOR REVIEW` afterwards. + +Cap the loop at three review rounds per worker. If the third round still needs fixes, reply +`ship` with the remaining points listed for the PR description, and put them in your report. + +### Any other `question` + +A worker should ask the developer, not you, about the product; if one asks you anyway, answer from what +you know, or tell it to use `AskUserQuestion`. Never invent an answer about intent. + +### `escalation` or `worker_done` + +Read the body. An `escalation` is a report, not an ending: answer it or act on it, and leave the +worker running. Only a `worker_done` settles the dispatch. + +For each dispatch a `worker_done` settled, release the worker: +`orca orchestration worker-release --dispatch --json`. A result of `retained` +with reason `user_takeover` means the developer typed in that worker's terminal; Orca keeps the terminal +open and there is nothing more to do. Confirm the Notion card is +in the state the outcome implies (`PR Pending` with a PR URL; `Has automation problems` with a +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. + +## Step 4 — Report + +One message to the developer, in this order: + +1. A table: Test Case ID, title, outcome (PR URL / has automation problems / blocked), Orca + worktree name. +2. For each PR, one or two sentences on what the test covers and what it does not, and any + decision preflight left for the developer. +3. The cards this run skipped because of an earlier `[improve-test-automation-coverage` note. +4. Anything that went wrong in the run itself (a worker restart, a lock wait over 30 minutes, a + Notion write that failed) and any papercut you logged. + +Each worker that opened a PR leaves its worktree in Personal Review for the developer to look +at. Do not move anything to Peer Review. diff --git a/.github/skills/improve-test-automation-coverage/e2e-lock.mjs b/.github/skills/improve-test-automation-coverage/e2e-lock.mjs new file mode 100644 index 000000000000..57c6394a5f4e --- /dev/null +++ b/.github/skills/improve-test-automation-coverage/e2e-lock.mjs @@ -0,0 +1,153 @@ +// Run one command while holding the machine-wide Bloom e2e lock. +// +// Several worktrees on one machine must not run the BloomE2E suite at the same time: each run +// launches its own Bloom.exe, and concurrent instances still collide on shared user settings +// (see the bloom-multi-instance notes) and on the visual-regression baselines. This script makes +// the runs take turns. It is deliberately independent of any coordinator: a lock survives as long +// as the process that took it, and a lock whose owner process is gone is stale and gets removed. +// +// Usage (from src/BloomE2E of the worktree under test): +// +// node /e2e-lock.mjs -- pnpm exec playwright test tests/my-test.spec.ts +// node /e2e-lock.mjs --timeout-minutes 90 -- pnpm test +// +// The lock is the directory %LOCALAPPDATA%\Bloom\e2e-run.lock (mkdir is atomic on NTFS). Its +// owner.json records the holder's pid, start time and cwd, so a waiting agent can say who it is +// waiting for. + +import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { spawn } from "node:child_process"; + +const localAppData = process.env.LOCALAPPDATA; +if (!localAppData) + throw new Error("LOCALAPPDATA is not set; this script expects Windows"); +const lockDir = join(localAppData, "Bloom", "e2e-run.lock"); +const ownerFile = join(lockDir, "owner.json"); + +const argv = process.argv.slice(2); +const separator = argv.indexOf("--"); +if (separator < 0 || separator === argv.length - 1) { + console.error( + "usage: node e2e-lock.mjs [--timeout-minutes N] -- [args...]", + ); + process.exit(2); +} +const options = argv.slice(0, separator); +const command = argv.slice(separator + 1); +const timeoutIndex = options.indexOf("--timeout-minutes"); +const timeoutMinutes = + timeoutIndex >= 0 ? Number(options[timeoutIndex + 1]) : 60; +if (!Number.isFinite(timeoutMinutes) || timeoutMinutes <= 0) { + console.error( + `--timeout-minutes needs a positive number, not ${options[timeoutIndex + 1]}`, + ); + process.exit(2); +} +const timeoutMs = timeoutMinutes * 60_000; +const pollMs = 5_000; + +const isAlive = (pid) => { + try { + process.kill(pid, 0); + return true; + } catch (e) { + // EPERM means the process exists but is not ours; ESRCH means it is gone. + return e.code === "EPERM"; + } +}; + +const readOwner = () => { + try { + return JSON.parse(readFileSync(ownerFile, "utf8")); + } catch { + return null; + } +}; + +const tryAcquire = () => { + try { + // The parent may not exist on a machine where Bloom has not run yet; only the lock + // directory itself must be created non-recursively, because that is the atomic step. + mkdirSync(join(localAppData, "Bloom"), { recursive: true }); + mkdirSync(lockDir, { recursive: false }); + } catch (e) { + if (e.code !== "EEXIST") throw e; + return false; + } + writeFileSync( + ownerFile, + JSON.stringify( + { + pid: process.pid, + started: new Date().toISOString(), + cwd: process.cwd(), + command, + }, + null, + 2, + ), + ); + return true; +}; + +const release = () => { + const owner = readOwner(); + if (owner && owner.pid === process.pid) + rmSync(lockDir, { recursive: true, force: true }); +}; + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +const acquire = async () => { + const deadline = Date.now() + timeoutMs; + let announced = false; + while (true) { + if (tryAcquire()) return; + const owner = readOwner(); + if (owner && !isAlive(owner.pid)) { + console.error( + `[e2e-lock] removing stale lock held by dead pid ${owner.pid} (${owner.cwd})`, + ); + rmSync(lockDir, { recursive: true, force: true }); + continue; + } + if (!owner) { + // The directory exists but owner.json is not written yet, or was just removed. Retry soon. + await sleep(500); + continue; + } + if (!announced) { + console.error( + `[e2e-lock] waiting: pid ${owner.pid} has run e2e tests in ${owner.cwd} since ${owner.started}`, + ); + announced = true; + } + if (Date.now() > deadline) { + console.error( + `[e2e-lock] gave up after ${timeoutMs / 60_000} minutes; the lock is still held by pid ${owner.pid}`, + ); + process.exit(3); + } + await sleep(pollMs); + } +}; + +await acquire(); +console.error(`[e2e-lock] acquired; running: ${command.join(" ")}`); + +// One command string with shell:true, so that pnpm/npx shims resolve the same way they do in a +// terminal. Quote an argument only when it contains whitespace. +const quote = (arg) => (/\s/.test(arg) ? `"${arg}"` : arg); +const child = spawn(command.map(quote).join(" "), { + stdio: "inherit", + shell: true, +}); +const forward = (signal) => () => child.kill(signal); +process.on("SIGINT", forward("SIGINT")); +process.on("SIGTERM", forward("SIGTERM")); +child.on("exit", (code, signal) => { + release(); + process.exit(code ?? (signal ? 1 : 0)); +}); +process.on("exit", release); diff --git a/.github/skills/improve-test-automation-coverage/notion_automation.py b/.github/skills/improve-test-automation-coverage/notion_automation.py new file mode 100644 index 000000000000..149eb677f4b0 --- /dev/null +++ b/.github/skills/improve-test-automation-coverage/notion_automation.py @@ -0,0 +1,239 @@ +"""Read and update the `Automation` lifecycle of test cases in the Notion "Test Case Runs" database. + +The improve-test-automation-coverage skill (controller and workers) drives this script so that +no agent hand-rolls Notion REST calls. The token comes from the BLOOM_TESTCASE_NOTION +environment variable (Windows User scope; read it with +[Environment]::GetEnvironmentVariable('BLOOM_TESTCASE_NOTION','User') in PowerShell). + +Usage: + + py notion_automation.py list-planned [--suite 6.5] + Print every card in the suite run whose Automation is Planned, lowest Test Case ID + first, as JSON: one object per card with testCaseId, pageId, url, title, summary, + automation, automationNotes, areas, and priority. + + py notion_automation.py show [--suite 6.5] + Print the card's properties plus its Test Steps (the to_do blocks) as JSON. + + py notion_automation.py claim [--suite 6.5] + Re-read the card; if Automation is still Planned, set it to Building and print + {"claimed": true}. Otherwise print {"claimed": false, "automation": ""} and exit 3. + This is the collision guard between developers who run the skill at the same time. + + py notion_automation.py set [--note ""] [--suite 6.5] + Set Automation to one of Manual, Planned, Building, "PR Pending", Automated, Partial, + "Has automation problems", "Keep manual". With --note, replace Automation Notes with + the text. + + py notion_automation.py note "" [--suite 6.5] + Replace Automation Notes only. + + py notion_automation.py brief --out [--unattended] [--suite 6.5] + Fill worker-brief.md (beside this script) for the card and write it to . With + --unattended the brief tells the worker never to ask the developer and how to decide alone. The + placeholders are replaced literally, with the skill folder written with forward slashes, + so no shell quoting or regex can mangle the path. Use this rather than sed. +""" + +import argparse, datetime, json, os, pathlib, sys, time, urllib.request, urllib.error + +BASE = "https://api.notion.com/v1/" +DATABASE_ID = "38c4bb19-df12-8123-8bc8-e65b962cb12f" +DEFAULT_SUITE = "6.5" +ATTENDED_QUESTIONS_RULE = """- **Ask the developer whenever the card is ambiguous.** Use the `AskUserQuestion` tool in this terminal; + the developer watches Orca and answers there. Ask about intent, scope, and what "pass" means. Do not + ask about things the add-e2e-test skill already decides.""" + +UNATTENDED_QUESTIONS_RULE = """- **Nobody answers questions in this run.** The developer is away. Never use `AskUserQuestion`; a + question in this terminal blocks the run until it times out. Decide for yourself, with these + rules. A small ambiguity that a careful tester would resolve the same way gets the + conservative reading; write the reading down in the PR description and in the `PR Pending` + note. A question of intent that the card leaves open (what counts as pass, which items are in + scope, whether a hard step may be skipped) is a card problem: set `Has automation problems` + with every question you would have asked in the note, and stop. Do not guess at intent.""" + +ATTENDED_UNDECIDED_RULE = "If you cannot decide, ask the developer with `AskUserQuestion`." + +UNATTENDED_UNDECIDED_RULE = ("If you cannot decide, the card is not ready: use the `Has automation problems` path " + "below and put the open questions in the note.") + +STATUSES = ["Manual", "Planned", "Building", "PR Pending", "Automated", "Partial", "Has automation problems", "Keep manual"] + + +def api(method, path, body=None): + """One JSON call to the Notion API. Waits and retries on HTTP 429; raises on any other non-2xx answer. + + Notion rate-limits the token, and a controller plus several workers share this one token, so a + 429 is routine rather than an error. Notion says how long to wait in the Retry-After header. + """ + data = json.dumps(body).encode() if body is not None else None + headers = { + "Authorization": "Bearer " + os.environ["BLOOM_TESTCASE_NOTION"], + "Notion-Version": "2022-06-28", + "Content-Type": "application/json", + } + for attempt in range(6): + request = urllib.request.Request(BASE + path, data=data, method=method, headers=headers) + try: + return json.load(urllib.request.urlopen(request)) + except urllib.error.HTTPError as e: + if e.code == 429 and attempt < 5: + wait = float(e.headers.get("Retry-After", "10")) + sys.stderr.write(f"[notion] 429 rate limited; waiting {wait:.0f}s (attempt {attempt + 1})\n") + time.sleep(wait) + continue + sys.stderr.write(e.read().decode()) + raise + + +def plain(rich_text): + return "".join(run["plain_text"] for run in rich_text) + + +def title_of(props): + for prop in props.values(): + if prop["type"] == "title": + return plain(prop["title"]) + raise KeyError("no title property") + + +def select_of(props, name): + value = props[name]["select"] + return value["name"] if value else None + + +def query(suite, extra_filters): + """Every card in the suite run that matches the extra filters.""" + filters = [{"property": "Test Suite Run", "select": {"equals": suite}}] + extra_filters + results, cursor = [], None + while True: + body = {"filter": {"and": filters}, "page_size": 100} + if cursor: + body["start_cursor"] = cursor + page = api("POST", f"databases/{DATABASE_ID}/query", body) + results += page["results"] + if not page["has_more"]: + return results + cursor = page["next_cursor"] + + +def find_card(test_case_id, suite): + cards = query(suite, [{"property": "Test Case ID", "number": {"equals": test_case_id}}]) + if len(cards) != 1: + raise SystemExit(f"expected 1 card with Test Case ID {test_case_id} in suite {suite}, found {len(cards)}") + return cards[0] + + +def summarize(card): + props = card["properties"] + return { + "testCaseId": props["Test Case ID"]["number"], + "pageId": card["id"], + "url": card["url"], + "title": title_of(props), + "summary": plain(props["Summary"]["rich_text"]), + "automation": select_of(props, "Automation"), + "automationNotes": plain(props["Automation Notes"]["rich_text"]), + "areas": [o["name"] for o in props["Areas"]["multi_select"]], + "priority": select_of(props, "Priority") if props["Priority"]["type"] == "select" else None, + } + + +def test_steps(page_id): + """The to_do blocks of the card body, in order, with any nested children flattened.""" + steps, cursor = [], None + while True: + path = f"blocks/{page_id}/children?page_size=100" + (f"&start_cursor={cursor}" if cursor else "") + page = api("GET", path) + for block in page["results"]: + kind = block["type"] + text = plain(block[kind].get("rich_text", [])) if isinstance(block[kind], dict) else "" + steps.append({"type": kind, "text": text, "checked": block[kind].get("checked") if kind == "to_do" else None}) + if block.get("has_children"): + steps += test_steps(block["id"]) + if not page["has_more"]: + return steps + cursor = page["next_cursor"] + + +def update(page_id, status=None, note=None): + props = {} + if status is not None: + if status not in STATUSES: + raise SystemExit(f"status must be one of {STATUSES}") + props["Automation"] = {"select": {"name": status}} + if note is not None: + props["Automation Notes"] = {"rich_text": [{"type": "text", "text": {"content": note[:2000]}}]} + return api("PATCH", f"pages/{page_id}", {"properties": props}) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("command", choices=["list-planned", "show", "claim", "set", "note", "brief"]) + parser.add_argument("args", nargs="*") + parser.add_argument("--suite", default=DEFAULT_SUITE) + parser.add_argument("--note") + parser.add_argument("--out") + parser.add_argument("--unattended", action="store_true") + a = parser.parse_args() + + if a.command == "list-planned": + cards = query(a.suite, [{"property": "Automation", "select": {"equals": "Planned"}}]) + rows = sorted((summarize(c) for c in cards), key=lambda r: r["testCaseId"]) + print(json.dumps(rows, indent=1)) + return + + if not a.args: + raise SystemExit(f"{a.command} needs a Test Case ID, for example: {a.command} 349") + try: + test_case_id = int(a.args[0]) + except ValueError: + raise SystemExit(f"the Test Case ID must be a number, not {a.args[0]!r}") + card = find_card(test_case_id, a.suite) + + if a.command == "show": + out = summarize(card) + out["testSteps"] = test_steps(card["id"]) + print(json.dumps(out, indent=1)) + elif a.command == "claim": + current = select_of(card["properties"], "Automation") + if current != "Planned": + print(json.dumps({"claimed": False, "automation": current})) + sys.exit(3) + update(card["id"], status="Building") + print(json.dumps({"claimed": True, "url": card["url"]})) + elif a.command == "set": + if len(a.args) < 2: + raise SystemExit(f"set needs a status; one of {STATUSES}") + update(card["id"], status=a.args[1], note=a.note) + print(json.dumps(summarize(find_card(test_case_id, a.suite)))) + elif a.command == "note": + if len(a.args) < 2: + raise SystemExit('note needs the text, for example: note 349 "..."') + update(card["id"], note=a.args[1]) + print(json.dumps(summarize(find_card(test_case_id, a.suite)))) + elif a.command == "brief": + if not a.out: + raise SystemExit("brief needs --out ") + info = summarize(card) + skill_dir = pathlib.Path(__file__).resolve().parent.as_posix() + template = (pathlib.Path(__file__).resolve().parent / "worker-brief.md").read_text(encoding="utf-8") + text = ( + template.replace("{{TEST_CASE_ID}}", str(info["testCaseId"])) + .replace("{{CARD_URL}}", info["url"]) + .replace("{{CARD_TITLE}}", info["title"]) + .replace("{{SKILL_DIR}}", skill_dir) + .replace("{{TODAY}}", datetime.date.today().isoformat()) + .replace("{{QUESTIONS_RULE}}", UNATTENDED_QUESTIONS_RULE if a.unattended else ATTENDED_QUESTIONS_RULE) + .replace("{{UNDECIDED_RULE}}", UNATTENDED_UNDECIDED_RULE if a.unattended else ATTENDED_UNDECIDED_RULE) + ) + if "{{" in text: + raise SystemExit("a placeholder in worker-brief.md was not filled: " + text[text.index("{{"):][:40]) + out = pathlib.Path(a.out) + out.parent.mkdir(parents=True, exist_ok=True) + out.write_text(text, encoding="utf-8") + print(json.dumps({"out": a.out, "testCaseId": info["testCaseId"], "title": info["title"]})) + + +if __name__ == "__main__": + main() diff --git a/.github/skills/improve-test-automation-coverage/worker-brief.md b/.github/skills/improve-test-automation-coverage/worker-brief.md new file mode 100644 index 000000000000..db6b3ab6c456 --- /dev/null +++ b/.github/skills/improve-test-automation-coverage/worker-brief.md @@ -0,0 +1,150 @@ +# Worker brief: automate Notion test case {{TEST_CASE_ID}} + +You are one worker in an Orca orchestration run started by the +`improve-test-automation-coverage` skill. Your one job: turn the manual test case below into an +edge-to-edge test in `src/BloomE2E/`, get it reviewed by the controller, and open a draft PR. + +- Notion card: {{CARD_URL}} +- Title: {{CARD_TITLE}} +- Test Case ID: {{TEST_CASE_ID}} +- Skill folder with the helper scripts (absolute path, in the controller's checkout): + `{{SKILL_DIR}}` +- Your worktree branch was created from `master`. PRs target `master`. `AGENTS.md` says new + work targets `Version6.5`; this task is the exception, because `src/BloomE2E` exists only on + master. Do not retarget the branch or the PR. + +The controller has already set the card's `Automation` property to `Building`. Do not set it +again. + +## Ground rules + +- **Follow `.github/skills/add-e2e-test/SKILL.md` in this worktree** as the authoritative + procedure. Read `src/BloomE2E/README.md`, `src/BloomE2E/AUTOMATION-DEBT.md`, and the existing + tests in `src/BloomE2E/tests/` before you write anything. +{{QUESTIONS_RULE}} +- **Never run the e2e suite directly.** Other worktrees run Bloom e2e tests on this machine at the + same time, and concurrent Bloom instances collide. Every Playwright run goes through the lock: + + ```powershell + Set-Location \src\BloomE2E + node {{SKILL_DIR}}/e2e-lock.mjs -- pnpm exec playwright test tests/.spec.ts + ``` + + The lock waits for the other run to finish, then runs yours. Do not kill a Bloom.exe you did + not start. +- Keep the Orca card current: `orca worktree set --worktree active --comment "" --json` + after each checkpoint (feasibility decided, test written, test green, review round, PR open). +- Follow the repo rules in `AGENTS.md`: never `pnpm build`; build C# through + `build/agent-dotnet.ps1`, with the one exception in step 0 (the wrapper builds no + `Bloom.exe`); never commit or push except inside the `preflight` skill in step 6. + +## Steps + +### 0. Finish the worktree setup yourself + +Orca's setup hook for this worktree has failed or will fail: it runs `./init.sh` under +`cmd.exe` with `NoDefaultCurrentDirectoryInExePath=1` inherited from Claude Code, which +prints `'.' is not recognized`. The repo is fine. If your branch is behind `origin/master`, +fast-forward it first. Then do the setup yourself, in this order: + +1. From the Bash tool, in this worktree, run `./init.sh`. It fetches the C# dependencies and + installs and builds the front-end. It does **not** touch the e2e package or the test inputs, + so steps 2 to 4 are yours as well. +2. `pnpm install` in `src/BloomE2E` (its own pnpm package), then confirm that + `src/BloomE2E/node_modules` exists. +3. `node build/get-testing-inputs.mjs`, then confirm that `output/testing-inputs` exists. +4. Build `Bloom.exe`, which the e2e suite launches from `output/Debug/`, with + `dotnet build src/BloomExe/BloomExe.csproj`. Use plain `dotnet` here, not the + `build/agent-dotnet.ps1` wrapper, because the wrapper builds no `Bloom.exe`. No Bloom runs + from this worktree, so nothing locks the output. + +### 1. Read the card + +```powershell +$env:BLOOM_TESTCASE_NOTION = [Environment]::GetEnvironmentVariable('BLOOM_TESTCASE_NOTION','User') +py {{SKILL_DIR}}/notion_automation.py show {{TEST_CASE_ID}} +``` + +The `testSteps` array is the card body, block by block: headings, bullets, and `to_do` items. +Together with `summary` it is the behavior contract. Some cards write every step as a bullet and +have no `to_do` items; treat those bullets as the steps. Read `automationNotes` too. + +### 2. Decide feasibility + +Decide whether the whole test, or a meaningful part, can be automated under the add-e2e-test +rules: no native OS dialogs, no WinForms surfaces without an `E2eTestingApi` hook, real UI for +the behavior under test, state-based waits. Look for the UI in the React source and confirm +selectors or the `data-testid` you will add. If a small Bloom change (a `data-testid`, an +`E2eTestingApi` hook) makes it feasible, that change is part of your PR. + +{{UNDECIDED_RULE}} + +If the case is **not feasible as written**, or the developer tells you the card is not ready, flag it for +the developer who wrote the card and stop. The note must tell that developer what to change in +the card, or what Bloom lacks, and list every question you would have asked: + +```powershell +py {{SKILL_DIR}}/notion_automation.py set {{TEST_CASE_ID}} "Has automation problems" --note "[improve-test-automation-coverage {{TODAY}}] " +orca orchestration send --type worker_done --subject "HAS AUTOMATION PROBLEMS: " --body "" --task-id --dispatch-id --outcome succeeded --json +``` + +Use the task id and dispatch id from the dispatch preamble at the top of your prompt. + +### 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. + +### 4. Run it through the lock, three times + +Run the file three times in a row through `e2e-lock.mjs`. Investigate any failure; a test that +passes 2 of 3 is not done. Confirm no `Bloom.exe` you started survives the run. + +Also run `pnpm typecheck` in `src/BloomE2E`. + +### 5. Ask the controller for review + +Send a blocking question to the coordinator and wait for the answer: + +```powershell +orca orchestration ask --question "READY FOR REVIEW {{TEST_CASE_ID}}: ; ; <3 lock runs green>" --options "ship,fixes" --timeout-ms 3600000 --json +``` + +The controller reads your worktree, then replies either `ship` or a list of fixes. Apply the +fixes, re-run step 4, and ask again. Repeat until the answer is `ship`. If the ask times out, +resume it with `orca orchestration ask --resume --timeout-ms 3600000 --json`; do not +proceed without a `ship`. + +### 6. Ship + +1. Run the `preflight` skill (`Skill` tool, name `preflight`). It commits, pushes, opens a draft + PR against `master`, and waits for the bots. Preflight looks for a `BL-` ticket in the branch + name; there is none, which is a normal outcome for this work. When it writes the PR + description, make sure the body contains this line, so the PR points at the Notion card: + + ``` + Automates Notion test case {{TEST_CASE_ID}} ({{CARD_TITLE}}): {{CARD_URL}} + ``` + + Also say which Test Steps the test covers and which it does not. +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: + + ```powershell + py {{SKILL_DIR}}/notion_automation.py set {{TEST_CASE_ID}} "PR Pending" --note "[improve-test-automation-coverage {{TODAY}}] PR: . Covers steps: . Not covered: ." + ``` + +3. Move the Orca card to Personal Review, never Peer Review: + `orca worktree set --worktree active --workspace-status status-7 --json` +4. Report once and stop: + + ```powershell + orca orchestration send --type worker_done --subject "PR open: " --body "" --task-id --dispatch-id --outcome succeeded --files-modified "" --json + ``` + +If the environment blocks you for good (build broken, Bloom will not launch, preflight cannot +open a PR), the card itself is fine: set it back to `Planned` with a +`[improve-test-automation-coverage {{TODAY}}] Blocked:` note, then send `worker_done` with +`--outcome failed` and the reason. Use `Has automation problems` only when the card or Bloom is +what stands in the way. From 5404f3a7137bbbdb19e94b1f24b1a00a1a881a4b Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 1 Sep 2026 17:59:56 -0600 Subject: [PATCH 4/8] Restore the current add-e2e-test skill text under the new Automation states The previous commit rebuilt this file from an older base and dropped the "every step is a helper call" revision. This puts that revision back and keeps only the two additions: the PR Pending and Has automation problems states. Co-Authored-By: Claude Fable 5.1 --- .github/skills/add-e2e-test/SKILL.md | 116 +++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 15 deletions(-) diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 930980785476..6218f9f1a0c0 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -147,21 +147,6 @@ The worker-scoped fixture launches Bloom on a temp copy of that collection and y stops Bloom, runs the callback, and starts it again, which is how a test changes what Bloom reads only at startup, such as the collection's languages. -Helper modules: - -- `helpers/workspace.ts` — `switchTab`, `getTabs`, `waitForActiveTab`. Bloom hides the - Edit and Publish tabs until a book is selected. -- `helpers/collection.ts` — `selectBook`, `waitForCollectionReady`. -- `helpers/bookMaking.ts` — `makeBookFromTemplate`, `addPage`, `findBookFolder`, - `setContentLanguages`, `getPages`, `getContentPages`, `goToPage`, `typeInGroup`. A book - made from a template starts with front and back matter only, so a test that needs - content calls `addPage`. Bloom writes a page only when the book leaves it, so `goToPage` - is also how a test saves what it typed. -- `helpers/api.ts` — `apiGet`, `apiPost`, `apiGetJson`, which run `fetch` inside the page - with a relative URL (Bloom's server rejects a `127.0.0.1` Host header; the CDP endpoint - does not answer on `localhost`). -- `helpers/realClick.ts` — `realClick`, `realClickAt`. - The fixture also watches for the "Bloom had a problem" dialog and fails the test with the exception it scrapes from behind the dialog's own "Learn More" link. See `src/BloomE2E/README.md` for the whole story. @@ -178,6 +163,101 @@ Rules that hold regardless of the final API: a "Bloom had a problem" dialog appears; do not loop-dismiss it. - Waits are event/state-based (poll an API, await a selector), never fixed sleeps. +## Every step is a helper call + +This suite will grow to a few thousand tests. It stays maintainable only if the knowledge +of HOW to do each thing in Bloom lives in exactly one place. A test says WHAT happens; the +helper layer in `src/BloomE2E/helpers/` says how. + +**A test spells out nothing that is not the behavior it measures.** If a test has to crop +an image on the way to what it checks, it calls `cropImage(...)`. It does not find the +crop handle, work out the drag, and wait for the result. When Bloom changes how an image is +cropped, one helper changes, and every test that crops an image keeps working. The same +holds for the step that IS under test: the journey test for cropping also calls the helper +that drives the UI, so the click path is written once. + +### When you write a test + +1. **Look for the helper before you write a step.** There is one module per Bloom surface + (see the list below). Read the header comment of the module for the surface you are on, + then grep the folder for the concept, e.g. `grep -ri crop src/BloomE2E/helpers`. +2. **If a helper nearly fits, extend it.** Add a parameter or an option. Do not copy the + helper into your test and change one line. +3. **If no helper exists, write one in `helpers/`, in the same PR.** Put it in the module + for its surface, or start a module when the surface is new. Do this even when yours is + the only test that needs it today. The second test comes soon, and the person who writes + it will copy from yours. +4. **Selectors, `data-testid`s, API paths, and Bloom's quirks live only in helpers.** A test + contains no CSS selector, no `getByTestId`, no `apiGet`/`apiPost`, and no retry loop. To + read Bloom's state for an assertion, call a named reading helper such as + `getLanguagesInBook`, so the API path is in one place. The one exception is the class of + a translation group, such as `.bookTitle`, passed to `typeInGroup`: that names book + content, not Bloom's UI. +5. **A sequence that appears twice is a helper.** Two tests, or two places in one test, + that do the same three lines get one function. This includes sequences that only build + a start state, such as "a book with two content pages in two languages". A helper that + composes other helpers into a start state belongs in `helpers/` as well. +6. **A repeated expected value gets a name.** When the same literal block of expected state + appears more than twice in a file, make it a constant or a small builder function at the + top of the file. +7. **A file-local function is fine only for file-local knowledge**, such as the shape of + the one book this file builds. The moment it encodes how to drive Bloom, it moves to + `helpers/`. + +### What a helper must do + +Thousands of tests will lean on each helper, so each one meets this bar: + +- **Named for the user's intent, in the words the UI uses.** `openPublishDestination`, + `cropImage`, `setContentLanguages`. Not `clickThirdTab` or `postLanguageChange`. +- **Takes `page` first, then what varies.** Nothing test-specific inside: no book titles, + no language lists, no page counts baked in. +- **Waits for its own result before it returns**, by polling Bloom's state. `goToPage` + returns when the page is showing; `addPage` returns when Bloom reports the new page + count. A caller never adds a wait after a helper call. +- **Fails with a message that names what Bloom offered instead.** When the template page + asked for does not exist, the error lists the pages that do. The existing helpers show + the pattern. +- **Carries a doc comment** that says which user action it stands for, and names any quirk + it absorbs, with the `AUTOMATION-DEBT.md` entry if there is one. +- **Puts both routes for one action side by side.** The API route for setup (`selectBook`) + and the UI route the journey test drives (a real click on the book tile) sit in the same + module. A UI change is then fixed in one file. +- **Never sleeps for a fixed time.** + +### Layers + +Dependencies point down only: + +1. **Primitives**: `helpers/api.ts` (`apiGet`, `apiPost`, `apiGetJson`, which run `fetch` + inside the page with a relative URL, because Bloom's server rejects a `127.0.0.1` Host + header and the CDP endpoint does not answer on `localhost`) and `helpers/realClick.ts` + (`realClick`, `realClickAt`). Tests do not import these; surface modules wrap them. +2. **One module per Bloom surface**, named after the surface: + - `helpers/workspace.ts` — `switchTab`, `getTabs`, `waitForActiveTab`. Bloom hides the + Edit and Publish tabs until a book is selected. + - `helpers/collection.ts` — `selectBook`, `waitForCollectionReady`. + - `helpers/bookMaking.ts` — `makeBookFromTemplate`, `addPage`, `duplicateCurrentPage`, + `findBookFolder`, `setContentLanguages`, `getPages`, `getContentPages`, `goToPage`, + `typeInGroup`, `waitForEditablePage`, `editablePageFrame`. A book made from a template + starts with front and back matter only, so a test that needs content calls `addPage`. + Bloom writes a page only when the book leaves it, so `goToPage` is also how a test + saves what it typed. + - `helpers/publish.ts` — `openPublishDestination`, `getTextLanguageRows`, + `expectTextLanguageRows`, `clickTextLanguage`, `showBloomPubPreview`, + `getPreviewLanguages`, `getLanguagesInBook`, `getTooltipForLanguage`. + This list is a map, not the index. The folder is the index: new modules appear there + before anyone updates this file. +3. **Tests**, which call surface helpers and nothing lower. + +### When you change a helper, or the Bloom UI a helper drives + +- Grep `src/BloomE2E/tests/` for the helper's name and run every test that calls it before + you open the PR. +- When you change a `data-testid`, an API endpoint, or a UI flow that a helper drives, grep + `src/BloomE2E/helpers/` for the old id or path and update the helper in the same PR. That + is the payoff of the whole rule: one change, in one file, and the suite follows. + ## Running From `src/BloomE2E/` (its own pnpm package — run `pnpm install` there once): @@ -200,6 +280,12 @@ the visual-regression job already does. ## Checklist before you call it done - [ ] Journey coverage exists for the UI path (yours or a pre-existing journey test). +- [ ] The test contains no selector, test id, API path, or retry loop. Every step is a + helper call. +- [ ] Every new helper is in `src/BloomE2E/helpers/`, has a doc comment, and waits for its + own result. +- [ ] No sequence appears twice; no expected-value literal appears more than twice. +- [ ] If you changed a helper, every test that calls it still passes. - [ ] The test passes locally, launched from a clean state (no Bloom running). - [ ] No Bloom.exe process survives the run. - [ ] New inputs: PR merged in bloom-testing-inputs, pin advanced here, both green. From 49ce121b0277ca3cdbcc59fc4fa891b8b92062ae Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 13:14:27 -0600 Subject: [PATCH 5/8] Split a card instead of marking it Partial when a test covers only part of it 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. This happened with Test Case ID 349, Duplicate Page, whose audio, video, and Duplicate Many Times steps stayed manual behind a note. The add-e2e-test skill now says to split such a card when it goes to PR Pending: the original becomes the [Automated portion] and keeps its id, the uncovered steps move to a new [Manual portion] row with a new id, and the two rows point at each other through the new Related Cases relation property. Partial is retired. The improve-test-automation-coverage skill and the BloomE2E README follow the same rule. Co-Authored-By: Claude Fable 5.1 --- .github/skills/add-e2e-test/SKILL.md | 32 ++++++++++++++----- .../improve-test-automation-coverage/SKILL.md | 31 ++++++++++++++++-- src/BloomE2E/README.md | 9 +++--- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 6218f9f1a0c0..f3f68689d367 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -54,27 +54,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/src/BloomE2E/README.md b/src/BloomE2E/README.md index 7d9789b61559..bcf55edd36a9 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -111,10 +111,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 From 94ab0db0d63a690149b5946a21bd3f3201b8865c Mon Sep 17 00:00:00 2001 From: Hatton <hattonjohn@gmail.com> Date: Wed, 2 Sep 2026 13:40:12 -0600 Subject: [PATCH 6/8] Replace the stale Partial note with the split-or-fail rule for a worker The `Partial` Automation state is retired: a test that covers only part of a card's steps splits the card instead. This step-3 instruction still told the worker to remember which part for a `Partial` note, which no longer exists. It now says what to do instead. A step the worker cannot implement is a problem, not something to record and move past. If a substantial portion is automatable and the steps split cleanly, split the Notion card into a manual and an automated portion. If they do not, stop and set the card's Automation property to "Has automation problems". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- .../skills/improve-test-automation-coverage/worker-brief.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/skills/improve-test-automation-coverage/worker-brief.md b/.github/skills/improve-test-automation-coverage/worker-brief.md index 111d812295ec..0e051e802744 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 From a9c6110ece3791bd94eb347338c91cc2e3edc294 Mon Sep 17 00:00:00 2001 From: Hatton <hattonjohn@gmail.com> Date: Wed, 2 Sep 2026 15:01:34 -0600 Subject: [PATCH 7/8] Ask Bloom, not only the DOM, whether the Edit tab has finished loading a page `waitForEditablePage` in pageThumbnails.ts decided the Edit tab was ready when the page iframe's document reached `readyState === "complete"`. The document can get there a moment before Bloom does, and in that window Bloom's editing model is still Navigating, where it silently ignores any command that begins by saving the page. Copy Page is one of those, so a test could click it, get no error, and find an empty clipboard. It now also polls `e2e/isEditingPage`, the hook master added for exactly this, the way the `bookMaking.ts` wait of the same name does. Also fix step 6 of the improve-test-automation-coverage worker brief. It still told the worker to record the uncovered steps in `Automation Notes` and stop, which is the retired `Partial` workflow. It now says to split the card into an automated and a manual portion first, as `add-e2e-test` requires, and to keep the note as the record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- .../improve-test-automation-coverage/worker-brief.md | 7 +++++-- src/BloomE2E/helpers/pageThumbnails.ts | 12 ++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/skills/improve-test-automation-coverage/worker-brief.md b/.github/skills/improve-test-automation-coverage/worker-brief.md index 0e051e802744..89171092cc4d 100644 --- a/.github/skills/improve-test-automation-coverage/worker-brief.md +++ b/.github/skills/improve-test-automation-coverage/worker-brief.md @@ -130,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/src/BloomE2E/helpers/pageThumbnails.ts b/src/BloomE2E/helpers/pageThumbnails.ts index 37889fbbc176..adabd90e8195 100644 --- a/src/BloomE2E/helpers/pageThumbnails.ts +++ b/src/BloomE2E/helpers/pageThumbnails.ts @@ -112,6 +112,9 @@ export async function selectPage( * 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, @@ -139,6 +142,15 @@ export async function waitForEditablePage( }, ) .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 From 688e42e39154ffc02cade9eda2426841045016eb Mon Sep 17 00:00:00 2001 From: Hatton <hattonjohn@gmail.com> Date: Wed, 2 Sep 2026 15:18:41 -0600 Subject: [PATCH 8/8] Say the copy-page card splits, not that it stays Partial The debt entry for cross-instance page copy still named the retired `Partial` status. The rule now is that a card whose test covers only part of its steps is split, so this says the cross-instance step belongs on a manual portion row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- src/BloomE2E/AUTOMATION-DEBT.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index c775ffb0b867..204e3978a070 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -221,7 +221,8 @@ one running Bloom into a second one. Bloom's page clipboard is a pair of fields 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, and the Notion case stays Partial. Fix direction: decide whether cross-instance +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.)