From 646af463eeb1d9c416d9421b512213a2dec954ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 7 Mar 2019 17:47:27 +0000 Subject: [PATCH 1/6] Add tests that show that retries are broken in jest-worker --- e2e/__tests__/fatalWorkerError.test.ts | 44 +++++++++++++++++++ .../__tests__/ChildProcessWorker.test.js | 23 ++++++++++ .../__tests__/NodeThreadsWorker.test.js | 23 ++++++++++ 3 files changed, 90 insertions(+) create mode 100644 e2e/__tests__/fatalWorkerError.test.ts diff --git a/e2e/__tests__/fatalWorkerError.test.ts b/e2e/__tests__/fatalWorkerError.test.ts new file mode 100644 index 000000000000..b2e6bf9ed00e --- /dev/null +++ b/e2e/__tests__/fatalWorkerError.test.ts @@ -0,0 +1,44 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import path from 'path'; +import os from 'os'; +import {cleanup, writeFiles} from '../Utils'; +import runJest from '../runJest'; + +const DIR = path.resolve(os.tmpdir(), 'fatal-worker-error'); + +beforeEach(() => cleanup(DIR)); +afterAll(() => cleanup(DIR)); + +const NUMBER_OF_TESTS_TO_FORCE_USING_WORKERS = 25; + +test('fails a test that terminates the worker with a fatal error', () => { + const testFiles = { + '__tests__/fatalWorkerError.test.js': ` + test('fatal worker error', () => { + process.exit(134); + }); + `, + }; + + for (let i = 0; i <= NUMBER_OF_TESTS_TO_FORCE_USING_WORKERS; i++) { + testFiles[`__tests__/test${i}.test.js`] = ` + test('test ${i}', () => {}); + `; + } + + writeFiles(DIR, { + ...testFiles, + 'package.json': '{}', + }); + + const {status, stderr} = runJest(DIR); + expect(status).toBe(1); + expect(stderr).toContain('FAIL __tests__/fatalWorkerError.test.js'); + expect(stderr).toContain('Call retries were exceeded'); +}); diff --git a/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js b/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js index 79944efadb65..25ea5c85fe66 100644 --- a/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js +++ b/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js @@ -151,6 +151,29 @@ it('sends the task to the child process', () => { expect(forkInterface.send.mock.calls[1][0]).toEqual(request); }); +it('resends the task to the child process after a retry', () => { + const worker = new Worker({ + forkOptions: {}, + maxRetries: 3, + workerPath: '/tmp/foo/bar/baz.js', + }); + + const request = [CHILD_MESSAGE_CALL, false, 'foo', []]; + + worker.send(request, () => {}, () => {}); + + // Skipping call "0" because it corresponds to the "initialize" one. + expect(forkInterface.send.mock.calls[1][0]).toEqual(request); + + const previousForkInterface = forkInterface; + forkInterface.emit('exit'); + + expect(forkInterface).not.toBe(previousForkInterface); + + // Skipping call "0" because it corresponds to the "initialize" one. + expect(forkInterface.send.mock.calls[1][0]).toEqual(request); +}); + it('calls the onProcessStart method synchronously if the queue is empty', () => { const worker = new Worker({ forkOptions: {}, diff --git a/packages/jest-worker/src/workers/__tests__/NodeThreadsWorker.test.js b/packages/jest-worker/src/workers/__tests__/NodeThreadsWorker.test.js index 747ba0531c88..96529d515460 100644 --- a/packages/jest-worker/src/workers/__tests__/NodeThreadsWorker.test.js +++ b/packages/jest-worker/src/workers/__tests__/NodeThreadsWorker.test.js @@ -160,6 +160,29 @@ it('sends the task to the child process', () => { expect(worker._worker.postMessage.mock.calls[1][0]).toEqual(request); }); +it('resends the task to the child process after a retry', () => { + const worker = new Worker({ + forkOptions: {}, + maxRetries: 3, + workerPath: '/tmp/foo/bar/baz.js', + }); + + const request = [CHILD_MESSAGE_CALL, false, 'foo', []]; + + worker.send(request, () => {}, () => {}); + + // Skipping call "0" because it corresponds to the "initialize" one. + expect(worker._worker.postMessage.mock.calls[1][0]).toEqual(request); + + const previousWorker = worker._worker; + worker._worker.emit('exit'); + + expect(worker._worker).not.toBe(previousWorker); + + // Skipping call "0" because it corresponds to the "initialize" one. + expect(worker._worker.postMessage.mock.calls[1][0]).toEqual(request); +}); + it('calls the onProcessStart method synchronously if the queue is empty', () => { const worker = new Worker({ forkOptions: {}, From 9bb12940d010ec239efff0f392af9bae6edf3213 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 7 Mar 2019 17:51:48 +0000 Subject: [PATCH 2/6] Re-send requests to workers after they're re-created to retry --- .../jest-worker/src/workers/ChildProcessWorker.ts | 11 +++++++++++ packages/jest-worker/src/workers/NodeThreadsWorker.ts | 10 ++++++++++ 2 files changed, 21 insertions(+) diff --git a/packages/jest-worker/src/workers/ChildProcessWorker.ts b/packages/jest-worker/src/workers/ChildProcessWorker.ts index 5a062ae2784b..876fb8091a07 100644 --- a/packages/jest-worker/src/workers/ChildProcessWorker.ts +++ b/packages/jest-worker/src/workers/ChildProcessWorker.ts @@ -43,10 +43,13 @@ export default class ChildProcessWorker implements WorkerInterface { private _child!: ChildProcess; private _options: WorkerOptions; private _onProcessEnd!: OnEnd; + private _request: ChildMessage | null; private _retries!: number; constructor(options: WorkerOptions) { this._options = options; + this._request = null; + this.initialize(); } @@ -99,6 +102,7 @@ export default class ChildProcessWorker implements WorkerInterface { switch (response[0]) { case PARENT_MESSAGE_OK: + this._request = null; this._onProcessEnd(null, response[1]); break; @@ -121,6 +125,7 @@ export default class ChildProcessWorker implements WorkerInterface { } } + this._request = null; this._onProcessEnd(error, null); break; @@ -131,6 +136,7 @@ export default class ChildProcessWorker implements WorkerInterface { error.type = response[1]; error.stack = response[3]; + this._request = null; this._onProcessEnd(error, null); break; @@ -142,6 +148,10 @@ export default class ChildProcessWorker implements WorkerInterface { onExit(exitCode: number) { if (exitCode !== 0) { this.initialize(); + + if (this._request) { + this._child.send(this._request); + } } } @@ -149,6 +159,7 @@ export default class ChildProcessWorker implements WorkerInterface { onProcessStart(this); this._onProcessEnd = onProcessEnd; + this._request = request; this._retries = 0; this._child.send(request); } diff --git a/packages/jest-worker/src/workers/NodeThreadsWorker.ts b/packages/jest-worker/src/workers/NodeThreadsWorker.ts index 6a96c6e72f10..576d561494aa 100644 --- a/packages/jest-worker/src/workers/NodeThreadsWorker.ts +++ b/packages/jest-worker/src/workers/NodeThreadsWorker.ts @@ -27,10 +27,12 @@ export default class ExperimentalWorker implements WorkerInterface { private _worker!: Worker; private _options: WorkerOptions; private _onProcessEnd!: OnEnd; + private _request: ChildMessage | null; private _retries!: number; constructor(options: WorkerOptions) { this._options = options; + this._request = null; this.initialize(); } @@ -85,6 +87,7 @@ export default class ExperimentalWorker implements WorkerInterface { switch (response[0]) { case PARENT_MESSAGE_OK: + this._request = null; this._onProcessEnd(null, response[1]); break; @@ -107,6 +110,7 @@ export default class ExperimentalWorker implements WorkerInterface { } } + this._request = null; this._onProcessEnd(error, null); break; case PARENT_MESSAGE_SETUP_ERROR: @@ -116,6 +120,7 @@ export default class ExperimentalWorker implements WorkerInterface { error.type = response[1]; error.stack = response[3]; + this._request = null; this._onProcessEnd(error, null); break; default: @@ -126,6 +131,10 @@ export default class ExperimentalWorker implements WorkerInterface { onExit(exitCode: number) { if (exitCode !== 0) { this.initialize(); + + if (this._request) { + this._worker.postMessage(this._request); + } } } @@ -133,6 +142,7 @@ export default class ExperimentalWorker implements WorkerInterface { onProcessStart(this); this._onProcessEnd = onProcessEnd; + this._request = request; this._retries = 0; this._worker.postMessage(request); From ca98560072239722561564d323788e205916b8fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 7 Mar 2019 17:56:07 +0000 Subject: [PATCH 3/6] Refactor request handling in workers --- packages/jest-worker/src/workers/ChildProcessWorker.ts | 10 ++++++---- packages/jest-worker/src/workers/NodeThreadsWorker.ts | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/jest-worker/src/workers/ChildProcessWorker.ts b/packages/jest-worker/src/workers/ChildProcessWorker.ts index 876fb8091a07..d7713121e0c5 100644 --- a/packages/jest-worker/src/workers/ChildProcessWorker.ts +++ b/packages/jest-worker/src/workers/ChildProcessWorker.ts @@ -102,7 +102,6 @@ export default class ChildProcessWorker implements WorkerInterface { switch (response[0]) { case PARENT_MESSAGE_OK: - this._request = null; this._onProcessEnd(null, response[1]); break; @@ -125,7 +124,6 @@ export default class ChildProcessWorker implements WorkerInterface { } } - this._request = null; this._onProcessEnd(error, null); break; @@ -136,7 +134,6 @@ export default class ChildProcessWorker implements WorkerInterface { error.type = response[1]; error.stack = response[3]; - this._request = null; this._onProcessEnd(error, null); break; @@ -157,7 +154,12 @@ export default class ChildProcessWorker implements WorkerInterface { send(request: ChildMessage, onProcessStart: OnStart, onProcessEnd: OnEnd) { onProcessStart(this); - this._onProcessEnd = onProcessEnd; + this._onProcessEnd = (...args) => { + // Clean the request to avoid sending past requests to workers that fail + // while waiting for a new request (timers, unhandled rejections...) + this._request = null; + return onProcessEnd(...args); + } this._request = request; this._retries = 0; diff --git a/packages/jest-worker/src/workers/NodeThreadsWorker.ts b/packages/jest-worker/src/workers/NodeThreadsWorker.ts index 576d561494aa..0ee9d6037290 100644 --- a/packages/jest-worker/src/workers/NodeThreadsWorker.ts +++ b/packages/jest-worker/src/workers/NodeThreadsWorker.ts @@ -87,7 +87,6 @@ export default class ExperimentalWorker implements WorkerInterface { switch (response[0]) { case PARENT_MESSAGE_OK: - this._request = null; this._onProcessEnd(null, response[1]); break; @@ -110,7 +109,6 @@ export default class ExperimentalWorker implements WorkerInterface { } } - this._request = null; this._onProcessEnd(error, null); break; case PARENT_MESSAGE_SETUP_ERROR: @@ -120,7 +118,6 @@ export default class ExperimentalWorker implements WorkerInterface { error.type = response[1]; error.stack = response[3]; - this._request = null; this._onProcessEnd(error, null); break; default: @@ -140,7 +137,12 @@ export default class ExperimentalWorker implements WorkerInterface { send(request: ChildMessage, onProcessStart: OnStart, onProcessEnd: OnEnd) { onProcessStart(this); - this._onProcessEnd = onProcessEnd; + this._onProcessEnd = (...args) => { + // Clean the request to avoid sending past requests to workers that fail + // while waiting for a new request (timers, unhandled rejections...) + this._request = null; + return onProcessEnd(...args); + } this._request = request; this._retries = 0; From 4c677bbd3423fb813b53e2c818e34a3b13a4293b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 7 Mar 2019 17:57:36 +0000 Subject: [PATCH 4/6] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7cd933dd99a..94a4e20cf389 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - `[jest-cli]` export functions compatible with `import {default}` ([#8080](https://github.com/facebook/jest/pull/8080)) +- `[jest-worker]`: Fix retries and error notification in workers ([#8079](https://github.com/facebook/jest/pull/8079)) ### Chore & Maintenance From f7eecec2cb2e90f3f6daba9f784a824acbd6f594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 7 Mar 2019 18:04:14 +0000 Subject: [PATCH 5/6] Fix lint issues --- packages/jest-worker/src/workers/ChildProcessWorker.ts | 2 +- packages/jest-worker/src/workers/NodeThreadsWorker.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/jest-worker/src/workers/ChildProcessWorker.ts b/packages/jest-worker/src/workers/ChildProcessWorker.ts index d7713121e0c5..d08f395bbcd5 100644 --- a/packages/jest-worker/src/workers/ChildProcessWorker.ts +++ b/packages/jest-worker/src/workers/ChildProcessWorker.ts @@ -159,7 +159,7 @@ export default class ChildProcessWorker implements WorkerInterface { // while waiting for a new request (timers, unhandled rejections...) this._request = null; return onProcessEnd(...args); - } + }; this._request = request; this._retries = 0; diff --git a/packages/jest-worker/src/workers/NodeThreadsWorker.ts b/packages/jest-worker/src/workers/NodeThreadsWorker.ts index 0ee9d6037290..473491dd9a39 100644 --- a/packages/jest-worker/src/workers/NodeThreadsWorker.ts +++ b/packages/jest-worker/src/workers/NodeThreadsWorker.ts @@ -142,7 +142,7 @@ export default class ExperimentalWorker implements WorkerInterface { // while waiting for a new request (timers, unhandled rejections...) this._request = null; return onProcessEnd(...args); - } + }; this._request = request; this._retries = 0; From 6cee8b23bf15b2b19112e8834e316e9bbf99022c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 7 Mar 2019 19:50:54 +0000 Subject: [PATCH 6/6] Improve e2e test --- e2e/__tests__/fatalWorkerError.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/e2e/__tests__/fatalWorkerError.test.ts b/e2e/__tests__/fatalWorkerError.test.ts index b2e6bf9ed00e..72a49181b428 100644 --- a/e2e/__tests__/fatalWorkerError.test.ts +++ b/e2e/__tests__/fatalWorkerError.test.ts @@ -37,8 +37,12 @@ test('fails a test that terminates the worker with a fatal error', () => { 'package.json': '{}', }); - const {status, stderr} = runJest(DIR); - expect(status).toBe(1); + const {status, stderr} = runJest(DIR, ['--maxWorkers=2']); + + const numberOfTestsPassed = (stderr.match(/\bPASS\b/g) || []).length; + + expect(status).not.toBe(0); + expect(numberOfTestsPassed).toBe(Object.keys(testFiles).length - 1); expect(stderr).toContain('FAIL __tests__/fatalWorkerError.test.js'); expect(stderr).toContain('Call retries were exceeded'); });