Extract watch mode #2362

rogeliog · 2016-12-19T18:20:35Z

Summary

This extracts the watch mode to its own module, which was suggested by @cpojer as a prerequisite for #2324.

This should make watch mode faster and removes some duplicated code from jest.js.
Now we only need to reconcile with the file system only when a file is added/removed, but not otherwise

More info #2324 (comment) I hope that this will also allow us to add more tests in the future.

Known issues:

~~Test interruption is not working and I'm not sure why. It seems that I'm setting the correct state in the TestWatcher :(~~ UPDATE: I first thought that the issue was introduced in this PR, but it seems that it is also happening in master. Based on my understanding it was introduced in #2312

codecov-io · 2016-12-19T19:03:56Z

Current coverage is 62.92% (diff: 21.46%)

Merging #2362 into master will increase coverage by 0.66%

@@             master      #2362   diff @@
==========================================
  Files           116        123     +7   
  Lines          4802       4826    +24   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2990       3037    +47   
+ Misses         1812       1789    -23   
  Partials          0          0

Powered by Codecov. Last update 87cb544...16b2780

cpojer · 2016-12-20T16:57:25Z

packages/jest-runtime/src/index.js

@@ -212,6 +215,7 @@ class Runtime {
      retainAllFiles: false,
      roots: config.testPathDirs,
      useWatchman: config.watchman,
+      watch: (options && options.watch),


can you remove the curly braces here? They don't add anything. We are only using it above for maxWorkers because we also use || but in the console case we don't add it.

cpojer · 2016-12-20T16:59:52Z

packages/jest-cli/src/watch.js

+  let testWatcher;
+  let displayHelp = true;
+
+  jestHasteMap.on('change', ({eventsQueue, hasteFS, moduleMap}) => {


can we call this hasteMap? It's called with jest prefixed in react-native mainly because the codebase there still has a legacy haste map ;)

cpojer · 2016-12-20T17:00:17Z

packages/jest-cli/src/jest.js

-            return null;
-          }
+      createDirectory(config.cacheDirectory);
+      const jestHasteMap = Runtime.createHasteMap(config, {


nit: hasteMap

It seems to me like we can get rid of Runtime.createHasteContext entirely with your PR applied, right?

packages/jest-cli/src/__tests__/SearchSource-test.js is using it, so I didn't remove it. If you prefer, I can also just move it inside of SearchSource-test.js as a helper function only for those tests.

cpojer · 2016-12-20T17:04:54Z

packages/jest-cli/src/jest.js

-          testWatcher = new TestWatcher({isWatchMode: true});
-          pipe.write(CLEAR);
+      return jestHasteMap.build().then(
+        hasteMap => ({


we could potentially move createHasteContext into a utility function inside of jest-cli and do this:

const createHasteContext = (config, {hasteFS, moduleMap}) => { hasteFS, resolver: Runtime.createResolver(config, moduleMap), });

this way we can reuse this "map" operation of the code in two places. I think it makes sense, this may change in the future and I'd like to reuse code if possible :)

cpojer · 2016-12-20T17:05:48Z

packages/jest-cli/src/jest.js

+        },
+      )
+      .then(hasteContext => {
+        if (argv.debug) {


Can we move this before creating the haste map again? I don't think we need to create the haste map before logging this. Could also factor this if out into a logDebugMessages.

cpojer · 2016-12-20T17:06:15Z

packages/jest-cli/src/lib/__tests__/getMaxWorkers-test.js

+* @emails oncall+jsinfra
+*/
+
+const getMaxWorkers = require('../getMaxWorkers');


omg tests. So good!

cpojer · 2016-12-20T17:29:34Z

packages/jest-cli/src/runJest.js

+
+const realFs = require('fs');
+const fs = require('graceful-fs');
+fs.gracefulify(realFs);


I don't think we need this here, it already happens in jest.js which should be the entry point for Jest.

We can simply use const fs = require('graceful-fs'); right?

cpojer · 2016-12-20T17:31:18Z

packages/jest-cli/src/watch.js

+
+const realFs = require('fs');
+const fs = require('graceful-fs');
+fs.gracefulify(realFs);


same here, we don't need this

cpojer · 2016-12-20T17:31:29Z

packages/jest-cli/src/watch.js

+const TestWatcher = require('./TestWatcher');
+const runJest = require('./runJest');
+const setWatchMode = require('./lib/setWatchMode');
+const HasteMap = require('jest-haste-map');


can you sort these alphabetically?

rogeliog · 2016-12-20T18:41:19Z

types/HasteMap.js

@@ -35,7 +35,7 @@ export type InternalHasteMap = {|
 export type HasteMap = {|
  hasteFS: HasteFS,
  moduleMap: ModuleMap,
-  __hasteMapForTest: ?InternalHasteMap,
+  __hasteMapForTest?: ?InternalHasteMap,


@cpojer this should be optional right?

yeah I guess this is fine.

rogeliog · 2016-12-20T19:07:44Z

packages/jest-cli/src/watch.js

@@ -86,7 +86,9 @@ const watch = (
      results => {
        isRunning = false;
        hasSnapshotFailure = !!results.snapshot.failure;
-        testWatcher.setState({interrupted: false});
+        if (config.bail) {


I think the test interruption issue was introduced here #2312 and it is happening in master...

@cpojer Is this the right fix? I think that we should only set {interrupted: false} when using bail... I decided to fix it as part of this PR but it's fine if you prefer me to do it as a separate one that just fixes the test interruption bug.

Ok, seems like my fix wasn't super thorough. I need to think about this, for now using config.bail as a flag seems reasonable. Do you mind opening another issue linking to this piece in the code? I'll take care of it at a later time.

Filed it here: #2385

cpojer · 2017-01-05T18:08:21Z

With this change we don't store the haste map on disk before starting to run tests. This means if Jest would like to run tests in parallel, we need to persist the haste map or pass it through to the workers, otherwise new files that get added can't be run and will fail.

I spent some time running some benchmarks on passing data to and back from the worker, see https://gist.github.com/cpojer/488c85f2cf86cacc3f4cc378be3f68eb This version takes about 6s on my mbp and about 1.3s when only sending data to the worker only (like we'd do in Jest). This is still a lot of overhead. One issue with persisting the state is also that the watchman clocks aren't updated through the file watcher – this means that storing the haste map as is will result in a recrawl of the file system on the next startup. There are a number of ways to address this:

Ignore the overhead this incurs. Fast tests will run in the same process and slow tests (3+ seconds or so) run in parallel. The added overhead isn't that big of a deal on an already slow test run. This could be optimized by only sending the data of the module map (about half the amount of data).
Split up the module map and haste fs data into separate cache files and only write the module map one. This should allow to properly reconcile the cached data with information from watchman on the next startup.

I'm inclined to go with the first solution and benchmarking it on our internal react-native codebase, which is relatively big, to see what the performance will look like. Either way, I need to find a way to write tests for this as it is easy to break. I'll push directly to this PR and merge it once done. @rogeliog nice work on making this all happen!

This reverts commit ba4baec.

cpojer · 2017-01-07T12:21:18Z

packages/jest-cli/src/watch.js

-  hasteMap.on('change', ({eventsQueue, hasteFS, moduleMap}) => {
-    if (eventsQueue.find(({type}) => type !== 'change')) {
-      hasteContext = createHasteContext(config, {hasteFS, moduleMap});
-    }


we always need a new instance of the hasteContext not just when they get added/deleted. At Facebook, if a module's "@providesModule" header changes, the module receives a new name for our "haste" module system. cc @rogeliog

cpojer · 2017-01-07T12:21:35Z

packages/jest-cli/src/TestRunner.js

      if (watcher.isInterrupted()) {
        return Promise.reject();
      }
      this._dispatcher.onTestStart(config, path);
-      return promisify(farm)({config, path});
+      return worker({config, path});


just nits here, not really fixes.

cpojer · 2017-01-07T12:28:34Z

Ok I tested this and it works now. @rogeliog mind checking my code before I merge this?

Here is what this solves:

Make it so all tests are run in parallel (Set https://github.com/cpojer/jest/blob/833dc59d3b2393c368c3c503d362e42b9f77d8ba/packages/jest-cli/src/TestRunner.js#L254 to false)
Add require('HasteModule') anywhere in a test.
jest --watch that test. It will fail.
Create a file with the content from below.
When saving, the test will pass.

HasteModule.js

/**
 * @providesModule HasteModule
 */
module.exports = () => 'banana';

Without my last commit, even after creating the "HasteModule.js" file, the test would fail and be unable to locate the new haste module (haste is Facebook's module resolution system) because we don't persist the haste map to disk so when we read it in the worker thread, it is still missing. My change makes it so that in watch mode, the module map is always passed to the workers and it is only read from disk in the regular mode.

One thing I haven't had time to do is write tests. It would be good to create some tests with some of the env mocked, like a worker-farm mock (similar to the tests in jest-haste-map) and a test that ensures that when TestWatcher's state is set to watch mode, it passes the raw module map and otherwise it doesn't. @rogeliog is this something you could help out with to get this PR over the finish line?

…tch mode.

rogeliog · 2017-01-08T02:37:35Z

@cpojer Code looks good, I've been working on adding the tests that you mentioned and doing some good progress there...

rogeliog · 2017-01-08T06:05:28Z

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

+
+    const runner = new TestRunner(hasteContext, config, {maxWorkers: 2});
+
+    return runner._createParallelTestRun(


I decided to tests _createParallelTestRun instead of runTests even though it is a private method.

I did this because the current implementation of runTests is a harder to test, mainly because of all of the extra logic that the inner callbacks have. If you prefer I can also spend some time refactoring some of it to make runTests easier to test :)

Yeah I was thinking that as well. We could do that as a follow-up. TestRunner is one of the oldest parts of Jest and has been through a lot. I'm open to any ideas on how to improve the code to make it more testable. Feel free to create an issues to discuss or a PR to fix.

One thing that is untested as well is the performance test cache and that Jest runs failed tests first on a re-run. This all may break at any time. I'm really grateful you are adding such extensive unit tests for the watch mode, as it was entirely untested previously.

rogeliog · 2017-01-08T06:38:41Z

packages/jest-cli/src/watch.js

@@ -93,7 +75,7 @@ const watch = (
        // and prevent test runs from the previous run.
        testWatcher = new TestWatcher({isWatchMode: true});
        if (displayHelp) {
-          console.log(usage(argv, hasSnapshotFailure));
+          pipe.write(usage(argv, hasSnapshotFailure));


@cpojer I changed this and 7909aeb#diff-d8a3d0a3a8b42ab87b18835ad8f7cb46L177 to use pipe.write instead of console.log for testability purposes, but I'm unsure if this has any impact in on how things are printed out that I'm unaware of... I'll keep investigating tomorrow

Should be fine. Unsure why we used console.log here in the first place.

* Add watch option to HastMapOptions * wip * Extract runJest and other utility functions * Start to extract watch to its own module * Fix flow and lint errors * Add tests to getMaxWorkers and setWatchMode * Use createHasteContext instead of custom method * Keep haste context up to date FS add/remove This reverts commit ba4baec. * Fix flow and eslint errors * Fix caps in require * Extract createHasteContext and logDebugMessages * Alphabetize requires and remove extra graceful-fs requires * Fix test interrupution issue with --bail * Fixes for new watch mode. * Pass the raw module map from the test runner to the test worker in watch mode. * Add test for injecting rawModuleMap to workers * Add tests for watch mode and extract KEYS to contstants module

github-actions · 2021-05-14T02:31:19Z

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 19, 2016

cpojer reviewed Dec 20, 2016

View reviewed changes

rogeliog commented Dec 20, 2016

View reviewed changes

This was referenced Dec 20, 2016

Watch mode extraction: pass haste map to workers manually #2386

Closed

Treat arrows as no-op while entering a pattern FIX #2427 #2428

Merged

cpojer mentioned this pull request Jan 5, 2017

Separate implementation of jest-snapshot #2497

Closed

rogeliog force-pushed the extract-watch branch from f2b637b to 3063f80 Compare January 5, 2017 15:45

cpojer mentioned this pull request Jan 6, 2017

How to ignore files on jest watch mode #2516

Closed

rogeliog added 11 commits January 7, 2017 10:03

Add watch option to HastMapOptions

95aa7f8

wip

4161822

Extract runJest and other utility functions

c17bc41

Start to extract watch to its own module

5ce07f1

Fix flow and lint errors

b8d1ea0

Add tests to getMaxWorkers and setWatchMode

0032bf7

Use createHasteContext instead of custom method

c9e77fc

Keep haste context up to date FS add/remove

f89cbdb

This reverts commit ba4baec.

Fix flow and eslint errors

63ec6b0

Fix caps in require

51918c2

Extract createHasteContext and logDebugMessages

e79d1b7

rogeliog and others added 3 commits January 7, 2017 10:03

Alphabetize requires and remove extra graceful-fs requires

90a452c

Fix test interrupution issue with --bail

e7c0d09

Fixes for new watch mode.

0504011

cpojer force-pushed the extract-watch branch from 3063f80 to 8053541 Compare January 7, 2017 12:19

cpojer mentioned this pull request Jan 7, 2017

Fix test watcher interruption properly. #2385

Closed

cpojer reviewed Jan 7, 2017

View reviewed changes

Pass the raw module map from the test runner to the test worker in wa…

16b2780

…tch mode.

cpojer force-pushed the extract-watch branch from 8053541 to 16b2780 Compare January 7, 2017 12:34

Add test for injecting rawModuleMap to workers

4a91dd6

Add tests for watch mode and extract KEYS to contstants module

7909aeb

rogeliog commented Jan 8, 2017

View reviewed changes

cpojer merged commit b2b2039 into jestjs:master Jan 8, 2017

rogeliog mentioned this pull request Jan 9, 2017

Fix pipe.write regression introduced after #2362 #2539

Merged

rogeliog deleted the extract-watch branch January 9, 2017 06:59

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract watch mode #2362

Extract watch mode #2362

rogeliog commented Dec 19, 2016 •

edited

codecov-io commented Dec 19, 2016 •

edited

cpojer Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

rogeliog Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

rogeliog Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

rogeliog Dec 20, 2016

cpojer Dec 20, 2016

rogeliog Dec 20, 2016

cpojer Dec 20, 2016

cpojer Dec 20, 2016

cpojer commented Jan 5, 2017

cpojer Jan 7, 2017

cpojer Jan 7, 2017

cpojer commented Jan 7, 2017

rogeliog commented Jan 8, 2017

rogeliog Jan 8, 2017

cpojer Jan 8, 2017

rogeliog Jan 8, 2017 •

edited

cpojer Jan 8, 2017

github-actions bot commented May 14, 2021


		const runner = new TestRunner(hasteContext, config, {maxWorkers: 2});

		return runner._createParallelTestRun(

Extract watch mode #2362

Extract watch mode #2362

Conversation

rogeliog commented Dec 19, 2016 • edited

codecov-io commented Dec 19, 2016 • edited

Current coverage is 62.92% (diff: 21.46%)

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

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

Choose a reason for hiding this comment

cpojer commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 7, 2017

rogeliog commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogeliog Jan 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 14, 2021

rogeliog commented Dec 19, 2016 •

edited

codecov-io commented Dec 19, 2016 •

edited

rogeliog Jan 8, 2017 •

edited