Bug 154530 – New intl-numberformat.js test fails on many Linux platforms

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

intl-numberformat.js introduced in http://trac.webkit.org/changeset/196850,
but fails on EFL ARM and GTK x86_64 buildbots:
- GTK bot: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/13917
- EFL ARM bot: https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release/builds/17225

It's strange that it passes on the EFL x86_64 bot. I don't know anything about this code path.
Is it possible that some 3rd party libraries with different version cause this failure?

[Sukolsak Sakshuwong]

This is likely the same issue as Bug 152448. It is because Intl uses POSIX locale.

    $ LANG=en_US.UTF-8 ./jsc -e "print(Infinity.toLocaleString())"
    ∞
    $ LANG=en_US_POSIX.UTF-8 ./jsc -e "print(Infinity.toLocaleString())"
    INF

The proposed patch in Bug 152448 should fix it.

[Andy VanWagoner]

The patch for Bug 152448 likely hasn't resolved this issue, because it only provided a hook for defaultLanguage that is implemented in WebCore.

[Darin Adler]

There are two separate questions here:

One is how to make WebKit and JavaScriptCore properly choose a default locale for each platform. I don’t know what the right way to do that is for Linux in general. Fixing that for WebCore is what bug 152448 is all about. But it almost certainly needs to be fixed for direct JavaScriptCore use too. At least I know it does need to be done on the Apple platforms. So we may need a bug to track that.

The other is how to make the test harness set a consistent language so that tests are reliable, rather than being sensitive to the language the user running the tests happens to prefer. That's something we need to address separately. There are lots of calls to setenv in various test tools, so it could be as simple as doing a setenv("LANG") in the right place for the appropriate platforms in some of the test tools.

[Carlos Alberto Lopez Perez]

(In reply to comment #3)
> There are two separate questions here:
> 
> One is how to make WebKit and JavaScriptCore properly choose a default
> locale for each platform. I don’t know what the right way to do that is for
> Linux in general. Fixing that for WebCore is what bug 152448 is all about.
> But it almost certainly needs to be fixed for direct JavaScriptCore use too.
> At least I know it does need to be done on the Apple platforms. So we may
> need a bug to track that.
> 


With glibc (GNU/Linux) the default locale is defined on the environment variable LANG. If LANG is not defined, then the locale defaults to 'C' (also known as 'POSIX') which is the default portable locale.

This default locale can be overridden with the environment variable LC_ALL.

So perhaps we can just do setenv("LC_ALL", "C") before starting the tests. This will override any locale defined by the user, and it will be portable across operating systems

Some quick tests:

# Linux
$ LANG=es_ES.UTF-8 WebKitBuild/Release/bin/jsc -e "print(Infinity.toLocaleString())"
∞
$ LC_ALL=POSIX LANG=es_ES.UTF-8 WebKitBuild/Release/bin/jsc -e "print(Infinity.toLocaleString())"
INF
$ LC_ALL=C LANG=es_ES.UTF-8 WebKitBuild/Release/bin/jsc -e "print(Infinity.toLocaleString())"
INF


# MacOS
$ LANG=es_ES.UTF-8 WebKitBuild//Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Infinity.toLocaleString())"
∞
$ LC_ALL=POSIX LANG=es_ES.UTF-8 WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Infinity.toLocaleString())"
INF
$ LC_ALL=C LANG=es_ES.UTF-8 WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Infinity.toLocaleString())"
INF


> The other is how to make the test harness set a consistent language so that
> tests are reliable, rather than being sensitive to the language the user
> running the tests happens to prefer. That's something we need to address
> separately. There are lots of calls to setenv in various test tools, so it
> could be as simple as doing a setenv("LANG") in the right place for the
> appropriate platforms in some of the test tools.

As explained above, just doing setenv("LC_ALL", "C") before starting the tests should work for all use cases.

More info: http://man7.org/linux/man-pages/man3/setlocale.3.html and https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/locale.1.html

[Carlos Alberto Lopez Perez]

Seems that my previous suggestion of setting LC_ALL is not going to help.

This test js/intl-numberformat.html is manually setting the locales and trying some numeric and monetary conversions and checking that it returns the right values.

For example see the follow examples:

linux # WebKitBuild/Release/bin/jsc -e "print(Intl.NumberFormat('es').format(1234.567))"
1 234,567
linux # $ WebKitBuild/Release/bin/jsc -e "print(Intl.NumberFormat('en').format(1234.567))"
1,234.567


