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

Extract watch mode #2362

Merged
merged 17 commits into from Jan 8, 2017

Conversation

Projects
None yet
4 participants
@rogeliog
Collaborator

rogeliog commented Dec 19, 2016

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

interrupt

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 19, 2016

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

codecov-io commented Dec 19, 2016

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

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

This comment has been minimized.

@cpojer

cpojer Dec 20, 2016

Contributor

nit: hasteMap

@cpojer

cpojer Dec 20, 2016

Contributor

nit: hasteMap

This comment has been minimized.

@cpojer

cpojer Dec 20, 2016

Contributor

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

@cpojer

cpojer Dec 20, 2016

Contributor

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

This comment has been minimized.

@rogeliog

rogeliog Dec 20, 2016

Collaborator

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.

@rogeliog

rogeliog Dec 20, 2016

Collaborator

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.

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

This comment has been minimized.

@cpojer

cpojer Dec 20, 2016

Contributor

omg tests. So good!

@cpojer

cpojer Dec 20, 2016

Contributor

omg tests. So good!

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

This comment has been minimized.

@rogeliog

rogeliog Dec 20, 2016

Collaborator

@cpojer this should be optional right?

@rogeliog

rogeliog Dec 20, 2016

Collaborator

@cpojer this should be optional right?

This comment has been minimized.

@cpojer

cpojer Dec 20, 2016

Contributor

yeah I guess this is fine.

@cpojer

cpojer Dec 20, 2016

Contributor

yeah I guess this is fine.

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Jan 5, 2017

Contributor

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!

Contributor

cpojer commented Jan 5, 2017

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!

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Jan 7, 2017

Contributor

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

Here is what this solves:

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?

Contributor

cpojer commented Jan 7, 2017

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

Here is what this solves:

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?

@rogeliog

This comment has been minimized.

Show comment
Hide comment
@rogeliog

rogeliog Jan 8, 2017

Collaborator

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

Collaborator

rogeliog commented Jan 8, 2017

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

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

This comment has been minimized.

@rogeliog

rogeliog Jan 8, 2017

Collaborator

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

@rogeliog

rogeliog Jan 8, 2017

Collaborator

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

This comment has been minimized.

@cpojer

cpojer Jan 8, 2017

Contributor

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.

@cpojer

cpojer Jan 8, 2017

Contributor

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.

@cpojer cpojer merged commit b2b2039 into facebook:master Jan 8, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rogeliog rogeliog deleted the rogeliog:extract-watch branch Jan 9, 2017

skovhus added a commit to skovhus/jest that referenced this pull request Apr 29, 2017

Extract watch mode (#2362)
* 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

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Extract watch mode (#2362)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment