Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow custom threshold #1339

Merged
merged 1 commit into from
Jun 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions apps/backend/db/migrations/20240630151704_screenshot-threshold.js
Original file line number Diff line number Diff line change
@@ -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");
});
};
6 changes: 4 additions & 2 deletions apps/backend/db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,8 @@ CREATE TABLE public.screenshots (
"testId" bigint,
metadata jsonb,
"playwrightTraceFileId" bigint,
"buildShardId" bigint
"buildShardId" bigint,
threshold real
);


Expand Down Expand Up @@ -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());
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());
2 changes: 2 additions & 0 deletions apps/backend/src/database/models/Screenshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
},
});

Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions apps/backend/src/database/services/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string[]> => {
): Promise<string[]> {
if (keys.length === 0) {
return [];
}
Expand All @@ -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))));
};
}
5 changes: 4 additions & 1 deletion apps/backend/src/database/services/screenshots.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { checkIsNonNullable } from "@argos/util/checkIsNonNullable";
import { invariant } from "@argos/util/invariant";
import type { PartialModelObject, TransactionOrKnex } from "objection";

Expand Down Expand Up @@ -51,6 +52,7 @@ type InsertFilesAndScreenshotsParams = {
name: string;
metadata?: ScreenshotMetadata | null;
pwTraceKey?: string | null;
threshold?: number | null;
}[];
build: Build;
shard?: BuildShard | null;
Expand All @@ -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],
Expand Down Expand Up @@ -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,
};
}),
);
Expand Down
8 changes: 6 additions & 2 deletions apps/backend/src/screenshot-diff/computeScreenshotDiff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string> => {
const fileStream = createReadStream(filepath);
Expand Down Expand Up @@ -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)
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
38 changes: 20 additions & 18 deletions apps/backend/src/screenshot-diff/util/image-diff/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -41,6 +42,7 @@ describe("#diffImages", () => {
new LocalImageFile({
filepath: resolve(dir, "base.png"),
}),
Number(threshold),
);

const diffPath = resolve(dir, "diff_tmp.png");
Expand Down
59 changes: 45 additions & 14 deletions apps/backend/src/screenshot-diff/util/image-diff/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<number> => {
}): Promise<number> {
const result = await compare(args.basePath, args.comparePath, args.diffPath, {
outputDiffMask: true,
threshold: args.threshold,
Expand All @@ -59,7 +78,7 @@ const computeDiff = async (args: {
default:
throw new Error("Unknown reason");
}
};
}

/**
* Get the maximum dimensions of a list of images.
Expand All @@ -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.
Expand All @@ -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<null | {
filepath: string;
score: number;
Expand All @@ -96,34 +124,37 @@ export async function diffImages(
tmpName({ postfix: ".png" }),
]);

// Resize images to the maximum dimensions
const [basePath, comparePath] = await Promise.all([
baseImage.enlarge(maxDimensions),
compareImage.enlarge(maxDimensions),
]);
// Resize images to the maximum dimensions (sequentially to avoid memory issues)
const basePath = await baseImage.enlarge(maxDimensions);
const comparePath = await compareImage.enlarge(maxDimensions);

const diffConfig = getConfiguration(threshold);

// Compute diff sequentiall to avoid memory issues
// Compute diffs (sequentially to avoid memory issues)
const baseScore = await computeDiff({
basePath,
comparePath,
diffPath: baseDiffPath,
threshold: BASE_THRESHOLD,
threshold: diffConfig.baseThreshold,
});
const colorSensitiveScore = await computeDiff({
basePath,
comparePath,
diffPath: colorDiffPath,
threshold: COLOR_SENSIBLE_THRESHOLD,
threshold: diffConfig.colorSensitiveThreshold,
});

const maxBaseScore = Math.min(
BASE_MAX_SCORE,
MAXIMUM_PIXELS_TO_IGNORE / (maxDimensions.width * maxDimensions.height),
diffConfig.baseMaxScore,
diffConfig.maximumPixelsToIgnore /
(maxDimensions.width * maxDimensions.height),
);
const adjustedBaseScore = baseScore < maxBaseScore ? 0 : baseScore;

const adjustedSensitiveScore =
colorSensitiveScore < COLOR_SENSIBLE_MAX_SCORE ? 0 : colorSensitiveScore;
colorSensitiveScore < diffConfig.colorSensitiveMaxScore
? 0
: colorSensitiveScore;

if (adjustedBaseScore > 0 && adjustedBaseScore > adjustedSensitiveScore) {
return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ async function compareLocalImages(params: {
new LocalImageFile({
filepath: params.compareFilepath,
}),
0.5,
);
}

Expand Down
6 changes: 3 additions & 3 deletions apps/backend/src/web/api/v2/builds/update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -100,7 +100,7 @@ const handleUpdateSingle = async ({
.patchAndFetch({ complete: true, screenshotCount });
});
await buildJob.push(build.id);
};
}

router.put(
"/builds/:buildId",
Expand Down
12 changes: 12 additions & 0 deletions apps/backend/src/web/api/v2/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
},
},
Expand Down Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions apps/backend/src/web/app.api-v2.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ describe("api v2", () => {
key: screenshot.key,
name: screenshot.name,
metadata: screenshot.metadata,
threshold: 0.3,
})),
})
.expect(200);
Expand All @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/util/src/checkIsNonNullable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
* Filter function to exclude `null` values
*/
export function checkIsNonNullable<T>(value: T): value is NonNullable<T> {
return value !== null;
return value !== null && value !== undefined;
}