Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for "Intl NumberFormat v3" proposal #3307

Merged
merged 2 commits into from
Dec 4, 2021

Conversation

jugglinmike
Copy link
Contributor

This patch is intended to cover only one aspect of the proposal for
ECMA402: the "interpret strings as decimals" feature.


The proposal text for this feature is still being refined; these tests reflect my understanding of the intention based on this discussion in the proposal's repository.

@sffc would you mind taking a look?

This patch is intended to cover only one aspect of the proposal for
ECMA402: the "interpret strings as decimals" feature.
Comment on lines 23 to 24
['Infinity', 'Infinity'],
['-Infinity', '-Infinity']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't test much; the test is


toNumberResults.forEach(pair => {
  const [value, result] = pair;
  assert.sameValue(nf.format(value), nf.format(result));
});

so you're only testing that nf.format('Infinity') is the same value as nf.format('Infinity'), which is an identity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's right. I've changed the second element in each case to be the Number representation of Infinity.

Comment on lines +14 to +19
// The value 100,000 should only be interpreted as infinity if the input is the
// string "Infinity".
assert.sameValue(nf.format('100000'), '100,000');
// The value -100,000 should only be interpreted as negative infinity if the
// input is the string "-Infinity".
assert.sameValue(nf.format('-100000'), '-100,000');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what invariant these tests are verifying. Is the number 100,000 significant in the spec somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my attempt to verify the following conditions in ToIntlMathematicalValue:

  1. If mv is 1010000 and str contains Infinity, return +∞.
  2. If mv is -1010000 and str contains Infinity, return -∞.

The intent of these assertions is to ensure that implementations reference not only the mathematical value of the input but also the original string representation. I believe these assertions would fail if an engine incorrectly implemented somethng like this:

  1. If mv is 1010000, return +∞.
  2. If mv is -1010000, return -∞.

Does that seem right to you?

@rwaldron rwaldron merged commit 4d23bbf into main Dec 4, 2021
@rwaldron rwaldron deleted the bocoup/intl-numberformat-v3-strings-as-decimals branch December 4, 2021 00:51
// input is the string "-Infinity".
assert.sameValue(nf.format('-100000'), '-100,000');

assert.sameValue(nf.format('1.0000000000000001'), '1.0000000000000001');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the result '1.0000000000000001' here?
I think it should be '1' becaues the rounding w/ the default values of fractionDigits and signification digits.

assert.sameValue(nf.format('-100000'), '-100,000');

assert.sameValue(nf.format('1.0000000000000001'), '1.0000000000000001');
assert.sameValue(nf.format('-1.0000000000000001'), '1.0000000000000001');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is surely wrong, a negative number in the string should not be formatted as a posotive number here and without rounding.

@sffc
Copy link
Contributor

sffc commented Dec 23, 2021

To close the loop: @FrankYFTang's comments were addressed in #3335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants