Fix bug with watchers on macOS causing test to crash #2957

ro-savage · 2017-02-21T15:21:15Z

Summary

This fixes an issue on OS X when there are too many files/folders to watch that causes the test to crash with error
2016-09-22 10:49 node[79167] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)

See Issue #1767

This solve the issue by updating sane to 1.6.0 and passing a regular expression passed on the the testPathIgnorePatterns config option. This allows sane to never apply the watchers.

Currently there are no defaults. Maybe it should default to ignoring .git|node_modules if nothing is passed? Or else people may still encounter the error.

The error is caused by some mixture node/OSX. (See nodejs/node-v0.x-archive/issues/5463), where node/osx can't handle the amount of watchers.

Reproduction

On macOS Sierra, uninstall watchman (if installed), run test in watch mode on a sufficiently large app. Jest exits with exception.

Test plan

I am unsure how to test this change. Feedback welcome.

Questions

I was unsure of the best way to handle the typing.

I went with testPathIgnorePattern: RegExp | null, with allows us to pass null if no test patterns have been supplied.

The other option was to pass a RegExp of /(?:)/ that matches everything, and then expect only type RegExp.

Help wanted

I have tested this on my machine and it working (linking to my project that fails usually). It has also passed the the test suite.

It would be great to have it tested by others to make sure that it solves the problem for everyone. Any who encountered the issue in the past and solve by installing watchman will need to uninstall (brew uninstall watchman) and try run the tests.

Steps to test:
brew uninstall watchman
git clone https://github.com/ro-savage/jest.git
git checkout fix-watcher-bug-osx
yarn install
yarn build
cd packages/jest-cli
yarn link

Then go to your project that you had crashing tests in a run
yarn link "jest-cli"

Make sure you have testPathIgnorePatterns config set to includenode_modules
Run your tests as usual.

Feedback

This is a minor code change, but took me a long time to track down and fix (and its now 430am). Any feedback, suggestions are very welcome.

codecov-io · 2017-02-21T15:45:53Z

Codecov Report

Merging #2957 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2957      +/-   ##
==========================================
+ Coverage    67.6%   67.61%   +<.01%     
==========================================
  Files         146      146              
  Lines        5338     5339       +1     
==========================================
+ Hits         3609     3610       +1     
  Misses       1729     1729

Impacted Files	Coverage Δ
packages/jest-haste-map/src/index.js	`95.39% <100%> (+0.02%)`	✅
...s/jest-editor-support/src/parsers/BabylonParser.js	`98.93% <0%> (ø)`	✅

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 fa80da7...ad182a0. Read the comment docs.

cpojer · 2017-02-21T19:40:16Z

Thanks! Shouldn't we use modulePathIgnorePatterns instead?

ro-savage · 2017-02-21T21:14:40Z

@cpojer - modulePathIgnorePatterns vs testPathIgnorePatterns. modulePathIgnorePatterns felt like it was very specifically node_modules or bower_components, while testPathIgnorePatterns can include any folders/files you want to ignore. E.g. .git, dist, static etc.

