From 6d055073444722cebe6102771fcd36a3ca74b7e3 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 3 Sep 2026 11:10:10 -0600 Subject: [PATCH] Report every failed image comparison in a visual-regression case, not the first Each case in src/BloomVisualRegressionTests/index.spec.ts threw on its first mismatch, so the comparisons after it never captured their images. A stale baseline therefore surfaced one image per run, and each run takes about three minutes: BL-16638 needed three accept-and-rerun rounds to get through one layout change. comparePreviewImage now records a mismatch in a per-case list instead of throwing, and the case body throws once at the end, naming every image that did not match. Anything thrown while comparing, such as Pixelmatch's "Image sizes do not match", is recorded the same way, so it is still a failure and it no longer hides the comparisons behind it. The list resets at the start of each case. Retires the AUTOMATION-DEBT.md entry "Visual-regression cases stop at the first failed comparison". Verified by type check only. This suite's baselines match the CI runner rather than a developer machine, so it cannot go green here. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomE2E/AUTOMATION-DEBT.md | 12 ------ src/BloomVisualRegressionTests/index.spec.ts | 45 +++++++++++++++++++- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index 21f533106693..3d0bd9f2af0d 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -21,7 +21,6 @@ the identity here. Before you start on a marked entry, ask the owner of its bran | Branch | What it pays down | | --- | --- | -| `BL-16799-vr-collect-failures` | A visual-regression case collects every failed comparison and fails once at the end. | | `BL-16799-component-tests-in-ci` | The component-tester Playwright suites get a nightly job. | | `BL-16799-vite-port` | `BLOOM_E2E_VITE_PORT` makes a run test the working tree's front end. Adds a new entry for what remains. | | `BL-16799-type-in-one-call` | Typing in a text box is one insertion, not one key press per character. Adds a new entry: typing now raises no key events. | @@ -112,17 +111,6 @@ refuses to upload at all rather than let an automated click publish under the de account. (Found 2026-09-02 automating Test Case ID 606, `upload-required-items.spec.ts`.) -## Visual-regression cases stop at the first failed comparison - -Each case in `src/BloomVisualRegressionTests/index.spec.ts` throws on the first -mismatch, so later comparisons never capture their images; stale baselines surface one -layer per ~3-minute run (BL-16638 took three accept-and-rerun rounds). Fix direction: -accumulate per-comparison failures and fail once at the end — proven during BL-16638 -(~20–30 lines, confined to the spec). Loop at `index.spec.ts:426`, assertion at -`index.spec.ts:486` (pre-rewire line numbers). (Promoted from PAPERCUTS 2026-07-30.) - -being fixed on `BL-16799-vr-collect-failures`. - ## The top bar has no stable test ids, so tests match on localized text `TopBar.tsx` renders the workspace tabs as `` with a localized `` diff --git a/src/BloomVisualRegressionTests/index.spec.ts b/src/BloomVisualRegressionTests/index.spec.ts index b7a260b9bdda..dfe130d1030c 100644 --- a/src/BloomVisualRegressionTests/index.spec.ts +++ b/src/BloomVisualRegressionTests/index.spec.ts @@ -208,6 +208,12 @@ describe("All books", () => { let playerPage: Page; let browser: Browser; let context: BrowserContext; + // Every image comparison the CURRENT case has failed. A case compares one book-preview image + // and one image per bloom-player page, and a stale baseline usually affects several of them. + // Throwing on the first mismatch meant the later comparisons never even captured their + // images, so accepting a real layout change took one ~3-minute run per image (BL-16638 took + // three rounds). So collect them all and fail once, at the end of the case. + let comparisonFailures: string[] = []; beforeAll(async () => { await launchDedicatedBloom(); @@ -297,6 +303,7 @@ describe("All books", () => { }); test.each(cases)("$title", async (testCase) => { + comparisonFailures = []; // Park the capture pages before we mutate this book. Otherwise the previous case's still-open // book-preview / bloom-player page keeps requesting book and staged-BloomPUB files while this // case rewrites them, which caused mid-run "file is being used by another process" and @@ -334,6 +341,14 @@ describe("All books", () => { await selectTab("publish"); const stagedUrl = await makeBloomPubPreview(); await capturePlayerPages(stagedUrl, testCase.label, screenshotsDir); + + // One failure for the whole case, listing every image that did not match, so a single run + // shows all the baselines that need looking at. + if (comparisonFailures.length > 0) + throw new Error( + `${comparisonFailures.length} of this case's images did not match their ` + + `reference:\n ${comparisonFailures.join("\n ")}`, + ); }); // Create the reference image if it does not exist yet; otherwise capture a current image and @@ -694,6 +709,11 @@ describe("All books", () => { } } + // Compare one captured image against its reference. This does NOT throw on a mismatch: it + // appends a description to comparisonFailures, and the test body fails the case once it has + // compared every image. Anything thrown while comparing (notably Pixelmatch's "Image sizes do + // not match", which is itself a real failure) is recorded the same way. + // // `likelyCause`, when given, is an explanation to attach to a mismatch (see andikaIsInstalled): // something we know about this machine that makes the difference expected rather than a regression. async function comparePreviewImage( @@ -701,6 +721,27 @@ describe("All books", () => { testPath: string, diffPath: string, likelyCause?: string, + ) { + try { + await compareOrThrow( + referencePath, + testPath, + diffPath, + likelyCause, + ); + } catch (error) { + comparisonFailures.push( + `${testPath}: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + + // The comparison itself: write a diff image and throw when the two images differ at all. + async function compareOrThrow( + referencePath: string, + testPath: string, + diffPath: string, + likelyCause?: string, ) { const referenceImage = PNG.sync.read(fs.readFileSync(referencePath)); const testImage = PNG.sync.read(fs.readFileSync(testPath)); @@ -732,9 +773,9 @@ describe("All books", () => { ); // A thrown Error rather than expect(...).toBe(0), so the failure itself carries the // diff path and, when we know one, the likely cause; the console lines above are lost in - // a long run's output. + // a long run's output. comparePreviewImage catches this and records it. throw new Error( - `${testPath} differed from the reference by ${numberOfDifferentPixels} pixels. ` + + `${testPath} differed from ${referencePath} by ${numberOfDifferentPixels} pixels. ` + `The diff image is at ${diffPath}.` + (likelyCause ? `\n\n${likelyCause}` : ""), );