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/schema.prisma b/prisma/schema.prisma index 4835aeae..104dd8aa 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,13 @@ 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? 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..7fc3bc27 100644 --- a/src/_data_/index.ts +++ b/src/_data_/index.ts @@ -99,6 +99,7 @@ export const generateTestRun = (testRun?: Partial): TestRun => { branchName: 'develop', merge: false, vlmDescription: null, + changeSignature: 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..5e9e47c9 --- /dev/null +++ b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts @@ -0,0 +1,87 @@ +import { PNG } from 'pngjs'; +import { computePixelmatchDiff } from './pixelmatch.core'; +import { computeChangeSignature } from './signature.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) => + computePixelmatchDiff({ + kind: 'diff', + baseline, + image, + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + allowDiffDimensions: false, + diffTolerancePercent: 0, + saveDiff: false, + withSignature, + }); + +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); + }); +}); diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts index 47bbc71e..9dc12968 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts @@ -2,6 +2,7 @@ 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'; /** * CPU-bound part of the pixelmatch comparison, extracted so it can run inside @@ -20,6 +21,13 @@ 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; } export interface PixelmatchJobOutput { @@ -28,6 +36,9 @@ 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[]; } // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. @@ -74,5 +85,23 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO 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; + + return { + equal: false, + isSameDimension, + pixelMisMatchCount, + diffPercent, + diffBuffer, + ...(signature ? { signature } : {}), + }; } diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts index 1696b97d..2a659f40 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts @@ -257,3 +257,75 @@ 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) => + service.getDiff( + { + baseline: 'baseline.png', + image: 'image.png', + ignoreAreas: [], + diffTollerancePercent: 0, + saveDiffAsFile: false, + }, + 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], + }); + }); + + 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..55b184a8 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -45,6 +45,12 @@ 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, }); if (output.equal) { @@ -59,6 +65,15 @@ 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) { diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts index 9909354c..822208d2 100644 --- a/src/compare/libs/pixelmatch/signature.core.ts +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -30,6 +30,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; @@ -106,17 +112,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 +153,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/test-runs/diffResult.ts b/src/test-runs/diffResult.ts index b4219595..c9bfce15 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,13 @@ 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; } diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 57d2da60..10a4f62e 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,47 @@ describe('TestRunsService', () => { pixelMisMatchCount: null, diffPercent: null, vlmDescription: null, + changeSignature: 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 +419,7 @@ describe('TestRunsService', () => { pixelMisMatchCount: diff.pixelMisMatchCount, diffPercent: diff.diffPercent, vlmDescription: diff.vlmDescription, + changeSignature: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); @@ -710,8 +748,12 @@ describe('TestRunsService', () => { }); 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 +816,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..423a6705 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,10 @@ 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, }, }) .then((testRun) => { @@ -676,6 +691,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.