test a malformed pattern

[karenetheridge]

I noticed we had no tests for bad patterns.

[Julian]

For drafts pre-2019-09 this is (potentially) incorrect I think -- it depends whether the metaschema it's run under is validating formats or treating them as annotations or whatever. So for those this'd need to go in optional. For 2019-09 I don't remember what changed -- do you enable format via a vocabulary and then it has to be respected, or what?

(Also this is invalid JSON but probably you'll notice that part)

[karenetheridge]

I'm not assuming/expecting the metaschema to use a format validation. Even if no validation is done on the schema at all, it should still return a non-valid result at runtime.

[karenetheridge]

invalid json fixed -- and the test suite is now happy, so it looks like the json was just parsing badly before.

[Julian]

That may not be the case. An implementation is free to blow up entirely in this case if you give it an invalid schema and it isn't validating it.

[Julian]

(i.e. it's valid for the result here to be "my validator blows up" rather than returning invalid)

[ssilverman]

As well, assuming that “valid=false” for failed schemas changes the meaning of “valid” for the tests in that suite. It’s also bad practice to have meanings of fields change depending on other variables.

[ChALkeR]

Perhaps there should be another flag to indicate invalid schemas (apart from valid: false on tests), perhaps in a separate directory/subdir, where the impl is free to blow up on schema compilation (like it should imo)?

That would be different from valid: false, as the impl can either follow this and return false, because none strings match invalid pattern, or it can as well reject the schema alltogehter which should also be a valid behavior. Per schema, pattern should be a regex, and this is not a regex.

[karenetheridge]

Yes, we've been discussing this across a few different PRs and in slack. There hasn't been any consensus yet as to how to declare this in tests (with an extra flag? a new directory under optional/? both of these?) or how to document how implementations should deal with it.

[Julian]

I want to merge the outstanding PRs on tests in the "normal" category before addressing this. I.e. #354 and the PRs being split off of it. Let's get those merged, then we can hit cases that may require a structural change. Could definitely use more eyes reviewing those PRs so we can get them off the queue.

[gregsdennis]

FWIW, my implementation gives an (arguably incorrect) error, but doesn't blow up:

The string value was not a match for the indicated regular expression

(Interestingly, .Net doesn't think this is an invalid pattern.)

I think this is one of the things I had to account for in some other regex test where .Net didn't support something. I switch from deserializing the regex directly to just deserializing a string and lazily parsing the regex.

[awwright]

An invalid schema doesn't return either valid or invalid; the validity of the instance is indeterminate.

[karenetheridge]

@awwright obligatory: https://thedailywtf.com/articles/What_Is_Truth_0x3f_ :)

[karenetheridge]

Putting on hold until we can figure out how to test schemas that would cause an implementation to throw an exception / send up an error / do something other than return validated=true/false.

[Julian]

It's going to be more backwards compatible for users of the suite to put these kinds of tests in a separate folder (because one wouldn't know to reference the flag until after we add such a thing).

So there's seemingly little disadvantage to doing it in a separate folder which we can bikeshed on (neither-valid-nor-invalid-tests/); I think I once discussed some of this with @ssilverman and put some notes in a doc.

But yes we should decide how to handle that "once" and then do it here, agreed.

[awwright]

I don't anticipate many compatibility problems, depending on how the tests are written. Especially if the "tests" property is empty or missing, that should not pose a compatibility problem.

However, I think the most intuitive way to write this would be to use tests: null to indicate the schema itself is malformed:

[
    {
        "description": "enum range error",
        "schema": { "enum": 4 },
        "tests": null
    }
]