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-process-env-tz fails #27856

Closed
mihan007 opened this issue May 24, 2019 · 11 comments
Closed

test-process-env-tz fails #27856

mihan007 opened this issue May 24, 2019 · 11 comments
Labels
confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests.

Comments

@mihan007
Copy link
Contributor

mihan007 commented May 24, 2019

  • Version:
    current master
  • Platform:
    Darwin iMac-Mikhail 17.7.0 Darwin Kernel Version 17.7.0: Wed Apr 24 21:17:24 PDT 2019; root:xnu-4570.71.45~1/RELEASE_X86_64 x86_64
  • Subsystem:

I'm running standard test suite using make test-only and got

=== release test-process-env-tz ===
Path: parallel/test-process-env-tz
assert.js:89
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'Sat Apr 14 2018 14:34:56 GMT+0200 (GMT+02:00)'
- 'Sat Apr 14 2018 14:34:56 GMT+0200 (CEST)'
    at Object.<anonymous> (/Users/mihan007/Projects/node/test/parallel/test-process-env-tz.js:29:8)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:837:10)
    at internal/main/run_main_module.js:17:11 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'Sat Apr 14 2018 14:34:56 GMT+0200 (GMT+02:00)',
  expected: 'Sat Apr 14 2018 14:34:56 GMT+0200 (CEST)',
  operator: 'strictEqual'
}
Command: out/Release/node /Users/mihan007/Projects/node/test/parallel/test-process-env-tz.js

My best guess here that it happens because I'm using Russian locale at my mac.

@bnoordhuis
Copy link
Member

That's no surprise. v11.13.0 doesn't have the necessary changes, they only just landed in master. make test-only isn't for running an arbitrary node.js release against an arbitrary snapshot of the test suite.

@bnoordhuis bnoordhuis added invalid Issues and PRs that are invalid. test Issues and PRs related to the tests. labels May 24, 2019
@mihan007
Copy link
Contributor Author

I'm sorry, I just tried to prepare for Node Code & Learn Saint Petersburg session. Manual said keep an eye that all tests passed. It's not. What is correct way to run tests against master branch?

@sam-github
Copy link
Contributor

Correct way is to git clone [email protected]:nodejs/node.git, which will checkout master, cd node; ./configure; make -j6 node; make test

@mihan007
Copy link
Contributor Author

Thanks. Will do and let you know if smth goes wrong

@ch1ller0
Copy link
Contributor

The test is still failing on a fresh master branch on my machine when I do make test-only. Maybe reopen the issue?

@addaleax
Copy link
Member

@kefir100 If you’re still around for the Code & Learn, could you reach out to one of the mentors and make sure nothing weird is happening?

@lal12
Copy link
Contributor

lal12 commented Jun 3, 2019

Hmm, I got the same exactly same error, after running:

# working dir is cloned git repo on master branch
git pull
./configure
make -j9 
make test

@bnoordhuis
Copy link
Member

@lal12 Can you open a new issue? I assume that's with a clean checkout, no local changes?

@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

This has happened for two persons on the C&L event in St. Petersburg. The original description mentioned v11.3.0 which was by mistake. The executed code was actually the current master (I updated the description above).

They both had a clean clone and just ran make test.

@BridgeAR BridgeAR reopened this Jun 4, 2019
@BridgeAR BridgeAR added confirmed-bug Issues with confirmed bugs. and removed invalid Issues and PRs that are invalid. labels Jun 4, 2019
@lal12
Copy link
Contributor

lal12 commented Jun 4, 2019

I looked a bit more into it. The string is built in deps/v8/src/builtins/builtins-date.cc:184, it retrieves the time zone from the internal time zone cache. Depending on the build options it chooses different Implementations either (if INTL is activated) the ICU one, else an OS dependent one (for Linux this e.g. is the POSIX one via localtime_r which ironically uses a glibc specific extension ^^). I tested the return of my glibc, which is as expected.

So the problem is ICU related (see deps/v8/src/objects/intl-objects.cc:1758). As the documentation of icu::TimeZone::getDisplayName states it will return GMT[+-]HH:mm, which is was apparently happens here.

@bnoordhuis
Copy link
Member

I had a look and I think the problem here is that V8 calls the version of icu::TimeZone::getDisplayName() that uses an implicit default locale - i.e., whatever icu::Locale::getDefault() returns, and that's influenced by the LC_ALL and LC_MESSAGES environment variables, it's not stable/predictable:

$ env LC_ALL=nl_NL ./out/Release/node test/parallel/test-process-env-tz.js 
assert.js:89
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'Sat Apr 14 2018 14:34:56 GMT+0200 (GMT+02:00)'
- 'Sat Apr 14 2018 14:34:56 GMT+0200 (CEST)'
                                      ^
    at Object.<anonymous> (/Users/bnoordhuis/src/v1.x/test/parallel/test-process-env-tz.js:29:8)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:837:10)
    at internal/main/run_main_module.js:17:11 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'Sat Apr 14 2018 14:34:56 GMT+0200 (GMT+02:00)',
  expected: 'Sat Apr 14 2018 14:34:56 GMT+0200 (CEST)',
  operator: 'strictEqual'
}

The easy fix is to set process.env.LC_ALL = 'C' in the test (and that's what I'll open a PR for) but it might be worth thinking about setting a default locale at startup. Consistency > configuration, IMO.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jun 6, 2019
Set the locale to a known good value because it affects ICU's date
string formatting. Setting LC_ALL needs to happen before the first
call to `icu::Locale::getDefault()` because ICU caches the result.

Fixes: nodejs#27856
@Trott Trott closed this as completed in 2bb93e1 Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants