fix(test runner): do not reuse worker that did not teardown scopes (#9872)

Two bug fixes:
- Do not use the worker that is being shutdown for a new job.
- Report unhandled errors during "expected to fail" tests as
  fatal errors.

packages/playwright-test/src/dispatcher.ts

@@ -63,7 +63,7 @@ export class Dispatcher {
     const job = this._queue[0];
 
     // 2. Find a worker with the same hash, or just some free worker.
-    let index = this._workerSlots.findIndex(w => !w.busy && w.worker && w.worker.hash() === job.workerHash);
+    let index = this._workerSlots.findIndex(w => !w.busy && w.worker && w.worker.hash() === job.workerHash && !w.worker.didSendStop());
     if (index === -1)
       index = this._workerSlots.findIndex(w => !w.busy);
     // No workers available, bail out.
@@ -86,8 +86,8 @@ export class Dispatcher {
   private async _startJobInWorker(index: number, job: TestGroup) {
     let worker = this._workerSlots[index].worker;
 
-    // 1. Restart the worker if it has the wrong hash.
-    if (worker && worker.hash() !== job.workerHash) {
+    // 1. Restart the worker if it has the wrong hash or is being stopped already.
+    if (worker && (worker.hash() !== job.workerHash || worker.didSendStop())) {
       await worker.stop();
       worker = undefined;
       if (this._isStopped) // Check stopped signal after async hop.
@@ -405,7 +405,7 @@ class Worker extends EventEmitter {
   private process: child_process.ChildProcess;
   private _hash: string;
   private workerIndex: number;
-  private didSendStop = false;
+  private _didSendStop = false;
   private _didFail = false;
   private didExit = false;
 
@@ -427,7 +427,7 @@ class Worker extends EventEmitter {
     });
     this.process.on('exit', () => {
       this.didExit = true;
-      this.emit('exit', this.didSendStop /* expectedly */);
+      this.emit('exit', this._didSendStop /* expectedly */);
     });
     this.process.on('error', e => {});  // do not yell at a send to dead process.
     this.process.on('message', (message: any) => {
@@ -461,6 +461,10 @@ class Worker extends EventEmitter {
     return this._didFail;
   }
 
+  didSendStop() {
+    return this._didSendStop;
+  }
+
   hash() {
     return this._hash;
   }
@@ -470,9 +474,9 @@ class Worker extends EventEmitter {
       this._didFail = true;
     if (this.didExit)
       return;
-    if (!this.didSendStop) {
+    if (!this._didSendStop) {
       this.process.send({ method: 'stop' });
-      this.didSendStop = true;
+      this._didSendStop = true;
     }
     await new Promise(f => this.once('exit', f));
   }

packages/playwright-test/src/workerRunner.ts

@@ -90,7 +90,17 @@ export class WorkerRunner extends EventEmitter {
   }
 
   unhandledError(error: Error | any) {
-    if (this._currentTest && this._currentTest.type === 'test') {
+    // 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.
+    //
+    // 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.
+    if (this._currentTest && this._currentTest.type === 'test' && this._currentTest.testInfo.expectedStatus !== 'failed') {
       if (!this._currentTest.testInfo.error) {
         this._currentTest.testInfo.status = 'failed';
         this._currentTest.testInfo.error = serializeError(error);

tests/page/page-request-intercept.spec.ts

@@ -221,10 +221,9 @@ it('should give access to the intercepted response status text', async ({ page,
   // @ts-expect-error
   const response = await route._continueToResponse();
 
+  await Promise.all([route.fulfill({ response }), evalPromise]);
   expect(response.statusText()).toBe('You are awesome');
   expect(response.url()).toBe(server.PREFIX + '/title.html');
-
-  await Promise.all([route.fulfill({ response }), evalPromise]);
 });
 
 it('should give access to the intercepted response body', async ({ page, server }) => {

tests/playwright-test/basic.spec.ts

@@ -396,3 +396,29 @@ test('should report unhandled rejection during worker shutdown', async ({ runInl
   expect(result.output).toContain('Error: Unhandled');
   expect(result.output).toContain('a.test.ts:7:33');
 });
+
+test('should not reuse worker after unhandled rejection in test.fail', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      const test = pwt.test.extend({
+        needsCleanup: async ({}, use) => {
+          await use();
+          await new Promise(f => setTimeout(f, 3000));
+        }
+      });
+
+      test('failing', async ({ needsCleanup }) => {
+        test.fail();
+        new Promise(() => { throw new Error('Oh my!') });
+      });
+
+      test('passing', async () => {
+      });
+    `
+  }, { workers: 1 });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.skipped).toBe(1);
+  expect(result.output).toContain(`Error: Oh my!`);
+  expect(result.output).not.toContain(`Did not teardown test scope`);
+});