diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 4678812759..49daf6adbc 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -354,13 +354,13 @@ export class TestInfoImpl implements TestInfo { // Do not overwrite any previous hard errors. // Some (but not all) scenarios include: // - expect() that fails after uncaught exception. - // - fail after the timeout, e.g. due to fixture teardown. + // - fail in fixture teardown after the test failure. if (isHardError && this._hasHardError) return; if (isHardError) this._hasHardError = true; if (this.status === 'passed' || this.status === 'skipped') - this.status = 'failed'; + this.status = error instanceof TimeoutManagerError ? 'timedOut' : 'failed'; const serialized = serializeError(error); const step = (error as any)[stepSymbol] as TestStepInternal | undefined; if (step && step.boxedStack) @@ -397,8 +397,6 @@ export class TestInfoImpl implements TestInfo { 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) { @@ -406,7 +404,13 @@ export class TestInfoImpl implements TestInfo { 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". + // Note: we handle timeout errors at the top level, so ignore them here. + // Unfortunately, we cannot ignore user errors here. 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 */); } throw e; @@ -414,17 +418,6 @@ export class TestInfoImpl implements TestInfo { }); 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'; - } stage.step?.complete({ error }); throw error; } finally { @@ -435,6 +428,15 @@ 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 && !this._didTimeout && (error instanceof TimeoutManagerError)) { + // this._didTimeout = true; + 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 5cd95d77e2..b308fdad8f 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -359,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. + }).catch(error => testInfo._handlePossibleTimeoutError(error)); // Update duration, so it is available in fixture teardown and afterEach hooks. testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0; @@ -462,11 +462,12 @@ export class WorkerMain extends ProcessRunner { if (firstAfterHooksError) throw firstAfterHooksError; - }).catch(() => {}); // Ignore top-level error. + }).catch(error => testInfo._handlePossibleTimeoutError(error)); - await testInfo._runAsStage({ title: 'stop tracing' }, async () => { + const tracingSlot = { timeout: this._project.project.timeout, elapsed: 0 }; + await testInfo._runAsStage({ title: 'stop tracing', canTimeout: true, runnableType: 'test', runnableSlot: tracingSlot }, async () => { await testInfo._tracing.stopIfNeeded(); - }).catch(() => {}); // Ignore top-level error. + }).catch(error => testInfo._handlePossibleTimeoutError(error)); testInfo.duration = (testInfo._timeoutManager.defaultSlotTimings().elapsed + afterHooksSlot.elapsed) | 0; diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts index dcc497f6dc..4d8999ba3f 100644 --- a/tests/playwright-test/hooks.spec.ts +++ b/tests/playwright-test/hooks.spec.ts @@ -526,7 +526,7 @@ test('afterAll timeout should be reported, run other afterAll hooks, and continu test.afterAll(async () => { console.log('\\n%%afterAll2'); }); - test('does not run', () => { + test('run in a different worker', () => { console.log('\\n%%test2'); }); `,