diff --git a/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql b/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql new file mode 100644 index 00000000..62cbaa8d --- /dev/null +++ b/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql @@ -0,0 +1,6 @@ +-- AlterTable +-- Nullable with no default, so this is a catalogue-only change on Postgres 11+: +-- no table rewrite and no long lock, which matters on a TestRun table holding +-- a hundred builds' worth of runs. Existing rows keep NULL and fall back to +-- computing the signature on demand. +ALTER TABLE "TestRun" ADD COLUMN "changeSignature" TEXT; diff --git a/prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql b/prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql new file mode 100644 index 00000000..f6e4cb67 --- /dev/null +++ b/prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql @@ -0,0 +1,6 @@ +-- AlterTable +-- Nullable with no default, so this is a catalogue-only change on Postgres 11+: +-- no table rewrite and no long lock. Existing rows keep NULL and the UI falls +-- back to the full-size image for them. +ALTER TABLE "TestRun" ADD COLUMN "imageThumbnailName" TEXT, + ADD COLUMN "diffThumbnailName" TEXT; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 4835aeae..51bb7e87 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -1,5 +1,5 @@ generator client { - provider = "prisma-client-js" + provider = "prisma-client-js" binaryTargets = ["native", "debian-openssl-3.0.x", "linux-arm64-openssl-3.0.x"] } @@ -72,6 +72,20 @@ model TestRun { ignoreAreas String @default("[]") tempIgnoreAreas String @default("[]") vlmDescription String? + // Position-independent colour signature of this run's change, as JSON, so the + // variations dialog can group a screen's locales without fetching and + // decoding every sibling's screenshots at review time. Written once, next to + // the diff that already decoded them. Null on runs that predate this, that + // have no diff, or that were compared by something other than pixelmatch — + // those fall back to computing it on demand. + changeSignature String? + // Small copies of the checkpoint and of the diff, made at ingest from the + // pixels the comparison had already decoded. The card grids draw pictures a + // hundred-odd pixels wide, and pulling the full-size files for that cost + // megabytes per screen. Null on runs ingested before this, which fall back to + // the full-size file. + imageThumbnailName String? + diffThumbnailName String? baseline Baseline? build Build @relation(fields: [buildId], references: [id]) project Project? @relation(fields: [projectId], references: [id]) diff --git a/src/_data_/index.ts b/src/_data_/index.ts index fcbb4df9..9b0c46ed 100644 --- a/src/_data_/index.ts +++ b/src/_data_/index.ts @@ -99,6 +99,9 @@ export const generateTestRun = (testRun?: Partial): TestRun => { branchName: 'develop', merge: false, vlmDescription: null, + changeSignature: null, + imageThumbnailName: null, + diffThumbnailName: null, ...testRun, }; }; diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts new file mode 100644 index 00000000..dfc82e3a --- /dev/null +++ b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts @@ -0,0 +1,136 @@ +import { PNG } from 'pngjs'; +import { computePixelmatchDiff } from './pixelmatch.core'; +import { computeChangeSignature } from './signature.core'; +import { THUMBNAIL_MAX_DIMENSION } from './thumbnail.core'; + +const WIDTH = 60; +const HEIGHT = 60; + +const png = (paint: (set: (x: number, y: number, rgb: [number, number, number]) => void) => void): Buffer => { + const image = new PNG({ width: WIDTH, height: HEIGHT }); + for (let i = 0; i < WIDTH * HEIGHT; i++) { + image.data[i * 4] = 255; + image.data[i * 4 + 1] = 255; + image.data[i * 4 + 2] = 255; + image.data[i * 4 + 3] = 255; + } + paint((x, y, [r, g, b]) => { + const index = (y * WIDTH + x) * 4; + image.data[index] = r; + image.data[index + 1] = g; + image.data[index + 2] = b; + }); + return PNG.sync.write(image); +}; + +const block = (left: number, top: number, rgb: [number, number, number]): Buffer => + png((set) => { + for (let y = top; y < top + 12; y++) { + for (let x = left; x < left + 12; x++) { + set(x, y, rgb); + } + } + }); + +const blank = png(() => undefined); +const RED: [number, number, number] = [255, 0, 0]; + +const diffJob = ( + baseline: Buffer, + image: Buffer, + withSignature: boolean, + withThumbnails = false, + { saveDiff = true, diffTolerancePercent = 0 } = {} +) => + computePixelmatchDiff({ + kind: 'diff', + baseline, + image, + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + allowDiffDimensions: false, + diffTolerancePercent, + saveDiff, + withSignature, + withThumbnails, + }); + +describe('computePixelmatchDiff with a signature', () => { + // The whole point of computing it here is to reuse the decode the diff + // already paid for. If the fused version answered differently from the + // standalone one, stored signatures would stop matching computed ones and + // variations would silently stop grouping. + it('gives the same signature the standalone job would', () => { + const image = block(10, 10, RED); + + const fused = diffJob(blank, image, true).signature; + const standalone = computeChangeSignature({ + kind: 'signature', + baseline: blank, + image, + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + }).signature; + + expect(fused).toEqual(standalone); + expect(fused).not.toBeNull(); + }); + + it('leaves the signature out when it was not asked for', () => { + expect(diffJob(blank, block(10, 10, RED), false).signature).toBeUndefined(); + }); + + it('has no signature when the screenshots are identical', () => { + expect(diffJob(blank, blank, true).signature).toBeUndefined(); + }); + + it('still reports the diff it was asked for', () => { + const result = diffJob(blank, block(10, 10, RED), true); + + expect(result.equal).toBe(false); + expect(result.pixelMisMatchCount).toBe(144); + }); +}); + +describe('computePixelmatchDiff with thumbnails', () => { + // Made here from the pixels the diff has already decoded and already drawn, + // so a thumbnail costs a resize rather than a second read of the screenshot. + it('returns a small picture of both the checkpoint and the diff', () => { + const result = diffJob(blank, block(10, 10, RED), false, true); + + const image = PNG.sync.read(Buffer.from(result.imageThumbnail)); + const diff = PNG.sync.read(Buffer.from(result.diffThumbnail)); + expect(Math.max(image.width, image.height)).toBeLessThanOrEqual(THUMBNAIL_MAX_DIMENSION); + expect(Math.max(diff.width, diff.height)).toBeLessThanOrEqual(THUMBNAIL_MAX_DIMENSION); + }); + + it('makes none when they were not asked for', () => { + const result = diffJob(blank, block(10, 10, RED), false, false); + + expect(result.imageThumbnail).toBeUndefined(); + expect(result.diffThumbnail).toBeUndefined(); + }); + + // A build is mostly runs that pass. Resizing two images for each of those, + // when the diff they belong to is thrown away seconds later, is work nobody + // ever sees the result of. + it('makes none when the change is under tolerance', () => { + const result = diffJob(blank, block(10, 10, RED), false, true, { diffTolerancePercent: 90 }); + + expect(result.imageThumbnail).toBeUndefined(); + expect(result.diffThumbnail).toBeUndefined(); + }); + + it('makes none when the caller is not keeping the diff', () => { + const result = diffJob(blank, block(10, 10, RED), false, true, { saveDiff: false }); + + expect(result.imageThumbnail).toBeUndefined(); + }); + + // nothing to show and nothing to shrink + it('makes none when the screenshots are identical', () => { + expect(diffJob(blank, blank, false, true).imageThumbnail).toBeUndefined(); + }); +}); diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts index 47bbc71e..a8e248cf 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts @@ -2,6 +2,8 @@ import { PNG } from 'pngjs'; import Pixelmatch from 'pixelmatch'; import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto'; import { applyIgnoreAreas, scaleImageToSize } from '../../utils'; +import { signatureOfDecoded } from './signature.core'; +import { encodeThumbnail } from './thumbnail.core'; /** * CPU-bound part of the pixelmatch comparison, extracted so it can run inside @@ -20,6 +22,19 @@ export interface PixelmatchJobInput { allowDiffDimensions: boolean; diffTolerancePercent: number; saveDiff: boolean; + /** + * Also produce the change signature the variations dialog matches on. Free + * here next to the diff — the images are decoded already — and computed once + * at ingest so reviewing never has to fetch and decode a build's screenshots + * again just to group them. + */ + withSignature?: boolean; + /** + * Also return small PNGs of the checkpoint and of the diff. The grids draw + * these a hundred-odd pixels wide; sending the full-size files and letting + * CSS shrink them is what made opening the variations dialog cost megabytes. + */ + withThumbnails?: boolean; } export interface PixelmatchJobOutput { @@ -28,6 +43,13 @@ export interface PixelmatchJobOutput { pixelMisMatchCount?: number; diffPercent?: number; diffBuffer?: Buffer | Uint8Array; + // absent when not asked for, when the screenshots are identical, or when + // their dimensions differ and there is nothing meaningful to sign + signature?: number[]; + // absent when not asked for, or when the screenshots matched and there is + // nothing to show + imageThumbnail?: Buffer | Uint8Array; + diffThumbnail?: Buffer | Uint8Array; } // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. @@ -69,10 +91,38 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO }); const diffPercent = (pixelMisMatchCount * 100) / (scaledImage.width * scaledImage.height); + // A build is mostly runs that pass, so anything produced for a diff that is + // about to be thrown away is work nobody ever sees the result of. + const keepingTheDiff = diffPercent > input.diffTolerancePercent && input.saveDiff; + let diffBuffer: Buffer; - if (diffPercent > input.diffTolerancePercent && input.saveDiff) { + if (keepingTheDiff) { diffBuffer = PNG.sync.write(diff); } - return { equal: false, isSameDimension, pixelMisMatchCount, diffPercent, diffBuffer }; + // Same source images, same ignore areas, same threshold as the standalone + // signature job — sharing signatureOfDecoded is what keeps the two answers + // identical, so a stored signature still matches one computed on the fly. + const signature = + input.withSignature && isSameDimension + ? signatureOfDecoded(baselineIgnored, imageIgnored, { + threshold: input.threshold, + includeAA: input.includeAA, + }) + : null; + + const thumbnails = + input.withThumbnails && keepingTheDiff + ? { imageThumbnail: encodeThumbnail(imageIgnored), diffThumbnail: encodeThumbnail(diff) } + : {}; + + return { + equal: false, + isSameDimension, + pixelMisMatchCount, + diffPercent, + diffBuffer, + ...(signature ? { signature } : {}), + ...thumbnails, + }; } diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts index 1696b97d..6c5de4c5 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts @@ -169,7 +169,9 @@ describe('getDiff', () => { threshold: 0.1, } ); - expect(saveImageMock).toHaveBeenCalledTimes(1); + // the full-size diff first, then the two thumbnails made beside it + expect(saveImageMock.mock.calls[0][0]).toBe('diff'); + expect(saveImageMock).toHaveBeenCalledTimes(3); expect(result).toStrictEqual({ status: TestStatus.unresolved, diffName, @@ -247,7 +249,9 @@ describe('getDiff', () => { DEFAULT_CONFIG ); - expect(saveImageMock).toHaveBeenCalledTimes(1); + // the full-size diff first, then the two thumbnails made beside it + expect(saveImageMock.mock.calls[0][0]).toBe('diff'); + expect(saveImageMock).toHaveBeenCalledTimes(3); expect(result).toStrictEqual({ status: TestStatus.unresolved, diffName, @@ -257,3 +261,129 @@ describe('getDiff', () => { }); }); }); + +describe('change signature', () => { + // Correctness of the signature itself is pinned in pixelmatch.core.spec + // against the real pixelmatch; Pixelmatch is mocked in this file, so what is + // worth testing here is only the plumbing — that the service asks for one and + // carries what comes back into the result. + const initWithPool = async (output: Record) => { + const run = jest.fn().mockResolvedValue(output); + const module: TestingModule = await Test.createTestingModule({ + providers: [ + PixelmatchService, + { provide: DiffWorkerPool, useValue: { run } }, + { + provide: StaticService, + useValue: { + getImageBuffer: jest.fn().mockResolvedValue(Buffer.from('png')), + saveImage: jest.fn(), + deleteImage: jest.fn(), + }, + }, + ], + }).compile(); + return { service: module.get(PixelmatchService), run }; + }; + + const compare = (service: PixelmatchService, saveDiffAsFile = false) => + service.getDiff( + { + baseline: 'baseline.png', + image: 'image.png', + ignoreAreas: [], + diffTollerancePercent: 0, + saveDiffAsFile, + }, + DEFAULT_CONFIG + ); + + // Asked for on every comparison, not only when the project has bulk approve + // switched on: computing it lazily would leave every run ingested before the + // flag was turned on without one. + it('asks for a signature and carries it into the result', async () => { + const { service, run } = await initWithPool({ + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + signature: [0.25, 0.75], + }); + + const result = await compare(service); + + expect(run).toHaveBeenCalledWith(expect.objectContaining({ kind: 'diff', withSignature: true })); + // stamped with the settings it was computed under, so a stored one can be + // discarded when the project's config moves on + expect(result.changeSignature).toEqual({ + threshold: DEFAULT_CONFIG.threshold, + includeAA: DEFAULT_CONFIG.ignoreAntialiasing, + signature: [0.25, 0.75], + }); + }); + + const initSaving = async (output: Record) => { + const saveImage = jest + .fn() + .mockResolvedValueOnce('diff.png') + .mockResolvedValueOnce('image.thumb.png') + .mockResolvedValueOnce('diff.thumb.png'); + const module: TestingModule = await Test.createTestingModule({ + providers: [ + PixelmatchService, + { provide: DiffWorkerPool, useValue: { run: jest.fn().mockResolvedValue(output) } }, + { + provide: StaticService, + useValue: { + getImageBuffer: jest.fn().mockResolvedValue(Buffer.from('png')), + saveImage, + deleteImage: jest.fn(), + }, + }, + ], + }).compile(); + return { service: module.get(PixelmatchService), saveImage }; + }; + + const overTolerance = { + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + imageThumbnail: Buffer.from('small image'), + diffThumbnail: Buffer.from('small diff'), + }; + + it('saves the thumbnails and reports the names they went under', async () => { + const { service } = await initSaving({ ...overTolerance, diffBuffer: Buffer.from('the diff') }); + + const result = await compare(service, true); + + expect(result.imageThumbnailName).toBe('image.thumb.png'); + expect(result.diffThumbnailName).toBe('diff.thumb.png'); + }); + + // shouldAutoApprove compares against past baselines with saveDiffAsFile off + // and throws the result away. Storing thumbnails for those comparisons would + // leave two objects per attempt that nothing ever references or deletes. + it('stores nothing when the caller is not keeping the diff', async () => { + const { service, saveImage } = await initSaving(overTolerance); + + const result = await compare(service, false); + + expect(saveImage).not.toHaveBeenCalled(); + expect(result.imageThumbnailName).toBeUndefined(); + expect(result.diffThumbnailName).toBeUndefined(); + }); + + it('leaves it out when the comparison produced none', async () => { + const { service } = await initWithPool({ + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + }); + + expect((await compare(service)).changeSignature).toBeUndefined(); + }); +}); diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts index 0cb9c800..1e22b3e7 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -45,6 +45,17 @@ export class PixelmatchService implements ImageComparator { allowDiffDimensions: config.allowDiffDimensions, diffTolerancePercent: data.diffTollerancePercent, saveDiff: data.saveDiffAsFile, + // Asked for on every comparison, not only when the project has bulk + // approve of variations switched on: the pass costs little beside the + // full-size diff that has already been paid for, and computing it lazily + // would leave every run ingested before the flag was turned on without + // one — which is exactly the build someone then tries to review. + withSignature: true, + // Only when the caller is keeping the diff. shouldAutoApprove compares + // against past baselines with this off and throws the result away — + // making thumbnails for those would burn the CPU and, worse, store two + // objects per attempt that nothing ever references or deletes. + withThumbnails: data.saveDiffAsFile, }); if (output.equal) { @@ -59,12 +70,39 @@ export class PixelmatchService implements ImageComparator { isSameDimension: output.isSameDimension, pixelMisMatchCount: output.pixelMisMatchCount, diffPercent: output.diffPercent, + ...(output.signature + ? { + changeSignature: { + threshold: config.threshold, + includeAA: config.ignoreAntialiasing, + signature: output.signature, + }, + } + : {}), }; if (result.diffPercent > data.diffTollerancePercent) { if (output.diffBuffer) { result.diffName = await this.staticService.saveImage('diff', Buffer.from(output.diffBuffer)); } + // Kept to the same condition as the diff itself: a thumbnail of a diff + // that was never saved would point at a file that does not exist. Stored + // as ordinary images so they follow the same storage, the same deletion + // and the same URLs as everything else, with no naming convention for + // the UI to guess at. + // tied to the diff actually landing, not merely to being over tolerance + if (result.diffName && output.imageThumbnail && output.diffThumbnail) { + const [imageThumbnailName, diffThumbnailName] = await Promise.all([ + this.staticService.saveImage('screenshot', Buffer.from(output.imageThumbnail)), + this.staticService.saveImage('diff', Buffer.from(output.diffThumbnail)), + ]); + // only when both came back: a result carrying one half of a pair, or a + // key set to undefined, is worse than none at all + if (imageThumbnailName && diffThumbnailName) { + result.imageThumbnailName = imageThumbnailName; + result.diffThumbnailName = diffThumbnailName; + } + } result.status = TestStatus.unresolved; } else { result.status = TestStatus.ok; diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts index 9909354c..020d37a7 100644 --- a/src/compare/libs/pixelmatch/signature.core.ts +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -1,7 +1,8 @@ import { PNG } from 'pngjs'; import Pixelmatch from 'pixelmatch'; import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto'; -import { applyIgnoreAreas } from '../../utils'; +import { RawImage } from '../../utils'; +import { applyIgnoreAreas, downscale } from '../../utils'; /** * CPU-bound part of "do these two screens carry the same change?": PNG decode, @@ -30,6 +31,12 @@ export interface SignatureJobOutput { // COLOR_BUCKETS_PER_CHANNEL^3 histogram buckets. const COLOR_BUCKETS_PER_CHANNEL = 4; +// How many buckets a signature has. Exported so a stored one can be checked +// against the shape this build produces: comparing signatures of two different +// lengths yields a similarity score computed over undefined entries — a number, +// and a meaningless one. +export const SIGNATURE_LENGTH = COLOR_BUCKETS_PER_CHANNEL ** 3; + // Longest side (px) images are downscaled to before computing the color // signature — keeps the histogram representative while cutting pixelmatch cost. const SIGNATURE_MAX_DIMENSION = 500; @@ -38,42 +45,11 @@ const SIGNATURE_MAX_DIMENSION = 500; // cosine similarity is at least this value. export const SIGNATURE_SIMILARITY_THRESHOLD = 0.9; -interface RawImage { - data: Buffer; - width: number; - height: number; -} - // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. function toBuffer(data: Buffer | Uint8Array): Buffer { return Buffer.isBuffer(data) ? data : Buffer.from(data.buffer, data.byteOffset, data.byteLength); } -// Nearest-neighbour downscale so the longest side is at most maxDimension. -// Returns the original when already small enough. -function downscale(source: RawImage, maxDimension: number): RawImage { - const scale = maxDimension / Math.max(source.width, source.height); - if (scale >= 1) { - return source; - } - const width = Math.max(1, Math.round(source.width * scale)); - const height = Math.max(1, Math.round(source.height * scale)); - const data: Buffer = Buffer.alloc(width * height * 4); - for (let y = 0; y < height; y++) { - const sourceY = Math.min(source.height - 1, Math.floor(y / scale)); - for (let x = 0; x < width; x++) { - const sourceX = Math.min(source.width - 1, Math.floor(x / scale)); - const sourceIndex = (sourceY * source.width + sourceX) * 4; - const targetIndex = (y * width + x) * 4; - data[targetIndex] = source.data[sourceIndex]; - data[targetIndex + 1] = source.data[sourceIndex + 1]; - data[targetIndex + 2] = source.data[sourceIndex + 2]; - data[targetIndex + 3] = source.data[sourceIndex + 3]; - } - } - return { data, width, height }; -} - /** * Position-independent signature of a change: a normalized histogram of the * colors that the changed pixels take in the new image. Because it ignores @@ -106,17 +82,31 @@ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOu applyIgnoreAreas(baselineImage, input.ignoreAreas); applyIgnoreAreas(checkpointImage, input.ignoreAreas); + return { signature: signatureOfDecoded(baselineImage, checkpointImage, input) }; +} + +/** + * The signature of a pair that is already decoded, of equal size, and with its + * ignore areas blanked. Split out so the diff can produce a signature from the + * images it has just decoded rather than paying for a second decode — the two + * must not drift apart, so there is only ever one implementation. + */ +export function signatureOfDecoded( + baselineImage: RawImage, + checkpointImage: RawImage, + options: { threshold: number; includeAA: boolean } +): number[] | null { const baseline = downscale(baselineImage, SIGNATURE_MAX_DIMENSION); const image = downscale(checkpointImage, SIGNATURE_MAX_DIMENSION); const { width, height } = baseline; const mask = new PNG({ width, height }); const changedPixels = Pixelmatch(baseline.data, image.data, mask.data, width, height, { - threshold: input.threshold, - includeAA: input.includeAA, + threshold: options.threshold, + includeAA: options.includeAA, diffMask: true, }); if (changedPixels === 0) { - return { signature: null }; + return null; } const bucketSize = 256 / COLOR_BUCKETS_PER_CHANNEL; @@ -133,9 +123,9 @@ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOu const total = histogram.reduce((sum, value) => sum + value, 0); if (total === 0) { - return { signature: null }; + return null; } - return { signature: histogram.map((value) => value / total) }; + return histogram.map((value) => value / total); } function cosineSimilarity(a: number[], b: number[]): number { diff --git a/src/compare/libs/pixelmatch/thumbnail.core.spec.ts b/src/compare/libs/pixelmatch/thumbnail.core.spec.ts new file mode 100644 index 00000000..4dcfa7a8 --- /dev/null +++ b/src/compare/libs/pixelmatch/thumbnail.core.spec.ts @@ -0,0 +1,46 @@ +import { PNG } from 'pngjs'; +import { encodeThumbnail, THUMBNAIL_MAX_DIMENSION } from './thumbnail.core'; + +const solid = (width: number, height: number): PNG => { + const png = new PNG({ width, height }); + for (let i = 0; i < width * height; i++) { + png.data[i * 4] = 200; + png.data[i * 4 + 1] = 100; + png.data[i * 4 + 2] = 50; + png.data[i * 4 + 3] = 255; + } + return png; +}; + +describe('encodeThumbnail', () => { + // The grids draw these a hundred-odd pixels wide. Sending the full + // screenshot and letting CSS shrink it cost ~7 MB and 68 requests every time + // the variations dialog opened. + it('brings a screenshot down to the thumbnail size', () => { + const thumbnail = PNG.sync.read(encodeThumbnail(solid(1284, 2778))); + + expect(Math.max(thumbnail.width, thumbnail.height)).toBe(THUMBNAIL_MAX_DIMENSION); + }); + + it('keeps the proportions of the screenshot it came from', () => { + const source = solid(1284, 2778); + + const thumbnail = PNG.sync.read(encodeThumbnail(source)); + + expect(thumbnail.width / thumbnail.height).toBeCloseTo(source.width / source.height, 2); + }); + + it('is a fraction of the bytes of the original', () => { + const source = solid(1284, 2778); + + expect(encodeThumbnail(source).length).toBeLessThan(PNG.sync.write(source).length / 10); + }); + + // a small screenshot re-encoded larger would be worse than leaving it alone + it('never enlarges an image that is already small', () => { + const thumbnail = PNG.sync.read(encodeThumbnail(solid(100, 80))); + + expect(thumbnail.width).toBe(100); + expect(thumbnail.height).toBe(80); + }); +}); diff --git a/src/compare/libs/pixelmatch/thumbnail.core.ts b/src/compare/libs/pixelmatch/thumbnail.core.ts new file mode 100644 index 00000000..1fef0cd6 --- /dev/null +++ b/src/compare/libs/pixelmatch/thumbnail.core.ts @@ -0,0 +1,25 @@ +import { PNG } from 'pngjs'; +import { downscale, RawImage } from '../../utils'; + +/** + * Longest side, in pixels, of the picture the grids draw. They lay these out a + * hundred-odd pixels wide, so this leaves room for a retina screen and nothing + * more. Sending the full screenshot and letting CSS shrink it cost roughly 7 MB + * and 68 requests every time the variations dialog opened. + */ +export const THUMBNAIL_MAX_DIMENSION = 400; + +/** + * A small PNG of an already-decoded image, for the card grids. Produced beside + * the diff, from the pixels it has already decoded, so a thumbnail costs a + * resize rather than a second read of the screenshot. + * + * An image already within the bound is re-encoded as it is: enlarging it would + * make it bigger on the wire for no gain. + */ +export function encodeThumbnail(source: RawImage): Buffer { + const small = downscale(source, THUMBNAIL_MAX_DIMENSION); + const png = new PNG({ width: small.width, height: small.height }); + small.data.copy(png.data); + return PNG.sync.write(png); +} diff --git a/src/compare/utils/index.ts b/src/compare/utils/index.ts index 79e593b1..430389d9 100644 --- a/src/compare/utils/index.ts +++ b/src/compare/utils/index.ts @@ -34,3 +34,34 @@ export const parseConfig = (configJson: string, defaultConfig: T, logger: Log } return defaultConfig; }; + +export interface RawImage { + data: Buffer; + width: number; + height: number; +} + +// Nearest-neighbour downscale so the longest side is at most maxDimension. +// Returns the original when already small enough. +export function downscale(source: RawImage, maxDimension: number): RawImage { + const scale = maxDimension / Math.max(source.width, source.height); + if (scale >= 1) { + return source; + } + const width = Math.max(1, Math.round(source.width * scale)); + const height = Math.max(1, Math.round(source.height * scale)); + const data: Buffer = Buffer.alloc(width * height * 4); + for (let y = 0; y < height; y++) { + const sourceY = Math.min(source.height - 1, Math.floor(y / scale)); + for (let x = 0; x < width; x++) { + const sourceX = Math.min(source.width - 1, Math.floor(x / scale)); + const sourceIndex = (sourceY * source.width + sourceX) * 4; + const targetIndex = (y * width + x) * 4; + data[targetIndex] = source.data[sourceIndex]; + data[targetIndex + 1] = source.data[sourceIndex + 1]; + data[targetIndex + 2] = source.data[sourceIndex + 2]; + data[targetIndex + 3] = source.data[sourceIndex + 3]; + } + } + return { data, width, height }; +} diff --git a/src/test-runs/diffResult.ts b/src/test-runs/diffResult.ts index b4219595..366c7bc2 100644 --- a/src/test-runs/diffResult.ts +++ b/src/test-runs/diffResult.ts @@ -1,5 +1,17 @@ import { TestStatus } from '@prisma/client'; +/** + * A signature and the comparison settings it was computed under. A signature + * only means anything under its own threshold, so comparing one taken at a + * different setting quietly makes grouping worse — the settings travel with it + * so a stored one can be discarded when the project's config moves on. + */ +export interface StampedSignature { + threshold: number; + includeAA: boolean; + signature: number[]; +} + export interface DiffResult { status: TestStatus; diffName: string; @@ -12,4 +24,20 @@ export interface DiffResult { * Can be displayed as bullet points in UI */ vlmDescription?: string; + /** + * Position-independent colour signature of the change, produced by the same + * pass that produced the diff, carrying the settings it was produced under. + * Stored on the run so the variations dialog can group a screen's locales + * without decoding their screenshots all over again. Absent when nothing + * changed, when the dimensions differ, or when the comparison was not + * pixelmatch. + */ + changeSignature?: StampedSignature; + /** + * Names the small copies of the checkpoint and the diff were saved under, for + * the card grids to draw instead of the full-size files. Absent when the + * comparison produced no diff, or was not pixelmatch. + */ + imageThumbnailName?: string; + diffThumbnailName?: string; } diff --git a/src/test-runs/dto/testRun.dto.ts b/src/test-runs/dto/testRun.dto.ts index 4aa53168..f0d01d7e 100644 --- a/src/test-runs/dto/testRun.dto.ts +++ b/src/test-runs/dto/testRun.dto.ts @@ -8,6 +8,11 @@ export class TestRunDto { buildId: string; @ApiProperty() imageName: string; + // small copies for the card grids; null on runs ingested before these existed + @ApiPropertyOptional() + imageThumbnailName?: string; + @ApiPropertyOptional() + diffThumbnailName?: string; @ApiProperty() diffName: string; @ApiProperty() @@ -51,6 +56,8 @@ export class TestRunDto { this.id = testRun.id; this.buildId = testRun.buildId; this.imageName = testRun.imageName; + this.imageThumbnailName = testRun.imageThumbnailName; + this.diffThumbnailName = testRun.diffThumbnailName; this.diffName = testRun.diffName; this.diffPercent = testRun.diffPercent; this.diffTollerancePercent = testRun.diffTollerancePercent; diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 57d2da60..aa636494 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -1,6 +1,7 @@ import { mocked } from 'jest-mock'; import { Test, TestingModule } from '@nestjs/testing'; import { SIGNATURE_CONCURRENCY, TestRunsService } from './test-runs.service'; +import { SIGNATURE_LENGTH } from '../compare/libs/pixelmatch/signature.core'; import { PrismaService } from '../prisma/prisma.service'; import { StaticService } from '../static/static.service'; import { TestStatus, TestRun, TestVariation } from '@prisma/client'; @@ -350,11 +351,49 @@ describe('TestRunsService', () => { pixelMisMatchCount: null, diffPercent: null, vlmDescription: null, + changeSignature: null, + imageThumbnailName: null, + diffThumbnailName: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); }); + // Written here, next to the diff that decoded the screenshots anyway, so + // the variations dialog never has to decode them again. + it('stores the change signature the comparison produced', async () => { + const testRunUpdateMock = jest.fn().mockResolvedValueOnce(testRun); + service = await initService({ testRunUpdateMock }); + + await service.saveDiffResult('some id', { + status: TestStatus.unresolved, + diffName: 'diff image name', + pixelMisMatchCount: 11, + diffPercent: 22, + isSameDimension: true, + changeSignature: { threshold: 0.1, includeAA: true, signature: [0.25, 0.75] }, + }); + + expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ + changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature: [0.25, 0.75] }), + }); + }); + + it('stores no signature when the comparison produced none', async () => { + const testRunUpdateMock = jest.fn().mockResolvedValueOnce(testRun); + service = await initService({ testRunUpdateMock }); + + await service.saveDiffResult('some id', { + status: TestStatus.ok, + diffName: null, + pixelMisMatchCount: 0, + diffPercent: 0, + isSameDimension: true, + }); + + expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ changeSignature: null }); + }); + it('with results', async () => { const diff: DiffResult = { status: TestStatus.unresolved, @@ -382,6 +421,9 @@ describe('TestRunsService', () => { pixelMisMatchCount: diff.pixelMisMatchCount, diffPercent: diff.diffPercent, vlmDescription: diff.vlmDescription, + changeSignature: null, + imageThumbnailName: null, + diffThumbnailName: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); @@ -709,9 +751,101 @@ describe('TestRunsService', () => { }); }); + describe('thumbnails', () => { + it('stores the names the comparison saved them under', async () => { + const testRunUpdateMock = jest.fn().mockResolvedValueOnce(generateTestRun()); + service = await initService({ testRunUpdateMock }); + + await service.saveDiffResult('some id', { + status: TestStatus.unresolved, + diffName: 'diff.png', + pixelMisMatchCount: 11, + diffPercent: 22, + isSameDimension: true, + imageThumbnailName: 'image.thumb.png', + diffThumbnailName: 'diff.thumb.png', + }); + + expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ + imageThumbnailName: 'image.thumb.png', + diffThumbnailName: 'diff.thumb.png', + }); + }); + + // Recalculating replaces the names on the row, so whatever they pointed at + // before has to go with the old diff — otherwise every ignore-area edit + // leaves two more objects nothing references. + it('removes the previous ones when the diff is recalculated', async () => { + const testRun = generateTestRun({ + diffName: 'old-diff.png', + imageThumbnailName: 'old-image.thumb.png', + diffThumbnailName: 'old-diff.thumb.png', + }); + const deleteImageMock = jest.fn(); + service = await initService({ + deleteImageMock, + compareGetDiffMock: jest.fn().mockResolvedValueOnce({ status: TestStatus.ok }), + testRunUpdateMock: jest.fn().mockResolvedValueOnce(testRun), + }); + + await service.calculateDiff('projectId', testRun); + + expect(deleteImageMock.mock.calls.map(([name]) => name).sort()).toEqual( + ['old-diff.png', 'old-diff.thumb.png', 'old-image.thumb.png'].sort() + ); + }); + + // Deleting before the replacement exists means a failed comparison leaves + // the row pointing at files that are gone — the reviewer opens the run and + // sees broken pictures, with nothing to recover from. + it('keeps the previous ones when the comparison fails', async () => { + const testRun = generateTestRun({ + diffName: 'old-diff.png', + imageThumbnailName: 'old-image.thumb.png', + diffThumbnailName: 'old-diff.thumb.png', + }); + const deleteImageMock = jest.fn(); + service = await initService({ + deleteImageMock, + compareGetDiffMock: jest.fn().mockRejectedValueOnce(new Error('storage is down')), + }); + + await expect(service.calculateDiff('projectId', testRun)).rejects.toThrow('storage is down'); + + expect(deleteImageMock).not.toHaveBeenCalled(); + }); + + // two extra objects per run: left behind on delete they would outlive every + // build that ever referenced them, and nothing would ever collect them + it('removes them with the run they belong to', async () => { + const testRun = generateTestRun({ + imageName: 'image.png', + diffName: 'diff.png', + imageThumbnailName: 'image.thumb.png', + diffThumbnailName: 'diff.thumb.png', + }); + const deleteImageMock = jest.fn(); + service = await initService({ + testRunFindUniqueMock: jest.fn().mockResolvedValueOnce(testRun), + testRunDeleteMock: jest.fn().mockResolvedValueOnce(testRun), + deleteImageMock, + }); + + await service.delete(testRun.id); + + expect(deleteImageMock.mock.calls.map(([name]) => name).sort()).toEqual( + ['diff.png', 'diff.thumb.png', 'image.png', 'image.thumb.png'].sort() + ); + }); + }); + describe('getMatchingVariations', () => { - const SAME_PALETTE = [1, 0]; - const OTHER_PALETTE = [0, 1]; + // Full-length histograms, because a stored one is only reused when its shape + // matches what this build produces. Orthogonal, so they never match. + const histogram = (hot: number): number[] => + Array.from({ length: SIGNATURE_LENGTH }, (_unused, index) => (index === hot ? 1 : 0)); + const SAME_PALETTE = histogram(0); + const OTHER_PALETTE = histogram(SIGNATURE_LENGTH - 1); // A sibling of the reviewed screen. imageName doubles as the run's identity // in the signature mocks below, which see buffers rather than runs. @@ -774,6 +908,99 @@ describe('TestRunsService', () => { ]); }); + // The whole point of storing it: a build's screenshots are fetched and + // decoded once at ingest, never again to group a screen at review time. + it('groups from the signatures stored at ingest, without decoding anything', async () => { + const signed = (id: string, signature: number[], overrides = {}) => + sibling(id, { + changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature }), + ...overrides, + }); + const testRun = signed('target', SAME_PALETTE, { customTags: '' }); + const matching = signed('locale-a', SAME_PALETTE); + const different = signed('locale-c', OTHER_PALETTE); + + const built = await initMatchingService({ + testRun, + siblings: [matching, different], + signatureOf: () => { + throw new Error('should not be computing a signature'); + }, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + expect(built.compareGetChangeSignatureMock).not.toHaveBeenCalled(); + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); + expect(result.skipped.map((item) => item.reason)).toEqual(['different change pattern']); + }); + + // A signature only means anything under the threshold it was computed with. + // The in-memory memo has always keyed on that; the stored one has to too, + // or changing a project's comparison config silently mixes signatures from + // two different settings and grouping quietly gets worse. + it('recomputes when the project config has moved on since ingest', async () => { + const stale = JSON.stringify({ threshold: 0.9, includeAA: false, signature: SAME_PALETTE }); + const testRun = sibling('target', { customTags: '', changeSignature: stale }); + const matching = sibling('locale-a', { changeSignature: stale }); + + const built = await initMatchingService({ + testRun, + siblings: [matching], + signatureOf: () => SAME_PALETTE, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString()); + expect(asked.sort()).toEqual([testRun.imageName, matching.imageName].sort()); + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); + }); + + // A signature is a fixed-length histogram. One of the wrong length compared + // against one of the right length gives a similarity score computed over + // undefined entries — a number, silently meaningless. The same self- + // invalidating guard as the config stamp: if the shape moves on, recompute. + it.each([ + ['the wrong length', [1, 0]], + ['values that are not numbers', new Array(SIGNATURE_LENGTH).fill('nope')], + ])('recomputes a stored signature with %s', async (_case, signature) => { + const stored = JSON.stringify({ threshold: 0.1, includeAA: true, signature }); + const testRun = sibling('target', { customTags: '', changeSignature: stored }); + + const built = await initMatchingService({ + testRun, + siblings: [], + signatureOf: () => SAME_PALETTE, + }); + + await built.service.getMatchingVariations(testRun.id); + + expect(built.compareGetChangeSignatureMock).toHaveBeenCalled(); + }); + + // builds ingested before the column existed, and runs compared by something + // other than pixelmatch, still have to group + it('falls back to computing for a run with nothing stored', async () => { + const testRun = sibling('target', { + customTags: '', + changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature: SAME_PALETTE }), + }); + const unsigned = sibling('locale-a'); + + const built = await initMatchingService({ + testRun, + siblings: [unsigned], + signatureOf: () => SAME_PALETTE, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString()); + expect(asked).toEqual([unsigned.imageName]); + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, unsigned.id]); + }); + it('skips a far larger change without paying for its signature', async () => { const testRun = sibling('target', { customTags: '' }); const bigger = sibling('locale-b', { diffPercent: 30 }); diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index 2104e37f..3840717d 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -4,7 +4,7 @@ import { IgnoreAreaDto } from './dto/ignore-area.dto'; import { StaticService } from '../static/static.service'; import { PrismaService } from '../prisma/prisma.service'; import { Baseline, Prisma, TestRun, TestStatus, TestVariation } from '@prisma/client'; -import { DiffResult } from './diffResult'; +import { DiffResult, StampedSignature } from './diffResult'; import { EventsGateway } from '../shared/events/events.gateway'; import { TestRunResultDto } from '../test-runs/dto/testRunResult.dto'; import { TestVariationsService } from '../test-variations/test-variations.service'; @@ -15,7 +15,7 @@ import { UpdateTestRunDto } from './dto/update-test.dto'; import { parseConfig } from '../compare/utils'; import { DEFAULT_CONFIG } from '../compare/libs/pixelmatch/pixelmatch.service'; import { PixelmatchConfig } from '../compare/libs/pixelmatch/pixelmatch.types'; -import { signaturesMatch } from '../compare/libs/pixelmatch/signature.core'; +import { signaturesMatch, SIGNATURE_LENGTH } from '../compare/libs/pixelmatch/signature.core'; @Injectable() export class TestRunsService { @@ -292,13 +292,21 @@ export class TestRunsService { /** * Position-independent color signature of the change between a test run's * baseline and image. Null when there is no baseline, dimensions differ, or - * nothing changed. The bytes go to the worker pool undecoded — decoding is - * the expensive part and must not happen on the event loop. + * nothing changed. * - * Memoized: a reviewer who reopens the variations dialog, or steps back to a - * screen already looked at, would otherwise pay for the same decodes again. + * Normally this was written at ingest, beside the diff that had already + * decoded both screenshots, and grouping a screen costs nothing but the row + * it is read from. The rest of this is the fallback for runs that carry no + * stored signature — ingested before the column existed, or compared by + * something other than pixelmatch: their bytes go to the worker pool + * undecoded, and the result is memoized so reopening the dialog on an old + * build does not pay for the same decodes twice. */ private async getChangeSignature(testRun: TestRun, config: PixelmatchConfig): Promise { + const stored = parseStoredSignature(testRun.changeSignature, config, this.logger); + if (stored) { + return stored; + } if (!testRun.baselineName) { return null; } @@ -358,7 +366,10 @@ export class TestRunsService { return this.findOne(id); } - async saveDiffResult(id: string, diffResult: DiffResult): Promise { + // Nullable because it genuinely is: a comparison that produced nothing at all + // clears the row back to "new". The body has always handled that; the + // signature just did not say so. + async saveDiffResult(id: string, diffResult: DiffResult | null): Promise { return this.prismaService.testRun .update({ where: { id }, @@ -368,6 +379,14 @@ export class TestRunsService { diffPercent: diffResult && diffResult.diffPercent, status: diffResult ? diffResult.status : TestStatus.new, vlmDescription: diffResult && diffResult?.vlmDescription, + // Always written, never merged: a recomputed diff — after the + // reviewer edits the ignore areas, say — must not leave the previous + // signature behind describing a change that no longer exists. + changeSignature: diffResult?.changeSignature ? JSON.stringify(diffResult.changeSignature) : null, + // Overwritten together with the diff they were made from, so a + // recomputed comparison never leaves a picture of the old change. + imageThumbnailName: diffResult?.imageThumbnailName ?? null, + diffThumbnailName: diffResult?.diffThumbnailName ?? null, }, }) .then((testRun) => { @@ -377,7 +396,17 @@ export class TestRunsService { } async calculateDiff(projectId: string, testRun: TestRun): Promise { - this.staticService.deleteImage(testRun.diffName); + // The recomputed result replaces all three names on the row, so the old + // pictures go with it — leaving the thumbnails behind would strand two more + // objects on every ignore-area edit. + // + // They go *after* the new result is persisted, not before. Deleting first + // meant a comparison that failed left the row pointing at files that were + // already gone: the reviewer opens the run and finds broken pictures, with + // nothing to fall back on. Deleting late can only ever leak bytes, which is + // the cheaper of the two failures by a distance. + const previous = [testRun.diffName, testRun.imageThumbnailName, testRun.diffThumbnailName]; + const diffResult = await this.compareService.getDiff({ projectId, data: { @@ -388,7 +417,10 @@ export class TestRunsService { saveDiffAsFile: true, }, }); - return this.saveDiffResult(testRun.id, diffResult); + const saved = await this.saveDiffResult(testRun.id, diffResult); + + previous.forEach((name) => this.staticService.deleteImage(name)); + return saved; } async create({ @@ -450,6 +482,10 @@ export class TestRunsService { await Promise.all([ this.staticService.deleteImage(testRun.diffName), this.staticService.deleteImage(testRun.imageName), + // left behind, these would outlive every build that referenced them and + // nothing would ever collect them + this.staticService.deleteImage(testRun.imageThumbnailName), + this.staticService.deleteImage(testRun.diffThumbnailName), ]); try { @@ -676,6 +712,44 @@ async function mapWithConcurrency(items: T[], limit: number, task: (item: return results; } +/** + * The signature stored on a run, if it is still usable. Null — meaning + * "recompute" — when there is none, when it cannot be read, or when the + * project's comparison settings have moved on since it was written. + * + * That last case is the point. Comparing a signature taken at one threshold + * against a sibling's taken at another quietly makes grouping worse, and the + * reviewer has no way to tell why. The in-memory memo has always keyed on the + * same settings; this is the stored equivalent. + */ +function parseStoredSignature( + stored: string | null | undefined, + config: PixelmatchConfig, + logger: Logger +): number[] | null { + if (!stored) { + return null; + } + try { + const parsed: StampedSignature = JSON.parse(stored); + // Shape first: a histogram of the wrong length, or one carrying anything + // that is not a number, compares against a current signature to produce a + // score that looks fine and means nothing. Recomputing is always safe. + const wellFormed = + Array.isArray(parsed?.signature) && + parsed.signature.length === SIGNATURE_LENGTH && + parsed.signature.every((value) => typeof value === 'number' && Number.isFinite(value)); + if (!wellFormed) { + return null; + } + const sameConfig = parsed.threshold === config.threshold && parsed.includeAA === config.ignoreAntialiasing; + return sameConfig ? parsed.signature : null; + } catch (error) { + logger.warn(`Ignoring unreadable stored change signature: ${error}`); + return null; + } +} + // Same palette but a much larger/smaller change area signals a different or // additional change (an extra element changed too), not just per-locale text // reflow — which stays well under this ratio. Such variations go to manual review.