Skip to content

feat(#455): handle baselineBranchName #330

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ services:
# container_name: pgadmin4
# # https://hub.docker.com/r/dpage/pgadmin4
# # https://www.pgadmin.org/docs/pgadmin4/latest/release_notes_7_4.html
# image: dpage/pgadmin4:7.4
# image: dpage/pgadmin4
# restart: always
# environment:
# PGADMIN_DEFAULT_EMAIL: admin@admin.com
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- AlterTable
ALTER TABLE "Project" ADD COLUMN "protectedBranch" TEXT;
3 changes: 2 additions & 1 deletion prisma/schema.prisma
Original file line number Diff line number Diff line change
@@ -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"]
}

Expand Down Expand Up @@ -41,6 +41,7 @@ model Project {
builds Build[]
TestRun TestRun[]
testVariations TestVariation[]
protectedBranch String?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I completely understand the purpose of this filtering
could we move this change to separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

model TestRun {
Expand Down
1 change: 1 addition & 0 deletions prisma/test/seed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const mockDefaultProject = {
autoApproveFeature: true,
imageComparison: 'pixelmatch',
imageComparisonConfig: '{ "threshold": 0.1, "ignoreAntialiasing": true, "allowDiffDimensions": false }',
protectedBranch: null,
} as const;

const mockDefaultUser = {
Expand Down
1 change: 1 addition & 0 deletions src/_data_/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const TEST_PROJECT: Project = {
autoApproveFeature: true,
imageComparisonConfig: '{ "threshold": 0.1, "ignoreAntialiasing": true, "allowDiffDimensions": false }',
imageComparison: ImageComparison.pixelmatch,
protectedBranch: 'release-[0-9]+',
};

export const TEST_BUILD: Build = {
Expand Down
7 changes: 6 additions & 1 deletion src/projects/dto/project.dto.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ApiProperty } from '@nestjs/swagger';
import { IsUUID, IsString, IsNumber, IsBoolean, IsEnum, IsJSON, IsDate } from 'class-validator';
import { IsUUID, IsString, IsNumber, IsBoolean, IsEnum, IsJSON, IsDate, IsOptional } from 'class-validator';
import { ImageComparison, Project } from '@prisma/client';
import { Type } from 'class-transformer';

Expand Down Expand Up @@ -49,4 +49,9 @@ export class ProjectDto implements Project {
@ApiProperty()
@IsJSON()
imageComparisonConfig: string;

@ApiProperty()
@IsString()
@IsOptional()
protectedBranch: string;
}
2 changes: 2 additions & 0 deletions src/projects/projects.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export class ProjectsService {
imageComparisonConfig: projectDto.imageComparisonConfig,
maxBuildAllowed: projectDto.maxBuildAllowed,
maxBranchLifetime: projectDto.maxBranchLifetime,
protectedBranch: projectDto.protectedBranch,
},
});
}
Expand All @@ -63,6 +64,7 @@ export class ProjectsService {
maxBuildAllowed: projectDto.maxBuildAllowed,
maxBranchLifetime: projectDto.maxBranchLifetime,
imageComparisonConfig: projectDto.imageComparisonConfig,
protectedBranch: projectDto.protectedBranch,
},
});
}
Expand Down
18 changes: 5 additions & 13 deletions src/shared/tasks/tasks.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { TestVariationsService } from '../../test-variations/test-variations.ser

