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

initial run at creating output tests #619

Merged
merged 21 commits into from
Dec 13, 2022
Merged

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis commented Nov 23, 2022

Begins to address #247

This is a go at creating some output tests. It includes 2019-09/2020-12 and draft-next tests for:

  • annotations shouldn't be reported for failed validations
  • type creates a proper node, and may include a message if it fails
  • readOnly generates an annotation of its value

Hat's off to @karenetheridge who proposed using a schema to validate the output. This creates a really nice way to target the bit of the output that a test is focused on without requiring an explicit error message or being reliant on specific output unit sequencing.

I still have structural tests planned, but I figured this would be enough to start.

I've also included a README for the folder.

@gregsdennis gregsdennis requested a review from a team as a code owner November 23, 2022 05:29
Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

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

Cool! Looks great overall to me I think. Left some pretty minor comments I think, and may take a second read after your response(s) but yeah think this is good!

output-tests/content/draft-next/general.json Outdated Show resolved Hide resolved
output-tests/content/draft2019-09/general.json Outdated Show resolved Hide resolved
output-tests/README.md Outdated Show resolved Hide resolved
output-tests/content/draft-next/general.json Outdated Show resolved Hide resolved
output-tests/README.md Outdated Show resolved Hide resolved
output-tests/README.md Show resolved Hide resolved
Copy link
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 haven't tied it out yet, but the concept looks sound.

.editorconfig Outdated Show resolved Hide resolved
output-tests/README.md Outdated Show resolved Hide resolved
test-schema.json Outdated Show resolved Hide resolved
bin/jsonschema_suite Outdated Show resolved Hide resolved
bin/jsonschema_suite Outdated Show resolved Hide resolved
gregsdennis and others added 7 commits November 29, 2022 10:14
Co-authored-by: Julian Berman <[email protected]>
Co-authored-by: Julian Berman <[email protected]>
* origin/main:
  remove schemas that are never referenced
  move anchors into defs
  move tests to draft-next
  added type:object to contentSchema schemas
  add tests for $dynamicAnchor in multiple branches of propertyDependencies
  add tests for unevaluatedProperties seeing inside propertyDependencies
  Add RJP test 'multi-digit integer prefix'
  Remove unneeded remotes
Output tests aren't actually overlapping with the normal test schema,
they don't contain the 'valid' property (which is required in normal
tests).
It doesn't contain tests, so it's indeed not valid under the output
schema.
@gregsdennis
Copy link
Member Author

I've added $ids to all of the output validation schemas. This provides a base URI for the $ref to use.

I also implemented this new suite in my lib (PR linked ☝️) and I'm happy to report that I fail the 2019 & 2020 cases! 🤦

@gregsdennis
Copy link
Member Author

I now pass most of the 2019/2020 cases. There's one test in each that I can't pass because my approach for draft-next doesn't always track the keywords in passing subschemas, so I can't correctly report the full keyword locations. It's fine though: I just mark those tests as inconclusive as I've just decided I'm not going to fully support those drafts.

And fix skipping regex format validation and the case where
dynamicRef blows up.

(The latter hopefully being temporary, but just so the PR passes)
@Julian
Copy link
Member

Julian commented Dec 12, 2022

I think we should likely merge these! I've only given the content of the tests themselves a few skims but they seemed nice as a starting point, may as well get them in and iterate I think, but would possibly give slightly more visibility if they were merged in even if we needed to make a tweak or two once others tried running them.

So yeah thumbs up from me for merging when you're comfortable, or if you're planning on hearing from others obviously fine with me too.

@jdesrosiers
Copy link
Member

I'm fine with merging as a starting point. Forgive me for not re-reading the readme changes, but does it say somewhere that people shouldn't rely on these test yet because they are still being developed and may contain errors?

@Julian
Copy link
Member

Julian commented Dec 12, 2022

Probably a very good idea especially considering the last conversation.

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