fix(test runner): proper serial mode with beforeAll/afterAll failures (#9183)

src/test/dispatcher.ts

@@ -126,7 +126,9 @@ export class Dispatcher {
       worker.stop();
       worker.didFail = true;
 
-      const retryCandidates = new Set<string>();
+      const failedTestIds = new Set<string>();
+      if (params.failedTestId)
+        failedTestIds.add(params.failedTestId);
 
       // In case of fatal error, report first remaining test as failing with this error,
       // and all others as skipped.
@@ -142,7 +144,7 @@ export class Dispatcher {
           result.error = params.fatalError;
           result.status = first ? 'failed' : 'skipped';
           this._reportTestEnd(test, result);
-          retryCandidates.add(test._id);
+          failedTestIds.add(test._id);
           first = false;
         }
         if (first) {
@@ -156,40 +158,45 @@ export class Dispatcher {
         remaining = [];
       }
 
-      if (params.failedTestId) {
-        retryCandidates.add(params.failedTestId);
+      const retryCandidates = new Set<string>();
+      const serialSuitesWithFailures = new Set<Suite>();
+
+      for (const failedTestId of failedTestIds) {
+        retryCandidates.add(failedTestId);
 
         let outermostSerialSuite: Suite | undefined;
-        for (let parent = this._testById.get(params.failedTestId)!.test.parent; parent; parent = parent.parent) {
+        for (let parent = this._testById.get(failedTestId)!.test.parent; parent; parent = parent.parent) {
           if (parent._parallelMode ===  'serial')
             outermostSerialSuite = parent;
         }
+        if (outermostSerialSuite)
+          serialSuitesWithFailures.add(outermostSerialSuite);
+      }
 
-        if (outermostSerialSuite) {
-          // Failed test belongs to a serial suite. We should skip all future tests
-          // from the same serial suite.
-          remaining = remaining.filter(test => {
-            let parent = test.parent;
-            while (parent && parent !== outermostSerialSuite)
-              parent = parent.parent;
+      // We have failed tests that belong to a serial suite.
+      // We should skip all future tests from the same serial suite.
+      remaining = remaining.filter(test => {
+        let parent = test.parent;
+        while (parent && !serialSuitesWithFailures.has(parent))
+          parent = parent.parent;
 
-            // Does not belong to the same serial suite, keep it.
-            if (!parent)
-              return true;
+        // Does not belong to the failed serial suite, keep it.
+        if (!parent)
+          return true;
 
-            // Emulate a "skipped" run, and drop this test from remaining.
-            const { result } = this._testById.get(test._id)!;
-            this._reporter.onTestBegin?.(test, result);
-            result.status = 'skipped';
-            this._reportTestEnd(test, result);
-            return false;
-          });
+        // Emulate a "skipped" run, and drop this test from remaining.
+        const { result } = this._testById.get(test._id)!;
+        this._reporter.onTestBegin?.(test, result);
+        result.status = 'skipped';
+        this._reportTestEnd(test, result);
+        return false;
+      });
 
-          // Add all tests from the same serial suite for possible retry.
-          // These will only be retried together, because they have the same
-          // "retries" setting and the same number of previous runs.
-          outermostSerialSuite.allTests().forEach(test => retryCandidates.add(test._id));
-        }
+      for (const serialSuite of serialSuitesWithFailures) {
+        // Add all tests from faiiled serial suites for possible retry.
+        // These will only be retried together, because they have the same
+        // "retries" setting and the same number of previous runs.
+        serialSuite.allTests().forEach(test => retryCandidates.add(test._id));
       }
 
       for (const testId of retryCandidates) {

src/test/workerRunner.ts

@@ -32,6 +32,8 @@ import { DeadlineRunner, raceAgainstDeadline } from '../utils/async';
 
 const removeFolderAsync = util.promisify(rimraf);
 
+type TestData = { testId: string, testInfo: TestInfoImpl, type: 'test' | 'beforeAll' | 'afterAll' };
+
 export class WorkerRunner extends EventEmitter {
   private _params: WorkerInitParams;
   private _loader!: Loader;
@@ -47,7 +49,7 @@ export class WorkerRunner extends EventEmitter {
   private _isStopped = false;
   private _runFinished = Promise.resolve();
   private _currentDeadlineRunner: DeadlineRunner<any> | undefined;
-  _currentTest: { testId: string, testInfo: TestInfoImpl, type: 'test' | 'beforeAll' | 'afterAll' } | null = null;
+  _currentTest: TestData | null = null;
 
   constructor(params: WorkerInitParams) {
     super();

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

@@ -100,6 +100,78 @@ test('test.describe.serial should work with retry', async ({ runInlineTest }) =>
   ]);
 });
 
+test('test.describe.serial should work with retry and beforeAll failure', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      const { test } = pwt;
+      test.describe.serial('serial suite', () => {
+        test('test1', async ({}) => {
+          console.log('\\n%%test1');
+        });
+
+        test.describe('inner suite', () => {
+          test.beforeAll(async ({}, testInfo) => {
+            console.log('\\n%%beforeAll');
+            expect(testInfo.retry).toBe(1);
+          });
+          test('test2', async ({}) => {
+            console.log('\\n%%test2');
+          });
+        });
+      });
+    `,
+  }, { retries: 1 });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(result.flaky).toBe(1);
+  expect(result.failed).toBe(0);
+  expect(result.skipped).toBe(0);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%test1',
+    '%%beforeAll',
+    '%%test1',
+    '%%beforeAll',
+    '%%test2',
+  ]);
+});
+
+test('test.describe.serial should work with retry and afterAll failure', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      const { test } = pwt;
+      test.describe.serial('serial suite', () => {
+        test.describe('inner suite', () => {
+          let firstRun = false;
+          test('test1', async ({}, testInfo) => {
+            console.log('\\n%%test1');
+            firstRun = testInfo.retry === 0;
+          });
+          test.afterAll(async ({}, testInfo) => {
+            console.log('\\n%%afterAll');
+            expect(firstRun).toBe(false);
+          });
+        });
+
+        test('test2', async ({}) => {
+          console.log('\\n%%test2');
+        });
+      });
+    `,
+  }, { retries: 1 });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(result.flaky).toBe(1);
+  expect(result.failed).toBe(0);
+  expect(result.skipped).toBe(0);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%test1',
+    '%%afterAll',
+    '%%test1',
+    '%%afterAll',
+    '%%test2',
+  ]);
+});
+
 test('test.describe.serial.only should work', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.ts': `