-
Notifications
You must be signed in to change notification settings - Fork 38
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: use test suite structure #118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the proposal here closely matches what is described in #116 (comment).
Thanks for pushing a first draft of its implementation. Overall, I am more convinced that it is a valuable step forward as we try to increase the coverage of our entire specification with validation schemas.
A few inline questions which are more RFEs about using more of the upstream test-schema.json features.
Apart from fixing the CI, I assume the main remaining thing is to port the invalid tests and consuming the valid
key in the test generation? And possibly apply the same changes to 0.4
in a separate PR?
latest/tests/test_validation.py
Outdated
import pytest | ||
|
||
from jsonschema import RefResolver, Draft202012Validator | ||
from jsonschema import RefResolver, Draft7Validator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember why we used the 202012 implementation. Is the goal here to have use the common-denominator? In that case would the version-agnostic Validator
API be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My local version no longer had the dated Draft so I went with the latest. @will-moore may be able to say more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, don't remember
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I seem to have introduced it in 9602cb4. Looking at the failing tests, I think I remember why.
The JSON schema makes use of the minContains/maxContains
features which were introduced in recent version of the schema (2019-09) so we'll need to use a matching version of the validator to test the examples.
with open(schema["id"]) as f: | ||
schema = json.load(f) | ||
for test in suite["tests"]: | ||
ids.append("validate_" + str(test["formerly"]).split("/")[-1][0:-5]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly points to using the id
key - see https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/test-schema.json#L61 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely happy to move away from these for the proper suite ones. For examples, we'll still need to generate.
Is that not what https://github.com/ome/ngff/pull/118/files#diff-55735767b6ee79dde0d4c0f1f3d88dced97cbeafb3bffc4ed05d514087a3e416R57 is doing? |
Ah you're right that both valid and invalid |
Likely unintentionally. Sorry, that as a copy-n-paste error when I split the files. Now fixed. |
Great, this leave one remaining failed test |
Looks like we're down to just the one "invalid_but_dont_fail" test:
@will-moore, what would you expect to do here? set |
Agree with @seb. Probably best to simply remove the |
Thanks, both. That gets us back to green. Other then potentially refactoring some of the code to the top-level to be re-used between versions, I think I'm happy with this as a first round of refactoring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good from my side. Happy to handle the conversion of the 0.4
tests as a follow-up
SHA: f17bbab Reason: push, by @joshmoore Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Alternate proposal to #116 described under #116 (comment) to make use of the test suite structure from https://github.com/json-schema-org/JSON-Schema-Test-Suite#structure-of-a-test
sciprt used to combine the JSON
Here we are not really using the
schema
field correctly, and perhaps we agree to use: