fix(test runner): serial suites inside parallel suite should run in parallel (#15769)

packages/playwright-test/src/runner.ts

@@ -653,10 +653,16 @@ function createTestGroups(rootSuite: Suite, workers: number): TestGroup[] {
   const groups = new Map<string, Map<string, {
     // Tests that must be run in order are in the same group.
     general: TestGroup,
-    // Tests that may be run independently each has a dedicated group with a single test.
+
-    parallel: TestGroup[],
+    // There are 3 kinds of parallel tests:
-    // Tests that are marked as parallel but have beforeAll/afterAll hooks should be grouped
+    // - Tests belonging to parallel suites, without beforeAll/afterAll hooks.
-    // as much as possible. We split them into equally sized groups, one per worker.
+    //   These can be run independently, they are put into their own group, key === test.
+    // - Tests belonging to parallel suites, with beforeAll/afterAll hooks.
+    //   These should share the worker as much as possible, put into single parallelWithHooks group.
+    //   We'll divide them into equally-sized groups later.
+    // - Tests belonging to serial suites inside parallel suites.
+    //   These should run as a serial group, each group is independent, key === serial suite.
+    parallel: Map<Suite | TestCase, TestGroup>,
     parallelWithHooks: TestGroup,
   }>>();
 
@@ -681,29 +687,34 @@ function createTestGroups(rootSuite: Suite, workers: number): TestGroup[] {
       if (!withRequireFile) {
         withRequireFile = {
           general: createGroup(test),
-          parallel: [],
+          parallel: new Map(),
           parallelWithHooks: createGroup(test),
         };
         withWorkerHash.set(test._requireFile, withRequireFile);
       }
 
+      // Note that a parallel suite cannot be inside a serial suite. This is enforced in TestType.
       let insideParallel = false;
-      let insideSerial = false;
+      let outerMostSerialSuite: Suite | undefined;
       let hasAllHooks = false;
       for (let parent: Suite | undefined = test.parent; parent; parent = parent.parent) {
-        insideSerial = insideSerial || parent._parallelMode === 'serial';
+        if (parent._parallelMode === 'serial')
-        // Serial cancels out any enclosing parallel.
+          outerMostSerialSuite = parent;
-        insideParallel = insideParallel || (!insideSerial && parent._parallelMode === 'parallel');
+        insideParallel = insideParallel || parent._parallelMode === 'parallel';
         hasAllHooks = hasAllHooks || parent._hooks.some(hook => hook.type === 'beforeAll' || hook.type === 'afterAll');
       }
 
       if (insideParallel) {
-        if (hasAllHooks) {
+        if (hasAllHooks && !outerMostSerialSuite) {
           withRequireFile.parallelWithHooks.tests.push(test);
         } else {
-          const group = createGroup(test);
+          const key = outerMostSerialSuite || test;
+          let group = withRequireFile.parallel.get(key);
+          if (!group) {
+            group = createGroup(test);
+            withRequireFile.parallel.set(key, group);
+          }
           group.tests.push(test);
-          withRequireFile.parallel.push(group);
         }
       } else {
         withRequireFile.general.tests.push(test);
@@ -714,10 +725,14 @@ function createTestGroups(rootSuite: Suite, workers: number): TestGroup[] {
   const result: TestGroup[] = [];
   for (const withWorkerHash of groups.values()) {
     for (const withRequireFile of withWorkerHash.values()) {
+      // Tests without parallel mode should run serially as a single group.
       if (withRequireFile.general.tests.length)
         result.push(withRequireFile.general);
-      result.push(...withRequireFile.parallel);
 
+      // Parallel test groups without beforeAll/afterAll can be run independently.
+      result.push(...withRequireFile.parallel.values());
+
+      // Tests with beforeAll/afterAll should try to share workers as much as possible.
       const parallelWithHooksGroupSize = Math.ceil(withRequireFile.parallelWithHooks.tests.length / workers);
       let lastGroup: TestGroup | undefined;
       for (const test of withRequireFile.parallelWithHooks.tests) {

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

@@ -330,22 +330,41 @@ test('test.describe.serial should work inside test.describe.parallel', async ({
       test.describe.parallel('parallel suite', () => {
         test.describe.serial('serial suite', () => {
           test('one', async ({}) => {
-            await new Promise(f => setTimeout(f, 1000));
+            await new Promise(f => setTimeout(f, 2000));
-            console.log('\\n%%one');
+            console.log('\\n%%1-one');
           });
           test('two', async ({}) => {
-            await new Promise(f => setTimeout(f, 500));
+            await new Promise(f => setTimeout(f, 1000));
-            console.log('\\n%%two');
+            console.log('\\n%%1-two');
+          });
+        });
+
+        test.describe.serial('serial suite 2', () => {
+          test('one', async ({}) => {
+            await new Promise(f => setTimeout(f, 2000));
+            console.log('\\n%%2-one');
+          });
+          test('two', async ({}) => {
+            await new Promise(f => setTimeout(f, 1000));
+            console.log('\\n%%2-two');
           });
         });
       });
     `,
   }, { workers: 2 });
   expect(result.exitCode).toBe(0);
-  expect(result.passed).toBe(2);
+  expect(result.passed).toBe(4);
-  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+  expect(result.output).toContain('Running 4 tests using 2 workers');
-    '%%one',
+  const lines = result.output.split('\n').filter(line => line.startsWith('%%'));
-    '%%two',
+  // First test in each worker started before the second test in the other.
+  // This means they were actually running in parallel.
+  expect(lines.indexOf('%%1-one')).toBeLessThan(lines.indexOf('%%2-two'));
+  expect(lines.indexOf('%%2-one')).toBeLessThan(lines.indexOf('%%1-two'));
+  expect(lines.sort()).toEqual([
+    '%%1-one',
+    '%%1-two',
+    '%%2-one',
+    '%%2-two',
   ]);
 });