Skip to content
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

fix: stop search if files names are undefined #6761

Merged
merged 6 commits into from
Aug 30, 2018

Conversation

emuraton
Copy link
Contributor

@emuraton emuraton commented Jul 26, 2018

Summary

Files names seems to be undefined on certain cases for Window

      names.forEach(file => {
            ^

TypeError: Cannot read property 'forEach' of undefined
    at fs.readdir (C:\Users\timbault\talend-ui\ui\node_modules\jest-haste-map\build\crawlers\node.js:36:13)
    at go$readdir$cb (C:\Users\timbault\talend-ui\ui\node_modules\graceful-fs\graceful-fs.js:149:14)
    at FSReqWrap.oncomplete (fs.js:135:15)
error Command failed with exit code 1.

I'm not sure why this variable is undefined, but I added defensive code to return earlier more gracefuly.

Test plan

Added an unit test to confirm we return early if names is undefined.

fixes: #6715

@emuraton emuraton changed the title fix: stop search if files names are undefined (WIP) fix: stop search if files names are undefined Jul 26, 2018
@emuraton emuraton force-pushed the crawlers-error-window branch from a432e4b to d56dcf3 Compare July 26, 2018 09:41
@codecov-io
Copy link

codecov-io commented Jul 26, 2018

Codecov Report

Merging #6761 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6761      +/-   ##
==========================================
- Coverage   66.98%   66.98%   -0.01%     
==========================================
  Files         250      250              
  Lines       10360    10363       +3     
  Branches        3        3              
==========================================
+ Hits         6940     6942       +2     
- Misses       3419     3420       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-haste-map/src/crawlers/node.js 98.78% <ø> (-1.22%) ⬇️

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 0b1e8c3...edcfe78. Read the comment docs.

@emuraton emuraton changed the title (WIP) fix: stop search if files names are undefined fix: stop search if files names are undefined Jul 29, 2018
@@ -30,7 +30,10 @@ function find(
activeCalls++;
fs.readdir(directory, (err, names) => {
activeCalls--;

if (!names) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about going a bit more idiomatic Node.js with if (err) {?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if that's what happens (unhandled error) that's the correct fix. @emuraton do you have a reproduction case for your error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB I did not suceed to replicate the issue case of @timbault.
But from the stack trace, callback doesnt return an error @thymikee, just names undefined.
I can add both check if you think its better (if( err || !names))

Doest it make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it returns undefined names, but null err, it's a bug in Node imho and you should also file it there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, the docs says you should always get an array: https://nodejs.org/api/fs.html#fs_fs_readdir_path_options_callback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late answer, after double thought I think you were right before actually. Checking err should be enough.
If I find a bit of time today, I'll try to replicate to see if its a bug from Node or if your first comments were right

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looking forward to it!

@emuraton
Copy link
Contributor Author

emuraton commented Aug 24, 2018

After few tests cases, I always got something following this shape. So I think checking err, as you suggested, should be enough.

      { Error: ENOTDIR: not a directory, scandir '/Users/emilienmuraton/Code/jest/packages/jest-haste-map/src/__tests__/index.test.js'
        errno: -20,
        code: 'ENOTDIR',
        syscall: 'scandir',
        path: '/Users/emilienmuraton/Code/jest/packages/jest-haste-map/src/__tests__/index.test.js' }
      }
  { names: undefined }

@emuraton emuraton force-pushed the crawlers-error-window branch from a1796df to 0f79b06 Compare August 24, 2018 15:01
@SimenB
Copy link
Member

SimenB commented Aug 27, 2018

Could you update the changelog as well? :)

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@
- `[babel-jest]` Make `getCacheKey()` take into account `createTransformer` options ([#6699](https://github.com/facebook/jest/pull/6699))
- `[docs]` Fix contributors link ([#6711](https://github.com/facebook/jest/pull/6711))
- `[website]` Fix website versions page to link to correct language ([#6734](https://github.com/facebook/jest/pull/6734))
- `[jest-haste-map`] Catch crawler error when unsuccessfully reading directories ([#6761](https://github.com/facebook/jest/pull/6761))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to rebase/merge master, as this CHANGELOG is outdated (latest Jest release is 23.5.0)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, fixed that for you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jest-haste-map crawlers error when trying to run tests on Windows
5 participants