Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: consolidate JSON schema tests using keys #116

Closed
wants to merge 3 commits into from

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented May 9, 2022

See #112 (comment) for more context. As new JSON schemas and associated validation samples are about to be introduced both for existing and upcoming specification, the initial layout for the JSON test files need to be revisited.

This PR explores an alternative proposal to the top-level valid, valid_strict, invalid folders introduced in #87 #92 and is inspired by the JSON schema test suite - https://github.com/json-schema-org/JSON-Schema-Test-Suite#structure-of-a-test.

Each JSON file can annoted with a top-level valid and/or recommended key which indicates whether the metadata is minimally compliant or fully compliant with the specification. The Python tests are updated to use a single glob.glob and collect all *.json files, select the schema to use based on the presence of the multiscales key and use the valid/recommended keys to test the validate outcome.

Using this approach, all JSON files associated with the image.schema can be simply grouped into the examples/image subfolder, examples associated with plate.schema would be under examples/plate etc

sbesson added 2 commits May 9, 2022 21:06
Rather than splitting files into individual folders, this commit makes
use of top-level keys (valid, recommended) to indicate whether a file
should validate against image.schema and strict_image.schema

The unit tests are updated to use a single glob and detect the
appropriate schema based on the presence of the multiscales key
@sbesson sbesson requested review from joshmoore and will-moore May 9, 2022 20:28
@will-moore
Copy link
Member

Looks good 👍

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Slightly counter-intuitive to add false flags, but I think I can understand.

@sbesson
Copy link
Member Author

sbesson commented May 10, 2022

Looks good. Slightly counter-intuitive to add false flags, but I think I can understand.

Do you mean the keys should be expected to have different default values? Or that the new keys should be defined in all snippets whether they are true or false (as in the JSON schema test suite)?

@joshmoore
Copy link
Member

My reading of https://github.com/json-schema-org/JSON-Schema-Test-Suite#structure-of-a-test made me expect rather than:

  {
    STUFF ...
    "valid": false
  }

to have:

{
    "tests": [
        {
            "data": {
                STUFF...
            }
            "valid": true
        }
    ]
}

@sbesson
Copy link
Member Author

sbesson commented May 10, 2022

True, the JSON schema test suite format wraps the JSON to test within a data key and each JSON test within a tests array. Overall, I thought this would be a very good next step especially for grouping more examples e.g. all the invalid ones.

I hesitated to implement it as part of the proposal primarily because it breaks the inclusion logic of #98. If the format sounds valuable, I can look into refactoring these examples one step further to follow the JSON schema test suite format more closely.

@joshmoore
Copy link
Member

I hesitated to implement it as part of the proposal primarily because it breaks the inclusion logic of #98

Ah, understood. But we'll need to strip valid, etc. out, right? And if that eventually conflicts with a spec field?! 😄 Perhaps we update the data field to point to the actual content? But then you are back to needing to decide whether or not to put "invalid" in the directory structure.

@sbesson
Copy link
Member Author

sbesson commented May 10, 2022

Ah, understood. But we'll need to strip valid, etc. out, right? And if that eventually conflicts with a spec field?! 😄 Perhaps we update the data field to point to the actual content? But then you are back to needing to decide whether or not to put "invalid" in the directory structure.

Maybe that's the bit of explanation that was missing in my answer to @will-moore's question above. To avoid stripping of the valid field from the official examples, the current implementation assumes "valid" is true by default unless overriden
I certainly see this is extremely fragile and adding an extra data level allows to annotate the test with descriptions and states without modifying the content. I'll give it a try

@sbesson
Copy link
Member Author

sbesson commented May 12, 2022

Summary of a quick discussion with @joshmoore. Having spent more time implementing #116 (comment), I am increasingly considering the following organisation which should bring a good compromise between extensibility and support for new specification schemas and usability:

  • primarily, split the content currently stored under examples under 2 categories:

    • the JSON files used as official representative examples. All of these should be valid and some of them can be embedded in the offical specification. Eventually all JSON snippets embedded in the specification should be extracted to examples and be validated
    • the JSON files used for validating the JSON schemas. There is no really incentive to keepting those under examples and this creates confusion
  • migrate all the test JSON samples under {0.4,latest}/tests and use the structure defined in the official JSON schema test suite i.e.

    [
      {
          "description": "suite of tests",
          "schema": {
              "type": "image"  // indicates the schema to use for the tests
          },
          "tests": [
              {
                  "description": "invalid scenario",
                  "data": {}, // the JSON to test
                  "valid": false,
              },
              {
                  "description": "valid but not recommende",
                  "data": {}, // the JSON to test
                  "valid": true,
                  "recommended": false
              }
          ]
      },
  • for the official examples, include a specification container and keep the valid/valid_strict separator for now:

    examples/
      image/
        valid/
      plate/
      well/
      ...
    
  • update test_validation.py to

  • add/update README to explain how to extend/update these tests as specifications and schemas are added/modified

@sbesson sbesson closed this May 12, 2022
@sbesson sbesson deleted the validation_test_annotations branch May 26, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants