Bug 214589 – REGRESSION(r264639): Intl.DisplayNames tests failing on all architectures

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

The new tests added in r264639 are failing in all architectures (aarch64, ppc64le, and s390x) on Red Hat's internal cloop CI. (x86_64 is currently broken due to bug #214491.)

Running stress/intl-displaynames.js.default
stress/intl-displaynames.js.default: Exception: Error: bad value: BZ
stress/intl-displaynames.js.default: [e-mail]:4:24
stress/intl-displaynames.js.default: global [e-mail]:47:13
stress/intl-displaynames.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/intl-displaynames.js.default

Running stress/intl-parse-unicode-subtags.js.default
stress/intl-parse-unicode-subtags.js.default: Exception: Error: bad value: Simplified Chinese (Hong Kong SAR China)
stress/intl-parse-unicode-subtags.js.default: [e-mail]:4:24
stress/intl-parse-unicode-subtags.js.default: global [e-mail]:53:17
stress/intl-parse-unicode-subtags.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/intl-parse-unicode-subtags.js.default

I'll haven't looked closely yet, but I suspect the test results are making assumptions about the ICU version in use. In the past, we've had to work around these by modifying the test itself to expect multiple possibilities.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> stress/intl-displaynames.js.default: Exception: Error: bad value: BZ

The test expects regionNames.of('BZ') to return "Belize" but it is returning "BZ" because HAVE_ICU_U_LOCALE_DISPLAY_NAMES requires ICU 61, but RHEL 8 uses ICU 60. I'll try updating the test to only run if ICU version is 61 or higher.

> stress/intl-parse-unicode-subtags.js.default: Exception: Error: bad value:
> Simplified Chinese (Hong Kong SAR China)

This one is simple enough. languageNames.of('zh-Hans-HK') used to return "Simplified Chinese (Hong Kong SAR China)" but now returns "Chinese, Simplified (Hong Kong)" so I'll try to make the test account for that.

[Michael Catanzaro]

All of the affected code is guarded by:

if ($vm.icuVersion() >= 61)

I'll continue to investigate.

Yusuke, you have in several places these guards:

if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)

Is this really intentional? ICU 64, 65, and 66 had different behavior, then ICU 67 switched back to the original behavior, in all of these different places?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)
> 
> Is this really intentional? ICU 64, 65, and 66 had different behavior, then
> ICU 67 switched back to the original behavior, in all of these different
> places?

No, because on my host system with ICU 65.1, I see:

stress/intl-displaynames.js.default: Exception: Error: bad value: Traditional Chinese

The test would pass if not for the if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66) guard. I don't understand the purpose of this guard.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
> The test expects regionNames.of('BZ') to return "Belize" but it is returning
> "BZ" because HAVE_ICU_U_LOCALE_DISPLAY_NAMES requires ICU 61, but RHEL 8
> uses ICU 60. 

I think our internal CI may be using some custom environment with a strange version of ICU. I've asked to learn more. I thought it more likely that $vm.icuVersion() was returning a bogus result, but it seems to work perfectly fine on my desktop with ICU 65.1 (where it returns "65").

Both these tests are broken on my desktop in addition to our CI. To make the tests pass, I had to do this:

diff --git a/JSTests/stress/intl-displaynames.js b/JSTests/stress/intl-displaynames.js
index ff15457fb37..d39437ac423 100644
--- a/JSTests/stress/intl-displaynames.js
+++ b/JSTests/stress/intl-displaynames.js
@@ -62,10 +62,7 @@ if ($vm.icuVersion() >= 61) {
     shouldBe(languageNames.of('fr'), "French");
     shouldBe(languageNames.of('de'), "German");
     shouldBe(languageNames.of('fr-CA'), "Canadian French");
-    if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)
-        shouldBe(languageNames.of('zh-Hant'), "Chinese, Traditional");
-    else
-        shouldBe(languageNames.of('zh-Hant'), "Traditional Chinese");
+    shouldBe(languageNames.of('zh-Hant'), "Traditional Chinese");
     shouldBe(languageNames.of('en-US'), "American English");
     shouldBe(languageNames.of('zh-TW'), "Chinese (Taiwan)");
 
@@ -79,10 +76,7 @@ if ($vm.icuVersion() >= 61) {
     shouldBe(languageNames.of('fr'), "French");
     shouldBe(languageNames.of('de'), "German");
     shouldBe(languageNames.of('fr-CA'), "Canadian French");
-    if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)
-        shouldBe(languageNames.of('zh-Hant'), "Chinese, Traditional");
-    else
-        shouldBe(languageNames.of('zh-Hant'), "Traditional Chinese");
+    shouldBe(languageNames.of('zh-Hant'), "Traditional Chinese");
     shouldBe(languageNames.of('en-US'), "US English");
     shouldBe(languageNames.of('zh-TW'), "Chinese (Taiwan)");
 
@@ -90,10 +84,7 @@ if ($vm.icuVersion() >= 61) {
     shouldBe(languageNames.of('fr'), "French");
     shouldBe(languageNames.of('de'), "German");
     shouldBe(languageNames.of('fr-CA'), "Canadian French");
-    if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)
-        shouldBe(languageNames.of('zh-Hant'), "Chinese, Traditional");
-    else
-        shouldBe(languageNames.of('zh-Hant'), "Traditional Chinese");
+    shouldBe(languageNames.of('zh-Hant'), "Traditional Chinese");
     shouldBe(languageNames.of('en-US'), "US English");
     shouldBe(languageNames.of('zh-TW'), "Chinese (Taiwan)");

