[RFC] Multi project runner #3156

cpojer · 2017-03-16T08:17:26Z

Summary

Context: #2970

This diff is the initial RFC for the multi-project runner. This is the proposal that merges all functionality into jest-cli which will make Facebook's internal multi-runner obsolete. The initial draft changes the Jest CLI to operate on a list of TestContexts, which is a new name for HasteContexts with the configuration object added to it. This is basically the entire extent of the added complexity to make this work, the rest is refactors that should be made regardless.

The refactoring of the TestSequencer, runCLI and usage of async/await alone make the codebase actually easier to understand and concerns are separated better with this diff applied. These changes were already landed as part of this PR.

TODO

We need to make it so that there is an overall config that is used for the multi-runner. Currently this is the first one it finds but I believe the right way to go about it is an entirely separate configuration object that only defines behavior rather than tests to run.
Make watch mode work with this. I wanted to get to a MVP asap, so I left this one alone for now.
The printed information on the CLI doesn't show the proper paths to tests because it uses the wrong config object to determine the root. It should be the working directory or the root of the fake configuration.
We need a customization option for the reporter output per test so that we know which project/config a test belongs to. Not sure yet how to best customize it and what information to show, given that we only have one line.
I left a few TODOs in the code.

Test plan

cd examples
jest --experimentalProjects */ --no-cache

cpojer · 2017-03-16T08:20:36Z

I would like to take a moment to celebrate. Please take a look at my afternoon's journey through the attached diffs. This issue has been on my mind for an entire year now and is a result of all the work that went into Jest by all of you, the community members as well as the countless discussions I had about this with @DmitriiAbramov. I finally made a breakthrough over the last couple of weeks after being forced by @ljharb (and some other people who insulted me) to organize my thoughts. I couldn't have done it without you.

I was also guided only by Flow and prettier (which is an amazing workflow, btw.) and did an occasional sanity check that Jest wasn't broken on the way. After I was done with the last line of code after a couple hours of intense focus, everything worked on the first try. I'm happy, given that I haven't written any real code in months. 🎉

ljharb · 2017-03-16T21:25:30Z

I hope I didn't insult you! :-) (if so, my apologies)

cpojer · 2017-03-17T00:05:11Z

Not at all! Appreciate any of your comments, please do it more often.

cpojer · 2017-03-17T06:29:01Z

Rebased. I shipped all the code that was done as preparatory work including tests in #3165 and #3166 so that this PR can focus on the actual multi-project runner.

cpojer · 2017-03-19T01:39:45Z

RFC updated. All tests are passing with this and there are no more major issues but there is a lot of work that still needs to be done before we should ship it. The changes up to now fix a number of things on top of the initial RFC:

The "No tests found" message now lists each project individually instead of printing "No tests found" multiple times.
Reporters now receive a "Test" object instead of the config. Every action on a test now uses the appropriate context and the "TestRunner" instance doesn't receive a singular context any more.
Remove "runnerContext" and pass a set of all contexts based on the tests that were passed to TestRunner on to reporters. This makes snapshot updates work properly as well as coverage reporting.
Cleanups around how patterns are dealt with.

More TODOs:

