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)

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.

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

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.

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

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.

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.

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.

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.

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.