macos # WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Intl.NumberFormat('es').format(1234.567))"
1.234,567
macos # WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Intl.NumberFormat('en').format(1234.567))"
1,234.567


So it seems there is fundamental difference here.
In MacOS we are getting a '.' in the thousands decimal mark for the spanish locale (right), meanwhile in Linux we are getting an space (wrong).

The above checks are part of the test js/intl-numberformat.html therefore on Linux we are getting:

-PASS Intl.NumberFormat('es').format(1234.567) is '1.234,567'
+FAIL Intl.NumberFormat('es').format(1234.567) should be 1.234,567. Was 1 234,567.


Not sure what is wrong, at least with printf on Linux I get the right values

LC_NUMERIC=en_US.UTF8 /usr/bin/printf "%'.2f\n" '1234.567'
1,234.57
LC_NUMERIC=es_ES.UTF8 /usr/bin/printf "%'.2f\n" '1234.567'
1.234,57

So this don't seems a problem on the Linux glibc, but at first it looks that something on jsc is not picking/setting the right values for the locales when running on Linux


This is the full text difference for the test js/intl-numberformat.html with WebKitGTK+ (Linux):

--- /home/clopez/webkit/webkit/layout-test-results/js/intl-numberformat-expected.txt
+++ /home/clopez/webkit/webkit/layout-test-results/js/intl-numberformat-actual.txt
@@ -140,7 +140,7 @@
 PASS Intl.NumberFormat('en').format(Number.MIN_VALUE) is '0'
 PASS Intl.NumberFormat('en').format(Number.MAX_VALUE) is '179,769,313,486,232,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000'
 PASS Intl.NumberFormat('en').format(1234.567) is '1,234.567'
-PASS Intl.NumberFormat('es').format(1234.567) is '1.234,567'
+FAIL Intl.NumberFormat('es').format(1234.567) should be 1.234,567. Was 1 234,567.
 PASS Intl.NumberFormat('fr').format(1234.567) is '1 234,567'
 PASS Intl.NumberFormat('en-u-nu-latn').format(1234.567) is '1,234.567'
 PASS Intl.NumberFormat('en-u-nu-fullwide').format(1234.567) is '１,２３４.５６７'
@@ -156,14 +156,14 @@
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD'}).format(Infinity) is '$∞'
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY'}).format(4.2) is '¥4'
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'xXx'}).format(4.2) is 'XXX4.20'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'code'}).format(4) is 'USD4.00'
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'code'}).format(4) should be USD4.00. Threw exception Error: Failed to format a number.
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'symbol'}).format(4) is '$4.00'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) is '4.00 US dollars'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'code'}).format(-4.2) is '-JPY4'
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) should be 4.00 US dollars. Threw exception Error: Failed to format a number.
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'code'}).format(-4.2) should be -JPY4. Threw exception Error: Failed to format a number.
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'symbol'}).format(-4.2) is '-¥4'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(-4.2) is '-4 Japanese yen'
-PASS Intl.NumberFormat('fr', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) is '4,00 dollars des États-Unis'
-PASS Intl.NumberFormat('fr', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(4) is '4 yens japonais'
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(-4.2) should be -4 Japanese yen. Threw exception Error: Failed to format a number.
+FAIL Intl.NumberFormat('fr', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) should be 4,00 dollars des États-Unis. Threw exception Error: Failed to format a number.
+FAIL Intl.NumberFormat('fr', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(4) should be 4 yens japonais. Threw exception Error: Failed to format a number.
 PASS Intl.NumberFormat('en', {minimumIntegerDigits: 4}).format(12) is '0,012'
 PASS Intl.NumberFormat('en', {minimumIntegerDigits: 4}).format(12345) is '12,345'
 PASS Intl.NumberFormat('en', {minimumFractionDigits: 3}).format(1) is '1.000'
@@ -184,7 +184,7 @@
 PASS Intl.NumberFormat('en', {maximumSignificantDigits: 4}).format(0.1234567) is '0.1235'
 PASS Intl.NumberFormat('en', {maximumSignificantDigits: 4}).format(1234567) is '1,235,000'
 PASS Intl.NumberFormat('en', {useGrouping: true}).format(1234567.123) is '1,234,567.123'
-PASS Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123) is '1.234.567,123'
+FAIL Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123) should be 1.234.567,123. Was 1 234 567,123.
 PASS Intl.NumberFormat('fr', {useGrouping: true}).format(1234567.123) is '1 234 567,123'
 PASS Intl.NumberFormat('en', {useGrouping: false}).format(1234567.123) is '1234567.123'
 PASS Intl.NumberFormat('es', {useGrouping: false}).format(1234567.123) is '1234567,123'

