Bug 210605 – REGRESSION(r259480): Two new failing i18n tests

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

Running stress/intl-datetimeformat.js.default
stress/intl-datetimeformat.js.default: Exception: Error: expected Etc/UTC but got UTC
stress/intl-datetimeformat.js.default: [e-mail]:3:24
stress/intl-datetimeformat.js.default: global [e-mail]: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: [e-mail]:3:24
stress/intl-datetimeformat.js.mini-mode: global [e-mail]: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.

[Ross Kirsling]

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.

[Ross Kirsling]

*** Bug 210604 has been marked as a duplicate of this bug. ***

[Ross Kirsling]

Created attachment 396676 [details]
Patch

[Darin Adler]

Comment on attachment 396676 [details]
Patch

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.

[Ross Kirsling]

Created attachment 396685 [details]
Patch for landing

[Michael Catanzaro]

Thanks Ross!

[Darin Adler]

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.

[Ross Kirsling]

Created attachment 396736 [details]
Patch for landing

[EWS]

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

[Ross Kirsling]

Created attachment 396738 [details]
Patch for landing

[EWS]

Committed r260237: <https://trac.webkit.org/changeset/260237>

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

[Radar WebKit Bug Importer]

<rdar://problem/61919754>