REGRESSION(r243512): Change locale of test in intl-datetimeformat.js
Created attachment 366263 [details] Patch
r243512 modifies Intl.DateTimeFormat to obey 2-digit hour. A test was added to test 2-digit hour in Russian using 12-hour setting as true and false. In the former case, the expected string appends the string 'AM' in Russian. This localized string made GTK-based ports fail since according to Glib the 'am_pm' values in Russian are not localized (https://github.molgen.mpg.de/git-mirror/glibc/blob/master/localedata/locales/ru_RU#L152). This patch keeps the same test but changes locale to 'en' to prevent the test from failing.
FWIW, the Chromium ticket that fixes this issue features the original test using the 'ru' locale, but 'am_pm' string is not localized. https://chromium-review.googlesource.com/c/v8/v8/+/1529260/3/test/intl/regress-527926.js
The original issue was about the ru locale, but I understand not relying on ru data being complete. I believe the '2-digit' hour was also sometimes ignored, so this is probably still a good enough test for us.
(In reply to Andy VanWagoner from comment #4) > The original issue was about the ru locale, but I understand not relying on > ru data being complete. I believe the '2-digit' hour was also sometimes > ignored, so this is probably still a good enough test for us. *was sometimes ignored in 'en'.
Perhaps the subtest can be split into a separate file, to have custom results for platforms that need them?
I run the test in an old version of WebKit (WebKit v2.22.6), and the 2-digit issue happens also if 'en' locale is used: > Intl.DateTimeFormat('en', {minute: '2-digit', hour: '2-digit', hour12: true, timezone: 'UTC'}).format(1e7) < "3:46 AM" (See attachment) Regarding isolating the test in a different file (so there's different outputs per platform), the problem is the test is valid for GTK but it fails because of the localized string. So this is more about expecting a different actual result per platform. In fact, the original issue that r243512 solves states that valid outputs for this test are: 03:00 03:00 AM (or a Cyrillic alternative to "AM") https://bugs.webkit.org/show_bug.cgi?id=195974 (In r243512, 10e7 is used instead of a number value equivalent to '03:00 AM'). So an alternative solution would be to modify the test to expect all these possible valid outputs. For instance: shouldBeTrue("['02:46 ДП', '02:46 AM', '02:46'].includes(Intl.DateTimeFormat('ru', { minute:'2-digit', hour:'2-digit', hour12: true, timeZone: 'UTC' }).format(1e7))"); Whether to change the locale to 'en' or adapt the test to expect different valid outputs works for me, although I have a preference for changing the locale to a language where am_pm string is normally translated (as in English), so the output of the test is different than the same test where `hour12: false`.
Created attachment 366394 [details] Screenshot of test executed in JavaScript console using 'en' locale
(In reply to Alexey Proskuryakov from comment #6) > Perhaps the subtest can be split into a separate file, to have custom > results for platforms that need them? I think this would be a good idea if we had more platform-specific failures. This is a sensitive test where it's possible to test the thing we want to test in ways that work cross-platform and ways that don't. E.g. Andy has previously rewritten checks that failed for newer versions of ICU to not be sensitive to the ICU version. It could have been done with subtests, like you suggest, but making the test work more generally was fine too. I suspect it's easier to maintain as just one test, though, so r=me.
Comment on attachment 366263 [details] Patch It might be more interesting to test a locale other than English, but like Andy says, the goal of this test was to show that hour:'2-digit' and hour:'numeric' give different output, so it seems fine. Why not just test 'pt-BR' though? Since it worked above, we already know it's localized properly on all the platforms WebKit cares about, right?
Comment on attachment 366263 [details] Patch Clearing flags on attachment: 366263 Committed r243740: <https://trac.webkit.org/changeset/243740>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49532466>