[Carlos Garcia Campos]

Many other tests started to fail after r201066, but the problem seems to be the same. run-javascriptcore-tests does $ENV{LANG}="en_US.UTF-8"; but we are not actually honoring the environment variables at all when using jsc binary. We are using setlocale with a nullptr locale to get the current one, but the current one is always "C", because to set the locale according to the environment variables we need to call setlocale with an empty string as locale. That's done by  gtk_init() which is called by all our binaries (web process, network process, etc.), but not by jsc (because jsc doesn't depend on GTK+). The reason why it has always worked for EFL is because they call ecore_init() in jsc that calls setlocale. I'll submit a patch to fix this and the new tests failing.

[Carlos Garcia Campos]

Created attachment 279758 [details]
Patch

I'm not sure this is also needed for jsc-only port. In that case it would be a matter of updating the ifdef

[Darin Adler]

Comment on attachment 279758 [details]
Patch

OK. Not sure this is the right way to do things. Seems unfortunate to have significant platform dependencies in a command line tool. Unpleasant to have iOS, EFL, Windows, and GTK specifics. I suppose I should be grateful we don’t have anything like this for Mac yet?

[Carlos Garcia Campos]

hmm, I was wrong, even with this patch intl-numberformat.js is filing, but all others that started to fail after r201066 are passing now, so I'll submit a separate bug.

[Csaba Osztrogonác]

(In reply to comment #7)
> Created attachment 279758 [details]
> Patch
> 
> I'm not sure this is also needed for jsc-only port. In that case it would be
> a matter of updating the ifdef

I'll check if it fixes failures on jsc-only ARM bots. 
( without guard, of course :) )

Could you wait ~2-3 hours for my testing before landing?

[Carlos Garcia Campos]

Comment on attachment 279758 [details]
Patch

Clearing flags, I moved the patch to bug #158066

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #7)
> > Created attachment 279758 [details]
> > Patch
> > 
> > I'm not sure this is also needed for jsc-only port. In that case it would be
> > a matter of updating the ifdef
> 
> I'll check if it fixes failures on jsc-only ARM bots. 
> ( without guard, of course :) )
> 
> Could you wait ~2-3 hours for my testing before landing?

Sure

[Carlos Garcia Campos]

intl-numberformat.js works for me locally, but not in the bots

[Carlos Garcia Campos]

In my laptop:

$ LANG=en_US.UTF-8 bin/jsc -e "print(Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123))"
1.234.567,123

In the bot:

$ LANG=en_US.UTF-8 bin/jsc -e "print(Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123))"
1 234 567,123

There must be something else, doesn't look like a WebKit problem.

[Carlos Garcia Campos]

Are we using ICU for this? could it be the ICU version? it's the only (relevant) thing that looks different between my laptop and the bot

[Carlos Garcia Campos]

Created attachment 279882 [details]
Patch for GTK+

So it was indeed the ICU version installed in the bots. Confirmed that using a newer version in the bots fixes the tests.

[Carlos Garcia Campos]

I tried the patch on a clean build in a tests bot we have and it worked, so I'm not sure about he EWS failure, maybe a clan build would fix it.

[Darin Adler]

Comment on attachment 279882 [details]
Patch for GTK+

Please look at the compile failure in GTK EWS.

[Carlos Garcia Campos]

Darin, I think it's a clean build issue, because I've tried in a test bot and it worked. I'll trigger clean builds in the bots and watch them after landing.

[Carlos Garcia Campos]

Committed r201449: <http://trac.webkit.org/changeset/201449>

[Yusuke Suzuki]

While your change solves the JSC tests (Great!!), the layout-tests in GTK ports pose many new failing tests instead. Do you already know the reason?

[Yusuke Suzuki]

Ah, no, these tests are previously failing. Sorry.

[Csaba Osztrogonác]

It broke the GTK ARM bot.

[Carlos Garcia Campos]

(In reply to comment #23)
> It broke the GTK ARM bot.

I already talked to Gustavo, who was going to look at it. For some reason wget is failing to download from sourceforge in the ARM bot. In any case, it's a problem of the bot itself, not the patch.

[Carlos Alberto Lopez Perez]

Committed r201509: <http://trac.webkit.org/changeset/201509>