Bug 210605 - REGRESSION(r259480): Two new failing i18n tests
Summary: REGRESSION(r259480): Two new failing i18n tests
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Ross Kirsling
Keywords: InRadar
: 210604 (view as bug list)
Depends on:
Reported: 2020-04-16 07:00 PDT by Michael Catanzaro
Modified: 2020-04-16 21:58 PDT (History)
12 users (show)

See Also:

Patch (6.53 KB, patch)
2020-04-16 11:29 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (6.72 KB, patch)
2020-04-16 12:16 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (8.07 KB, patch)
2020-04-16 21:17 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (8.07 KB, patch)
2020-04-16 21:34 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 Michael Catanzaro 2020-04-16 07:00:11 PDT
Running stress/intl-datetimeformat.js.default
stress/intl-datetimeformat.js.default: Exception: Error: expected Etc/UTC but got UTC
stress/intl-datetimeformat.js.default: shouldBe@intl-datetimeformat.js:3:24
stress/intl-datetimeformat.js.default: global code@intl-datetimeformat.js:245:9
stress/intl-datetimeformat.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/intl-datetimeformat.js.default

Running stress/intl-datetimeformat.js.mini-mode
stress/intl-datetimeformat.js.mini-mode: Exception: Error: expected Etc/UTC but got UTC
stress/intl-datetimeformat.js.mini-mode: shouldBe@intl-datetimeformat.js:3:24
stress/intl-datetimeformat.js.mini-mode: global code@intl-datetimeformat.js:245:9
stress/intl-datetimeformat.js.mini-mode: ERROR: Unexpected exit code: 3
FAIL: stress/intl-datetimeformat.js.mini-mode

And these two tests expect UTC to be named differently. These tests starting failing in our internal nightly CI on April 3 (sorry for delay), so it's surely due to r259480 "Move Intl tests from LayoutTests to JSTests." The failing check is:

// Default time zone is a valid canonical time zone.
shouldBe(Intl.DateTimeFormat('en', { timeZone: Intl.DateTimeFormat().resolvedOptions().timeZone }).resolvedOptions().timeZone, Intl.DateTimeFormat().resolvedOptions().timeZone);

TBH I'm not sure what to do about this one. Is it a code bug, or a test bug...? Surely we can work around the error by changing the timezone on the system, but nicer to not do that.
Comment 1 Ross Kirsling 2020-04-16 11:10:45 PDT
Ah, this test was probably failing for Linux before, but it was easier to set more granular expectations on the LayoutTests side.

I think this is a code bug -- the spec says the default time zone needs to be canonicalized too but we're only performing step 3 (https://tc39.es/ecma402/#sec-canonicalizetimezonename) for the non-default path. Will fix.
Comment 2 Ross Kirsling 2020-04-16 11:25:13 PDT
*** Bug 210604 has been marked as a duplicate of this bug. ***
Comment 3 Ross Kirsling 2020-04-16 11:29:49 PDT
Created attachment 396676 [details]
Comment 4 Darin Adler 2020-04-16 11:43:29 PDT
Comment on attachment 396676 [details]

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:134
> +    if (canonical.isNull() || canonical == "Etc/UTC"_s || canonical == "Etc/GMT"_s)
> +        canonical = "UTC"_s;

Nicer if we found a way to share code with the below instead of repeating twice.
Comment 5 Ross Kirsling 2020-04-16 12:16:30 PDT
Created attachment 396685 [details]
Patch for landing
Comment 6 Michael Catanzaro 2020-04-16 12:41:19 PDT
Thanks Ross!
Comment 7 Darin Adler 2020-04-16 12:48:48 PDT
Comment on attachment 396685 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:107
> +static ALWAYS_INLINE bool isUTCEquivalent(StringView timeZone)

I think ALWAYS_INLINE is overkill here. Not incorrect, but not necessary.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:109
> +    return timeZone == "Etc/UTC"_s || timeZone == "Etc/GMT"_s;

Using _s here makes the code a tiny bit larger and a tiny bit slower. I suggest just leaving it off. Alternatively we could overload operator== to take ASCIILiteral, but I don’t really want to do that.
Comment 8 Ross Kirsling 2020-04-16 21:17:28 PDT
Created attachment 396736 [details]
Patch for landing
Comment 9 EWS 2020-04-16 21:17:50 PDT
ChangeLog entry in JSTests/ChangeLog contains OOPS!.
Comment 10 Ross Kirsling 2020-04-16 21:34:23 PDT
Created attachment 396738 [details]
Patch for landing
Comment 11 EWS 2020-04-16 21:57:19 PDT
Committed r260237: <https://trac.webkit.org/changeset/260237>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396738 [details].
Comment 12 Radar WebKit Bug Importer 2020-04-16 21:58:12 PDT