2020-12-26 08:55

differentiate tests of optional behavior from undefined behavior

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

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


  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答


  • weixin_39722025 weixin_39722025 4月前

    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".

    点赞 评论 复制链接分享
  • weixin_39909366 weixin_39909366 4月前

    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.

    点赞 评论 复制链接分享
  • weixin_39722025 weixin_39722025 4月前

    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")

    点赞 评论 复制链接分享
  • weixin_39909366 weixin_39909366 4月前

    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.

    点赞 评论 复制链接分享