fix: do not retry missing snapshot errors (#29272)

When `updateSnapshots === 'missing'` we generate new expectations on the
first attempt and don't retry the test afterwards instead of trying it
retries-1 times and only writing new expectation on the last attempt.
This logic infects all serial mode suites that contain the test with
missing expectations, so they also will not be retried.

Reference https://github.com/microsoft/playwright/issues/29073

packages/playwright/src/common/ipc.ts

@@ -84,6 +84,7 @@ export type TestEndPayload = {
   duration: number;
   status: TestStatus;
   errors: TestInfoError[];
+  hasNonRetriableError: boolean;
   expectedStatus: TestStatus;
   annotations: { type: string, description?: string }[];
   timeout: number;

packages/playwright/src/matchers/toMatchSnapshot.ts

@@ -141,8 +141,6 @@ class SnapshotHelper<T extends ImageComparatorOptions> {
     this.locator = locator;
 
     this.updateSnapshots = testInfo.config.updateSnapshots;
-    if (this.updateSnapshots === 'missing' && testInfo.retry < testInfo.project.retries)
-      this.updateSnapshots = 'none';
     this.mimeType = mime.getType(path.basename(this.snapshotPath)) ?? 'application/octet-string';
     this.comparator = getComparator(this.mimeType);
 
@@ -206,7 +204,7 @@ class SnapshotHelper<T extends ImageComparatorOptions> {
       return this.createMatcherResult(message, true);
     }
     if (this.updateSnapshots === 'missing') {
-      this.testInfo._failWithError(new Error(message), false /* isHardError */);
+      this.testInfo._failWithError(new Error(message), false /* isHardError */, false /* retriable */);
       return this.createMatcherResult('', true);
     }
     return this.createMatcherResult(message, false);

packages/playwright/src/runner/dispatcher.ts

@@ -243,6 +243,7 @@ class JobDispatcher {
 
   private _listeners: RegisteredListener[] = [];
   private _failedTests = new Set<TestCase>();
+  private _failedWithNonRetriableError = new Set<TestCase|Suite>();
   private _remainingByTestId = new Map<string, TestCase>();
   private _dataByTestId = new Map<string, { test: TestCase, result: TestResult, steps: Map<string, TestStep> }>();
   private _parallelIndex = 0;
@@ -293,10 +294,20 @@ class JobDispatcher {
     const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus;
     if (isFailure)
       this._failedTests.add(test);
+    if (params.hasNonRetriableError)
+      this._addNonretriableTestAndSerialModeParents(test);
     this._reportTestEnd(test, result);
     this._currentlyRunning = undefined;
   }
 
+  private _addNonretriableTestAndSerialModeParents(test: TestCase) {
+    this._failedWithNonRetriableError.add(test);
+    for (let parent: Suite | undefined = test.parent; parent; parent = parent.parent) {
+      if (parent._parallelMode === 'serial')
+        this._failedWithNonRetriableError.add(parent);
+    }
+  }
+
   private _onStepBegin(params: StepBeginPayload) {
     const data = this._dataByTestId.get(params.testId);
     if (!data) {
@@ -435,6 +446,8 @@ class JobDispatcher {
     const serialSuitesWithFailures = new Set<Suite>();
 
     for (const failedTest of this._failedTests) {
+      if (this._failedWithNonRetriableError.has(failedTest))
+        continue;
       retryCandidates.add(failedTest);
 
       let outermostSerialSuite: Suite | undefined;
@@ -442,7 +455,7 @@ class JobDispatcher {
         if (parent._parallelMode ===  'serial')
           outermostSerialSuite = parent;
       }
-      if (outermostSerialSuite)
+      if (outermostSerialSuite && !this._failedWithNonRetriableError.has(outermostSerialSuite))
         serialSuitesWithFailures.add(outermostSerialSuite);
     }
 

packages/playwright/src/worker/testInfo.ts

@@ -69,6 +69,8 @@ export class TestInfoImpl implements TestInfo {
   _afterHooksStep: TestStepInternal | undefined;
   _onDidFinishTestFunction: (() => Promise<void>) | undefined;
 
+  _hasNonRetriableError = false;
+
   // ------------ TestInfo fields ------------
   readonly testId: string;
   readonly repeatEachIndex: number;
@@ -241,7 +243,7 @@ export class TestInfoImpl implements TestInfo {
         if (this.status === 'passed')
           this.status = 'skipped';
       } else {
-        this._failWithError(error, true /* isHardError */);
+        this._failWithError(error, true /* isHardError */, true /* retriable */);
         return error;
       }
     }
@@ -318,7 +320,7 @@ export class TestInfoImpl implements TestInfo {
         this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments);
 
         if (step.isSoft && result.error)
-          this._failWithError(result.error, false /* isHardError */);
+          this._failWithError(result.error, false /* isHardError */, true /* retriable */);
       }
     };
     const parentStepList = parentStep ? parentStep.steps : this._steps;
@@ -346,7 +348,9 @@ export class TestInfoImpl implements TestInfo {
       this.status = 'interrupted';
   }
 
-  _failWithError(error: Error, isHardError: boolean) {
+  _failWithError(error: Error, isHardError: boolean, retriable: boolean) {
+    if (!retriable)
+      this._hasNonRetriableError = true;
     // Do not overwrite any previous hard errors.
     // Some (but not all) scenarios include:
     //   - expect() that fails after uncaught exception.

packages/playwright/src/worker/workerMain.ts

@@ -169,7 +169,7 @@ export class WorkerMain extends ProcessRunner {
     // and unhandled errors - both lead to the test failing. This is good for regular tests,
     // so that you can, e.g. expect() from inside an event handler. The test fails,
     // and we restart the worker.
-    this._currentTest._failWithError(error, true /* isHardError */);
+    this._currentTest._failWithError(error, true /* isHardError */, true /* retriable */);
 
     // For tests marked with test.fail(), this might be a problem when unhandled error
     // is not coming from the user test code (legit failure), but from fixtures or test runner.
@@ -624,6 +624,7 @@ function buildTestEndPayload(testInfo: TestInfoImpl): TestEndPayload {
     duration: testInfo.duration,
     status: testInfo.status!,
     errors: testInfo.errors,
+    hasNonRetriableError: testInfo._hasNonRetriableError,
     expectedStatus: testInfo.expectedStatus,
     annotations: testInfo.annotations,
     timeout: testInfo.timeout,

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

@@ -74,6 +74,56 @@ test('should disable animations by default', async ({ runInlineTest }, testInfo)
   expect(result.exitCode).toBe(0);
 });
 
+test('should not retry missing expectation errors', async ({ runInlineTest }, testInfo) => {
+  const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html'));
+  const result = await runInlineTest({
+    ...playwrightConfig({
+      retries: 2,
+    }),
+    'a.spec.js': `
+      const { test, expect } = require('@playwright/test');
+      test('is a test', async ({ page }) => {
+        await page.goto('${cssTransitionURL}');
+        await expect(page).toHaveScreenshot('foo.png', { timeout: 1000 });
+        await expect(page).toHaveScreenshot('bar.png', { timeout: 1000 });
+      });
+    `
+  });
+  expect(result.output).not.toContain(`retry #`);
+  expect(result.output).toMatch(/A snapshot doesn't exist.*foo.*, writing actual./);
+  expect(result.output).toMatch(/A snapshot doesn't exist.*bar.*, writing actual./);
+  expect(result.exitCode).toBe(1);
+});
+
+test('should not retry serial mode suites with missing expectation errors', async ({ runInlineTest }, testInfo) => {
+  const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html'));
+  const result = await runInlineTest({
+    ...playwrightConfig({
+      retries: 2,
+    }),
+    'a.spec.js': `
+      const { test, expect } = require('@playwright/test');
+      test.describe.serial('outer', () => {
+        test('last', async ({ page }) => {
+        });
+        test.describe('nested', () => {
+          test('is a test', async ({ page }) => {
+            await page.goto('${cssTransitionURL}');
+            await expect(page).toHaveScreenshot({ timeout: 1000 });
+            await expect(page).toHaveScreenshot({ timeout: 1000 });
+          });
+          test('last', async ({ page }) => {
+          });
+        });
+      });
+    `
+  });
+  expect(result.output).not.toContain(`retry #`);
+  expect(result.output).toMatch(/A snapshot doesn't exist.*1.*, writing actual./);
+  expect(result.output).toMatch(/A snapshot doesn't exist.*2.*, writing actual./);
+  expect(result.exitCode).toBe(1);
+});
+
 test.describe('expect config animations option', () => {
   test('disabled', async ({ runInlineTest }, testInfo) => {
     const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html'));