From e1f647dabce6bf28746896dc90fddded1cdeb028 Mon Sep 17 00:00:00 2001 From: Timur Mashara <62305878+tmashara@users.noreply.github.com> Date: Thu, 22 Oct 2026 15:30:32 +0200 Subject: [PATCH 1/2] Add accepted differences and --rerun-failed --accept "" records each band, region and size difference of the run in renderdiff.accepted.json (next to the config, or in cwd), scoped by the run's routes and widths. Later runs report a shot whose every change matches as accepted: its own summary line, no threshold failure, a results.json flag and a report badge. Stale entries are listed and pruned by the next --accept over them. --rerun-failed keeps the out dir, reruns only shots that weren't clean or accepted, and merges them back in route and width order; a run that doesn't repeat the previous one exits 2. Co-Authored-By: Claude Opus 5.5 --- README.md | 58 ++++++++- skills/renderdiff/SKILL.md | 6 + skills/renderdiff/reference.md | 13 +- src/accept.ts | 123 ++++++++++++++++++ src/cli.ts | 25 +++- src/compare.ts | 39 ++++++ src/index.ts | 1 + src/report.ts | 39 +++++- src/run.ts | 210 +++++++++++++++++++++++++------ tests/helpers/cli.ts | 4 +- tests/helpers/parse.ts | 59 +++++++-- tests/unit/cli-args.test.ts | 14 +++ tests/visual/accepted.test.ts | 224 +++++++++++++++++++++++++++++++++ tests/visual/controls.test.ts | 2 +- 14 files changed, 762 insertions(+), 55 deletions(-) create mode 100644 src/accept.ts create mode 100644 tests/visual/accepted.test.ts diff --git a/README.md b/README.md index 4f659bf..14bbf55 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,8 @@ renderdiff [options] | `--left-selector `, `--right-selector ` | That side's selector, over `--selector`. One without the other side (or `--selector`) is an error. | | `--storage-state ` | Start both sides with this Playwright storage state (cookies, localStorage), e.g. signed in. Relative to cwd. Over the config's `storageState`. | | `--left-storage-state `, `--right-storage-state ` | That side's storage state, over `--storage-state`. One side alone is fine. | +| `--accept ` | Record this run's differences as accepted, with a reason. See **Accepted differences and reruns**. | +| `--rerun-failed` | Rerun only what the last run in `--out` didn't find clean or accepted, keeping the rest. | | `--dry-run` | Print the URL pairs and exit. No browser. | | `--open` | Open the report when the run finishes. | | `--progress` | A line per shot as it finishes, then the worst scores. The default on a terminal; piped, renderdiff prints one compact summary at the end. | @@ -253,6 +255,58 @@ See [`renderdiff.config.example.ts`](./renderdiff.config.example.ts) for every f with `satisfies RenderdiffConfig` is for editor types only; Node erases it, so the file loads without renderdiff installed in that project. +## Accepted differences and reruns + +A migration starts with differences that already exist between the untouched local build +and production. Record them once, then change code and re-check only what's red: + +``` +renderdiff http://localhost:3000 https://example.com / /about --accept "differs before the migration" +# …change code… +renderdiff http://localhost:3000 https://example.com / /about --rerun-failed +``` + +`--accept ""` runs as usual, then writes every difference it saw that isn't accepted +yet to `renderdiff.accepted.json`, next to the config file (or in cwd without one), each with +the reason. Scope it the way you scope any run: pass only the routes and `--width`s to accept. +Commit the file; it is one entry per line, so its diff reads as what was accepted. + +A shot whose every difference matches an accepted one is `accepted`: it is listed on an +`accepted:` line like `clean:`, doesn't count as differing or toward `--threshold`, and has +`"accepted": true` in `results.json` and a badge in the report. Anything else on the page +still shows: a row with some changes accepted says `1 accepted`, and its `dom` line skips +them. A shot with unstable pixels is never accepted, since part of it went unmeasured. +Accepted entries for a route and width the run measured but didn't see are **stale**: they +print on a `stale accepted:` line, sit in `summary.stale` and the report, and the next +`--accept` over that route and width prunes them. + +Each entry is one change: a band, a region, or the page's size difference, keyed by route +(as the summary names it) and width, with a fingerprint. Two changes match when they have +the same kind and side, the same `x`, width and height (regions sit on the 16px grid, bands +are exact rows), the same `dom` elements with the same changed values (when both runs named +them), and each side's pixels look alike: a 4×4 grid of mean colours that may drift one step +in sixteen per channel. That makes a match: + +- robust to sub-threshold noise (pixelmatch's tolerance, the grid and the colour step), to a + change above it moving it down the page (`y` is left out), and to unrelated changes + elsewhere (they are their own changes, and stay unaccepted); +- strict about the spot itself: a different colour or value there, a change touching it (the + region grows), or a second identical change (an entry matches once) all surface. + +The cost is false alarms, never hidden changes, except one: a change inside an accepted +region too small to move its mean colours and touching no element `dom` names. A new change +that makes repeated identical rows align differently can also turn an accepted region into +bands, so it shows again; accept it again once you've looked. + +`--rerun-failed` reads `results.json` in `--out`, keeps every shot that was clean or is +accepted (against the accepted file as it is now), reruns the rest — differences, unstable +shots, errors — and merges them back into `results.json` and `report.html` in route and width +order. The out dir is kept rather than wiped; only a rerun shot's own old files are replaced. +A `rerun:` line says how many shots it reran on how many routes, and how many it reused. It +must repeat the previous run: different origins, routes, widths, masks, hidden selectors, +block patterns, storage state or renderdiff version exit 2, as does a missing +`results.json`. + ## How it reads The score is the share of pixels that match, per route per width. Green above 99.5%, @@ -377,7 +431,9 @@ Everything lands in the out dir (`./renderdiff-out` by default): "summary": { "compared": 1, "errors": 1, "mean": 0.9714, "worst": { "url": "/about", "width": 1440, "score": 0.9714 }, - "belowThreshold": 0 + "belowThreshold": 0, // accepted shots aside + "accepted": 0, // shots whose every difference was accepted + "stale": [] // accepted entries the run measured but didn't see } } ``` diff --git a/skills/renderdiff/SKILL.md b/skills/renderdiff/SKILL.md index f7934f3..88324bc 100644 --- a/skills/renderdiff/SKILL.md +++ b/skills/renderdiff/SKILL.md @@ -53,5 +53,11 @@ Exit: 0 nothing below `--threshold` and no errors — without `--threshold` a di so read the summary · 1 below `--threshold` · 2 bad args/config/origin down · 3 a route errored or the run crashed. `ERROR` = unmeasured, report it as unverified. Everything clean? Confirm both sides actually rendered. +**Migration loop.** Record what already differs on the untouched build, with a reason: +`--accept "differs before the migration"`. After each change, `--rerun-failed` (same +arguments) reruns only what isn't clean or accepted. `accepted:` routes are known; a row that +says `1 accepted` still has something new. Accept only what you looked at, never to get green. +Details: reference.md. + **Before you mask, lower the threshold, drop a route or skip widths,** read the Rationalizations table in [reference.md](reference.md). diff --git a/skills/renderdiff/reference.md b/skills/renderdiff/reference.md index be690ab..2a51669 100644 --- a/skills/renderdiff/reference.md +++ b/skills/renderdiff/reference.md @@ -21,6 +21,9 @@ | `unstable Npx` | Pixels that kept changing between shots of the same load on one origin (a ticker, a carousel, a video). Painted out on both sides and excluded from the score. A large number means a big area went unmeasured: look at the magenta on the full shots. Such a route is never on the `clean:` line. | | `ERROR selector "…" matched N elements on left (/a), expected 1` | A selector must match exactly one element per side. Make it more specific (`>> nth=0`, an id, `data-test`), or list the route once per element. | | `ERROR selector "…" has no box on …` | It matched an element that isn't rendered (`display: none`, zero size). | +| `accepted: /a /b` | Every difference on those routes matches `renderdiff.accepted.json`. Known, not verified again: say so in the verdict. | +| `N accepted` on a row | Some of that shot's changes are accepted; the rest are new. `dom` names only the new ones. | +| `stale accepted: /a@1440` | An accepted difference wasn't seen. It was fixed, or the page changed under it; `--accept` over that route and width prunes it. | | `ERROR page did not settle: Npx unstable at …` | More than 25% of the page kept changing between shots, so too little was measured to score. Mask or stop the moving part. | Report colouring (`--open`, for a human): green above 99.5%, amber to 97%, red below. @@ -35,6 +38,7 @@ Report colouring (`--open`, for a human): green above 99.5%, amber to 97%, red b | "`dom` says margin, done." | It's a computed value. Confirm the source rule changed on purpose, and still open the crop when the region is large or `dom` names nothing. | | "The crops looked fine, done." | There are at most three: bands, then the largest regions. Check the region count, any `clipped` crop in results.json, and open the diff PNG for the rest and for any height delta: padding is never cropped. | | "Masking the header gets this to 100%." | Every mask is coverage surrendered. Mask only content *proven* to vary per load (clocks, star counts, rotating quotes), with a `reason`, which the report prints verbatim. | +| "I'll `--accept` it so the run is green." | Accept only differences you opened and that exist before your change (or are intended), with a reason that says which. | | "I'll lower `--threshold` until it passes." | Threshold is a CI gate, not a dial for reaching green. | | "That route errors, I'll drop it from the list." | Shrinking the route list shrinks what you verified. Fix it, or say it's unverified. | | "One width is enough." | Migrations break at one width and not others. That's why there are five defaults. | @@ -88,7 +92,14 @@ cookie script ran wedges the renderer). - **stdout is compact when piped** (what an agent sees). `--progress` prints a line per shot instead; `--no-progress` forces the summary on a terminal. -- **The out dir is wiped at the start of every run.** To keep a previous run's evidence +- **Accepted differences** (`--accept ""`) go to `renderdiff.accepted.json` next to the + config (or in cwd), one entry per band, region or size difference, scoped by the run's routes + and widths. A match ignores `y` and tolerates noise, but a different value, a grown region or + an extra copy in the same spot still surfaces. A new change can make an accepted one re-read + as different (repeated rows aligning differently): look, then accept again. +- **`--rerun-failed`** keeps the out dir and reruns only what wasn't clean or accepted. Give it + the same origins, routes, widths and config as the run it continues, or it exits 2. +- **The out dir is wiped at the start of every run** (except `--rerun-failed`). To keep a previous run's evidence next to a new one, pass `--out renderdiff-before` / `--out renderdiff-after`. A directory holding files renderdiff didn't write is refused (exit 2). - **Routes normalize** to no trailing slash; `/` stays `/`. Globs are rejected. diff --git a/src/accept.ts b/src/accept.ts new file mode 100644 index 0000000..0f75c52 --- /dev/null +++ b/src/accept.ts @@ -0,0 +1,123 @@ +import fs from "node:fs"; +import type { Band, Region } from "./compare.ts"; +import { name } from "./config.ts"; +import { fmtElements } from "./format.ts"; +import type { ComparedResult, RouteResult } from "./report.ts"; + +export const ACCEPTED_FILE = "renderdiff.accepted.json"; + +/** + * One change, without its `y`: a change above it moves it down the page without changing it. + * `elements` and `thumb` tell a new change in the same spot from the accepted one. + */ +export interface Fingerprint { + kind: "band" | "region" | "size"; + side?: "left" | "right"; + /** A region's; a band's width is in its thumb, and `size` holds the width and height deltas. */ + x?: number; + w: number; + h: number; + /** The `dom` of each element under it, sorted; absent when they weren't looked at. */ + elements?: string[]; + thumb?: string; +} + +/** A difference recorded as already there or intended, so later runs report it as accepted. */ +export interface Accepted { + /** The route as the summary names it: `/a`, `/a::/b`, `/a [.sel]`. */ + route: string; + width: number; + reason: string; + fingerprint: Fingerprint; +} + +type Change = { fp: Fingerprint; target: Region | Band | null }; + +/** Each band and region, plus the size difference, which padding hides from both. */ +function changes(r: ComparedResult): Change[] { + const els = (g: Region | Band) => + g.elements && { elements: g.elements.map((e) => fmtElements([e], Infinity)).sort() }; + return [ + ...(r.bands ?? []).map((b) => ({ + fp: { kind: "band" as const, side: b.side, w: 0, h: b.h, ...els(b), thumb: b.thumb }, + target: b, + })), + ...r.regions.map((g) => ({ + fp: { kind: "region" as const, x: g.x, w: g.w, h: g.h, ...els(g), thumb: g.thumb }, + target: g, + })), + ...(r.heightDelta || r.widthDelta + ? [{ fp: { kind: "size" as const, w: r.widthDelta, h: r.heightDelta }, target: null }] + : []), + ]; +} + +/** A cell may drift by one step a channel: noise, not a change. */ +const near = (a = "", b = "") => + a.length === b.length && + [...a].every((c, i) => Math.abs(parseInt(c, 16) - parseInt(b[i]!, 16)) <= 1); + +const same = (a: Fingerprint, b: Fingerprint) => + a.kind === b.kind && + a.side === b.side && + a.x === b.x && + a.w === b.w && + a.h === b.h && + (!a.elements || !b.elements || a.elements.join("\n") === b.elements.join("\n")) && + near(a.thumb, b.thumb); + +export function readAccepted(file: string): Accepted[] { + if (!fs.existsSync(file)) return []; + let list: unknown; + try { + list = JSON.parse(fs.readFileSync(file, "utf8")); + } catch (e) { + throw new Error(`${file} is not valid JSON: ${e instanceof Error ? e.message : e}`); + } + const ok = (a: Record) => + typeof a.route === "string" && + typeof a.width === "number" && + typeof a.reason === "string" && + a.reason.trim() && + typeof a.fingerprint === "object" && + a.fingerprint !== null; + if (!Array.isArray(list) || !list.every(ok)) { + throw new Error(`${file} must be an array of { route, width, reason, fingerprint }`); + } + return list as Accepted[]; +} + +/** + * Mark each change an accepted entry matches, one entry per change, and a result `accepted` + * when every change matched and nothing went unmeasured. Returns the changes nothing matched, + * and the entries for a measured route and width that matched nothing. + */ +export function applyAccepted(results: RouteResult[], list: Accepted[]) { + const used = new Set(); + const unmatched: Omit[] = []; + for (const r of results) { + if (r.status !== "compared") continue; + const mine = list.filter((a) => a.route === name(r) && a.width === r.width); + const cs = r.diffPixels > 0 ? changes(r) : []; + let missed = 0; + for (const { fp, target } of cs) { + const hit = mine.find((a) => !used.has(a) && same(a.fingerprint, fp)); + if (hit) used.add(hit); + else missed = unmatched.push({ route: name(r), width: r.width, fingerprint: fp }); + if (target && hit) target.accepted = hit.reason; + else if (target) delete target.accepted; + } + r.accepted = cs.length > 0 && !missed && r.unstablePixels === 0; + } + const measured = new Set( + results.filter((r) => r.status === "compared").map((r) => `${name(r)}@${r.width}`), + ); + const stale = list.filter((a) => !used.has(a) && measured.has(`${a.route}@${a.width}`)); + return { unmatched, stale }; +} + +/** One entry per line, so a diff of the file reads as the differences it adds and drops. */ +export function writeAccepted(file: string, list: Accepted[]) { + const rows = list.map((a) => ` ${JSON.stringify(a)}`).join(",\n"); + fs.writeFileSync(file, rows ? `[\n${rows}\n]\n` : "[]\n"); +} diff --git a/src/cli.ts b/src/cli.ts index af513c7..3db1aef 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -10,6 +10,8 @@ * Per-site knobs (masks for content that drifts, extra selectors to hide, defaults for * the flags below) live in a `renderdiff.config.ts` / `.mjs` in the working directory. */ +import path from "node:path"; +import { ACCEPTED_FILE } from "./accept.ts"; import { label, loadConfig, @@ -34,6 +36,9 @@ interface Args extends CliOverrides { routes: string[]; /** From --selector / --left-selector / --right-selector; replaces every route's own. */ selector?: Sides; + /** The reason to accept this run's differences with. */ + accept?: string; + rerunFailed: boolean; } /** `Number(raw)`, but reject anything that isn't a finite number with a flag-named error. */ @@ -49,6 +54,7 @@ function parseArgs(argv: string[]): Args { const a: Args = { dryRun: false, open: false, + rerunFailed: false, help: false, version: false, routes: [], @@ -79,7 +85,11 @@ function parseArgs(argv: string[]): Args { else if (v === "--open") a.open = true; else if (v === "--progress") a.progress = true; else if (v === "--no-progress") a.progress = false; - else if (v === "--left") a.left = next(); + else if (v === "--rerun-failed") a.rerunFailed = true; + else if (v === "--accept") { + a.accept = next().trim(); + if (!a.accept) throw new Error("--accept needs a reason"); + } else if (v === "--left") a.left = next(); else if (v === "--right") a.right = next(); else if (v === "--out") a.outDir = next(); else if (v === "--config") a.config = next(); @@ -132,6 +142,9 @@ function parseArgs(argv: string[]): Args { throw new Error(`--${has}-selector needs --${lacks}-selector or --selector`); } if (left && right) a.selector = { left, right }; + if (a.dryRun && (a.rerunFailed || a.accept !== undefined)) { + throw new Error(`${a.rerunFailed ? "--rerun-failed" : "--accept"} needs a run, not --dry-run`); + } if (state.both || state.left || state.right) { a.storageState = { left: state.left ?? state.both, right: state.right ?? state.both }; } @@ -159,6 +172,10 @@ const HELP = ` npx playwright codegen --save-storage=auth.json A before hook in the config runs on each page before it is shot, e.g. to open a menu. + --accept "" records this run's differences in renderdiff.accepted.json (next to + the config, or in cwd); later runs report them as accepted, and anything new still shows. + --rerun-failed reruns only what the previous run in --out didn't find clean or accepted. + Writes report.html, results.json and crops of each change to --out. Exits 1 if a route falls below --threshold, 3 if one errored (unmeasured) or the run crashed, 2 on bad arguments or an origin that is down. A diff without --threshold exits 0. @@ -178,6 +195,9 @@ const HELP = ` Start both sides with this storage state JSON. Never commit it. --left-storage-state , --right-storage-state That side's storage state, over --storage-state. + --accept Accept this run's differences, scoped by its routes and widths. + --rerun-failed Rerun what the last run in --out didn't find clean or accepted, + keeping the rest. Same origins, routes and widths as that run. --dry-run Print the URL pairs and exit. No browser. --open Open the report when the run finishes. --progress A line per shot as it finishes. The default on a terminal; @@ -303,6 +323,9 @@ try { cfg, open: args.open, progress: args.progress ?? !!process.stdout.isTTY, + accept: args.accept ?? null, + rerun: args.rerunFailed, + acceptedFile: path.join(configPath ? path.dirname(configPath) : process.cwd(), ACCEPTED_FILE), }), ); } catch (e) { diff --git a/src/compare.ts b/src/compare.ts index 1cc86d0..b6510b9 100644 --- a/src/compare.ts +++ b/src/compare.ts @@ -29,6 +29,10 @@ export interface Region { pixels: number; /** The elements under it; absent when not looked at. */ elements?: ElementDiff[]; + /** A coarse picture of the change on each side, for matching it against accepted ones. */ + thumb?: string; + /** The reason of the accepted difference it matched. */ + accepted?: string; } /** Rows one side has that the other lacks, at `y` in that side's own coordinates. */ @@ -38,6 +42,10 @@ export interface Band { h: number; /** The elements in its rows; absent when not looked at. */ elements?: ElementDiff[]; + /** A coarse picture of its rows, for matching it against accepted ones. */ + thumb?: string; + /** The reason of the accepted difference it matched. */ + accepted?: string; } /** Left is the changed version, right the reference: `added` is on left only, `removed` on right only. */ @@ -153,6 +161,7 @@ export async function compare(left: Shot, right: Shot): Promise { target: found[i]!, })), ]; + for (const s of spots) s.target.thumb = thumb(l, w, s.left) + thumb(r, w, s.right); // Padding is magenta against real content, so a height difference lands in // `diffPixels` and drags the score down. Intended: a page 400px shorter than the // one it's compared against has not reached parity. `heightDelta` names the cause. @@ -464,6 +473,36 @@ function inPlace(p: ReturnType, ha: Uint32Array, hb: Uint32Array) return { bands, at, rows }; } +const THUMB = 4; + +/** + * Mean colour of each cell of a 4×4 grid over the box, 4 bits a channel, as hex: a picture of + * the change that survives sub-threshold noise but not a recolour. "" for an insertion point. + */ +function thumb(raw: Buffer, stride: number, b: Box): string { + if (!b.w || !b.h) return ""; + let out = ""; + const edge = (i: number, n: number) => Math.floor((i * n) / THUMB); + for (let gy = 0; gy < THUMB; gy++) { + for (let gx = 0; gx < THUMB; gx++) { + const [x0, y0] = [b.x + edge(gx, b.w), b.y + edge(gy, b.h)]; + const [x1, y1] = [ + Math.max(x0 + 1, b.x + edge(gx + 1, b.w)), + Math.max(y0 + 1, b.y + edge(gy + 1, b.h)), + ]; + const sum = [0, 0, 0]; + for (let y = y0; y < y1; y++) { + for (let x = x0, o = (y * stride + x) * 4; x < x1; x++, o += 4) { + for (let c = 0; c < 3; c++) sum[c]! += raw[o + c]!; + } + } + const n = (x1 - x0) * (y1 - y0); + for (const v of sum) out += (Math.round(v / n) >> 4).toString(16); + } + } + return out; +} + /** Columns holding anything but each row's x=0 colour, over rows `y0..y1`; the full width if none. */ function inkX(raw: Buffer, stride: number, w: number, y0: number, y1: number) { const px = new Uint32Array(raw.buffer, raw.byteOffset, raw.length / 4); diff --git a/src/index.ts b/src/index.ts index b6db13a..a02d0e6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -38,3 +38,4 @@ export { export { type Band, type Box, type Region, type TextDiff } from "./compare.ts"; export { type ElementDiff } from "./locate.ts"; +export { type Accepted, type Fingerprint } from "./accept.ts"; diff --git a/src/report.ts b/src/report.ts index d7d9b03..2868be8 100644 --- a/src/report.ts +++ b/src/report.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import path from "node:path"; +import type { Accepted } from "./accept.ts"; import type { Band, Box, Region, TextDiff } from "./compare.ts"; import { CONTEXT_OPTIONS, name, type ResolvedConfig, type Sides } from "./config.ts"; import type { ElementDiff } from "./locate.ts"; @@ -40,6 +41,8 @@ export interface ComparedResult extends BaseResult { images: { left: string; right: string; diff: string }; /** At most three: bands, tallest first, then the largest regions. Not one per region. */ crops: CropResult[]; + /** Every band, region and size difference matched an accepted one, and none went unmeasured. */ + accepted: boolean; } /** A crop of one change: left and right panels, a 1px red box round it, or a 2px red seam. */ @@ -92,8 +95,12 @@ export interface Summary { /** Null when nothing was compared. */ mean: number | null; worst: { url: string; width: number; score: number } | null; - /** Comparisons under `meta.threshold`; 0 when no threshold was set. */ + /** Comparisons under `meta.threshold`, accepted ones aside; 0 when no threshold was set. */ belowThreshold: number; + /** Comparisons whose every difference was accepted. */ + accepted: number; + /** Accepted differences for a measured route and width that weren't seen: prune them. */ + stale: Accepted[]; } /** The shape of `results.json`, written to the out dir on every run. */ @@ -114,7 +121,12 @@ export function stats(rows: ComparedResult[]) { const round = (key: string, v: unknown) => typeof v === "number" && /^(score|alignedScore|mean)$/.test(key) ? floor4(v) : v; -export function writeResults(results: RouteResult[], meta: RunMeta, outDir: string): string { +export function writeResults( + results: RouteResult[], + meta: RunMeta, + outDir: string, + stale: Accepted[], +): string { const compared = results.filter((r): r is ComparedResult => r.status === "compared"); const { mean, worst } = compared.length ? stats(compared) : { mean: null, worst: null }; const out: Results = { @@ -126,7 +138,11 @@ export function writeResults(results: RouteResult[], meta: RunMeta, outDir: stri mean, worst: worst && { url: worst.url, width: worst.width, score: worst.score }, belowThreshold: - meta.threshold === null ? 0 : compared.filter((r) => r.score < meta.threshold!).length, + meta.threshold === null + ? 0 + : compared.filter((r) => !r.accepted && r.score < meta.threshold!).length, + accepted: compared.filter((r) => r.accepted).length, + stale, }, }; const rows = results.map((r) => ` ${JSON.stringify(r, round)}`).join(",\n"); @@ -210,6 +226,11 @@ function findings(r: ComparedResult): string { return items.length ? `
${items.join("")}
` : ""; } +const reasons = (r: ComparedResult) => + [ + ...new Set([...(r.bands ?? []), ...r.regions].flatMap((g) => (g.accepted ? [g.accepted] : []))), + ].join("; "); + function card(r: ComparedResult): string { const imgs = r.images; return ` @@ -219,6 +240,7 @@ function card(r: ComparedResult): string { ${esc(name(r))} @${r.width}px ${r.hook ? `before hook` : ""} + ${r.accepted ? `accepted: ${esc(reasons(r))}` : ""} ${r.heightDelta ? `height ${signed(r.heightDelta)}px` : ""} ${r.widthDelta ? `width ${signed(r.widthDelta)}px` : ""} ${r.unstablePixels ? `${r.unstablePixels.toLocaleString("en-US")} px unstable` : ""} @@ -238,6 +260,7 @@ export async function writeReport( results: RouteResult[], meta: RunMeta, cfg: ResolvedConfig, + stale: Accepted[], ): Promise { const compared = results.filter((r): r is ComparedResult => r.status === "compared"); const errors = results.filter((r): r is ErrorResult => r.status === "error"); @@ -262,7 +285,7 @@ export async function writeReport( margin:0 0 1rem; padding:.75rem 1rem 1rem; } .card header { display:flex; align-items:center; gap:.75rem; flex-wrap:wrap; margin-bottom:.75rem } .score { font-weight:700; font-variant-numeric:tabular-nums; min-width:4.5rem } - .w, .px, .delta, .unstable, .aligned, .hook { color:var(--mut); font-size:12px } + .w, .px, .delta, .unstable, .aligned, .hook, .accepted { color:var(--mut); font-size:12px } .findings { font-size:12px; color:var(--mut); margin:0 0 .75rem } .findings p { margin:.25rem 0 0 } .findings ul { margin:.25rem 0; font:12px/1.4 ui-monospace,SFMono-Regular,Menlo,monospace } @@ -304,6 +327,14 @@ ${ : "" } +${ + stale.length + ? `

Stale accepted differences

+

Accepted, but not seen on this run: prune them from the accepted file.

+
    ${stale.map((a) => `
  • ${esc(a.route)} @${a.width}px — ${esc(a.reason)}
  • `).join("")}
` + : "" +} +
What was neutralized

Blocked on both sides — analytics, consent, and A/B tooling that would otherwise diff --git a/src/run.ts b/src/run.ts index c191c93..a7701f9 100644 --- a/src/run.ts +++ b/src/run.ts @@ -2,7 +2,8 @@ import { spawn } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; -import { chromium, errors, type Page } from "playwright"; +import { chromium, errors, type Browser, type Page } from "playwright"; +import { applyAccepted, readAccepted, writeAccepted } from "./accept.ts"; import { cancelUnstable, capture, HookError, prepare, type Shot } from "./capture.ts"; import { compare, crops, type Band } from "./compare.ts"; import { locate, type ElementDiff } from "./locate.ts"; @@ -23,6 +24,7 @@ import { writeResults, type ComparedResult, type ErrorResult, + type Results, type RouteResult, } from "./report.ts"; @@ -43,6 +45,11 @@ interface RunOpts { open: boolean; /** A line per shot as it lands; otherwise one compact summary at the end. */ progress: boolean; + /** Accept every difference not yet accepted, with this reason. */ + accept: string | null; + /** Rerun only what the previous results.json in the out dir didn't find clean or accepted. */ + rerun: boolean; + acceptedFile: string; } const slug = (url: string) => @@ -88,29 +95,106 @@ function wipeable(dir: string): boolean { return fs.statSync(dir).isDirectory() && fs.readdirSync(dir).every(ours); } -export async function run({ routes, cfg, open, progress }: RunOpts): Promise { - if (!wipeable(cfg.outDir)) { +const reusable = (r: RouteResult) => + r.status === "compared" && ((r.diffPixels === 0 && r.unstablePixels === 0) || r.accepted); + +const files = (r: RouteResult) => + r.status === "compared" ? [...Object.values(r.images), ...r.crops.map((c) => c.path)] : []; + +/** The previous run's results when this run repeats it, else why it doesn't. */ +function previous(file: string, now: Record): Results | string { + if (!fs.existsSync(file)) return `no ${file} to rerun from; run without --rerun-failed first`; + let prev: Results; + try { + prev = JSON.parse(fs.readFileSync(file, "utf8")); + } catch { + return `${file} is not valid JSON; run without --rerun-failed`; + } + const was: Record = { + ...prev.meta, + routes: [...new Set(prev.results?.map(name))], + }; + for (const [key, value] of Object.entries(now)) { + if (JSON.stringify(value) !== JSON.stringify(was[key])) { + return ( + `${key} ${JSON.stringify(value)} differ from the previous run's ${JSON.stringify(was[key])} ` + + `in ${file}; run without --rerun-failed to start over` + ); + } + } + return prev; +} + +export async function run({ + routes, + cfg, + open, + progress, + accept, + rerun, + acceptedFile, +}: RunOpts): Promise { + const names = routes.map(name); + const setup = { + left: cfg.left, + right: cfg.right, + widths: cfg.widths, + blockUrlPatterns: cfg.blockUrlPatterns, + hideSelectors: cfg.hideSelectors, + masks: cfg.masks.map(({ urlPath, reason }) => ({ urlPath, reason })), + storageState: { + left: cfg.storageState.left && path.basename(cfg.storageState.left), + right: cfg.storageState.right && path.basename(cfg.storageState.right), + }, + }; + let accepted; + try { + accepted = readAccepted(acceptedFile); + } catch (e) { + console.error(`✗ ${e instanceof Error ? e.message : String(e)}`); + return 2; + } + const prev = rerun + ? previous(path.join(cfg.outDir, "results.json"), { + version: version(), + ...setup, + routes: names, + }) + : null; + if (typeof prev === "string") { + console.error(`✗ --rerun-failed: ${prev}`); + return 2; + } + if (!prev && !wipeable(cfg.outDir)) { console.error( `✗ output directory ${cfg.outDir} holds files renderdiff didn't write — ` + `refusing to wipe it. Pass --out with a new or empty directory.`, ); return 2; } - fs.rmSync(cfg.outDir, { recursive: true, force: true }); + if (!prev) fs.rmSync(cfg.outDir, { recursive: true, force: true }); fs.mkdirSync(cfg.outDir, { recursive: true }); + if (prev) applyAccepted(prev.results, accepted); + const before = new Map(prev?.results.map((r) => [`${name(r)}@${r.width}`, r])); + const results: RouteResult[] = []; const jobs: Job[] = []; - for (const route of routes) for (const width of cfg.widths) jobs.push({ route, width }); + for (const route of routes) { + for (const width of cfg.widths) { + const had = before.get(`${name(route)}@${width}`); + if (had && reusable(had)) results.push(had); + else jobs.push({ route, width }); + } + } const stems = slugs(routes); - const results: RouteResult[] = []; - const { browser, channel } = await launch(); + const launched = jobs.length ? await launch() : null; const queue = [...jobs]; let done = 0; // Whole minute, so an HH:MM display reads the same on both sides. const now = Math.floor(Date.now() / 60_000) * 60_000; - const worker = async () => { + const worker = async (browser: Browser) => { // One context per origin so cookies/localStorage never cross between them. const leftCtx = await browser.newContext({ ...CONTEXT_OPTIONS, @@ -195,6 +279,7 @@ export async function run({ routes, cfg, open, progress }: RunOpts): Promise worker(launched.browser))); + await launched.browser.close(); + } + const browserMeta = launched + ? { name: launched.channel ?? "chromium", version: launched.browser.version() } + : prev!.meta.browser; + + // A rerun overwrites a shot's files in place; drop the ones it no longer writes, ours only. + const kept = new Set(results.flatMap(files)); + for (const { route, width } of jobs) { + const had = before.get(`${name(route)}@${width}`); + for (const f of had ? files(had) : []) { + if (!kept.has(f) && ours(f)) fs.rmSync(path.join(cfg.outDir, f), { force: true }); + } + } // Workers finish out of order; job order keeps results.json stable across runs. - const names = routes.map(name); const order = (r: RouteResult) => names.indexOf(name(r)) * cfg.widths.length + cfg.widths.indexOf(r.width); results.sort((a, b) => order(a) - order(b)); + let { unmatched, stale } = applyAccepted(results, accepted); + if (accept !== null) { + writeAccepted(acceptedFile, [ + ...accepted.filter((a) => !stale.includes(a)), + ...unmatched.map((u) => ({ ...u, reason: accept })), + ]); + console.log( + `accepted ${unmatched.length} difference(s), pruned ${stale.length} stale — ${acceptedFile}`, + ); + ({ stale } = applyAccepted(results, readAccepted(acceptedFile))); + } + + const { left, right, widths, ...rest } = setup; const meta = { tool: "renderdiff" as const, version: version(), generatedAt: new Date().toISOString(), - left: cfg.left, - right: cfg.right, - widths: cfg.widths, + left, + right, + widths, threshold: cfg.threshold, clock: new Date(now).toISOString(), - blockUrlPatterns: cfg.blockUrlPatterns, - hideSelectors: cfg.hideSelectors, - masks: cfg.masks.map(({ urlPath, reason }) => ({ urlPath, reason })), + ...rest, browser: browserMeta, - storageState: { - left: cfg.storageState.left && path.basename(cfg.storageState.left), - right: cfg.storageState.right && path.basename(cfg.storageState.right), - }, }; - const reportPath = await writeReport(results, meta, cfg); - const resultsPath = writeResults(results, meta, cfg.outDir); - + const reportPath = await writeReport(results, meta, cfg, stale); + const resultsPath = writeResults(results, meta, cfg.outDir, stale); + + if (prev) { + const rerunRoutes = new Set(jobs.map((j) => name(j.route))).size; + console.log( + `rerun: ${jobs.length} shot(s) on ${rerunRoutes} route(s), ` + + `${results.length - jobs.length} reused, ${names.length - rerunRoutes} route(s) untouched`, + ); + } + if (stale.length) { + console.log( + `stale accepted: ${stale.map((a) => `${a.route}@${a.width}`).join(" ")} — ${acceptedFile}`, + ); + } if (progress) summarize(results, reportPath, resultsPath); else console.log(compact(results, names, resultsPath, cfg.threshold)); @@ -279,7 +395,8 @@ export async function run({ routes, cfg, open, progress }: RunOpts): Promise r.status === "compared" && cfg.threshold !== null && r.score < cfg.threshold, + (r) => + r.status === "compared" && !r.accepted && cfg.threshold !== null && r.score < cfg.threshold, ); if (below.length) { console.error(`\n✗ ${below.length} route(s) below ${pct(cfg.threshold!)}.`); @@ -349,6 +466,13 @@ function summarize(results: RouteResult[], reportPath: string, resultsPath: stri } } + const accepted = compared.filter((r) => r.accepted); + if (accepted.length) { + console.log( + `\nAccepted (${accepted.length}): ${accepted.map((r) => `${name(r)}@${r.width}`).join(" ")}`, + ); + } + const errors = results.filter((r): r is ErrorResult => r.status === "error"); if (errors.length) { console.log(`\nErrors (${errors.length}):`); @@ -362,13 +486,13 @@ function summarize(results: RouteResult[], reportPath: string, resultsPath: stri /** At most this many elements per summary line. */ const DOM_ELEMENTS = 3; -/** Located elements in crop order, bands tallest first then regions, one per selector. */ +/** Located elements in crop order, bands tallest first then regions, one per selector; none accepted. */ function elementsOf(r: { - regions: { elements?: ElementDiff[] }[]; - bands: (Band & { elements?: ElementDiff[] })[] | null; + regions: { elements?: ElementDiff[]; accepted?: string }[]; + bands: Band[] | null; }): ElementDiff[] { - const all = [...[...(r.bands ?? [])].sort((a, b) => b.h - a.h), ...r.regions].flatMap( - (g) => g.elements ?? [], + const all = [...[...(r.bands ?? [])].sort((a, b) => b.h - a.h), ...r.regions].flatMap((g) => + g.accepted ? [] : (g.elements ?? []), ); return all .filter((e, i) => all.findIndex((o) => o.selector === e.selector) === i) @@ -391,19 +515,29 @@ export function compact( ): string { const out: string[] = []; const clean: string[] = []; + const accepted: string[] = []; let differ = 0; for (const route of routes) { const rows = results.filter((r) => name(r) === route); const ok = rows.filter((r): r is ComparedResult => r.status === "compared"); // Unstable pixels went unmeasured, so a shot with any is not clean, only unchanged. const same = ok.filter((r) => r.diffPixels === 0 && r.unstablePixels === 0); - const changed = ok.filter((r) => !same.includes(r)); + const acc = ok.filter((r) => r.accepted); + const changed = ok.filter((r) => !same.includes(r) && !r.accepted); if (same.length === rows.length) { clean.push(route); continue; } + if (same.length + acc.length === rows.length) { + accepted.push(route); + continue; + } if (changed.some((r) => r.diffPixels > 0)) differ++; - out.push(route + (same.length ? ` clean @${same.map((r) => r.width).join(",")}` : "")); + out.push( + route + + (same.length ? ` clean @${same.map((r) => r.width).join(",")}` : "") + + (acc.length ? ` accepted @${acc.map((r) => r.width).join(",")}` : ""), + ); const groups = new Map(); for (const r of changed) { const key = JSON.stringify([ @@ -428,10 +562,12 @@ export function compact( if (r.status === "error") failed.set(r.error, [...(failed.get(r.error) ?? []), r.width]); for (const [error, widths] of failed) out.push(` @${widths.join(",")} ERROR ${error}`); } + if (accepted.length) out.unshift(`accepted: ${accepted.join(" ")}`); if (clean.length) out.unshift(`clean: ${clean.join(" ")}`); - const compared = results.filter((r): r is ComparedResult => r.status === "compared"); + const all = results.filter((r): r is ComparedResult => r.status === "compared"); + const compared = all.filter((r) => !r.accepted); const shots = compared.filter((r) => r.diffPixels > 0).length; - const errors = results.length - compared.length; + const errors = results.length - all.length; const worst = compared.length ? stats(compared).worst : null; out.push( `renderdiff: ${differ} of ${routes.length} routes differ (${shots} of ${results.length} shots), ` + @@ -453,6 +589,9 @@ function row(g: ComparedResult[]): string { return lo === hi ? f(lo) : `${f(lo)}–${f(hi)}`; }; const regions = g.map((x) => x.regions.length); + const accepted = g.map( + (x) => [...(x.bands ?? []), ...x.regions].filter((c) => c.accepted).length, + ); const unstable = Math.max(...g.map((x) => x.unstablePixels)); // Band y varies with width; a collapsed row shows the first one's, marked approximate. const y = (b: Band) => `@y${g.length > 1 ? "~" : ""}${b.y}`; @@ -469,6 +608,7 @@ function row(g: ComparedResult[]): string { r.alignedScore !== null ? `aligned ${pct(r.alignedScore)}` : "", Math.max(...regions) ? `${span(regions, String)} reg` : "", unstable ? `unstable ${unstable.toLocaleString("en-US")}px` : "", + Math.max(...accepted) ? `${span(accepted, String)} accepted` : "", text.length ? `text ${text.slice(0, TEXT_LINES).join(" ")}` + (text.length > TEXT_LINES ? ` (+${text.length - TEXT_LINES} more)` : "") diff --git a/tests/helpers/cli.ts b/tests/helpers/cli.ts index 559fe13..2adfa47 100644 --- a/tests/helpers/cli.ts +++ b/tests/helpers/cli.ts @@ -46,6 +46,8 @@ export interface CliOptions { cwd?: string; /** Skip appending `--out`. Only for runs that never reach the browser (`--dry-run`). */ noOut?: boolean; + /** Reuse an out dir from an earlier run, e.g. for `--rerun-failed`. Must be a `CliResult.outDir`. */ + outDir?: string; timeoutMs?: number; /** Extra environment variables for the subprocess. */ env?: Record; @@ -66,7 +68,7 @@ export interface CliResult { export function runCli(args: string[], opts: CliOptions = {}): Promise { const cwd = opts.cwd ?? tempDir("renderdiff-cwd-"); - const outDir = path.join(tempDir("renderdiff-out-"), "out"); + const outDir = opts.outDir ?? path.join(tempDir("renderdiff-out-"), "out"); const argv = opts.noOut ? [...args] : [...args, "--out", outDir]; return new Promise((resolve, reject) => { diff --git a/tests/helpers/parse.ts b/tests/helpers/parse.ts index 7cc0f07..22e0ca1 100644 --- a/tests/helpers/parse.ts +++ b/tests/helpers/parse.ts @@ -45,11 +45,18 @@ export function result( export interface Summary { /** Routes on the `clean:` line; a selector route is `/a [sel]`, as is its key in `routes`. */ clean: string[]; - /** Each differing route: widths that were clean, then one row per collapsed width group. */ + /** Routes on the `accepted:` line: every width clean or accepted. */ + accepted: string[]; + /** `route@width` of each stale accepted difference, from the `stale accepted:` line. */ + stale: string[]; + /** The `rerun:` line of a `--rerun-failed` run, else null. */ + rerun: { shots: number; routes: number; reused: number; untouched: number } | null; + /** Each differing route: widths that were clean or accepted, then one row per collapsed width group. */ routes: Map< string, { clean: number[]; + accepted: number[]; rows: { widths: number[]; text: string; dom: string | null; crops: string[] }[]; errors: { widths: number[]; message: string }[]; } @@ -77,13 +84,25 @@ export function parseSummary(stdout: string): Summary { const v = lines.map((l) => VERDICT.exec(l)).find(Boolean); if (!v) throw new Error(`no verdict line in:\n${stdout}`); // A selector can hold spaces, so a route ends where the next `/` starts or the line ends. - const clean = /^clean: (.*)$/m.exec(stdout)?.[1]!.match(/\/\S*(?: \[.*?\])?(?= \/|$)/g) ?? []; + const listed = (label: string) => + new RegExp(`^${label}: (.*)$`, "m").exec(stdout)?.[1]!.match(/\/\S*(?: \[.*?\])?(?= \/|$)/g) ?? + []; + const stale = /^stale accepted: (.*) — .*$/m.exec(stdout)?.[1]!.split(" ") ?? []; + const rerun = + /^rerun: (\d+) shot\(s\) on (\d+) route\(s\), (\d+) reused, (\d+) route\(s\) untouched$/m.exec( + stdout, + ); const routes: Summary["routes"] = new Map(); let at: ReturnType; for (const line of lines) { let m; - if ((m = /^(\/\S*(?: \[.*\])?)(?: clean @([\d,]+))?$/.exec(line))) { - at = { clean: m[2] ? m[2].split(",").map(Number) : [], rows: [], errors: [] }; + if ((m = /^(\/\S*(?: \[.*\])?)(?: clean @([\d,]+))?(?: accepted @([\d,]+))?$/.exec(line))) { + at = { + clean: m[2] ? m[2].split(",").map(Number) : [], + accepted: m[3] ? m[3].split(",").map(Number) : [], + rows: [], + errors: [], + }; routes.set(m[1]!, at); } else if (at && (m = /^ @([\d,]+) ERROR (.*)$/.exec(line))) { at.errors.push({ widths: m[1]!.split(",").map(Number), message: m[2]! }); @@ -96,7 +115,15 @@ export function parseSummary(stdout: string): Summary { } } return { - clean, + clean: listed("clean"), + accepted: listed("accepted"), + stale, + rerun: rerun && { + shots: Number(rerun[1]), + routes: Number(rerun[2]), + reused: Number(rerun[3]), + untouched: Number(rerun[4]), + }, routes, verdict: { differ: Number(v[1]), @@ -238,6 +265,8 @@ export interface ReportCard { crops: string[]; /** The card carries the "before hook" badge. */ hook: boolean; + /** The accepted badge's reasons, or null. */ + accepted: string | null; } export interface ReportErrorRow { @@ -249,6 +278,8 @@ export interface ReportErrorRow { export interface Report { cards: ReportCard[]; errors: ReportErrorRow[]; + /** The stale accepted differences section; `message` is the reason. */ + stale: ReportErrorRow[]; /** Block globs listed in the "What was neutralized" disclosure. */ blockPatterns: string[]; /** Mask `reason` strings listed in the same disclosure. */ @@ -265,6 +296,8 @@ const unesc = (s: string) => .replace(/"/g, '"') .replace(/&/g, "&"); +const unescOr = (s: string | undefined) => (s === undefined ? null : unesc(s)); + function between(html: string, start: string, end: string): string { const i = html.indexOf(start); if (i === -1) return ""; @@ -323,14 +356,18 @@ export function parseReport(html: string): Report { images: [...card.matchAll(/ m[1]!), crops: [...card.matchAll(/ m[1]!), hook: card.includes('before hook'), + accepted: unescOr(/accepted: ([^<]*)<\/span>/.exec(card)?.[1]), }); } - const errors: ReportErrorRow[] = []; - const errorSection = between(html, '

Errors

', ""); - for (const m of errorSection.matchAll(/
  • ([^<]*)<\/code> @(\d+)px — ([\s\S]*?)<\/li>/g)) { - errors.push({ route: unesc(m[1]!), width: Number(m[2]), message: m[3]! }); - } + const listed = (section: string) => + [...section.matchAll(/
  • ([^<]*)<\/code> @(\d+)px — ([\s\S]*?)<\/li>/g)].map((m) => ({ + route: unesc(m[1]!), + width: Number(m[2]), + message: m[3]!, + })); + const errors = listed(between(html, '

    Errors

    ', "")); + const stale = listed(between(html, "

    Stale accepted differences

    ", "")); const neutralized = between(html, "
    ", "
    "); const blockPatterns = [...neutralized.matchAll(/
  • ([^<]*)<\/code><\/li>/g)].map( @@ -338,7 +375,7 @@ export function parseReport(html: string): Report { ); const maskReasons = [...neutralized.matchAll(/
  • ((?!)[^<]*)<\/li>/g)].map((m) => m[1]!); - return { cards, errors, blockPatterns, maskReasons, neutralized }; + return { cards, errors, stale, blockPatterns, maskReasons, neutralized }; } /** The report card for one comparison. Throws with context when absent. */ diff --git a/tests/unit/cli-args.test.ts b/tests/unit/cli-args.test.ts index 7432974..0b47aef 100644 --- a/tests/unit/cli-args.test.ts +++ b/tests/unit/cli-args.test.ts @@ -54,6 +54,8 @@ describe("help and version", () => { "--storage-state", "--left-storage-state", "--right-storage-state", + "--accept", + "--rerun-failed", ]) { assert.ok(r.stdout.includes(flag), `--help does not mention ${flag}`); } @@ -349,6 +351,18 @@ describe("storage state", () => { }); }); +describe("accept and rerun", () => { + test("--accept needs a reason", async () => { + await rejects(["--dry-run", L, R, "/a", "--accept"], "--accept needs a value"); + await rejects(["/a", "--left", L, "--right", R, "--accept", " "], "--accept needs a reason"); + }); + + test("--accept and --rerun-failed need a run, not --dry-run", async () => { + await rejects(["--dry-run", L, R, "/a", "--accept", "known"], "--accept needs a run"); + await rejects(["--dry-run", L, R, "/a", "--rerun-failed"], "--rerun-failed needs a run"); + }); +}); + describe("preflight", () => { // Port 1 is reserved and never listening, so this is a closed port rather than a race // against something we just shut down. diff --git a/tests/visual/accepted.test.ts b/tests/visual/accepted.test.ts new file mode 100644 index 0000000..e62dad7 --- /dev/null +++ b/tests/visual/accepted.test.ts @@ -0,0 +1,224 @@ +/** + * Accepted differences and --rerun-failed: the loop a migration runs in. Record what already + * differs, change code, re-check only what's red. + * + * A `before` hook that restyles the left side stands in for "changed code": the fixture pages + * are static, and a hook is the one way to change a side between two runs of the same route. + * Each acceptance is paired with a control, so the mechanism is shown hiding only what it was + * told to: a new change elsewhere on an accepted page, and a different change in the same spot, + * both still surface. + */ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import { after, before, describe, test } from "node:test"; +import { cleanupTempDirs, context, renderdiffArgs, runCli, tempDir } from "../helpers/cli.ts"; +import { card, parseReport, parseSummary, result, results } from "../helpers/parse.ts"; +import { startFixtureSite, type FixtureSite } from "../helpers/server.ts"; + +let site: FixtureSite; + +before(async () => { + site = await startFixtureSite(); +}); + +after(async () => { + await site?.close(); + cleanupTempDirs(); +}); + +const args = (routes: string[], extra: string[] = [], widths = [1440]) => [ + ...renderdiffArgs({ left: site.left, right: site.right, routes, widths, concurrency: 2 }), + ...extra, +]; + +/** A config in `dir` whose hook restyles the left side, so the accepted file is found beside it. */ +function leftCss(dir: string, css: string) { + const file = path.join(dir, "hook.mjs"); + fs.writeFileSync( + file, + `export default { before: async (page, { side }) => { if (side === "left") await page.addStyleTag({ content: ${JSON.stringify(css)} }); } };\n`, + ); + return ["--config", file]; +} + +const THIRD = ".strip:first-of-type > .swatch:nth-child(3)"; + +describe("accepted differences", () => { + let dir: string; + const file = () => path.join(dir, "renderdiff.accepted.json"); + + before(async () => { + dir = tempDir(); + const run = await runCli(args(["/", "/color", "/nav"], ["--accept", "known before the move"]), { + cwd: dir, + }); + assert.equal(run.code, 0, context(run)); + assert.match(run.stdout, /^accepted 3 difference\(s\), pruned 0 stale — .*$/m, context(run)); + }); + + test("--accept records each change with its route, width, fingerprint and reason", () => { + const list = JSON.parse(fs.readFileSync(file(), "utf8")) as { + route: string; + width: number; + reason: string; + fingerprint: { kind: string }; + }[]; + // /color is one region; /nav is one band and the 24px it costs the page. + assert.deepEqual( + list.map((a) => [a.route, a.width, a.fingerprint.kind, a.reason]), + [ + ["/color", 1440, "region", "known before the move"], + ["/nav", 1440, "band", "known before the move"], + ["/nav", 1440, "size", "known before the move"], + ], + fs.readFileSync(file(), "utf8"), + ); + }); + + test("a later run reports them as accepted, not as differing, and passes the threshold", async () => { + const run = await runCli(args(["/", "/color", "/nav"], ["--threshold", "0.999"]), { + cwd: dir, + }); + assert.equal(run.code, 0, context(run)); + const s = parseSummary(run.stdout); + assert.deepEqual([s.clean, s.accepted, [...s.routes.keys()]], [["/"], ["/color", "/nav"], []]); + assert.deepEqual([s.verdict.differ, s.verdict.shots, s.verdict.below?.count], [0, 0, 0]); + const res = results(run.outDir); + assert.equal(result(res, "/color", 1440).accepted, true); + assert.equal(result(res, "/color", 1440).regions[0]!.accepted, "known before the move"); + assert.deepEqual([res.summary.accepted, res.summary.belowThreshold], [2, 0]); + const report = parseReport(run.report()!); + assert.equal(card(report, "/nav", 1440).accepted, "known before the move"); + assert.equal(card(report, "/", 1440).accepted, null); + }); + + test("a new change elsewhere on an accepted page still surfaces, the accepted one aside", async () => { + const run = await runCli(args(["/color"], [...leftCss(dir, ".notes h2 { color: #2e7d32 }")]), { + cwd: dir, + }); + const s = parseSummary(run.stdout); + assert.deepEqual(s.accepted, [], context(run)); + const [row] = s.routes.get("/color")!.rows; + assert.match(row!.text, / 2 reg 1 accepted/, context(run)); + // The dom line names the new heading, not the swatch already accepted. + assert.match(row!.dom ?? "", /h2 color/); + assert.doesNotMatch(row!.dom ?? "", /rgb\(198, 40, 40\)/, "the accepted red swatch"); + const r = result(results(run.outDir), "/color", 1440); + assert.equal(r.accepted, false); + assert.deepEqual(r.regions.map((g) => g.accepted ?? null).sort(), [ + "known before the move", + null, + ]); + }); + + test("a different change in the accepted spot is not the accepted one", async () => { + const run = await runCli(args(["/color"], leftCss(dir, `${THIRD} { background: #2e7d32 }`)), { + cwd: dir, + }); + const s = parseSummary(run.stdout); + assert.deepEqual(s.accepted, [], context(run)); + const r = result(results(run.outDir), "/color", 1440); + assert.equal(r.regions.length, 1, context(run)); + assert.equal(r.regions[0]!.accepted, undefined); + assert.equal(s.verdict.differ, 1); + }); + + test("an accepted difference no longer seen is listed as stale; unchecked routes aren't", async () => { + const run = await runCli(args(["/color"], leftCss(dir, `${THIRD} { background: #c62828 }`)), { + cwd: dir, + }); + assert.equal(run.code, 0, context(run)); + const s = parseSummary(run.stdout); + assert.deepEqual([s.clean, s.stale], [["/color"], ["/color@1440"]], context(run)); + assert.deepEqual( + parseReport(run.report()!).stale.map((a) => [a.route, a.width, a.message]), + [["/color", 1440, "known before the move"]], + ); + assert.equal(results(run.outDir).summary.stale.length, 1); + }); + + test("--accept is scoped to the run's routes and widths, and prunes their stale entries", async () => { + const dir2 = tempDir(); + fs.copyFileSync(file(), path.join(dir2, "renderdiff.accepted.json")); + const hook = leftCss(dir2, `${THIRD} { background: #c62828 }`); + const run = await runCli(args(["/color"], [...hook, "--accept", "fixed"]), { cwd: dir2 }); + assert.match(run.stdout, /^accepted 0 difference\(s\), pruned 1 stale/m, context(run)); + const list = JSON.parse( + fs.readFileSync(path.join(dir2, "renderdiff.accepted.json"), "utf8"), + ) as { route: string }[]; + assert.deepEqual( + list.map((a) => a.route), + ["/nav", "/nav"], + ); + }); +}); + +describe("--rerun-failed", () => { + test("reruns only what wasn't clean or accepted, and leaves the rest untouched", async () => { + const dir = tempDir(); + const routes = ["/", "/color", "/nav"]; + const first = await runCli(args(routes, [], [1440, 375])); + assert.equal(first.code, 0, context(first)); + // Accept /color elsewhere: the rerun reads it from cwd and matches it against results.json. + const accepted = await runCli(args(["/color"], ["--accept", "known"], [1440, 375]), { + cwd: dir, + }); + assert.equal(accepted.code, 0, context(accepted)); + + const index = first.file("index.1440.left.png"); + const mtime = fs.statSync(index).mtimeMs; + const was = results(first.outDir); + const hits = { home: site.hits("/"), color: site.hits("/color"), nav: site.hits("/nav") }; + + const run = await runCli(args(routes, ["--rerun-failed"], [1440, 375]), { + cwd: dir, + outDir: first.outDir, + }); + assert.equal(run.code, 0, context(run)); + assert.deepEqual( + [site.hits("/"), site.hits("/color")], + [hits.home, hits.color], + "clean and accepted routes are not loaded again", + ); + assert.ok(site.hits("/nav") > hits.nav, context(run)); + const s = parseSummary(run.stdout); + assert.deepEqual(s.rerun, { shots: 2, routes: 1, reused: 4, untouched: 2 }, context(run)); + assert.deepEqual([s.clean, s.accepted, [...s.routes.keys()]], [["/"], ["/color"], ["/nav"]]); + + const now = results(run.outDir); + assert.deepEqual( + now.results.map((r) => `${r.url}@${r.width}`), + ["/@1440", "/@375", "/color@1440", "/color@375", "/nav@1440", "/nav@375"], + ); + assert.deepEqual(result(now, "/", 1440), result(was, "/", 1440)); + assert.equal(fs.statSync(index).mtimeMs, mtime, "a reused shot's files are not rewritten"); + assert.equal(parseReport(run.report()!).cards.length, 6); + for (const r of now.results) { + if (r.status === "compared") assert.ok(fs.existsSync(run.file(r.images.diff)), r.url); + } + }); + + test("a run that doesn't repeat the previous one is refused", async () => { + const first = await runCli(args(["/color"])); + assert.equal(first.code, 0, context(first)); + const again = (a: string[]) => runCli(a, { outDir: first.outDir }); + for (const [a, needle] of [ + [args(["/color"], ["--rerun-failed"], [375]), /--rerun-failed: widths \[375\] differ/], + [args(["/nav"], ["--rerun-failed"]), /--rerun-failed: routes \["\/nav"\] differ/], + [ + [...args(["/color"], ["--rerun-failed"]), "--right", site.left], + /--rerun-failed: right .* differ/, + ], + ] as const) { + const r = await again([...a]); + assert.equal(r.code, 2, context(r)); + assert.match(r.stderr, needle); + } + assert.ok(first.report(), "a refused rerun leaves the out dir alone"); + + const fresh = await runCli(args(["/color"], ["--rerun-failed"])); + assert.equal(fresh.code, 2, context(fresh)); + assert.match(fresh.stderr, /no .*results\.json to rerun from/); + }); +}); diff --git a/tests/visual/controls.test.ts b/tests/visual/controls.test.ts index 8e6ed0e..6365a4a 100644 --- a/tests/visual/controls.test.ts +++ b/tests/visual/controls.test.ts @@ -202,7 +202,7 @@ for (const route of ["/color", "/nav"]) { assert.equal(r.widthDelta, c.widthDelta); assert.equal(r.unstablePixels, c.unstablePixels); assert.deepEqual( - r.regions.slice(0, 5).map(({ elements, ...g }) => g), + r.regions.slice(0, 5).map(({ elements, thumb, ...g }) => g), c.regions.map(({ dom, ...g }) => g), ); assert.deepEqual(r.text, { added: c.added, removed: c.removed }); From 42a3a921af71d877fc9c56a9dc4513d2bba092eb Mon Sep 17 00:00:00 2001 From: Timur Mashara <62305878+tmashara@users.noreply.github.com> Date: Thu, 22 Oct 2026 15:39:27 +0200 Subject: [PATCH 2/2] Tighten accepted matching and rerun checks after review - Fingerprint a change by a hash of every pixel under it (4 bits a channel) instead of a 4x4 mean-colour grid, and require the elements an entry recorded to be named again, so a same-size, same-tone redraw surfaces. - Never accept a shot whose padding no band or region covers (width delta, or a height delta that didn't align); the size fingerprint is gone. - Leave accepted shots out of summary.mean/worst, the --progress summary and the report's aggregate line. - Hash the resolved config, hook and locate sources included, into meta.config, and refuse --rerun-failed when it changed. - --accept re-reads the file before writing, merges, sorts by route then width, writes through a temp file and rename, and skips the write when nothing changed. - Document that meta describes the rerun, not the reused shots. Co-Authored-By: Claude Opus 5.5 --- README.md | 45 +++++++++++-------- skills/renderdiff/reference.md | 10 +++-- src/accept.ts | 81 +++++++++++++++++++++------------- src/compare.ts | 48 +++++++++----------- src/report.ts | 28 +++++++++--- src/run.ts | 54 ++++++++++++++++++----- tests/visual/accepted.test.ts | 44 +++++++++++++----- tests/visual/controls.test.ts | 2 +- 8 files changed, 201 insertions(+), 111 deletions(-) diff --git a/README.md b/README.md index 14bbf55..4504765 100644 --- a/README.md +++ b/README.md @@ -276,36 +276,43 @@ A shot whose every difference matches an accepted one is `accepted`: it is liste `"accepted": true` in `results.json` and a badge in the report. Anything else on the page still shows: a row with some changes accepted says `1 accepted`, and its `dom` line skips them. A shot with unstable pixels is never accepted, since part of it went unmeasured. +`summary.mean` and `summary.worst` leave accepted shots out, like the verdict line. Accepted entries for a route and width the run measured but didn't see are **stale**: they print on a `stale accepted:` line, sit in `summary.stale` and the report, and the next `--accept` over that route and width prunes them. -Each entry is one change: a band, a region, or the page's size difference, keyed by route -(as the summary names it) and width, with a fingerprint. Two changes match when they have -the same kind and side, the same `x`, width and height (regions sit on the 16px grid, bands -are exact rows), the same `dom` elements with the same changed values (when both runs named -them), and each side's pixels look alike: a 4×4 grid of mean colours that may drift one step -in sixteen per channel. That makes a match: +Each entry is one change, a band or a region, keyed by route (as the summary names it) and +width, with a fingerprint. Two changes match when they have the same kind and side, the same +`x`, width and height (regions sit on the 16px grid, bands are exact rows), the same hash of +every pixel under the change on both sides at 4 bits a channel, and, when the entry named +elements, the same `dom` elements with the same changed values. That makes a match: -- robust to sub-threshold noise (pixelmatch's tolerance, the grid and the colour step), to a - change above it moving it down the page (`y` is left out), and to unrelated changes - elsewhere (they are their own changes, and stay unaccepted); -- strict about the spot itself: a different colour or value there, a change touching it (the - region grows), or a second identical change (an entry matches once) all surface. +- robust to a change above it moving it down the page (`y` is left out), to unrelated + changes elsewhere (they are their own changes, and stay unaccepted), and to shade jitter + that stays within one of sixteen steps per channel; +- strict about the spot itself: any other change to what is drawn there, a change touching + it (the region grows), or a second identical change (an entry matches once) all surface. -The cost is false alarms, never hidden changes, except one: a change inside an accepted -region too small to move its mean colours and touching no element `dom` names. A new change -that makes repeated identical rows align differently can also turn an accepted region into -bands, so it shows again; accept it again once you've looked. +The cost is false alarms rather than hidden changes. Noise that crosses a 4-bit step, an +element probe that times out, or a new change that makes repeated identical rows align +differently (turning an accepted region into bands) makes an accepted change show again; +accept it again once you've looked. What can still hide is a shade change under one step +(e.g. `#000` to `#080808`) that doesn't cross a step boundary anywhere in the change. + +A shot whose pages differ in width, or in height without the rows aligning into bands, is +never accepted: the padding past the shorter page belongs to no band or region, so no +fingerprint covers it. `--rerun-failed` reads `results.json` in `--out`, keeps every shot that was clean or is accepted (against the accepted file as it is now), reruns the rest — differences, unstable shots, errors — and merges them back into `results.json` and `report.html` in route and width order. The out dir is kept rather than wiped; only a rerun shot's own old files are replaced. -A `rerun:` line says how many shots it reran on how many routes, and how many it reused. It -must repeat the previous run: different origins, routes, widths, masks, hidden selectors, -block patterns, storage state or renderdiff version exit 2, as does a missing -`results.json`. +A `rerun:` line says how many shots it reran on how many routes, and how many it reused. +`meta` describes the rerun (its clock, browser and time); a reused shot was taken under the +earlier run's. It must repeat the previous run: different origins, routes, widths, config +(`meta.config` hashes masks, `before` hooks and `locate` sources, hidden selectors, block +patterns, extra CSS and storage state file names) or renderdiff version exit 2, as does a +missing `results.json`. ## How it reads diff --git a/skills/renderdiff/reference.md b/skills/renderdiff/reference.md index 2a51669..aa75610 100644 --- a/skills/renderdiff/reference.md +++ b/skills/renderdiff/reference.md @@ -93,12 +93,14 @@ cookie script ran wedges the renderer). - **stdout is compact when piped** (what an agent sees). `--progress` prints a line per shot instead; `--no-progress` forces the summary on a terminal. - **Accepted differences** (`--accept ""`) go to `renderdiff.accepted.json` next to the - config (or in cwd), one entry per band, region or size difference, scoped by the run's routes - and widths. A match ignores `y` and tolerates noise, but a different value, a grown region or - an extra copy in the same spot still surfaces. A new change can make an accepted one re-read + config (or in cwd), one entry per band or region, scoped by the run's routes and widths. A + match ignores `y` and needs the same box, the same pixel hash and the same named elements, so + anything else drawn in that spot still surfaces. A shot whose widths differ, or whose heights + differ without aligning, is never accepted. A new change can make an accepted one re-read as different (repeated rows aligning differently): look, then accept again. - **`--rerun-failed`** keeps the out dir and reruns only what wasn't clean or accepted. Give it - the same origins, routes, widths and config as the run it continues, or it exits 2. + the same origins, routes, widths and config (hooks included) as the run it continues, or it + exits 2. - **The out dir is wiped at the start of every run** (except `--rerun-failed`). To keep a previous run's evidence next to a new one, pass `--out renderdiff-before` / `--out renderdiff-after`. A directory holding files renderdiff didn't write is refused (exit 2). diff --git a/src/accept.ts b/src/accept.ts index 0f75c52..edce758 100644 --- a/src/accept.ts +++ b/src/accept.ts @@ -7,19 +7,20 @@ import type { ComparedResult, RouteResult } from "./report.ts"; export const ACCEPTED_FILE = "renderdiff.accepted.json"; /** - * One change, without its `y`: a change above it moves it down the page without changing it. - * `elements` and `thumb` tell a new change in the same spot from the accepted one. + * One band or region, without its `y`: a change above it moves it down the page without + * changing it. `hash` and `elements` tell a new change in the same spot from the accepted one. */ export interface Fingerprint { - kind: "band" | "region" | "size"; + kind: "band" | "region"; side?: "left" | "right"; - /** A region's; a band's width is in its thumb, and `size` holds the width and height deltas. */ + /** A region's box; a band's rows are in its hash. */ x?: number; - w: number; + w?: number; h: number; /** The `dom` of each element under it, sorted; absent when they weren't looked at. */ elements?: string[]; - thumb?: string; + /** Both sides' pixels under it, 4 bits a channel. */ + hash?: string; } /** A difference recorded as already there or intended, so later runs report it as accepted. */ @@ -31,40 +32,39 @@ export interface Accepted { fingerprint: Fingerprint; } -type Change = { fp: Fingerprint; target: Region | Band | null }; +type Change = { fp: Fingerprint; target: Region | Band }; -/** Each band and region, plus the size difference, which padding hides from both. */ function changes(r: ComparedResult): Change[] { const els = (g: Region | Band) => g.elements && { elements: g.elements.map((e) => fmtElements([e], Infinity)).sort() }; return [ ...(r.bands ?? []).map((b) => ({ - fp: { kind: "band" as const, side: b.side, w: 0, h: b.h, ...els(b), thumb: b.thumb }, + fp: { kind: "band" as const, side: b.side, h: b.h, ...els(b), hash: b.hash }, target: b, })), ...r.regions.map((g) => ({ - fp: { kind: "region" as const, x: g.x, w: g.w, h: g.h, ...els(g), thumb: g.thumb }, + fp: { kind: "region" as const, x: g.x, w: g.w, h: g.h, ...els(g), hash: g.hash }, target: g, })), - ...(r.heightDelta || r.widthDelta - ? [{ fp: { kind: "size" as const, w: r.widthDelta, h: r.heightDelta }, target: null }] - : []), ]; } -/** A cell may drift by one step a channel: noise, not a change. */ -const near = (a = "", b = "") => - a.length === b.length && - [...a].every((c, i) => Math.abs(parseInt(c, 16) - parseInt(b[i]!, 16)) <= 1); - +/** `a` is the accepted entry: elements it recorded must be named again, not merely unprobed. */ const same = (a: Fingerprint, b: Fingerprint) => a.kind === b.kind && a.side === b.side && a.x === b.x && a.w === b.w && a.h === b.h && - (!a.elements || !b.elements || a.elements.join("\n") === b.elements.join("\n")) && - near(a.thumb, b.thumb); + a.hash === b.hash && + (!a.elements || a.elements.join("\n") === b.elements?.join("\n")); + +/** + * Padding no band or region covers: columns past the narrower page, or rows past the shorter + * one when they didn't align. A shot with any, or with unstable pixels, is never accepted. + */ +const acceptable = (r: ComparedResult) => + r.unstablePixels === 0 && r.widthDelta === 0 && (r.bands !== null || r.heightDelta === 0); export function readAccepted(file: string): Accepted[] { if (!fs.existsSync(file)) return []; @@ -89,8 +89,8 @@ export function readAccepted(file: string): Accepted[] { /** * Mark each change an accepted entry matches, one entry per change, and a result `accepted` - * when every change matched and nothing went unmeasured. Returns the changes nothing matched, - * and the entries for a measured route and width that matched nothing. + * when every change matched and it is `acceptable`. Returns the changes of acceptable results + * nothing matched, and the entries for a measured route and width that matched nothing. */ export function applyAccepted(results: RouteResult[], list: Accepted[]) { const used = new Set(); @@ -102,12 +102,16 @@ export function applyAccepted(results: RouteResult[], list: Accepted[]) { let missed = 0; for (const { fp, target } of cs) { const hit = mine.find((a) => !used.has(a) && same(a.fingerprint, fp)); - if (hit) used.add(hit); - else missed = unmatched.push({ route: name(r), width: r.width, fingerprint: fp }); - if (target && hit) target.accepted = hit.reason; - else if (target) delete target.accepted; + if (hit) { + used.add(hit); + target.accepted = hit.reason; + } else { + delete target.accepted; + missed++; + if (acceptable(r)) unmatched.push({ route: name(r), width: r.width, fingerprint: fp }); + } } - r.accepted = cs.length > 0 && !missed && r.unstablePixels === 0; + r.accepted = cs.length > 0 && !missed && acceptable(r); } const measured = new Set( results.filter((r) => r.status === "compared").map((r) => `${name(r)}@${r.width}`), @@ -116,8 +120,25 @@ export function applyAccepted(results: RouteResult[], list: Accepted[]) { return { unmatched, stale }; } -/** One entry per line, so a diff of the file reads as the differences it adds and drops. */ -export function writeAccepted(file: string, list: Accepted[]) { +const key = (a: Accepted) => JSON.stringify([a.route, a.width, a.reason, a.fingerprint]); + +/** + * Drop `stale` and add `added` to the file as it is now, so a concurrent `--accept` isn't lost, + * sorted by route then width, through a rename so a crash never leaves it half written. + * Returns false, writing nothing, when there is nothing to change. + */ +export function updateAccepted(file: string, stale: Accepted[], added: Accepted[]): boolean { + if (!stale.length && !added.length) return false; + const drop = new Set(stale.map(key)); + const kept = readAccepted(file).filter((a) => !drop.has(key(a))); + const have = new Set(kept.map(key)); + const list = [...kept, ...added.filter((a) => !have.has(key(a)))].sort((a, b) => + a.route < b.route ? -1 : a.route > b.route ? 1 : a.width - b.width, + ); + // One entry per line, so a diff of the file reads as the differences it adds and drops. const rows = list.map((a) => ` ${JSON.stringify(a)}`).join(",\n"); - fs.writeFileSync(file, rows ? `[\n${rows}\n]\n` : "[]\n"); + const tmp = `${file}.${process.pid}.tmp`; + fs.writeFileSync(tmp, rows ? `[\n${rows}\n]\n` : "[]\n"); + fs.renameSync(tmp, file); + return true; } diff --git a/src/compare.ts b/src/compare.ts index b6510b9..3a696c0 100644 --- a/src/compare.ts +++ b/src/compare.ts @@ -29,8 +29,8 @@ export interface Region { pixels: number; /** The elements under it; absent when not looked at. */ elements?: ElementDiff[]; - /** A coarse picture of the change on each side, for matching it against accepted ones. */ - thumb?: string; + /** Both sides' pixels under it, hashed, for matching it against accepted ones. */ + hash?: string; /** The reason of the accepted difference it matched. */ accepted?: string; } @@ -42,8 +42,8 @@ export interface Band { h: number; /** The elements in its rows; absent when not looked at. */ elements?: ElementDiff[]; - /** A coarse picture of its rows, for matching it against accepted ones. */ - thumb?: string; + /** Its rows' pixels, hashed, for matching it against accepted ones. */ + hash?: string; /** The reason of the accepted difference it matched. */ accepted?: string; } @@ -161,7 +161,7 @@ export async function compare(left: Shot, right: Shot): Promise { target: found[i]!, })), ]; - for (const s of spots) s.target.thumb = thumb(l, w, s.left) + thumb(r, w, s.right); + for (const s of spots) s.target.hash = pixelHash(l, r, w, [s.left, s.right]); // Padding is magenta against real content, so a height difference lands in // `diffPixels` and drags the score down. Intended: a page 400px shorter than the // one it's compared against has not reached parity. `heightDelta` names the cause. @@ -473,34 +473,26 @@ function inPlace(p: ReturnType, ha: Uint32Array, hb: Uint32Array) return { bands, at, rows }; } -const THUMB = 4; - /** - * Mean colour of each cell of a 4×4 grid over the box, 4 bits a channel, as hex: a picture of - * the change that survives sub-threshold noise but not a recolour. "" for an insertion point. + * FNV-1a over every pixel of the change on both sides, 4 bits a channel, as hex: shade jitter + * under one step survives, any other change to what is drawn there doesn't. */ -function thumb(raw: Buffer, stride: number, b: Box): string { - if (!b.w || !b.h) return ""; - let out = ""; - const edge = (i: number, n: number) => Math.floor((i * n) / THUMB); - for (let gy = 0; gy < THUMB; gy++) { - for (let gx = 0; gx < THUMB; gx++) { - const [x0, y0] = [b.x + edge(gx, b.w), b.y + edge(gy, b.h)]; - const [x1, y1] = [ - Math.max(x0 + 1, b.x + edge(gx + 1, b.w)), - Math.max(y0 + 1, b.y + edge(gy + 1, b.h)), - ]; - const sum = [0, 0, 0]; - for (let y = y0; y < y1; y++) { - for (let x = x0, o = (y * stride + x) * 4; x < x1; x++, o += 4) { - for (let c = 0; c < 3; c++) sum[c]! += raw[o + c]!; - } +function pixelHash(l: Buffer, r: Buffer, stride: number, boxes: [Box, Box]): string { + let hash = 0x811c9dc5; + for (const [raw, b] of [ + [l, boxes[0]], + [r, boxes[1]], + ] as const) { + for (let y = b.y; y < b.y + b.h; y++) { + for (let x = b.x, o = (y * stride + x) * 4; x < b.x + b.w; x++, o += 4) { + hash = Math.imul( + hash ^ ((raw[o]! >> 4) | (raw[o + 1]! & 0xf0) | ((raw[o + 2]! >> 4) << 8)), + 16777619, + ); } - const n = (x1 - x0) * (y1 - y0); - for (const v of sum) out += (Math.round(v / n) >> 4).toString(16); } } - return out; + return (hash >>> 0).toString(16).padStart(8, "0"); } /** Columns holding anything but each row's x=0 colour, over rows `y0..y1`; the full width if none. */ diff --git a/src/report.ts b/src/report.ts index 2868be8..ca4b8bf 100644 --- a/src/report.ts +++ b/src/report.ts @@ -41,7 +41,7 @@ export interface ComparedResult extends BaseResult { images: { left: string; right: string; diff: string }; /** At most three: bands, tallest first, then the largest regions. Not one per region. */ crops: CropResult[]; - /** Every band, region and size difference matched an accepted one, and none went unmeasured. */ + /** Every band and region matched an accepted one, and no padding or unstable pixel went unmatched. */ accepted: boolean; } @@ -77,7 +77,10 @@ export interface RunMeta { right: string; widths: number[]; threshold: number | null; - /** The instant both sides' clocks start at on every load, ISO. */ + /** + * The instant both sides' clocks start at on every load of this run, ISO. A `--rerun-failed` + * shot reused from an earlier run was taken under that run's clock. + */ clock: string; blockUrlPatterns: string[]; hideSelectors: string[]; @@ -87,6 +90,8 @@ export interface RunMeta { browser: { name: string; version: string }; /** The storage state file's basename per side, or null. Never its contents. */ storageState: { left: string | null; right: string | null }; + /** A hash of everything that decides how a shot is taken; `--rerun-failed` needs it unchanged. */ + config: string; } export interface Summary { @@ -128,7 +133,8 @@ export function writeResults( stale: Accepted[], ): string { const compared = results.filter((r): r is ComparedResult => r.status === "compared"); - const { mean, worst } = compared.length ? stats(compared) : { mean: null, worst: null }; + const open = compared.filter((r) => !r.accepted); + const { mean, worst } = open.length ? stats(open) : { mean: null, worst: null }; const out: Results = { meta, results, @@ -166,13 +172,21 @@ const band = (score: number) => (score >= 0.995 ? "good" : score >= 0.97 ? "warn function section(rows: ComparedResult[]): string { if (!rows.length) return ""; - const { sorted, mean, worst } = stats(rows); + const { sorted } = stats(rows); + const open = rows.filter((r) => !r.accepted); + const agg = open.length ? stats(open) : null; return `
    -

    mean ${pct(mean)} · - worst ${pct(worst.score)} - (${esc(name(worst))} @${worst.width}) · ${sorted.length} comparison(s)

    +

    ${ + agg + ? `mean ${pct(agg.mean)} · + worst ${pct(agg.worst.score)} + (${esc(name(agg.worst))} @${agg.worst.width}) · ` + : "" + }${sorted.length} comparison(s)${ + rows.length - open.length ? `, ${rows.length - open.length} accepted` : "" + }

    ${sorted.map(card).join("\n")}
    `; } diff --git a/src/run.ts b/src/run.ts index a7701f9..65ca000 100644 --- a/src/run.ts +++ b/src/run.ts @@ -1,9 +1,10 @@ import { spawn } from "node:child_process"; +import { createHash } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; import { chromium, errors, type Browser, type Page } from "playwright"; -import { applyAccepted, readAccepted, writeAccepted } from "./accept.ts"; +import { applyAccepted, readAccepted, updateAccepted } from "./accept.ts"; import { cancelUnstable, capture, HookError, prepare, type Shot } from "./capture.ts"; import { compare, crops, type Band } from "./compare.ts"; import { locate, type ElementDiff } from "./locate.ts"; @@ -101,6 +102,20 @@ const reusable = (r: RouteResult) => const files = (r: RouteResult) => r.status === "compared" ? [...Object.values(r.images), ...r.crops.map((c) => c.path)] : []; +/** Everything that decides how a shot is taken, hook and `locate` sources included. */ +function configHash(cfg: ResolvedConfig, routes: Route[]): string { + const { threshold, outDir, concurrency, storageState, ...rest } = cfg; + const json = JSON.stringify( + { + ...rest, + storageState: [storageState.left, storageState.right].map((f) => f && path.basename(f)), + routes, + }, + (_, v) => (typeof v === "function" ? String(v) : v), + ); + return createHash("sha256").update(json).digest("hex").slice(0, 16); +} + /** The previous run's results when this run repeats it, else why it doesn't. */ function previous(file: string, now: Record): Results | string { if (!fs.existsSync(file)) return `no ${file} to rerun from; run without --rerun-failed first`; @@ -115,6 +130,12 @@ function previous(file: string, now: Record): Results | string routes: [...new Set(prev.results?.map(name))], }; for (const [key, value] of Object.entries(now)) { + if (key === "config" && value !== was[key]) { + return ( + `the config (masks, hooks, hidden selectors, blocked URLs, CSS, storage state) ` + + `differs from the one that wrote ${file}; run without --rerun-failed to start over` + ); + } if (JSON.stringify(value) !== JSON.stringify(was[key])) { return ( `${key} ${JSON.stringify(value)} differ from the previous run's ${JSON.stringify(was[key])} ` + @@ -154,11 +175,15 @@ export async function run({ console.error(`✗ ${e instanceof Error ? e.message : String(e)}`); return 2; } + const config = configHash(cfg, routes); const prev = rerun ? previous(path.join(cfg.outDir, "results.json"), { version: version(), - ...setup, + left: cfg.left, + right: cfg.right, + widths: cfg.widths, routes: names, + config, }) : null; if (typeof prev === "string") { @@ -347,14 +372,17 @@ export async function run({ let { unmatched, stale } = applyAccepted(results, accepted); if (accept !== null) { - writeAccepted(acceptedFile, [ - ...accepted.filter((a) => !stale.includes(a)), - ...unmatched.map((u) => ({ ...u, reason: accept })), - ]); - console.log( - `accepted ${unmatched.length} difference(s), pruned ${stale.length} stale — ${acceptedFile}`, - ); - ({ stale } = applyAccepted(results, readAccepted(acceptedFile))); + const added = unmatched.map(({ route, width, fingerprint }) => ({ + route, + width, + reason: accept, + fingerprint, + })); + const pruned = stale.length; + if (updateAccepted(acceptedFile, stale, added)) { + ({ stale } = applyAccepted(results, readAccepted(acceptedFile))); + } + console.log(`accepted ${added.length} difference(s), pruned ${pruned} stale — ${acceptedFile}`); } const { left, right, widths, ...rest } = setup; @@ -369,6 +397,7 @@ export async function run({ clock: new Date(now).toISOString(), ...rest, browser: browserMeta, + config, }; const reportPath = await writeReport(results, meta, cfg, stale); const resultsPath = writeResults(results, meta, cfg.outDir, stale); @@ -455,8 +484,9 @@ async function shoot( function summarize(results: RouteResult[], reportPath: string, resultsPath: string) { const compared = results.filter((r): r is ComparedResult => r.status === "compared"); - if (compared.length) { - const { sorted, mean, worst } = stats(compared); + const open = compared.filter((r) => !r.accepted); + if (open.length) { + const { sorted, mean, worst } = stats(open); console.log(`\nmean ${pct(mean)} worst ${pct(worst.score)} (${name(worst)} @${worst.width})`); for (const r of sorted.slice(0, 10)) { console.log( diff --git a/tests/visual/accepted.test.ts b/tests/visual/accepted.test.ts index e62dad7..03e967b 100644 --- a/tests/visual/accepted.test.ts +++ b/tests/visual/accepted.test.ts @@ -32,12 +32,12 @@ const args = (routes: string[], extra: string[] = [], widths = [1440]) => [ ...extra, ]; -/** A config in `dir` whose hook restyles the left side, so the accepted file is found beside it. */ -function leftCss(dir: string, css: string) { +/** A config in `dir` whose hook restyles one side, so the accepted file is found beside it. */ +function leftCss(dir: string, css: string, side = "left") { const file = path.join(dir, "hook.mjs"); fs.writeFileSync( file, - `export default { before: async (page, { side }) => { if (side === "left") await page.addStyleTag({ content: ${JSON.stringify(css)} }); } };\n`, + `export default { before: async (page, { side }) => { if (side === "${side}") await page.addStyleTag({ content: ${JSON.stringify(css)} }); } };\n`, ); return ["--config", file]; } @@ -54,7 +54,7 @@ describe("accepted differences", () => { cwd: dir, }); assert.equal(run.code, 0, context(run)); - assert.match(run.stdout, /^accepted 3 difference\(s\), pruned 0 stale — .*$/m, context(run)); + assert.match(run.stdout, /^accepted 2 difference\(s\), pruned 0 stale — .*$/m, context(run)); }); test("--accept records each change with its route, width, fingerprint and reason", () => { @@ -64,13 +64,12 @@ describe("accepted differences", () => { reason: string; fingerprint: { kind: string }; }[]; - // /color is one region; /nav is one band and the 24px it costs the page. + // /color is one region; /nav is one band. assert.deepEqual( list.map((a) => [a.route, a.width, a.fingerprint.kind, a.reason]), [ ["/color", 1440, "region", "known before the move"], ["/nav", 1440, "band", "known before the move"], - ["/nav", 1440, "size", "known before the move"], ], fs.readFileSync(file(), "utf8"), ); @@ -124,6 +123,23 @@ describe("accepted differences", () => { assert.equal(s.verdict.differ, 1); }); + test("the accepted band redrawn at the same size and tone is not the accepted one", async () => { + // Overlining the right-only link instead of underlining it changes no compared property, no text and no size, and + // barely moves the band's mean colour: only its pixels say it is new. + const run = await runCli( + args(["/nav"], leftCss(dir, 'a[href="#pricing"] { text-decoration: overline }', "right")), + { cwd: dir }, + ); + const r = result(results(run.outDir), "/nav", 1440); + assert.deepEqual( + r.bands?.map((b) => [b.side, b.h]), + [["right", 24]], + context(run), + ); + assert.equal(r.accepted, false, context(run)); + assert.deepEqual(parseSummary(run.stdout).accepted, []); + }); + test("an accepted difference no longer seen is listed as stale; unchecked routes aren't", async () => { const run = await runCli(args(["/color"], leftCss(dir, `${THIRD} { background: #c62828 }`)), { cwd: dir, @@ -144,13 +160,17 @@ describe("accepted differences", () => { const hook = leftCss(dir2, `${THIRD} { background: #c62828 }`); const run = await runCli(args(["/color"], [...hook, "--accept", "fixed"]), { cwd: dir2 }); assert.match(run.stdout, /^accepted 0 difference\(s\), pruned 1 stale/m, context(run)); - const list = JSON.parse( - fs.readFileSync(path.join(dir2, "renderdiff.accepted.json"), "utf8"), - ) as { route: string }[]; + const at = path.join(dir2, "renderdiff.accepted.json"); + const list = JSON.parse(fs.readFileSync(at, "utf8")) as { route: string }[]; assert.deepEqual( list.map((a) => a.route), - ["/nav", "/nav"], + ["/nav"], ); + const mtime = fs.statSync(at).mtimeMs; + const again = await runCli(args(["/color"], [...hook, "--accept", "fixed"]), { cwd: dir2 }); + assert.match(again.stdout, /^accepted 0 difference\(s\), pruned 0 stale/m, context(again)); + assert.equal(fs.statSync(at).mtimeMs, mtime, "nothing to change, so the file isn't rewritten"); + assert.deepEqual(fs.readdirSync(dir2).sort(), ["hook.mjs", "renderdiff.accepted.json"]); }); }); @@ -210,6 +230,10 @@ describe("--rerun-failed", () => { [...args(["/color"], ["--rerun-failed"]), "--right", site.left], /--rerun-failed: right .* differ/, ], + [ + args(["/color"], ["--rerun-failed", ...leftCss(tempDir(), "h1 { color: red }")]), + /--rerun-failed: the config \(masks, hooks/, + ], ] as const) { const r = await again([...a]); assert.equal(r.code, 2, context(r)); diff --git a/tests/visual/controls.test.ts b/tests/visual/controls.test.ts index 6365a4a..90d4456 100644 --- a/tests/visual/controls.test.ts +++ b/tests/visual/controls.test.ts @@ -202,7 +202,7 @@ for (const route of ["/color", "/nav"]) { assert.equal(r.widthDelta, c.widthDelta); assert.equal(r.unstablePixels, c.unstablePixels); assert.deepEqual( - r.regions.slice(0, 5).map(({ elements, thumb, ...g }) => g), + r.regions.slice(0, 5).map(({ elements, hash, ...g }) => g), c.regions.map(({ dom, ...g }) => g), ); assert.deepEqual(r.text, { added: c.added, removed: c.removed });