Skip to content

Commit

Permalink
feat(s3): group comparison images under new top-level directory (#573)
Browse files Browse the repository at this point in the history
  • Loading branch information
bensbigolbeard authored Jul 26, 2024
1 parent a6c7226 commit a4e5301
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 62 deletions.
5 changes: 3 additions & 2 deletions action/dist/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -29611,6 +29611,7 @@ var import_core = __toESM(require_core());
// ../shared/index.ts
var VISUAL_REGRESSION_CONTEXT = "Visual Regression";
var BASE_IMAGES_DIRECTORY = "base-images";
var NEW_IMAGES_DIRECTORY = "new-images";
var BASE_IMAGE_NAME = "base";
var NEW_IMAGE_NAME = "new";
var VISUAL_TESTS_FAILED_TO_EXECUTE = "Visual tests failed to execute successfully.";
Expand Down Expand Up @@ -29644,12 +29645,12 @@ var uploadAllImages = async () => {
return (0, import_bluebird.map)(
packagePaths,
(packagePath) => (0, import_exec.exec)(
`aws s3 cp ${screenshotsDirectory}/${packagePath} s3://${bucketName}/${commitHash}/${packagePath} --recursive`
`aws s3 cp ${screenshotsDirectory}/${packagePath} s3://${bucketName}/${NEW_IMAGES_DIRECTORY}/${commitHash}/${packagePath} --recursive`
)
);
}
return (0, import_exec.exec)(
`aws s3 cp ${screenshotsDirectory} s3://${bucketName}/${commitHash} --recursive`
`aws s3 cp ${screenshotsDirectory} s3://${bucketName}/${NEW_IMAGES_DIRECTORY}/${commitHash} --recursive`
);
};
var uploadBaseImages = async (newFilePaths) => {
Expand Down
2 changes: 1 addition & 1 deletion action/dist/main.js.map

Large diffs are not rendered by default.

11 changes: 8 additions & 3 deletions action/src/s3-operations.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { exec } from '@actions/exec';
import { getInput } from '@actions/core';
import { BASE_IMAGE_NAME, BASE_IMAGES_DIRECTORY, NEW_IMAGE_NAME } from 'shared';
import {
BASE_IMAGE_NAME,
BASE_IMAGES_DIRECTORY,
NEW_IMAGES_DIRECTORY,
NEW_IMAGE_NAME
} from 'shared';
import { map } from 'bluebird';
import * as path from 'path';

Expand Down Expand Up @@ -31,13 +36,13 @@ export const uploadAllImages = async () => {
if (packagePaths) {
return map(packagePaths, packagePath =>
exec(
`aws s3 cp ${screenshotsDirectory}/${packagePath} s3://${bucketName}/${commitHash}/${packagePath} --recursive`
`aws s3 cp ${screenshotsDirectory}/${packagePath} s3://${bucketName}/${NEW_IMAGES_DIRECTORY}/${commitHash}/${packagePath} --recursive`
)
);
}

return exec(
`aws s3 cp ${screenshotsDirectory} s3://${bucketName}/${commitHash} --recursive`
`aws s3 cp ${screenshotsDirectory} s3://${bucketName}/${NEW_IMAGES_DIRECTORY}/${commitHash} --recursive`
);
};

Expand Down
7 changes: 4 additions & 3 deletions action/test/run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { octokit } from '../src/octokit';
import { sync } from 'glob';
import {
BASE_IMAGES_DIRECTORY,
NEW_IMAGES_DIRECTORY,
VISUAL_REGRESSION_CONTEXT,
VISUAL_TESTS_FAILED_TO_EXECUTE
} from 'shared';
Expand Down Expand Up @@ -211,13 +212,13 @@ describe('main', () => {
`aws s3 cp s3://some-bucket/${BASE_IMAGES_DIRECTORY} path/to/screenshots --recursive`
);
expect(exec).toHaveBeenCalledWith(
'aws s3 cp path/to/screenshots/path/1 s3://some-bucket/sha/path/1 --recursive'
`aws s3 cp path/to/screenshots/path/1 s3://some-bucket/${NEW_IMAGES_DIRECTORY}/sha/path/1 --recursive`
);
expect(exec).toHaveBeenCalledWith(
'aws s3 cp path/to/screenshots/path/2 s3://some-bucket/sha/path/2 --recursive'
`aws s3 cp path/to/screenshots/path/2 s3://some-bucket/${NEW_IMAGES_DIRECTORY}/sha/path/2 --recursive`
);
expect(exec).not.toHaveBeenCalledWith(
'aws s3 cp path/to/screenshots s3://some-bucket/sha --recursive'
`aws s3 cp path/to/screenshots s3://some-bucket/${NEW_IMAGES_DIRECTORY}/sha --recursive`
);
});