For examples Create React App uses:
"testPathIgnorePatterns": [<rootDir>/(build|docs|node_modules|scripts|coverage)/"],

cpojer · 2017-02-27T02:32:57Z

Hmm, in Jest, modulePathIgnorePatterns means Jest will actually ignore those files for real. jest-haste-map also doesn't know anything about "tests" because the module doesn't have anything to do with tests. I'd prefer to change it to that; which also means the change will be more minimal (you can add a new "ignorePattern" option; which is a bit redundant but will probably be ok). I suggest create-react-app to change once this change has landed.

cpojer · 2017-03-02T13:51:04Z

Ping @ro-savage!

ro-savage · 2017-03-02T14:31:14Z

@cpojer - Updated to use modulePathIgnorePatterns.

Tested by linking updated jest. Would crash with when package.json was using testPathIgnorePatterns to filter node_modules and ran perfect when usingmodulePathIgnorePatterns to filter node_modules.

Previous to this, we were using this fix locally and was working 100% of the time for a week (20+ runs).

I don't think anyone has taken time to test. But certainly appears to fix the issue and as far as I can tell the changes a very minor and shouldn't cause any unforeseen issues.

Let me know if you'd like to merge and I can rebase into a single commit.

cpojer · 2017-03-02T16:23:11Z

Wait, are you sure this version works? Sane uses anymatch with the ignored option but ignorePattern is a regex, not a glob.

ro-savage · 2017-03-02T19:35:01Z

Yes I am sure. I tested with and without the package.json including"modulePathIgnorePatterns": [ "<rootDir>/(build|docs|node_modules|scripts|coverage)/" ], and it crashed without and worked with. (And also re-ran the jest test suite, and my own 2 projects tests suites)

anymatch accepts glob string and RegExp.

matchers: (Array|String|RegExp|Function) String to be directly matched, string with glob patterns,
regular expression test, function that takes the testString as an argument 
and returns a truthy value if it should be matched, or an array of any number and mix of these types.

and was taking a regex in the first PR as well which has been more extensively tested.

cpojer · 2017-03-02T20:18:40Z

Would you mind double checking whether a duck-typed RegEx will also work? We are using it here: https://github.com/facebook/react-native/blob/0cb2bc104f9958dfea2a7e22b229a19abeb59fca/packager/src/node-haste/index.js#L105

Arguably that's not a great use of this API, but we had to make it work. If this doesn't work, we have to change the code in react-native to support it somehow :)

ro-savage · 2017-03-03T13:12:23Z

{ test: 'some/path' } when passed to anymatch will always return false (i.e. Anything matching the test path will not be ignored by sane and will be processed by nodejs_walker.)

So if this was used as the ignorePattern on a react-native project, it will still get exceptions/crash if running through too many files.

I am not sure why it's ducked typed?

We could make it a real RegExp when passed to JestHasteMap or have a check within JestHasteMap itself or leave it as it doesn't break any existing functionality.

cpojer · 2017-03-03T13:22:57Z

Mainly because we couldn't change react-native. react-native needs a function to evaluate this instead of a regex and we need to make sure this use-case continues to work. We can type ignorePattern as Function | RegExp from now on and forward that to anymatch. That way, all we need to do is change react-native to pass the ignorePattern directly instead of the duck-typing, right?

ro-savage · 2017-03-03T13:44:28Z

Yes that sounds correct to me.

(Although I admit I still don't fully understand how the ducktyping works. As test is a string not a function? And ignorePattern is an object with one prop. I don't understand how that fixes 'react-native needs a function instead of regex').

cpojer · 2017-03-03T14:00:03Z

That's because the only use of the pattern is here: https://github.com/cpojer/jest/blob/master/packages/jest-haste-map/src/index.js#L687

Would you mind changing the flow types to also accept a function instead and change the code to call .test if it is a regex, otherwise call the ignorePattern directly?

ro-savage · 2017-03-03T14:56:13Z

Sorry, not entirely sure what you'd like me to do.

Do you mean changing
const ignorePattern = this._options.ignorePattern;
to
const ignorePattern = Object.prototype.toString.call(this._options.ignorePattern) === '[object RegExp]' ? this._options.ignorePattern : this._options.ignorePattern.test;

So that anymatch will received the regex, or a function/string held in ignorePattern.test if it exists?

And then flowtype should be ignorePattern: RegExp | {test: Function | string}

Or...

Will react-native here https://github.com/facebook/react-native/blob/0cb2bc104f9958dfea2a7e22b229a19abeb59fca/packager/src/node-haste/index.js#L105 be changed to pass a function?

In which case const ignorePattern = this._options.ignorePattern remains the same and type is updated toignorePattern: RegExp | Function and anymatch/sane will automatically handle either case.

Or...

Did I totally miss what you wanted?

cpojer · 2017-03-03T15:08:04Z

The latter, yes! You still need to update the one place inside of jest-haste-map where we call test directly to check for function/regex and do the right thing.

ro-savage · 2017-03-03T16:02:35Z

I spent a while trying to figure this flowtype error out, but no luck. (Never used flow type or any type system before).
ignorePattern: RegExp | Function, for both type Options and type InternalOptions

_ignore is updated

 _ignore(filePath: Path): boolean {
    const ignorePattern = this._options.ignorePattern;
    const ignoreMatched =
      Object.prototype.toString.call(ignorePattern) === '[object RegExp]'
      ? ignorePattern.test(filePath)
      : ignorePattern(filePath);

    return (
      ignoreMatched ||
      (!this._options.retainAllFiles && this._isNodeModulesDir(filePath))
    );
  }

typecheck throws

packages/jest-haste-map/src/index.js:697
697:         : ignorePattern(filePath) ||
               ^^^^^^^^^^^^^^^^^^^^^^^ function call. Callable signature not found in
697:         : ignorePattern(filePath) ||
               ^^^^^^^^^^^^^ RegExp

My guess is this is because RegExp aren't callable in their type definition. But I don't understand why it doesn't use the Function definition but instead only regex. (It should never reach this call if RegExp is used).

cpojer · 2017-03-03T16:21:32Z

Ah ok, for this one, you can do // $FlowFixMe. This is currently a known limitation in Flow, unfortunately.

ro-savage · 2017-03-03T17:12:54Z

Dang. Spent a while trying to figure that one out.

Changes have been made. As usual, let me know any feedback. Or if ready to merge I can rebase into a single commit.

cpojer · 2017-03-03T17:47:26Z

Let's do it. Thanks @ro-savage

SimenB · 2017-03-03T18:14:36Z

packages/jest-haste-map/src/index.js

-    return this._options.ignorePattern.test(filePath) ||
+    const ignorePattern = this._options.ignorePattern;
+    const ignoreMatched =
+      Object.prototype.toString.call(ignorePattern) === '[object RegExp]'


You can do ignorePattern instanceof RegExp, maybe flow understands that?

Yep that worked actually. Will submit another PR to make that change. Thanks for the tip Simen.

* Update to pass ignorepath regex to sane * Changed jest-haste-map to pass modulePathIgnorePatterns * Update jest-haste-map ignorePattern to accept RegExp or Function

