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 roots on Windows #9549

Closed
wants to merge 2 commits into from
Closed

Fix roots on Windows #9549

wants to merge 2 commits into from

Conversation

KuroyukihimeSenpaiSamaChan
Copy link

@KuroyukihimeSenpaiSamaChan KuroyukihimeSenpaiSamaChan commented Feb 10, 2020

I noticed that on Windows, if you had one bad route in your jest.config.js, it would return 0 route found.
It looked like a bug to me, because this was not what happened on Linux, and there were no error message directly saying a root was wrong.
This simple fix ensure that all roots are checked and registered in the result array before returning anything to callback. What was happening is that the calls being asynchronous and the error check being at the beginning, when it encountered a bad root, it would return the callback and ignore the other roots, which could be ok.

Summary

Test plan

I noticed that on Windows, if you had one bad route in your jest.config.js, it would return 0 route found.
It looked like a bug to me, because this was not what happened on Linux, and there was no error message directly saying a route was wrong.
This simple fix ensure that all routes are checked and registered in the result array before returning anything to callback. What was happening is that the calls being asynchronous and the error check being at the beginning, when it encountered a bad route, it would return the callback and ignore the other route, wich could be ok.
@KuroyukihimeSenpaiSamaChan KuroyukihimeSenpaiSamaChan changed the title Fix routes on Windows Fix roots on Windows Feb 10, 2020
@codecov-io
Copy link

codecov-io commented Feb 10, 2020

Codecov Report

Merging #9549 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9549      +/-   ##
==========================================
- Coverage   65.05%   65.03%   -0.03%     
==========================================
  Files         286      286              
  Lines       12123    12124       +1     
  Branches     3004     3005       +1     
==========================================
- Hits         7887     7885       -2     
- Misses       3599     3600       +1     
- Partials      637      639       +2
Impacted Files Coverage Δ
packages/jest-haste-map/src/crawlers/node.ts 84.52% <50%> (-1.02%) ⬇️
packages/expect/src/utils.ts 94.96% <0%> (-1.26%) ⬇️

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 017264f...fedf75a. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Feb 10, 2020

Seems odd to me that we just ignore errors... This fix seems correct though.

@cpojer thoughts?

@cpojer
Copy link
Member

cpojer commented Feb 10, 2020

I think this could benefit from tests.

@KuroyukihimeSenpaiSamaChan
Copy link
Author

To reproduce the case:

  • In a node project, install jest
  • Add the jest.config.js with correct roots and one root pointing to a non-existing folder
  • Run jest
    • Windows : Says 0 roots found
    • Linux : Find only the existing roots

@KuroyukihimeSenpaiSamaChan
Copy link
Author

Actually, I'm sorry. If you want tests, I don't know how to do them for this type of code, I can only tell you of to reproduce the error, like I did earlier.

@SimenB
Copy link
Member

SimenB commented Feb 12, 2020

Looked into this, and if you provide a non-existent root the behaviour varies quite a bit. With watchman we error with jest-haste-map: Watchman crawl failed. Retrying once with node crawler. and then the node crawler silently ignores the missing directory. However, if we're running on windows it reports 0 regardless of whether there are tests in other directories. This is due to this logic: https://github.com/facebook/jest/blob/00e9e0972bf959e041b1dd249d907402a58c23b5/packages/jest-haste-map/src/crawlers/node.ts#L196-L200

I think all of these behaviors except for watchman's are bugs (and since it fallbacks to a bug, even that behaviour is wrong). IMO the correct solution is to validate that all directories provided as roots exist when we do config normalization, meaning we'd error out way earlier than when we crawl.

I also think we should error in jest-haste-map, but I'm not sure if Metro won't barf if we start doing that?

@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 11, 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.

5 participants