JSON-Schema-Test-Suite
JSON-Schema-Test-Suite copied to clipboard
json-schema-org
Re-add the future keywords tests.
The intent of them is not to express that an implementation might wish to recognize these keywords.
The intent of them is precisely the same as the original intent -- to allow an implementation to test itself for not recognizing new keywords. Such an implementation should enable these tests, as it is free to do so, and these tests will help ensure it does not accidentally leak newer keywords backwards in time if it doesn't mean to.
All of the previous discussion was strictly centered on not putting these in the required directory.
The reason is that all drafts allow implementations to add extra keywords. So a compliant implementation may indeed decide it actively wants to implement new keywords. Then this file would not help them, and they wouldn't run it. They would presumably love a version of this file that did contain all the future assertions as correct for future drafts, and in the future we could provide one. But given no implementations really do do this, at least as far as I'm aware, and given that this version of the file is here and written and more common, it would seem perfectly reasonable to have it and hope for the other version if or when someone wants it.
Further discussion is in #383 and later #559.
This reverts commit f605fbfe463333b72ead7a156195f1317f72d45f.
CC @handrews @karenetheridge let's discuss here so we're not using a closed PR perhaps?
I am opposed to merging this at this time.
- I think this may conflict with our decision to add $schema keywords for drafts 2019-09 and later. If we are testing strictly against a metaschema, I do not see how extra keywords/vocabularies could be allowed. This is something that should be discussed in the context of that discussion, and whose conclusions should documented in an ADR.
- "Then this file would not help them, and they wouldn't run it." -- there currently isn't any sort of organization in the tests, or clear indication as to when tests could or couldn't be run, other than an "optional" label which we let implementors interpret as they will. I'd like to get this sorted out before we add more categories of "optional" tests that force people to try to divine their meaning.
Can you elaborate how this conflicts with adding $schema? I've cited sections of the spec in #376 which conflict with your interpretation. Do you have sections which support it?
As for the second point, I agree it'd be nice to get to tackling optional, which is why I'd like to get changes merged and out of the way in order to get to it, and especially to illustrate the scenarios we need to support. The notion we'd need to potentially support conflicting optional tests isn't new to me, it's been mentioned in previous issues, but it seems it was non-obvious to others --
In short what new is unclear about this PR that isn't already unclear about existing tests like zeroTerminatedFloats or bignum or others?
Would you feel more comfortable with a comment indicating which implementations should run this file?
Putting this with other "optional" tests might cause some amount of confusion. Maybe we can create a new class of "behavior" tests.
For example, in https://cache-tests.fyi/, "behavior" tests are those with grey "+" or "-" symbols, where the cache behavior may legitimately vary depending on the needs or environmental conditions of the cache.
@Julian
As for the second point, I agree it'd be nice to get to tackling optional, which is why I'd like to get changes merged and out of the way in order to get to it
Is it really necessary to have this merged? We know that this is case to sort out, whether it's checked in or not. If the current state of affairs is too confusing to reach agreement, can we just table this until we've made enough progress to agree on where this ought to go?
I generally don't like having PRs and issues hanging around. But perhaps we're better off looking at this as a dependency.
I certainly would rather do so than keep talking about trivial changes. There's nothing different about these tests than existing optional ones, but I won't keep pushing to get them in. We can come back to them later.
to allow an implementation to test itself for not recognizing new keywords.
I'm not sure what this PR is testing.
Possible ways how a validator could behave would be:
- Refuse to compile schemas with unsupported keywords
- Ignore those keywords and let everything pass as
true(ugh, but spec-compliant) - Implement those so those behave as in some spec other version (still spec-compliat)
- Some weird case of behavior not covered by 1-3.
What does this PR test for when it expects $dynamicRef to fail validation of some input in draft4 but prefixItems to always pass on anything?
I don't believe 1 is spec compliant, but would have to double check we still have language requiring unknown keywords to be ignored.
2 is what this PR was meant to cover, and the fact that 3 is equally spec compliant was why I removed them from required and moved them here (so authors of implementations that want 2 can still run them -- in theory we could offer versions of these for mutually exclusive 2-or-3 behavior and implementers would use whichever they wanted).
What does this PR test for when it expects $dynamicRef to fail validation of some input in draft4 but prefixItems to always pass on anything?
Sorry, just to add -- this is just a copy paste error and meant to be all true, the point is to test 2, though as you say, implementations are free to do something in between, and many such things are not "weird" -- e.g. choosing to support only some future keywords and not all.
@Julian re: 1 -- a validator that has either opt-in or opt-out on that would still be spec-compliant, I think ).
Sorry, just to add -- this is just a copy paste error and meant to be all
true
Ah, so that was a bug. But that also implies that this wasn't actually tested on any real implementation )
a validator that has either opt-in or opt-out on that would still be spec-compliant - @ChALkeR
The spec says that implementations MUST ignore unrecognized keywords. This needs to be the default for the implementation. If an implementation does not do this without configuration, then it's not compliant. (So your 1 is not spec-compliant behavior.)
An implementation could have a configuration setting that enables a "strict" mode. That would be opt-in.
The spec says that implementations MUST ignore unrecognized keywords.
It's SHOULD ignore up through 2019-09, and then SHOULD collect as annotations in 2020-12 (and if not, SHOULD or MUST ignore, but we forgot to actually say that - it's kind of implied, though, as implementations that don't collect annotations ignore them).
Reading back over this after #574, this gets to the same question of what sort of "configuration" (including not just configuration options but also additional things not required or explicitly allowed by the spec) is appropriate for testing compliance. I know @Relequestual has some thoughts on this which probably warranted a discussion being opened somewhere (as it's a topic that touches on both the spec and the test suite).
In the meantime, @Julian is correct that implementations can add arbitrary non-vocabulary keywords and have them always be on. They cannot enable random vocabularies, but that's irrelevant for draft-07 and earlier. For 2019-09 or later, it matters whether the keywords are implemented as a vocabulary or not, but we should just rely on the $vocabulary tests to indicate that vocabularies not present in $vocabulary cannot be enabled.
So we should assume that any "future" keywords that show up as always-on are non-vocabulary keywords and therefore allowed. The rationale in #574 does not apply here, as we are not trying to make a distinction between implementing the standard (which doesn't have these keywords in it, by definition) vs implementing something that just happens to look like the standard. In this case, it's automatically a "happens to look like a future standard" thing.
Like the backwards compatibility dependencies test as discussed in #590 , I'd put these in the proposed additional directory, as there is no clear relationship between the functionality and the specification being tested (the past and future specifications don't come into play for this determination, I think).
As I understand it, the difference between this situation and the required unknownKeyword.json test is that that test is valid for any unrecognized extension keyword name, and the keyword in it could therefore be renamed to something even less likely to be present if we ever had trouble with a collision. On the other hand, these tests (and the $vocabulary tests) are about specific keywords, so a collision could not be solved by renaming. Therefore they go to additional (in my opinion, this is still being debated in #590).