Skip to content

Commit

Permalink
fix(fixtures): tear down base fixture after error in derived (#29337)
Browse files Browse the repository at this point in the history 
Fixes #29325
  • Loading branch information
@pavelfeldman
pavelfeldman authored Feb 6, 2024
1 parent 8c007fd commit 5f5e058
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 33 deletions.
40 changes: 19 additions & 21 deletions packages/playwright/src/worker/fixtureRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,17 +163,10 @@ class Fixture {
await this._teardownWithDepsComplete;
}

private _setTeardownDescription(timeoutManager: TimeoutManager) {
this._runnableDescription.phase = 'teardown';
timeoutManager.setCurrentFixture(this._runnableDescription);
}

private async _teardownInternal(timeoutManager: TimeoutManager) {
if (typeof this.registration.fn !== 'function')
return;
try {
for (const fixture of this._usages)
await fixture.teardown(timeoutManager);
if (this._usages.size !== 0) {
// TODO: replace with assert.
console.error('Internal error: fixture integrity at', this._runnableDescription.title); // eslint-disable-line no-console
Expand All @@ -192,6 +185,19 @@ class Fixture {
this.runner.instanceForId.delete(this.registration.id);
}
}

private _setTeardownDescription(timeoutManager: TimeoutManager) {
this._runnableDescription.phase = 'teardown';
timeoutManager.setCurrentFixture(this._runnableDescription);
}

_collectFixturesInTeardownOrder(scope: FixtureScope, collector: Set<Fixture>) {
if (this.registration.scope !== scope)
return;
for (const fixture of this._usages)
fixture._collectFixturesInTeardownOrder(scope, collector);
collector.add(this);
}
}

export class FixtureRunner {
Expand All @@ -213,24 +219,16 @@ export class FixtureRunner {
this.pool = pool;
}

async teardownScope(scope: FixtureScope, timeoutManager: TimeoutManager) {
let error: Error | undefined;
async teardownScope(scope: FixtureScope, timeoutManager: TimeoutManager, onFixtureError: (error: Error) => void) {
// Teardown fixtures in the reverse order.
const fixtures = Array.from(this.instanceForId.values()).reverse();
for (const fixture of fixtures) {
if (fixture.registration.scope === scope) {
try {
await fixture.teardown(timeoutManager);
} catch (e) {
if (error === undefined)
error = e;
}
}
}
const collector = new Set<Fixture>();
for (const fixture of fixtures)
fixture._collectFixturesInTeardownOrder(scope, collector);
for (const fixture of collector)
await fixture.teardown(timeoutManager).catch(onFixtureError);
if (scope === 'test')
this.testScopeClean = true;
if (error !== undefined)
throw error;
}

async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object | null> {
Expand Down
35 changes: 23 additions & 12 deletions packages/playwright/src/worker/workerMain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { applyRepeatEachIndex, bindFileSuiteToProject, filterTestsRemoveEmptySui
import { PoolBuilder } from '../common/poolBuilder';
import type { TestInfoError } from '../../types/test';
import type { Location } from '../../types/testReporter';
import type { FixtureScope } from '../common/fixtures';
import { inheritFixutreNames } from '../common/fixtures';

export class WorkerMain extends ProcessRunner {
Expand Down Expand Up @@ -144,13 +145,29 @@ export class WorkerMain extends ProcessRunner {
}
}

private async _teardownScope(scope: FixtureScope, testInfo: TestInfoImpl) {
const error = await this._teardownScopeAndReturnFirstError(scope, testInfo);
if (error)
throw error;
}

private async _teardownScopeAndReturnFirstError(scope: FixtureScope, testInfo: TestInfoImpl): Promise<Error | undefined> {
let error: Error | undefined;
await this._fixtureRunner.teardownScope(scope, testInfo._timeoutManager, e => {
testInfo._failWithError(e, true, false);
if (error === undefined)
error = e;
});
return error;
}

private async _teardownScopes() {
// TODO: separate timeout for teardown?
const timeoutManager = new TimeoutManager(this._project.project.timeout);
await timeoutManager.withRunnable({ type: 'teardown' }, async () => {
const timeoutError = await timeoutManager.runWithTimeout(async () => {
await this._fixtureRunner.teardownScope('test', timeoutManager);
await this._fixtureRunner.teardownScope('worker', timeoutManager);
await this._fixtureRunner.teardownScope('test', timeoutManager, e => this._fatalErrors.push(serializeError(e)));
await this._fixtureRunner.teardownScope('worker', timeoutManager, e => this._fatalErrors.push(serializeError(e)));
});
if (timeoutError)
this._fatalErrors.push(timeoutError);
Expand Down Expand Up @@ -421,9 +438,7 @@ export class WorkerMain extends ProcessRunner {
// Teardown test-scoped fixtures. Attribute to 'test' so that users understand
// they should probably increase the test timeout to fix this issue.
debugTest(`tearing down test scope started`);
const testScopeError = await testInfo._runAndFailOnError(() => {
return this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
});
const testScopeError = await this._teardownScopeAndReturnFirstError('test', testInfo);
debugTest(`tearing down test scope finished`);
firstAfterHooksError = firstAfterHooksError || testScopeError;

Expand Down Expand Up @@ -454,9 +469,7 @@ export class WorkerMain extends ProcessRunner {
await testInfo._timeoutManager.withRunnable({ type: 'teardown', slot: teardownSlot }, async () => {
// Attribute to 'test' so that users understand they should probably increate the test timeout to fix this issue.
debugTest(`tearing down test scope started`);
const testScopeError = await testInfo._runAndFailOnError(() => {
return this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
});
const testScopeError = await this._teardownScopeAndReturnFirstError('test', testInfo);
debugTest(`tearing down test scope finished`);
firstAfterHooksError = firstAfterHooksError || testScopeError;

Expand All @@ -467,9 +480,7 @@ export class WorkerMain extends ProcessRunner {

// Attribute to 'teardown' because worker fixtures are not perceived as a part of a test.
debugTest(`tearing down worker scope started`);
const workerScopeError = await testInfo._runAndFailOnError(() => {
return this._fixtureRunner.teardownScope('worker', testInfo._timeoutManager);
});
const workerScopeError = await this._teardownScopeAndReturnFirstError('worker', testInfo);
debugTest(`tearing down worker scope finished`);
firstAfterHooksError = firstAfterHooksError || workerScopeError;
});
Expand Down Expand Up @@ -552,7 +563,7 @@ export class WorkerMain extends ProcessRunner {
}
// Each beforeAll/afterAll hook has its own scope for test fixtures. Attribute to the same runnable and timeSlot.
// Note: we must teardown even after hook fails, because we'll run more hooks.
await this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
await this._teardownScope('test', testInfo);
}
});
}, allowSkips ? 'allowSkips' : undefined);
Expand Down
29 changes: 29 additions & 0 deletions tests/playwright-test/fixture-errors.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -647,3 +647,32 @@ test('should provide helpful error message when digests do not match', async ({
expect(result.failed).toBe(1);
expect(result.output).toContain('Playwright detected inconsistent test.use() options.');
});

test('tear down base fixture after error in derived', async ({ runInlineTest }) => {
const result = await runInlineTest({
'a.test.ts': `
import { test as base, expect } from '@playwright/test';
const test = base.extend({
context: async ({}, use, testInfo) => {
console.log('\\n%%context setup ' + testInfo.status);
await use();
console.log('\\n%%context teardown ' + testInfo.status);
},
page: async ({ context }, use, testInfo) => {
console.log('\\n%%page setup ' + testInfo.status);
await use();
console.log('\\n%%page teardown ' + testInfo.status);
throw new Error('Error in page teardown');
},
});
test('test', async ({ page }) => {});
`
});
expect(result.exitCode).toBe(1);
expect(result.outputLines).toEqual([
'context setup passed',
'page setup passed',
'page teardown passed',
'context teardown failed',
]);
});

0 comments on commit 5f5e058

Please sign in to comment.