search

json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
625 stars 209 forks source link

ref: test empty tokens in json-pointer #664

Closed santhosh-tekuri closed 1 year ago

santhosh-tekuri commented 1 year ago

I added the test to just draft4. if the test looks fine. will add to remaining drafts.

the test passes with my impl https://github.com/santhosh-tekuri/boon

FYI: for example java.util.StringTokenizer does not return empty tokens.

gregsdennis commented 1 year ago

I don't support draft 4. Can you go ahead and add to draft 6 so I can run it, please? (also needs a rebase)

This looks like a valid test to me.

santhosh-tekuri commented 1 year ago

added tests in remaining drafts

santhosh-tekuri commented 1 year ago

@gregsdennis added $id to schemas

the check still seems to be failing

gregsdennis commented 1 year ago

I'm sorry. I assumed that was the problem, but it's not.

The error is:

AssertionError: is not a known keyword for draft2020-12. If this test is testing behavior related to unknown keywords you may need to add it to the allowlist in the jsonschema_suite checker. Otherwise it may contain a typo!

It looks like the check is trying to validate the definition "" key as a keyword. @Julian/@jdesrosiers do you know about this?

santhosh-tekuri commented 1 year ago

let me know if $id added has to be removed

Julian commented 1 year ago

The sanity check is somewhat brutish, essentially because doing what it wants to do in general for this is hard -- yes there is no reason you need an $id, that's not required by the spec. I can look at why it's confused by this case in a bit.

Julian commented 1 year ago

The commit I pushed should fix this (yes please remove the IDs when they're not needed).

santhosh-tekuri commented 1 year ago

removed '$id' from schemas

Julian commented 1 year ago

(You removed the fix commit, you'll need to cherry pick it back)

santhosh-tekuri commented 1 year ago

@Julian not sure how to do cherry picking. trying it from google

Julian commented 1 year ago

If you pulled before force pushing, just run git cherry-pick a81ddee and then push. If you didn't, run git pull first then run that and hopefully that works I forget, if the commit is not on any branch GitHub may not send it to you -- if so I'll repush it when I get back to a computer.

santhosh-tekuri commented 1 year ago

I did not pull your change, before I did force push. now a81ddee is not showing up in my repo

santhosh-tekuri commented 1 year ago

@Julian I took diff from your commit and applied it.

santhosh-tekuri commented 1 year ago

I reverted my change. please recommit your change when you are back. sorry for the confusion.

Julian commented 1 year ago

(Re-pushed.)

gregsdennis commented 1 year ago

I think we're good now. Thanks @Julian. I'm going to go ahead and merge.