fix: TestScheduler now take in count the runInBand config #7518

nasreddineskandrani · 2018-12-15T22:36:32Z

Summary

In our team we add test file per file. So we run mostly by command line, webstorm extension, vscode extension against one file at a time.

In command line by using:

npm run jest -- --runTestsByPath path\to\a\spec\file.spec.ts --runInBand --watch

The problem is in the jest 23 we realized a regression comprared to 22.
I documented it here:
#7110 (comment)

the solution
In test scheduler runInBand args value was not considered. An algorithm was computing a runInBand variable.
As a user of jest, i want the test scheduler to use the arg runInBand value if i supply it.
So we don't want to rely on any algorithm to alter the value when we know already that it is ALWAYS better with a specific value coming from args (in this case true).

Test plan

Before my fix:
i added a console log for the runInBand variable inside TestScheduler file.

sometimes as you can see it was using false

After my fix:
It will be always using the command line arg if present which give us optimal time run (always) with the command line mentionned on top (since it remains always true)

facebook-github-bot · 2018-12-15T22:36:44Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-12-15T22:59:50Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

thymikee

Um, how did we miss that? Please add a regression test so we don't break it again :)

thymikee · 2018-12-15T23:00:27Z

packages/jest-config/src/normalize.js

@@ -682,6 +682,7 @@ export default function normalize(options: InitialOptions, argv: Argv) {
      case 'watch':
      case 'watchAll':
      case 'watchman':
+      case 'i':


I think this should state runInBand, i is an alias

thanks
as you can see when i did my code cleanup => i didn't retest it
(working with a baby in one hand and the other one in the keyboard ^^)
I was also missing another file change for my fix (added)

I'll add tests 🙈 => wanted to have a first feedback.

codecov-io · 2018-12-16T01:30:43Z

Codecov Report

Merging #7518 into master will decrease coverage by 0.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7518      +/-   ##
==========================================
- Coverage   67.73%   67.48%   -0.25%     
==========================================
  Files         247      248       +1     
  Lines        9513     9517       +4     
  Branches        5        5              
==========================================
- Hits         6444     6423      -21     
- Misses       3067     3092      +25     
  Partials        2        2

Impacted Files	Coverage Δ
packages/jest-cli/src/testSchedulerHelper.js	`100% <100%> (ø)`
packages/jest-cli/src/TestScheduler.js	`72.5% <100%> (-0.89%)`	⬇️
packages/babel-jest/src/index.js	`40.74% <0%> (-34.26%)`	⬇️
packages/jest-runner/src/index.js	`75% <0%> (-1.2%)`	⬇️
packages/jest-runtime/src/index.js	`76.99% <0%> (-0.42%)`	⬇️
packages/jest-resolve-dependencies/src/index.js	`97.43% <0%> (-0.3%)`	⬇️
...ages/jest-worker/src/workers/ChildProcessWorker.js	`88.09% <0%> (-0.28%)`	⬇️
packages/jest-runtime/src/script_transformer.js	`88.46% <0%> (-0.08%)`	⬇️
packages/expect/src/matchers.js	`99.38% <0%> (-0.01%)`	⬇️
packages/jest-util/src/installCommonGlobals.js	`100% <0%> (ø)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9208ac6...10123ac. Read the comment docs.

nasreddineskandrani · 2018-12-16T13:09:36Z

@thymikee tests done

thymikee · 2018-12-16T20:49:04Z

packages/jest-cli/src/TestScheduler.js

-        ? runInBandWatch
-        : runInBandNonWatch;
+    // if runInBand set by config we do not try to compute the value
+    let runInBand = this._globalConfig.runInBand;


Oh, now I remember. We do this on purpose, because it's easy to leave runInBand in the config and make it look like Jest is slow all the time. See: #3215

So, it's only supported through CLI args on purpose:
https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17

The correct fix would be to add this._globalConfig.maxWorkers <= 1 condition to runInBandWatch, can you make that work?

ok i see! ~~this will fix also the regression with watchAll~~ Sure i'll do the change and adjust tests

…uler watch mode case

nasreddineskandrani · 2018-12-16T23:33:57Z

@thymikee ok! so for sure i tested and now runInBand supplied assure that a run on one file with watch is fast as in v22 which is solving the problem i flagged. I added a test to catch the remove of the condition again in futur :P So i think we are ready to merge :) your call. (thanks for the help)

For the case of the other guy regression with watchAll on windows it'll not solve the problem since he doesn't use explicitly runInBand.
i have some ideas on what can cause it... to don't elaborate here
=> i'll take the other problem in a second PR during the week if no one solves it.

thymikee

LGTM :) @SimenB mind taking a look?

thymikee · 2018-12-17T09:37:01Z

packages/jest-cli/src/testSchedulerHelper.js

+  // 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);
+


Suggested change

done ~~( I'll commit all review in one - be patient)~~

thymikee · 2018-12-17T09:42:00Z

packages/jest-cli/src/__tests__/testSchedulerHelper.test.js

+
+    test('slow tests', () => {
+      expect(
+        computeRunInBand(getTestsMock(), false, undefined, [2000, 500]),


looks like it would make sense to refactor all these tests to test.each, what do you think? :)

thymikee · 2018-12-17T09:45:58Z

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';


I'd prefer to import by name, because why not:

Suggested change

import * as testSchedulerHelper from './testSchedulerHelper';

import {computeRunInBand} from './testSchedulerHelper';

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?

It's the same module, so it shouldn't matter 🤔

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 it

I don't see how's that related. Check out my latest commit to this PR

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

@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)

SimenB

Missing changelog entry 🙂

packages/jest-cli/src/testSchedulerHelper.js

nasreddineskandrani · 2018-12-21T13:58:03Z

@thymikee @SimenB review pass done. One comment (for the import) left with an answer.

thymikee · 2018-12-21T14:58:59Z

Looks like Node 8 changed the error messages again :|. Can you adjust that? https://github.com/facebook/jest/blob/master/e2e/__tests__/failures.test.js

SimenB · 2018-12-21T15:41:04Z

Yeah, they reverted the previous change. Just find my pr to jest and revert it (posting from mobile, searching is a pita)

nasreddineskandrani · 2018-12-21T17:07:54Z

@thymikee @SimenB done

nasreddineskandrani · 2018-12-21T18:06:03Z

thanks for the review :) i learnt things @SimenB @thymikee

looking forward for this merge to move on to the watchAll problem flagged for windows. since no one started it i guess I'll do it. I am on vacations for 2w tomorow so i have time to deal with it :)

* refactor: review - check for runInBand via worker count in test scheduler watch mode case * test: updates tests for computeRunInBand * refactor: code review * doc: update CHANGELOG * fix; failure on CI due to node8 changes * use named exports instead of asterisk

github-actions · 2021-05-12T04:09:22Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the cla signed label Dec 15, 2018

thymikee reviewed Dec 15, 2018

View reviewed changes

This was referenced Dec 16, 2018

Investigate speed regressions #7110

Closed

Re-run test in v23 is slower than v22 on Windows #6783

Closed

thymikee reviewed Dec 16, 2018

View reviewed changes

nasreddineskandrani added 2 commits December 16, 2018 17:55

refactor: review - check for runInBand via worker count in test sched…

d564650

…uler watch mode case

test: updates tests for computeRunInBand

68684f6

thymikee approved these changes Dec 17, 2018

View reviewed changes

SimenB requested changes Dec 17, 2018

View reviewed changes

packages/jest-cli/src/testSchedulerHelper.js Outdated Show resolved Hide resolved

packages/jest-cli/src/testSchedulerHelper.js Outdated Show resolved Hide resolved

packages/jest-cli/src/testSchedulerHelper.js Show resolved Hide resolved

nasreddineskandrani added 2 commits December 21, 2018 08:40

refactor: code review

d5c2ca8

doc: update CHANGELOG

959ddbc

fix; failure on CI due to node8 changes

10123ac

use named exports instead of asterisk

39893d9

SimenB approved these changes Dec 24, 2018

View reviewed changes

SimenB merged commit 3c7b110 into jestjs:master Dec 24, 2018

roblourens mentioned this pull request Jan 9, 2019

Unable to debug jest tests from CRA unless I have run npm run eject first microsoft/vscode#65793

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: TestScheduler now take in count the runInBand config #7518

fix: TestScheduler now take in count the runInBand config #7518

nasreddineskandrani commented Dec 15, 2018 •

edited

Loading

facebook-github-bot commented Dec 15, 2018

facebook-github-bot commented Dec 15, 2018

thymikee left a comment

thymikee Dec 15, 2018

nasreddineskandrani Dec 16, 2018 •

edited

Loading

codecov-io commented Dec 16, 2018 •

edited

Loading

nasreddineskandrani commented Dec 16, 2018 •

edited

Loading

thymikee Dec 16, 2018

nasreddineskandrani Dec 16, 2018 •

edited

Loading

thymikee Dec 16, 2018

nasreddineskandrani commented Dec 16, 2018 •

edited

Loading

thymikee left a comment

thymikee Dec 17, 2018

nasreddineskandrani Dec 21, 2018 •

edited

Loading

thymikee Dec 17, 2018

thymikee Dec 17, 2018

nasreddineskandrani Dec 21, 2018 •

edited

Loading

thymikee Dec 21, 2018

nasreddineskandrani Dec 21, 2018 •

edited

Loading

thymikee Dec 21, 2018

nasreddineskandrani Dec 21, 2018 •

edited

Loading

nasreddineskandrani Dec 21, 2018 •

edited

Loading

SimenB left a comment

nasreddineskandrani commented Dec 21, 2018 •

edited

Loading

thymikee commented Dec 21, 2018

SimenB commented Dec 21, 2018

nasreddineskandrani commented Dec 21, 2018

nasreddineskandrani commented Dec 21, 2018 •

edited

Loading

github-actions bot commented May 12, 2021

	import * as testSchedulerHelper from './testSchedulerHelper';
	import {computeRunInBand} from './testSchedulerHelper';

fix: TestScheduler now take in count the runInBand config #7518

fix: TestScheduler now take in count the runInBand config #7518

Conversation

nasreddineskandrani commented Dec 15, 2018 • edited Loading

Summary

Test plan

facebook-github-bot commented Dec 15, 2018

facebook-github-bot commented Dec 15, 2018

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasreddineskandrani Dec 16, 2018 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Dec 16, 2018 • edited Loading

Codecov Report

nasreddineskandrani commented Dec 16, 2018 • edited Loading

Choose a reason for hiding this comment

nasreddineskandrani Dec 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasreddineskandrani commented Dec 16, 2018 • edited Loading

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasreddineskandrani Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasreddineskandrani Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasreddineskandrani Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasreddineskandrani Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

nasreddineskandrani Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

nasreddineskandrani commented Dec 21, 2018 • edited Loading

thymikee commented Dec 21, 2018

SimenB commented Dec 21, 2018

nasreddineskandrani commented Dec 21, 2018

nasreddineskandrani commented Dec 21, 2018 • edited Loading

github-actions bot commented May 12, 2021

nasreddineskandrani commented Dec 15, 2018 •

edited

Loading

nasreddineskandrani Dec 16, 2018 •

edited

Loading

codecov-io commented Dec 16, 2018 •

edited

Loading

nasreddineskandrani commented Dec 16, 2018 •

edited

Loading

nasreddineskandrani Dec 16, 2018 •

edited

Loading

nasreddineskandrani commented Dec 16, 2018 •

edited

Loading

nasreddineskandrani Dec 21, 2018 •

edited

Loading

nasreddineskandrani Dec 21, 2018 •

edited

Loading

nasreddineskandrani Dec 21, 2018 •

edited

Loading

nasreddineskandrani Dec 21, 2018 •

edited

Loading

nasreddineskandrani Dec 21, 2018 •

edited

Loading

nasreddineskandrani commented Dec 21, 2018 •

edited

Loading

nasreddineskandrani commented Dec 21, 2018 •

edited

Loading