-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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.
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
fix: TestScheduler now take in count the runInBand config #7518
Changes from 2 commits
d564650
68684f6
d5c2ca8
959ddbc
10123ac
39893d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
/** | ||
* 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('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]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like it would make sense to refactor all these tests to |
||
).toBeFalsy(); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -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. | ||||
thymikee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
*/ | ||||
'use strict'; | ||||
|
||||
const SLOW_TEST_TIME = 1000; | ||||
|
||||
export function computeRunInBand(tests, isWatchMode, maxWorkers, timings) { | ||||
SimenB marked this conversation as resolved.
Show resolved
Hide resolved
SimenB marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
// 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); | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||
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) | ||||
); | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to import by name, because why not:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i need this import to be able to use
spyOn
in the tests.do you have an example on how to
spyOn
if i change the import?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same module, so it shouldn't matter 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackblitz.com/edit/testing-import-vs-import?file=src%2Fapp%2Ftest%2Ftest.spec.ts
example online with jasmine since i don't have jest on stackblitz but it doesn't matter for our case :)
It shows you the problem with
spyOn
with your import proposition. You can fork and fix it if you have a way to solve itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how's that related. Check out my latest commit to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel really dumb now
it's working o O (same module)
https://stackblitz.com/edit/testing-import-vs-import-cqfvh3?file=src%2Fapp%2Ftest%2Ftest.spec.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee for some reason i assumed it was failing by old experience... my bad it's all fine
Thanks for the fix (i corrected a bad assumption in my head that was following me :P)