Skip to content

Commit

Permalink
fix(fixtures): attribute teardown step to the right TestInfo instance
Browse files Browse the repository at this point in the history
  • Loading branch information
@dgozman
dgozman committed Feb 16, 2024
1 parent 1a21e3c commit a165cbf
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 41 deletions.
70 changes: 29 additions & 41 deletions packages/playwright/src/worker/fixtureRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ class Fixture {
value: any;
failed = false;

_useFuncFinished: ManualPromise<void> | undefined;
_selfTeardownComplete: Promise<void> | undefined;
_teardownWithDepsComplete: Promise<void> | undefined;
_runnableDescription: FixtureDescription;
private _useFuncFinished: ManualPromise<void> | undefined;
private _selfTeardownComplete: Promise<void> | undefined;
private _teardownWithDepsComplete: Promise<void> | undefined;
private _runnableDescription: FixtureDescription;
private _shouldGenerateStep = false;
private _isInternalFixture = false;
_deps = new Set<Fixture>();
_usages = new Set<Fixture>();

Expand All @@ -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) {
Expand All @@ -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<void>();
Expand All @@ -93,37 +92,26 @@ class Fixture {
this._useFuncFinished = new ManualPromise<void>();
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(),
});
}

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);
Expand All @@ -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) {
Expand All @@ -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)
Expand All @@ -173,11 +166,6 @@ class Fixture {
}
}

private _setTeardownDescription(testInfo: TestInfoImpl) {
this._runnableDescription.phase = 'teardown';
testInfo._timeoutManager.setCurrentFixture(this._runnableDescription);
}

_collectFixturesInTeardownOrder(scope: FixtureScope, collector: Set<Fixture>) {
if (this.registration.scope !== scope)
return;
Expand Down
46 changes: 46 additions & 0 deletions tests/playwright-test/playwright.trace.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => {
' fixture: context',
' fixture: request',
' apiRequestContext.dispose',
' fixture: browser',
]);
});

Expand Down Expand Up @@ -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',
]);
});

0 comments on commit a165cbf

Please sign in to comment.