test: make sure toHaveScreenshot doesn't tolerate bad option values (#13627)

Fixes #13495

packages/playwright-core/src/server/page.ts

@@ -21,7 +21,7 @@ import * as input from './input';
 import * as js from './javascript';
 import * as network from './network';
 import type { ScreenshotOptions } from './screenshotter';
-import { Screenshotter } from './screenshotter';
+import { Screenshotter, validateScreenshotOptions } from './screenshotter';
 import { TimeoutSettings } from '../common/timeoutSettings';
 import type * as types from './types';
 import { BrowserContext } from './browserContext';
@@ -477,6 +477,13 @@ export class Page extends SdkObject {
     const controller = new ProgressController(metadata, this);
     if (!options.expected && options.isNot)
       return { errorMessage: '"not" matcher requires expected result' };
+    try {
+      const format = validateScreenshotOptions(options.screenshotOptions || {});
+      if (format !== 'png')
+        throw new Error('Only PNG screenshots are supported');
+    } catch (error) {
+      return { errorMessage: error.message };
+    }
     let intermediateResult: {
       actual?: Buffer,
       previous?: Buffer,
@@ -549,7 +556,7 @@ export class Page extends SdkObject {
       return {
         log: e.message ? [...metadata.log, e.message] : metadata.log,
         ...intermediateResult,
-        errorMessage: intermediateResult?.errorMessage ?? e.message,
+        errorMessage: e.message,
       };
     });
   }

packages/playwright-core/src/server/screenshotter.ts

@@ -321,7 +321,7 @@ function trimClipToSize(clip: types.Rect, size: types.Size): types.Rect {
   return result;
 }
 
-function validateScreenshotOptions(options: ScreenshotOptions): 'png' | 'jpeg' {
+export function validateScreenshotOptions(options: ScreenshotOptions): 'png' | 'jpeg' {
   if (options.fonts && !experimentalFeaturesEnabled())
     throw new Error(`To use the experimental option "fonts", set PLAYWRIGHT_EXPERIMENTAL_FEATURES=1 enviroment variable.`);
 

packages/playwright-test/src/matchers/toMatchSnapshot.ts

@@ -357,12 +357,8 @@ export async function toHaveScreenshot(
     });
     // 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) {
+    if (errorMessage)
-      const title = actual && previous ?
+      return helper.handleDifferent(actual, undefined, previous, diff, undefined, log, errorMessage);
-        `Timeout ${timeout}ms exceeded while generating screenshot because ${locator ? 'element' : 'page'} kept changing` :
-        `Timeout ${timeout}ms exceeded while generating screenshot`;
-      return helper.handleDifferent(actual, undefined, previous, diff, undefined, log, title);
-    }
 
     // We successfully (re-)generated new screenshot.
     if (!hasSnapshot)

tests/playwright-test/to-have-screenshot.spec.ts

@@ -50,7 +50,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(`Timeout 2000ms exceeded`);
   expect(stripAnsi(result.output)).toContain(`expect.toHaveScreenshot with timeout 2000ms`);
   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);
@@ -74,6 +74,29 @@ test('should disable animations by default', async ({ runInlineTest }, testInfo)
   expect(result.exitCode).toBe(0);
 });
 
+test('should fail with proper error when unsupported argument is given', async ({ runInlineTest }, testInfo) => {
+  const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html'));
+  const result = await runInlineTest({
+    ...playwrightConfig({}),
+    'a.spec.js': `
+      pwt.test('is a test', async ({ page }) => {
+        await page.goto('${cssTransitionURL}');
+        await expect(page).toHaveScreenshot({
+          clip: {
+            x: 0,
+            y: 0,
+            width: 0,
+            height: 0,
+          },
+          timeout: 2000,
+        });
+      });
+    `
+  }, { 'update-snapshots': true });
+  expect(result.exitCode).toBe(1);
+  expect(stripAnsi(result.output)).toContain(`Expected options.clip.width not to be 0`);
+});
+
 test('should have scale:css by default', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     ...playwrightConfig({ screenshotsDir: '__screenshots__' }),
@@ -284,7 +307,7 @@ test('should fail to screenshot an element with infinite animation', async ({ ru
     `
   });
   expect(result.exitCode).toBe(1);
-  expect(stripAnsi(result.output)).toContain(`Timeout 2000ms exceeded while generating screenshot because element kept changing`);
+  expect(stripAnsi(result.output)).toContain(`Timeout 2000ms exceeded`);
   expect(stripAnsi(result.output)).toContain(`expect.toHaveScreenshot with timeout 2000ms`);
   expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-previous.png'))).toBe(true);
   expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-actual.png'))).toBe(true);