diff --git a/apps/backend/db/migrations/20240630151704_screenshot-threshold.js b/apps/backend/db/migrations/20240630151704_screenshot-threshold.js new file mode 100644 index 000000000..54d6d72cb --- /dev/null +++ b/apps/backend/db/migrations/20240630151704_screenshot-threshold.js @@ -0,0 +1,17 @@ +/** + * @param {import('knex').Knex} knex + */ +export const up = async (knex) => { + await knex.schema.alterTable("screenshots", (table) => { + table.float("threshold"); + }); +}; + +/** + * @param {import('knex').Knex} knex + */ +export const down = async (knex) => { + await knex.schema.alterTable("screenshots", (table) => { + table.dropColumn("threshold"); + }); +}; diff --git a/apps/backend/db/structure.sql b/apps/backend/db/structure.sql index a8a33a525..f29638129 100644 --- a/apps/backend/db/structure.sql +++ b/apps/backend/db/structure.sql @@ -1018,7 +1018,8 @@ CREATE TABLE public.screenshots ( "testId" bigint, metadata jsonb, "playwrightTraceFileId" bigint, - "buildShardId" bigint + "buildShardId" bigint, + threshold real ); @@ -2805,4 +2806,5 @@ INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('2024042 INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240505121926_slack-installation.js', 1, NOW()); INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240604133729_comment_id_big_integer.js', 1, NOW()); INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240614204320_build_shards.js', 1, NOW()); -INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240616142430_build_shards_indices.js', 1, NOW()); \ No newline at end of file +INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240616142430_build_shards_indices.js', 1, NOW()); +INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240630151704_screenshot-threshold.js', 1, NOW()); \ No newline at end of file diff --git a/apps/backend/src/database/models/Screenshot.ts b/apps/backend/src/database/models/Screenshot.ts index 2a3e71f9e..4b14ffd85 100644 --- a/apps/backend/src/database/models/Screenshot.ts +++ b/apps/backend/src/database/models/Screenshot.ts @@ -162,6 +162,7 @@ export class Screenshot extends Model { metadata: ScreenshotMetadataJsonSchema, playwrightTraceFileId: { type: ["string", "null"] }, buildShardId: { type: ["string", "null"] }, + threshold: { type: ["number", "null"], minimum: 0, maximum: 1 }, }, }); @@ -173,6 +174,7 @@ export class Screenshot extends Model { metadata!: ScreenshotMetadata | null; playwrightTraceFileId!: string | null; buildShardId!: string | null; + threshold!: number | null; static override get relationMappings(): RelationMappings { return { diff --git a/apps/backend/src/database/services/file.ts b/apps/backend/src/database/services/file.ts index 5fd78f81d..4a6184030 100644 --- a/apps/backend/src/database/services/file.ts +++ b/apps/backend/src/database/services/file.ts @@ -2,10 +2,10 @@ import type { TransactionOrKnex } from "objection"; import { File } from "@/database/models/index.js"; -export const getUnknownFileKeys = async ( +export async function getUnknownFileKeys( keys: string[], trx?: TransactionOrKnex, -): Promise => { +): Promise { if (keys.length === 0) { return []; } @@ -14,4 +14,4 @@ export const getUnknownFileKeys = async ( .whereIn("key", keys); const existingKeys = existingFiles.map((file) => file.key); return Array.from(new Set(keys.filter((key) => !existingKeys.includes(key)))); -}; +} diff --git a/apps/backend/src/database/services/screenshots.ts b/apps/backend/src/database/services/screenshots.ts index 18141dbc2..a2ea5484c 100644 --- a/apps/backend/src/database/services/screenshots.ts +++ b/apps/backend/src/database/services/screenshots.ts @@ -1,3 +1,4 @@ +import { checkIsNonNullable } from "@argos/util/checkIsNonNullable"; import { invariant } from "@argos/util/invariant"; import type { PartialModelObject, TransactionOrKnex } from "objection"; @@ -51,6 +52,7 @@ type InsertFilesAndScreenshotsParams = { name: string; metadata?: ScreenshotMetadata | null; pwTraceKey?: string | null; + threshold?: number | null; }[]; build: Build; shard?: BuildShard | null; @@ -72,7 +74,7 @@ export async function insertFilesAndScreenshots( const screenshotKeys = screenshots.map((screenshot) => screenshot.key); const pwTraceKeys = screenshots .map((screenshot) => screenshot.pwTraceKey) - .filter(Boolean) as string[]; + .filter(checkIsNonNullable); const unknownKeys = await getUnknownFileKeys( [...screenshotKeys, ...pwTraceKeys], @@ -167,6 +169,7 @@ export async function insertFilesAndScreenshots( metadata: screenshot.metadata ?? null, playwrightTraceFileId: pwTraceFile?.id ?? null, buildShardId: params.shard?.id ?? null, + threshold: screenshot.threshold ?? null, }; }), ); diff --git a/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts b/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts index 514b5dcc8..32b62acc9 100644 --- a/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts +++ b/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts @@ -11,7 +11,7 @@ import { S3ImageFile } from "@/storage/index.js"; import { chunk } from "@/util/chunk.js"; import { getRedisLock } from "@/util/redis/index.js"; -import { diffImages } from "./util/image-diff/index.js"; +import { DEFAULT_THRESHOLD, diffImages } from "./util/image-diff/index.js"; const hashFile = async (filepath: string): Promise => { const fileStream = createReadStream(filepath); @@ -210,7 +210,11 @@ export const computeScreenshotDiff = async ( let diffKey: string | null = null; if (baseImage && baseImage.key !== compareImage.key && !screenshotDiff.s3Id) { - const diffResult = await diffImages(baseImage, compareImage); + const diffResult = await diffImages( + baseImage, + compareImage, + screenshotDiff.compareScreenshot.threshold ?? DEFAULT_THRESHOLD, + ); if (diffResult === null) { await ScreenshotDiff.query() .findById(screenshotDiff.id) diff --git a/apps/backend/src/screenshot-diff/util/image-diff/__fixtures__/alphaSmallGraph/base.png b/apps/backend/src/screenshot-diff/util/image-diff/__fixtures__/alphaSmallGraph/base.png new file mode 100644 index 000000000..eb8b18cb9 Binary files /dev/null and b/apps/backend/src/screenshot-diff/util/image-diff/__fixtures__/alphaSmallGraph/base.png differ diff --git a/apps/backend/src/screenshot-diff/util/image-diff/__fixtures__/alphaSmallGraph/compare.png b/apps/backend/src/screenshot-diff/util/image-diff/__fixtures__/alphaSmallGraph/compare.png new file mode 100644 index 000000000..5bb5c3665 Binary files /dev/null and b/apps/backend/src/screenshot-diff/util/image-diff/__fixtures__/alphaSmallGraph/compare.png differ diff --git a/apps/backend/src/screenshot-diff/util/image-diff/index.test.ts b/apps/backend/src/screenshot-diff/util/image-diff/index.test.ts index c52f68944..2294384cc 100644 --- a/apps/backend/src/screenshot-diff/util/image-diff/index.test.ts +++ b/apps/backend/src/screenshot-diff/util/image-diff/index.test.ts @@ -11,27 +11,28 @@ import { diffImages } from "./index.js"; const __dirname = fileURLToPath(new URL(".", import.meta.url)); const tests = [ - ["alphaBackground", false], - ["big-images", true], - ["border", false], - ["boxShadow", false], - ["fontAliasing", false], - ["imageCompression1", false], - ["imageCompression2", false], - ["imageCompression3", false], - ["imageCompression4", false], - ["imageCompression4", false], - ["imageCompression5", false], - ["imageCompression6", false], - ["imageCompression7", false], - ["simple", true], - ["tableAlpha", true], - ["minorChange1", true], - ["minorChange2", true], + ["alphaBackground", 0.5, false], + ["alphaSmallGraph", 0.2, true], + ["big-images", 0.5, true], + ["border", 0.5, false], + ["boxShadow", 0.5, false], + ["fontAliasing", 0.5, false], + ["imageCompression1", 0.5, false], + ["imageCompression2", 0.5, false], + ["imageCompression3", 0.5, false], + ["imageCompression4", 0.5, false], + ["imageCompression4", 0.5, false], + ["imageCompression5", 0.5, false], + ["imageCompression6", 0.5, false], + ["imageCompression7", 0.5, false], + ["simple", 0.5, true], + ["tableAlpha", 0.5, true], + ["minorChange1", 0.5, true], + ["minorChange2", 0.5, true], ]; describe("#diffImages", () => { - test.each(tests)("diffImages %s", async (name, hasDiff) => { + test.each(tests)("diffImages %s", async (name, threshold, hasDiff) => { const dir = resolve(__dirname, `__fixtures__/${name}`); const result = await diffImages( @@ -41,6 +42,7 @@ describe("#diffImages", () => { new LocalImageFile({ filepath: resolve(dir, "base.png"), }), + Number(threshold), ); const diffPath = resolve(dir, "diff_tmp.png"); diff --git a/apps/backend/src/screenshot-diff/util/image-diff/index.ts b/apps/backend/src/screenshot-diff/util/image-diff/index.ts index 71c87e70a..8d8634e19 100644 --- a/apps/backend/src/screenshot-diff/util/image-diff/index.ts +++ b/apps/backend/src/screenshot-diff/util/image-diff/index.ts @@ -30,15 +30,34 @@ const MAXIMUM_PIXELS_TO_IGNORE = 20; const COLOR_SENSIBLE_THRESHOLD = 0.0225; const COLOR_SENSIBLE_MAX_SCORE = 0.03; // 3% of image is different +/** + * Get the configuration for the diff. + * The threshold is a number between 0 and 1. + * The higher the threshold, the less sensitive the diff will be. + * The default threshold is 0.5. + * A threshold of 0 is 100% strict. + */ +function getConfiguration(threshold: number) { + // The default threshold is 0.5. By multipliying it by 2, we make it 1 by default. + const relativeThreshold = threshold * 2; + return { + baseThreshold: BASE_THRESHOLD * relativeThreshold, + baseMaxScore: BASE_MAX_SCORE * relativeThreshold, + maximumPixelsToIgnore: MAXIMUM_PIXELS_TO_IGNORE * relativeThreshold, + colorSensitiveThreshold: COLOR_SENSIBLE_THRESHOLD * relativeThreshold, + colorSensitiveMaxScore: COLOR_SENSIBLE_MAX_SCORE * relativeThreshold, + }; +} + /** * Compute the diff between two images and returns the score. */ -const computeDiff = async (args: { +async function computeDiff(args: { basePath: string; comparePath: string; diffPath: string; threshold: number; -}): Promise => { +}): Promise { const result = await compare(args.basePath, args.comparePath, args.diffPath, { outputDiffMask: true, threshold: args.threshold, @@ -59,7 +78,7 @@ const computeDiff = async (args: { default: throw new Error("Unknown reason"); } -}; +} /** * Get the maximum dimensions of a list of images. @@ -75,6 +94,11 @@ async function getMaxDimensions(...images: ImageFile[]) { }; } +/** + * The default threshold for the diff. + */ +export const DEFAULT_THRESHOLD = 0.5; + /** * Compute the difference between two images. * Returns null if the difference is not significant. @@ -83,6 +107,10 @@ async function getMaxDimensions(...images: ImageFile[]) { export async function diffImages( baseImage: ImageFile, compareImage: ImageFile, + /** + * A threshold between 0 and 1 to adjust the sensitivity of the diff. + */ + threshold: number, ): Promise 0 && adjustedBaseScore > adjustedSensitiveScore) { return { diff --git a/apps/backend/src/screenshot-diff/util/image-diff/test/diff.test.ts b/apps/backend/src/screenshot-diff/util/image-diff/test/diff.test.ts index 94d60f96b..874232012 100644 --- a/apps/backend/src/screenshot-diff/util/image-diff/test/diff.test.ts +++ b/apps/backend/src/screenshot-diff/util/image-diff/test/diff.test.ts @@ -21,6 +21,7 @@ async function compareLocalImages(params: { new LocalImageFile({ filepath: params.compareFilepath, }), + 0.5, ); } diff --git a/apps/backend/src/web/api/v2/builds/update.ts b/apps/backend/src/web/api/v2/builds/update.ts index 083c46c8c..eced80dbd 100644 --- a/apps/backend/src/web/api/v2/builds/update.ts +++ b/apps/backend/src/web/api/v2/builds/update.ts @@ -81,13 +81,13 @@ async function handleUpdateParallel({ } } -const handleUpdateSingle = async ({ +async function handleUpdateSingle({ req, build, }: { req: UpdateRequest; build: Build; -}) => { +}) { await transaction(async (trx) => { const screenshotCount = await insertFilesAndScreenshots({ screenshots: req.body.screenshots, @@ -100,7 +100,7 @@ const handleUpdateSingle = async ({ .patchAndFetch({ complete: true, screenshotCount }); }); await buildJob.push(build.id); -}; +} router.put( "/builds/:buildId", diff --git a/apps/backend/src/web/api/v2/util.ts b/apps/backend/src/web/api/v2/util.ts index 1474c6245..113e0abab 100644 --- a/apps/backend/src/web/api/v2/util.ts +++ b/apps/backend/src/web/api/v2/util.ts @@ -185,6 +185,17 @@ export const validateUpdateRequest = validate({ type: "string", }, metadata: ScreenshotMetadataJsonSchema, + pwTraceKey: { + type: "string", + pattern: SHA256_REGEX_STR, + nullable: true, + }, + threshold: { + type: "number", + minimum: 0, + maximum: 1, + nullable: true, + }, }, }, }, @@ -218,6 +229,7 @@ export type UpdateRequest = Request< name: string; metadata?: ScreenshotMetadata | null; pwTraceKey?: string | null; + threshold?: number | null; }[]; parallel?: boolean | null; parallelTotal?: number | null; diff --git a/apps/backend/src/web/app.api-v2.e2e.test.ts b/apps/backend/src/web/app.api-v2.e2e.test.ts index e4ec8003e..fcb373559 100644 --- a/apps/backend/src/web/app.api-v2.e2e.test.ts +++ b/apps/backend/src/web/app.api-v2.e2e.test.ts @@ -206,6 +206,7 @@ describe("api v2", () => { key: screenshot.key, name: screenshot.name, metadata: screenshot.metadata, + threshold: 0.3, })), }) .expect(200); @@ -215,6 +216,12 @@ describe("api v2", () => { .first() .throwIfNotFound(); + expect( + build.compareScreenshotBucket!.screenshots?.every( + (s) => s.threshold === 0.3, + ), + ).toBe(true); + const screenshotWithMetadata = build.compareScreenshotBucket!.screenshots!.find( (screenshot) => screenshot.name === "second", diff --git a/packages/util/src/checkIsNonNullable.ts b/packages/util/src/checkIsNonNullable.ts index 79519fb3a..fc4ecf8fb 100644 --- a/packages/util/src/checkIsNonNullable.ts +++ b/packages/util/src/checkIsNonNullable.ts @@ -2,5 +2,5 @@ * Filter function to exclude `null` values */ export function checkIsNonNullable(value: T): value is NonNullable { - return value !== null; + return value !== null && value !== undefined; }