feat(test runner): show multiple errors, at most one per stage (#30024)

Previously, there was at most one "hard error", as opposite to multiple
"soft errors". This was done to preserve the historic behavior at the
time of introducing multiple `TestInfo.errors`.

With this change, every user callback that is executed `withRunnable()`
can throw an error and/or timeout, and both of these will end up in
`TestInfo.errors`.

Additionally, there is at most one "unhandled exception" error, to avoid
flooding the report with mass failures.

Drive-by: remove boolean arguments from `_failWithError()`.

Fixes #29876.

packages/playwright/src/matchers/expect.ts

@@ -267,7 +267,6 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
         params: args[0] ? { expected: args[0] } : undefined,
         wallTime,
         infectParentStepsWithError: this._info.isSoft,
-        isSoft: this._info.isSoft,
       };
 
       const step = testInfo._addStep(stepInfo);
@@ -275,7 +274,9 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
       const reportStepError = (jestError: ExpectError) => {
         const error = new ExpectError(jestError, customMessage, stackFrames);
         step.complete({ error });
-        if (!this._info.isSoft)
+        if (this._info.isSoft)
+          testInfo._failWithError(error);
+        else
           throw error;
       };
 

packages/playwright/src/matchers/toMatchSnapshot.ts

@@ -232,7 +232,8 @@ class SnapshotHelper {
       return this.createMatcherResult(message, true);
     }
     if (this.updateSnapshots === 'missing') {
-      this.testInfo._failWithError(new Error(message), false /* isHardError */, false /* retriable */);
+      this.testInfo._hasNonRetriableError = true;
+      this.testInfo._failWithError(new Error(message));
       return this.createMatcherResult('', true);
     }
     return this.createMatcherResult(message, false);

packages/playwright/src/worker/testInfo.ts

@@ -44,7 +44,6 @@ export interface TestStepInternal {
   error?: TestInfoError;
   infectParentStepsWithError?: boolean;
   box?: boolean;
-  isSoft?: boolean;
   isStage?: boolean;
 }
 
@@ -62,7 +61,6 @@ export class TestInfoImpl implements TestInfo {
   readonly _timeoutManager: TimeoutManager;
   readonly _startTime: number;
   readonly _startWallTime: number;
-  private _hasHardError: boolean = false;
   readonly _tracing: TestTracing;
 
   _wasInterrupted = false;
@@ -74,6 +72,7 @@ export class TestInfoImpl implements TestInfo {
   _onDidFinishTestFunction: (() => Promise<void>) | undefined;
   private readonly _stages: TestStage[] = [];
   _hasNonRetriableError = false;
+  _hasUnhandledError = false;
   _allowSkips = false;
 
   // ------------ TestInfo fields ------------
@@ -314,9 +313,6 @@ export class TestInfoImpl implements TestInfo {
         this._onStepEnd(payload);
         const errorForTrace = step.error ? { name: '', message: step.error.message || '', stack: step.error.stack } : undefined;
         this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments);
-
-        if (step.isSoft && result.error)
-          this._failWithError(result.error, false /* isHardError */, true /* retriable */);
       }
     };
     const parentStepList = parentStep ? parentStep.steps : this._steps;
@@ -344,17 +340,7 @@ export class TestInfoImpl implements TestInfo {
       this.status = 'interrupted';
   }
 
-  _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.
-    //   - fail in fixture teardown after the test failure.
-    if (isHardError && this._hasHardError)
-      return;
-    if (isHardError)
-      this._hasHardError = true;
+  _failWithError(error: Error) {
     if (this.status === 'passed' || this.status === 'skipped')
       this.status = error instanceof TimeoutManagerError ? 'timedOut' : 'failed';
     const serialized = serializeError(error);
@@ -378,24 +364,31 @@ export class TestInfoImpl implements TestInfo {
         try {
           await cb();
         } catch (e) {
+          // Only handle errors directly thrown by the user code.
+          if (!stage.runnable)
+            throw e;
           if (this._allowSkips && (e instanceof SkipError)) {
             if (this.status === 'passed')
               this.status = 'skipped';
-          } else if (!(e instanceof TimeoutManagerError)) {
-            // Note: we handle timeout errors at the top level, so ignore them here.
-            // Unfortunately, we cannot ignore user errors here. Consider the following scenario:
+          } else {
+            // Unfortunately, we have to handle user errors and timeout errors differently.
+            // Consider the following scenario:
             // - locator.click times out
             // - all stages containing the test function finish with TimeoutManagerError
             // - test finishes, the page is closed and this triggers locator.click error
             // - we would like to present the locator.click error to the user
             // - therefore, we need a try/catch inside the "run with timeout" block and capture the error
-            this._failWithError(e, true /* isHardError */, true /* retriable */);
+            this._failWithError(e);
           }
           throw e;
         }
       });
       stage.step?.complete({});
     } catch (error) {
+      // When interrupting, we arrive here with a TimeoutManagerError, but we should not
+      // consider it a timeout.
+      if (!this._wasInterrupted && (error instanceof TimeoutManagerError) && stage.runnable)
+        this._failWithError(error);
       stage.step?.complete({ error });
       throw error;
     } finally {
@@ -406,13 +399,6 @@ export class TestInfoImpl implements TestInfo {
     }
   }
 
-  _handlePossibleTimeoutError(error: Error) {
-    // When interrupting, we arrive here with a TimeoutManagerError, but we should not
-    // consider it a timeout.
-    if (!this._wasInterrupted && (error instanceof TimeoutManagerError))
-      this._failWithError(error, false /* isHardError */, true /* retriable */);
-  }
-
   _isFailure() {
     return this.status !== 'skipped' && this.status !== this.expectedStatus;
   }

packages/playwright/src/worker/workerMain.ts

@@ -147,8 +147,9 @@ export class WorkerMain extends ProcessRunner {
   private async _teardownScopes() {
     const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {});
     const runnable = { type: 'teardown' } as const;
-    await this._fixtureRunner.teardownScope('test', fakeTestInfo, runnable).catch(error => fakeTestInfo._handlePossibleTimeoutError(error));
-    await this._fixtureRunner.teardownScope('worker', fakeTestInfo, runnable).catch(error => fakeTestInfo._handlePossibleTimeoutError(error));
+    // Ignore top-level errors, they are already inside TestInfo.errors.
+    await this._fixtureRunner.teardownScope('test', fakeTestInfo, runnable).catch(() => {});
+    await this._fixtureRunner.teardownScope('worker', fakeTestInfo, runnable).catch(() => {});
     this._fatalErrors.push(...fakeTestInfo.errors);
   }
 
@@ -165,7 +166,10 @@ 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 */, true /* retriable */);
+    if (!this._currentTest._hasUnhandledError) {
+      this._currentTest._hasUnhandledError = true;
+      this._currentTest._failWithError(error);
+    }
 
     // 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.
@@ -355,7 +359,7 @@ export class WorkerMain extends ProcessRunner {
         const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]").
         await fn(testFunctionParams, testInfo);
       });
-    }).catch(error => testInfo._handlePossibleTimeoutError(error));
+    }).catch(() => {});  // Ignore the top-level error, it is already inside TestInfo.errors.
 
     // Update duration, so it is available in fixture teardown and afterEach hooks.
     testInfo.duration = testInfo._timeoutManager.defaultSlot().elapsed | 0;
@@ -416,7 +420,7 @@ export class WorkerMain extends ProcessRunner {
       }
       if (firstAfterHooksError)
         throw firstAfterHooksError;
-    }).catch(error => testInfo._handlePossibleTimeoutError(error));
+    }).catch(() => {});  // Ignore the top-level error, it is already inside TestInfo.errors.
 
     if (testInfo._isFailure())
       this._isStopped = true;
@@ -455,13 +459,13 @@ export class WorkerMain extends ProcessRunner {
 
         if (firstWorkerCleanupError)
           throw firstWorkerCleanupError;
-      }).catch(error => testInfo._handlePossibleTimeoutError(error));
+      }).catch(() => {});  // Ignore the top-level error, it is already inside TestInfo.errors.
     }
 
     const tracingSlot = { timeout: this._project.project.timeout, elapsed: 0 };
     await testInfo._runAsStage({ title: 'stop tracing', runnable: { type: 'test', slot: tracingSlot } }, async () => {
       await testInfo._tracing.stopIfNeeded();
-    }).catch(error => testInfo._handlePossibleTimeoutError(error));
+    }).catch(() => {});  // Ignore the top-level error, it is already inside TestInfo.errors.
 
     testInfo.duration = (testInfo._timeoutManager.defaultSlot().elapsed + afterHooksSlot.elapsed) | 0;
 

tests/playwright-test/fixture-errors.spec.ts

@@ -733,3 +733,24 @@ test('should not continue with scope teardown after fixture teardown timeout', a
   expect(result.output).toContain('Test finished within timeout of 1000ms, but tearing down "fixture2" ran out of time.');
   expect(result.output).not.toContain('in fixture teardown');
 });
+
+test('should report fixture teardown error after test error', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test as base, expect } from '@playwright/test';
+      const test = base.extend({
+        foo: async ({ }, use) => {
+          await use();
+          throw new Error('Error from the fixture foo');
+        },
+      });
+      test('fails', async ({ foo }) => {
+        throw new Error('Error from the test');
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.output).toContain('Error from the fixture foo');
+  expect(result.output).toContain('Error from the test');
+});