diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 9a3bedc429..f889d7df1e 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -16,7 +16,7 @@ import { formatLocation, debugTest, filterStackFile } from '../util'; import { ManualPromise } from 'playwright-core/lib/utils'; -import type { TestInfoImpl, TestStepInternal } from './testInfo'; +import type { TestInfoImpl } from './testInfo'; import type { FixtureDescription } from './timeoutManager'; import { fixtureParameterNames, type FixturePool, type FixtureRegistration, type FixtureScope } from '../common/fixtures'; import type { WorkerInfo } from '../../types/test'; @@ -28,10 +28,13 @@ class Fixture { value: any; failed = false; - _useFuncFinished: ManualPromise | undefined; - _selfTeardownComplete: Promise | undefined; - _teardownWithDepsComplete: Promise | undefined; - _runnableDescription: FixtureDescription; + private _useFuncFinished: ManualPromise | undefined; + private _selfTeardownComplete: Promise | undefined; + private _teardownWithDepsComplete: Promise | undefined; + private _setupDescription: FixtureDescription; + private _teardownDescription: FixtureDescription; + private _shouldGenerateStep = false; + private _isInternalFixture = false; _deps = new Set(); _usages = new Set(); @@ -40,7 +43,7 @@ class Fixture { this.registration = registration; this.value = null; const title = this.registration.customTitle || this.registration.name; - this._runnableDescription = { + this._setupDescription = { title, phase: 'setup', location: registration.location, @@ -49,6 +52,9 @@ class Fixture { elapsed: 0, } }; + this._teardownDescription = { ...this._setupDescription, phase: 'teardown' }; + this._shouldGenerateStep = !this.registration.hideStep && !this.registration.name.startsWith('_') && !this.registration.option; + this._isInternalFixture = this.registration.location && filterStackFile(this.registration.location.file); } async setup(testInfo: TestInfoImpl) { @@ -74,13 +80,25 @@ class Fixture { } } - // Break the registration function into before/after steps. Create these before/after stacks - // w/o scopes, and create single mutable step that will be converted into the after step. - const shouldGenerateStep = !this.registration.hideStep && !this.registration.name.startsWith('_') && !this.registration.option; - const isInternalFixture = this.registration.location && filterStackFile(this.registration.location.file); - let beforeStep: TestStepInternal | undefined; - let afterStep: TestStepInternal | undefined; + testInfo._timeoutManager.setCurrentFixture(this._setupDescription); + const beforeStep = this._shouldGenerateStep ? testInfo._addStep({ + title: `fixture: ${this.registration.name}`, + category: 'fixture', + location: this._isInternalFixture ? this.registration.location : undefined, + wallTime: Date.now(), + }) : undefined; + try { + await this._setupInternal(testInfo, params); + beforeStep?.complete({}); + } catch (error) { + beforeStep?.complete({ error }); + throw error; + } finally { + testInfo._timeoutManager.setCurrentFixture(undefined); + } + } + private async _setupInternal(testInfo: TestInfoImpl, params: object) { let called = false; const useFuncStarted = new ManualPromise(); debugTest(`setup ${this.registration.name}`); @@ -89,41 +107,17 @@ class Fixture { throw new Error(`Cannot provide fixture value for the second time`); called = true; this.value = value; - beforeStep?.complete({}); this._useFuncFinished = new ManualPromise(); useFuncStarted.resolve(); await this._useFuncFinished; - - if (shouldGenerateStep) { - afterStep = testInfo._addStep({ - wallTime: Date.now(), - title: `fixture: ${this.registration.name}`, - category: 'fixture', - location: isInternalFixture ? this.registration.location : undefined, - }); - } }; const workerInfo: WorkerInfo = { config: testInfo.config, parallelIndex: testInfo.parallelIndex, workerIndex: testInfo.workerIndex, project: testInfo.project }; const info = this.registration.scope === 'worker' ? workerInfo : testInfo; - testInfo._timeoutManager.setCurrentFixture(this._runnableDescription); - - if (shouldGenerateStep) { - beforeStep = testInfo._addStep({ - title: `fixture: ${this.registration.name}`, - category: 'fixture', - location: isInternalFixture ? this.registration.location : undefined, - wallTime: Date.now(), - }); - } - this._selfTeardownComplete = (async () => { try { await this.registration.fn(params, useFunc, info); - afterStep?.complete({}); } catch (error) { - beforeStep?.complete({ error }); - afterStep?.complete({ error }); this.failed = true; if (!useFuncStarted.isDone()) useFuncStarted.reject(error); @@ -131,40 +125,43 @@ class Fixture { throw error; } })(); - await useFuncStarted; - testInfo._timeoutManager.setCurrentFixture(undefined); } async teardown(testInfo: TestInfoImpl) { - if (this._teardownWithDepsComplete) { - // When we are waiting for the teardown for the second time, - // most likely after the first time did timeout, annotate current fixture - // for better error messages. - this._setTeardownDescription(testInfo); + const afterStep = this._shouldGenerateStep ? testInfo?._addStep({ + wallTime: Date.now(), + title: `fixture: ${this.registration.name}`, + category: 'fixture', + location: this._isInternalFixture ? this.registration.location : undefined, + }) : undefined; + testInfo._timeoutManager.setCurrentFixture(this._teardownDescription); + try { + if (!this._teardownWithDepsComplete) + this._teardownWithDepsComplete = this._teardownInternal(); await this._teardownWithDepsComplete; + afterStep?.complete({}); + } catch (error) { + afterStep?.complete({ error }); + throw error; + } finally { testInfo._timeoutManager.setCurrentFixture(undefined); - return; } - this._teardownWithDepsComplete = this._teardownInternal(testInfo); - await this._teardownWithDepsComplete; } - private async _teardownInternal(testInfo: TestInfoImpl) { + private async _teardownInternal() { if (typeof this.registration.fn !== 'function') return; try { if (this._usages.size !== 0) { // TODO: replace with assert. - console.error('Internal error: fixture integrity at', this._runnableDescription.title); // eslint-disable-line no-console + console.error('Internal error: fixture integrity at', this._teardownDescription.title); // eslint-disable-line no-console this._usages.clear(); } if (this._useFuncFinished) { debugTest(`teardown ${this.registration.name}`); - this._setTeardownDescription(testInfo); this._useFuncFinished.resolve(); await this._selfTeardownComplete; - testInfo._timeoutManager.setCurrentFixture(undefined); } } finally { for (const dep of this._deps) @@ -173,11 +170,6 @@ class Fixture { } } - private _setTeardownDescription(testInfo: TestInfoImpl) { - this._runnableDescription.phase = 'teardown'; - testInfo._timeoutManager.setCurrentFixture(this._runnableDescription); - } - _collectFixturesInTeardownOrder(scope: FixtureScope, collector: Set) { if (this.registration.scope !== scope) return; diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index 32b27b3751..69f1912752 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -129,6 +129,7 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => { ' fixture: context', ' fixture: request', ' apiRequestContext.dispose', + ' fixture: browser', ]); }); @@ -988,3 +989,48 @@ test('should record nested steps, even after timeout', async ({ runInlineTest }, ' fixture: browser', ]); }); + +test('should attribute worker fixture teardown to the right test', async ({ runInlineTest }, testInfo) => { + const result = await runInlineTest({ + 'playwright.config.ts': ` + module.exports = { + use: { trace: { mode: 'on' } }, + }; + `, + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + foo: [async ({}, use) => { + expect(1, 'step in foo setup').toBe(1); + await use('foo'); + expect(1, 'step in foo teardown').toBe(1); + }, { scope: 'worker' }], + }); + + test('one', async ({ foo }) => { + }); + + test('two', async ({ foo }) => { + throw new Error('failure'); + }); + `, + }, { workers: 1 }); + + expect(result.exitCode).toBe(1); + expect(result.passed).toBe(1); + expect(result.failed).toBe(1); + const trace1 = await parseTrace(testInfo.outputPath('test-results', 'a-one', 'trace.zip')); + expect(trace1.actionTree).toEqual([ + 'Before Hooks', + ' fixture: foo', + ' step in foo setup', + 'After Hooks', + ]); + const trace2 = await parseTrace(testInfo.outputPath('test-results', 'a-two', 'trace.zip')); + expect(trace2.actionTree).toEqual([ + 'Before Hooks', + 'After Hooks', + ' fixture: foo', + ' step in foo teardown', + ]); +});