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

idea for expected failure tests #727

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis commented Apr 6, 2024

This PR adds an idea for tests that check failure scenarios. Please see the included readme for details.

(I'm pretty sure we have an issue for this. I'll come back and reference it, but I have look for it first.)

Looks like this starts to address #589.

@gregsdennis gregsdennis requested a review from a team as a code owner April 6, 2024 22:23
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's unfortunate that these tests would be separate from the regular tests, but from a backward compatibility perspective, this is is probably the best approach.

My first thought was that providing an instance wasn't necessary. Implementations that don't have a way to validate a schema without an instance can just hard code null or something for the instance. But, then I realized that there are some cases that may not get evaluated fully without an instance. For example, if the schema is for an object with a property that includes an invalid reference and you pass null as the instance, the property keyword would never get evaluated and the invalid reference wouldn't be caught. So, we'd have to take care to think about different possible implementation choices to make sure we use instances that produce the desired result.

@gregsdennis
Copy link
Member Author

Yeah, I ran through all of that same logic when putting this together.

@gregsdennis
Copy link
Member Author

There's also a question that I don't think is addressed by the spec: if the error in the schema isn't exercised by the instance, is the implementation still required to error?

For example, a bad items keyword with an object instance.

I don't think that the spec says anything about this case. If it doesn't, then we really can't provide a test for it. Maybe we can add an optional/ subfolder here, too.

@jdesrosiers
Copy link
Member

I agree that the spec doesn't say anything about that case. I'm not worried about covering pathological cases, so I don't think an optional folder is necessary. I think providing instances that will exercise parts of the schema that have errors is good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants