From 5f5e058e962d34bea83518462e6fba99c3d3408e Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 5 Feb 2024 16:47:15 -0800 Subject: [PATCH] fix(fixtures): tear down base fixture after error in derived (#29337) Fixes https://github.com/microsoft/playwright/issues/29325 --- .../playwright/src/worker/fixtureRunner.ts | 40 +++++++++---------- packages/playwright/src/worker/workerMain.ts | 35 ++++++++++------ tests/playwright-test/fixture-errors.spec.ts | 29 ++++++++++++++ 3 files changed, 71 insertions(+), 33 deletions(-) diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 520921722d4a1..7f320a2b5b417 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -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 @@ -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) { + if (this.registration.scope !== scope) + return; + for (const fixture of this._usages) + fixture._collectFixturesInTeardownOrder(scope, collector); + collector.add(this); + } } export class FixtureRunner { @@ -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(); + 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 { diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index f4bda09d23618..9d75ebe825059 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -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 { @@ -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 { + 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); @@ -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; @@ -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; @@ -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; }); @@ -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); diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index f1813ab26c8e5..90dc344c4da3a 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -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', + ]); +});