And:

diff --git a/JSTests/stress/intl-parse-unicode-subtags.js b/JSTests/stress/intl-parse-unicode-subtags.js
index f087cef4f28..997ade666f7 100644
--- a/JSTests/stress/intl-parse-unicode-subtags.js
+++ b/JSTests/stress/intl-parse-unicode-subtags.js
@@ -45,13 +45,8 @@ if ($vm.icuVersion() >= 61) {
     shouldThrow(() => languageNames.of("root-US"), `RangeError: argument is not a language id`);
     if ($vm.icuVersion() >= 64)
         shouldBe(languageNames.of("es-419"), `Latin American Spanish`);
-    if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66) {
-        shouldBe(languageNames.of('zh-Hant'), `Chinese, Traditional`);
-        shouldBe(languageNames.of('zh-Hans-HK'), `Chinese, Simplified (Hong Kong)`);
-    } else {
-        shouldBe(languageNames.of('zh-Hant'), `Traditional Chinese`);
-        shouldBe(languageNames.of('zh-Hans-HK'), `Simplified Chinese (Hong Kong)`);
-    }
+    shouldBe(languageNames.of('zh-Hant'), `Traditional Chinese`);
+    shouldBe(languageNames.of('zh-Hans-HK'), `Simplified Chinese (Hong Kong)`);
     shouldThrow(() => languageNames.of("Hant"), `RangeError: argument is not a language id`); // Script only
     shouldBe(languageNames.of('sr-Latn'), `Serbian (Latin)`); // Language-Script
     shouldBe(languageNames.of('sr-Cyrl'), `Serbian (Cyrillic)`); // Language-Script

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #3)
> (In reply to Michael Catanzaro from comment #2)
> > if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)
> > 
> > Is this really intentional? ICU 64, 65, and 66 had different behavior, then
> > ICU 67 switched back to the original behavior, in all of these different
> > places?
> 
> No, because on my host system with ICU 65.1, I see:
> 
> stress/intl-displaynames.js.default: Exception: Error: bad value:
> Traditional Chinese
> 
> The test would pass if not for the if ($vm.icuVersion() >= 64 &&
> $vm.icuVersion() <= 66) guard. I don't understand the purpose of this guard.

This is because ICU 64 and ICU 66 have different outputs for these strings.

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #5)
> This is because ICU 64 and ICU 66 have different outputs for these strings.

But your condition is:

if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)

This implies that there are two sets of behavior:

 * One behavior for ICU 63 and earlier and for ICU 67 and later
 * One behavior for ICU 64, 65, and 66 alone

That is, it implies that the behavior changed in ICU 64, then changed back to the way it was before in ICU 67. This seems unlikely. Did you intend to write the conditions that way?

Also, because I'm seeing "Traditional Chinese" and "Simplified Chinese (Hong Kong)" with ICU 65, not "Chinese, Traditional" and "Chinese, Simplified (Hong Kong)", I know the check cannot be right. :)

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Yusuke Suzuki from comment #5)
> > This is because ICU 64 and ICU 66 have different outputs for these strings.
> 
> But your condition is:
> 
> if ($vm.icuVersion() >= 64 && $vm.icuVersion() <= 66)
> 
> This implies that there are two sets of behavior:
> 
>  * One behavior for ICU 63 and earlier and for ICU 67 and later
>  * One behavior for ICU 64, 65, and 66 alone
> 
> That is, it implies that the behavior changed in ICU 64, then changed back
> to the way it was before in ICU 67. This seems unlikely. Did you intend to
> write the conditions that way?
> 
> Also, because I'm seeing "Traditional Chinese" and "Simplified Chinese (Hong
> Kong)" with ICU 65, not "Chinese, Traditional" and "Chinese, Simplified
> (Hong Kong)", I know the check cannot be right. :)

Yeah, IIRC, I tested 61, 64, 66, and 67. And 64 and 66 shows the same behavior while 61 and 67 shows the same behavior. So I putted >= 64 and <= 66. If 65 has different behavior, please add it too.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> I think our internal CI may be using some custom environment with a strange
> version of ICU. I've asked to learn more.

Turns out it's running Fedora 31, which has ICU 63. Surprise for me.

So in summary, you have tested ICU 61, 64, 66, and 67, while I've tested ICU 63 and 65. You found ICU 61 and 67 print "Traditional Chinese" while ICU 64 and 66 print "Chinese, Traditional". Meanwhile, I found that both ICU 63 and 65 print "Traditional Chinese".

Conclusion: odd-numbered versions use "Traditional Chinese" while even-numbered versions use "Chinese, Traditional" so we need only check $vm.icuVersion() % 2 :D

More seriously, I'll guess the behavior has maybe been changed in minor release versions, but I've failed to find any relevant commit in the ICU git repo. Probably I'll try to make the test work either way without checking the version.

[Michael Catanzaro]

Created attachment 405083 [details]
Patch

[Darin Adler]

Comment on attachment 405083 [details]
Patch

Great to do this for now. Later, we should reflect on how OK we are with all of this poor behavior in new WebKit with old ICU.

[Michael Catanzaro]

FWIW I would say that new internationalization features are just not expected to work with old versions of ICU. Nothing wrong with that.

[Michael Catanzaro]

Comment on attachment 405083 [details]
Patch

I got a bit eager here, let's wait for the remaining JSC EWS, just in case.

[EWS]

Committed r264823: <https://trac.webkit.org/changeset/264823>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405083 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66053870>