diff --git a/packages/playwright-core/src/utils/timeoutRunner.ts b/packages/playwright-core/src/utils/timeoutRunner.ts index fc4db8aed9..dd60551f62 100644 --- a/packages/playwright-core/src/utils/timeoutRunner.ts +++ b/packages/playwright-core/src/utils/timeoutRunner.ts @@ -45,11 +45,11 @@ export class TimeoutRunner { timeoutPromise: new ManualPromise(), }; try { + this._updateTimeout(running, this._timeout); const resultPromise = Promise.race([ cb(), running.timeoutPromise ]); - this._updateTimeout(running, this._timeout); return await resultPromise; } finally { this._updateTimeout(running, 0); diff --git a/packages/playwright/src/common/testType.ts b/packages/playwright/src/common/testType.ts index 1411b249b4..35e2a4668c 100644 --- a/packages/playwright/src/common/testType.ts +++ b/packages/playwright/src/common/testType.ts @@ -21,7 +21,7 @@ import { wrapFunctionWithLocation } from '../transform/transform'; import type { FixturesWithLocation } from './config'; import type { Fixtures, TestType, TestDetails } from '../../types/test'; import type { Location } from '../../types/testReporter'; -import { getPackageManagerExecCommand } from 'playwright-core/lib/utils'; +import { getPackageManagerExecCommand, zones } from 'playwright-core/lib/utils'; const testTypeSymbol = Symbol('testType'); @@ -263,9 +263,16 @@ export class TestTypeImpl { const testInfo = currentTestInfo(); if (!testInfo) throw new Error(`test.step() can only be called from a test`); - return testInfo._runAsStep({ category: 'test.step', title, box: options.box }, async () => { - // Make sure that internal "step" is not leaked to the user callback. - return await body(); + const step = testInfo._addStep({ wallTime: Date.now(), category: 'test.step', title, box: options.box }); + return await zones.run('stepZone', step, async () => { + try { + const result = await body(); + step.complete({}); + return result; + } catch (error) { + step.complete({ error }); + throw error; + } }); } diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 85c2dddab9..7405a83cab 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { formatLocation, debugTest, filterStackFile } from '../util'; +import { formatLocation, filterStackFile } from '../util'; import { ManualPromise } from 'playwright-core/lib/utils'; import type { TestInfoImpl } from './testInfo'; import type { FixtureDescription } from './timeoutManager'; @@ -65,22 +65,16 @@ class Fixture { return; } - testInfo._timeoutManager.setCurrentFixture(this._setupDescription); - const beforeStep = this._shouldGenerateStep ? testInfo._addStep({ + await testInfo._runAsStage({ title: `fixture: ${this.registration.name}`, - category: 'fixture', + canTimeout: true, location: this._isInternalFixture ? this.registration.location : undefined, - wallTime: Date.now(), - }) : undefined; - try { + stepCategory: this._shouldGenerateStep ? 'fixture' : undefined, + }, async () => { + testInfo._timeoutManager.setCurrentFixture(this._setupDescription); await this._setupInternal(testInfo); - beforeStep?.complete({}); - } catch (error) { - beforeStep?.complete({ error }); - throw error; - } finally { testInfo._timeoutManager.setCurrentFixture(undefined); - } + }); } private async _setupInternal(testInfo: TestInfoImpl) { @@ -107,7 +101,6 @@ class Fixture { let called = false; const useFuncStarted = new ManualPromise(); - debugTest(`setup ${this.registration.name}`); const useFunc = async (value: any) => { if (called) throw new Error(`Cannot provide fixture value for the second time`); @@ -135,24 +128,18 @@ class Fixture { } async teardown(testInfo: TestInfoImpl) { - const afterStep = this._shouldGenerateStep ? testInfo?._addStep({ - wallTime: Date.now(), + await testInfo._runAsStage({ title: `fixture: ${this.registration.name}`, - category: 'fixture', + canTimeout: true, location: this._isInternalFixture ? this.registration.location : undefined, - }) : undefined; - testInfo._timeoutManager.setCurrentFixture(this._teardownDescription); - try { + stepCategory: this._shouldGenerateStep ? 'fixture' : undefined, + }, async () => { + testInfo._timeoutManager.setCurrentFixture(this._teardownDescription); if (!this._teardownWithDepsComplete) this._teardownWithDepsComplete = this._teardownInternal(); await this._teardownWithDepsComplete; - afterStep?.complete({}); - } catch (error) { - afterStep?.complete({ error }); - throw error; - } finally { testInfo._timeoutManager.setCurrentFixture(undefined); - } + }); } private async _teardownInternal() { @@ -165,7 +152,6 @@ class Fixture { this._usages.clear(); } if (this._useFuncFinished) { - debugTest(`teardown ${this.registration.name}`); this._useFuncFinished.resolve(); await this._selfTeardownComplete; } @@ -214,14 +200,16 @@ export class FixtureRunner { collector.add(registration); } - async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl, onFixtureError: (error: Error) => void) { + async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl) { // 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); - for (const fixture of collector) - await fixture.teardown(testInfo).catch(onFixtureError); + await testInfo._runAsStage({ title: `teardown ${scope} scope` }, async () => { + for (const fixture of collector) + await fixture.teardown(testInfo); + }); if (scope === 'test') this.testScopeClean = true; } @@ -250,17 +238,16 @@ export class FixtureRunner { this._collectFixturesInSetupOrder(this.pool!.resolve(name)!, collector); // Setup fixtures. - for (const registration of collector) { - const fixture = await this._setupFixtureForRegistration(registration, testInfo); - if (fixture.failed) - return null; - } + await testInfo._runAsStage({ title: 'setup fixtures', stopOnChildError: true }, async () => { + for (const registration of collector) + await this._setupFixtureForRegistration(registration, testInfo); + }); // Create params object. const params: { [key: string]: any } = {}; for (const name of names) { const registration = this.pool!.resolve(name)!; - const fixture = this.instanceForId.get(registration.id)!; + const fixture = this.instanceForId.get(registration.id); if (!fixture || fixture.failed) return null; params[name] = fixture.value; @@ -274,7 +261,9 @@ export class FixtureRunner { // Do not run the function when fixture setup has already failed. return null; } - return fn(params, testInfo); + await testInfo._runAsStage({ title: 'run function', canTimeout: true }, async () => { + await fn(params, testInfo); + }); } private async _setupFixtureForRegistration(registration: FixtureRegistration, testInfo: TestInfoImpl): Promise { diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index e79bd61529..792cb3d096 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -21,10 +21,10 @@ import type { TestInfoError, TestInfo, TestStatus, FullProject, FullConfig } fro import type { AttachmentPayload, StepBeginPayload, StepEndPayload, WorkerInitParams } from '../common/ipc'; import type { TestCase } from '../common/test'; import { TimeoutManager } from './timeoutManager'; -import type { RunnableType, TimeSlot } from './timeoutManager'; +import type { RunnableDescription, RunnableType, TimeSlot } from './timeoutManager'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; import type { Location } from '../../types/testReporter'; -import { filteredStackTrace, getContainedPath, normalizeAndSaveAttachment, serializeError, trimLongString } from '../util'; +import { debugTest, filteredStackTrace, formatLocation, getContainedPath, normalizeAndSaveAttachment, serializeError, trimLongString } from '../util'; import { TestTracing } from './testTracing'; import type { Attachment } from './testTracing'; import type { StackFrame } from '@protocol/channels'; @@ -45,9 +45,26 @@ export interface TestStepInternal { infectParentStepsWithError?: boolean; box?: boolean; isSoft?: boolean; - forceNoParent?: boolean; + isStage?: boolean; } +export type TestStage = { + title: string; + location?: Location; + stepCategory?: 'hook' | 'fixture'; + runnableType?: RunnableType; + runnableSlot?: TimeSlot; + canTimeout?: boolean; + allowSkip?: boolean; + stopOnChildError?: boolean; + continueOnChildTimeout?: boolean; + + step?: TestStepInternal; + error?: Error; + triggeredSkip?: boolean; + triggeredTimeout?: boolean; +}; + export class TestInfoImpl implements TestInfo { private _onStepBegin: (payload: StepBeginPayload) => void; private _onStepEnd: (payload: StepEndPayload) => void; @@ -64,9 +81,9 @@ export class TestInfoImpl implements TestInfo { private readonly _requireFile: string; readonly _projectInternal: FullProjectInternal; readonly _configInternal: FullConfigInternal; - readonly _steps: TestStepInternal[] = []; + private readonly _steps: TestStepInternal[] = []; _onDidFinishTestFunction: (() => Promise) | undefined; - + private readonly _stages: TestStage[] = []; _hasNonRetriableError = false; // ------------ TestInfo fields ------------ @@ -218,36 +235,6 @@ export class TestInfoImpl implements TestInfo { } } - async _runWithTimeout(cb: () => Promise): Promise { - const timeoutError = await this._timeoutManager.runWithTimeout(cb); - // When interrupting, we arrive here with a timeoutError, but we should not - // consider it a timeout. - if (!this._wasInterrupted && timeoutError && !this._didTimeout) { - 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'; - } - this.duration = this._timeoutManager.defaultSlotTimings().elapsed | 0; - } - - async _runAndFailOnError(fn: () => Promise, skips?: 'allowSkips'): Promise { - try { - await fn(); - } catch (error) { - if (skips === 'allowSkips' && error instanceof SkipError) { - if (this.status === 'passed') - this.status = 'skipped'; - } else { - this._failWithError(error, true /* isHardError */, true /* retriable */); - return error; - } - } - } - private _findLastNonFinishedStep(filter: (step: TestStepInternal) => boolean) { let result: TestStepInternal | undefined; const visit = (step: TestStepInternal) => { @@ -259,33 +246,32 @@ export class TestInfoImpl implements TestInfo { return result; } + private _findLastStageStep() { + for (let i = this._stages.length - 1; i >= 0; i--) { + if (this._stages[i].step) + return this._stages[i].step; + } + } + _addStep(data: Omit): TestStepInternal { const stepId = `${data.category}@${++this._lastStepId}`; const rawStack = captureRawStack(); let parentStep: TestStepInternal | undefined; - if (data.category === 'hook' || data.category === 'fixture') { - // Predefined steps form a fixed hierarchy - find the last non-finished one. - parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook'); + if (data.isStage) { + // Predefined stages form a fixed hierarchy - use the current one as parent. + parentStep = this._findLastStageStep(); } else { parentStep = zones.zoneData('stepZone', rawStack!) || undefined; - if (parentStep?.category === 'hook' || parentStep?.category === 'fixture') { - // Prefer last non-finished predefined step over the on-stack one, because - // some predefined steps may be missing on the stack. - parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook'); - } else if (!parentStep) { - if (data.category === 'test.step') { - // Nest test.step without a good stack in the last non-finished predefined step like a hook. - parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook'); - } else { - // Do not nest chains of route.continue. - parentStep = this._findLastNonFinishedStep(step => step.title !== data.title); - } + if (!parentStep && data.category !== 'test.step') { + // API steps (but not test.step calls) can be nested by time, instead of by stack. + // However, do not nest chains of route.continue by checking the title. + parentStep = this._findLastNonFinishedStep(step => step.title !== data.title); + } + if (!parentStep) { + // If no parent step on stack, assume the current stage as parent. + parentStep = this._findLastStageStep(); } - } - if (data.forceNoParent) { - // This is used to reset step hierarchy after test timeout. - parentStep = undefined; } const filteredStack = filteredStackTrace(rawStack); @@ -366,6 +352,13 @@ 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; @@ -387,33 +380,91 @@ export class TestInfoImpl implements TestInfo { this._tracing.appendForError(serialized); } - async _runAsStepWithRunnable( - stepInfo: Omit & { - wallTime?: number, - runnableType: RunnableType; - runnableSlot?: TimeSlot; - }, cb: (step: TestStepInternal) => Promise): Promise { - return await this._timeoutManager.withRunnable({ - type: stepInfo.runnableType, - slot: stepInfo.runnableSlot, - location: stepInfo.location, - }, async () => { - return await this._runAsStep(stepInfo, cb); - }); - } + async _runAsStage(stage: TestStage, cb: () => Promise) { + // Inherit some properties from parent. + const parent = this._stages[this._stages.length - 1]; + stage.allowSkip = stage.allowSkip ?? parent?.allowSkip ?? false; - async _runAsStep(stepInfo: Omit & { wallTime?: number }, cb: (step: TestStepInternal) => Promise): Promise { - const step = this._addStep({ wallTime: Date.now(), ...stepInfo }); - return await zones.run('stepZone', step, async () => { + 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}`); + } + stage.step = stage.stepCategory ? this._addStep({ title: stage.title, category: stage.stepCategory, location: stage.location, wallTime: Date.now(), isStage: true }) : undefined; + this._stages.push(stage); + + let runnable: RunnableDescription | undefined; + if (stage.canTimeout) { + // Choose the deepest runnable configuration. + runnable = { type: 'test' }; + for (const s of this._stages) { + if (s.runnableType) { + runnable.type = s.runnableType; + runnable.location = s.location; + } + if (s.runnableSlot) + runnable.slot = s.runnableSlot; + } + } + + const timeoutError = await this._timeoutManager.withRunnable(runnable, async () => { try { - const result = await cb(step); - step.complete({}); - return result; + await cb(); } catch (e) { - step.complete({ error: e instanceof SkipError ? undefined : e }); - throw 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 */); + } } }); + 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'; + } + + 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() { diff --git a/packages/playwright/src/worker/timeoutManager.ts b/packages/playwright/src/worker/timeoutManager.ts index 94e182f682..a4dc7963ca 100644 --- a/packages/playwright/src/worker/timeoutManager.ts +++ b/packages/playwright/src/worker/timeoutManager.ts @@ -56,14 +56,22 @@ export class TimeoutManager { this._timeoutRunner.interrupt(); } - async withRunnable(runnable: RunnableDescription, cb: () => Promise): Promise { + async withRunnable(runnable: RunnableDescription | undefined, cb: () => Promise): Promise { + if (!runnable) { + await cb(); + return; + } const existingRunnable = this._runnable; const effectiveRunnable = { ...runnable }; if (!effectiveRunnable.slot) effectiveRunnable.slot = this._runnable.slot; this._updateRunnables(effectiveRunnable, undefined); try { - return await cb(); + await this._timeoutRunner.run(cb); + } catch (error) { + if (!(error instanceof TimeoutRunnerError)) + throw error; + return this._createTimeoutError(); } finally { this._updateRunnables(existingRunnable, undefined); } @@ -85,16 +93,6 @@ export class TimeoutManager { this._timeoutRunner.updateTimeout(slot.timeout); } - async runWithTimeout(cb: () => Promise): Promise { - try { - await this._timeoutRunner.run(cb); - } catch (error) { - if (!(error instanceof TimeoutRunnerError)) - throw error; - return this._createTimeoutError(); - } - } - setTimeout(timeout: number) { const slot = this._currentSlot(); if (!slot.timeout) diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index d678e485aa..319485cf11 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -15,7 +15,7 @@ */ import { colors } from 'playwright-core/lib/utilsBundle'; -import { debugTest, formatLocation, relativeFilePath, serializeError } from '../util'; +import { debugTest, relativeFilePath, serializeError } from '../util'; import { type TestBeginPayload, type TestEndPayload, type RunPayload, type DonePayload, type WorkerInitParams, type TeardownErrorsPayload, stdioChunkToParams } from '../common/ipc'; import { setCurrentTestInfo, setIsWorkerProcess } from '../common/globals'; import { deserializeConfig } from '../common/configLoader'; @@ -23,14 +23,13 @@ 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 } from './testInfo'; +import { TestInfoImpl, type TestStage } from './testInfo'; import { ProcessRunner } from '../common/process'; import { loadTestFile } from '../common/testLoader'; import { applyRepeatEachIndex, bindFileSuiteToProject, filterTestsRemoveEmptySuites } from '../common/suiteUtils'; 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,30 +143,12 @@ 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, e => { - testInfo._failWithError(e, true, false); - if (error === undefined) - error = e; - }); - return error; - } - private async _teardownScopes() { // TODO: separate timeout for teardown? const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {}); - await fakeTestInfo._timeoutManager.withRunnable({ type: 'teardown' }, async () => { - await fakeTestInfo._runWithTimeout(async () => { - await this._teardownScopeAndReturnFirstError('test', fakeTestInfo); - await this._teardownScopeAndReturnFirstError('worker', fakeTestInfo); - }); + await fakeTestInfo._runAsStage({ title: 'teardown scopes', runnableType: 'teardown' }, async () => { + await this._fixtureRunner.teardownScope('test', fakeTestInfo); + await this._fixtureRunner.teardownScope('worker', fakeTestInfo); }); this._fatalErrors.push(...fakeTestInfo.errors); } @@ -185,7 +166,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._failWithError(error, true /* isHardError */, true /* retriable */); + this._currentTest._unhandledError(error); // 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. @@ -323,11 +304,10 @@ export class WorkerMain extends ProcessRunner { this._lastRunningTests.push(test); if (this._lastRunningTests.length > 10) this._lastRunningTests.shift(); - let didFailBeforeAllForSuite: Suite | undefined; let shouldRunAfterEachHooks = false; - await testInfo._runWithTimeout(async () => { - const traceError = await testInfo._runAndFailOnError(async () => { + await testInfo._runAsStage({ title: 'setup and test', runnableType: 'test', allowSkip: true, stopOnChildError: 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. // However, for backwards compatibility, we have to read it from a fixture today. @@ -339,8 +319,6 @@ export class WorkerMain extends ProcessRunner { throw new Error(`"trace" option cannot be a function`); await testInfo._tracing.startIfNeeded(traceFixtureRegistration.fn); }); - if (traceError) - return; if (this._isStopped || isSkipped) { // Two reasons to get here: @@ -348,45 +326,24 @@ export class WorkerMain extends ProcessRunner { // - Worker is requested to stop, but was not able to run full cleanup yet. // We should skip the test, but run the cleanup. testInfo.status = 'skipped'; - didFailBeforeAllForSuite = undefined; return; } await removeFolders([testInfo.outputDir]); let testFunctionParams: object | null = null; - await testInfo._runAsStep({ category: 'hook', title: 'Before Hooks' }, async step => { + const beforeHooksStage: TestStage = { title: 'Before Hooks', stepCategory: 'hook', stopOnChildError: true }; + await testInfo._runAsStage(beforeHooksStage, async () => { // Run "beforeAll" hooks, unless already run during previous tests. - for (const suite of suites) { - didFailBeforeAllForSuite = suite; // Assume failure, unless reset below. - const beforeAllError = await this._runBeforeAllHooksForSuite(suite, testInfo); - if (beforeAllError) { - step.complete({ error: beforeAllError }); - return; - } - didFailBeforeAllForSuite = undefined; - if (testInfo.expectedStatus === 'skipped') - return; - } + for (const suite of suites) + await this._runBeforeAllHooksForSuite(suite, testInfo); - const beforeEachError = await testInfo._runAndFailOnError(async () => { - // Run "beforeEach" hooks. Once started with "beforeEach", we must run all "afterEach" hooks as well. - shouldRunAfterEachHooks = true; - await this._runEachHooksForSuites(suites, 'beforeEach', testInfo); - }, 'allowSkips'); - if (beforeEachError) { - step.complete({ error: beforeEachError }); - return; - } - if (testInfo.expectedStatus === 'skipped') - return; + // Run "beforeEach" hooks. Once started with "beforeEach", we must run all "afterEach" hooks as well. + shouldRunAfterEachHooks = !beforeHooksStage.error && !beforeHooksStage.triggeredSkip && !beforeHooksStage.triggeredTimeout; + await this._runEachHooksForSuites(suites, 'beforeEach', testInfo); - const fixturesError = await testInfo._runAndFailOnError(async () => { - // Setup fixtures required by the test. - testFunctionParams = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test'); - }, 'allowSkips'); - if (fixturesError) - step.complete({ error: fixturesError }); + // Setup fixtures required by the test. + testFunctionParams = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test'); }); if (testFunctionParams === null) { @@ -394,58 +351,50 @@ export class WorkerMain extends ProcessRunner { return; } - await testInfo._runAndFailOnError(async () => { + await testInfo._runAsStage({ title: 'test function', canTimeout: true }, async () => { // Now run the test itself. - debugTest(`test function started`); const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]"). await fn(testFunctionParams, testInfo); - debugTest(`test function finished`); - }, 'allowSkips'); + }); }); - if (didFailBeforeAllForSuite) { - // 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 = didFailBeforeAllForSuite; - } + // 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; - await testInfo._runAsStepWithRunnable({ category: 'hook', title: 'After Hooks', runnableType: 'afterHooks', runnableSlot: afterHooksSlot, forceNoParent: true }, async step => { - let firstAfterHooksError: Error | undefined; - await testInfo._runWithTimeout(async () => { - // Note: do not wrap all teardown steps together, because failure in any of them - // does not prevent further teardown steps from running. - + await testInfo._runAsStage({ + title: 'After Hooks', + 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 () => { // Run "immediately upon test function finish" callback. - debugTest(`on-test-function-finish callback started`); - const didFinishTestFunctionError = await testInfo._runAndFailOnError(async () => testInfo._onDidFinishTestFunction?.()); - firstAfterHooksError = firstAfterHooksError || didFinishTestFunctionError; - debugTest(`on-test-function-finish callback finished`); + await testInfo._runAsStage({ title: 'on-test-function-finish', canTimeout: true }, async () => testInfo._onDidFinishTestFunction?.()); // Run "afterEach" hooks, unless we failed at beforeAll stage. - if (shouldRunAfterEachHooks) { - const afterEachError = await testInfo._runAndFailOnError(() => this._runEachHooksForSuites(reversedSuites, 'afterEach', testInfo)); - firstAfterHooksError = firstAfterHooksError || afterEachError; - } + if (shouldRunAfterEachHooks) + await this._runEachHooksForSuites(reversedSuites, 'afterEach', testInfo); // 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 this._teardownScopeAndReturnFirstError('test', testInfo); - debugTest(`tearing down test scope finished`); - firstAfterHooksError = firstAfterHooksError || testScopeError; + await testInfo._runAsStage({ title: 'teardown test scope', runnableType: 'test' }, async () => { + await this._fixtureRunner.teardownScope('test', 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()) { - const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo); - firstAfterHooksError = firstAfterHooksError || afterAllError; + // 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); } - } + }); }); if (testInfo._isFailure()) @@ -457,39 +406,28 @@ export class WorkerMain extends ProcessRunner { this._didRunFullCleanup = true; // Give it more time for the full cleanup. - await testInfo._runWithTimeout(async () => { - debugTest(`running full cleanup after the failure`); - - const teardownSlot = { timeout: this._project.project.timeout, elapsed: 0 }; - 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 this._teardownScopeAndReturnFirstError('test', testInfo); - debugTest(`tearing down test scope finished`); - firstAfterHooksError = firstAfterHooksError || testScopeError; - - for (const suite of reversedSuites) { - const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo); - firstAfterHooksError = firstAfterHooksError || afterAllError; - } - - // Attribute to 'teardown' because worker fixtures are not perceived as a part of a test. - debugTest(`tearing down worker scope started`); - const workerScopeError = await this._teardownScopeAndReturnFirstError('worker', testInfo); - debugTest(`tearing down worker scope finished`); - firstAfterHooksError = firstAfterHooksError || workerScopeError; + const teardownSlot = { timeout: this._project.project.timeout, elapsed: 0 }; + await testInfo._runAsStage({ title: 'full cleanup', runnableType: 'teardown', runnableSlot: teardownSlot }, async () => { + // 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 this._fixtureRunner.teardownScope('test', testInfo); }); + + for (const suite of reversedSuites) + await this._runAfterAllHooksForSuite(suite, testInfo); + + // Attribute to 'teardown' because worker fixtures are not perceived as a part of a test. + await this._fixtureRunner.teardownScope('worker', testInfo); }); } - - if (firstAfterHooksError) - step.complete({ error: firstAfterHooksError }); }); - await testInfo._runAndFailOnError(async () => { + await testInfo._runAsStage({ title: 'stop tracing' }, async () => { await testInfo._tracing.stopIfNeeded(); }); + testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0; + this._currentTest = null; setCurrentTestInfo(null); this.dispatchEvent('testEnd', buildTestEndPayload(testInfo)); @@ -529,73 +467,67 @@ export class WorkerMain extends ProcessRunner { return; const extraAnnotations: Annotation[] = []; this._activeSuites.set(suite, extraAnnotations); - return await this._runAllHooksForSuite(suite, testInfo, 'beforeAll', extraAnnotations); + await this._runAllHooksForSuite(suite, testInfo, 'beforeAll', extraAnnotations); } private async _runAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl, type: 'beforeAll' | 'afterAll', extraAnnotations?: Annotation[]) { - const allowSkips = type === 'beforeAll'; - let firstError: Error | undefined; - for (const hook of this._collectHooksAndModifiers(suite, type, testInfo)) { - debugTest(`${hook.type} hook at "${formatLocation(hook.location)}" started`); - const error = await testInfo._runAndFailOnError(async () => { + // 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)) { // Separate time slot for each beforeAll/afterAll hook. const timeSlot = { timeout: this._project.project.timeout, elapsed: 0 }; - await testInfo._runAsStepWithRunnable({ - category: 'hook', + const stage: TestStage = { title: hook.title, - location: hook.location, runnableType: hook.type, runnableSlot: timeSlot, - }, async () => { + stepCategory: 'hook', + location: hook.location, + continueOnChildTimeout: true, // Make sure to teardown the scope even after hook timeout. + }; + await testInfo._runAsStage(stage, async () => { const existingAnnotations = new Set(testInfo.annotations); - 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._teardownScope('test', testInfo); + 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); } + // 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); }); - }, allowSkips ? 'allowSkips' : undefined); - firstError = firstError || error; - debugTest(`${hook.type} hook at "${formatLocation(hook.location)}" finished`); - // Skip inside a beforeAll hook/modifier prevents others from running. - if (allowSkips && testInfo.expectedStatus === 'skipped') - break; - } - return firstError; + if ((stage.error || stage.triggeredTimeout) && 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; + } + } + }); } - private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl): Promise { + private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl) { if (!this._activeSuites.has(suite)) return; this._activeSuites.delete(suite); - return await this._runAllHooksForSuite(suite, testInfo, 'afterAll'); + await this._runAllHooksForSuite(suite, testInfo, 'afterAll'); } private async _runEachHooksForSuites(suites: Suite[], type: 'beforeEach' | 'afterEach', testInfo: TestInfoImpl) { - const hooks = suites.map(suite => this._collectHooksAndModifiers(suite, type, testInfo)).flat(); - let error: Error | undefined; - for (const hook of hooks) { - try { - await testInfo._runAsStepWithRunnable({ - category: 'hook', + // 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) { + await testInfo._runAsStage({ title: hook.title, - location: hook.location, runnableType: hook.type, - }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'test')); - } catch (e) { - // Always run all the hooks, and capture the first error. - error = error || e; + location: hook.location, + stepCategory: 'hook', + }, async () => { + await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'test'); + }); } - } - if (error) - throw error; + }); } } diff --git a/tests/playwright-test/basic.spec.ts b/tests/playwright-test/basic.spec.ts index 4c4b28ceeb..6476255936 100644 --- a/tests/playwright-test/basic.spec.ts +++ b/tests/playwright-test/basic.spec.ts @@ -559,7 +559,7 @@ test('should not allow mixing test types', async ({ runInlineTest }) => { export const test2 = test.extend({ value: 42, }); - + test.describe("test1 suite", () => { test2("test 2", async () => {}); }); diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index 41ce802500..95450bf483 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -752,7 +752,7 @@ test('should not throw when screenshot on failure fails', async ({ runInlineTest expect(result.exitCode).toBe(0); expect(result.passed).toBe(1); const trace = await parseTrace(testInfo.outputPath('test-results', 'a-has-pdf-page', 'trace.zip')); - const attachedScreenshots = trace.actionTree.filter(s => s === ` attach "screenshot"`); + const attachedScreenshots = trace.actionTree.filter(s => s.trim() === `attach "screenshot"`); // One screenshot for the page, no screenshot for pdf page since it should have failed. expect(attachedScreenshots.length).toBe(1); }); diff --git a/tests/playwright-test/timeout.spec.ts b/tests/playwright-test/timeout.spec.ts index 741b10e62f..bed9429ad3 100644 --- a/tests/playwright-test/timeout.spec.ts +++ b/tests/playwright-test/timeout.spec.ts @@ -455,3 +455,27 @@ test('should respect test.describe.configure', async ({ runInlineTest }) => { expect(result.output).toContain('test1-1000'); expect(result.output).toContain('test2-2000'); }); + +test('beforeEach timeout should prevent others from running', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test, expect } from '@playwright/test'; + test.beforeEach(async () => { + console.log('\\n%%beforeEach1'); + await new Promise(f => setTimeout(f, 2500)); + }); + test.beforeEach(async () => { + console.log('\\n%%beforeEach2'); + }); + test('test', async ({}) => { + }); + test.afterEach(async () => { + console.log('\\n%%afterEach'); + await new Promise(f => setTimeout(f, 1500)); + }); + ` + }, { timeout: 2000 }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.outputLines).toEqual(['beforeEach1', 'afterEach']); +});