From aa3414af6100229707862cdd8ca37efc4feaac73 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 25 Oct 2024 09:15:20 -0700 Subject: [PATCH 1/4] chore: unify toHaveScreenshot error formatting --- packages/html-reporter/src/testResultView.tsx | 3 +- packages/playwright-core/src/client/page.ts | 2 +- .../playwright-core/src/protocol/validator.ts | 1 + packages/playwright-core/src/server/page.ts | 3 +- .../src/matchers/toMatchSnapshot.ts | 36 +++++++++---------- packages/protocol/src/channels.ts | 1 + packages/protocol/src/protocol.yml | 1 + tests/playwright-test/reporter-html.spec.ts | 4 ++- 8 files changed, 29 insertions(+), 22 deletions(-) diff --git a/packages/html-reporter/src/testResultView.tsx b/packages/html-reporter/src/testResultView.tsx index bb18422dd084f..3a562f3fcf789 100644 --- a/packages/html-reporter/src/testResultView.tsx +++ b/packages/html-reporter/src/testResultView.tsx @@ -152,7 +152,8 @@ export const TestResultView: React.FC<{ function classifyErrors(testErrors: string[], diffs: ImageDiff[]) { return testErrors.map(error => { - if (error.includes('Screenshot comparison failed:')) { + const firstLine = error.split('\n')[0]; + if (firstLine.includes('toHaveScreenshot') || firstLine.includes('toMatchSnapshot')) { const matchingDiff = diffs.find(diff => { const attachmentName = diff.actual?.attachment.name; return attachmentName && error.includes(attachmentName); diff --git a/packages/playwright-core/src/client/page.ts b/packages/playwright-core/src/client/page.ts index 6654294edd3e4..87317c9018fc1 100644 --- a/packages/playwright-core/src/client/page.ts +++ b/packages/playwright-core/src/client/page.ts @@ -589,7 +589,7 @@ export class Page extends ChannelOwner implements api.Page return result.binary; } - async _expectScreenshot(options: ExpectScreenshotOptions): Promise<{ actual?: Buffer, previous?: Buffer, diff?: Buffer, errorMessage?: string, log?: string[]}> { + async _expectScreenshot(options: ExpectScreenshotOptions): Promise<{ actual?: Buffer, previous?: Buffer, diff?: Buffer, errorMessage?: string, log?: string[], timeout?: number}> { const mask = options?.mask ? options?.mask.map(locator => ({ frame: (locator as Locator)._frame._channel, selector: (locator as Locator)._selector, diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index 7bad26f4987b9..b6dbc0bbf7eda 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -1193,6 +1193,7 @@ scheme.PageExpectScreenshotResult = tObject({ errorMessage: tOptional(tString), actual: tOptional(tBinary), previous: tOptional(tBinary), + timeout: tOptional(tNumber), log: tOptional(tArray(tString)), }); scheme.PageScreenshotParams = tObject({ diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index b78bd91ee17cc..d46df2df9b33c 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -674,11 +674,12 @@ export class Page extends SdkObject { throw e; let errorMessage = e.message; if (e instanceof TimeoutError && intermediateResult?.previous) - errorMessage = `Failed to take two consecutive stable screenshots. ${e.message}`; + errorMessage = `Failed to take two consecutive stable screenshots.`; return { log: e.message ? [...metadata.log, e.message] : metadata.log, ...intermediateResult, errorMessage, + ...((e instanceof TimeoutError) ? { timeout: callTimeout } : {}), }; }); } diff --git a/packages/playwright/src/matchers/toMatchSnapshot.ts b/packages/playwright/src/matchers/toMatchSnapshot.ts index 86504062d30d7..edcead15cad77 100644 --- a/packages/playwright/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchSnapshot.ts @@ -18,7 +18,7 @@ import type { Locator, Page } from 'playwright-core'; import type { ExpectScreenshotOptions, Page as PageEx } from 'playwright-core/lib/client/page'; import { currentTestInfo } from '../common/globals'; import type { ImageComparatorOptions, Comparator } from 'playwright-core/lib/utils'; -import { getComparator, sanitizeForFilePath } from 'playwright-core/lib/utils'; +import { getComparator, isString, sanitizeForFilePath } from 'playwright-core/lib/utils'; import { addSuffixToFilePath, trimLongString, callLogText, @@ -31,7 +31,7 @@ import path from 'path'; import { mime } from 'playwright-core/lib/utilsBundle'; import type { TestInfoImpl } from '../worker/testInfo'; import type { ExpectMatcherState } from '../../types/test'; -import type { MatcherResult } from './matcherHint'; +import { matcherHint, type MatcherResult } from './matcherHint'; import type { FullProjectInternal } from '../common/config'; type NameOrSegments = string | string[]; @@ -250,16 +250,10 @@ class SnapshotHelper { expected: Buffer | string | undefined, previous: Buffer | string | undefined, diff: Buffer | string | undefined, - diffError: string | undefined, - log: string[] | undefined, - title = `${this.kind} comparison failed:`): ImageMatcherResult { - const output = [ - colors.red(title), - '', - ]; - if (diffError) - output.push(indent(diffError, ' ')); - + header: string, + diffError: string, + log: string[] | undefined): ImageMatcherResult { + const output = [`${header}${indent(diffError, ' ')}`]; if (expected !== undefined) { // Copy the expectation inside the `test-results/` folder for backwards compatibility, // so that one can upload `test-results/` directory and have all the data inside. @@ -338,7 +332,9 @@ export function toMatchSnapshot( return helper.createMatcherResult(helper.expectedPath + ' running with --update-snapshots, writing actual.', true); } - return helper.handleDifferent(received, expected, undefined, result.diff, result.errorMessage, undefined); + const receiver = isString(received) ? 'string' : 'Buffer'; + const header = matcherHint(this, undefined, 'toMatchSnapshot', receiver, undefined, undefined); + return helper.handleDifferent(received, expected, undefined, result.diff, header, result.errorMessage, undefined); } export function toHaveScreenshotStepTitle( @@ -410,13 +406,16 @@ export async function toHaveScreenshot( if (helper.updateSnapshots === 'none' && !hasSnapshot) return helper.createMatcherResult(`A snapshot doesn't exist at ${helper.expectedPath}.`, false); + const receiver = locator ? 'locator' : 'page'; if (!hasSnapshot) { // Regenerate a new screenshot by waiting until two screenshots are the same. - const { actual, previous, diff, errorMessage, log } = await page._expectScreenshot(expectScreenshotOptions); + const { actual, previous, diff, errorMessage, log, timeout } = await page._expectScreenshot(expectScreenshotOptions); // We tried re-generating new snapshot but failed. // This can be due to e.g. spinning animation, so we want to show it as a diff. - if (errorMessage) - return helper.handleDifferent(actual, undefined, previous, diff, undefined, log, errorMessage); + if (errorMessage) { + const header = matcherHint(this, locator, 'toHaveScreenshot', receiver, undefined, undefined, timeout); + return helper.handleDifferent(actual, undefined, previous, diff, header, errorMessage, log); + } // We successfully generated new screenshot. return helper.handleMissing(actual!); @@ -427,7 +426,7 @@ export async function toHaveScreenshot( // - regular matcher (i.e. not a `.not`) // - perhaps an 'all' flag to update non-matching screenshots expectScreenshotOptions.expected = await fs.promises.readFile(helper.expectedPath); - const { actual, previous, diff, errorMessage, log } = await page._expectScreenshot(expectScreenshotOptions); + const { actual, previous, diff, errorMessage, log, timeout } = await page._expectScreenshot(expectScreenshotOptions); if (!errorMessage) return helper.handleMatching(); @@ -440,7 +439,8 @@ export async function toHaveScreenshot( return helper.createMatcherResult(helper.expectedPath + ' running with --update-snapshots, writing actual.', true); } - return helper.handleDifferent(actual, expectScreenshotOptions.expected, previous, diff, errorMessage, log); + const header = matcherHint(this, undefined, 'toHaveScreenshot', receiver, undefined, undefined, timeout); + return helper.handleDifferent(actual, expectScreenshotOptions.expected, previous, diff, header, errorMessage, log); } function writeFileSync(aPath: string, content: Buffer | string) { diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 7fcb815468560..60afe65b255a9 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -2193,6 +2193,7 @@ export type PageExpectScreenshotResult = { errorMessage?: string, actual?: Binary, previous?: Binary, + timeout?: number, log?: string[], }; export type PageScreenshotParams = { diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index 98428cae2f75b..d16931dd05f67 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -1501,6 +1501,7 @@ Page: errorMessage: string? actual: binary? previous: binary? + timeout: number? log: type: array? items: string diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index 11169fadffa0e..51734be9ffee7 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -330,7 +330,9 @@ for (const useIntermediateMergeReport of [true, false] as const) { await expect(page.locator('text=Image mismatch')).toHaveCount(1); await expect(page.locator('text=Snapshot mismatch')).toHaveCount(0); await expect(page.locator('.chip-header', { hasText: 'Screenshots' })).toHaveCount(0); - await expect(page.getByTestId('test-result-image-mismatch-tabs').locator('div')).toHaveText([ + const errorChip = page.getByTestId('test-screenshot-error-view'); + await expect(errorChip).toContainText('Failed to take two consecutive stable screenshots.'); + await expect(errorChip.getByTestId('test-result-image-mismatch-tabs').locator('div')).toHaveText([ 'Diff', 'Actual', 'Previous', From 70ddc7ebe27f34aa464ddaf93613e6a7a9999941 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 25 Oct 2024 09:55:29 -0700 Subject: [PATCH 2/4] Update expectations --- tests/page/expect-matcher-result.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/page/expect-matcher-result.spec.ts b/tests/page/expect-matcher-result.spec.ts index 7767ecf5f62a0..fe983c6f6386f 100644 --- a/tests/page/expect-matcher-result.spec.ts +++ b/tests/page/expect-matcher-result.spec.ts @@ -295,7 +295,7 @@ test('toHaveScreenshot should populate matcherResult', async ({ page, server, is actual: expect.stringContaining('screenshot-sanity-actual'), expected: expect.stringContaining('screenshot-sanity-'), diff: expect.stringContaining('screenshot-sanity-diff'), - message: expect.stringContaining(`Screenshot comparison failed`), + message: expect.stringContaining(`expect(page).toHaveScreenshot(expected)`), name: 'toHaveScreenshot', pass: false, log: expect.any(Array), @@ -303,7 +303,7 @@ test('toHaveScreenshot should populate matcherResult', async ({ page, server, is printedReceived: expect.stringContaining('screenshot-sanity-actual'), }); - expect.soft(stripAnsi(e.toString())).toContain(`Error: Screenshot comparison failed: + expect.soft(stripAnsi(e.toString())).toContain(`Error: expect(page).toHaveScreenshot(expected) 23362 pixels (ratio 0.10 of all image pixels) are different. From d2ee0916d262b53abd9cd6b8f60445903d97046d Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 25 Oct 2024 10:58:40 -0700 Subject: [PATCH 3/4] return timedOut bool --- packages/playwright-core/src/client/page.ts | 3 ++- packages/playwright-core/src/protocol/validator.ts | 4 ++-- packages/playwright-core/src/server/page.ts | 2 +- packages/playwright/src/matchers/toMatchSnapshot.ts | 11 ++++++----- packages/protocol/src/channels.ts | 5 ++--- packages/protocol/src/protocol.yml | 4 ++-- 6 files changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/playwright-core/src/client/page.ts b/packages/playwright-core/src/client/page.ts index 87317c9018fc1..c8d816f62ac9e 100644 --- a/packages/playwright-core/src/client/page.ts +++ b/packages/playwright-core/src/client/page.ts @@ -63,6 +63,7 @@ type PDFOptions = Omit & export type ExpectScreenshotOptions = Omit & { expected?: Buffer, locator?: api.Locator, + timeout: number, isNot: boolean, mask?: api.Locator[], }; @@ -589,7 +590,7 @@ export class Page extends ChannelOwner implements api.Page return result.binary; } - async _expectScreenshot(options: ExpectScreenshotOptions): Promise<{ actual?: Buffer, previous?: Buffer, diff?: Buffer, errorMessage?: string, log?: string[], timeout?: number}> { + async _expectScreenshot(options: ExpectScreenshotOptions): Promise<{ actual?: Buffer, previous?: Buffer, diff?: Buffer, errorMessage?: string, log?: string[], timedOut?: boolean}> { const mask = options?.mask ? options?.mask.map(locator => ({ frame: (locator as Locator)._frame._channel, selector: (locator as Locator)._selector, diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index b6dbc0bbf7eda..fd30bbba09bd1 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -1165,7 +1165,7 @@ scheme.PageReloadResult = tObject({ }); scheme.PageExpectScreenshotParams = tObject({ expected: tOptional(tBinary), - timeout: tOptional(tNumber), + timeout: tNumber, isNot: tBoolean, locator: tOptional(tObject({ frame: tChannel(['Frame']), @@ -1193,7 +1193,7 @@ scheme.PageExpectScreenshotResult = tObject({ errorMessage: tOptional(tString), actual: tOptional(tBinary), previous: tOptional(tBinary), - timeout: tOptional(tNumber), + timedOut: tOptional(tBoolean), log: tOptional(tArray(tString)), }); scheme.PageScreenshotParams = tObject({ diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index d46df2df9b33c..55f99250c8e42 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -679,7 +679,7 @@ export class Page extends SdkObject { log: e.message ? [...metadata.log, e.message] : metadata.log, ...intermediateResult, errorMessage, - ...((e instanceof TimeoutError) ? { timeout: callTimeout } : {}), + timedOut: (e instanceof TimeoutError), }; }); } diff --git a/packages/playwright/src/matchers/toMatchSnapshot.ts b/packages/playwright/src/matchers/toMatchSnapshot.ts index edcead15cad77..24b387f3ea45d 100644 --- a/packages/playwright/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchSnapshot.ts @@ -370,6 +370,7 @@ export async function toHaveScreenshot( throw new Error(`Screenshot name "${path.basename(helper.expectedPath)}" must have '.png' extension`); expectTypes(pageOrLocator, ['Page', 'Locator'], 'toHaveScreenshot'); const style = await loadScreenshotStyles(helper.options.stylePath); + const timeout = helper.options.timeout ?? this.timeout; const expectScreenshotOptions: ExpectScreenshotOptions = { locator, animations: helper.options.animations ?? 'disabled', @@ -382,7 +383,7 @@ export async function toHaveScreenshot( scale: helper.options.scale ?? 'css', style, isNot: !!this.isNot, - timeout: helper.options.timeout ?? this.timeout, + timeout, comparator: helper.options.comparator, maxDiffPixels: helper.options.maxDiffPixels, maxDiffPixelRatio: helper.options.maxDiffPixelRatio, @@ -409,11 +410,11 @@ export async function toHaveScreenshot( const receiver = locator ? 'locator' : 'page'; if (!hasSnapshot) { // Regenerate a new screenshot by waiting until two screenshots are the same. - const { actual, previous, diff, errorMessage, log, timeout } = await page._expectScreenshot(expectScreenshotOptions); + const { actual, previous, diff, errorMessage, log, timedOut } = await page._expectScreenshot(expectScreenshotOptions); // We tried re-generating new snapshot but failed. // This can be due to e.g. spinning animation, so we want to show it as a diff. if (errorMessage) { - const header = matcherHint(this, locator, 'toHaveScreenshot', receiver, undefined, undefined, timeout); + const header = matcherHint(this, locator, 'toHaveScreenshot', receiver, undefined, undefined, timedOut ? timeout : undefined); return helper.handleDifferent(actual, undefined, previous, diff, header, errorMessage, log); } @@ -426,7 +427,7 @@ export async function toHaveScreenshot( // - regular matcher (i.e. not a `.not`) // - perhaps an 'all' flag to update non-matching screenshots expectScreenshotOptions.expected = await fs.promises.readFile(helper.expectedPath); - const { actual, previous, diff, errorMessage, log, timeout } = await page._expectScreenshot(expectScreenshotOptions); + const { actual, previous, diff, errorMessage, log, timedOut } = await page._expectScreenshot(expectScreenshotOptions); if (!errorMessage) return helper.handleMatching(); @@ -439,7 +440,7 @@ export async function toHaveScreenshot( return helper.createMatcherResult(helper.expectedPath + ' running with --update-snapshots, writing actual.', true); } - const header = matcherHint(this, undefined, 'toHaveScreenshot', receiver, undefined, undefined, timeout); + const header = matcherHint(this, undefined, 'toHaveScreenshot', receiver, undefined, undefined, timedOut ? timeout : undefined); return helper.handleDifferent(actual, expectScreenshotOptions.expected, previous, diff, header, errorMessage, log); } diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 60afe65b255a9..43e878a97a0d9 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -2141,7 +2141,7 @@ export type PageReloadResult = { }; export type PageExpectScreenshotParams = { expected?: Binary, - timeout?: number, + timeout: number, isNot: boolean, locator?: { frame: FrameChannel, @@ -2166,7 +2166,6 @@ export type PageExpectScreenshotParams = { }; export type PageExpectScreenshotOptions = { expected?: Binary, - timeout?: number, locator?: { frame: FrameChannel, selector: string, @@ -2193,7 +2192,7 @@ export type PageExpectScreenshotResult = { errorMessage?: string, actual?: Binary, previous?: Binary, - timeout?: number, + timedOut?: boolean, log?: string[], }; export type PageScreenshotParams = { diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index d16931dd05f67..69830cf8e8bbc 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -1482,7 +1482,7 @@ Page: expectScreenshot: parameters: expected: binary? - timeout: number? + timeout: number isNot: boolean locator: type: object? @@ -1501,7 +1501,7 @@ Page: errorMessage: string? actual: binary? previous: binary? - timeout: number? + timedOut: boolean? log: type: array? items: string From 5a5586a407b79b553dec4dec25632befdd5b943f Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 25 Oct 2024 11:04:15 -0700 Subject: [PATCH 4/4] update expectations --- tests/playwright-test/golden.spec.ts | 5 +++-- tests/playwright-test/to-have-screenshot.spec.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index 205eeee7f0520..340851e384025 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -223,7 +223,7 @@ test('should write detailed failure result to an output folder', async ({ runInl expect(result.exitCode).toBe(1); const outputText = result.output; - expect(outputText).toContain('Snapshot comparison failed:'); + expect(outputText).toContain('Error: expect(string).toMatchSnapshot(expected)'); const expectedSnapshotArtifactPath = testInfo.outputPath('test-results', 'a-is-a-test', 'snapshot-expected.txt'); const actualSnapshotArtifactPath = testInfo.outputPath('test-results', 'a-is-a-test', 'snapshot-actual.txt'); expect(outputText).toMatch(/Expected:.*a\.spec\.js-snapshots.snapshot\.txt/); @@ -635,7 +635,8 @@ test('should compare different PNG images', async ({ runInlineTest }, testInfo) const outputText = result.output; expect(result.exitCode).toBe(1); - expect(outputText).toContain('Screenshot comparison failed:'); + expect(outputText).toContain('Error: expect(Buffer).toMatchSnapshot(expected)'); + expect(outputText).toContain('1 pixels (ratio 1.00 of all image pixels) are different.'); const expectedSnapshotArtifactPath = testInfo.outputPath('test-results', 'a-is-a-test', 'snapshot-expected.png'); const actualSnapshotArtifactPath = testInfo.outputPath('test-results', 'a-is-a-test', 'snapshot-actual.png'); const diffSnapshotArtifactPath = testInfo.outputPath('test-results', 'a-is-a-test', 'snapshot-diff.png'); diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index a4eb131f07bdc..ee053cc1304fa 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -551,7 +551,7 @@ test('should fail when screenshot is different pixels', async ({ runInlineTest } ` }); expect(result.exitCode).toBe(1); - expect(result.output).toContain('Screenshot comparison failed'); + expect(result.output).toContain('Error: expect(page).toHaveScreenshot(expected)'); expect(result.output).toContain('12345 pixels'); expect(result.output).toContain('Call log'); expect(result.output).toContain('ratio 0.02');