tr-did-timeout-top-level

packages/playwright/src/worker/testInfo.ts

@@ -354,13 +354,13 @@ export class TestInfoImpl implements TestInfo {
     // Do not overwrite any previous hard errors.
     // Some (but not all) scenarios include:
     //   - expect() that fails after uncaught exception.
-    //   - fail after the timeout, e.g. due to fixture teardown.
+    //   - fail in fixture teardown after the test failure.
     if (isHardError && this._hasHardError)
       return;
     if (isHardError)
       this._hasHardError = true;
     if (this.status === 'passed' || this.status === 'skipped')
-      this.status = 'failed';
+      this.status = error instanceof TimeoutManagerError ? 'timedOut' : 'failed';
     const serialized = serializeError(error);
     const step = (error as any)[stepSymbol] as TestStepInternal | undefined;
     if (step && step.boxedStack)
@@ -397,8 +397,6 @@ export class TestInfoImpl implements TestInfo {
 
     try {
       await this._timeoutManager.withRunnable(runnable, async () => {
-        // Note: separate try/catch is here to report errors after timeout.
-        // This way we get a nice "locator.click" error after the test times out and closes the page.
         try {
           await cb();
         } catch (e) {
@@ -406,7 +404,13 @@ export class TestInfoImpl implements TestInfo {
             if (this.status === 'passed')
               this.status = 'skipped';
           } else if (!(e instanceof TimeoutManagerError)) {
-            // Do not consider timeout errors in child stages as a regular "hard error".
+            // Note: we handle timeout errors at the top level, so ignore them here.
+            // Unfortunately, we cannot ignore user errors here. 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 */);
           }
           throw e;
@@ -414,17 +418,6 @@ export class TestInfoImpl implements TestInfo {
       });
       stage.step?.complete({});
     } catch (error) {
-      // When interrupting, we arrive here with a TimeoutManagerError, but we should not
-      // consider it a timeout.
-      if (!this._wasInterrupted && !this._didTimeout && (error instanceof TimeoutManagerError)) {
-        this._didTimeout = true;
-        const serialized = serializeError(error);
-        this.errors.push(serialized);
-        this._tracing.appendForError(serialized);
-        // Do not overwrite existing failure upon hook/teardown timeout.
-        if (this.status === 'passed' || this.status === 'skipped')
-          this.status = 'timedOut';
-      }
       stage.step?.complete({ error });
       throw error;
     } finally {
@@ -435,6 +428,15 @@ 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 && !this._didTimeout && (error instanceof TimeoutManagerError)) {
+      // this._didTimeout = true;
+      this._failWithError(error, false /* isHardError */, true /* retriable */);
+    }
+  }
+
   _isFailure() {
     return this.status !== 'skipped' && this.status !== this.expectedStatus;
   }

packages/playwright/src/worker/workerMain.ts

@@ -359,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(() => {});  // Ignore top-level error, we still have to run after hooks.
+    }).catch(error => testInfo._handlePossibleTimeoutError(error));
 
     // Update duration, so it is available in fixture teardown and afterEach hooks.
     testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0;
@@ -462,11 +462,12 @@ export class WorkerMain extends ProcessRunner {
 
       if (firstAfterHooksError)
         throw firstAfterHooksError;
-    }).catch(() => {});  // Ignore top-level error.
+    }).catch(error => testInfo._handlePossibleTimeoutError(error));
 
-    await testInfo._runAsStage({ title: 'stop tracing' }, async () => {
+    const tracingSlot = { timeout: this._project.project.timeout, elapsed: 0 };
+    await testInfo._runAsStage({ title: 'stop tracing', canTimeout: true, runnableType: 'test', runnableSlot: tracingSlot }, async () => {
       await testInfo._tracing.stopIfNeeded();
-    }).catch(() => {});  // Ignore top-level error.
+    }).catch(error => testInfo._handlePossibleTimeoutError(error));
 
     testInfo.duration = (testInfo._timeoutManager.defaultSlotTimings().elapsed + afterHooksSlot.elapsed) | 0;
 

tests/playwright-test/hooks.spec.ts

@@ -526,7 +526,7 @@ test('afterAll timeout should be reported, run other afterAll hooks, and continu
       test.afterAll(async () => {
         console.log('\\n%%afterAll2');
       });
-      test('does not run', () => {
+      test('run in a different worker', () => {
         console.log('\\n%%test2');
       });
     `,