fix(test runner): properly handle uncaught errors in test.fail() (#27734)

Before this fix, unhandled error during test.fail():
- marks this test as "interrupted";
- fails next test in the file with "fatal error".

After this fix:
- marks this test as "failed as expected";
- restarts worker for the next test.

packages/playwright/src/worker/workerMain.ts

@@ -158,41 +158,32 @@ export class WorkerMain extends ProcessRunner {
   }
 
   unhandledError(error: Error | any) {
-    const failWithFatalErrorAndStop = () => {
+    // No current test - fatal error.
+    if (!this._currentTest) {
       if (!this._fatalErrors.length)
         this._fatalErrors.push(serializeError(error));
       void this._stop();
-    };
-
-    // No current test - fatal error.
-    if (!this._currentTest)
-      return failWithFatalErrorAndStop();
-
-    // Usually, we do not differentiate between errors in the control flow
-    // 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.
-    if (this._currentTest.expectedStatus !== 'failed') {
-      this._currentTest._failWithError(serializeError(error), true /* isHardError */);
-      void this._stop();
       return;
     }
 
-    // However, for tests marked with test.fail(), this is a problem. Unhandled error
-    // could come either from the user test code (legit failure), or from a fixture or
-    // a test runner. In the latter case, the worker state could be messed up,
-    // and continuing to run tests in the same worker is problematic. Therefore,
-    // we turn this into a fatal error and restart the worker anyway.
+    // We do not differentiate between errors in the control flow
+    // 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(serializeError(error), true /* isHardError */);
+
+    // 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.
     //
-    // The only exception is the expect() error that we still consider ok.
+    // Ideally, we would mark this test as "failed unexpectedly" and show that in the report.
+    // However, we do not have such a special test status, so the test will be considered ok (failed as expected).
+    //
+    // To avoid messing up future tests, we forcefully stop the worker, unless it is
+    // an expect() error which we know does not mess things up.
     const isExpectError = (error instanceof Error) && !!(error as any).matcherResult;
-    if (isExpectError) {
-      // Note: do not stop the worker, because test marked with test.fail() that fails an assertion
-      // is perfectly fine.
-      this._currentTest._failWithError(serializeError(error), true /* isHardError */);
-    } else {
-      failWithFatalErrorAndStop();
-    }
+    const shouldContinueInThisWorker = this._currentTest.expectedStatus === 'failed' && isExpectError;
+    if (!shouldContinueInThisWorker)
+      void this._stop();
   }
 
   private async _loadIfNeeded() {

tests/playwright-test/basic.spec.ts

@@ -447,10 +447,10 @@ test('should not reuse worker after unhandled rejection in test.fail', async ({
       });
     `
   }, { workers: 1 });
-  expect(result.exitCode).toBe(1);
-  expect(result.failed).toBe(1);
-  expect(result.interrupted).toBe(1);
-  expect(result.output).toContain(`Error: Oh my!`);
+  expect(result.exitCode).toBe(0);
+  expect(result.failed).toBe(0);
+  expect(result.passed).toBe(2);
+  expect(result.output).not.toContain(`Oh my!`);
   expect(result.output).not.toContain(`Did not teardown test scope`);
 });
 

tests/playwright-test/playwright.unhandled.spec.ts

@@ -16,12 +16,11 @@
 
 import { test, expect } from './playwright-test-fixtures';
 
-test('should lead in uncaughtException when page.route raises', async ({ runInlineTest, server }) => {
+test('should produce uncaughtException when page.route raises', async ({ runInlineTest, server }) => {
   const result = await runInlineTest({
     'a.test.ts': `
       import { test, expect } from '@playwright/test';
       test('fail', async ({ page }) => {
-        test.fail();
         await page.route('**/empty.html', route => {
           throw new Error('foobar');
         });
@@ -29,16 +28,15 @@ test('should lead in uncaughtException when page.route raises', async ({ runInli
       });
     `,
   }, { workers: 1 });
-  expect(result.interrupted).toBe(1);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('foobar');
 });
 
-test('should lead in unhandledRejection when page.route raises', async ({ runInlineTest, server }) => {
+test('should produce unhandledRejection when page.route raises', async ({ runInlineTest, server }) => {
   const result = await runInlineTest({
     'a.test.ts': `
       import { test, expect } from '@playwright/test';
       test('fail', async ({ page }) => {
-        test.fail();
         await page.route('**/empty.html', async route => {
           throw new Error('foobar');
         });
@@ -46,16 +44,15 @@ test('should lead in unhandledRejection when page.route raises', async ({ runInl
       });
     `,
   }, { workers: 1 });
-  expect(result.interrupted).toBe(1);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('foobar');
 });
 
-test('should lead in uncaughtException when context.route raises', async ({ runInlineTest, server }) => {
+test('should produce uncaughtException when context.route raises', async ({ runInlineTest, server }) => {
   const result = await runInlineTest({
     'a.test.ts': `
       import { test, expect } from '@playwright/test';
       test('fail', async ({ context, page }) => {
-        test.fail();
         await context.route('**/empty.html', route => {
           throw new Error('foobar');
         });
@@ -63,16 +60,15 @@ test('should lead in uncaughtException when context.route raises', async ({ runI
       });
     `,
   }, { workers: 1 });
-  expect(result.interrupted).toBe(1);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('foobar');
 });
 
-test('should lead in unhandledRejection when context.route raises', async ({ runInlineTest, server }) => {
+test('should produce unhandledRejection when context.route raises', async ({ runInlineTest, server }) => {
   const result = await runInlineTest({
     'a.test.ts': `
       import { test, expect } from '@playwright/test';
       test('fail', async ({ context, page }) => {
-        test.fail();
         await context.route('**/empty.html', async route => {
           throw new Error('foobar');
         });
@@ -80,6 +76,6 @@ test('should lead in unhandledRejection when context.route raises', async ({ run
       });
     `,
   }, { workers: 1 });
-  expect(result.interrupted).toBe(1);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('foobar');
 });

tests/playwright-test/runner.spec.ts

@@ -751,3 +751,28 @@ test('slow double SIGINT should be respected in reporter.onExit', async ({ inter
   expect(result.output).toContain('MyReporter.onExit started');
   expect(result.output).not.toContain('MyReporter.onExit finished');
 });
+
+test('unhandled exception in test.fail should restart worker and continue', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+
+      test('bad', async () => {
+        test.fail();
+        console.log('\\n%%bad running worker=' + test.info().workerIndex);
+        setTimeout(() => {
+          throw new Error('oh my!');
+        }, 0);
+        await new Promise(f => setTimeout(f, 1000));
+      });
+
+      test('good', () => {
+        console.log('\\n%%good running worker=' + test.info().workerIndex);
+      });
+    `
+  }, { retries: 1, reporter: 'list' });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(2);
+  expect(result.failed).toBe(0);
+  expect(result.outputLines).toEqual(['bad running worker=0', 'good running worker=1']);
+});