fix(test runner): separate test fixture scope for beforeAll/afterAll hooks (#22746)

There was a single test fixture scope that covers all hooks, modifiers
and test function. Now beforeAll-like modifiers, beforeAll and afterAll
hooks get a scope each.

Fixes #22256.

packages/playwright-test/src/worker/workerMain.ts

@@ -409,23 +409,23 @@ export class WorkerMain extends ProcessRunner {
           firstAfterHooksError = firstAfterHooksError || afterEachError;
         }
 
+        // Teardown test-scoped fixtures. Attribute to 'test' so that users understand
+        // they should probably increase the test timeout to fix this issue.
+        testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot });
+        debugTest(`tearing down test scope started`);
+        const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
+        debugTest(`tearing down test scope finished`);
+        firstAfterHooksError = firstAfterHooksError || testScopeError;
+
         // Run "afterAll" hooks for suites that are not shared with the next test.
         // In case of failure the worker will be stopped and we have to make sure that afterAll
-        // hooks run before test fixtures teardown.
+        // hooks run before worker fixtures teardown.
         for (const suite of reversedSuites) {
           if (!nextSuites.has(suite) || testInfo._isFailure()) {
             const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo);
             firstAfterHooksError = firstAfterHooksError || afterAllError;
           }
         }
-
-        // Teardown test-scoped fixtures. Attribute to 'test' so that users understand
-        // they should probably increate the test timeout to fix this issue.
-        testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot });
-        debugTest(`tearing down test scope started`);
-        const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
-        debugTest(`tearing down test scope finished`);
-        firstAfterHooksError = firstAfterHooksError || testScopeError;
       });
 
       if (testInfo._isFailure())
@@ -439,10 +439,7 @@ export class WorkerMain extends ProcessRunner {
         // Give it more time for the full cleanup.
         await testInfo._runWithTimeout(async () => {
           debugTest(`running full cleanup after the failure`);
-          for (const suite of reversedSuites) {
+
-            const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo);
-            firstAfterHooksError = firstAfterHooksError || afterAllError;
-          }
           const teardownSlot = { timeout: this._project.project.timeout, elapsed: 0 };
           // Attribute to 'test' so that users understand they should probably increate the test timeout to fix this issue.
           testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: teardownSlot });
@@ -450,6 +447,12 @@ export class WorkerMain extends ProcessRunner {
           const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
           debugTest(`tearing down test scope finished`);
           firstAfterHooksError = firstAfterHooksError || testScopeError;
+
+          for (const suite of reversedSuites) {
+            const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo);
+            firstAfterHooksError = firstAfterHooksError || afterAllError;
+          }
+
           // Attribute to 'teardown' because worker fixtures are not perceived as a part of a test.
           testInfo._timeoutManager.setCurrentRunnable({ type: 'teardown', slot: teardownSlot });
           debugTest(`tearing down worker scope started`);
@@ -512,7 +515,15 @@ export class WorkerMain extends ProcessRunner {
           category: 'hook',
           title: `${hook.type} hook`,
           location: hook.location,
-        }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'));
+        }, async () => {
+          try {
+            await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only');
+          } finally {
+            // Each beforeAll hook has its own scope for test fixtures. Attribute to the same runnable and timeSlot.
+            // Note: we must teardown even after beforeAll fails, because we'll run more beforeAlls.
+            await this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
+          }
+        });
       } catch (e) {
         // Always run all the hooks, and capture the first error.
         beforeAllError = beforeAllError || e;
@@ -540,7 +551,15 @@ export class WorkerMain extends ProcessRunner {
           category: 'hook',
           title: `${hook.type} hook`,
           location: hook.location,
-        }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'));
+        }, async () => {
+          try {
+            await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only');
+          } finally {
+            // Each afterAll hook has its own scope for test fixtures. Attribute to the same runnable and timeSlot.
+            // Note: we must teardown even after afterAll fails, because we'll run more afterAlls.
+            await this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
+          }
+        });
       });
       firstError = firstError || afterAllError;
       debugTest(`${hook.type} hook at "${formatLocation(hook.location)}" finished`);

tests/playwright-test/fixtures.spec.ts

