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

Add path case-insensitivity if onlyChanged option is active, fixes #4644 #4730

Merged
merged 10 commits into from
Nov 17, 2017
Merged

Add path case-insensitivity if onlyChanged option is active, fixes #4644 #4730

merged 10 commits into from
Nov 17, 2017

Conversation

peterdanis
Copy link
Contributor

@peterdanis peterdanis commented Oct 19, 2017

Summary
This PR fixes #4644. On Windows process.cwd and git rev-parse --show-toplevel outputs can diverge in the same directory:
image

This is causing Jest run in onlyChanged mode to find no suitable tests.

Test plan
After changing the exists function and _rootPattern variable to case-insensitive regex (on "win32" platform only), jest -o runs 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).

image

@cpojer
Copy link
Member

cpojer commented Oct 19, 2017

Thanks for sending a PR. What if people use windows with a case sensitive file system? This would break for them, no?

@SimenB
Copy link
Member

SimenB commented Oct 19, 2017

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Needs a test.

Also, please update the changelog

@SimenB
Copy link
Member

SimenB commented Oct 19, 2017

Is it possible to check for case sensitivity?

https://superuser.com/a/1026536 or something in the startup

@peterdanis
Copy link
Contributor Author

peterdanis commented Oct 20, 2017

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

If this solution is ok for you, maybe we can discuss about doing the same if --projects option is defined (only on Windows, I think that Linux/MacOS users are used to type the correct case paths)

For reference:
nodejs/node#15776
nodejs/node#8715

@@ -142,7 +142,11 @@ const getProjectListFromCLIArgs = (argv, project: ?Path) => {
}

if (!projects.length) {
projects.push(process.cwd());
if (process.platform === 'win32') {
projects.push(process.binding('fs').realpath(process.cwd()));
Copy link
Member

Choose a reason for hiding this comment

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

why not fs.realpathSync()? process.binding seems overkill

Copy link
Contributor Author

@peterdanis peterdanis Oct 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Huh, that's weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@SimenB
Copy link
Member

SimenB commented Oct 21, 2017

I think the change here makes sense. @cpojer thoughts?

This still needs a test, though 🙂

@peterdanis
Copy link
Contributor Author

Test added and changelog updated. Should I squash the commits together?

@SimenB
Copy link
Member

SimenB commented Oct 31, 2017

We squash on merge, so multiple commits are easier to review. Thanks!

@@ -82,7 +82,12 @@ const cleanup = (directory: string) => rimraf.sync(directory);
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']
Copy link
Member

Choose a reason for hiding this comment

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

can we do const filePath = fileOrPath.split(path.posix.sep); here? (or just fileOrPath.split('/'))

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 for suggestion. Changed to const filePath = fileOrPath.split('/')

@SimenB
Copy link
Member

SimenB commented Oct 31, 2017

@peterdanis this fails flow, run yarn flow locally to see (needs // $FlowFixMe)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Would love to actually see a green appveyor. Can we clear the queue manually? The site does not cooperate with mobile

@SimenB
Copy link
Member

SimenB commented Oct 31, 2017

@cpojer @Daniel15 I'm not allowed to do anything on Appveyor, can you try cancelling some builds or contacting support about the endless queue?

@cpojer cpojer merged commit 84fe06a into jestjs:master Nov 17, 2017
@Daniel15
Copy link
Contributor

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

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

Windows - path case sensitivity
5 participants