Skip to content

Add validation suite checker and schema for the new validation/ directory#912

Open
codefromlani wants to merge 2 commits into
json-schema-org:unifyfrom
codefromlani:validation-checker
Open

Add validation suite checker and schema for the new validation/ directory#912
codefromlani wants to merge 2 commits into
json-schema-org:unifyfrom
codefromlani:validation-checker

Conversation

@codefromlani
Copy link
Copy Markdown

Following your suggestion to use a new validation/ directory to avoid conflict with the existing tests/, this adds:

  • validation-test-schema.json — schema for files in validation/,
    extending test-schema.json with the compatibility field
  • check_validation_suite.py — sanity checker for validation/,
    similar to bin/jsonschema_suite

@codefromlani codefromlani requested a review from a team as a code owner June 1, 2026 13:23
Copy link
Copy Markdown
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Here are a few things I found since our call earlier.

You need to update tox.ini to use check_validation_suite.py instead of the old script. Then the ci check will happen properly.

This doesn't take into account the v1 tests. The v1 tests cover future releases that we don't necessarily have a validator for. Currently there are several case where the checks will fail if we don't have a validator for them. I think if we don't have a validator needed for a test, then we should just skip that check.

Comment thread validation-test-schema.json Outdated
Comment thread check_validation_suite.py Outdated
Comment thread validation/check_validation_suite.py
Comment thread check_validation_suite.py Outdated
Comment thread check_validation_suite.py Outdated
Comment thread check_validation_suite.py Outdated
Comment thread check_validation_suite.py Outdated
Comment thread validation-test-schema.json Outdated
Comment thread validation/validation-test-schema.json
Copy link
Copy Markdown
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

I left a few comments based on your last push. You still need a way to handle future releases.

"additionalProperties": false
},

"$defs": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In draft-07, it should be definitions, not $defs.

"description": {
"description": "The test description, briefly explaining which behavior it exercises",
"type": "string",
"maxLength": 69
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"maxLength": 69
"maxLength": 70

"description": {
"description": "The test case description",
"type": "string",
"maxLength": 149
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"maxLength": 149
"maxLength": 150

Validator.check_schema(schema)
except jsonschema.SchemaError as e:
self.fail(
f"Schema is invalid under draft {dialect} "
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message won't make sense for future releases because we won't be calling them "draft" anymore.

Suggested change
f"Schema is invalid under draft {dialect} "
f"Schema is invalid under release {dialect} "

@codefromlani
Copy link
Copy Markdown
Author

I left a few comments based on your last push. You still need a way to handle future releases.

I have guard against missing validators with if dialect not in DIALECT_VALIDATORS: continue

Or, is there another approach to handle it?

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.

2 participants