From c3beb71b07d988b7e600711dc81d16d5f56d5a08 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 13 May 2022 11:17:20 +0100 Subject: [PATCH] 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 | 141 ++++++++++++++++++ packages/playwright-test/src/fixtures.ts | 27 ++-- packages/playwright-test/src/index.ts | 2 +- packages/playwright-test/src/workerRunner.ts | 10 +- tests/playwright-test/fixtures.spec.ts | 14 +- .../playwright-test/playwright.trace.spec.ts | 35 +++++ 6 files changed, 212 insertions(+), 17 deletions(-) diff --git a/docs/src/test-fixtures-js.md b/docs/src/test-fixtures-js.md index 0125a75a98..914759afc9 100644 --- a/docs/src/test-fixtures-js.md +++ b/docs/src/test-fixtures-js.md @@ -609,3 +609,144 @@ const config: PlaywrightTestConfig = { }; 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. diff --git a/packages/playwright-test/src/fixtures.ts b/packages/playwright-test/src/fixtures.ts index c82e248264..bbf6a98db7 100644 --- a/packages/playwright-test/src/fixtures.ts +++ b/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 { - // Install all automatic fixtures. + async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise { + // Install automatic fixtures. for (const registration of this.pool!.registrations.values()) { - const shouldSkip = !testInfo && registration.scope === 'test'; - if (registration.auto && !shouldSkip) + if (registration.auto === false) + 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) { - const params = await this.resolveParametersForFunction(fn, testInfo); + async resolveParametersAndRunFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only') { + const params = await this.resolveParametersForFunction(fn, testInfo, autoFixtures); return fn(params, testInfo); } diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index 9440d50f6d..76e31d5b94 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -429,7 +429,7 @@ export const test = _baseTest.extend({ 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; diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index 706ebd0bf3..70841081f0 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/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, diff --git a/tests/playwright-test/fixtures.spec.ts b/tests/playwright-test/fixtures.spec.ts index 4f93658d16..6abc16b6ac 100644 --- a/tests/playwright-test/fixtures.spec.ts +++ b/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); }); ` diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index d74eee0e2e..5ccf498254 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/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> { const zipFS = new ZipFileSystem(file); const resources = new Map();