test: use normalize() for unicode paths

[silverwind]

Went ahead and changed the failing test to use String.prototype.normalize so 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

[silverwind]

CI: https://ci.nodejs.org/job/node-test-pull-request/362/

(Let's hope CI builds have ICU enabled)

[srl295]

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

[silverwind]

@srl295 thanks. Can one assume that both the root and en locales will always be present when built with ICU? What's the root locale exactly, by the way?

[silverwind]

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/

[srl295]

@silverwind no.. you can't really assume it. but see process.config.variables.icu_locales

 > process.config.variables.icu_locales.split(',')
 [ 'en', 'root' ]

Root is just the "base" locale ID. It should have been named und if iso-639-2 had been around way back when. It's not English, but its collation is (close to) UCA, and it contains items that are common to other locales.

[silverwind]

I will rework this once #3089 is landed.

[silverwind]

Using process.version.icu now. Any feedback?

[jbergstroem]

LGTM

[silverwind]

There's a slight issue here: the test change depends on process.versions.icu which will land in 4.2, while this test change could go in earlier in 4.1.x. Won't be an issue on master thought.

cc: @nodejs/release

[silverwind]

It won't fail dramatically in 4.1.x thought, it just won't actually test .normalize, even if Intl is there, so might be a non-issue.

[rvagg]

might leave it out of 4.1.2 eh? don't want the commit suggesting to people that it's actually doing something

[silverwind]

Alright, will wait after 4.1.2 with landing this.

[Fishrock123]

LGTM if CI is happy and others sign-off.

[silverwind]

One more CI with the latest change: https://ci.nodejs.org/job/node-test-pull-request/418/

[thefourtheye]

Change LGTM, hope it fixes the failure.

[silverwind]

Landed in 81e98e9, thanks!

[Fishrock123]

@silverwind this requires ICU right? Does this test still work properly without it?

Fwiw, the CI compiles without ICU for test runs.

[silverwind]

@Fishrock123 it works in non-ICU builds too, where process.version.icu will be undefined, and in which case we write an emoji-filename which can't be decomposed 😉.

[silverwind]

Also, .normalize() returns the string as-is in non-ICU builds, if you're wondering.