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

Increase coverage NFv3 - results of SetNumberFormatDigitOptions #3552

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

romulocintra
Copy link
Member

Uses Example given in tc39/ecma402#575 (review) to increment coverage in NFv3 tests and AO - SetNumberFormatDigitOptions.

Related with #3508

@romulocintra romulocintra force-pushed the nfv3-resolvedoptions branch from b661792 to 40a33ac Compare June 8, 2022 13:42
@romulocintra romulocintra marked this pull request as ready for review June 9, 2022 21:36
@romulocintra romulocintra force-pushed the nfv3-resolvedoptions branch from 40a33ac to f355f4a Compare June 10, 2022 07:51
assert.sameValue(actual.minimumIntegerDigits, expected.MinimumIntegerDigits);
assert.sameValue(actual.minimumFractionDigits, expected.MinimumFractionDigits);
assert.sameValue(actual.maximumFractionDigits, expected.MaximumFractionDigits);
assert.sameValue(actual.minimumSignificantDigits, expected.MinimumSignificantDigits);
Copy link
Member Author

@romulocintra romulocintra Jun 10, 2022

Choose a reason for hiding this comment

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

  • Check if options has min and max SignificantDigits to assert on resolvedOptions

@romulocintra romulocintra marked this pull request as draft June 10, 2022 09:11
@romulocintra romulocintra added the ECMA-402 ECMA-402 related PRs label Jun 16, 2022
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 11, 2022

This PR contains an implementation of the relevant part of the spec to compare against the user agent's implementation. The main question here is if this is an approach we'd like to land in test262. We could also consider landing it in staging for now, to at least have the increased coverage while we discuss.

@ptomato
Copy link
Contributor

ptomato commented Nov 2, 2022

Unfortunately, I forgot to add this to the last maintainers meeting agenda. I've put it on the agenda for next time.

@jugglinmike
Copy link
Contributor

Hi there, @romulocintra! We reviewed this patch during the Test262 maintainers' meeting yesterday. Coul you use a more declarative style? We'd prefer a set of meaningful cases with hard-coded expected results.

While your recreation of the proposal's algorithm is impressive, it becomes a sort of "reference implementation" if maintained here in Test262. We're not just interested in verifying the correctness of other runtimes; we also want to validate the design of the proposals themselves. In that sense, writing and reviewing a curated set of assertions (common cases, edge cases, etc.) is an important part of the design process--even if it doesn't inspire changes to the text.

If semantically-related "groups" of assertions emerge from this change, then all the better! Factoring those into dedicated tests will give implementors a better understanding of their bugs when/if failures occur (remember that a Test262 test can only fail once).

Thanks for the help!

@sffc
Copy link
Contributor

sffc commented Dec 12, 2022

Hi @jugglinmike -- This particular AO has a lot of corner cases that are difficult to cover fully from higher level assertions. It was very helpful when editorially refactoring this AO to write this test in order to ensure that we produce the same output given the same input. I think adding this test is useful in this specific case, even if this type of test isn't generally applicable in other cases.

@jugglinmike
Copy link
Contributor

Hi there, @sffc! I've also written reference implementations to help validate others' work, and I've even used projects from third parties along these lines. While it may be tempting to enshrine those implementations as the de-facto source of truth, test authors and their reviewers would miss an opportunity to reaffirm the specified behavior. Abstractions like this patch's SetNumberFormatDigitOptions_v3 meaningfully distances editors and implementors such that accepting a patch is more akin to saying, "this code matches the proposal," and less like declaring, "this is how we wish implementations to behave."

Using an alternate implementation is a great way to identify edge cases, and once identified, we'd all benefit from tests which give some structure to those edges--grouping them as appropriate under titles/messages which describe their semantic significance. Such intentional curation does not have to mean settling for inferior coverage, though it almost certainly will lead to fewer assertions. (That also satisfies practical resource constraints--my machine takes 14 seconds to execute the 2 million assertions hidden within this test, indicating a degree of exhaustiveness which won't scale to the entire language.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author ECMA-402 ECMA-402 related PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants