From 227e3da62f458974f7a549f807dd964a1a6b20f1 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 29 Oct 2021 13:36:12 -0700 Subject: [PATCH] fix(test runner): do not reuse worker that did not teardown scopes (#9872) Two bug fixes: - Do not use the worker that is being shutdown for a new job. - Report unhandled errors during "expected to fail" tests as fatal errors. --- packages/playwright-test/src/dispatcher.ts | 18 ++++++++------ packages/playwright-test/src/workerRunner.ts | 12 ++++++++- tests/page/page-request-intercept.spec.ts | 3 +-- tests/playwright-test/basic.spec.ts | 26 ++++++++++++++++++++ 4 files changed, 49 insertions(+), 10 deletions(-) diff --git a/packages/playwright-test/src/dispatcher.ts b/packages/playwright-test/src/dispatcher.ts index 3d4c6efec4..b831ca0c03 100644 --- a/packages/playwright-test/src/dispatcher.ts +++ b/packages/playwright-test/src/dispatcher.ts @@ -63,7 +63,7 @@ export class Dispatcher { const job = this._queue[0]; // 2. Find a worker with the same hash, or just some free worker. - let index = this._workerSlots.findIndex(w => !w.busy && w.worker && w.worker.hash() === job.workerHash); + let index = this._workerSlots.findIndex(w => !w.busy && w.worker && w.worker.hash() === job.workerHash && !w.worker.didSendStop()); if (index === -1) index = this._workerSlots.findIndex(w => !w.busy); // No workers available, bail out. @@ -86,8 +86,8 @@ export class Dispatcher { private async _startJobInWorker(index: number, job: TestGroup) { let worker = this._workerSlots[index].worker; - // 1. Restart the worker if it has the wrong hash. - if (worker && worker.hash() !== job.workerHash) { + // 1. Restart the worker if it has the wrong hash or is being stopped already. + if (worker && (worker.hash() !== job.workerHash || worker.didSendStop())) { await worker.stop(); worker = undefined; if (this._isStopped) // Check stopped signal after async hop. @@ -405,7 +405,7 @@ class Worker extends EventEmitter { private process: child_process.ChildProcess; private _hash: string; private workerIndex: number; - private didSendStop = false; + private _didSendStop = false; private _didFail = false; private didExit = false; @@ -427,7 +427,7 @@ class Worker extends EventEmitter { }); this.process.on('exit', () => { this.didExit = true; - this.emit('exit', this.didSendStop /* expectedly */); + this.emit('exit', this._didSendStop /* expectedly */); }); this.process.on('error', e => {}); // do not yell at a send to dead process. this.process.on('message', (message: any) => { @@ -461,6 +461,10 @@ class Worker extends EventEmitter { return this._didFail; } + didSendStop() { + return this._didSendStop; + } + hash() { return this._hash; } @@ -470,9 +474,9 @@ class Worker extends EventEmitter { this._didFail = true; if (this.didExit) return; - if (!this.didSendStop) { + if (!this._didSendStop) { this.process.send({ method: 'stop' }); - this.didSendStop = true; + this._didSendStop = true; } await new Promise(f => this.once('exit', f)); } diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index c745624a3e..518ee32997 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -90,7 +90,17 @@ export class WorkerRunner extends EventEmitter { } unhandledError(error: Error | any) { - if (this._currentTest && this._currentTest.type === 'test') { + // 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. + // + // 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. + if (this._currentTest && this._currentTest.type === 'test' && this._currentTest.testInfo.expectedStatus !== 'failed') { if (!this._currentTest.testInfo.error) { this._currentTest.testInfo.status = 'failed'; this._currentTest.testInfo.error = serializeError(error); diff --git a/tests/page/page-request-intercept.spec.ts b/tests/page/page-request-intercept.spec.ts index 8ca46dea06..770a8853c5 100644 --- a/tests/page/page-request-intercept.spec.ts +++ b/tests/page/page-request-intercept.spec.ts @@ -221,10 +221,9 @@ it('should give access to the intercepted response status text', async ({ page, // @ts-expect-error const response = await route._continueToResponse(); + await Promise.all([route.fulfill({ response }), evalPromise]); expect(response.statusText()).toBe('You are awesome'); expect(response.url()).toBe(server.PREFIX + '/title.html'); - - await Promise.all([route.fulfill({ response }), evalPromise]); }); it('should give access to the intercepted response body', async ({ page, server }) => { diff --git a/tests/playwright-test/basic.spec.ts b/tests/playwright-test/basic.spec.ts index 6d1ae12c22..f96cda2253 100644 --- a/tests/playwright-test/basic.spec.ts +++ b/tests/playwright-test/basic.spec.ts @@ -396,3 +396,29 @@ test('should report unhandled rejection during worker shutdown', async ({ runInl expect(result.output).toContain('Error: Unhandled'); expect(result.output).toContain('a.test.ts:7:33'); }); + +test('should not reuse worker after unhandled rejection in test.fail', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + const test = pwt.test.extend({ + needsCleanup: async ({}, use) => { + await use(); + await new Promise(f => setTimeout(f, 3000)); + } + }); + + test('failing', async ({ needsCleanup }) => { + test.fail(); + new Promise(() => { throw new Error('Oh my!') }); + }); + + test('passing', async () => { + }); + ` + }, { workers: 1 }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.skipped).toBe(1); + expect(result.output).toContain(`Error: Oh my!`); + expect(result.output).not.toContain(`Did not teardown test scope`); +});