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

fix: pwt static image accepter #596

Merged
merged 2 commits into from
Aug 29, 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
2 changes: 1 addition & 1 deletion .github/workflows/node.js.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:

strategy:
matrix:
node-version: [18.x, 20.x, 22.x]
node-version: [18.x, 20.x, 22.6] # https://github.com/nodejs/node/issues/54532

steps:
- uses: actions/checkout@v2
Expand Down
12 changes: 8 additions & 4 deletions lib/adapters/test-result/playwright.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ export type PwtImageDiffError = ExtendedError<{snapshotName: string, diffCluster

export type PwtNoRefImageError = ExtendedError<{snapshotName: string}>;

export type PlaywrightImageFile = ImageFile & { relativePath: string };
Copy link
Member Author

@KuznetsovRoman KuznetsovRoman Aug 28, 2024

Choose a reason for hiding this comment

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

I don't want to pollute ImageFile type, which is used in Testplane and generally has no guaranteed "relativePath" property


export enum PwtTestStatus {
PASSED = 'passed',
FAILED = 'failed',
Expand Down Expand Up @@ -139,15 +141,15 @@ const extractToMatchScreenshotError = (result: PlaywrightTestResult, {state, exp
} : null;
};

const getImageData = (attachment: PlaywrightAttachment | undefined): ImageFile | null => {
const getImageData = (attachment: PlaywrightAttachment | undefined): PlaywrightImageFile | null => {
if (!attachment) {
return null;
}

return {
path: attachment.path as string,
size: !attachment.size ? _.pick(sizeOf(attachment.path as string), ['height', 'width']) as ImageSize : attachment.size,
...(attachment.relativePath ? {relativePath: attachment.relativePath} : {})
relativePath: attachment.relativePath || path.relative(process.cwd(), attachment.path as string)
};
};

Expand Down Expand Up @@ -239,7 +241,7 @@ export class PlaywrightTestResultAdapter implements ReporterTestResult {

const error = extractToMatchScreenshotError(this._testResult, {state, expectedAttachment, diffAttachment, actualAttachment}) || this.error;

// We don't provide refImg here, because on some pwt versions it's impossible to provide correct path:
// We now provide refImg here, though on some pwt versions it's impossible to provide correct path:
// older pwt versions had test-results directory in expected path instead of project directory.
if (error?.name === ErrorName.IMAGE_DIFF && expectedImg && diffImg && actualImg) {
return {
Expand All @@ -248,6 +250,7 @@ export class PlaywrightTestResultAdapter implements ReporterTestResult {
diffImg,
actualImg,
expectedImg,
refImg: _.clone(expectedImg),
diffClusters: _.get(error, 'diffClusters', []),
// TODO: extract diffOptions from config
diffOptions: {current: actualImg.path, reference: expectedImg.path, ...DEFAULT_DIFF_OPTIONS}
Expand All @@ -257,7 +260,8 @@ export class PlaywrightTestResultAdapter implements ReporterTestResult {
status: ERROR,
stateName: state,
error: _.pick(error, ['message', 'name', 'stack']),
actualImg
actualImg,
...(expectedImg ? {refImg: _.clone(expectedImg)} : {})
} satisfies ImageInfoNoRef;
} else if (expectedAttachment?.isUpdated && expectedImg && actualImg) {
return {
Expand Down
4 changes: 4 additions & 0 deletions lib/common-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ export const isNoRefImageError = (error?: unknown): error is NoRefImageError =>
return (error as {name?: string})?.name === ErrorName.NO_REF_IMAGE;
};

export const isImageError = (error?: unknown): boolean => {
return isAssertViewError(error) || isImageDiffError(error) || isNoRefImageError(error);
};

export const hasNoRefImageErrors = ({assertViewResults = []}: {assertViewResults?: {name?: string}[]}): boolean => {
return assertViewResults.some((assertViewResult) => isNoRefImageError(assertViewResult));
};
Expand Down
6 changes: 3 additions & 3 deletions lib/static/modules/reducers/tree/helpers.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {get, pick, set, uniq, isEmpty} from 'lodash';
import {EXPAND_ALL, EXPAND_ERRORS, EXPAND_RETRIES} from '../../../../constants/expand-modes';
import {ensureDiffProperty, getUpdatedProperty} from '../../utils/state';
import {determineFinalStatus, determineStatus, isAssertViewError, isUpdatedStatus} from '../../../../common-utils';
import {determineFinalStatus, determineStatus, isImageError, isUpdatedStatus} from '../../../../common-utils';
import {updateParentSuitesStatus, calcSuitesShowness, calcSuitesOpenness} from './nodes/suites';
import {calcBrowsersShowness, calcBrowsersOpenness} from './nodes/browsers';
import {changeImageStatus, calcImagesOpenness} from './nodes/images';
Expand Down Expand Up @@ -110,9 +110,9 @@ export function updateImagesStatus({tree, view, images, imageIdsArray, newStatus
.map(imageId => getUpdatedProperty(tree, diff, ['images', 'byId', imageId, 'status']));

const finalStatus = determineFinalStatus(imageStatuses);
const trueResultStatus = isAssertViewError(result.error) && finalStatus === ERROR ? FAIL : finalStatus;
const trueResultStatus = isImageError(result.error) && finalStatus === ERROR ? FAIL : finalStatus;

if (isAssertViewError(result.error) && trueResultStatus !== result.status) {
if (isImageError(result.error) && trueResultStatus !== result.status) {
Comment on lines +113 to +115
Copy link
Member Author

Choose a reason for hiding this comment

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

In Testplane, we have a contract: if test result has image errors, test result has "AssertViewError", and images can have either ImageDiffError or NoRefImageError.

In Playwright adapter, though, we can have ImageDiffError or NoRefImageError as result.error. Looks like we could encounter other issues because of that.

browserIds.push(result.parentId);
changeNodeState(tree.results.byId, resultId, {status: trueResultStatus}, diff.results.byId);
}
Expand Down
42 changes: 36 additions & 6 deletions test/unit/lib/adapters/test-result/playwright.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
DEFAULT_DIFF_OPTIONS,
ImageTitleEnding,
PlaywrightAttachment,
PlaywrightImageFile,
PwtTestStatus
} from 'lib/adapters/test-result/playwright';
import {ErrorName} from 'lib/errors';
Expand Down Expand Up @@ -189,14 +190,35 @@ describe('PlaywrightTestResultAdapter', () => {
assert.equal(adapter.imagesInfo.length, 2);
assert.deepEqual(adapter.imagesInfo.find(info => (info as ImageInfoDiff).stateName === undefined), {
status: ERROR,
actualImg: {path: `test-results/test-name-1.png`, size: {height: 100, width: 200}}
actualImg: {
path: `test-results/test-name-1.png`,
relativePath: `test-results/test-name-1.png`,
size: {height: 100, width: 200}
} as PlaywrightImageFile
});
assert.deepEqual(adapter.imagesInfo.find(info => (info as ImageInfoDiff).stateName === 'header'), {
status: FAIL,
stateName: 'header',
actualImg: {path: `test-results/header-actual.png`, size: {height: 100, width: 200}},
expectedImg: {path: 'project-dir/header-expected.png', size: {height: 100, width: 200}},
diffImg: {path: 'test-results/header-diff.png', size: {height: 100, width: 200}},
actualImg: {
path: `test-results/header-actual.png`,
relativePath: `test-results/header-actual.png`,
size: {height: 100, width: 200}
} as PlaywrightImageFile,
expectedImg: {
path: 'project-dir/header-expected.png',
relativePath: 'project-dir/header-expected.png',
size: {height: 100, width: 200}
} as PlaywrightImageFile,
refImg: {
path: 'project-dir/header-expected.png',
relativePath: 'project-dir/header-expected.png',
size: {height: 100, width: 200}
} as PlaywrightImageFile,
diffImg: {
path: 'test-results/header-diff.png',
relativePath: 'test-results/header-diff.png',
size: {height: 100, width: 200}
} as PlaywrightImageFile,
diffClusters: [],
diffOptions: {current: 'test-results/header-actual.png', reference: 'project-dir/header-expected.png', ...DEFAULT_DIFF_OPTIONS}
});
Expand All @@ -214,7 +236,11 @@ describe('PlaywrightTestResultAdapter', () => {
assert.equal(adapter.imagesInfo.length, 2);
assert.deepEqual(adapter.imagesInfo.find(info => (info as ImageInfoNoRef).stateName === undefined), {
status: ERROR,
actualImg: {path: `test-results/test-name-1.png`, size: {height: 100, width: 200}}
actualImg: {
path: `test-results/test-name-1.png`,
relativePath: `test-results/test-name-1.png`,
size: {height: 100, width: 200}
} as PlaywrightImageFile
});
assert.deepEqual(adapter.imagesInfo.find(info => (info as ImageInfoNoRef).stateName === 'header'), {
status: ERROR,
Expand All @@ -224,7 +250,11 @@ describe('PlaywrightTestResultAdapter', () => {
message: 'snapshot doesn\'t exist at some.png',
stack: ''
},
actualImg: {path: `test-results/header-actual.png`, size: {height: 100, width: 200}}
actualImg: {
path: `test-results/header-actual.png`,
relativePath: `test-results/header-actual.png`,
size: {height: 100, width: 200}
} as PlaywrightImageFile
});
});
});
Expand Down
Loading