Skip to content

fix: Add proper error message for Odiff with S3 #332

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

Merged
merged 8 commits into from
Jun 28, 2025
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
8 changes: 7 additions & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@ services:
dockerfile: Dockerfile
environment:
DATABASE_URL: postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@postgres:5432/${POSTGRES_DB}
STATIC_SERVICE: ${STATIC_SERVICE}
AWS_ACCESS_KEY_ID: ${AWS_ACCESS_KEY_ID}
AWS_SECRET_ACCESS_KEY: ${AWS_SECRET_ACCESS_KEY}
AWS_REGION: ${AWS_REGION}
AWS_S3_BUCKET_NAME: ${AWS_S3_BUCKET_NAME}
JWT_SECRET: ${JWT_SECRET}
JWT_LIFE_TIME: ${JWT_LIFE_TIME}
BODY_PARSER_JSON_LIMIT: ${BODY_PARSER_JSON_LIMIT}
APP_FRONTEND_URL: ${APP_FRONTEND_URL}
BODY_PARSER_JSON_LIMIT: ${BODY_PARSER_JSON_LIMIT}
ELASTIC_URL: ${ELASTIC_URL}
ports:
- "${APP_PORT}:3000"
expose:
Expand Down
39 changes: 39 additions & 0 deletions src/compare/compare.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,14 @@ import { LookSameService } from './libs/looks-same/looks-same.service';
import { OdiffService } from './libs/odiff/odiff.service';
import { PixelmatchService } from './libs/pixelmatch/pixelmatch.service';
import { StaticModule } from '../static/static.module';
import { ImageComparison } from '@prisma/client';
import * as utils from '../static/utils';

describe('CompareService', () => {
let service: CompareService;
let pixelmatchService: PixelmatchService;
let lookSameService: LookSameService;
let odiffService: OdiffService;

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
Expand All @@ -16,9 +21,43 @@ describe('CompareService', () => {
}).compile();

service = module.get<CompareService>(CompareService);
pixelmatchService = module.get<PixelmatchService>(PixelmatchService);
lookSameService = module.get<LookSameService>(LookSameService);
odiffService = module.get<OdiffService>(OdiffService);
});

it('should be defined', () => {
expect(service).toBeDefined();
});

describe('getComparator', () => {
it('should return pixelmatchService', () => {
const result = service.getComparator(ImageComparison.pixelmatch);
expect(result).toBe(pixelmatchService);
});

it('should return lookSameService', () => {
const result = service.getComparator(ImageComparison.lookSame);
expect(result).toBe(lookSameService);
});

it('should return odiffService', () => {
jest.spyOn(utils, 'isHddStaticServiceConfigured').mockReturnValue(true);

expect(service.getComparator(ImageComparison.odiff)).toBe(odiffService);
});

it('should throw if not HDD for Odiff', () => {
jest.spyOn(utils, 'isHddStaticServiceConfigured').mockReturnValue(false);

expect(() => service.getComparator(ImageComparison.odiff)).toThrow(
'Odiff can only be used with HDD static service. Please use another image comparison lib in project settings or switch STATIC_SERVICE envitonmental variable to HDD.'
);
});

it('should return pixelmatchService for unknown value', () => {
const result = service.getComparator('unknown' as ImageComparison);
expect(result).toBe(pixelmatchService);
});
});
});
20 changes: 15 additions & 5 deletions src/compare/compare.service.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
import { ImageComparison, Project } from '@prisma/client';
import { Injectable } from '@nestjs/common';
import { Injectable, Logger } from '@nestjs/common';
import { PixelmatchService } from './libs/pixelmatch/pixelmatch.service';
import { ImageComparator } from './libs/image-comparator.interface';
import { ImageCompareInput } from './libs/ImageCompareInput';
import { PrismaService } from '../prisma/prisma.service';
import { DiffResult } from '../test-runs/diffResult';
import { LookSameService } from './libs/looks-same/looks-same.service';
import { OdiffService } from './libs/odiff/odiff.service';
import { isHddStaticServiceConfigured } from '../static/utils';

@Injectable()
export class CompareService {
private readonly logger: Logger = new Logger(CompareService.name);

constructor(
private pixelmatchService: PixelmatchService,
private lookSameService: LookSameService,
private odiffService: OdiffService,
private prismaService: PrismaService
private readonly pixelmatchService: PixelmatchService,
private readonly lookSameService: LookSameService,
private readonly odiffService: OdiffService,
private readonly prismaService: PrismaService
) {}

async getDiff({ projectId, data }: { projectId: string; data: ImageCompareInput }): Promise<DiffResult> {
Expand All @@ -33,9 +36,16 @@ export class CompareService {
return this.lookSameService;
}
case ImageComparison.odiff: {
if (!isHddStaticServiceConfigured()) {
throw new Error(
'Odiff can only be used with HDD static service. Please use another image comparison lib in project settings or switch STATIC_SERVICE envitonmental variable to HDD.'
);
}

return this.odiffService;
}
default: {
this.logger.warn(`Unknown ImageComparison value: ${imageComparison}. Falling back to pixelmatch.`);
return this.pixelmatchService;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/compare/libs/looks-same/looks-same.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const DEFAULT_CONFIG: LooksSameConfig = {
export class LookSameService implements ImageComparator {
private readonly logger: Logger = new Logger(LookSameService.name);

constructor(private staticService: StaticService) {}
constructor(private readonly staticService: StaticService) {}

parseConfig(configJson: string): LooksSameConfig {
return parseConfig(configJson, DEFAULT_CONFIG, this.logger);
Expand Down
8 changes: 1 addition & 7 deletions src/compare/libs/odiff/odiff.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { compare } from 'odiff-bin';
import { IgnoreAreaDto } from 'src/test-runs/dto/ignore-area.dto';
import { OdiffConfig, OdiffIgnoreRegions, OdiffResult } from './odiff.types';
import { HddService } from 'src/static/hdd/hdd.service';
import { isHddStaticServiceConfigured } from '../../../static/utils';

export const DEFAULT_CONFIG: OdiffConfig = {
outputDiffMask: true,
Expand All @@ -24,12 +23,7 @@ export class OdiffService implements ImageComparator {
private readonly logger: Logger = new Logger(OdiffService.name);
private readonly hddService: HddService;

constructor(private staticService: StaticService) {
if (!isHddStaticServiceConfigured()) {
return undefined;
// If we throw an exception, the application does not start.
// throw new Error('OdiffService can only be used with HddService');
}
constructor(private readonly staticService: StaticService) {
this.hddService = this.staticService as unknown as HddService;
}

Expand Down
2 changes: 1 addition & 1 deletion src/compare/libs/pixelmatch/pixelmatch.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export const DEFAULT_CONFIG: PixelmatchConfig = { threshold: 0.1, ignoreAntialia
export class PixelmatchService implements ImageComparator {
private readonly logger: Logger = new Logger(PixelmatchService.name);

constructor(private staticService: StaticService) {}
constructor(private readonly staticService: StaticService) {}

parseConfig(configJson: string): PixelmatchConfig {
return parseConfig(configJson, DEFAULT_CONFIG, this.logger);
Expand Down