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

test: fix fs realpath.native test #20954

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented May 25, 2018

If you're on a mac with a case sensitive filesystem this test fails.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 25, 2018
@devsnek devsnek requested a review from bnoordhuis May 25, 2018 00:34
@Trott
Copy link
Member

Trott commented May 26, 2018

/ping @bnoordhuis who originally added the test.

@BridgeAR
Copy link
Member

@bnoordhuis PTAL

@bnoordhuis
Copy link
Member

Just curious, what kind of Mac is that? The test should work assuming you're using HFS+ in its default case-preserving mode, unless you deleted /Users and recreated it as /users (and who does that anyway?)

Losing the test would be a bit of a shame because it's the only coverage for this specific condition that we have.

@devsnek
Copy link
Member Author

devsnek commented May 28, 2018

@bnoordhuis i just updated my mac to sierra and while doing so decided it was about time i switched on case sensitivity (which has been great except for steam not working and this bug)

anyone on HFS+ (Case-sensitive) and APFS (Case-Sensitive) filesystems will experience this.

would this test also work with symlinks? i'd be happy to rewrite it.

@joyeecheung
Copy link
Member

Maybe we can keep the test if we detect the file system type with diskutil info / in the test?

@bnoordhuis
Copy link
Member

bnoordhuis commented May 28, 2018

I suspect you're in for a lot of pain, a lot of mac apps don't deal with case sensitivity well. That aside, does this patch work for you?

diff --git a/test/parallel/test-fs-realpath-native.js b/test/parallel/test-fs-realpath-native.js
index 93b5a278cf..4e9987fa8e 100644
--- a/test/parallel/test-fs-realpath-native.js
+++ b/test/parallel/test-fs-realpath-native.js
@@ -1,13 +1,18 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
 const fs = require('fs');
 
-if (!common.isOSX) common.skip('MacOS-only test.');
+const filename = __filename.toUpperCase();
+try {
+  fs.accessSync(filename);
+} catch {
+  common.skip('not a case-insensitive fs');
+}
 
