[244] Add a bunch of meta-schema tests

[ssilverman]

This adds tests using the meta-schema as a schema. This does not test things
that cannot currently be guaranteed to be represented in a schema, for example,
normalized URL requirements and optional "format" for "pattern".

Closes #244

[ssilverman]

I hope I didn't get too many false/true validity tests wrong as typos due to faulty cut & paste.

[ssilverman]

Maybe I’ll split this up more in each file...

[ssilverman]

Ok, I added a second commit that groups everything better and also orders them as they appear in the spec.

[ssilverman]

This also now closes #301.

[karenetheridge]

hope I didn't get too many false/true validity tests wrong

Hopefully you ran them against an implementation first? 🐙

[ssilverman]

@karenetheridge I started testing them today now that my new validator Is just about done. I wrote one so I could better understand the spec and hence the tests. I’ve already found one test error.

[ssilverman]

Force-pushed to track changes in master.

[karenetheridge]

I didn't look through all these by hand, but I just ran them against my implementation with default settings and it flagged the regex issue I just commented about.

When this is ready to merge, please rebase against master and squash the commits together -- also can you (assuming you're doing the merging) merge it with a "merge commit" (the first option out of the three available), so it stays associated with the pull request in the commit history? thanks!

[karenetheridge]

This only fails to validate if formats are enabled as validators, not just annotations, which is not the default configuration -- consequently this should be moved into the optional/formats directory.

[ssilverman]

I'm not sure what you want here. "pattern" isn't the same thing as "format". "pattern" is not optional.

[ssilverman]

It appears, from your comments, that you're using your own validator to run the tests. That's fine, but if you're not already, try the others, because they may treat "pattern" differently, i.e. not as an "optional format", which is what your suggestion to move to optional/formats seems to suggest.

[karenetheridge]

You are validating against this in the metaschema:

    "properties": {
        ...
        "pattern": {
            "type": "string",
            "format": "regex"
        },
        ...
}

There is nothing to cause a failure here unless "format":"regex" is treated as a validation assertion.

[ssilverman]

Some references: https://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.3 says that Section 7 (format) and Section 8 (content) can be treated optionally as assertions and it doesn't mention anything about "pattern", which is in Section 6. The specific text:

The Section 7 keyword is intended primarily as an annotation, but can optionally be used as an assertion.

[karenetheridge]

The issue is not "pattern", but "format". "pattern" is the property being validated. The keyword it is being validated against is "format".

section 7.2.2 says "When the vocabulary is declared with a value of false, an implementation... MUST NOT evaluate "format" as an assertion unless it is explicitly configured to do so", which is exactly why some tests are split off into the optional/format directory, and no format-as-assertion tests exist in the main test files.

[ssilverman]

You're right about this. I must have, in one of my rebases, included this here. I'll move them to a different PR.

[ssilverman]

Ok, I was confused. I actually want this to be here, but I'll exclude from this set of tests until we resolve how to test un-testable schema words because of things like "must be normalized" and "format is optional".

[karenetheridge]

Leaving it out would be fine as well, although it would be fair to be included in optional/format/regex.json -- it should be as possible (or not) to validate as the other invalid case there.

[ssilverman]

That's not a bad suggestion, but then it's not testing the "pattern" keyword specifically, which is the explicit intent of those tests, even though it may use "format=regex" under the covers; that's why I'm hesitant to put it in the "format"-specific tests. It would be my preference to just leave this for a future PR pending some more discussion.

[ssilverman]

I'll only do "merge" if that's @Julian's preference. Otherwise I'll continue to do rebasing. And please don't try to mandate process again, unless you run it by @Julian.

[ssilverman]

I'll add: You may have missed that I've been rebasing against master regularly, above. No need to suggest that. And besides, it won't even let you merge anything if there's conflicts. I'm keeping on top of those.

[karenetheridge]

You are validating against this in the metaschema:

    "properties": {
        ...
        "pattern": {
            "type": "string",
            "format": "regex"
        },
        ...
}

There is nothing to cause a failure here unless "format":"regex" is treated as a validation assertion.

[karenetheridge]

Rebasing against master during a review causes some history to be lost (and unfortunately, also marks review comments as "resolved"), which is I think what @handrews was mentioning the other day.

If you were going to rebase anyway before merging, then that's great, and my comment was redundant! :)

[ssilverman]

No history is lost. All the history in a PR is accessible, even the ones that may appear lost after a force-push. Before I merge in a PR, my practice is to reduce to a minimum set of applicable commits. This keeps the commit history clean. This is also why my personal preference is to have the submitter do the merge (assuming trust) because there's a last chance to squash relevant commits into one or more, as opposed to squashing everything into one commit.

Edit: Maybe there's difference between different levels of GitHub, or maybe what I'm thinking is still accessible isn't really what you're referring to. I don't know, but I'm open to hearing suggestions to make reviewers' lives easier, including not force-pushing until the end. I was doing it as I go because in the past, PRs had been merged in by the reviewer, not giving the submitter time to squash multiple commits appropriately (to potentially more than one instead of only one).

[ssilverman]

Ok, since this has gone through a few iterations and a bunch of waiting time, I'll use this comment as a summary of what's happening in this PR. I just updated the description to be:

This adds tests using the meta-schema as a schema. This does not test things that cannot currently be guaranteed to be represented in a schema, for example, normalized URL requirements and optional "format" for "pattern".

This PR adds tests that can be tested currently, and tests that cannot be tested currently will be pushed to a future PR pending some more discussion on how we can add those tests.

[Julian]

I am obviously still behind, but just clarifying the "policy" -- as usual this is just de facto (it's what I do on all other repos I manage/maintain, as well as $WORK, etc. -- it may not be what @Relequestual / @awwright / Henry do on other repos, which does not mean I disagree! I just didn't coordinate with them, so we can reconcile if need be or write things down if need be, whatever helps, but laying it out here):

• the only hard-fast policy is "do not squash when you merge to master if there are multiple units of work in your branch" -- i.e. always create a merge commit, and don't mash work together if it was done separately, so merging to master should create 2 or more commits.

The soft policy is I personally don't generally squash things if each commit does one thing. But that much is up to your personal taste if it happens on a feature branch -- if you want to squash fixme commits that's fine with me (though as I say, generally I'll only do that when the initial commit is "defective" for what it's meant for, not if additional self contained pieces are added to it, but fine if others do what they usually do).

The medium-to-hard tangential policy is also "keep commits small and self contained" -- 200 lines or so is the max. I generally will not personally review things that are much larger than that -- once or twice I'll give a free pass :) but after that I just won't do it and ask to break it into smaller self contained chunks that are each independently 200 lines or less. In this repo, I'll also ignore if it's copy pasting 200 lines across draft versions.

And yes, I generally do "submitter commits / merges" after approval (which is as simple as any person with commit bit saying "you are good" or hitting green buttons or whatever they feel like doing).

And yeah again :) all the above is meant just to facilitate, so we can of course discuss or reconcile or write this down if there's other preferences or if I've contradicted what we do in the spec repo (which I don't contribute to as much as I should, so I don't know the rules there!).

[Relequestual]

We mandate very little.
@karenetheridge is impressive for even attempting to review this. I wouldn't.
I MIGHT consider reviewing it if there were many commits, but there's just too much here for someone to sensibly review.

I generally find if it's going to take more than 20-30 minutes to review, you'll have to really convince me it's worth my time.

That being said, @Julian has ownership over this repo. Pretty much what he says goes because he puts in monumental effort, and we got to support that as much as possible.

[jdesrosiers]

This was too much for me to review manually, but I did run the tests through Hyperjump JSV and they all passed, so it looks good as far as I can tell.

[ssilverman]

I’m working on simplifying this PR into smaller chunks, starting with some README updates.

[Julian]

@ssilverman just in case you happen to be around -- I'm going to start pulling additional chunks out of this PR to merge. Thanks again for sending it and sorry it took so long.