| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
/ping @bnoordhuis who originally added the test. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
Maybe we can keep the test if we detect the file system type with diskutil info / in the test? |
Sorry, something went wrong.
|
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? Detailsdiff --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);
})); |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
It would be good to keep the assert.ifError since it provides a much better error message in case of an error.
Sorry, something went wrong.
Sorry, something went wrong.
|
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)
... |
Sorry, something went wrong.
|
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');
|
Sorry, something went wrong.
|
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()? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
this still needs to be fixed on windows and i still don't have a windows machine |
Sorry, something went wrong.
|
@devsnek Would it help if you had Remote Desktop access to a Windows machine? Or would you still likely need someone else to assist? |
Sorry, something went wrong.
|
Added in progress label and removed author ready so that no one tries to land this before the Windows issue is sorted. |
Sorry, something went wrong.
|
@Trott is there another way to test realpath besides case sensitivity stuff? will it resolve symlinks? relative paths? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
/ping @nodejs/fs |
Sorry, something went wrong.
|
@nodejs/fs again |
Sorry, something went wrong.
|
@nodejs/platform-windows maybe? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
CI https://ci.nodejs.org/job/node-test-pull-request/15923/ OSX Rebuild: https://ci.nodejs.org/job/node-test-commit-osx/19888/ |
Sorry, something went wrong.
|
all tests passing... landing tomorrow morning |
Sorry, something went wrong.
PR-URL: nodejs#20954 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis FWIW I used a case sensitive FS on macOS for the last two years without running into any issues. |
Sorry, something went wrong.
PR-URL: #20954 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
| Back | FazBrowse Home | New Git URL |
If you're on a mac with a case sensitive filesystem this test fails.
Checklist