fix(test.fail): expect() failure should not skip future tests (microsoft#26663)

dgozman · web-flow · commit c90c9431540e · 2023-08-24T08:32:14.000-07:00
We used to stop the worker that would skip future tests. Regressed in microsoft#11850. Fixes microsoft#26435.
diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts
@@ -164,28 +164,41 @@ export class WorkerMain extends ProcessRunner {
   }
 
   unhandledError(error: Error | any) {
+    const failWithFatalErrorAndStop = () => {
+      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.
+    //
     // The only exception is the expect() error that we still consider ok.
     const isExpectError = (error instanceof Error) && !!(error as any).matcherResult;
-    const isCurrentTestExpectedToFail = this._currentTest?.expectedStatus === 'failed';
-    const shouldConsiderAsTestError = isExpectError || !isCurrentTestExpectedToFail;
-    if (this._currentTest && shouldConsiderAsTestError) {
+    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 {
-      // No current test - fatal error.
-      if (!this._fatalErrors.length)
-        this._fatalErrors.push(serializeError(error));
+      failWithFatalErrorAndStop();
     }
-    void this._stop();
   }
 
   private async _loadIfNeeded() {
diff --git a/tests/playwright-test/basic.spec.ts b/tests/playwright-test/basic.spec.ts
@@ -470,6 +470,35 @@ test('should allow unhandled expects in test.fail', async ({ runInlineTest }) =>
   expect(result.output).not.toContain(`Error: expect`);
 });
 
+test('should not skip tests after test.fail', async ({ runInlineTest, server }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('failing', async ({}) => {
+        test.fail();
+        expect(Promise.resolve('a')).resolves.toBe('b');
+        await new Promise(f => setTimeout(f, 1000));
+      });
+    `,
+    'b.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('passing', async ({}) => {
+        console.log('b-passing');
+      });
+    `,
+    'c.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('passing', async ({}) => {
+        console.log('c-passing');
+      });
+    `,
+  }, { workers: '1' });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(3);
+  expect(result.output).toContain('b-passing');
+  expect(result.output).toContain('c-passing');
+});
+
 test('should support describe.skip', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'nested-skip.spec.ts': `