@@ -388,26 +388,26 @@ test('automatic fixtures should work', async ({ runInlineTest }) => {
       test.beforeEach(async ({}) => {
         expect(counterWorker).toBe(1);
         expect(counterTest === 1 || counterTest === 2).toBe(true);
-        expect(counterHooksIncluded === 1 || counterHooksIncluded === 2).toBe(true);
+        expect(counterHooksIncluded === 2 || counterHooksIncluded === 3).toBe(true);
       });
       test('test 1', async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterHooksIncluded).toBe(1);
+        expect(counterHooksIncluded).toBe(2);
         expect(counterTest).toBe(1);
       });
       test('test 2', async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterHooksIncluded).toBe(2);
+        expect(counterHooksIncluded).toBe(3);
         expect(counterTest).toBe(2);
       });
       test.afterEach(async ({}) => {
         expect(counterWorker).toBe(1);
         expect(counterTest === 1 || counterTest === 2).toBe(true);
-        expect(counterHooksIncluded === 1 || counterHooksIncluded === 2).toBe(true);
+        expect(counterHooksIncluded === 2 || counterHooksIncluded === 3).toBe(true);
       });
       test.afterAll(async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterHooksIncluded).toBe(2);
+        expect(counterHooksIncluded).toBe(4);
         expect(counterTest).toBe(2);
       });
     `

tests/playwright-test/hooks.spec.ts

@@ -44,9 +44,15 @@ test('hooks should work with fixtures', async ({ runInlineTest }) => {
         test.beforeAll(async ({ w, t }) => {
           global.logs.push('beforeAll-' + w + '-' + t);
         });
+        test.beforeAll(async ({ w, t }) => {
+          global.logs.push('beforeAll2-' + w + '-' + t);
+        });
         test.afterAll(async ({ w, t }) => {
           global.logs.push('afterAll-' + w + '-' + t);
         });
+        test.afterAll(async ({ w, t }) => {
+          global.logs.push('afterAll2-' + w + '-' + t);
+        });
 
         test.beforeEach(async ({ w, t }) => {
           global.logs.push('beforeEach-' + w + '-' + t);
@@ -65,10 +71,20 @@ test('hooks should work with fixtures', async ({ runInlineTest }) => {
           '+w',
           '+t',
           'beforeAll-17-42',
-          'beforeEach-17-42',
+          '-t',
-          'test-17-42',
+          '+t',
-          'afterEach-17-42',
+          'beforeAll2-17-43',
-          'afterAll-17-42',
+          '-t',
+          '+t',
+          'beforeEach-17-44',
+          'test-17-44',
+          'afterEach-17-44',
+          '-t',
+          '+t',
+          'afterAll-17-45',
+          '-t',
+          '+t',
+          'afterAll2-17-46',
           '-t',
           '+t',
         ]);

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

@@ -700,6 +700,10 @@ test('should nest steps based on zones', async ({ runInlineTest }) => {
           ],
           location: { file: 'a.test.ts', line: 'number', column: 'number' }
         },
+        {
+          title: 'browserContext.close',
+          category: 'pw:api'
+        },
         {
           title: 'afterAll hook',
           category: 'hook',
@@ -712,10 +716,6 @@ test('should nest steps based on zones', async ({ runInlineTest }) => {
           ],
           location: { file: 'a.test.ts', line: 'number', column: 'number' }
         },
-        {
-          title: 'browserContext.close',
-          category: 'pw:api'
-        }
       ]
     }
   ]);

tests/playwright-test/timeout.spec.ts

@@ -331,6 +331,9 @@ test('test timeout should still run hooks before fixtures teardown', async ({ ru
         await new Promise(f => setTimeout(f, 500));
         console.log('\\n%%afterAll-2');
       });
+      test.afterEach(async () => {
+        console.log('\\n%%afterEach');
+      });
       test('test fail', async ({}) => {
         test.setTimeout(100);
         console.log('\\n%%test');
@@ -344,9 +347,10 @@ test('test timeout should still run hooks before fixtures teardown', async ({ ru
   expect(result.outputLines).toEqual([
     'before-auto',
     'test',
+    'afterEach',
+    'after-auto',
     'afterAll-1',
     'afterAll-2',
-    'after-auto',
   ]);
 });