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

Tests combinations max-min fractionDigits and Significant digits #3515

Merged

Conversation

romulocintra
Copy link
Member

@romulocintra romulocintra commented May 2, 2022

#3508 (comment)

Todo's for other tests files:

  • Combine "notation" : "compact" with all other values
  • validate resolvedOptions for each set of options

The goal's to have correct matrix results aligned with spec

cc @sffc @FrankYFTang

@romulocintra romulocintra force-pushed the nfv3-coverage-SetNumberFormatDigitOptions branch from 97b4940 to 1b94e1d Compare May 2, 2022 15:37
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I think the test matrix approach looks good, I trust that the champions know best what the correct format outputs should be 😄 Currently they are all the same though, I assume that's because it's a draft and the real values will be entered later?

One improvement might be to use keys that explain what that particular line tests. e.g.

var optionsMatrix = {
  'same-minimums': { useGrouping: false, minimumSignificantDigits: 2, minimumFractionDigits: 2 },
  'min-sd-larger': { useGrouping: false, minimumSignificantDigits: 3, minimumFractionDigits: 1 },
  ...

and

'auto': {
  'same-minimums': { '1': '1.0', ...

@romulocintra romulocintra marked this pull request as ready for review May 5, 2022 11:51
@romulocintra romulocintra requested a review from ptomato May 5, 2022 11:53
ptomato
ptomato previously approved these changes May 13, 2022
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I think this is basically correct — I didn't check the rounding results exhaustively, but I sampled maybe about 40% of them.

If you wanted to add some comments to explain what is going on in each entry, I'm sure that would really help implementors debug their implementations. For example, in the expected results matrix,

'morePrecision': {
  // 2-21 significant digits
  'same-minimums': { ... },
  // 1-2 significant digits or 0-2 fraction digits, whichever is more precise
  'same-maximums': { ... },
  // 3-21 significant digits
  'minSd-larger-minFd': { ... },

and in the options matrix, for example,

// max significant digits wins
'same-minimums': { ... },
// max digits equal, result with more precision wins
'same-maximums': { ... },

etc.

'minSd-smaller-minFd' :{ minimumSignificantDigits: 1, minimumFractionDigits: 3},
'minSd-smaller-maxFd' :{ minimumSignificantDigits: 1, maximumFractionDigits: 3},
'minSd-larger-maxFd' :{ minimumSignificantDigits: 3, maximumFractionDigits: 1},
'maxSd-larger-minFd' :{ maximumSignificantDigits: 3, minimumFractionDigits: 1},
Copy link
Contributor

Choose a reason for hiding this comment

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

What is supposed to happen in this case? My understanding is that maximumSignificantDigits and maximumFractionDigits are both 3, so it depends on the rounding result itself, which one wins the conflict. Is that correct?

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

OK by me, I still think the comments I suggested would be useful if you have time to add them, but what you have is already fine.

@romulocintra
Copy link
Member Author

OK by me, I still think the comments I suggested would be useful if you have time to add them, but what you have is already fine.

If it's ok for you I can work on those comments while doing pending tasks in upcoming PRs:

  • Combine "notation" : "compact" with all other values
  • validate resolvedOptions for each set of options

That way we can land current tests

@ptomato ptomato merged commit 406ec00 into tc39:main May 25, 2022
catamorphism pushed a commit to nicolo-ribaudo/test262 that referenced this pull request May 30, 2022
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