| Summary: | REGRESSION(r264639): Intl.DisplayNames tests failing on all architectures | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||
| Component: | JavaScriptCore | Assignee: | Michael Catanzaro <mcatanzaro> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | darin, mcatanzaro, webkit-bug-importer, ysuzuki | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| Attachments: |
|
||||||
|
Description
Michael Catanzaro
2020-07-21 05:11:02 PDT
(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. 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? (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. (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 (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. (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. :) (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. (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. Created attachment 405083 [details]
Patch
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.
FWIW I would say that new internationalization features are just not expected to work with old versions of ICU. Nothing wrong with that. Comment on attachment 405083 [details]
Patch
I got a bit eager here, let's wait for the remaining JSC EWS, just in case.
Committed r264823: <https://trac.webkit.org/changeset/264823> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405083 [details]. |