diff --git a/packages/playwright/src/matchers/expect.ts b/packages/playwright/src/matchers/expect.ts index 0b6b3da8ad..55f121474f 100644 --- a/packages/playwright/src/matchers/expect.ts +++ b/packages/playwright/src/matchers/expect.ts @@ -267,7 +267,6 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { params: args[0] ? { expected: args[0] } : undefined, wallTime, infectParentStepsWithError: this._info.isSoft, - isSoft: this._info.isSoft, }; const step = testInfo._addStep(stepInfo); @@ -275,7 +274,9 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { const reportStepError = (jestError: ExpectError) => { const error = new ExpectError(jestError, customMessage, stackFrames); step.complete({ error }); - if (!this._info.isSoft) + if (this._info.isSoft) + testInfo._failWithError(error); + else throw error; }; diff --git a/packages/playwright/src/matchers/toMatchSnapshot.ts b/packages/playwright/src/matchers/toMatchSnapshot.ts index f6e5859d4f..442483a0de 100644 --- a/packages/playwright/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchSnapshot.ts @@ -232,7 +232,8 @@ class SnapshotHelper { return this.createMatcherResult(message, true); } if (this.updateSnapshots === 'missing') { - this.testInfo._failWithError(new Error(message), false /* isHardError */, false /* retriable */); + this.testInfo._hasNonRetriableError = true; + this.testInfo._failWithError(new Error(message)); return this.createMatcherResult('', true); } return this.createMatcherResult(message, false); diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 315b42665c..3e0dab8b95 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -44,7 +44,6 @@ export interface TestStepInternal { error?: TestInfoError; infectParentStepsWithError?: boolean; box?: boolean; - isSoft?: boolean; isStage?: boolean; } @@ -62,7 +61,6 @@ export class TestInfoImpl implements TestInfo { readonly _timeoutManager: TimeoutManager; readonly _startTime: number; readonly _startWallTime: number; - private _hasHardError: boolean = false; readonly _tracing: TestTracing; _wasInterrupted = false; @@ -74,6 +72,7 @@ export class TestInfoImpl implements TestInfo { _onDidFinishTestFunction: (() => Promise) | undefined; private readonly _stages: TestStage[] = []; _hasNonRetriableError = false; + _hasUnhandledError = false; _allowSkips = false; // ------------ TestInfo fields ------------ @@ -314,9 +313,6 @@ export class TestInfoImpl implements TestInfo { this._onStepEnd(payload); const errorForTrace = step.error ? { name: '', message: step.error.message || '', stack: step.error.stack } : undefined; this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments); - - if (step.isSoft && result.error) - this._failWithError(result.error, false /* isHardError */, true /* retriable */); } }; const parentStepList = parentStep ? parentStep.steps : this._steps; @@ -344,17 +340,7 @@ export class TestInfoImpl implements TestInfo { this.status = 'interrupted'; } - _failWithError(error: Error, isHardError: boolean, retriable: boolean) { - if (!retriable) - this._hasNonRetriableError = true; - // Do not overwrite any previous hard errors. - // Some (but not all) scenarios include: - // - expect() that fails after uncaught exception. - // - fail in fixture teardown after the test failure. - if (isHardError && this._hasHardError) - return; - if (isHardError) - this._hasHardError = true; + _failWithError(error: Error) { if (this.status === 'passed' || this.status === 'skipped') this.status = error instanceof TimeoutManagerError ? 'timedOut' : 'failed'; const serialized = serializeError(error); @@ -378,24 +364,31 @@ export class TestInfoImpl implements TestInfo { try { await cb(); } catch (e) { + // Only handle errors directly thrown by the user code. + if (!stage.runnable) + throw e; if (this._allowSkips && (e instanceof SkipError)) { if (this.status === 'passed') this.status = 'skipped'; - } else if (!(e instanceof TimeoutManagerError)) { - // Note: we handle timeout errors at the top level, so ignore them here. - // Unfortunately, we cannot ignore user errors here. Consider the following scenario: + } else { + // Unfortunately, we have to handle user errors and timeout errors differently. + // Consider the following scenario: // - locator.click times out // - all stages containing the test function finish with TimeoutManagerError // - test finishes, the page is closed and this triggers locator.click error // - we would like to present the locator.click error to the user // - therefore, we need a try/catch inside the "run with timeout" block and capture the error - this._failWithError(e, true /* isHardError */, true /* retriable */); + this._failWithError(e); } 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 && (error instanceof TimeoutManagerError) && stage.runnable) + this._failWithError(error); stage.step?.complete({ error }); throw error; } finally { @@ -406,13 +399,6 @@ export class TestInfoImpl implements TestInfo { } } - _handlePossibleTimeoutError(error: Error) { - // When interrupting, we arrive here with a TimeoutManagerError, but we should not - // consider it a timeout. - if (!this._wasInterrupted && (error instanceof TimeoutManagerError)) - this._failWithError(error, false /* isHardError */, true /* retriable */); - } - _isFailure() { return this.status !== 'skipped' && this.status !== this.expectedStatus; } diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 44a2c451f2..20c7feca6a 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -147,8 +147,9 @@ export class WorkerMain extends ProcessRunner { private async _teardownScopes() { const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {}); const runnable = { type: 'teardown' } as const; - await this._fixtureRunner.teardownScope('test', fakeTestInfo, runnable).catch(error => fakeTestInfo._handlePossibleTimeoutError(error)); - await this._fixtureRunner.teardownScope('worker', fakeTestInfo, runnable).catch(error => fakeTestInfo._handlePossibleTimeoutError(error)); + // Ignore top-level errors, they are already inside TestInfo.errors. + await this._fixtureRunner.teardownScope('test', fakeTestInfo, runnable).catch(() => {}); + await this._fixtureRunner.teardownScope('worker', fakeTestInfo, runnable).catch(() => {}); this._fatalErrors.push(...fakeTestInfo.errors); } @@ -165,7 +166,10 @@ 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 */); + if (!this._currentTest._hasUnhandledError) { + this._currentTest._hasUnhandledError = true; + this._currentTest._failWithError(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. @@ -355,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(error => testInfo._handlePossibleTimeoutError(error)); + }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. // Update duration, so it is available in fixture teardown and afterEach hooks. testInfo.duration = testInfo._timeoutManager.defaultSlot().elapsed | 0; @@ -416,7 +420,7 @@ export class WorkerMain extends ProcessRunner { } if (firstAfterHooksError) throw firstAfterHooksError; - }).catch(error => testInfo._handlePossibleTimeoutError(error)); + }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. if (testInfo._isFailure()) this._isStopped = true; @@ -455,13 +459,13 @@ export class WorkerMain extends ProcessRunner { if (firstWorkerCleanupError) throw firstWorkerCleanupError; - }).catch(error => testInfo._handlePossibleTimeoutError(error)); + }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. } const tracingSlot = { timeout: this._project.project.timeout, elapsed: 0 }; await testInfo._runAsStage({ title: 'stop tracing', runnable: { type: 'test', slot: tracingSlot } }, async () => { await testInfo._tracing.stopIfNeeded(); - }).catch(error => testInfo._handlePossibleTimeoutError(error)); + }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. testInfo.duration = (testInfo._timeoutManager.defaultSlot().elapsed + afterHooksSlot.elapsed) | 0; diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index a58814104f..ae323c287a 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -733,3 +733,24 @@ test('should not continue with scope teardown after fixture teardown timeout', a 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'); }); + +test('should report fixture teardown error after test error', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + foo: async ({ }, use) => { + await use(); + throw new Error('Error from the fixture foo'); + }, + }); + test('fails', async ({ foo }) => { + throw new Error('Error from the test'); + }); + `, + }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.output).toContain('Error from the fixture foo'); + expect(result.output).toContain('Error from the test'); +});