-assert.strictEqual(fs.realpathSync.native('/users'), '/Users');
-fs.realpath.native('/users', common.mustCall(function(err, res) {
+assert.strictEqual(fs.realpathSync.native(filename), __filename);
+fs.realpath.native(filename, common.mustCall(function(err, res) {
   assert.ifError(err);
-  assert.strictEqual(res, '/Users');
+  assert.strictEqual(res, __filename);
   assert.strictEqual(this, undefined);
 }));

@devsnek devsnek force-pushed the fix/osx-sometimes-has-case-sensitive-filesystems branch from 47dd96b to b1b596d Compare May 29, 2018 02:45
@devsnek
Copy link
Member Author

devsnek commented May 29, 2018

assert.ifError(err);
assert.strictEqual(res, '/Users');
fs.realpath.native(filename, common.mustCall(function(err, res) {
assert.strictEqual(res, __filename);
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to keep the assert.ifError since it provides a much better error message in case of an error.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 29, 2018
@devsnek devsnek force-pushed the fix/osx-sometimes-has-case-sensitive-filesystems branch from b1b596d to 792e7cf Compare June 13, 2018 02:28
@devsnek
Copy link
Member Author

devsnek commented Jun 18, 2018

@Trott
Copy link
Member

Trott commented Jun 18, 2018

Windows failures sure seem relevant:

not ok 158 parallel/test-fs-realpath-native
  ---
  duration_ms: 0.133
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:80
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 'C:\\workspace\\node-test-binary-windows\\test\\parallel\\test-fs-realpath-native.js'
    + 'c:\\workspace\\node-test-binary-windows\\test\\parallel\\test-fs-realpath-native.js'
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-realpath-native.js:13:8)
        at Module._compile (internal/modules/cjs/loader.js:702:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
        at Module.load (internal/modules/cjs/loader.js:612:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
        at Function.Module._load (internal/modules/cjs/loader.js:543:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
        at startup (internal/bootstrap/node.js:241:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:565:3)
  ...

@bnoordhuis
Copy link
Member

Ah, Windows... I guess that could be worked around by comparing the drive volume case-insensitively.

Aside, the test won't work right as long as it still has this in it:

assert.strictEqual(fs.realpathSync.native('/users'), '/Users');

@Trott
Copy link
Member

Trott commented Jun 19, 2018

Since the test was previously skipped everywhere but macOS, it would still be an improvement if it was skipped only on Windows now. So that could be a simple workaround here.

On the other hand, since this really tests underlying OS functionality more than Node.js functionality, I wonder if it would be acceptable to get rid of the string comparisons entirely and just have it resolve . and .. type stuff.

Or (a bit more brittle but arguably more thorough), have it monkey-patch process.binding('fs').realpath and have the monkey-patched function confirm that it receives the expected arguments from the calls to fs.realpathSync.native() and fs.realpath.native()?

@devsnek
Copy link
Member Author

devsnek commented Jun 21, 2018

i don't have a windows machine and i don't know anything about the quirks of window's filesystem so someone who can actually test that will need to get me a patch for it.

on the other hand it seems like its really fiddly to try and detect the proper operation of case sensitive filesystems, is there another way we can test realpath? it must have some use besides resolving case.

@devsnek devsnek force-pushed the fix/osx-sometimes-has-case-sensitive-filesystems branch from 792e7cf to d57049c Compare June 21, 2018 22:41
@apapirovski
Copy link
Member

@devsnek
Copy link
Member Author

devsnek commented Jun 27, 2018

this still needs to be fixed on windows and i still don't have a windows machine

@Trott
Copy link
Member

Trott commented Jun 28, 2018

@devsnek Would it help if you had Remote Desktop access to a Windows machine? Or would you still likely need someone else to assist?

@Trott Trott added wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 29, 2018
@Trott
Copy link
Member

Trott commented Jun 29, 2018

Added in progress label and removed author ready so that no one tries to land this before the Windows issue is sorted.

@devsnek
Copy link
Member Author

devsnek commented Jul 1, 2018

@Trott is there another way to test realpath besides case sensitivity stuff? will it resolve symlinks? relative paths?

@Trott
Copy link
Member

Trott commented Jul 1, 2018

@Trott is there another way to test realpath besides case sensitivity stuff? will it resolve symlinks? relative paths?

http://man7.org/linux/man-pages/man3/realpath.3.html

Resolves symlinks, ., .., extra /.

I haven't tried to figure out why the test was added in the first place, but if removing it is not an option, maybe we can alter it to confirm that fs.realpathSync.native() invokes process.binding('fs').realpath(), confirm that there's no error, and that it gets back something that resembles a path, but don't worry too much about the details beyond that.

@devsnek devsnek force-pushed the fix/osx-sometimes-has-case-sensitive-filesystems branch from d57049c to fc70224 Compare July 1, 2018 04:01
@Trott
Copy link
Member

Trott commented Jul 9, 2018

/ping @nodejs/fs

@devsnek
Copy link
Member Author

devsnek commented Jul 13, 2018

@nodejs/fs again

@devsnek devsnek changed the title test: remove faulty fs test test: fix fs realpath.native test Jul 13, 2018
@Trott
Copy link
Member

Trott commented Jul 13, 2018

@nodejs/platform-windows maybe?

@bzoz
Copy link
Contributor

bzoz commented Jul 14, 2018

On Windows the __filename will be equal to the argv[1]. Whatever filename capitalization test runner uses to spawn the test, it will be used. IMHO the test comparison should be made case insensitive on Windows.

@devsnek
Copy link
Member Author

devsnek commented Jul 18, 2018

@devsnek
Copy link
Member Author

devsnek commented Jul 18, 2018

all tests passing... landing tomorrow morning

@devsnek devsnek added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. and removed wip Issues and PRs that are still a work in progress. labels Jul 18, 2018
@devsnek devsnek force-pushed the fix/osx-sometimes-has-case-sensitive-filesystems branch from ddfa979 to bacfa87 Compare July 18, 2018 15:46
PR-URL: nodejs#20954
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@devsnek devsnek force-pushed the fix/osx-sometimes-has-case-sensitive-filesystems branch from bacfa87 to 9cd932f Compare July 18, 2018 15:48
@devsnek devsnek closed this Jul 18, 2018
@devsnek devsnek deleted the fix/osx-sometimes-has-case-sensitive-filesystems branch July 18, 2018 15:48
@devsnek devsnek merged commit 9cd932f into nodejs:master Jul 18, 2018
@devsnek
Copy link
Member Author

devsnek commented Jul 18, 2018

landed in 9cd932f

@devsnek devsnek restored the fix/osx-sometimes-has-case-sensitive-filesystems branch July 18, 2018 15:48
@devsnek devsnek deleted the fix/osx-sometimes-has-case-sensitive-filesystems branch July 18, 2018 15:49
@gibfahn
Copy link
Member

gibfahn commented Jul 19, 2018

I suspect you're in for a lot of pain, a lot of mac apps don't deal with case sensitivity well. That aside, does this patch work for you?

@bnoordhuis FWIW I used a case sensitive FS on macOS for the last two years without running into any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.