Closed Bug 853706 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mozillabugs, Assigned: mozillabugs)

References

()

Details

Attachments

(5 files, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: general → mozillabugs
Status: NEW → ASSIGNED
Attachment #769533 - Flags: review?(jwalden+bmo)
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).
Attachment #769533 - Flags: review?(jwalden+bmo) → feedback+
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.  :-(
Attachment #769945 - Flags: review?(jwalden+bmo) → review+
(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.
As requested in comment 3. Sorry, I can't do a Test402 test currently for lack of a contract with any Ecma member.
Attachment #771186 - Flags: review?(jwalden+bmo)
Separated out the patch to ICU sources so that update-icu.sh can refer to it.
Attachment #769533 - Attachment is obsolete: true
Attachment #771188 - Flags: review?(jwalden+bmo)
Attachment #771189 - Flags: review?(jwalden+bmo)
As suggested in comment 5.
Attachment #771196 - Flags: review?(jwalden+bmo)
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.
Attachment #771186 - Flags: review?(jwalden+bmo) → review+
Attachment #771188 - Flags: review?(jwalden+bmo) → review+
Attachment #771189 - Flags: review?(jwalden+bmo) → review+
Attachment #771196 - Flags: review?(jwalden+bmo) → review+
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!
Target Milestone: --- → mozilla25
(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.
You need to log in before you can comment on or make changes to this bug.