diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 2aed5c08f79514..15c67d26c9f9b4 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -28,10 +28,12 @@ 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 _runnableDescription: FixtureDescription; + private _shouldGenerateStep = false; + private _isInternalFixture = false; _deps = new Set(); _usages = new Set(); @@ -49,6 +51,8 @@ class Fixture { elapsed: 0, } }; + 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,12 +78,7 @@ 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; let called = false; const useFuncStarted = new ManualPromise(); @@ -93,26 +92,17 @@ class Fixture { 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) { + if (this._shouldGenerateStep) { beforeStep = testInfo._addStep({ title: `fixture: ${this.registration.name}`, category: 'fixture', - location: isInternalFixture ? this.registration.location : undefined, + location: this._isInternalFixture ? this.registration.location : undefined, wallTime: Date.now(), }); } @@ -120,10 +110,8 @@ class Fixture { 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); @@ -137,17 +125,12 @@ class Fixture { } 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); - await this._teardownWithDepsComplete; - testInfo._timeoutManager.setCurrentFixture(undefined); - return; - } - this._teardownWithDepsComplete = this._teardownInternal(testInfo); + if (!this._teardownWithDepsComplete) + this._teardownWithDepsComplete = this._teardownInternal(testInfo); + this._runnableDescription.phase = 'teardown'; + testInfo._timeoutManager.setCurrentFixture(this._runnableDescription); await this._teardownWithDepsComplete; + testInfo._timeoutManager.setCurrentFixture(undefined); } private async _teardownInternal(testInfo: TestInfoImpl) { @@ -161,10 +144,20 @@ class Fixture { } if (this._useFuncFinished) { debugTest(`teardown ${this.registration.name}`); - this._setTeardownDescription(testInfo); - this._useFuncFinished.resolve(); - await this._selfTeardownComplete; - testInfo._timeoutManager.setCurrentFixture(undefined); + const afterStep = this._shouldGenerateStep ? testInfo?._addStep({ + wallTime: Date.now(), + title: `fixture: ${this.registration.name}`, + category: 'fixture', + location: this._isInternalFixture ? this.registration.location : undefined, + }) : undefined; + try { + this._useFuncFinished.resolve(); + await this._selfTeardownComplete; + afterStep?.complete({}); + } catch (error) { + afterStep?.complete({ error }); + throw error; + } } } finally { for (const dep of this._deps) @@ -173,11 +166,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 32b27b3751114c..69f191275299b1 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', + ]); +});