853706 - ECMA-402 test ch11/11.3/11.3.2_TRP.js failing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

Test ch11/11.3/11.3.2_TRP.js of the conformance test suite for standard ECMA-402, ECMAScript Internationalization API, fails when run against the implementation currently being landed.

Intl.NumberFormat.prototype.format returns "0.000" for 0.0 when configured with minimumSignificantDigits=3 and maximumSignificantDigits=5.

The failure is caused by ICU bug 10045:
http://bugs.icu-project.org/trac/ticket/10045

Comment 1

[Norbert Lindenberg]

Attachment: patch

Backported fix for ICU bug 10045 from:
http://bugs.icu-project.org/trac/changeset?reponame=&new=33751%40icu%2Ftrunk%2Fsource%2Fi18n%2Fdecimfmt.cpp&old=33457%40icu%2Ftrunk%2Fsource%2Fi18n%2Fdecimfmt.cpp

Enabled the previously failing test case.

Comment 2

[Norbert Lindenberg]

Attachment: Fixed test case that relied on previous incorrect behavior

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 769533 [details] [diff] [review]
patch

Review of attachment 769533 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.  It does break the we're-not-touching-ICU rule, tho.  Given this is fixed upstream, I'm willing to run with that.  We need to update intl/update-icu.sh for this, tho, to include a patchfile and then apply it post-ICU-update.  This patch-application should be temporary, but I'd like to set a precedent of our leaving a clear trail of breadcrumbs, easily repeated if needed, for someone to follow concerning all our ICU tweaks.  Mind adding a little bit extra here for that?

I had to sit down and work through a few ToRawPrecision calls with varying significant-digit arguments to demonstrate to myself that this was all right.  Could I get some sort of test for results that exercises ToRawPrecision(0, x, y) for all cases where x is in the range [1, 3] and y is in the range [x, 5] (feel free to go further if you want :-) ) as a concentrated double-check of all this zero-raw-precision-formatted behavior?  The existing test only kind of happens to exercise this at all -- I'd like to see more narrowly-targeted tests for the formatting-zero edge case.  I'm happy to do this upstream in test262/test402, too, as this seems like a totally kosher test that doesn't rely on any unspecified behaviors (so long as we pick a simple numbering system, of course).

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 769945 [details] [diff] [review]
Fixed test case that relied on previous incorrect behavior

Review of attachment 769945 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/Intl/NumberFormat/format.js
@@ +39,5 @@
>  format = new Intl.NumberFormat("th-th-u-nu-thai",
>                                 {style: "percent",
>                                  minimumSignificantDigits: 2,
>                                  maximumSignificantDigits: 2});
> +assertEq(format.format(0), "à¹.à¹%");

Bleh, I can't seem to set charset=UTF-8 on this attachment for this to display properly in Bugzilla.  :-(

Comment 5

[Norbert Lindenberg]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)

> ...  We need to
> update intl/update-icu.sh for this, tho, to include a patchfile and then
> apply it post-ICU-update.  This patch-application should be temporary, but
> I'd like to set a precedent of our leaving a clear trail of breadcrumbs,
> easily repeated if needed, for someone to follow concerning all our ICU
> tweaks.

I agree with the "trail of breadcrumbs" idea, but updating update-icu.sh with a patch that most likely will fail the first (and only) time it's used seems excessive. How about I add another warning to update-icu.sh telling its users that they may have to grab another patch from bugzilla if they update to an older ICU version that doesn't have the fix yet? That's what we did for the ICU build system patch.

Comment 6

[Norbert Lindenberg]

Attachment: Added new test case for formatting 0 with significant digits

As requested in comment 3. Sorry, I can't do a Test402 test currently for lack of a contract with any Ecma member.

Comment 7

[Norbert Lindenberg]

Attachment: Backported fix for formatting 0 with significant digits from ICU

Separated out the patch to ICU sources so that update-icu.sh can refer to it.

Comment 8

[Norbert Lindenberg]

Attachment: Enable previously failing conformance test

Comment 9

[Norbert Lindenberg]

Attachment: Added warning about backported fix to update-icu.sh

As suggested in comment 5.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 771186 [details] [diff] [review]
Added new test case for formatting 0 with significant digits

Review of attachment 771186 [details] [diff] [review]:
-----------------------------------------------------------------

Is there something in this test that means it's not upstreamable?  This decimal number formatting is something the spec prescribes, as best I remember.

::: js/src/tests/Intl/NumberFormat/significantDigitsOfZero.js
@@ +27,5 @@
> +    var format = new Intl.NumberFormat("en-US", options);
> +    var actual = format.format(0);
> +    var expected = testData[i].expected;
> +    if (actual !== expected) {
> +        throw new Error("Wrong formatted string for 0 with " +

It's more idiomatic to use assertEq(actual, expected, ...error string...) here.

Comment 11

[Jeff Walden [:Waldo]]

Oops, forgot to nitfix before pushing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/463162c42d13

Also I merged a couple of the patches together for conceptual sensibility.

Thanks!

Comment 12

[Jeff Walden [:Waldo]]

Also

https://hg.mozilla.org/integration/mozilla-inbound/rev/cace398994a6

Comment 13

[Norbert Lindenberg]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)

> Is there something in this test that means it's not upstreamable?  This
> decimal number formatting is something the spec prescribes, as best I
> remember.

As written, the test assumes that the en-US locale is supported and uses Latin digits and the period as decimal separator. The spec doesn't require that, so a conformance test has to be written to not make these assumptions. The existing test 11.3.2_TRP shows how to do it.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/107335e20036
https://hg.mozilla.org/mozilla-central/rev/be7dfe6686b5
https://hg.mozilla.org/mozilla-central/rev/703acacad625
https://hg.mozilla.org/mozilla-central/rev/463162c42d13
https://hg.mozilla.org/mozilla-central/rev/cace398994a6