From c18077c0dec1ed002ee7fa816522424689ad8cba Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Mon, 21 Mar 2022 16:10:33 -0600 Subject: [PATCH] feat(toHaveScreenshot): align screenshot generation & comparison (#12812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch aligns the strategies that are used to generate new screnshot expectations and to compare screenshot expectations against baseline. With this patch, `toHaveScreenshot` will: - when generating a new expectation: will wait for 2 consecutive screenshots to match and accept the last one as expectation. - when given an expectation: * will compare first screenshot against expectation. If matches, resolve successfully * if first screenshot doesn't match, then wait for 2 consecutive screenshots to match and then compare last screenshot with the expectation. An example of a new detailed call log: ``` 1) a.spec.ts:3:1 › should work =================================================================== Error: Screenshot comparison failed: 20000 pixels (ratio 0.03 of all image pixels) are different Call log: - expect.toHaveScreenshot with timeout 5000ms - verifying given screenshot expectation - fast-path: checking first screenshot to match expectation - taking page screenshot - disabled all CSS animations - waiting for fonts to load... - fonts in all frames are loaded - fast-path failed: first screenshot did not match expectation - 20000 pixels (ratio 0.03 of all image pixels) are different - waiting for 2 consecutive screenshots to match - waiting 100ms before taking screenshot - taking page screenshot - disabled all CSS animations - waiting for fonts to load... - fonts in all frames are loaded - 2 consecutive screenshots matched - final screenshot did not match expectation - 20000 pixels (ratio 0.03 of all image pixels) are different - 20000 pixels (ratio 0.03 of all image pixels) are different Expected: /Users/andreylushnikov/tmp/test-results/a-should-work/should-work-1-expected.png Received: /Users/andreylushnikov/tmp/test-results/a-should-work/should-work-1-actual.png Diff: /Users/andreylushnikov/tmp/test-results/a-should-work/should-work-1-diff.png 3 | test('should work', async ({ page }) => { 4 | await page.goto('file:///Users/andreylushnikov/prog/playwright/tests/assets/rotate-z.html'); > 5 | await expect(page).toHaveScreenshot(); | ^ 6 | }); 7 | ``` --- packages/playwright-core/src/server/page.ts | 69 ++++++++++++------- .../playwright-core/src/utils/comparators.ts | 2 +- .../to-have-screenshot.spec.ts | 7 +- 3 files changed, 48 insertions(+), 30 deletions(-) diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index 10c542d625..06bb2cbdb3 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -31,7 +31,7 @@ import { Progress, ProgressController } from './progress'; import { assert, isError } from '../utils/utils'; import { ManualPromise } from '../utils/async'; import { debugLogger } from '../utils/debugLogger'; -import { mimeTypeToComparator, ImageComparatorOptions, ComparatorResult } from '../utils/comparators'; +import { mimeTypeToComparator, ImageComparatorOptions } from '../utils/comparators'; import { SelectorInfo, Selectors } from './selectors'; import { CallMetadata, SdkObject } from './instrumentation'; import { Artifact } from './artifact'; @@ -458,53 +458,72 @@ export class Page extends SdkObject { const comparator = mimeTypeToComparator['image/png']; const controller = new ProgressController(metadata, this); - const isGeneratingNewScreenshot = !options.expected; - if (isGeneratingNewScreenshot && options.isNot) + if (!options.expected && options.isNot) return { errorMessage: '"not" matcher requires expected result' }; let intermediateResult: { actual?: Buffer, previous?: Buffer, - errorMessage?: string, + errorMessage: string, diff?: Buffer, } | undefined = undefined; + const areEqualScreenshots = (actual: Buffer | undefined, expected: Buffer | undefined, previous: Buffer | undefined) => { + const comparatorResult = actual && expected ? comparator(actual, expected, options.comparatorOptions) : undefined; + if (comparatorResult !== undefined && !!comparatorResult === !!options.isNot) + return true; + if (comparatorResult) + intermediateResult = { errorMessage: comparatorResult.errorMessage, diff: comparatorResult.diff, actual, previous }; + return false; + }; const callTimeout = this._timeoutSettings.timeout(options); return controller.run(async progress => { let actual: Buffer | undefined; let previous: Buffer | undefined; const pollIntervals = [0, 100, 250, 500]; progress.log(`${metadata.apiName}${callTimeout ? ` with timeout ${callTimeout}ms` : ''}`); - if (isGeneratingNewScreenshot) - progress.log(` generating new screenshot expectation: waiting for 2 consecutive screenshots to match`); + if (options.expected) + progress.log(` verifying given screenshot expectation`); else - progress.log(` waiting for screenshot to match expectation`); + progress.log(` generating new stable screenshot expectation`); + let isFirstIteration = true; while (true) { progress.throwIfAborted(); if (this.isClosed()) throw new Error('The page has closed'); - let comparatorResult: ComparatorResult | undefined; const screenshotTimeout = pollIntervals.shift() ?? 1000; if (screenshotTimeout) progress.log(`waiting ${screenshotTimeout}ms before taking screenshot`); - if (isGeneratingNewScreenshot) { - previous = actual; - actual = await rafrafScreenshot(progress, screenshotTimeout).catch(e => undefined); - comparatorResult = actual && previous ? comparator(actual, previous, options.comparatorOptions) : undefined; - } else { - actual = await rafrafScreenshot(progress, screenshotTimeout).catch(e => undefined); - comparatorResult = actual ? comparator(actual, options.expected!, options.comparatorOptions) : undefined; - } - if (comparatorResult !== undefined && !!comparatorResult === !!options.isNot) + previous = actual; + actual = await rafrafScreenshot(progress, screenshotTimeout).catch(e => { + progress.log(`failed to take screenshot - ` + e.message); + return undefined; + }); + if (!actual) + continue; + // Compare against expectation for the first iteration. + const expectation = options.expected && isFirstIteration ? options.expected : previous; + if (areEqualScreenshots(actual, expectation, previous)) break; - if (comparatorResult) { - if (isGeneratingNewScreenshot) - progress.log(`2 last screenshots do not match: ${comparatorResult.errorMessage}`); - else - progress.log(`screenshot does not match expectation: ${comparatorResult.errorMessage}`); - intermediateResult = { errorMessage: comparatorResult.errorMessage, diff: comparatorResult.diff, actual, previous }; - } + if (intermediateResult) + progress.log(intermediateResult.errorMessage); + isFirstIteration = false; } - return isGeneratingNewScreenshot ? { actual } : {}; + if (!isFirstIteration) + progress.log(`captured a stable screenshot`); + + if (!options.expected) + return { actual }; + + if (isFirstIteration) { + progress.log(`screenshot matched expectation`); + return {}; + } + + if (areEqualScreenshots(actual, options.expected, previous)) { + progress.log(`screenshot matched expectation`); + return {}; + } + throw new Error(intermediateResult!.errorMessage); }, callTimeout).catch(e => { // Q: Why not throw upon isSessionClosedError(e) as in other places? // A: We want user to receive a friendly diff between actual and expected/previous. diff --git a/packages/playwright-core/src/utils/comparators.ts b/packages/playwright-core/src/utils/comparators.ts index 4a6319e00e..1fb342572a 100644 --- a/packages/playwright-core/src/utils/comparators.ts +++ b/packages/playwright-core/src/utils/comparators.ts @@ -24,7 +24,7 @@ import { diff_match_patch, DIFF_INSERT, DIFF_DELETE, DIFF_EQUAL } from '../third const { PNG } = require(require.resolve('pngjs', { paths: [require.resolve('pixelmatch')] })) as typeof import('pngjs'); export type ImageComparatorOptions = { threshold?: number, maxDiffPixels?: number, maxDiffPixelRatio?: number }; -export type ComparatorResult = { diff?: Buffer; errorMessage?: string; } | null; +export type ComparatorResult = { diff?: Buffer; errorMessage: string; } | null; export type Comparator = (actualBuffer: Buffer | string, expectedBuffer: Buffer, options?: any) => ComparatorResult; export const mimeTypeToComparator: { [key: string]: Comparator } = { 'application/octet-string': compareBuffersOrStrings, diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index 3e21226c24..2043210d61 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -52,7 +52,7 @@ test('should fail to screenshot a page with infinite animation', async ({ runInl expect(result.exitCode).toBe(1); expect(stripAnsi(result.output)).toContain(`Timeout 2000ms exceeded while generating screenshot because page kept changing`); expect(stripAnsi(result.output)).toContain(`expect.toHaveScreenshot with timeout 2000ms`); - expect(stripAnsi(result.output)).toContain(`generating new screenshot expectation: waiting for 2 consecutive screenshots to match`); + expect(stripAnsi(result.output)).toContain(`generating new stable screenshot expectation`); expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-actual.png'))).toBe(true); expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-expected.png'))).toBe(false); expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-previous.png'))).toBe(true); @@ -370,8 +370,8 @@ test('should fail when screenshot is different size', async ({ runInlineTest }) ` }); expect(result.exitCode).toBe(1); - expect(stripAnsi(result.output)).toContain(`Timeout 2000ms exceeded`); - expect(stripAnsi(result.output)).toContain(`waiting for screenshot to match expectation`); + expect(stripAnsi(result.output)).toContain(`verifying given screenshot expectation`); + expect(stripAnsi(result.output)).toContain(`captured a stable screenshot`); expect(result.output).toContain('Expected an image 22px by 33px, received 1280px by 720px.'); }); @@ -411,7 +411,6 @@ 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(`Timeout 2000ms exceeded`); expect(result.output).toContain('12345 pixels'); expect(result.output).toContain('Call log'); expect(result.output).toContain('ratio 0.02');