diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index b01414f889..fd6f1f18a7 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -158,41 +158,32 @@ export class WorkerMain extends ProcessRunner { } unhandledError(error: Error | any) { - const failWithFatalErrorAndStop = () => { + // No current test - fatal error. + if (!this._currentTest) { if (!this._fatalErrors.length) this._fatalErrors.push(serializeError(error)); void this._stop(); - }; - - // No current test - fatal error. - if (!this._currentTest) - return failWithFatalErrorAndStop(); - - // Usually, we do not differentiate between errors in the control flow - // 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. - if (this._currentTest.expectedStatus !== 'failed') { - this._currentTest._failWithError(serializeError(error), true /* isHardError */); - void this._stop(); return; } - // However, for tests marked with test.fail(), this is a problem. Unhandled error - // could come either from the user test code (legit failure), or from a fixture or - // a test runner. In the latter case, the worker state could be messed up, - // and continuing to run tests in the same worker is problematic. Therefore, - // we turn this into a fatal error and restart the worker anyway. + // We do not differentiate between errors in the control flow + // 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(serializeError(error), true /* isHardError */); + + // 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. // - // The only exception is the expect() error that we still consider ok. + // Ideally, we would mark this test as "failed unexpectedly" and show that in the report. + // However, we do not have such a special test status, so the test will be considered ok (failed as expected). + // + // To avoid messing up future tests, we forcefully stop the worker, unless it is + // an expect() error which we know does not mess things up. const isExpectError = (error instanceof Error) && !!(error as any).matcherResult; - if (isExpectError) { - // Note: do not stop the worker, because test marked with test.fail() that fails an assertion - // is perfectly fine. - this._currentTest._failWithError(serializeError(error), true /* isHardError */); - } else { - failWithFatalErrorAndStop(); - } + const shouldContinueInThisWorker = this._currentTest.expectedStatus === 'failed' && isExpectError; + if (!shouldContinueInThisWorker) + void this._stop(); } private async _loadIfNeeded() { diff --git a/tests/playwright-test/basic.spec.ts b/tests/playwright-test/basic.spec.ts index 18afbe2fd4..3b47603c25 100644 --- a/tests/playwright-test/basic.spec.ts +++ b/tests/playwright-test/basic.spec.ts @@ -447,10 +447,10 @@ test('should not reuse worker after unhandled rejection in test.fail', async ({ }); ` }, { workers: 1 }); - expect(result.exitCode).toBe(1); - expect(result.failed).toBe(1); - expect(result.interrupted).toBe(1); - expect(result.output).toContain(`Error: Oh my!`); + expect(result.exitCode).toBe(0); + expect(result.failed).toBe(0); + expect(result.passed).toBe(2); + expect(result.output).not.toContain(`Oh my!`); expect(result.output).not.toContain(`Did not teardown test scope`); }); diff --git a/tests/playwright-test/playwright.unhandled.spec.ts b/tests/playwright-test/playwright.unhandled.spec.ts index e4a02a99ac..b820f8246a 100644 --- a/tests/playwright-test/playwright.unhandled.spec.ts +++ b/tests/playwright-test/playwright.unhandled.spec.ts @@ -16,12 +16,11 @@ import { test, expect } from './playwright-test-fixtures'; -test('should lead in uncaughtException when page.route raises', async ({ runInlineTest, server }) => { +test('should produce uncaughtException when page.route raises', async ({ runInlineTest, server }) => { const result = await runInlineTest({ 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('fail', async ({ page }) => { - test.fail(); await page.route('**/empty.html', route => { throw new Error('foobar'); }); @@ -29,16 +28,15 @@ test('should lead in uncaughtException when page.route raises', async ({ runInli }); `, }, { workers: 1 }); - expect(result.interrupted).toBe(1); + expect(result.failed).toBe(1); expect(result.output).toContain('foobar'); }); -test('should lead in unhandledRejection when page.route raises', async ({ runInlineTest, server }) => { +test('should produce unhandledRejection when page.route raises', async ({ runInlineTest, server }) => { const result = await runInlineTest({ 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('fail', async ({ page }) => { - test.fail(); await page.route('**/empty.html', async route => { throw new Error('foobar'); }); @@ -46,16 +44,15 @@ test('should lead in unhandledRejection when page.route raises', async ({ runInl }); `, }, { workers: 1 }); - expect(result.interrupted).toBe(1); + expect(result.failed).toBe(1); expect(result.output).toContain('foobar'); }); -test('should lead in uncaughtException when context.route raises', async ({ runInlineTest, server }) => { +test('should produce uncaughtException when context.route raises', async ({ runInlineTest, server }) => { const result = await runInlineTest({ 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('fail', async ({ context, page }) => { - test.fail(); await context.route('**/empty.html', route => { throw new Error('foobar'); }); @@ -63,16 +60,15 @@ test('should lead in uncaughtException when context.route raises', async ({ runI }); `, }, { workers: 1 }); - expect(result.interrupted).toBe(1); + expect(result.failed).toBe(1); expect(result.output).toContain('foobar'); }); -test('should lead in unhandledRejection when context.route raises', async ({ runInlineTest, server }) => { +test('should produce unhandledRejection when context.route raises', async ({ runInlineTest, server }) => { const result = await runInlineTest({ 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('fail', async ({ context, page }) => { - test.fail(); await context.route('**/empty.html', async route => { throw new Error('foobar'); }); @@ -80,6 +76,6 @@ test('should lead in unhandledRejection when context.route raises', async ({ run }); `, }, { workers: 1 }); - expect(result.interrupted).toBe(1); + expect(result.failed).toBe(1); expect(result.output).toContain('foobar'); }); diff --git a/tests/playwright-test/runner.spec.ts b/tests/playwright-test/runner.spec.ts index e3156c7cff..4b10f678cd 100644 --- a/tests/playwright-test/runner.spec.ts +++ b/tests/playwright-test/runner.spec.ts @@ -751,3 +751,28 @@ test('slow double SIGINT should be respected in reporter.onExit', async ({ inter expect(result.output).toContain('MyReporter.onExit started'); expect(result.output).not.toContain('MyReporter.onExit finished'); }); + +test('unhandled exception in test.fail should restart worker and continue', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test, expect } from '@playwright/test'; + + test('bad', async () => { + test.fail(); + console.log('\\n%%bad running worker=' + test.info().workerIndex); + setTimeout(() => { + throw new Error('oh my!'); + }, 0); + await new Promise(f => setTimeout(f, 1000)); + }); + + test('good', () => { + console.log('\\n%%good running worker=' + test.info().workerIndex); + }); + ` + }, { retries: 1, reporter: 'list' }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(2); + expect(result.failed).toBe(0); + expect(result.outputLines).toEqual(['bad running worker=0', 'good running worker=1']); +});