differentiate tests of optional behavior from undefined behavior

[notEthan]

tests of undefined behavior should be differentiated from defined-but-optional behavior. this PR moves the test for a $ref pointing at a location that is not known (optional/refOfUnknownKeyword.json) to contain a schema to a new folder undefined, instead of the folder optional. this test's behavior is undefined per json-schema-core 8.2.4.4

having a reference target in such an unrecognized structure cannot be reliably implemented, and the resulting behavior is undefined.

I think there is value in keeping tests of undefined behavior - as an implementer, I want to see how my implementation handles any abuse of its inputs that a user may throw at it. I intend to introduce more tests of undefined behavior, depending how this PR lands. but those should certainly be separate from defined behavior.

this PR is a draft, pending:

• feedback
• checking pre-2019-09 to brush up on whether this behavior used to be defined, and performing the same move
• check how the existing test runner might handled the directory change

[Julian]

What's the value in distinguishing these, isn't practically speaking the difference the same?

The optional directory doesn't replace the spec obviously, it's more for files that an implementation may-or-may-not choose to run.

I'm conscious of overcomplicating the layout of the suite repo since it makes it harder for implementers to parse it (e.g. look, the thing I have is already 239 lines long).

Not saying this obviously does fit that (overcomplicating), but that's the counterargument against e.g. "clarity".

[notEthan]

the difference between behavior that is defined by the spec and behavior that isn't, seems to me pretty fundamental to a test suite of that spec.

the rest of the optional tests are defined behavior. if/when an implementation supports the specified functionality, it must pass the tests if it conforms to the way the spec says to do it. this doesn't apply to undefined behavior. there is no spec to implement and test, in fact the spec goes out of its way to say it's unspecified.

I might go further than this change and remove the expected outcomes (valid: true / valid: false) from the test, since that outcome is the result of behavior that is explicitly not defined by the spec.

defined behavior belongs in the main test suite, whether it's optional or required. undefined behavior should be treated differently.

[Julian]

Sure that makes sense given the "go further" piece -- so I could understand removing the test in that case (from 2019-09 only would be my preference -- where I think that language was added explicitly because some implementations e.g. mine implemented it because the spec didn't say not to) if the point is "the spec explicitly calls this undefined behavior".

Having some tests that test beyond just valid/invalid is so far out-of-scope for what we had but I agree it'd be nice to have. I've never seen that as belonging in this suite specifically, more in some pathological suite with things like circular references (which are to me the canonical example), but where exactly that lives I guess isn't a big deal, maybe here.

My point though is I don't really like the specific move from this PR -- to me, either they should be moved to a pathological place that doesn't deal with valid/invalid (if the goal is to use them to test edge cases beyond validation), or if there is some intention that an implementation may want to implement that behavior (as, again, to me, was the case pre-2019-09), that was what I was responding to (that there's not much of a difference there between "optional and defined" and "optional and undefined", at the end of the day the difference is just "you may or may not want to include this file in your implementation's runner")

[notEthan]

that's cool, I have no attachment to the particular move I made here. I'm happy to see this test outside of tests/dreft2019-09 entirely. I think we're on the same page on the main point that testing of undefined behavior doesn't go in the main test suite.

so, where should it go? should it stay in this repo? I would like to add other tests of undefined behavior, including circular refs and other things. that would be a broadening of scope for the test suite, but I think it's worthwhile to include.

I've amended this to instead create a new folder from the root at /tests_undefined/draft2019-09, which seems like a reasonable place if this is to stay in this repo. if pathological tests are too far outside this repo's scope, I could change this to just delete the test instead.

[Julian]

Cool! So, maybe before we decide on a concrete location for where these go, at least for me, it'd be helpful to put some examples down for the kinds of semantics we'll need to represent, and what we'll need to say about them.

E.g., what do you envision we want to "say" about a the other undefined behavior tests you have in mind?

Is there anything more we can say other than just denoting the schema and instance and then implementations "blindly" run them? E.g. ideally for the circular reference test you want to be able to express that a validator may optionally want to have raised a non-validation-related error in that case. I suspect the same thing may apply to other cases -- e.g. here I think that's the case too, a validator may choose to decide to implement the behavior here and call the result valid, or may choose to raise an error --

Obviously for these pathological cases the point is less to assert against them and I think more to signal to implementers of the pathological suite what assumptions they can make about running them -- e.g. if we stick a circular ref test in there with no signal a given implementation may loop forever.

(All the above just being rough thoughts from thinking about this a few times over the past few years though)

Maybe though (again all rough) just calling the folder pathological-examples but yeah trying not to bikeshed too much yet until I have thought through the semantics enough.

[Julian]

(We're going to address the entire optional/ folder at once, so going to close this for now, but feedback will be welcome on how to best do so).