Bug 210085 - Update JSTests to assume ICU 60+
Summary: Update JSTests to assume ICU 60+
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-06 17:49 PDT by Ross Kirsling
Modified: 2020-04-07 12:36 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.98 KB, patch)
2020-04-06 17:50 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.14 KB, patch)
2020-04-06 20:55 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-04-06 17:49:23 PDT
Update JSTests to assume ICU 60+
Comment 1 Ross Kirsling 2020-04-06 17:50:43 PDT
Created attachment 395639 [details]
Patch
Comment 2 Ross Kirsling 2020-04-06 20:44:39 PDT
Argh, the following test case depends on ICU version (incidentally one of the same cases affected by bug 154530 but in a new way):

  Intl.NumberFormat('es').format(1234.567)
    • using ICU 63 (WinCairo): 1.234,567
    • using ICU 64 (Catalina): 1234,567

A simple fix would be to add a digit, in which case both use the thousands separator.
Comment 3 Ross Kirsling 2020-04-06 20:55:56 PDT
Created attachment 395649 [details]
Patch
Comment 4 Ross Kirsling 2020-04-06 22:41:04 PDT
To elaborate a bit more about the 'es' locale change...

Evidently this was a bug fix, since CLDR data specifies "minimumGroupingDigits" as 2 for Spanish:
https://www.unicode.org/cldr/charts/35/by_type/numbers.symbols.html#70ef5e0c9d323e01

It is a surprising change though, so apparently folks have filed bugs about it elsewhere:
https://unicode-org.atlassian.net/browse/CLDR-13676
https://bugs.chromium.org/p/chromium/issues/detail?id=1019268

Intl.NumberFormat V3 plans to make useGrouping an enum so that this can be better configured:
https://github.com/tc39/proposal-intl-numberformat-v3#part-3-grouping-enum-ecma-402-367
Comment 5 Ross Kirsling 2020-04-07 11:46:31 PDT
Comment on attachment 395649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395649&action=review

> JSTests/stress/intl-pluralrules.js:290
> -shouldBe(new Intl.PluralRules('ar').resolvedOptions().pluralCategories.join(), 'zero,one,two,few,many,other');
> +shouldBe(new Intl.PluralRules('ar').resolvedOptions().pluralCategories.join(), 'few,many,one,two,zero,other');

Also, regarding this fix to a previously-disabled test:
ECMA-402 doesn't specify an order for these, so engines are all just keeping them in the order received from ICU.
Comment 6 Yusuke Suzuki 2020-04-07 11:58:42 PDT
Comment on attachment 395649 [details]
Patch

r=me
Comment 7 EWS 2020-04-07 12:36:00 PDT
Committed r259658: <https://trac.webkit.org/changeset/259658>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395649 [details].
Comment 8 Radar WebKit Bug Importer 2020-04-07 12:36:16 PDT
<rdar://problem/61405323>