json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
627 stars 208 forks source link

add non-string uuid tests #603

Closed santhosh-tekuri closed 2 years ago

Julian commented 2 years ago

These are already present here in the non-optional tests (because this isn't optional behavior) -- though I'm not thrilled with how that changed, so possibly you know they're there and are proposing these for the same reason I kind of dislike the current layout -- had you seen them?

santhosh-tekuri commented 2 years ago

but those cases are again tested inside optional for others say email.json etc

santhosh-tekuri commented 2 years ago

another reason is:

in my library I have isUUID(Object), in non optional tests, my implemnataion does not call this method. with optional this method is called but always with strings (I want isUUIID method to be called with non-strings)

gregsdennis commented 2 years ago

Are these valid UUIDs, though? According to the spec, uuid expects only strings:

A string instance is valid against this attribute if it is a valid string representation of a UUID, according to [RFC4122].

santhosh-tekuri commented 2 years ago

@gregsdennis as per json-schema-spec, string formats should return true for non-string values

gregsdennis commented 2 years ago

If that's what you're trying to test, then those are already present: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft2020-12/format.json

These optional tests are for testing format validation, and for uuid only strings apply.

Julian commented 2 years ago

in non optional tests, my implemnataion does not call this method.

I'll have to look back to see if this was discussed, but this is the reason I don't like the layout personally (I don't believe I was involved in moving these but watch me be lying and turn out to be the one who approved the PR :)

But yes, in my implementation this is the same, and the fact that we removed these tests from optional means code paths are not properly tested.

(I'm on mobile for a few hours but probably finding the PR that moved these is a good next step to see what the thoughts were then)

gregsdennis commented 2 years ago

These optional tests are for testing format validation, and for uuid only strings apply.

Note also that a meta-schema with the format-assertion vocabulary isn't being used here, meaning that format validation should not occur unless the implementation is configured to do so. When I run the tests in my repo, I explicitly configure for format validation for these particular tests.

Julian commented 2 years ago

Note also that a meta-schema with the format-assertion vocabulary isn't being used here, meaning that format validation should not occur unless the implementation is configured to do so. When I run the tests in my repo, I explicitly configure for format validation for these particular tests.

Right! That's the point even -- imagine you have an implementation with a "format-on-or-off" switch -- if your implementation supports drafts pre-2019, it's highly likely you do (and that it's independent from $vocabulary).

"Historically", when you run tests in this repo, you run with format turned off for all tests in the root directory of a draft, and then you secondly perhaps run with it on for the optional format tests if you support format-on, but then you may only run the optional/format directory and not the whole suite a second time.

But now (that tests in optional/ don't test non-string values), when you run with your implementation in format-on mode, you never test a non-string value because that file no longer contains it! So if you have a bug that occurs only when presented with a non-string but with format on, you won't notice.

gregsdennis commented 2 years ago

when you run with your implementation in format-on mode, you never test a non-string value because that file no longer contains it!

If that's something you feel this suite needs to test, sure.

Julian commented 2 years ago

OK, I looked back and yeah weird, this does just look like an oversight.

All other files have this except for the URI format tests:

⊙  rg --files-without-match 'all string ' tests/draft2020-12/optional/format/                                                                                                       julian@Airm
tests/draft2020-12/optional/format/unknown.json  # this actually does contain these tests just with a different name
tests/draft2020-12/optional/format/uuid.json
tests/draft2020-12/optional/format/uri.json

@santhosh-tekuri can you add that there too (to uri.json across all drafts?)

santhosh-tekuri commented 2 years ago

@Julian added non-string uri tests

Julian commented 2 years ago

Great, thanks!