From 1d4bdc6898b894fe80c3c2fa2040eb49ffb48f57 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 5 Mar 2024 16:35:11 -0800 Subject: [PATCH] chore(test runner): make runAsStage throw and catch errors explicitly (#29814) --- .../playwright/src/worker/fixtureRunner.ts | 43 +++-- packages/playwright/src/worker/testInfo.ts | 108 ++++------- .../playwright/src/worker/timeoutManager.ts | 16 +- packages/playwright/src/worker/workerMain.ts | 169 ++++++++++++------ tests/playwright-test/fixture-errors.spec.ts | 29 ++- 5 files changed, 217 insertions(+), 148 deletions(-) diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 7405a83cab..8036bee289 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -17,7 +17,7 @@ import { formatLocation, filterStackFile } from '../util'; import { ManualPromise } from 'playwright-core/lib/utils'; import type { TestInfoImpl } from './testInfo'; -import type { FixtureDescription } from './timeoutManager'; +import { TimeoutManagerError, type FixtureDescription } from './timeoutManager'; import { fixtureParameterNames, type FixturePool, type FixtureRegistration, type FixtureScope } from '../common/fixtures'; import type { WorkerInfo } from '../../types/test'; import type { Location } from '../../types/testReporter'; @@ -156,12 +156,16 @@ class Fixture { await this._selfTeardownComplete; } } finally { - for (const dep of this._deps) - dep._usages.delete(this); - this.runner.instanceForId.delete(this.registration.id); + this._cleanupInstance(); } } + _cleanupInstance() { + for (const dep of this._deps) + dep._usages.delete(this); + this.runner.instanceForId.delete(this.registration.id); + } + _collectFixturesInTeardownOrder(scope: FixtureScope, collector: Set) { if (this.registration.scope !== scope) return; @@ -201,17 +205,36 @@ 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); - await testInfo._runAsStage({ title: `teardown ${scope} scope` }, async () => { - for (const fixture of 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; + } + } if (scope === 'test') this.testScopeClean = true; + if (firstError) + throw firstError; } async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise { @@ -238,10 +261,8 @@ export class FixtureRunner { this._collectFixturesInSetupOrder(this.pool!.resolve(name)!, collector); // Setup fixtures. - await testInfo._runAsStage({ title: 'setup fixtures', stopOnChildError: true }, async () => { - for (const registration of collector) - await this._setupFixtureForRegistration(registration, testInfo); - }); + for (const registration of collector) + await this._setupFixtureForRegistration(registration, testInfo); // Create params object. const params: { [key: string]: any } = {}; diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 792cb3d096..70880af1c2 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -20,7 +20,7 @@ import { MaxTime, captureRawStack, monotonicTime, zones, sanitizeForFilePath, st import type { TestInfoError, TestInfo, TestStatus, FullProject, FullConfig } from '../../types/test'; import type { AttachmentPayload, StepBeginPayload, StepEndPayload, WorkerInitParams } from '../common/ipc'; import type { TestCase } from '../common/test'; -import { TimeoutManager } from './timeoutManager'; +import { TimeoutManager, TimeoutManagerError } from './timeoutManager'; import type { RunnableDescription, RunnableType, TimeSlot } from './timeoutManager'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; import type { Location } from '../../types/testReporter'; @@ -56,13 +56,7 @@ export type TestStage = { runnableSlot?: TimeSlot; canTimeout?: boolean; allowSkip?: boolean; - stopOnChildError?: boolean; - continueOnChildTimeout?: boolean; - step?: TestStepInternal; - error?: Error; - triggeredSkip?: boolean; - triggeredTimeout?: boolean; }; export class TestInfoImpl implements TestInfo { @@ -352,13 +346,6 @@ export class TestInfoImpl implements TestInfo { this.status = 'interrupted'; } - _unhandledError(error: Error) { - this._failWithError(error, true /* isHardError */, true /* retriable */); - const stage = this._stages[this._stages.length - 1]; - if (stage) - stage.error = stage.error ?? error; - } - _failWithError(error: Error, isHardError: boolean, retriable: boolean) { if (!retriable) this._hasNonRetriableError = true; @@ -385,22 +372,6 @@ export class TestInfoImpl implements TestInfo { const parent = this._stages[this._stages.length - 1]; stage.allowSkip = stage.allowSkip ?? parent?.allowSkip ?? false; - if (parent?.allowSkip && parent?.triggeredSkip) { - // Do not run more child steps after "skip" has been triggered. - debugTest(`ignored stage "${stage.title}" after previous skip`); - return; - } - if (parent?.stopOnChildError && parent?.error) { - // Do not run more child steps after a previous one failed. - debugTest(`ignored stage "${stage.title}" after previous error`); - return; - } - if (parent?.triggeredTimeout && !parent?.continueOnChildTimeout) { - // Do not run more child steps after a previous one timed out. - debugTest(`ignored stage "${stage.title}" after previous timeout`); - return; - } - if (debugTest.enabled) { const location = stage.location ? ` at "${formatLocation(stage.location)}"` : ``; debugTest(`started stage "${stage.title}"${location}`); @@ -422,49 +393,44 @@ export class TestInfoImpl implements TestInfo { } } - const timeoutError = await this._timeoutManager.withRunnable(runnable, async () => { - try { - await cb(); - } catch (e) { - if (stage.allowSkip && (e instanceof SkipError)) { - stage.triggeredSkip = true; - if (this.status === 'passed') - this.status = 'skipped'; - } else { - // Prefer the first error. - stage.error = stage.error ?? e; - this._failWithError(e, true /* isHardError */, true /* retriable */); + try { + await this._timeoutManager.withRunnable(runnable, async () => { + // Note: separate try/catch is here to report errors after timeout. + // This way we get a nice "locator.click" error after the test times out and closes the page. + try { + await cb(); + } catch (e) { + if (stage.allowSkip && (e instanceof SkipError)) { + if (this.status === 'passed') + this.status = 'skipped'; + } else if (!(e instanceof TimeoutManagerError)) { + // Do not consider timeout errors in child stages as a regular "hard error". + this._failWithError(e, true /* isHardError */, true /* retriable */); + } + throw e; } + }); + stage.step?.complete({}); + } catch (error) { + // When interrupting, we arrive here with a TimeoutManagerError, but we should not + // consider it a timeout. + if (!this._wasInterrupted && !this._didTimeout && (error instanceof TimeoutManagerError)) { + this._didTimeout = true; + const serialized = serializeError(error); + this.errors.push(serialized); + this._tracing.appendForError(serialized); + // Do not overwrite existing failure upon hook/teardown timeout. + if (this.status === 'passed' || this.status === 'skipped') + this.status = 'timedOut'; } - }); - if (timeoutError) - stage.triggeredTimeout = true; - - // When interrupting, we arrive here with a timeoutError, but we should not - // consider it a timeout. - if (!this._wasInterrupted && !this._didTimeout && timeoutError) { - stage.error = stage.error ?? timeoutError; - this._didTimeout = true; - const serialized = serializeError(timeoutError); - this.errors.push(serialized); - this._tracing.appendForError(serialized); - // Do not overwrite existing failure upon hook/teardown timeout. - if (this.status === 'passed' || this.status === 'skipped') - this.status = 'timedOut'; + stage.step?.complete({ error }); + throw error; + } finally { + if (this._stages[this._stages.length - 1] !== stage) + throw new Error(`Internal error: inconsistent stages!`); + this._stages.pop(); + debugTest(`finished stage "${stage.title}"`); } - - if (parent) { - // Notify parent about child error, skip and timeout. - parent.error = parent.error ?? stage.error; - parent.triggeredSkip = parent.triggeredSkip || stage.triggeredSkip; - parent.triggeredTimeout = parent.triggeredTimeout || stage.triggeredTimeout; - } - - if (this._stages[this._stages.length - 1] !== stage) - throw new Error(`Internal error: inconsistent stages!`); - this._stages.pop(); - stage.step?.complete({ error: stage.error }); - debugTest(`finished stage "${stage.title}"`); } _isFailure() { @@ -557,7 +523,7 @@ export class TestInfoImpl implements TestInfo { } } -class SkipError extends Error { +export class SkipError extends Error { } const stepSymbol = Symbol('step'); diff --git a/packages/playwright/src/worker/timeoutManager.ts b/packages/playwright/src/worker/timeoutManager.ts index a4dc7963ca..d90ee06547 100644 --- a/packages/playwright/src/worker/timeoutManager.ts +++ b/packages/playwright/src/worker/timeoutManager.ts @@ -56,22 +56,20 @@ export class TimeoutManager { this._timeoutRunner.interrupt(); } - async withRunnable(runnable: RunnableDescription | undefined, cb: () => Promise): Promise { - if (!runnable) { - await cb(); - return; - } + async withRunnable(runnable: RunnableDescription | undefined, cb: () => Promise): Promise { + if (!runnable) + return await cb(); const existingRunnable = this._runnable; const effectiveRunnable = { ...runnable }; if (!effectiveRunnable.slot) effectiveRunnable.slot = this._runnable.slot; this._updateRunnables(effectiveRunnable, undefined); try { - await this._timeoutRunner.run(cb); + return await this._timeoutRunner.run(cb); } catch (error) { if (!(error instanceof TimeoutRunnerError)) throw error; - return this._createTimeoutError(); + throw this._createTimeoutError(); } finally { this._updateRunnables(existingRunnable, undefined); } @@ -171,10 +169,12 @@ export class TimeoutManager { message = `Fixture "${fixtureWithSlot.title}" timeout of ${timeout}ms exceeded during ${fixtureWithSlot.phase}.`; message = colors.red(message); const location = (fixtureWithSlot || this._runnable).location; - const error = new Error(message); + const error = new TimeoutManagerError(message); error.name = ''; // Include location for hooks, modifiers and fixtures to distinguish between them. error.stack = message + (location ? `\n at ${location.file}:${location.line}:${location.column}` : ''); return error; } } + +export class TimeoutManagerError extends Error {} diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 319485cf11..a2550cea75 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -23,7 +23,7 @@ import type { Suite, TestCase } from '../common/test'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; import { FixtureRunner } from './fixtureRunner'; import { ManualPromise, gracefullyCloseAll, removeFolders } from 'playwright-core/lib/utils'; -import { TestInfoImpl, type TestStage } from './testInfo'; +import { SkipError, TestInfoImpl } from './testInfo'; import { ProcessRunner } from '../common/process'; import { loadTestFile } from '../common/testLoader'; import { applyRepeatEachIndex, bindFileSuiteToProject, filterTestsRemoveEmptySuites } from '../common/suiteUtils'; @@ -31,6 +31,7 @@ import { PoolBuilder } from '../common/poolBuilder'; import type { TestInfoError } from '../../types/test'; import type { Location } from '../../types/testReporter'; import { inheritFixutreNames } from '../common/fixtures'; +import { TimeoutManagerError } from './timeoutManager'; export class WorkerMain extends ProcessRunner { private _params: WorkerInitParams; @@ -147,8 +148,11 @@ export class WorkerMain extends ProcessRunner { // TODO: separate timeout for teardown? const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {}); await fakeTestInfo._runAsStage({ title: 'teardown scopes', runnableType: 'teardown' }, async () => { - await this._fixtureRunner.teardownScope('test', fakeTestInfo); - await this._fixtureRunner.teardownScope('worker', fakeTestInfo); + try { + await this._fixtureRunner.teardownScope('test', fakeTestInfo); + } finally { + await this._fixtureRunner.teardownScope('worker', fakeTestInfo); + } }); this._fatalErrors.push(...fakeTestInfo.errors); } @@ -166,7 +170,7 @@ export class WorkerMain extends ProcessRunner { // and unhandled errors - both lead to the test failing. This is good for regular tests, // so that you can, e.g. expect() from inside an event handler. The test fails, // and we restart the worker. - this._currentTest._unhandledError(error); + this._currentTest._failWithError(error, true /* isHardError */, true /* retriable */); // For tests marked with test.fail(), this might be a problem when unhandled error // is not coming from the user test code (legit failure), but from fixtures or test runner. @@ -306,7 +310,7 @@ export class WorkerMain extends ProcessRunner { this._lastRunningTests.shift(); let shouldRunAfterEachHooks = false; - await testInfo._runAsStage({ title: 'setup and test', runnableType: 'test', allowSkip: true, stopOnChildError: true }, async () => { + await testInfo._runAsStage({ title: 'setup and test', runnableType: 'test', allowSkip: true }, async () => { await testInfo._runAsStage({ title: 'start tracing', canTimeout: true }, async () => { // Ideally, "trace" would be an config-level option belonging to the // test runner instead of a fixture belonging to Playwright. @@ -332,14 +336,13 @@ export class WorkerMain extends ProcessRunner { await removeFolders([testInfo.outputDir]); let testFunctionParams: object | null = null; - const beforeHooksStage: TestStage = { title: 'Before Hooks', stepCategory: 'hook', stopOnChildError: true }; - await testInfo._runAsStage(beforeHooksStage, async () => { + await testInfo._runAsStage({ title: 'Before Hooks', stepCategory: 'hook' }, async () => { // Run "beforeAll" hooks, unless already run during previous tests. for (const suite of suites) await this._runBeforeAllHooksForSuite(suite, testInfo); // Run "beforeEach" hooks. Once started with "beforeEach", we must run all "afterEach" hooks as well. - shouldRunAfterEachHooks = !beforeHooksStage.error && !beforeHooksStage.triggeredSkip && !beforeHooksStage.triggeredTimeout; + shouldRunAfterEachHooks = true; await this._runEachHooksForSuites(suites, 'beforeEach', testInfo); // Setup fixtures required by the test. @@ -356,7 +359,7 @@ export class WorkerMain extends ProcessRunner { const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]"). await fn(testFunctionParams, testInfo); }); - }); + }).catch(() => {}); // Ignore top-level error, we still have to run after hooks. // Update duration, so it is available in fixture teardown and afterEach hooks. testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0; @@ -368,34 +371,56 @@ export class WorkerMain extends ProcessRunner { stepCategory: 'hook', runnableType: 'afterHooks', runnableSlot: afterHooksSlot, - continueOnChildTimeout: true, // Make sure the full cleanup still runs after regular cleanup timeout. }, async () => { - // Wrap cleanup steps in a stage, to stop running after one of them times out. - await testInfo._runAsStage({ title: 'regular cleanup' }, async () => { + let firstAfterHooksError: Error | undefined; + let didTimeoutInRegularCleanup = false; + + try { // Run "immediately upon test function finish" callback. await testInfo._runAsStage({ title: 'on-test-function-finish', canTimeout: true }, async () => testInfo._onDidFinishTestFunction?.()); + } catch (error) { + if (error instanceof TimeoutManagerError) + didTimeoutInRegularCleanup = true; + firstAfterHooksError = firstAfterHooksError ?? error; + } + try { // Run "afterEach" hooks, unless we failed at beforeAll stage. - if (shouldRunAfterEachHooks) + if (!didTimeoutInRegularCleanup && shouldRunAfterEachHooks) await this._runEachHooksForSuites(reversedSuites, 'afterEach', testInfo); + } catch (error) { + if (error instanceof TimeoutManagerError) + didTimeoutInRegularCleanup = true; + firstAfterHooksError = firstAfterHooksError ?? error; + } - // Teardown test-scoped fixtures. Attribute to 'test' so that users understand - // they should probably increase the test timeout to fix this issue. - await testInfo._runAsStage({ title: 'teardown test scope', runnableType: 'test' }, async () => { - await this._fixtureRunner.teardownScope('test', testInfo); - }); + try { + if (!didTimeoutInRegularCleanup) { + // Teardown test-scoped fixtures. Attribute to 'test' so that users understand + // they should probably increase the test timeout to fix this issue. + await testInfo._runAsStage({ title: 'teardown test scope', runnableType: 'test' }, async () => { + await this._fixtureRunner.teardownScope('test', testInfo); + }); + } + } catch (error) { + if (error instanceof TimeoutManagerError) + didTimeoutInRegularCleanup = true; + firstAfterHooksError = firstAfterHooksError ?? error; + } - // Run "afterAll" hooks for suites that are not shared with the next test. - // In case of failure the worker will be stopped and we have to make sure that afterAll - // hooks run before worker fixtures teardown. - // Continue running "afterAll" hooks even after some of them timeout. - await testInfo._runAsStage({ title: `after hooks suites`, continueOnChildTimeout: true }, async () => { - for (const suite of reversedSuites) { - if (!nextSuites.has(suite) || testInfo._isFailure()) - await this._runAfterAllHooksForSuite(suite, testInfo); + // Run "afterAll" hooks for suites that are not shared with the next test. + // In case of failure the worker will be stopped and we have to make sure that afterAll + // hooks run before worker fixtures teardown. + for (const suite of reversedSuites) { + if (!nextSuites.has(suite) || testInfo._isFailure()) { + try { + await this._runAfterAllHooksForSuite(suite, testInfo); + } catch (error) { + // Continue running "afterAll" hooks even after some of them timeout. + firstAfterHooksError = firstAfterHooksError ?? error; } - }); - }); + } + } if (testInfo._isFailure()) this._isStopped = true; @@ -407,24 +432,40 @@ export class WorkerMain extends ProcessRunner { // Give it more time for the full cleanup. const teardownSlot = { timeout: this._project.project.timeout, elapsed: 0 }; - await testInfo._runAsStage({ title: 'full cleanup', runnableType: 'teardown', runnableSlot: teardownSlot }, async () => { + try { // Attribute to 'test' so that users understand they should probably increate the test timeout to fix this issue. - await testInfo._runAsStage({ title: 'teardown test scope', runnableType: 'test' }, async () => { + await testInfo._runAsStage({ title: 'teardown test scope', runnableType: 'test', runnableSlot: teardownSlot }, async () => { await this._fixtureRunner.teardownScope('test', testInfo); }); + } catch (error) { + firstAfterHooksError = firstAfterHooksError ?? error; + } - for (const suite of reversedSuites) + for (const suite of reversedSuites) { + try { await this._runAfterAllHooksForSuite(suite, testInfo); + } catch (error) { + firstAfterHooksError = firstAfterHooksError ?? error; + } + } + try { // Attribute to 'teardown' because worker fixtures are not perceived as a part of a test. - await this._fixtureRunner.teardownScope('worker', testInfo); - }); + await testInfo._runAsStage({ title: 'teardown worker scope', runnableType: 'teardown', runnableSlot: teardownSlot }, async () => { + await this._fixtureRunner.teardownScope('worker', testInfo); + }); + } catch (error) { + firstAfterHooksError = firstAfterHooksError ?? error; + } } - }); + + if (firstAfterHooksError) + throw firstAfterHooksError; + }).catch(() => {}); // Ignore top-level error. await testInfo._runAsStage({ title: 'stop tracing' }, async () => { await testInfo._tracing.stopIfNeeded(); - }); + }).catch(() => {}); // Ignore top-level error. testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0; @@ -472,38 +513,47 @@ export class WorkerMain extends ProcessRunner { private async _runAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl, type: 'beforeAll' | 'afterAll', extraAnnotations?: Annotation[]) { // Always run all the hooks, and capture the first error. - await testInfo._runAsStage({ title: `${type} hooks`, continueOnChildTimeout: true }, async () => { - for (const hook of this._collectHooksAndModifiers(suite, type, testInfo)) { + let firstError: Error | undefined; + for (const hook of this._collectHooksAndModifiers(suite, type, testInfo)) { + try { // Separate time slot for each beforeAll/afterAll hook. const timeSlot = { timeout: this._project.project.timeout, elapsed: 0 }; - const stage: TestStage = { + await testInfo._runAsStage({ title: hook.title, runnableType: hook.type, runnableSlot: timeSlot, stepCategory: 'hook', location: hook.location, - continueOnChildTimeout: true, // Make sure to teardown the scope even after hook timeout. - }; - await testInfo._runAsStage(stage, async () => { + }, async () => { const existingAnnotations = new Set(testInfo.annotations); - await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'); - if (extraAnnotations) { - // Inherit all annotations defined in the beforeAll/modifer to all tests in the suite. - const newAnnotations = testInfo.annotations.filter(a => !existingAnnotations.has(a)); - extraAnnotations.push(...newAnnotations); + try { + await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'); + } finally { + if (extraAnnotations) { + // Inherit all annotations defined in the beforeAll/modifer to all tests in the suite. + const newAnnotations = testInfo.annotations.filter(a => !existingAnnotations.has(a)); + extraAnnotations.push(...newAnnotations); + } + // 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); } - // 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); }); - if ((stage.error || stage.triggeredTimeout) && type === 'beforeAll' && !this._skipRemainingTestsInSuite) { + } catch (error) { + firstError = firstError ?? error; + // Skip in beforeAll/modifier prevents others from running. + if (type === 'beforeAll' && (error instanceof SkipError)) + break; + if (type === 'beforeAll' && !this._skipRemainingTestsInSuite) { // This will inform dispatcher that we should not run more tests from this group // because we had a beforeAll error. // This behavior avoids getting the same common error for each test. this._skipRemainingTestsInSuite = suite; } } - }); + } + if (firstError) + throw firstError; } private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl) { @@ -514,10 +564,11 @@ export class WorkerMain extends ProcessRunner { } private async _runEachHooksForSuites(suites: Suite[], type: 'beforeEach' | 'afterEach', testInfo: TestInfoImpl) { - // Wrap hooks in a stage, to always run all of them and capture the first error. - await testInfo._runAsStage({ title: `${type} hooks` }, async () => { - const hooks = suites.map(suite => this._collectHooksAndModifiers(suite, type, testInfo)).flat(); - for (const hook of hooks) { + // Always run all the hooks, unless one of the times out, and capture the first error. + let firstError: Error | undefined; + const hooks = suites.map(suite => this._collectHooksAndModifiers(suite, type, testInfo)).flat(); + for (const hook of hooks) { + try { await testInfo._runAsStage({ title: hook.title, runnableType: hook.type, @@ -526,8 +577,14 @@ export class WorkerMain extends ProcessRunner { }, async () => { await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'test'); }); + } catch (error) { + if (error instanceof TimeoutManagerError) + throw error; + firstError = firstError ?? error; } - }); + } + if (firstError) + throw firstError; } } diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index 90dc344c4d..59f875e991 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -495,8 +495,7 @@ test('should not report fixture teardown timeout twice', async ({ runInlineTest expect(result.failed).toBe(1); expect(result.output).toContain('Test finished within timeout of 1000ms, but tearing down "fixture" ran out of time.'); expect(result.output).not.toContain('base.extend'); // Should not point to the location. - // TODO: this should be "not.toContain" actually. - expect(result.output).toContain('Worker teardown timeout of 1000ms exceeded while tearing down "fixture".'); + expect(result.output).not.toContain('Worker teardown timeout'); }); test('should handle fixture teardown error after test timeout and continue', async ({ runInlineTest }) => { @@ -676,3 +675,29 @@ test('tear down base fixture after error in derived', async ({ runInlineTest }) 'context teardown failed', ]); }); + +test('should not continue with scope teardown after fixture teardown timeout', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + fixture: async ({ }, use) => { + await use(); + console.log('in fixture teardown'); + }, + fixture2: async ({ fixture }, use) => { + await use(); + console.log('in fixture2 teardown'); + await new Promise(() => {}); + }, + }); + test.use({ trace: 'on' }); + test('good', async ({ fixture2 }) => { + }); + `, + }, { reporter: 'list', timeout: 1000 }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.output).toContain('Test finished within timeout of 1000ms, but tearing down "fixture2" ran out of time.'); + expect(result.output).not.toContain('in fixture teardown'); +});