Skip to content

detectOpenHandles imply runInBand #8283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeysal merged 4 commits into from
Apr 7, 2019
Merged

detectOpenHandles imply runInBand #8283

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1945356
pass config into shouldRunInBand
jeysal Apr 7, 2019
a281212
detectOpenHandles imply runInBand
jeysal Apr 7, 2019
2d17dd1
Update CHANGELOG.md
jeysal Apr 7, 2019
28b18fe
docs
jeysal Apr 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Fixes

- `[jest-snapshot]` Inline snapshots: do not indent empty lines ([#8277](https://github.com/facebook/jest/pull/8277))
- `[jest-core]` Make `detectOpenHandles` imply `runInBand` ([#8283](https://github.com/facebook/jest/pull/8283))

### Chore & Maintenance

Expand Down
7 changes: 1 addition & 6 deletions packages/jest-core/src/TestScheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,7 @@ export default class TestScheduler {
getEstimatedTime(timings, this._globalConfig.maxWorkers) / 1000,
);

const runInBand = shouldRunInBand(
tests,
this._globalConfig.watch || this._globalConfig.watchAll,
this._globalConfig.maxWorkers,
timings,
);
const runInBand = shouldRunInBand(tests, timings, this._globalConfig);

const onResult = async (test: Test, testResult: TestResult) => {
if (watcher.isInterrupted()) {
Expand Down
33 changes: 17 additions & 16 deletions packages/jest-core/src/__tests__/testSchedulerHelper.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,24 @@ const getTestMock = () => ({
const getTestsMock = () => [getTestMock(), getTestMock()];

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}
tests | timings | detectOpenHandles | maxWorkers | watch | expectedResult
${[getTestMock()]} | ${[500, 500]} | ${false} | ${undefined} | ${true} | ${true}
${getTestsMock()} | ${[2000, 500]} | ${false} | ${1} | ${true} | ${true}
${getTestsMock()} | ${[2000, 500]} | ${false} | ${2} | ${true} | ${false}
${[getTestMock()]} | ${[2000, 500]} | ${false} | ${undefined} | ${true} | ${false}
${getTestMock()} | ${[500, 500]} | ${false} | ${undefined} | ${true} | ${false}
${getTestsMock()} | ${[2000, 500]} | ${false} | ${1} | ${false} | ${true}
${getTestMock()} | ${[2000, 500]} | ${false} | ${2} | ${false} | ${false}
${[getTestMock()]} | ${[2000]} | ${false} | ${undefined} | ${false} | ${true}
${getTestsMock()} | ${[500, 500]} | ${false} | ${undefined} | ${false} | ${true}
${new Array(45)} | ${[500]} | ${false} | ${undefined} | ${false} | ${false}
${getTestsMock()} | ${[2000, 500]} | ${false} | ${undefined} | ${false} | ${false}
${getTestsMock()} | ${[2000, 500]} | ${true} | ${undefined} | ${false} | ${true}
`(
'shouldRunInBand() - should return $expectedResult for runInBand mode',
({tests, watch, maxWorkers, timings, expectedResult}) => {
expect(shouldRunInBand(tests, watch, maxWorkers, timings)).toBe(
expectedResult,
);
({tests, timings, detectOpenHandles, maxWorkers, watch, expectedResult}) => {
expect(
shouldRunInBand(tests, timings, {detectOpenHandles, maxWorkers, watch}),
).toBe(expectedResult);
},
);
12 changes: 9 additions & 3 deletions packages/jest-core/src/testSchedulerHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,22 @@
* LICENSE file in the root directory of this source tree.
*/

import {Config} from '@jest/types';
import {Test} from 'jest-runner';

const SLOW_TEST_TIME = 1000;

export function shouldRunInBand(
tests: Array<Test>,
isWatchMode: boolean,
maxWorkers: number,
timings: Array<number>,
{detectOpenHandles, maxWorkers, watch, watchAll}: Config.GlobalConfig,
) {
/**
// detectOpenHandles makes no sense without runInBand, because it cannot detect leaks in workers
Copy link
Copy Markdown
Contributor

@thymikee thymikee Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it cannot detect leaks in workers

Is this a limitation of async_hooks API?

Anyway, since detectOpenHandles flag is handled before initializing TestScheduler, how about moving this logic there and adding a proper error message so users are aware? We need to alarm users about that anyway.

I'm good passing whole config to shouldRunInBand 👍

Copy link
Copy Markdown
Collaborator Author

@jeysal jeysal Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a limitation of async_hooks API?

I mean, you could certainly make leak detection work in each worker, but I don't think it's worth it - a detectOpenHandles debug run doesn't need performance through parallelization.

adding a proper error message so users are aware? We need to alarm users about that anyway.

Not sure I understand correctly, do you want to error if detectOpenHandles without runInBand is used instead of just activating runInBand implicitly? I think forcing users to specify both would be quite annoying. Printing "did not exit within 1 second, please use --detectOpenHandles --runInBand" also doesn't look good :/

Copy link
Copy Markdown
Contributor

@thymikee thymikee Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the second thought, we can hide it from the user. Would be nice to add a description note that adding this flag will make tests running serially

Copy link
Copy Markdown
Collaborator Author

@jeysal jeysal Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to docs & yargs 👍

if (detectOpenHandles) {
return true;
}

/*
* 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
Expand All @@ -26,6 +31,7 @@ export function shouldRunInBand(
* force running in band.
* https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17
*/
const isWatchMode = watch || watchAll;
const areFastTests = timings.every(timing => timing < SLOW_TEST_TIME);
const oneWorkerOrLess = maxWorkers <= 1;
const oneTestOrLess = tests.length <= 1;
Expand Down
⚡