diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 8036bee289..107bdb9c1f 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -30,7 +30,6 @@ class Fixture { private _useFuncFinished: ManualPromise | undefined; private _selfTeardownComplete: Promise | undefined; - private _teardownWithDepsComplete: Promise | undefined; private _setupDescription: FixtureDescription; private _teardownDescription: FixtureDescription; private _shouldGenerateStep = false; @@ -135,9 +134,7 @@ class Fixture { stepCategory: this._shouldGenerateStep ? 'fixture' : undefined, }, async () => { testInfo._timeoutManager.setCurrentFixture(this._teardownDescription); - if (!this._teardownWithDepsComplete) - this._teardownWithDepsComplete = this._teardownInternal(); - await this._teardownWithDepsComplete; + await this._teardownInternal(); testInfo._timeoutManager.setCurrentFixture(undefined); }); } @@ -153,6 +150,7 @@ class Fixture { } if (this._useFuncFinished) { this._useFuncFinished.resolve(); + this._useFuncFinished = undefined; await this._selfTeardownComplete; } } finally { @@ -205,36 +203,32 @@ export class FixtureRunner { } async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl) { - if (scope === 'worker') { - const collector = new Set(); - for (const fixture of this.instanceForId.values()) - fixture._collectFixturesInTeardownOrder('test', collector); - // Clean up test-scoped fixtures that did not teardown because of timeout in one of them. - // This preserves fixture integrity for worker fixtures. - for (const fixture of collector) - fixture._cleanupInstance(); - this.testScopeClean = true; - } - // Teardown fixtures in the reverse order. const fixtures = Array.from(this.instanceForId.values()).reverse(); const collector = new Set(); for (const fixture of fixtures) fixture._collectFixturesInTeardownOrder(scope, collector); - let firstError: Error | undefined; - for (const fixture of collector) { - try { - await fixture.teardown(testInfo); - } catch (error) { - if (error instanceof TimeoutManagerError) - throw error; - firstError = firstError ?? error; + try { + let firstError: Error | undefined; + for (const fixture of collector) { + try { + await fixture.teardown(testInfo); + } catch (error) { + if (error instanceof TimeoutManagerError) + throw error; + firstError = firstError ?? error; + } } + if (firstError) + throw firstError; + } finally { + // To preserve fixtures integrity, forcefully cleanup fixtures that did not teardown + // due to a timeout in one of them. + for (const fixture of collector) + fixture._cleanupInstance(); + if (scope === 'test') + this.testScopeClean = true; } - if (scope === 'test') - this.testScopeClean = true; - if (firstError) - throw firstError; } 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 a2550cea75..5cd95d77e2 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -364,8 +364,9 @@ export class WorkerMain extends ProcessRunner { // Update duration, so it is available in fixture teardown and afterEach hooks. testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0; - // A timed-out test gets a full additional timeout to run after hooks. - const afterHooksSlot = testInfo._didTimeout ? { timeout: this._project.project.timeout, elapsed: 0 } : undefined; + // After hooks get an additional timeout. + const afterHooksTimeout = calculateMaxTimeout(this._project.project.timeout, testInfo.timeout); + const afterHooksSlot = { timeout: afterHooksTimeout, elapsed: 0 }; await testInfo._runAsStage({ title: 'After Hooks', stepCategory: 'hook', @@ -467,7 +468,7 @@ export class WorkerMain extends ProcessRunner { await testInfo._tracing.stopIfNeeded(); }).catch(() => {}); // Ignore top-level error. - testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0; + testInfo.duration = (testInfo._timeoutManager.defaultSlotTimings().elapsed + afterHooksSlot.elapsed) | 0; this._currentTest = null; setCurrentTestInfo(null); @@ -624,4 +625,9 @@ function formatTestTitle(test: TestCase, projectName: string) { return `${projectTitle}${location} › ${titles.join(' › ')}`; } +function calculateMaxTimeout(t1: number, t2: number) { + // Zero means "no timeout". + return (!t1 || !t2) ? 0 : Math.max(t1, t2); +} + export const create = (params: WorkerInitParams) => new WorkerMain(params); diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index 59f875e991..a58814104f 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -425,6 +425,38 @@ test('should give enough time for fixture teardown', async ({ runInlineTest }) = }); `, }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(1); + expect(result.outputLines).toEqual([ + 'teardown start', + 'teardown finished', + ]); +}); + +test('should not give enough time for second fixture teardown after timeout', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + fixture2: async ({ }, use) => { + await use(); + console.log('\\n%%teardown2 start'); + await new Promise(f => setTimeout(f, 3000)); + console.log('\\n%%teardown2 finished'); + }, + fixture: async ({ fixture2 }, use) => { + await use(); + console.log('\\n%%teardown start'); + await new Promise(f => setTimeout(f, 3000)); + console.log('\\n%%teardown finished'); + }, + }); + test('fast enough but close', async ({ fixture }) => { + test.setTimeout(3000); + await new Promise(f => setTimeout(f, 2000)); + }); + `, + }, { timeout: 2000 }); expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); expect(result.output).toContain('Test finished within timeout of 3000ms, but tearing down "fixture" ran out of time.'); diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index d6a07a47a3..a819f25be8 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -2052,9 +2052,9 @@ for (const useIntermediateMergeReport of [false, true] as const) { // Failing test first, then sorted by the run order. await expect(page.locator('.test-file-test')).toHaveText([ - /main › fails\d+m?smain.spec.ts:9/, - /main › first › passes\d+m?sfirst.ts:12/, - /main › second › passes\d+m?ssecond.ts:5/, + /main › fails\d+m?s?main.spec.ts:9/, + /main › first › passes\d+m?s?first.ts:12/, + /main › second › passes\d+m?s?second.ts:5/, ]); });