src: fix recent --icu-data-dir= regression

[bnoordhuis]

Commit a8734af ("src: make copies of startup environment variables")
introduced a regression in the capturing of the --icu-data-dir= switch.

This commit fixes that and shuffles code around so it can be properly
unit-tested.

cc @sam-github @srl295

[richardlau]

This flag is invalid if Node.js is compiled without intl. and will probably cause the test runner to fail to execute the test (as opposed to gracefully skipping over it).

[bnoordhuis]

I guess you're right but see nodejs/build#419. Since we don't test non-intl builds I feel somewhat ambivalent about complicating the test. I'll update it if you feel strongly about it.

[richardlau]

Fair enough. I did a quick test and other tests are already failing without intl (see comment in main PR) so I'm okay with keeping the test as it is. If someone really does care about having clean runs with non-intl builds we can address in another PR.

[richardlau]

This flag is invalid if Node.js is compiled without intl. and will probably cause the test runner to fail to execute the test (as opposed to gracefully skipping over it).

I guess you're right but see nodejs/build#419. Since we don't test non-intl builds I feel somewhat ambivalent about complicating the test. I'll update it if you feel strongly about it.

Just ran a quick build/test with configure --without-intl and other tests fail so I'm +0 about updating the new test to work without intl.

For the record, these tests fail with configure --without-intl (only test-intl-no-icu-data is relevant to this PR):

-bash-4.2$ ./tools/test.py parallel
=== release test-cluster-inspector-debug-port ===
Path: parallel/test-cluster-inspector-debug-port
Inspector support is not available with this Node.js build
out/Release/node: bad option: --inspect=12346
Command: out/Release/node --inspect=12346 /home/users/riclau/sandbox/github/node/test/parallel/test-cluster-inspector-debug-port.js
=== release test-intl-no-icu-data ===
Path: parallel/test-intl-no-icu-data
out/Release/node: bad option: --icu-data-dir=test/fixtures/empty/
Command: out/Release/node --icu-data-dir=test/fixtures/empty/ /home/users/riclau/sandbox/github/node/test/parallel/test-intl-no-icu-data.js
=== release test-url-domain-ascii-unicode ===
Path: parallel/test-url-domain-ascii-unicode
assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'ıídÙÙ´' === 'xn--d-iga7ro0q9f'
    at domainWithASCII.forEach (/home/users/riclau/sandbox/github/node/test/parallel/test-url-domain-ascii-unicode.js:24:3)
    at Array.forEach (native)
    at Object.<anonymous> (/home/users/riclau/sandbox/github/node/test/parallel/test-url-domain-ascii-unicode.js:20:17)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:418:7)
Command: out/Release/node /home/users/riclau/sandbox/github/node/test/parallel/test-url-domain-ascii-unicode.js
=== release test-url-format-whatwg ===
Path: parallel/test-url-format-whatwg
assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'http://xn--lck1c3crb1723bpq4a.com/a?a=b#c' === 'http://ç容ãã«ã ã©.com/a?a=b#c'
    at Object.<anonymous> (/home/users/riclau/sandbox/github/node/test/parallel/test-url-format-whatwg.js:79:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:418:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:533:3
Command: out/Release/node /home/users/riclau/sandbox/github/node/test/parallel/test-url-format-whatwg.js
[03:56|% 100|+ 1259|-   4]: Done
-bash-4.2$

[sam-github]

LGTM assuming that the change to the config binding property name is entirely internal, and not an API change.

[bnoordhuis]

The binding was introduced to avoid leaking implementation details that then become de facto API, so yes, we should be good on that front. :-)

[bnoordhuis]

I decided to split the change into two commits so that the actual bug fix isn't buried so much in the churn. New CI: https://ci.nodejs.org/job/node-test-pull-request/6353/

[italoacasas]

@bnoordhuis the commits are not landing in v7.x clearly, any plan to backport?

[bnoordhuis]

@italoacasas See #11051 (comment), this PR depends on a few others but @sam-github is working on it.