Drafts 4,6,7: add complex types tests for oneOf, anyOf

[deBhal]

This PR adds tests using complex types for allOf and oneOf, similar to the ones for allOf.

The background here is that I broke a bunch of downstream projects (mafintosh/is-my-json-valid#149) with a change that passed this suite.

These additional tests would have caught my mistake there, so this seems like a useful way to say sorry.

I've got the sanity tests passing as well as running them through the is-my-json-valid test suite, and I've tried to be conservative - these are close copies of the existing allOf tests.

That said, I've got a very fresh respect for downstream consequences, and there's a long list of consumers here, so I'd love some guidance on managing those.

[handrews]

@deBhal These look good, could you please also add them to draft6 and draft7 (unless they are already covered there and we forgot to backport in which case oops)? Nothing changed about the behavior of these keywords in the last several drafts so a simple copy and past should be sufficient (one day maybe we'll do something to avoid duplication...)

[handrews]

Not sure what's going on with the test failure- I thought we had sorted out the meta-schema stuff. @epoberezkin @Julian ?

[Julian]

Looks like they're coming from ajv.

[deBhal]

That error is caused by Ajv and the metaschema disagreeing on $id vs id. ajv = new Ajv({format: 'full', meta: false, schemaId: 'auto'}); makes everything run, but that's probably not an adequate fix here.

[deBhal]

could you please also add them to draft6 and draft7 (unless they are already covered there and we forgot to backport in which case oops)? Nothing changed about the behavior of these keywords in the last several drafts so a simple copy and past should be sufficient

No problem.

I'm just taking your word that the the spec hasn't changed, but the simple copy and paste seems fine.

I do need to add the schemaId option, but the tests run clean with that, and it doesn't seem like there's any connection from these tests to the ref resolution.

I had a quick look as I went through to confirm that they do make sense there, and I didn't see anything that would make them redundant either.

[handrews]

I'm just taking your word that the the spec hasn't changed, but the simple copy and paste seems fine.

I was pretty heavily involved in drafts 6 and 7 so I'm pretty sure I'm right :-) Also, ajv supports those drafts so if we can get the Travis CI build working again it will check them.

[deBhal]

It's not clear to me what the correct behaviour should be, but I'm happy to implement the fix for these tests (in a separate PR) with some guidance.

Ajv will happily accept id or $id we just need to tell it which for each draft, and updating the meta-schemas to match should be trivial.

[epoberezkin]

@deBhal I will update

[epoberezkin]

@deBhal It's fixed now, all these tests now will pass if you rebase the PR on top of the current master.

[epoberezkin]

I think we discussed it previously somewhere in issues, but I believe that this test suite exists more for the purpose of illustrating and disambiguating the keywords behaviours rather than trying to establish that the implementation is fully spec compliant.

While the former is achieved by the existing set of orthogonal tests, the latter is impossible in principle - passing this tests suite is required for an implementation to be spec compliant but it can never be sufficient, however many tests we add here.

Validators usually have their own additional tests that cater for various edge cases that are different between implementations.

It may be worth considering kicking off another repo or a separate folder here with extra tests to make it clear that unlike the existing test suite it has the purpose of catching various edge cases of validators.

Such tests could actually be grouped by the validators authors that added them - it this case we won't have to worry about some possible overlap between them.

@handrews @Julian what do you think?

[Julian]

@epoberezkin hmm, If you're referring to comments I made in some other recent issues, I was referring specifically to other RFCs that the spec implements. In other words, I think that the test suite should not try to encapsulate all of the RFC on dates -- that should be captured in a test suite designed specifically for the date RFC.

For validation (and JSON Schema more broadly though), I think the opposite is true :) -- this exactly is the suite for ensuring that implementors don't make mistakes, so I think it should include any test that an implementor can potentially err on (and if one implementor erred on it it might be at least some evidence that others could / would).

I definitely agree that some implementors add additional tests for other behavior, but e.g. mine does not add any additional test that could not go upstream here. As an example, it tests to ensure that users can decide that the JSON Schema type "number" can be redefined to include the Python type Decimal. But nothing that could be cross-language should be hidden in my test suite I think, I've upstreamed anything that'd be runnable elsewhere.

(And really all this needs clarifying, which is why @handrews [correctly] asked me to write up #204)

[epoberezkin]

Ok then. I have quite a few tests that were added over time in the process of fixing various issues but most of them always felt very specific to my implementation. It can't hurt too much having overlapping tests here I guess.

In any case, I think it's worth adding a note in readme that "compliant" => "passes all tests", but not the other way around.

[Julian]

Yeah, agreed, don't think we'll ever be perfect, much as we might hope...

[deBhal]

@deBhal It's fixed now, all these tests now will pass if you rebase the PR on top of the current master.

@epoberezkin: The tests do indeed pass now, thanks!

Are there any other blockers here I should address?

I think we discussed it previously somewhere in issues, but I believe that this test suite exists more for the purpose of illustrating and disambiguating the keywords behaviours rather than trying to establish that the implementation is fully spec compliant. [...] the latter is impossible in principle

That's true, but it's still generally good to move towards impossible ideals as far as is practical :)

With these specific tests I have an obvious bias, but they seem justified.

Every implementation will need something very like recursion, so it's very broadly applicable, and while we can't test arbitrary depth, we can test a significant amount of that functionality by adding that second level.

It's already there in allOf, and there are strong parallels with anyOf and oneOf.

It may be worth considering kicking off another repo or a separate folder here with extra tests to make it clear that unlike the existing test suite it has the purpose of catching various edge cases of validators.

I can see a lot of value in a some tests that "try" to break implementations with bigger complex examples and/or gotchas, but I think they'd need to be well curated.

In particular, adding validator specific tests seems a bit dissonant. Validators will have their own test suite that would be more convenient for them to maintain, and the tests themselves are almost by definition not so widely useful.

[epoberezkin]

@deBhal Somebody needs to review, that's it.

[handrews]

Since it seems like @Julian and @epoberezkin are both generally fine with this now, I'm comfortable merging it.