test: make realpath tests case-insensitive on win32

[rvagg]

Just ran in to this on Windows when running manually.

process.cwd(), which is used in lots of the tests, may return a different value than what realpath will return even though they are the same path when compared case-insensitive.

[bnoordhuis]

The change LGTM but it would be good to get confirmation from someone like @piscisaureus or @seishun on whether this is a bug or merely a bad assumption in the test.

There's a small but persistent typo in the commit log: s/case-insensntive/case-insensitive/

[rvagg]

typo fixed, thanks for picking that up!

[seishun]

With this change realpath will pass the tests even if it gives you "c:\PROGRAM files\nodeJS" instead of "C:\Program Files\nodejs". While the two paths are equivalent in Windows, only the latter is displayed in cmd etc and thus should be expected from a function that returns a "canonicalized pathname".

I believe the real problem here is that realpath currently gives you lowercase drive letters. We should fix that, not the tests.

[seishun]

It turns out the rabbit hole goes quite deep. Here's how it went:

18 Mar 2013: nodejs/node-v0.x-archive@a05f973 changes capitalization of drive letter in path.normalize for reasons no one knows
14 Oct 2014: @piscisaureus submits node-forward/node#20 due to issues the aforementioned patch has caused
20 Oct 2014: nodejs/node-v0.x-archive@f6e5740 changes path.resolve to be consistent with path.normalize, which changes the behavior of realpath

So now we have two problems. Should we just revert both commits?

[rvagg]

Alternatively I could just change my commit so that it only does a toLowerCase() on the drive letter for the tests. My main concern here is the tests not matching the implementation and getting too involved in discussing the functionality it's testing is going to get further away from that and perhaps we should just open a new issue to tighten the spec.

[seishun]

No, the drive letter should be uppercase. The test is fine.

My main concern here is the tests not matching the implementation

Isn't that when you fix the implementation? :)

perhaps we should just open a new issue to tighten the spec

Maybe, but to me the behavior "give the path that Windows displays" seems obvious. In any case, I see no downside in guaranteeing that the drive letter will be uppercase. It's way better than forcing the user to call toLowerCase on everything.

[rvagg]

@seishun would you mind putting in a pull request to change the implementation along with a new test case specifically for it, then we can discuss further over here. I was just taking the short path but am happy to defer if there is a strong enough opinion on this from someone else.

[piscisaureus]

+1 on reverting both.

Verzonden van mijn HTC

----- Reply message -----
Van: "Nikolai Vavilov" [email protected]
Aan: "iojs/io.js" [email protected]
CC: "Bert Belder" [email protected]
Onderwerp: [io.js] test: make realpath tests case-insensitive on win32 (#17)
Datum: za, dec. 6, 2014 00:07

It turns out the rabbit hole goes quite deep. Here's how it went:

18 Mar 2013: nodejs/node-v0.x-archive@a05f973nodejs/node-v0.x-archive@a05f973 changes capitalization of drive letter in path.normalize for reasons no one knows
14 Oct 2014: @piscisaureushttps://github.com/piscisaureus submits node-forward/node#20https://github.com/node-forward/node/issues/20 due to issues the aforementioned patch has caused
20 Oct 2014: nodejs/node-v0.x-archive@f6e5740nodejs/node-v0.x-archive@f6e5740 changes path.resolve to be consistent with path.normalize, which changes the behavior of realpath

So now we have two problems. Should we just revert both commits?

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-65869241.

[piscisaureus]

Clearly I disagree that the drive letter should be uppercase. Instead I prefer not to change it at all, just like we don't enforce a particular casing on any other path component.

It would be kind of nice if realpath() could return the actual on-disk case that has been stored on a case preserving filesystem. However that's not possible on Windows afaik and I am not sure about OS X either.

[seishun]

I just played around with git revert a bit. While f6e5740 reverts almost cleanly with just a minor conflict, a05f973, being 1.5 years old, confuses the hell out of git (and it also introduced other changes besides the "normalization"). So should I...

1. Fix everything manually in a single commit, or
2. Git-revert f6e5740 and "fix" a05f973 manually, or
3. Just revert f6e5740, since that's what breaks realpath, and leave the rest to node-forward/node#20?

Clearly I disagree that the drive letter should be uppercase. Instead I prefer not to change it at all, just like we don't enforce a particular casing on any other path component.

Can the drive letter even be lowercase on Windows? Anyway, path.resolve uses process.cwd() internally which should just give you whatever Windows thinks the actual path is.

[piscisaureus]

Fix everything manually in a single commit, or
Git-revert f6e5740 and "fix" a05f973 manually, or
Just revert f6e5740, since that's what breaks realpath, and leave the rest to node-forward/node#20?

Whatever is easiest.

Can the drive letter even be lowercase on Windows? Anyway, path.resolve uses process.cwd() internally which should just give you whatever Windows thinks the actual path is.

Certainly! Windows couldn't care less.
The fact that cmd.exe displays the drive letter as upper case is just a matter of convention.
Path.resolve uses process.cwd() or an hidden environment variable, depending on the situation. The latter may produce lower case drive letters.

[rvagg]

needs to be closed if #100 gets merged

[bnoordhuis]

This has been addressed by e24fa83, if I'm not mistaken.