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

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Norbert Lindenberg, Assigned: Norbert Lindenberg)

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 769533 [details] [diff] [review]
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.
Assignee: general → mozillabugs
Status: NEW → ASSIGNED
Attachment #769533 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 769945 [details] [diff] [review]
Fixed test case that relied on previous incorrect behavior
Attachment #769945 - 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+
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 771186 [details] [diff] [review]
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.
Attachment #771186 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

5 years ago
Created attachment 771188 [details] [diff] [review]
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.
Attachment #769533 - Attachment is obsolete: true
Attachment #771188 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 771189 [details] [diff] [review]
Enable previously failing conformance test
Attachment #771189 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

5 years ago
Created attachment 771196 [details] [diff] [review]
Added warning about backported fix to update-icu.sh

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
Also

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

Comment 13

5 years ago
(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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.