-
-
Notifications
You must be signed in to change notification settings - Fork 238
Add if/then/else test cases for issue #767 #804
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
Add if/then/else test cases for issue #767 #804
Conversation
karenetheridge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One test case is wrong.
I'm concerned that if it passed for your implementation, you have other issues you need to worry about! e.g. the properties keyword is never invalid when the data instance is not an object.
Also, these tests should be copied to all the version directories where they are relevant (if/then/else was added in draft7).
jdesrosiers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just the third test is sufficient. Or, just the first two. But, the first two and the third seem to be testing the same thing.
Thanks for the feedback — that makes sense. I see how the third test is logically implied by the first two and doesn’t introduce a new failure mode. I’ll remove that case and update the tests accordingly. I’ll push an update shortly. |
Thanks for raising that. I didn’t add new tests for not because handling of false with not is already covered in the existing not.json tests, which assert that false is treated as a schema that always fails (rather than {}). This PR was scoped specifically to fill the gap for conditional keywords (if / then / else), where equivalent coverage didn’t previously exist. If you’d prefer additional or cross-referenced coverage for not in this context, I’m happy to add it. |
|
All checks are passing now. I’ve removed the redundant test case from all three files. Please let me know if anything else needs adjustment. |
|
hello @jdesrosiers
|
This PR adds additional tests for correct handling of the
falseschema withinconditional keywords (
if,then,else), as described in Issue #767.What these tests cover
ifmatches,then: falsemust cause validation to fail.ifdoes not match,else: falsemust cause validation to fail.thenandelsearefalse, all data must be invalid regardlessof the branch taken.
falseis treated as a schema that always fails, matching JSONSchema semantics, rather than being interpreted as
{}.These tests fill gaps in draft2020-12 conditional logic coverage, addressing the
behavior described in Issue #767.
Fixes #767.