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

beef up bignum tests #378

Open
awwright opened this issue May 28, 2020 · 9 comments
Open

beef up bignum tests #378

awwright opened this issue May 28, 2020 · 9 comments
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.

Comments

@awwright
Copy link
Member

The "bignum" tests don't appear to be testing much. Most of them are missing boundary tests, and tests on either side of the boundary.

For example, the test for "exclusiveMinimum" should include a test above, below, and exactly equal to the argument.

Tests also be added for "enum" and "const" where applicable.

@gregsdennis
Copy link
Member

What are "bignum" tests testing? Is this a requirement of JSON Schema or of JSON compatibility?

@awwright
Copy link
Member Author

awwright commented May 28, 2020

It tests support for arbitrary-precision decimals, as opposed to binary64 numbers (i.e. a double-precision float like ECMAScript uses).

Presumably, the tests should be written so that if you're using a binary64 float to store numbers, there will be at least one failing test for each schema.

It technically is a requirement (JSON Schema is designed to test the document itself, not how it's stored in memory), but the vast majority of implementations and uses get by just fine with nothing more than a binary64 float, so for practical reasons, tests of arbitrary precision goes in optional/.

@gregsdennis
Copy link
Member

It tests support for arbitrary-precision decimals, as opposed to binary64 numbers (i.e. a double-precision float like ECMAScript uses).

Support for arbitrary-precision numbers is required by JSON, not JSON Schema.

Testing this is like testing for arbitrary string length. It's an inherited requirement of working with JSON rather than something the spec states.

@karenetheridge
Copy link
Member

It would be reasonable to put some tests of bignums into the optional/ directory, surely? At the least, it makes an implementor aware of these cases and they can decide whether to support them or not.

@awwright
Copy link
Member Author

@gregsdennis Yes. That supports the need for this test, doesn't it?

@karenetheridge Yes, the test file is already found in optional/bignum.json

@gregsdennis
Copy link
Member

gregsdennis commented May 28, 2020

No... I don't test the ability to add numbers with the + operator in .net. It's inherent.

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). If you want to start a JSON test suite, this is a perfect test for that.

However, JSON recognizes that support for arbitrary-precision numbers is limited to the environment and framework on which it's running. JSON Schema also inherits that caveat. At best, this needs to be an optional test, but I don't think it belong because it's not explicitly testing a requirement of JSON Schema.

@karenetheridge
Copy link
Member

I support adding more tests to optional/bignum.json. Otherwise, it could be very easy for an implementor to overlook that these values exist in the JSON data model, and since many (most?) JSON decoders would treat these values differently, the need for JSON Schema to treat these as if they were any other number necessitates special treatment in the JSON Schema implementation as well.

@awwright
Copy link
Member Author

awwright commented May 29, 2020

@gregsdennis I'm not sure I follow your argument then... JSON Schema normatively includes JSON, and is defined to parse JSON documents (i.e. application/json documents).

When we have a test like

true
{const: true}

What do you think this is doing? It's testing that the JSON document is valid against the schema.

Same thing for numbers:

3
{ minimum: 2 }

and even very large numbers:

10000000000000000000000.00000000000004
{ exclusiveMinimum: 1e22 }

And I believe we've established, if there's a reason that more than a few implementations might not be compliant because of coding environment, we can add tests for that. For example, we have tests for empty keys in objects (since some environments want minimum one character in a key) and regular expressions (since some implementations don't support the same dialect, or unicode).

Also, while JSON describes the format of numbers, it doesn't proscribe how the value must be made available to applications. JSON Schema does.

@gregsdennis
Copy link
Member

gregsdennis commented May 29, 2020

I'm not sure I follow your argument then

The examples you showed aren't testing the JSON, per se. They're testing the schema's ability to match and compare JSON values assuming the implementation handles it. Precise numeric values is something that JSON concedes is a limitation of implementation, so I'm not sure that it's JSON Schema's responsibility to test for it. You'd want a JSON Test Suite to do that.

Empty keys would be the same: it's a limitation of the JSON support rather than of the JSON Schema support. It's a fine line, but it's still there.

Regex support, on the other hand, is something we should definitely test, since JSON just sees a string.

it doesn't proscribe how the value must be made available to applications

I'm not sure what you mean here.


I don't know. If these are optional anyway, fine. It just seems weird to me to test the underlying technology rather than the functionality we've defined.

@Julian Julian added the missing test A request to add a test to the suite that is currently not covered elsewhere. label May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.
Projects
None yet
Development

No branches or pull requests

4 participants