…e key (#4063) I plan to revert `ignorePattern` to only being a strict `RegExp`, after removing the function use case from React Native and Metro bundler. The reason for getting rid of the function use case is that `ignorePattern` should be part of the cache key, and a function is not analysable at runtime. If it can only be a `RegExp`, then we can use `ignorePattern.toString()` as part of the cache key. The reason it needs to be part of the cache key is because the cache needs to be discarded—or at least reevaluated—when the ignore pattern changes. Otherwise, we can be missing some new modules that should be included, or the reverse, we can be including modules that should now be ignored. I have observed considerable trouble caused by this issue. For example, in React Native, people would reload a project and it wouldn't find a module, or, duplicates modules would be detected while in fact one of them should have been ignored. This changeset add a deprecation notice for the functional use case (so that we can release this as a minor/revision), and starts using the string representation of the ignore pattern in the cache key (so that we can get a correct behavior as soon as possible for callsites that do already use a `RegExp`). See also #2957, that introduced `ignorePattern` as a function. Alternatively, we could require callsites to provide their own cache key if they do want to use a function, but this makes it more complicated and I'm not sure there's really any other callsites than React Native that does that, that we will fix ourselves as well.

* Update to pass ignorepath regex to sane * Changed jest-haste-map to pass modulePathIgnorePatterns * Update jest-haste-map ignorePattern to accept RegExp or Function

…e key (jestjs#4063) I plan to revert `ignorePattern` to only being a strict `RegExp`, after removing the function use case from React Native and Metro bundler. The reason for getting rid of the function use case is that `ignorePattern` should be part of the cache key, and a function is not analysable at runtime. If it can only be a `RegExp`, then we can use `ignorePattern.toString()` as part of the cache key. The reason it needs to be part of the cache key is because the cache needs to be discarded—or at least reevaluated—when the ignore pattern changes. Otherwise, we can be missing some new modules that should be included, or the reverse, we can be including modules that should now be ignored. I have observed considerable trouble caused by this issue. For example, in React Native, people would reload a project and it wouldn't find a module, or, duplicates modules would be detected while in fact one of them should have been ignored. This changeset add a deprecation notice for the functional use case (so that we can release this as a minor/revision), and starts using the string representation of the ignore pattern in the cache key (so that we can get a correct behavior as soon as possible for callsites that do already use a `RegExp`). See also jestjs#2957, that introduced `ignorePattern` as a function. Alternatively, we could require callsites to provide their own cache key if they do want to use a function, but this makes it more complicated and I'm not sure there's really any other callsites than React Native that does that, that we will fix ourselves as well.

github-actions · 2021-05-13T10:07:02Z

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.

ro-savage changed the title ~~Fix bug with watchers on OS X~~ Fix bug with watchers on OS X crashing testing to crash Feb 21, 2017

facebook-github-bot added the CLA Signed ✔️ label Feb 21, 2017

ro-savage changed the title ~~Fix bug with watchers on OS X crashing testing to crash~~ Fix bug with watchers on macOS causing test to crash Feb 21, 2017

ro-savage mentioned this pull request Feb 21, 2017

Watch mode stopped working on macOS Sierra #1767

Closed

gaearon mentioned this pull request Feb 21, 2017

Update to Jest 19 facebook/create-react-app#1604

Closed

ro-savage added 3 commits March 4, 2017 04:07

Update to pass ignorepath regex to sane

2db71d9

Changed jest-haste-map to pass modulePathIgnorePatterns

6e9cfeb

Update jest-haste-map ignorePattern to accept RegExp or Function

ef28d7b

ro-savage force-pushed the fix-watcher-bug-osx branch from 62ddbe3 to ef28d7b Compare March 3, 2017 17:11

cpojer merged commit b2e1731 into jestjs:master Mar 3, 2017

SimenB reviewed Mar 3, 2017

View reviewed changes

ro-savage mentioned this pull request Mar 3, 2017

jest-haste-map - updated typecheck for regex and remove FlowFixMe #3061

Merged

just-boris mentioned this pull request Apr 5, 2017

Add note about installing watchman facebook/create-react-app#1933

Closed

jeanlauliac mentioned this pull request Jul 18, 2017

jest-haste-map: deprecate functional ignorePattern and use it in cache key #4063

Merged

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

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug with watchers on macOS causing test to crash #2957

Fix bug with watchers on macOS causing test to crash #2957

ro-savage commented Feb 21, 2017 •

edited

Loading

codecov-io commented Feb 21, 2017 •

edited

Loading

cpojer commented Feb 21, 2017

ro-savage commented Feb 21, 2017 •

edited

Loading

cpojer commented Feb 27, 2017

cpojer commented Mar 2, 2017

ro-savage commented Mar 2, 2017 •

edited

Loading

cpojer commented Mar 2, 2017

ro-savage commented Mar 2, 2017 •

edited

Loading

cpojer commented Mar 2, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017 •

edited

Loading

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

SimenB Mar 3, 2017

ro-savage Mar 3, 2017

github-actions bot commented May 13, 2021

Fix bug with watchers on macOS causing test to crash #2957

Fix bug with watchers on macOS causing test to crash #2957

Conversation

ro-savage commented Feb 21, 2017 • edited Loading

Summary

Reproduction

Test plan

Questions

Help wanted

Feedback

codecov-io commented Feb 21, 2017 • edited Loading

Codecov Report

cpojer commented Feb 21, 2017

ro-savage commented Feb 21, 2017 • edited Loading

cpojer commented Feb 27, 2017

cpojer commented Mar 2, 2017

ro-savage commented Mar 2, 2017 • edited Loading

cpojer commented Mar 2, 2017

ro-savage commented Mar 2, 2017 • edited Loading

cpojer commented Mar 2, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017 • edited Loading

cpojer commented Mar 3, 2017

ro-savage commented Mar 3, 2017

cpojer commented Mar 3, 2017

SimenB Mar 3, 2017

Choose a reason for hiding this comment

ro-savage Mar 3, 2017

Choose a reason for hiding this comment

github-actions bot commented May 13, 2021

ro-savage commented Feb 21, 2017 •

edited

Loading

codecov-io commented Feb 21, 2017 •

edited

Loading

ro-savage commented Feb 21, 2017 •

edited

Loading

ro-savage commented Mar 2, 2017 •

edited

Loading

ro-savage commented Mar 2, 2017 •

edited

Loading

ro-savage commented Mar 3, 2017 •

edited

Loading