fix(test runner): toHaveScreenshot should not overwrite matching expectations (#15028)

Even in the `--update-snapshots` mode we should keep existing files if
they are matching under the threshold, to avoid needless churn.

docs/src/test-api/class-testconfig.md

@@ -578,7 +578,7 @@ export default config;
 - type: ?<[UpdateSnapshots]<"all"|"none"|"missing">>
 
 Whether to update expected snapshots with the actual results produced by the test run. Defaults to `'missing'`.
-* `'all'` - All tests that are executed will update snapshots.
+* `'all'` - All tests that are executed will update snapshots that did not match. Matching snapshots will not be updated.
 * `'none'` - No snapshots are updated.
 * `'missing'` - Missing snapshots are created, for example when authoring a new test and running it for the first time. This is the default.
 

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

@@ -344,7 +344,7 @@ export async function toHaveScreenshot(
   if (helper.updateSnapshots === 'none' && !hasSnapshot)
     return { pass: false, message: () => `${helper.snapshotPath} is missing in snapshots.` };
 
-  if (helper.updateSnapshots === 'all' || !hasSnapshot) {
+  if (!hasSnapshot) {
     // Regenerate a new screenshot by waiting until two screenshots are the same.
     const timeout = currentExpectTimeout(helper.allOptions);
     const { actual, previous, diff, errorMessage, log } = await page._expectScreenshot(customStackTrace, {
@@ -360,23 +360,14 @@ export async function toHaveScreenshot(
     if (errorMessage)
       return helper.handleDifferent(actual, undefined, previous, diff, undefined, log, errorMessage);
 
-    // We successfully (re-)generated new screenshot.
-    if (!hasSnapshot)
-      return helper.handleMissing(actual!);
-
-    writeFileSync(helper.snapshotPath, actual!);
-    /* eslint-disable no-console */
-    console.log(helper.snapshotPath + ' is re-generated, writing actual.');
-    return {
-      pass: true,
-      message: () => helper.snapshotPath + ' running with --update-snapshots, writing actual.'
-    };
+    // We successfully generated new screenshot.
+    return helper.handleMissing(actual!);
   }
 
   // General case:
   // - snapshot exists
   // - regular matcher (i.e. not a `.not`)
-  // - no flags to update screenshots
+  // - perhaps an 'all' flag to update non-matching screenshots
   const expected = await fs.promises.readFile(helper.snapshotPath);
   const { actual, diff, errorMessage, log } = await page._expectScreenshot(customStackTrace, {
     expected,
@@ -387,9 +378,21 @@ export async function toHaveScreenshot(
     timeout: currentExpectTimeout(helper.allOptions),
   });
 
-  return errorMessage ?
-    helper.handleDifferent(actual, expected, undefined, diff, errorMessage, log) :
-    helper.handleMatching();
+  if (!errorMessage)
+    return helper.handleMatching();
+
+  if (helper.updateSnapshots === 'all') {
+    writeFileSync(helper.snapshotPath, actual!);
+    writeFileSync(helper.actualPath, actual!);
+    /* eslint-disable no-console */
+    console.log(helper.snapshotPath + ' is re-generated, writing actual.');
+    return {
+      pass: true,
+      message: () => helper.snapshotPath + ' running with --update-snapshots, writing actual.'
+    };
+  }
+
+  return helper.handleDifferent(actual, expected, undefined, diff, errorMessage, log);
 }
 
 function writeFileSync(aPath: string, content: Buffer | string) {

packages/playwright-test/types/test.d.ts

@@ -881,7 +881,8 @@ interface TestConfig {
 
   /**
    * Whether to update expected snapshots with the actual results produced by the test run. Defaults to `'missing'`.
-   * - `'all'` - All tests that are executed will update snapshots.
+   * - `'all'` - All tests that are executed will update snapshots that did not match. Matching snapshots will not be
+   *   updated.
    * - `'none'` - No snapshots are updated.
    * - `'missing'` - Missing snapshots are created, for example when authoring a new test and running it for the first
    *   time. This is the default.
@@ -1155,7 +1156,8 @@ export interface FullConfig<TestArgs = {}, WorkerArgs = {}> {
   shard: { total: number, current: number } | null;
   /**
    * Whether to update expected snapshots with the actual results produced by the test run. Defaults to `'missing'`.
-   * - `'all'` - All tests that are executed will update snapshots.
+   * - `'all'` - All tests that are executed will update snapshots that did not match. Matching snapshots will not be
+   *   updated.
    * - `'none'` - No snapshots are updated.
    * - `'missing'` - Missing snapshots are created, for example when authoring a new test and running it for the first
    *   time. This is the default.

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

@@ -768,6 +768,31 @@ test('should respect maxDiffPixels option', async ({ runInlineTest }) => {
   })).exitCode, 'make sure maxDiffPixels option in project config is respected').toBe(0);
 });
 
+test('should not update screenshot that matches with maxDiffPixels option when -u is passed', async ({ runInlineTest }, testInfo) => {
+  const BAD_PIXELS = 120;
+  const EXPECTED_SNAPSHOT = paintBlackPixels(whiteImage, BAD_PIXELS);
+
+  const result = await runInlineTest({
+    ...playwrightConfig({ screenshotsDir: '__screenshots__' }),
+    '__screenshots__/a.spec.js/snapshot.png': EXPECTED_SNAPSHOT,
+    'a.spec.js': `
+      pwt.test('is a test', async ({ page }) => {
+        await expect(page).toHaveScreenshot('snapshot.png', { maxDiffPixels: ${BAD_PIXELS} });
+      });
+    `
+  }, { 'update-snapshots': true });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.output).not.toContain(`is re-generated, writing actual`);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-actual.png'))).toBe(false);
+  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(false);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'a-is-a-test', 'is-a-test-1-diff.png'))).toBe(false);
+
+  const data = fs.readFileSync(testInfo.outputPath('__screenshots__/a.spec.js/snapshot.png'));
+  expect(pngComparator(data, EXPECTED_SNAPSHOT)).toBe(null);
+});
+
 test('should satisfy both maxDiffPixelRatio and maxDiffPixels', async ({ runInlineTest }) => {
   const BAD_RATIO = 0.25;
   const BAD_COUNT = Math.floor(IMG_WIDTH * IMG_HEIGHT * BAD_RATIO);