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

multipleOf contains incorrectly optional (bignum dependent) tests #536

Open
jimmylewis opened this issue Dec 28, 2021 · 6 comments
Open
Labels
bug A test is wrong, or tooling is broken or buggy.

Comments

@jimmylewis
Copy link
Contributor

I'm working on updating my validator to consume the latest version of the test suite (after about 18 months; I usually aim for once per year but hey...), and hit issues with the multipleOf.json tests now containing bignum scenarios (see #438). I also noticed that there's recent discussion about changing or adding more bignum tests to multipleOf (see #534).

As @Julian called out in #438:

Unless, is arbitrary-precision support optional?

Arbitrary precision is optional yeah. You are allowed to use IEEE floats or doubles (and presumably also this -- 16 byte floats)

Can we agree to place these tests in the optional suite so that implementors who wish to leverage the official suite but don't have bignum support can more easily navigate around this obstacle? In #438, the float version of the test was placed under optional, but the integer version was not.

/cc'ing some folks who've been involved in the other issues: @karenetheridge, @Zac-HD

@karenetheridge
Copy link
Member

The tests should already be restricted to optional/ -- the one test in multipleOf.json that tests large numbers is testing that you either 1. can handle bignums and get the correct result, or 2. correctly identify that you overflowed and returned valid: false. I agree that putting a test in multpleOf.json that expects valid: true would be inappropriate, as it is essentially mandating bignum support, which may not be possible for all architectures/languages.

@ChALkeR
Copy link
Member

ChALkeR commented Dec 28, 2021

For the tests that I recommended adding in #534 -- I agree that those should be placed in optional/.

@jimmylewis
Copy link
Contributor Author

jimmylewis commented Dec 28, 2021

In my implementation, the validator returns neither true nor false, but instead an error that the data could not be handled, and thus we cannot determine whether the document is valid. This seems like a fair behavior ("I can't tell you what I don't know"), but the test is making assumptions about the implementation returning false. Unless I've missed something, I don't see an expectation for how implementations should behave in cases of data inputs they can't handle.

Edit:
"implementations can't handle X input" sounds bad, but platform interoperability, at least as far as numerical types are concerned, is called out in the JSON standard itself:

A JSON number such as 1E400 or
3.141592653589793238462643383279 may indicate potential
interoperability problems, since it suggests that the software that
created it expects receiving software to have greater capabilities
for numeric magnitude and precision than is widely available.

Recognizing that not all platforms are capable of working with these values should also be respected by the schema test suite, as @gregsdennis pointed out in #378:

This test suite is supposed to test JSON Schema. Anything that isn't supported because of the decisions of the way JSON is implemented is out of the Schema library's control (assuming they're separate libraries).

@gregsdennis
Copy link
Member

I think the issue here is a misunderstanding of a nuance in failure modes.

correctly identify that you overflowed and returned valid: false - @karenetheridge

An overflow is an error, so it's incorrect to return valid: false.

If the implementation errors, it's not valid or invalid. The validity can't be determined because the schema/instance can't be processed. The error itself is the correct outcome.

I catch these cases and mark them as "inconclusive" when I'm running the tests. It's not a failure of my code that .Net (the platform I'm running on) can't handle the values, so I don't consider it a test failure.

I agree that these cases should be in the optional tests, though.

@Julian
Copy link
Member

Julian commented Dec 30, 2021

@jimmylewis a PR to move these is definitely welcome.

@karenetheridge
Copy link
Member

@jimmylewis a PR to move these is definitely welcome.

see #538.

@Julian Julian changed the title Could we please limit bignum tests to the optional suites? multipleOf contains incorrectly optional (bignum dependent) tests Jun 30, 2022
@Julian Julian added the bug A test is wrong, or tooling is broken or buggy. label Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A test is wrong, or tooling is broken or buggy.
Projects
None yet
Development

No branches or pull requests

5 participants