Refactor the use of configs into a global and context local config.
Add "Ran all test suites […] in X projects."
Watch mode.
Add an integration test for the multi-runner (although the examples folder can act as such an integration test.
See TODOs above in the original comment.

Note: I'm getting married this week and will be on my honeymoon soon, so it'll be a while until this diff is done. I do appreciate intermediate reviews up until this point though as well as suggestions if this could be broken up into smaller pieces, something I currently can't conceive. Nevertheless, this diff isn't as big as I thought it would be. Also happy if anybody wants to build on top of this PR to work on any of the TODOs that I outlined.

rogeliog

I looked at half of it, I'll finish it tomorrow 😄

rogeliog · 2017-03-30T06:12:23Z

packages/jest-cli/src/cli/runCLI.js

    if (argv.debug) {
-      logDebugMessages(config, pipe);
+      // TODO fix/remove this: there should be a `--show-config` argument.


Do we have a ticket tracking this? I'm happy to add the arg if needed

We also could make a checklist under #2970 and treat it as an umbrella task

Before merging anything, I'll get rid of all the inline TODOs at least. Thanks for pointing it out.

rogeliog · 2017-03-30T22:40:01Z

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

@@ -30,7 +30,7 @@ jest.mock('../TestWorker', () => {});
 jest.mock('../reporters/DefaultReporter');



Do we have tests for "multi contexts" test runs?

rogeliog · 2017-03-30T22:56:43Z

packages/jest-cli/src/cli/runCLI.js

    if (argv.watch || argv.watchAll) {
+      const {config} = configs[0];


Why only the first config? Don't we need to pass all of them?

Oops, I just read your todo comment

Make watch mode work with this. I wanted to get to a MVP asap, so I left this one alone for now.

rogeliog · 2017-03-30T23:04:18Z

Are we thinking of adding a way to specify projects that are in the same root but that use different config?

- jest.config.someOtherStuff.json
- jest.config.e2e.json
- jest.config.json

Or would I need to trick Jest by nesting each config into its own directory and then setting the root of them to ../?

codecov-io · 2017-04-09T03:07:41Z

Codecov Report

Merging #3156 into master will increase coverage by 0.02%.
The diff coverage is 53.7%.

@@            Coverage Diff             @@
##           master    #3156      +/-   ##
==========================================
+ Coverage   65.02%   65.04%   +0.02%     
==========================================
  Files         177      178       +1     
  Lines        6510     6537      +27     
  Branches        4        4              
==========================================
+ Hits         4233     4252      +19     
- Misses       2276     2284       +8     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-cli/src/reporters/BaseReporter.js	`66.66% <ø> (ø)`	⬆️
packages/jest-cli/src/reporters/DefaultReporter.js	`16.39% <ø> (ø)`	⬆️
packages/jest-cli/src/reporters/NotifyReporter.js	`25.92% <ø> (ø)`	⬆️
...ages/jest-cli/src/lib/handleDeprecationWarnings.js	`0% <0%> (ø)`
packages/jest-cli/src/lib/setState.js	`95.45% <100%> (ø)`	⬆️
packages/jest-cli/src/TestPathPatternPrompt.js	`98.07% <100%> (+0.03%)`	⬆️
packages/jest-cli/src/TestSequencer.js	`100% <100%> (ø)`	⬆️
packages/jest-cli/src/lib/getTestPathPattern.js	`60% <100%> (ø)`
packages/jest-cli/src/TestRunner.js	`31.32% <11.76%> (-1.61%)`	⬇️
packages/jest-cli/src/reporters/SummaryReporter.js	`26.15% <28.57%> (-0.52%)`	⬇️
... and 5 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 890ac11...8831e00. Read the comment docs.

cpojer · 2017-04-12T06:25:22Z

Pulling things out of this PR to make the things I add on top of the existing code easier to review over time: #3289

thymikee · 2017-04-12T08:24:32Z

packages/jest-cli/src/cli/runCLI.js

-        // TODO
-        configs[0].hasDeprecationWarnings,
-      );
+      if (hasDeprecationWarnings) {


Oh, this is so much nicer

cpojer · 2017-04-12T10:26:36Z

Done:

Watch mode now works.
Added "Ran all test suites […] in X projects."

Todo:

The CLI doesn't show the proper paths because we are using the wrong config object to determine the root, this also makes the pattern mode highlight a bit broken right now I believe.

Follow-up before Jest 20, but shouldn't block this PR (it would get way too big):

Integration test using the examples folder.
Refactor the use of configs into a global and context local config. Right now everything uses the first config it finds, which is not ideal.
Need a customization option for the reporter output per test so that we know which project/config a test belongs to. Not sure yet how to best customize it and what information to show, given that we only have one line.

@thymikee @rogeliog ready for another round of review!

thymikee · 2017-04-12T10:28:21Z

Wonder if this commit Refactor watch mode prompts. could be extracted to separate PR?

cpojer · 2017-04-12T13:49:41Z

Alright, the two other diffs were pulled out and are merged and we are fully rebased. Now we are back down to a manageable +500 -500 diff :)

thymikee · 2017-04-12T16:50:06Z

packages/jest-cli/src/TestRunner.js

+      contexts.forEach(context => {
+        const status = snapshot.cleanup(
+          context.hasteFS,
+          this._config.updateSnapshot,


I think in the end we would like to use context.config.updateSnapshot?

nvm, discard that comment

aaronabramov

I think separating the config into sharedConfig and projectConfig is really important on this stage.
If we don't do it now, we might end up with a lot of leaky abstractions or modules that have way too much context on the outside world.

Having a simple external api (and probably a single config for both shared and project values for the end used) but having a hard separation once it gets into jest-config module into two separate configs seems to be the way to go.

aaronabramov · 2017-04-12T18:13:23Z

packages/jest-cli/src/runJest.js

-  sequencer.cacheResults(tests, results);
+  const allTests = testRunData
+    .reduce((tests, testRun) => tests.concat(testRun.tests), [])
+    .sort((a: Test, b: Test) => {


why is this logic not inside TestSequencer?

aaronabramov · 2017-04-12T18:25:21Z

packages/jest-cli/src/runJest.js

+
+  return data;
+};
+
 const runJest = async (


can you rewrite this function as something like this and take everything else out?

runJest = async (/* ... */) => { const maxWorkers = getMaxWorkers(argv); const testPathPattern = getTestPathPattern(argv); const firstContext = contexts[0]; const testRunData = await getTestRunData(/*...*/); const sortedTestsFromAllProjects = getSortedTestsFromAllProjects(/*...*/); warnIfNoTestsFound(/*...*/); setVerboseIfRunningOnlyOneTest(contexts); const testResults = await runTests(/*...*/); return processResults(testResults); };

sorry :( very hard to review and read the code that has dozens of branches :(

aaronabramov · 2017-04-12T18:33:06Z

packages/jest-cli/src/runJest.js

+  const results = await new TestRunner(
+    context.config,
+    {
+      getTestSummary: () =>


this looks like a leaky abstraction of SummaryReporter, is it possible to move it down the hierarchy and pass needed data?

i just realized that it was already there before, is it still possible to move it inside?

aaronabramov · 2017-04-12T18:52:08Z

packages/jest-cli/src/cli/runCLI.js

-      watch: config.watch,
-    });
+    const hasteMapInstances = Array(configs.length);
+    const contexts = await Promise.all(


following our meeting discussion: we can put both sharedConfig and projectConfig in the context. i think

aaronabramov · 2017-04-12T18:55:18Z

packages/jest-cli/src/lib/handleDeprecationWarnings.js

+const chalk = require('chalk');
+const {KEYS} = require('../constants');
+
+module.exports = (


do you mind adding a line or two explaining why we need this? 🙂
the callsite doesn't really give any information either

aaronabramov · 2017-04-12T19:01:18Z

packages/jest-cli/src/reporters/SummaryReporter.js

    this._estimatedTime = options.estimatedTime;
  }

  onRunComplete(
+    contexts: Set<Context>,
    config: Config,


is this going to be a config for project where the last test was run from?
this looks a little strange, since onRunComplete seems to be responsible for summarizing the whole (multi-project) run and not having any dependencies on a specific project

cpojer · 2017-04-17T05:11:07Z

Ok, addressed all of Dmitrii's concerns except the config split which we agreed we'll do in a follow-up. Also made the paths relative to process.cwd() instead of the individual rootDirs when running with multiple configs.

This diff is now ready for a final review (minus some CI issue that I'll fix :) ).

…jest-cli test run.

…rom reporters.

… level.

* Add `—experimentalProjects` to run multiple projects within the same jest-cli test run. * Improve the “no tests found” message for multiple projects. * Do not pass a single context to TestRunner and remove RunnerContext from reporters. * Rename `patternInfo` to `PathPattern` * Remove `hasDeprecationWarnings` from `watch` function, move it up one level. * Make watch mode work with multiple projects. * Refactor runJest and Reporters, show proper relative paths. * SearchSource now returns `tests: Array<Test>`. * Use one TestSequencer instance for all contexts. * Fix runJest-test. * Fix TestSequencer-test on Windows.

github-actions · 2021-05-13T17:24:17Z

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 Mar 16, 2017

cpojer force-pushed the multi-runner branch from 7ae7a3d to 3646288 Compare March 17, 2017 00:51

This was referenced Mar 17, 2017

Don’t pass config to the VerboseReporter. #3165

Merged

Refactor runner #3166

Merged

cpojer force-pushed the multi-runner branch from 3646288 to 1a30f7c Compare March 17, 2017 06:28

cpojer force-pushed the multi-runner branch from 1a30f7c to 5db683f Compare March 17, 2017 06:36

cpojer mentioned this pull request Mar 17, 2017

Rename HasteContext to Context. #3171

Merged

cpojer force-pushed the multi-runner branch 5 times, most recently from 65488b3 to 07acc0c Compare March 19, 2017 01:30

rogeliog reviewed Mar 30, 2017

View reviewed changes

cpojer force-pushed the multi-runner branch from 07acc0c to ff516d5 Compare April 9, 2017 02:42

cpojer mentioned this pull request Apr 12, 2017

Pass “Test”s to reporters; update Test to contain contexts. #3289

Merged

cpojer force-pushed the multi-runner branch 2 times, most recently from 8057960 to 1f900d0 Compare April 12, 2017 08:12

thymikee reviewed Apr 12, 2017

View reviewed changes

cpojer force-pushed the multi-runner branch from 7995cbd to 762042f Compare April 12, 2017 10:22

cpojer force-pushed the multi-runner branch 3 times, most recently from d6b4e2e to 8ecccf5 Compare April 12, 2017 13:48

thymikee reviewed Apr 12, 2017

View reviewed changes

aaronabramov suggested changes Apr 12, 2017

View reviewed changes

thymikee mentioned this pull request Apr 14, 2017

Make sure updateSnapshot propagates hasteContext.config #3298

Merged

cpojer force-pushed the multi-runner branch from 8ecccf5 to 8f8ff04 Compare April 17, 2017 02:17

cpojer force-pushed the multi-runner branch from 8f8ff04 to 7d64794 Compare April 18, 2017 00:56

cpojer added 9 commits April 17, 2017 17:57

Add —experimentalProjects to run multiple projects within the same …

9a13fd6

…jest-cli test run.

Improve the “no tests found” message for multiple projects.

e339ff6

Do not pass a single context to TestRunner and remove RunnerContext f…

5752f24

…rom reporters.

Rename patternInfo to PathPattern

720d37b

Remove hasDeprecationWarnings from watch function, move it up one…

a0f2587

… level.

Make watch mode work with multiple projects.

2977a2e

Refactor runJest and Reporters, show proper relative paths.

d44e8b5

SearchSource now returns tests: Array<Test>.

ac61b80

Use one TestSequencer instance for all contexts.

e8e609e

cpojer force-pushed the multi-runner branch from 7d64794 to e8e609e Compare April 18, 2017 00:58

cpojer added 2 commits April 17, 2017 19:30

Fix runJest-test.

8db8140

Fix TestSequencer-test on Windows.

8831e00

aaronabramov approved these changes Apr 18, 2017

View reviewed changes

cpojer merged commit e94ea05 into jestjs:master Apr 18, 2017

MicheleBertoli mentioned this pull request May 3, 2017

Problems with testEnvironment "node" styled-components/jest-styled-components#9

Closed

cpojer deleted the multi-runner branch May 4, 2017 15:44

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Multi project runner #3156

[RFC] Multi project runner #3156

cpojer commented Mar 16, 2017 •

edited

Loading

cpojer commented Mar 16, 2017 •

edited

Loading

ljharb commented Mar 16, 2017 •

edited

Loading

cpojer commented Mar 17, 2017

cpojer commented Mar 17, 2017

cpojer commented Mar 19, 2017

rogeliog left a comment

rogeliog Mar 30, 2017

thymikee Mar 30, 2017

cpojer Mar 30, 2017

rogeliog Mar 30, 2017

rogeliog Mar 30, 2017

rogeliog Mar 30, 2017

rogeliog commented Mar 30, 2017 •

edited

Loading

codecov-io commented Apr 9, 2017 •

edited

Loading

cpojer commented Apr 12, 2017

thymikee Apr 12, 2017

cpojer commented Apr 12, 2017 •

edited

Loading

thymikee commented Apr 12, 2017 •

edited

Loading

cpojer commented Apr 12, 2017

thymikee Apr 12, 2017

thymikee Apr 12, 2017

aaronabramov left a comment

aaronabramov Apr 12, 2017

aaronabramov Apr 12, 2017

aaronabramov Apr 12, 2017

aaronabramov Apr 12, 2017

aaronabramov Apr 12, 2017

aaronabramov Apr 12, 2017

aaronabramov Apr 12, 2017

cpojer commented Apr 17, 2017

github-actions bot commented May 13, 2021

		@@ -30,7 +30,7 @@ jest.mock('../TestWorker', () => {});
		jest.mock('../reporters/DefaultReporter');

		if (argv.watch \|\| argv.watchAll) {
		const {config} = configs[0];

[RFC] Multi project runner #3156

[RFC] Multi project runner #3156

Conversation

cpojer commented Mar 16, 2017 • edited Loading

cpojer commented Mar 16, 2017 • edited Loading

ljharb commented Mar 16, 2017 • edited Loading

cpojer commented Mar 17, 2017

cpojer commented Mar 17, 2017

cpojer commented Mar 19, 2017

rogeliog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogeliog commented Mar 30, 2017 • edited Loading

codecov-io commented Apr 9, 2017 • edited Loading

Codecov Report

cpojer commented Apr 12, 2017

Choose a reason for hiding this comment

cpojer commented Apr 12, 2017 • edited Loading

thymikee commented Apr 12, 2017 • edited Loading

cpojer commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronabramov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Apr 17, 2017

github-actions bot commented May 13, 2021

cpojer commented Mar 16, 2017 •

edited

Loading

cpojer commented Mar 16, 2017 •

edited

Loading

ljharb commented Mar 16, 2017 •

edited

Loading

rogeliog commented Mar 30, 2017 •

edited

Loading

codecov-io commented Apr 9, 2017 •

edited

Loading

cpojer commented Apr 12, 2017 •

edited

Loading

thymikee commented Apr 12, 2017 •

edited

Loading