From d564650e400f61a4bc6a067a66b2d7710e390ad7 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 16 Dec 2018 17:55:26 -0500 Subject: [PATCH 1/6] refactor: review - check for runInBand via worker count in test scheduler watch mode case --- packages/jest-cli/src/TestScheduler.js | 28 ++----- .../src/__tests__/TestScheduler.test.js | 53 ++++++++++++ .../src/__tests__/testSchedulerHelper.test.js | 84 +++++++++++++++++++ packages/jest-cli/src/testSchedulerHelper.js | 33 ++++++++ 4 files changed, 177 insertions(+), 21 deletions(-) create mode 100644 packages/jest-cli/src/__tests__/testSchedulerHelper.test.js create mode 100644 packages/jest-cli/src/testSchedulerHelper.js diff --git a/packages/jest-cli/src/TestScheduler.js b/packages/jest-cli/src/TestScheduler.js index 682cde647d1e..1153d364cdc9 100644 --- a/packages/jest-cli/src/TestScheduler.js +++ b/packages/jest-cli/src/TestScheduler.js @@ -29,8 +29,7 @@ import SummaryReporter from './reporters/summary_reporter'; import TestRunner from 'jest-runner'; import TestWatcher from './TestWatcher'; import VerboseReporter from './reporters/verbose_reporter'; - -const SLOW_TEST_TIME = 1000; +import * as testSchedulerHelper from './testSchedulerHelper'; // The default jest-runner is required because it is the default test runner // and required implicitly through the `runner` ProjectConfig option. @@ -85,25 +84,12 @@ export default class TestScheduler { getEstimatedTime(timings, this._globalConfig.maxWorkers) / 1000, ); - // Run in band if we only have one test or one worker available, unless we - // are using the watch mode, in which case the TTY has to be responsive and - // we cannot schedule anything in the main thread. Same logic applies to - // watchAll. - // - // If we are confident from previous runs that the tests will finish - // quickly we also run in band to reduce the overhead of spawning workers. - const areFastTests = timings.every(timing => timing < SLOW_TEST_TIME); - - const runInBandWatch = tests.length <= 1 && areFastTests; - const runInBandNonWatch = - this._globalConfig.maxWorkers <= 1 || - tests.length <= 1 || - (tests.length <= 20 && timings.length > 0 && areFastTests); - - const runInBand = - this._globalConfig.watch || this._globalConfig.watchAll - ? runInBandWatch - : runInBandNonWatch; + const runInBand = testSchedulerHelper.computeRunInBand( + tests, + this._globalConfig.watch || this._globalConfig.watchAll, + this._globalConfig.maxWorkers, + timings, + ); const onResult = async (test: Test, testResult: TestResult) => { if (watcher.isInterrupted()) { diff --git a/packages/jest-cli/src/__tests__/TestScheduler.test.js b/packages/jest-cli/src/__tests__/TestScheduler.test.js index d73eb9684f2d..7e0147591d01 100644 --- a/packages/jest-cli/src/__tests__/TestScheduler.test.js +++ b/packages/jest-cli/src/__tests__/TestScheduler.test.js @@ -10,6 +10,7 @@ import TestScheduler from '../TestScheduler'; import SummaryReporter from '../reporters/summary_reporter'; +import * as testSchedulerHelper from '../testSchedulerHelper'; jest.mock('../reporters/default_reporter'); const mockSerialRunner = { @@ -27,8 +28,12 @@ jest.mock('jest-runner-parallel', () => jest.fn(() => mockParallelRunner), { virtual: true, }); +const spyComputeRunInBand = jest.spyOn(testSchedulerHelper, 'computeRunInBand'); + beforeEach(() => { mockSerialRunner.runTests.mockClear(); + mockParallelRunner.runTests.mockClear(); + spyComputeRunInBand.mockClear(); }); test('config for reporters supports `default`', () => { @@ -180,3 +185,51 @@ test('should not bail if less than `n` failures', async () => { }); expect(setState).not.toBeCalled(); }); + +test('should compute runInBand to run in serial', async () => { + const scheduler = new TestScheduler({}, {}); + const test = { + context: { + config: { + runner: 'jest-runner-parallel', + }, + hasteFS: { + matchFiles: jest.fn(() => []), + }, + }, + path: './test/path.js', + }; + const tests = [test, test]; + + spyComputeRunInBand.mockReturnValue(true); + + await scheduler.scheduleTests(tests, {isInterrupted: jest.fn()}); + + expect(spyComputeRunInBand).toHaveBeenCalled(); + expect(mockParallelRunner.runTests).toHaveBeenCalled(); + expect(mockParallelRunner.runTests.mock.calls[0][5].serial).toBeTruthy(); +}); + +test('should compute runInBand to not run in serial', async () => { + const scheduler = new TestScheduler({}, {}); + const test = { + context: { + config: { + runner: 'jest-runner-parallel', + }, + hasteFS: { + matchFiles: jest.fn(() => []), + }, + }, + path: './test/path.js', + }; + const tests = [test, test]; + + spyComputeRunInBand.mockReturnValue(false); + + await scheduler.scheduleTests(tests, {isInterrupted: jest.fn()}); + + expect(spyComputeRunInBand).toHaveBeenCalled(); + expect(mockParallelRunner.runTests).toHaveBeenCalled(); + expect(mockParallelRunner.runTests.mock.calls[0][5].serial).toBeFalsy(); +}); diff --git a/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js b/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js new file mode 100644 index 000000000000..4d82a28434c1 --- /dev/null +++ b/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js @@ -0,0 +1,84 @@ +/** + * Copyright (c) 2014-present, Facebook, Inc. 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. + */ +'use strict'; + +import {computeRunInBand} from '../testSchedulerHelper'; + +const getTestMock = () => ({ + context: { + config: { + runner: 'jest-runner-parallel', + }, + hasteFS: { + matchFiles: jest.fn(() => []), + }, + }, + path: './test/path.js', +}); + +const getTestsMock = () => [getTestMock(), getTestMock()]; + +describe('computeRunInBand()', () => { + describe('watch mode enabled', () => { + test('fast tests and only one test', () => { + expect( + computeRunInBand([getTestMock()], true, undefined, [500, 500]), + ).toBeTruthy(); + }); + + // This apply also when runInBand arg present + // https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17 + test('one worker only', () => { + expect( + computeRunInBand(getTestsMock(), true, 1, [2000, 500]), + ).toBeTruthy(); + }); + + test('slow tests', () => { + expect( + computeRunInBand([getTestMock()], true, undefined, [2000, 500]), + ).toBeFalsy(); + }); + + test('more than one test', () => { + expect( + computeRunInBand(getTestsMock(), true, undefined, [500, 500]), + ).toBeFalsy(); + }); + }); + + describe('watch mode disabled', () => { + test('one worker only', () => { + expect( + computeRunInBand(getTestsMock(), false, 1, [2000, 500]), + ).toBeTruthy(); + }); + + test('one test only', () => { + expect( + computeRunInBand([getTestMock()], false, undefined, [2000]), + ).toBeTruthy(); + }); + + test('fast tests and less than 20', () => { + expect( + computeRunInBand([getTestMock()], false, undefined, [2000]), + ).toBeTruthy(); + }); + + test('slow tests', () => { + expect( + computeRunInBand(getTestsMock(), false, undefined, [2000]), + ).toBeFalsy(); + }); + + test('too much tests more than 20', () => { + const tests = new Array(45); + expect(computeRunInBand(tests, false, undefined, [500])).toBeFalsy(); + }); + }); +}); diff --git a/packages/jest-cli/src/testSchedulerHelper.js b/packages/jest-cli/src/testSchedulerHelper.js new file mode 100644 index 000000000000..3b9f7d260ded --- /dev/null +++ b/packages/jest-cli/src/testSchedulerHelper.js @@ -0,0 +1,33 @@ +/** + * Copyright (c) 2014-present, Facebook, Inc. 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. + */ +'use strict'; + +const SLOW_TEST_TIME = 1000; + +export function computeRunInBand(tests, isWatchMode, maxWorkers, timings) { + // Run in band if we only have one test or one worker available, unless we + // are using the watch mode, in which case the TTY has to be responsive and + // we cannot schedule anything in the main thread. Same logic applies to + // watchAll. + // + // If we are confident from previous runs that the tests will finish + // quickly we also run in band to reduce the overhead of spawning workers. + const areFastTests = timings.every(timing => timing < SLOW_TEST_TIME); + + const oneWorkerOrLess = maxWorkers <= 1; + const oneTestOrLess = tests.length <= 1; + + if (isWatchMode) { + return oneWorkerOrLess || (oneTestOrLess && areFastTests); + } + + return ( + oneWorkerOrLess || + oneTestOrLess || + (tests.length <= 20 && timings.length > 0 && areFastTests) + ); +} From 68684f6d63ae3c181c4e2e4648c4b2588384c855 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 16 Dec 2018 18:04:23 -0500 Subject: [PATCH 2/6] test: updates tests for computeRunInBand --- .../src/__tests__/testSchedulerHelper.test.js | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js b/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js index 4d82a28434c1..1ce6b210d999 100644 --- a/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js +++ b/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js @@ -38,6 +38,12 @@ describe('computeRunInBand()', () => { ).toBeTruthy(); }); + test('more than one worker', () => { + expect( + computeRunInBand(getTestsMock(), true, 2, [2000, 500]), + ).toBeFalsy(); + }); + test('slow tests', () => { expect( computeRunInBand([getTestMock()], true, undefined, [2000, 500]), @@ -58,27 +64,33 @@ describe('computeRunInBand()', () => { ).toBeTruthy(); }); - test('one test only', () => { + test('more than one worker', () => { expect( - computeRunInBand([getTestMock()], false, undefined, [2000]), - ).toBeTruthy(); + computeRunInBand(getTestsMock(), false, 2, [2000, 500]), + ).toBeFalsy(); }); - test('fast tests and less than 20', () => { + test('one test only', () => { expect( computeRunInBand([getTestMock()], false, undefined, [2000]), ).toBeTruthy(); }); - test('slow tests', () => { + test('fast tests and less than 20', () => { expect( - computeRunInBand(getTestsMock(), false, undefined, [2000]), - ).toBeFalsy(); + computeRunInBand(getTestsMock(), false, undefined, [500, 500]), + ).toBeTruthy(); }); test('too much tests more than 20', () => { const tests = new Array(45); expect(computeRunInBand(tests, false, undefined, [500])).toBeFalsy(); }); + + test('slow tests', () => { + expect( + computeRunInBand(getTestsMock(), false, undefined, [2000, 500]), + ).toBeFalsy(); + }); }); }); From d5c2ca82b90899588230b7f916855266edf011cd Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 21 Dec 2018 08:40:21 -0500 Subject: [PATCH 3/6] refactor: code review --- packages/jest-cli/src/TestScheduler.js | 2 +- .../src/__tests__/TestScheduler.test.js | 16 ++-- .../src/__tests__/testSchedulerHelper.test.js | 95 +++++-------------- packages/jest-cli/src/testSchedulerHelper.js | 13 ++- 4 files changed, 42 insertions(+), 84 deletions(-) diff --git a/packages/jest-cli/src/TestScheduler.js b/packages/jest-cli/src/TestScheduler.js index 1153d364cdc9..f2072fcdf320 100644 --- a/packages/jest-cli/src/TestScheduler.js +++ b/packages/jest-cli/src/TestScheduler.js @@ -84,7 +84,7 @@ export default class TestScheduler { getEstimatedTime(timings, this._globalConfig.maxWorkers) / 1000, ); - const runInBand = testSchedulerHelper.computeRunInBand( + const runInBand = testSchedulerHelper.shouldRunInBand( tests, this._globalConfig.watch || this._globalConfig.watchAll, this._globalConfig.maxWorkers, diff --git a/packages/jest-cli/src/__tests__/TestScheduler.test.js b/packages/jest-cli/src/__tests__/TestScheduler.test.js index 7e0147591d01..b5a791163151 100644 --- a/packages/jest-cli/src/__tests__/TestScheduler.test.js +++ b/packages/jest-cli/src/__tests__/TestScheduler.test.js @@ -28,12 +28,12 @@ jest.mock('jest-runner-parallel', () => jest.fn(() => mockParallelRunner), { virtual: true, }); -const spyComputeRunInBand = jest.spyOn(testSchedulerHelper, 'computeRunInBand'); +const spyShouldRunInBand = jest.spyOn(testSchedulerHelper, 'shouldRunInBand'); beforeEach(() => { mockSerialRunner.runTests.mockClear(); mockParallelRunner.runTests.mockClear(); - spyComputeRunInBand.mockClear(); + spyShouldRunInBand.mockClear(); }); test('config for reporters supports `default`', () => { @@ -186,7 +186,7 @@ test('should not bail if less than `n` failures', async () => { expect(setState).not.toBeCalled(); }); -test('should compute runInBand to run in serial', async () => { +test('should set runInBand to run in serial', async () => { const scheduler = new TestScheduler({}, {}); const test = { context: { @@ -201,16 +201,16 @@ test('should compute runInBand to run in serial', async () => { }; const tests = [test, test]; - spyComputeRunInBand.mockReturnValue(true); + spyShouldRunInBand.mockReturnValue(true); await scheduler.scheduleTests(tests, {isInterrupted: jest.fn()}); - expect(spyComputeRunInBand).toHaveBeenCalled(); + expect(spyShouldRunInBand).toHaveBeenCalled(); expect(mockParallelRunner.runTests).toHaveBeenCalled(); expect(mockParallelRunner.runTests.mock.calls[0][5].serial).toBeTruthy(); }); -test('should compute runInBand to not run in serial', async () => { +test('should set runInBand to not run in serial', async () => { const scheduler = new TestScheduler({}, {}); const test = { context: { @@ -225,11 +225,11 @@ test('should compute runInBand to not run in serial', async () => { }; const tests = [test, test]; - spyComputeRunInBand.mockReturnValue(false); + spyShouldRunInBand.mockReturnValue(false); await scheduler.scheduleTests(tests, {isInterrupted: jest.fn()}); - expect(spyComputeRunInBand).toHaveBeenCalled(); + expect(spyShouldRunInBand).toHaveBeenCalled(); expect(mockParallelRunner.runTests).toHaveBeenCalled(); expect(mockParallelRunner.runTests.mock.calls[0][5].serial).toBeFalsy(); }); diff --git a/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js b/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js index 1ce6b210d999..8f72b3686afd 100644 --- a/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js +++ b/packages/jest-cli/src/__tests__/testSchedulerHelper.test.js @@ -6,7 +6,7 @@ */ 'use strict'; -import {computeRunInBand} from '../testSchedulerHelper'; +import {shouldRunInBand} from '../testSchedulerHelper'; const getTestMock = () => ({ context: { @@ -22,75 +22,24 @@ const getTestMock = () => ({ const getTestsMock = () => [getTestMock(), getTestMock()]; -describe('computeRunInBand()', () => { - describe('watch mode enabled', () => { - test('fast tests and only one test', () => { - expect( - computeRunInBand([getTestMock()], true, undefined, [500, 500]), - ).toBeTruthy(); - }); - - // This apply also when runInBand arg present - // https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17 - test('one worker only', () => { - expect( - computeRunInBand(getTestsMock(), true, 1, [2000, 500]), - ).toBeTruthy(); - }); - - test('more than one worker', () => { - expect( - computeRunInBand(getTestsMock(), true, 2, [2000, 500]), - ).toBeFalsy(); - }); - - test('slow tests', () => { - expect( - computeRunInBand([getTestMock()], true, undefined, [2000, 500]), - ).toBeFalsy(); - }); - - test('more than one test', () => { - expect( - computeRunInBand(getTestsMock(), true, undefined, [500, 500]), - ).toBeFalsy(); - }); - }); - - describe('watch mode disabled', () => { - test('one worker only', () => { - expect( - computeRunInBand(getTestsMock(), false, 1, [2000, 500]), - ).toBeTruthy(); - }); - - test('more than one worker', () => { - expect( - computeRunInBand(getTestsMock(), false, 2, [2000, 500]), - ).toBeFalsy(); - }); - - test('one test only', () => { - expect( - computeRunInBand([getTestMock()], false, undefined, [2000]), - ).toBeTruthy(); - }); - - test('fast tests and less than 20', () => { - expect( - computeRunInBand(getTestsMock(), false, undefined, [500, 500]), - ).toBeTruthy(); - }); - - test('too much tests more than 20', () => { - const tests = new Array(45); - expect(computeRunInBand(tests, false, undefined, [500])).toBeFalsy(); - }); - - test('slow tests', () => { - expect( - computeRunInBand(getTestsMock(), false, undefined, [2000, 500]), - ).toBeFalsy(); - }); - }); -}); +test.each` + tests | watch | maxWorkers | timings | expectedResult + ${[getTestMock()]} | ${true} | ${undefined} | ${[500, 500]} | ${true} + ${getTestsMock()} | ${true} | ${1} | ${[2000, 500]} | ${true} + ${getTestsMock()} | ${true} | ${2} | ${[2000, 500]} | ${false} + ${[getTestMock()]} | ${true} | ${undefined} | ${[2000, 500]} | ${false} + ${getTestMock()} | ${true} | ${undefined} | ${[500, 500]} | ${false} + ${getTestsMock()} | ${false} | ${1} | ${[2000, 500]} | ${true} + ${getTestMock()} | ${false} | ${2} | ${[2000, 500]} | ${false} + ${[getTestMock()]} | ${false} | ${undefined} | ${[2000]} | ${true} + ${getTestsMock()} | ${false} | ${undefined} | ${[500, 500]} | ${true} + ${new Array(45)} | ${false} | ${undefined} | ${[500]} | ${false} + ${getTestsMock()} | ${false} | ${undefined} | ${[2000, 500]} | ${false} +`( + 'shouldRunInBand() - should return $expectedResult for runInBand mode', + ({tests, watch, maxWorkers, timings, expectedResult}) => { + expect(shouldRunInBand(tests, watch, maxWorkers, timings)).toBe( + expectedResult, + ); + }, +); diff --git a/packages/jest-cli/src/testSchedulerHelper.js b/packages/jest-cli/src/testSchedulerHelper.js index 3b9f7d260ded..dad51249c419 100644 --- a/packages/jest-cli/src/testSchedulerHelper.js +++ b/packages/jest-cli/src/testSchedulerHelper.js @@ -3,12 +3,20 @@ * * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. + * + * @flow */ 'use strict'; +import type {Test} from 'types/TestRunner'; const SLOW_TEST_TIME = 1000; -export function computeRunInBand(tests, isWatchMode, maxWorkers, timings) { +export function shouldRunInBand( + tests: Array, + isWatchMode: boolean, + maxWorkers: number, + timings: number[], +) { // Run in band if we only have one test or one worker available, unless we // are using the watch mode, in which case the TTY has to be responsive and // we cannot schedule anything in the main thread. Same logic applies to @@ -17,7 +25,8 @@ export function computeRunInBand(tests, isWatchMode, maxWorkers, timings) { // If we are confident from previous runs that the tests will finish // quickly we also run in band to reduce the overhead of spawning workers. const areFastTests = timings.every(timing => timing < SLOW_TEST_TIME); - + // This apply also when runInBand arg present + // https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17 const oneWorkerOrLess = maxWorkers <= 1; const oneTestOrLess = tests.length <= 1; From 959ddbc8d2139a4d2b18e51932b605dbbc16e1bc Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 21 Dec 2018 08:50:52 -0500 Subject: [PATCH 4/6] doc: update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c9cddc2c0bd..e0de77bdd531 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,7 @@ - `[jest-haste-map]` Fix `require` detection with trailing commas and ignore `import typeof` modules ([#7385](https://github.com/facebook/jest/pull/7385)) - `[jest-cli]` Fix to set prettierPath via config file ([#7412](https://github.com/facebook/jest/pull/7412)) - `[jest-cli]` Support dashed args ([#7497](https://github.com/facebook/jest/pull/7497)) +- `[jest-cli]` Fix to run in band tests if watch mode enable when runInBand arg used ([#7518](https://github.com/facebook/jest/pull/7518)) ### Chore & Maintenance From 10123acf96e00abef09948bd0bf26e4897a44575 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 21 Dec 2018 11:50:21 -0500 Subject: [PATCH 5/6] fix; failure on CI due to node8 changes --- e2e/__tests__/failures.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/__tests__/failures.test.js b/e2e/__tests__/failures.test.js index 8a88033c580c..5b79e1ed3cd8 100644 --- a/e2e/__tests__/failures.test.js +++ b/e2e/__tests__/failures.test.js @@ -43,7 +43,7 @@ test('works with node assert', () => { // Node 9 started to include the error for `doesNotThrow` // https://github.com/nodejs/node/pull/12167 - if (nodeMajorVersion >= 8) { + if (nodeMajorVersion >= 9) { expect(summary).toContain(` assert.doesNotThrow(function) @@ -71,7 +71,7 @@ test('works with node assert', () => { Got unwanted exception. `; - if (nodeMajorVersion === 8 || nodeMajorVersion === 9) { + if (nodeMajorVersion === 9) { const specificErrorMessage = `Message: Got unwanted exception. err! From 39893d9b70891198595d2c0744680e24cc501482 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 21 Dec 2018 18:14:40 +0100 Subject: [PATCH 6/6] use named exports instead of asterisk --- packages/jest-cli/src/TestScheduler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/jest-cli/src/TestScheduler.js b/packages/jest-cli/src/TestScheduler.js index f2072fcdf320..7cb71decadce 100644 --- a/packages/jest-cli/src/TestScheduler.js +++ b/packages/jest-cli/src/TestScheduler.js @@ -29,7 +29,7 @@ import SummaryReporter from './reporters/summary_reporter'; import TestRunner from 'jest-runner'; import TestWatcher from './TestWatcher'; import VerboseReporter from './reporters/verbose_reporter'; -import * as testSchedulerHelper from './testSchedulerHelper'; +import {shouldRunInBand} from './testSchedulerHelper'; // The default jest-runner is required because it is the default test runner // and required implicitly through the `runner` ProjectConfig option. @@ -84,7 +84,7 @@ export default class TestScheduler { getEstimatedTime(timings, this._globalConfig.maxWorkers) / 1000, ); - const runInBand = testSchedulerHelper.shouldRunInBand( + const runInBand = shouldRunInBand( tests, this._globalConfig.watch || this._globalConfig.watchAll, this._globalConfig.maxWorkers,