Expand Down
6 changes: 4 additions & 2 deletions app/backend/src/getGroupedKeys.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { TRPCError } from '@trpc/server';
import { join, parse } from 'path';
import { groupBy } from 'lodash';
import { NEW_IMAGE_NAME } from 'shared';
import { NEW_IMAGE_NAME, NEW_IMAGES_DIRECTORY } from 'shared';
import { getKeysFromS3 } from './getKeysFromS3';

export const getGroupedKeys = async (hash: string, bucket: string) => {
Expand Down Expand Up @@ -42,5 +42,7 @@ export const getGroupedKeys = async (hash: string, bucket: string) => {
};

const getPathFromKey = (path: string) => {
return path.slice(path.indexOf('/') + 1);
const trimmedPath = path.replace(`${NEW_IMAGES_DIRECTORY}/`, '');

return trimmedPath.slice(trimmedPath.indexOf('/') + 1);
};
5 changes: 4 additions & 1 deletion app/backend/src/getKeysFromS3.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { NEW_IMAGES_DIRECTORY } from 'shared';
import { S3Client } from './s3Client';

// Info on working with nested object path prefixes: https://realguess.net/2014/05/24/amazon-s3-delimiter-and-prefix/#Prefix
export const getKeysFromS3 = async (hash: string, bucket: string) => {
const response = await S3Client.listObjectsV2({
Bucket: bucket,
Prefix: hash
Prefix: `${NEW_IMAGES_DIRECTORY}/${hash}/`,
Delimiter: '/'
});

return (
Expand Down
12 changes: 9 additions & 3 deletions app/backend/src/updateBaseImagesInS3.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { S3Client } from './s3Client';
import { BASE_IMAGE_NAME, BASE_IMAGES_DIRECTORY, NEW_IMAGE_NAME } from 'shared';
import {
BASE_IMAGE_NAME,
BASE_IMAGES_DIRECTORY,
NEW_IMAGE_NAME,
NEW_IMAGES_DIRECTORY
} from 'shared';
import { findReasonToPreventBaseImageUpdate } from './findReasonToPreventBaseImageUpdate';
import { TRPCError } from '@trpc/server';
import { UpdateBaseImagesInput } from './schema';
Expand Down Expand Up @@ -36,8 +41,9 @@ export const filterNewImages = (s3Paths: string[]) => {

export const getBaseImagePaths = (newImagePaths: string[]) => {
return newImagePaths.map(path => {
const commitHash = path.split('/')[0] ?? '';
return path
const trimmedPath = path.replace(`${NEW_IMAGES_DIRECTORY}/`, '');
const commitHash = trimmedPath.split('/')[0] ?? '';
return trimmedPath
.replace(commitHash, BASE_IMAGES_DIRECTORY)
.replace(NEW_IMAGE_NAME, BASE_IMAGE_NAME);
});
Expand Down
59 changes: 31 additions & 28 deletions app/backend/test/getGroupedKeys.test.ts
Original file line number Diff line number Diff line change
@@ -1,73 +1,76 @@
import { NEW_IMAGES_DIRECTORY } from 'shared';
import { getGroupedKeys } from '../src/getGroupedKeys';
import { getKeysFromS3 } from '../src/getKeysFromS3';
import { expect } from '@jest/globals';

jest.mock('../src/getKeysFromS3');

const pathPrefix = `${NEW_IMAGES_DIRECTORY}/hash`;

describe('getGroupedKeys', () => {
it('returns only the keys where there is a base, new, and diff', async () => {
(getKeysFromS3 as jest.Mock).mockResolvedValue([
'hash/EXTRA_LARGE/srpPage/base.png',
'hash/SMALL/srpPage/base.png',
'hash/EXTRA_LARGE/pdpPage/base.png',
'hash/EXTRA_LARGE/pdpPage/diff.png',
'hash/EXTRA_LARGE/pdpPage/new.png',
`${pathPrefix}/EXTRA_LARGE/srpPage/base.png`,
`${pathPrefix}/SMALL/srpPage/base.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/base.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/new.png`,
'ome/actions-runner/something'
]);
const paths = await getGroupedKeys('hash', 'bucket');
expect(paths).toEqual([
{
title: 'EXTRA_LARGE/pdpPage',
keys: [
'hash/EXTRA_LARGE/pdpPage/base.png',
'hash/EXTRA_LARGE/pdpPage/diff.png',
'hash/EXTRA_LARGE/pdpPage/new.png'
`${pathPrefix}/EXTRA_LARGE/pdpPage/base.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/new.png`
]
}
]);
});

it('returns keys where there is a new image but no base image', async () => {
(getKeysFromS3 as jest.Mock).mockResolvedValue([
'hash/EXTRA_LARGE/srpPage/base.png',
'hash/SMALL/pdpPage/new.png',
'hash/EXTRA_LARGE/pdpPage/base.png'
`${pathPrefix}/EXTRA_LARGE/srpPage/base.png`,
`${pathPrefix}/SMALL/pdpPage/new.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/base.png`
]);
const paths = await getGroupedKeys('hash', 'bucket');
expect(paths).toEqual([
{
title: 'SMALL/pdpPage',
keys: ['hash/SMALL/pdpPage/new.png']
keys: [`${pathPrefix}/SMALL/pdpPage/new.png`]
}
]);
});

it('returns multiple pages', async () => {
(getKeysFromS3 as jest.Mock).mockResolvedValue([
'hash/EXTRA_LARGE/srpPage/base.png',
'hash/SMALL/srpPage/base.png',
'hash/SMALL/srpPage/diff.png',
'hash/SMALL/srpPage/new.png',
'hash/EXTRA_LARGE/pdpPage/base.png',
'hash/EXTRA_LARGE/pdpPage/diff.png',
'hash/EXTRA_LARGE/pdpPage/new.png'
`${pathPrefix}/EXTRA_LARGE/srpPage/base.png`,
`${pathPrefix}/SMALL/srpPage/base.png`,
`${pathPrefix}/SMALL/srpPage/diff.png`,
`${pathPrefix}/SMALL/srpPage/new.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/base.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/new.png`
]);
const paths = await getGroupedKeys('hash', 'bucket');
expect(paths).toEqual([
{
title: 'SMALL/srpPage',
keys: [
'hash/SMALL/srpPage/base.png',
'hash/SMALL/srpPage/diff.png',
'hash/SMALL/srpPage/new.png'
`${pathPrefix}/SMALL/srpPage/base.png`,
`${pathPrefix}/SMALL/srpPage/diff.png`,
`${pathPrefix}/SMALL/srpPage/new.png`
]
},
{
title: 'EXTRA_LARGE/pdpPage',
keys: [
'hash/EXTRA_LARGE/pdpPage/base.png',
'hash/EXTRA_LARGE/pdpPage/diff.png',
'hash/EXTRA_LARGE/pdpPage/new.png'
`${pathPrefix}/EXTRA_LARGE/pdpPage/base.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/new.png`
]
}
]);
Expand All @@ -82,9 +85,9 @@ describe('getGroupedKeys', () => {

it('tells us if there are no new or diff images associated with the commit hash', async () => {
(getKeysFromS3 as jest.Mock).mockResolvedValue([
'hash/EXTRA_LARGE/srpPage/base.png',
'hash/SMALL/srpPage/base.png',
'hash/EXTRA_LARGE/pdpPage/base.png'
`${pathPrefix}/EXTRA_LARGE/srpPage/base.png`,
`${pathPrefix}/SMALL/srpPage/base.png`,
`${pathPrefix}/EXTRA_LARGE/pdpPage/base.png`
]);
await expect(() => getGroupedKeys('hash', 'bucket')).rejects.toThrow(
'There was no new or diff images associated with the commit hash.\nThis might be because the tests failed before a picture could be taken and it could be compared to the base.'
Expand Down
37 changes: 18 additions & 19 deletions app/backend/test/updateBaseImagesInS3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
getBaseImagePaths,
updateBaseImagesInS3
} from '../src/updateBaseImagesInS3';
import { BASE_IMAGES_DIRECTORY } from 'shared';
import { BASE_IMAGES_DIRECTORY, NEW_IMAGES_DIRECTORY } from 'shared';
import { findReasonToPreventBaseImageUpdate } from '../src/findReasonToPreventBaseImageUpdate';
import { updateCommitStatus } from '../src/updateCommitStatus';
import { expect } from '@jest/globals';
Expand All @@ -14,36 +14,35 @@ jest.mock('../src/findReasonToPreventBaseImageUpdate');
jest.mock('../src/s3Client');
jest.mock('../src/updateCommitStatus');

const pathPrefix = `${NEW_IMAGES_DIRECTORY}/030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5`;
describe('filterNewImages', () => {
it('should filter only the new images from the given paths', () => {
const newImage =
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/new.png';
const diffImage =
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/diff.png';
const newImage = `${pathPrefix}/SMALL/pdpPage/new.png`;
const diffImage = `${pathPrefix}/SMALL/pdpPage/diff.png`;
const images = filterNewImages([newImage, diffImage]);
expect(images).toHaveLength(1);
expect(images[0]).toBe(newImage);
});

it('should filter only the new images when many paths given', () => {
const images = filterNewImages([
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/new.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/diff.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/LARGE/srpPage/new.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/LARGE/pdpPage/base.png'
`${pathPrefix}/SMALL/pdpPage/new.png`,
`${pathPrefix}/SMALL/pdpPage/diff.png`,
`${pathPrefix}/LARGE/srpPage/new.png`,
`${pathPrefix}/LARGE/pdpPage/base.png`
]);
expect(images).toEqual([
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/new.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/LARGE/srpPage/new.png'
`${pathPrefix}/SMALL/pdpPage/new.png`,
`${pathPrefix}/LARGE/srpPage/new.png`
]);
});
});

describe('getBaseImagesPaths', () => {
it('should return the base image paths given the new image paths', () => {
const paths = [
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/new.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/LARGE/srpPage/new.png'
`${pathPrefix}/SMALL/pdpPage/new.png`,
`${pathPrefix}/LARGE/srpPage/new.png`
];
const result = getBaseImagePaths(paths);
expect(result).toEqual([
Expand All @@ -58,22 +57,22 @@ describe('updateBaseImagesInS3', () => {
const expectedBucket = 'expected-bucket-name';
await replaceImagesInS3(
[
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/new.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/srpPage/new.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/srpPage/base.png',
'030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/base.png'
`${pathPrefix}/SMALL/pdpPage/new.png`,
`${pathPrefix}/SMALL/srpPage/new.png`,
`${pathPrefix}/SMALL/srpPage/base.png`,
`${pathPrefix}/SMALL/pdpPage/base.png`
],
expectedBucket
);
expect(S3Client.copyObject).toHaveBeenCalledWith({
Bucket: expectedBucket,
CopySource: `${expectedBucket}/030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/pdpPage/new.png`,
CopySource: `${expectedBucket}/${pathPrefix}/SMALL/pdpPage/new.png`,
Key: `${BASE_IMAGES_DIRECTORY}/SMALL/pdpPage/base.png`,
ACL: 'bucket-owner-full-control'
});
expect(S3Client.copyObject).toHaveBeenCalledWith({
Bucket: expectedBucket,
CopySource: `${expectedBucket}/030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5/SMALL/srpPage/new.png`,
CopySource: `${expectedBucket}/${pathPrefix}/SMALL/srpPage/new.png`,
Key: `${BASE_IMAGES_DIRECTORY}/SMALL/srpPage/base.png`,
ACL: 'bucket-owner-full-control'
});
Expand Down
1 change: 1 addition & 0 deletions shared/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export const VISUAL_REGRESSION_CONTEXT = 'Visual Regression';
export const BASE_IMAGES_DIRECTORY = 'base-images';
export const NEW_IMAGES_DIRECTORY = 'new-images';
export const BASE_IMAGE_NAME = 'base';
export const DIFF_IMAGE_NAME = 'diff';
export const NEW_IMAGE_NAME = 'new';
Expand Down

0 comments on commit a4e5301

Please sign in to comment.