test: use normalize() for unicode paths - #3007
Conversation
|
CI: https://ci.nodejs.org/job/node-test-pull-request/362/ (Let's hope CI builds have ICU enabled) |
|
@silverwind they have small ICU enabled, because that's the default since v3.1.0. See https://github.com/nodejs/node/blob/master/test/parallel/test-intl.js#L13 for an example of testing whether ICU was enabled. |
|
@srl295 thanks. Can one assume that both the |
|
Updated the test so it works on non-ICU builds too by using characters that can't be decomposed (emoji) when ICU is not present. Another CI: https://ci.nodejs.org/job/node-test-pull-request/365/ |
|
@silverwind no.. you can't really assume it. but see Root is just the "base" locale ID. It should have been named |
|
I will rework this once #3089 is landed. |
|
Using |
|
LGTM |
|
There's a slight issue here: the test change depends on cc: @nodejs/release |
|
It won't fail dramatically in 4.1.x thought, it just won't actually test |
|
might leave it out of 4.1.2 eh? don't want the commit suggesting to people that it's actually doing something |
|
Alright, will wait after 4.1.2 with landing this. |
|
LGTM if CI is happy and others sign-off. |
|
One more CI with the latest change: https://ci.nodejs.org/job/node-test-pull-request/418/ |
|
Change LGTM, hope it fixes the failure. |
OS X 10.11 changed the unicode normalization form of certain code points returned by system calls like getcwd() from NFC to NFD which made results in this test failing. The consensus of #2165 is to delegate the task of unicode normalization to the user, and work will continue to document how to handle unicode in a form-sensitive file system. PR-URL: #3007 Fixes: #2165 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
dfd7a13 to
81e98e9
Compare
|
Landed in 81e98e9, thanks! |
|
@silverwind this requires ICU right? Does this test still work properly without it? Fwiw, the CI compiles without ICU for test runs. |
|
@Fishrock123 it works in non-ICU builds too, where |
|
Also, |
OS X 10.11 changed the unicode normalization form of certain code points returned by system calls like getcwd() from NFC to NFD which made results in this test failing. The consensus of #2165 is to delegate the task of unicode normalization to the user, and work will continue to document how to handle unicode in a form-sensitive file system. PR-URL: #3007 Fixes: #2165 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Went ahead and changed the failing test to use
String.prototype.normalizeso path strings are compared form-independently.@srl295 The test will still fail on debug builds that don't include ICU. Is there a way I can feature-test if the current build includes ICU? Maybe we should add
process.versions.icu?Related: #2165
Docs Issue: nodejs/docs#42