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

more multipleOf and overflow tests, consolidated into optional/ #538

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

Conversation

karenetheridge
Copy link
Member

More argument combinations for multipleOf, including floats that will be treated as integers, as these might be internally represented as floats rather than ints, and therefore may behave differently.

All tests that might overflow (i.e. with very large/small numbers or a high number of significant digits) are consolidated into optional/ as an error may not be treated the same as valid: false.

an implementation might use a different internal representation for 1.0 vs 1, therefore we test both
@karenetheridge
Copy link
Member Author

closes #534.

an implementation might use a different internal representation for 1.0 vs 1, therefore we test both
an overflow result is an error, which may not be treated as valid: false by an implementation
@karenetheridge
Copy link
Member Author

I've tested these on drafts 2020-12, 2019-09 and 7; if someone else with support could check them on draft6 and draft4 that would be great.

@karenetheridge karenetheridge requested a review from a team December 30, 2021 19:37
@Julian
Copy link
Member

Julian commented Jan 4, 2022

(Thanks!)

I'm not sure how I feel about fully combining these yet personally.

There are 2 distinct pieces here to me --

  • whether your implementation uses bignums (for integral values)
  • whether your implementation supports infinite precision fixed-or-floating point values

JSON only has numbers, but many languages (mine, Python one of them) uses floats for JSON numbers with decimals, and ints for ones without. Which means I support (by default) bignum but not infinite precision floats. I think that will not be uncommon -- so personally I'd probably lean towards splitting these tests into two separate optional files rather than just the one.

Opinions welcome from others though.

@gregsdennis
Copy link
Member

I'm happy either way. My main concern was putting both cases into optional, which is being addressed, so 👍

@karenetheridge
Copy link
Member Author

karenetheridge commented Jan 9, 2022

JSON only has numbers, but many languages (mine, Python one of them) uses floats for JSON numbers with decimals, and ints for ones without. Which means I support (by default) bignum but not infinite precision floats.

I'm not following your logic here. You would need bignums for high precision calculations, whether that's with integers or floats. e.g. for a calculation like 1e+308 / 0.123456789, both the arguments can be represented with native-architecture floats, but the calculation itself requires a high degree of precision in order to determine if the remainder is zero.

@Julian
Copy link
Member

Julian commented Jan 9, 2022

I'm talking about tests like this: https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/538/files#diff-3070fe12dfc9ffeaf3ec0a633efce5b08f5cd07358f512904e3ffdb8f0b5103eR76-R95

That test mixes two unrelated concepts -- 18446744073709551600 passes if your implementation uses bigintegers. 18446744073709551600.0 requires the use of non-IEEE floats. An implementation may support one and not the other. And I was saying it's easier to deal with that if we have the two concepts separated.

@karenetheridge
Copy link
Member Author

ok, I see.
I can split the tests back out.

@Julian
Copy link
Member

Julian commented Jun 15, 2022

@karenetheridge any chance you're still up for splitting the above?

@Julian Julian added the waiting for author A pull request which is waiting for an update from its author. label Jun 22, 2022
@karenetheridge karenetheridge marked this pull request as draft June 25, 2022 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author A pull request which is waiting for an update from its author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants