Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fixtures): attribute teardown step to the right TestInfo instance #29523

Merged
merged 2 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 47 additions & 55 deletions packages/playwright/src/worker/fixtureRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -28,10 +28,13 @@ 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 _setupDescription: FixtureDescription;
private _teardownDescription: FixtureDescription;
private _shouldGenerateStep = false;
private _isInternalFixture = false;
_deps = new Set<Fixture>();
_usages = new Set<Fixture>();

Expand All @@ -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,
Expand All @@ -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) {
Expand All @@ -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<void>();
debugTest(`setup ${this.registration.name}`);
Expand All @@ -89,82 +107,61 @@ class Fixture {
throw new Error(`Cannot provide fixture value for the second time`);
called = true;
this.value = value;
beforeStep?.complete({});
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) {
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);
else
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)
Expand All @@ -173,11 +170,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
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',
]);
});
Loading