const initService = async ({
projectFindManyMock = jest.fn(),
testVariationFindManyMock = jest.fn(),
findOldTestVariationsMock = jest.fn(),
testVariationsDeleteMock = jest.fn(),
}) => {
const module: TestingModule = await Test.createTestingModule({
Expand All @@ -15,9 +15,6 @@ const initService = async ({
{
provide: PrismaService,
useValue: {
testVariation: {
findMany: testVariationFindManyMock,
},
project: {
findMany: projectFindManyMock,
},
Expand All @@ -27,6 +24,7 @@ const initService = async ({
provide: TestVariationsService,
useValue: {
delete: testVariationsDeleteMock,
findOldTestVariations: findOldTestVariationsMock,
},
},
],
Expand All @@ -43,11 +41,11 @@ describe('cleanOldTestVariations', () => {
const project = TEST_PROJECT;
const testVariation = generateTestVariation();
const projectFindManyMock = jest.fn().mockResolvedValueOnce([project]);
const testVariationFindManyMock = jest.fn().mockResolvedValueOnce([testVariation]);
const findOldTestVariationsMock = jest.fn().mockResolvedValueOnce([testVariation]);
const testVariationsDeleteMock = jest.fn();
service = await initService({
projectFindManyMock: projectFindManyMock,
testVariationFindManyMock: testVariationFindManyMock,
findOldTestVariationsMock: findOldTestVariationsMock,
testVariationsDeleteMock: testVariationsDeleteMock,
});
const dateNow = new Date('2022-10-23');
Expand All @@ -59,13 +57,7 @@ describe('cleanOldTestVariations', () => {
await service.cleanOldTestVariations();

// .Assert
expect(testVariationFindManyMock).toHaveBeenCalledWith({
where: {
updatedAt: { lte: dateRemoveAfter },
branchName: { not: project.mainBranchName },
projectId: project.id,
},
});
expect(findOldTestVariationsMock).toHaveBeenCalledWith(project, dateRemoveAfter);
expect(testVariationsDeleteMock).toBeCalledWith(testVariation.id);
});
});
9 changes: 2 additions & 7 deletions src/shared/tasks/tasks.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,8 @@ export class TasksService {
const dateRemoveAfter: Date = new Date();
dateRemoveAfter.setDate(dateRemoveAfter.getDate() - project.maxBranchLifetime);

const testVariations = await this.prismaService.testVariation.findMany({
where: {
projectId: project.id,
updatedAt: { lte: dateRemoveAfter },
branchName: { not: project.mainBranchName },
},
});
const testVariations = await this.testVariationService.findOldTestVariations(project, dateRemoveAfter);

this.logger.debug(
`Removing ${testVariations.length} TestVariations for ${project.name} later than ${dateRemoveAfter}`
);
Expand Down
5 changes: 5 additions & 0 deletions src/test-runs/dto/create-test-request.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,9 @@ export class CreateTestRequestDto extends BaselineDataDto {
@IsOptional()
@IsString()
comment?: string;

@ApiPropertyOptional()
@IsOptional()
@IsString()
baselineBranchName?: string;
}
20 changes: 17 additions & 3 deletions src/test-runs/test-runs.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,16 @@ export class TestRunsService {

// try auto approve
if (project.autoApproveFeature) {
testRunWithResult = await this.tryAutoApproveByPastBaselines({ testVariation, testRun: testRunWithResult });
testRunWithResult = await this.tryAutoApproveByNewBaselines({ testVariation, testRun: testRunWithResult });
testRunWithResult = await this.tryAutoApproveByPastBaselines({
testVariation,
testRun: testRunWithResult,
baselineBranchName: createTestRequestDto.baselineBranchName,
});
testRunWithResult = await this.tryAutoApproveByNewBaselines({
testVariation,
testRun: testRunWithResult,
baselineBranchName: createTestRequestDto.baselineBranchName,
});
}
return new TestRunResultDto(testRunWithResult, testVariation);
}
Expand Down Expand Up @@ -348,7 +356,11 @@ export class TestRunsService {
* @param testVariation
* @param testRun
*/
private async tryAutoApproveByNewBaselines({ testVariation, testRun }: AutoApproveProps): Promise<TestRun> {
private async tryAutoApproveByNewBaselines({
testVariation,
testRun,
baselineBranchName,
}: AutoApproveProps): Promise<TestRun> {
if (testRun.status === TestStatus.ok) {
return testRun;
}
Expand All @@ -358,6 +370,7 @@ export class TestRunsService {
where: {
...getTestVariationUniqueData(testVariation),
baselineName: testVariation.baselineName,
baselineBranchName,
status: TestStatus.approved,
testVariation: {
projectId: testVariation.projectId,
Expand Down Expand Up @@ -407,4 +420,5 @@ export class TestRunsService {
interface AutoApproveProps {
testVariation: TestVariation;
testRun: TestRun;
baselineBranchName?: string;
}
120 changes: 120 additions & 0 deletions src/test-variations/test-variations.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,126 @@ describe('TestVariationsService', () => {
});
expect(result).toBe(variationMainMock);
});

it('can find by baselineBranchName', async () => {
const createRequest: CreateTestRequestDto = {
buildId: 'buildId',
projectId: projectMock.id,
name: 'Test name',
os: 'OS',
browser: 'browser',
viewport: 'viewport',
device: 'device',
customTags: '',
branchName: 'develop',
baselineBranchName: 'main',
};

const variationMock: TestVariation = {
id: '123',
projectId: projectMock.id,
name: 'Test name',
baselineName: 'main',
os: 'OS',
browser: 'browser',
viewport: 'viewport',
device: 'device',
customTags: '',
ignoreAreas: '[]',
comment: 'some comment',
branchName: 'develop',
createdAt: new Date(),
updatedAt: new Date(),
};
const projectFindUniqueMock = jest.fn().mockReturnValueOnce(projectMock);
service = await initModule({ projectFindUniqueMock });
service.findUnique = jest.fn().mockResolvedValueOnce(variationMock).mockResolvedValueOnce(undefined);

const result = await service.find(createRequest);

expect(projectFindUniqueMock).toHaveBeenCalledWith({ where: { id: createRequest.projectId } });
expect(service.findUnique).toHaveBeenNthCalledWith(1, {
name: createRequest.name,
projectId: createRequest.projectId,
os: createRequest.os,
browser: createRequest.browser,
viewport: createRequest.viewport,
device: createRequest.device,
customTags: createRequest.customTags,
branchName: createRequest.baselineBranchName,
});
expect(service.findUnique).toHaveBeenNthCalledWith(2, {
name: createRequest.name,
projectId: createRequest.projectId,
os: createRequest.os,
browser: createRequest.browser,
viewport: createRequest.viewport,
device: createRequest.device,
customTags: createRequest.customTags,
branchName: createRequest.branchName,
});
expect(result).toBe(variationMock);
});

it("can find by current branch if baselineBranchName doesn't exist", async () => {
const createRequest: CreateTestRequestDto = {
buildId: 'buildId',
projectId: projectMock.id,
name: 'Test name',
os: 'OS',
browser: 'browser',
viewport: 'viewport',
device: 'device',
customTags: '',
branchName: 'main',
baselineBranchName: 'release-1',
};

const variationMock: TestVariation = {
id: '123',
projectId: projectMock.id,
name: 'Test name',
baselineName: 'baselineName',
os: 'OS',
browser: 'browser',
viewport: 'viewport',
device: 'device',
customTags: '',
ignoreAreas: '[]',
comment: 'some comment',
branchName: 'develop',
createdAt: new Date(),
updatedAt: new Date(),
};
const projectFindUniqueMock = jest.fn().mockReturnValueOnce(projectMock);
service = await initModule({ projectFindUniqueMock });
service.findUnique = jest.fn().mockResolvedValueOnce(undefined).mockResolvedValueOnce(variationMock);

const result = await service.find(createRequest);

expect(projectFindUniqueMock).toHaveBeenCalledWith({ where: { id: createRequest.projectId } });
expect(service.findUnique).toHaveBeenNthCalledWith(1, {
name: createRequest.name,
projectId: createRequest.projectId,
os: createRequest.os,
browser: createRequest.browser,
viewport: createRequest.viewport,
device: createRequest.device,
customTags: createRequest.customTags,
branchName: createRequest.baselineBranchName,
});
expect(service.findUnique).toHaveBeenNthCalledWith(2, {
name: createRequest.name,
projectId: createRequest.projectId,
os: createRequest.os,
browser: createRequest.browser,
viewport: createRequest.viewport,
device: createRequest.device,
customTags: createRequest.customTags,
branchName: createRequest.branchName,
});
expect(result).toBe(variationMock);
});
});

it('create', async () => {
Expand Down
28 changes: 18 additions & 10 deletions src/test-variations/test-variations.service.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Injectable, Inject, forwardRef, Logger } from '@nestjs/common';
import { PrismaService } from '../prisma/prisma.service';
import { TestVariation, Baseline, Build, TestRun, User } from '@prisma/client';
import { TestVariation, Baseline, Build, TestRun, User, type Project } from '@prisma/client';
import { StaticService } from '../static/static.service';
import { BuildsService } from '../builds/builds.service';
import { TestRunsService } from '../test-runs/test-runs.service';
Expand Down Expand Up @@ -79,36 +79,34 @@ export class TestVariationsService {
* @param baselineData
* @returns
*/
async find(
createTestRequestDto: BaselineDataDto & { projectId: string; sourceBranch?: string }
): Promise<TestVariation | null> {
async find(createTestRequestDto: Omit<CreateTestRequestDto, 'buildId'>): Promise<TestVariation | null> {
const project = await this.prismaService.project.findUnique({ where: { id: createTestRequestDto.projectId } });
const mainBranch = createTestRequestDto.sourceBranch ?? project.mainBranchName;
const baselineBranch = createTestRequestDto.baselineBranchName ?? project.mainBranchName;
Comment on lines -86 to +84
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that sourceBranch was never used, so I replaced it with baselineBranchName to match existing parameter since that can already be set on the config and is send by the sdk-js if present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was used before to make manual test runs with comparison within branches not base on main branch
please, update docs on this method


const [mainBranchTestVariation, currentBranchTestVariation] = await Promise.all([
// search main branch variation
// search baseline branch variation
this.findUnique({
projectId: createTestRequestDto.projectId,
branchName: mainBranch,
branchName: baselineBranch,
...getTestVariationUniqueData(createTestRequestDto),
}),
// search current branch variation
createTestRequestDto.branchName !== mainBranch &&
createTestRequestDto.branchName !== baselineBranch &&
this.findUnique({
projectId: createTestRequestDto.projectId,
branchName: createTestRequestDto.branchName,
...getTestVariationUniqueData(createTestRequestDto),
}),
]);

if (!!currentBranchTestVariation) {
if (currentBranchTestVariation) {
if (mainBranchTestVariation && mainBranchTestVariation.updatedAt > currentBranchTestVariation.updatedAt) {
return mainBranchTestVariation;
}
return currentBranchTestVariation;
}

if (!!mainBranchTestVariation) {
if (mainBranchTestVariation) {
return mainBranchTestVariation;
}
}
Expand Down Expand Up @@ -248,4 +246,14 @@ export class TestVariationsService {
where: { id: baseline.id },
});
}

async findOldTestVariations(project: Project, dateRemoveAfter: Date) {
return await this.prismaService.$queryRaw<Project[]>`
SELECT * from public."TestVariation"
WHERE "projectId" = ${project.id}
AND "updatedAt" <= ${dateRemoveAfter}
AND "branchName" NOT LIKE ${project.mainBranchName}
AND "branchName" NOT SIMILAR TO ${project.protectedBranch}
`;
}
}
Loading
Loading