Skip to content

added tests for checking the entire resource for anchors instead of just the local subschema #679

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

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis commented Jul 30, 2023

See Slack conversation

Have tested against my implementation

@gregsdennis gregsdennis requested a review from a team as a code owner July 30, 2023 22:06
@karenetheridge
Copy link
Member

I'm not a huge fan of the draft6 remotes being added here, since now I need to modify my test harness to skip those (I only support draft7 and up), but I can't really say "don't do that" -- so I'll go ahead and make the appropriate mods.

..and after doing so, tests pass in my implementation.

@gregsdennis
Copy link
Member Author

Yeah, the draft6 and draft7 ref files are the same, but it didn't feel right referencing one from the other, and I didn't think it was right to put it in the "unversioned" parent folder, either.

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.

Passes for me.

@gregsdennis gregsdennis merged commit a0097d4 into main Aug 1, 2023
@gregsdennis gregsdennis deleted the gregsdennis/ref-to-ref-to-non-local-target branch August 1, 2023 05:51
@Julian
Copy link
Member

Julian commented Aug 2, 2023

These tests actually slightly break the promised API for remotes -- specifically we "support" / tell implementers there are 2 ways they can know what additional schema resources and at what remote URLs might need to be available for running tests:

  • Run jsonschema_suite remotes and use the output of that, a mapping between URIs and schemas
  • Take any URL looking like "http://localhost:1234/<foo>" and lookup <foo> directly in the remotes/ directory off the filesystem, such that foo/bar/baz maps to the file remotes/foo/bar/baz, expected to be available during a test run.

These tests though do something subtly different -- specifically, they use an $id of e.g. "http://localhost:1234/draft2019-09/detached-ref" but they are not available at remotes/draft2019-09/detached-ref, they're instead available at remotes/draft2019-09/detached-ref*.json*.

You can see the effects of this in the new failures of a bunch of implementations in Bowtie.

It's true that implementations may need/want to support discovering different $ids found internally, but given that's not the purpose of this test, it seems to me we should change the $ids here.

@gregsdennis
Copy link
Member Author

they're instead available at remotes/draft2019-09/detached-ref*.json*

I assume the * is meant to make italics?

I thought about this while testing, and I had looked at a few other remotes that seemed to have this same pattern of .json in the file name but not in the $id. I didn't look at any $refs to those files, though. I'll have a look again and post.

You can see the effects of this in the new failures of a bunch of implementations in Bowtie.

Interestingly, mine is among the errored ones. Not sure what that's about. "Works on my machine."

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.

4 participants