Add path case-insensitivity if onlyChanged option is active, fixes #4644#4730
Add path case-insensitivity if onlyChanged option is active, fixes #4644#4730cpojer merged 10 commits intojestjs:masterfrom peterdanis:dev
Conversation
|
Thanks for sending a PR. What if people use windows with a case sensitive file system? This would break for them, no? |
|
Or macOS with case sensitivity turned on, is it an issue there as well? That said, this is definitely a bug fix, and we have other breaking changes in master already, so next release should be a major anyways |
SimenB
left a comment
SimenB
left a comment
There was a problem hiding this comment.
Needs a test.
Also, please update the changelog
|
Is it possible to check for case sensitivity? https://superuser.com/a/1026536 or something in the startup |
|
Thanks for your feedback. I agree to you, this should be handled in a more complex way. I have reverted the original changes. Instead changed how If this solution is ok for you, maybe we can discuss about doing the same if For reference: |
| if (!projects.length) { | ||
| projects.push(process.cwd()); | ||
| if (process.platform === 'win32') { | ||
| projects.push(process.binding('fs').realpath(process.cwd())); |
There was a problem hiding this comment.
why not fs.realpathSync()? process.binding seems overkill
There was a problem hiding this comment.
fs.realpathSync() does not correct the path.
They differ:
fs.realpathSync(process.cwd()):
C:\Users\pdanis\desktop\projects\my-js-learning\other\temp
(edit: corrected \\projects to \projects)
process.binding('fs').realpath(process.cwd()):
C:\Users\pdanis\Desktop\Projects\my-js-learning\other\temp
There was a problem hiding this comment.
If I understand it correctly fs.realpathSync() uses JS fs and process.binding('fs').realpath() is using GetFinalPathNameByHandle() on Windows.
There is a PR, which should expose process.binding('fs').realpath() as fs.realpathSync.native().
nodejs/node#15776
|
I think the change here makes sense. @cpojer thoughts? This still needs a test, though 🙂 |
|
Test added and changelog updated. Should I squash the commits together? |
|
We squash on merge, so multiple commits are easier to review. Thanks! |
| const writeFiles = (directory: string, files: {[filename: string]: string}) => { | ||
| mkdirp.sync(directory); | ||
| Object.keys(files).forEach(fileOrPath => { | ||
| const filePath = fileOrPath.split(path.sep); // ['tmp', 'a.js'] |
There was a problem hiding this comment.
can we do const filePath = fileOrPath.split(path.posix.sep); here? (or just fileOrPath.split('/'))
There was a problem hiding this comment.
Thanks for suggestion. Changed to const filePath = fileOrPath.split('/')
|
@peterdanis this fails flow, run |
SimenB
left a comment
SimenB
left a comment
There was a problem hiding this comment.
Would love to actually see a green appveyor. Can we clear the queue manually? The site does not cooperate with mobile
|
The AppVeyor issues should be resolved, @cpojer reached out to me about it two weeks ago. I need to figure out how to move the Jest project to the Facebook org on AppVeyor. https://help.appveyor.com/discussions/problems/9507-large-number-of-builds-are-queued |
|
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. |
5 participants
Summary
This PR fixes #4644. On Windows
process.cwdandgit rev-parse --show-topleveloutputs can diverge in the same directory:This is causing Jest run in
onlyChangedmode to find no suitable tests.Test plan
After changing theexistsfunction and_rootPatternvariable to case-insensitive regex (on "win32" platform only),jest -oruns fine:I have reverted the original changes. Instead changed how projects array is constructed if no --projects option is defined. I think it is only affecting Windows, because on MacOS and Linux process.cwd() should return the correct case (see the first comment in nodejs/node#8237).