Bug 218763 – stress/intl-datetimeformat-formatrange.js and stress/intl-datetimeformat-formatrange-relevant-extensions.js fail with ICU 65.1

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

Yesterday Red Hat upgraded its internal CI to Fedora 32 (ICU 65.1), causing stress/intl-datetimeformat-formatrange-relevant-extensions.js and stress/intl-datetimeformat-formatrange.js to begin failing. The problem is that the space characters used in the range format changed at some point. The current version of the test expects normal ASCII space characters for ICU 67 and newer, and special spaces for older versions of ICU.

The test passes for me locally on Fedora 33 (ICU 67.1), so perhaps we just need to use the new codepath for ICU 65 and 66 as well. I'm going to try this first, but I'm not sure if it will pass EWS. Problem is, we seem to have strange differences in behavior between Apple and Linux. So if this fails EWS, we'll need to find some more creative way to ensure the test passes regardless of which space character ICU decides to use.

[Michael Catanzaro]

Created attachment 413718 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 413718 [details]
Patch

r=me

[Radar WebKit Bug Importer]

<rdar://problem/71257288>

[EWS]

Committed r269667: <https://trac.webkit.org/changeset/269667>

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

[Michael Catanzaro]

Reopening since these tests are broken again after bug #213822. I'll look closer and try to come up with something next week.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> next week.

Ah well, better late than never!

[Michael Catanzaro]

Created attachment 415561 [details]
Patch

[Michael Catanzaro]

Comment on attachment 415561 [details]
Patch

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

> JSTests/stress/intl-datetimeformat-formatrange-relevant-extensions.js:51
> -    shouldBe(fmt1.formatRange(date1, date2), `ä¸/ä¸ã/ãä¸, äº:ãã${range}ä¸:ãã AM`);
> -    shouldBe(fmt1.formatRange(date1, date3), `ä¸/ä¸ã/ãä¸, äº:ãã AM${range}ä¸/äºã/ãä¸, äº:ãã AM`);
> +    shouldBe(fmt1.formatRange(date1, date2), `ä¸/ä¸ã/ãä¸, äº:ãã â ä¸:ãã AM`);
> +    shouldBe(fmt1.formatRange(date1, date3), `ä¸/ä¸ã/ãä¸, äº:ãã AM â ä¸/äºã/ãä¸, äº:ãã AM`);

Of course these diffs are unreadable in the Bugzilla review tool, since it doesn't use UTF-8....

[Michael Catanzaro]

Comment on attachment 415561 [details]
Patch

jsc EWS is unhappy. I'll need to look over its results.

[Michael Catanzaro]

Created attachment 415592 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415592 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 415592 [details]
Patch

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

> JSTests/stress/intl-datetimeformat-formatrange-relevant-extensions-ja.js:5
> +    if (Object.prototype.toString.call(actual) === '[object String]')

We can just do, `typeof actual === 'string'`.

[Michael Catanzaro]

Thanks for reviewing.

(In reply to Yusuke Suzuki from comment #12)
> We can just do, `typeof actual === 'string'`.

I think it will work in this case since everything is a string literal or template, but StackOverflow says to avoid it since it has a tendency to return garbage results: see https://stackoverflow.com/a/7894320/1120203 linking to http://bonsaiden.github.io/JavaScript-Garden/#types.

Do you want me to try changing this anyway?

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #13)
> Thanks for reviewing.
> 
> (In reply to Yusuke Suzuki from comment #12)
> > We can just do, `typeof actual === 'string'`.
> 
> I think it will work in this case since everything is a string literal or
> template, but StackOverflow says to avoid it since it has a tendency to
> return garbage results: see https://stackoverflow.com/a/7894320/1120203
> linking to http://bonsaiden.github.io/JavaScript-Garden/#types.
> 
> Do you want me to try changing this anyway?

Yes, I think `typeof` is better. `Object.prototype.toString.call(actual)` returns `"[object String]"` when,

1. The input is actually String. Then "ToObject" abstract operation will be performed and it is converted to String object. Then, `Object.prototype.toString` returns "[object String]"
2. The input is weird String object (new String("Hello")). Then it just returns "[object String]"

On the other hand, `typeof actual` returns "string" only when `actual` is actually string primitive type. It returns "object" when the input is String object.

In this case, we will never pass String object. If String object appears, rather, this is a bug.

The stack overflow entry attempts to find what class the given object is. `typeof` is not appropriate for that purpose.
But here, what we would like to know is whether the input is string primitive type (not object), not a class of object. So `typeof` is the best.

[Michael Catanzaro]

OK, 'typeof' it is. I don't pretend to understand JavaScript, anyway. :D

[Michael Catanzaro]

Created attachment 415798 [details]
Patch

[EWS]

Committed r270608: <https://trac.webkit.org/changeset/270608>

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