fix(test runner): do not run automatic fixtures for beforeAll hooks (#14104)

There are a few issues this covers:
- Some fixtures like `page` and `context` are not allowed in `beforeAll`
  hooks, so using them in automatic fixture makes it throw.
- Running automatic fixture solely for `afterAll` is unexpected.
  This currently happens when `afterAll` is run for cleanup after
  fixture timeout/throw.

For built-in playwright fixture, we keep `'all-hooks-included'` auto mode.

Added a doc explaining the execution order.

docs/src/test-fixtures-js.md

@@ -609,3 +609,144 @@ const config: PlaywrightTestConfig<MyOptions> = {
 };
 export default config;
 ```
+
+## Execution order
+
+Each fixture has a setup and teardown phase separated by the `await use()` call in the fixture. Setup is executed before the fixture is used by the test/hook, and teardown is executed when the fixture will not be used by the test/hook anymore.
+
+Fixtures follow these rules to determine the execution order:
+* When fixture A depends on fixture B: B is always set up before A and teared down after A.
+* Non-automatic fixtures are executed lazily, only when the test/hook needs them.
+* Test-scoped fixtures are teared down after each test, while worker-scoped fixtures are only teared down when the worker process executing tests is shutdown.
+
+Consider the following example:
+
+```js js-flavor=js
+const { test: base } = require('@playwright/test');
+
+const test = base.extend({
+  workerFixture: [async ({ browser }) => {
+    // workerFixture setup...
+    await use('workerFixture');
+    // workerFixture teardown...
+  }, { scope: 'worker' }],
+
+  autoWorkerFixture: [async ({ browser }) => {
+    // autoWorkerFixture setup...
+    await use('autoWorkerFixture');
+    // autoWorkerFixture teardown...
+  }, { scope: 'worker', auto: true }],
+
+  testFixture: [async ({ page, workerFixture }) => {
+    // testFixture setup...
+    await use('testFixture');
+    // testFixture teardown...
+  }, { scope: 'test' }],
+
+  autoTestFixture: [async () => {
+    // autoTestFixture setup...
+    await use('autoTestFixture');
+    // autoTestFixture teardown...
+  }, { scope: 'test', auto: true }],
+
+  unusedFixture: [async ({ page }) => {
+    // unusedFixture setup...
+    await use('unusedFixture');
+    // unusedFixture teardown...
+  }, { scope: 'test' }],
+});
+
+test.beforeAll(async () => { /* ... */ });
+test.beforeEach(async ({ page }) => { /* ... */ });
+test('first test', async ({ page }) => { /* ... */ });
+test('second test', async ({ testFixture }) => { /* ... */ });
+test.afterEach(async () => { /* ... */ });
+test.afterAll(async () => { /* ... */ });
+```
+
+```js js-flavor=ts
+import { test as base } from '@playwright/test';
+
+const test = base.extend<{
+  testFixture: string,
+  autoTestFixture: string,
+  unusedFixture: string,
+}, {
+  workerFixture: string,
+  autoWorkerFixture: string,
+}>({
+  workerFixture: [async ({ browser }) => {
+    // workerFixture setup...
+    await use('workerFixture');
+    // workerFixture teardown...
+  }, { scope: 'worker' }],
+
+  autoWorkerFixture: [async ({ browser }) => {
+    // autoWorkerFixture setup...
+    await use('autoWorkerFixture');
+    // autoWorkerFixture teardown...
+  }, { scope: 'worker', auto: true }],
+
+  testFixture: [async ({ page, workerFixture }) => {
+    // testFixture setup...
+    await use('testFixture');
+    // testFixture teardown...
+  }, { scope: 'test' }],
+
+  autoTestFixture: [async () => {
+    // autoTestFixture setup...
+    await use('autoTestFixture');
+    // autoTestFixture teardown...
+  }, { scope: 'test', auto: true }],
+
+  unusedFixture: [async ({ page }) => {
+    // unusedFixture setup...
+    await use('unusedFixture');
+    // unusedFixture teardown...
+  }, { scope: 'test' }],
+});
+
+test.beforeAll(async () => { /* ... */ });
+test.beforeEach(async ({ page }) => { /* ... */ });
+test('first test', async ({ page }) => { /* ... */ });
+test('second test', async ({ testFixture }) => { /* ... */ });
+test.afterEach(async () => { /* ... */ });
+test.afterAll(async () => { /* ... */ });
+```
+
+Normally, if all tests pass and no errors are thrown, the order of execution is as following.
+* worker setup and `beforeAll` section:
+  * `browser` setup because it is required by `autoWorkerFixture`.
+  * `autoWorkerFixture` setup because automatic worker fixtures are always set up before anything else.
+  * `beforeAll` runs.
+* `first test` section:
+  * `autoTestFixture` setup because automatic test fixtures are always set up before test and `beforeEach` hooks.
+  * `page` setup because it is required in `beforeEach` hook.
+  * `beforeEach` runs.
+  * `first test` runs.
+  * `afterEach` runs.
+  * `page` teardown because it is a test-scoped fixture and should be teared down after the test finishes.
+  * `autoTestFixture` teardown because it is a test-scoped fixture and should be teared down after the test finishes.
+* `second test` section:
+  * `autoTestFixture` setup because automatic test fixtures are always set up before test and `beforeEach` hooks.
+  * `page` setup because it is required in `beforeEach` hook.
+  * `beforeEach` runs.
+  * `workerFixture` setup because it is required by `testFixture` that is required by the `second test`.
+  * `testFixture` setup because it is required by the `second test`.
+  * `second test` runs.
+  * `afterEach` runs.
+  * `testFixture` teardown because it is a test-scoped fixture and should be teared down after the test finishes.
+  * `page` teardown because it is a test-scoped fixture and should be teared down after the test finishes.
+  * `autoTestFixture` teardown because it is a test-scoped fixture and should be teared down after the test finishes.
+* `afterAll` and worker teardown section:
+  * `afterAll` runs.
+  * `workerFixture` teardown because it is a workers-scoped fixture and should be teared down once at the end.
+  * `autoWorkerFixture` teardown because it is a workers-scoped fixture and should be teared down once at the end.
+  * `browser` teardown because it is a workers-scoped fixture and should be teared down once at the end.
+
+A few observations:
+* `page` and `autoTestFixture` are set up and teared down for each test, as test-scoped fixtures.
+* `unusedFixture` is never set up because it is not used by any tests/hooks.
+* `testFixture` depends on `workerFixture` and triggers its setup.
+* `workerFixture` is lazily set up before the second test, but teared down once during worker shutdown, as a worker-scoped fixture.
+* `autoWorkerFixture` is set up for `beforeAll` hook, but `autoTestFixture` is not.

packages/playwright-test/src/fixtures.ts

@@ -22,8 +22,9 @@ import type { TestInfoImpl } from './testInfo';
 import type { FixtureDescription, TimeoutManager } from './timeoutManager';
 
 type FixtureScope = 'test' | 'worker';
+type FixtureAuto = boolean | 'all-hooks-included';
 const kScopeOrder: FixtureScope[] = ['test', 'worker'];
-type FixtureOptions = { auto?: boolean, scope?: FixtureScope, option?: boolean, timeout?: number | undefined };
+type FixtureOptions = { auto?: FixtureAuto, scope?: FixtureScope, option?: boolean, timeout?: number | undefined };
 type FixtureTuple = [ value: any, options: FixtureOptions ];
 type FixtureRegistration = {
   // Fixture registration location.
@@ -34,7 +35,7 @@ type FixtureRegistration = {
   // Either a fixture function, or a fixture value.
   fn: Function | any;
   // Auto fixtures always run without user explicitly mentioning them.
-  auto: boolean;
+  auto: FixtureAuto;
   // An "option" fixture can have a value set in the config.
   option: boolean;
   // Custom title to be used instead of the name, internal-only.
@@ -160,10 +161,10 @@ export class FixturePool {
       for (const entry of Object.entries(fixtures)) {
         const name = entry[0];
         let value = entry[1];
-        let options: { auto: boolean, scope: FixtureScope, option: boolean, timeout: number | undefined, customTitle: string | undefined } | undefined;
+        let options: { auto: FixtureAuto, scope: FixtureScope, option: boolean, timeout: number | undefined, customTitle: string | undefined } | undefined;
         if (isFixtureTuple(value)) {
           options = {
-            auto: !!value[1].auto,
+            auto: value[1].auto ?? false,
             scope: value[1].scope || 'test',
             option: !!value[1].option,
             timeout: value[1].timeout,
@@ -293,11 +294,17 @@ export class FixtureRunner {
       throw error;
   }
 
-  async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl): Promise<object> {
+  async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object> {
-    // Install all automatic fixtures.
+    // Install automatic fixtures.
     for (const registration of this.pool!.registrations.values()) {
-      const shouldSkip = !testInfo && registration.scope === 'test';
+      if (registration.auto === false)
-      if (registration.auto && !shouldSkip)
+        continue;
+      let shouldRun = true;
+      if (autoFixtures === 'all-hooks-only')
+        shouldRun = registration.scope === 'worker' || registration.auto === 'all-hooks-included';
+      else if (autoFixtures === 'worker')
+        shouldRun = registration.scope === 'worker';
+      if (shouldRun)
         await this.setupFixtureForRegistration(registration, testInfo);
     }
 
@@ -312,8 +319,8 @@ export class FixtureRunner {
     return params;
   }
 
-  async resolveParametersAndRunFunction(fn: Function, testInfo: TestInfoImpl) {
+  async resolveParametersAndRunFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only') {
-    const params = await this.resolveParametersForFunction(fn, testInfo);
+    const params = await this.resolveParametersForFunction(fn, testInfo, autoFixtures);
     return fn(params, testInfo);
   }
 

packages/playwright-test/src/index.ts

@@ -429,7 +429,7 @@ export const test = _baseTest.extend<TestFixtures, WorkerFixtures>({
       else
         await fs.promises.unlink(file).catch(() => {});
     }));
-  }, { auto: true,  _title: 'built-in playwright configuration' } as any],
+  }, { auto: 'all-hooks-included',  _title: 'built-in playwright configuration' } as any],
 
   _contextFactory: [async ({ browser, video, _artifactsDir }, use, testInfo) => {
     let videoMode = typeof video === 'string' ? video : video.mode;

packages/playwright-test/src/workerRunner.ts

@@ -349,7 +349,7 @@ export class WorkerRunner extends EventEmitter {
 
         // Setup fixtures required by the test.
         testInfo._timeoutManager.setCurrentRunnable({ type: 'test' });
-        const params = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo);
+        const params = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test');
         beforeHooksStep.complete({}); // Report fixture hooks step as completed.
 
         // Now run the test itself.
@@ -447,7 +447,7 @@ export class WorkerRunner extends EventEmitter {
       if (actualScope !== scope)
         continue;
       testInfo._timeoutManager.setCurrentRunnable({ type: modifier.type, location: modifier.location, slot: timeSlot });
-      const result = await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(modifier.fn, testInfo), {
+      const result = await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(modifier.fn, testInfo, scope), {
         category: 'hook',
         title: `${modifier.type} modifier`,
         canHaveChildren: true,
@@ -472,7 +472,7 @@ export class WorkerRunner extends EventEmitter {
         // Separate time slot for each "beforeAll" hook.
         const timeSlot = { timeout: this._project.timeout, elapsed: 0 };
         testInfo._timeoutManager.setCurrentRunnable({ type: 'beforeAll', location: hook.location, slot: timeSlot });
-        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo), {
+        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'), {
           category: 'hook',
           title: `${hook.type} hook`,
           canHaveChildren: true,
@@ -500,7 +500,7 @@ export class WorkerRunner extends EventEmitter {
         // Separate time slot for each "afterAll" hook.
         const timeSlot = { timeout: this._project.timeout, elapsed: 0 };
         testInfo._timeoutManager.setCurrentRunnable({ type: 'afterAll', location: hook.location, slot: timeSlot });
-        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo), {
+        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'), {
           category: 'hook',
           title: `${hook.type} hook`,
           canHaveChildren: true,
@@ -519,7 +519,7 @@ export class WorkerRunner extends EventEmitter {
     for (const hook of hooks) {
       try {
         testInfo._timeoutManager.setCurrentRunnable({ type, location: hook.location, slot: timeSlot });
-        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo), {
+        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'test'), {
           category: 'hook',
           title: `${hook.type} hook`,
           canHaveChildren: true,

tests/playwright-test/fixtures.spec.ts

@@ -297,6 +297,7 @@ test('automatic fixtures should work', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
       let counterTest = 0;
+      let counterHooksIncluded = 0;
       let counterWorker = 0;
       const test = pwt.test;
       test.use({
@@ -305,6 +306,11 @@ test('automatic fixtures should work', async ({ runInlineTest }) => {
           await runTest();
         }, { auto: true } ],
 
+        automaticTestFixtureHooksIncluded: [ async ({}, runTest) => {
+          ++counterHooksIncluded;
+          await runTest();
+        }, { auto: 'all-hooks-included' } ],
+
         automaticWorkerFixture: [ async ({}, runTest) => {
           ++counterWorker;
           await runTest();
@@ -312,26 +318,32 @@ test('automatic fixtures should work', async ({ runInlineTest }) => {
       });
       test.beforeAll(async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterTest).toBe(1);
+        expect(counterHooksIncluded).toBe(1);
+        expect(counterTest).toBe(0);
       });
       test.beforeEach(async ({}) => {
         expect(counterWorker).toBe(1);
         expect(counterTest === 1 || counterTest === 2).toBe(true);
+        expect(counterHooksIncluded === 1 || counterHooksIncluded === 2).toBe(true);
       });
       test('test 1', async ({}) => {
         expect(counterWorker).toBe(1);
+        expect(counterHooksIncluded).toBe(1);
         expect(counterTest).toBe(1);
       });
       test('test 2', async ({}) => {
         expect(counterWorker).toBe(1);
+        expect(counterHooksIncluded).toBe(2);
         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);
       });
       test.afterAll(async ({}) => {
         expect(counterWorker).toBe(1);
+        expect(counterHooksIncluded).toBe(2);
         expect(counterTest).toBe(2);
       });
     `

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

@@ -176,6 +176,41 @@ test('should not save sources when not requested', async ({ runInlineTest }, tes
   expect([...resources.keys()].filter(f => f.includes('src@'))).toHaveLength(0);
 });
 
+test('should work in serial mode', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = { use: { trace: 'retain-on-failure' } };
+    `,
+    'a.spec.ts': `
+      const { test } = pwt;
+
+      test.describe.serial('serial', () => {
+        let page;
+        test.beforeAll(async ({ browser }) => {
+          page = await browser.newPage();
+        });
+
+        test.afterAll(async () => {
+          await page.close();
+        });
+
+        test('passes', async ({}, testInfo) => {
+        });
+
+        test('fails', async ({}, testInfo) => {
+          throw new Error('oh my');
+        });
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'a-serial-passes', 'trace.zip'))).toBeFalsy();
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'a-serial-fails', 'trace.zip'))).toBeTruthy();
+});
+
 async function parseTrace(file: string): Promise<Map<string, Buffer>> {
   const zipFS = new ZipFileSystem(file);
   const resources = new Map<string, Buffer>();