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

Create tests for new (upcoming?) meta-schema #136

Closed
awwright opened this issue Nov 21, 2016 · 36 comments
Closed

Create tests for new (upcoming?) meta-schema #136

awwright opened this issue Nov 21, 2016 · 36 comments
Assignees
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.
Milestone

Comments

@awwright
Copy link
Member

The latest I-D makes a handful of additions and changes. Copy, modify, and add tests as appropriate -- when the meta-schema is eventually published.

@Relequestual
Copy link
Member

Is there a changelog?

@Relequestual Relequestual added this to the draft 6 milestone Nov 21, 2016
@awwright awwright added the missing test A request to add a test to the suite that is currently not covered elsewhere. label Nov 22, 2016
@handrews
Copy link
Contributor

@awwright are you actively working on this? I was going to add cases for the spec changes that I've written, but don't want to duplicate work. Let me know if there is anything you'd like me to pick up.

@Relequestual
Copy link
Member

If either of you are working on this, should probably be in a branch in this repo, right? =]

@handrews
Copy link
Contributor

If either of you are working on this, should probably be in a branch in this repo, right? =]

Probably :-) I'm a little unclear on the branching strategy for this repo, and also whether it's the right time to populate a draft 6 directory or not.

@Relequestual
Copy link
Member

You'd have to ask @Julian on that one =]

@Julian
Copy link
Member

Julian commented Nov 25, 2016

6, have I already missed that much :)?

I'm not 100% happy with two branches here (I started with two, but then once or twice someone wanted an actual "cut" release with a version number, which kind of makes having two branches pointless) but I haven't changed it because I recommend people make use of the test suite via git subtrees, and I assume some people have subtree'd into develop (which wouldn't be a problem other than the next time they subtree pull'ed they'd need to come check that hey yeah develop went away just use master).

TL;DR: yeah, feel free to work off of develop (add some tests in a new branch off of develop, send a PR targeting develop, have someone review it [not necessarily me, whoever feels comfortable doing so] etc.), and definitely starting work sounds good to me, especially if I've managed to miss an entire draft release :)

@seagreen
Copy link

@Julian and @Relequestual I opened an issue on the website's repo suggesting we make a page listing every single specification draft along with links to their respective changelogs. Hopefully if we do decide to make such a page it will get a little harder to fall behind.

@handrews
Copy link
Contributor

@Julian we're trying to wrap up what we have now as Draft 6, in part to address errors that crept in to Draft 5. And also to get an updated meta-schema out there, which is important to some of us.

I laid out a scope for v6 in json-schema-org/json-schema-spec#137 (comment)

@Julian
Copy link
Member

Julian commented Nov 25, 2016

@handrews sounds great will have a look there

@handrews
Copy link
Contributor

@Julian I should probably pull that comment out an make a v6 release tracking issue...

@Relequestual
Copy link
Member

If either of you are working on this, should probably be in a branch in this repo, right? =]

@epoberezkin
Copy link
Member

Could we instead of duplicating all the tests in draft-06 only add the additional tests (given that draft 6 is backwards compatible)? And tell everybody that draft04 tests should pass too (maybe mark those we want to drop, like "1.0 not an integer")? The advantage is that it will be easy to see what has been added.

@Relequestual
Copy link
Member

Relequestual commented Dec 23, 2016

@epoberezkin Interesting idea! I'm not sure I'm sold on it though. I mean if one wants, you could diff the two folders easily enough to see what's been changed..., but I agree it's not as easy as just seeing the additions.

You have highlighted an issue though: How would you denote the tests which are no longer valid for draft-6? Even if we worked out an agreeable way, libraries which digest these tests are unlikely to work that way.

As such, I'm inclined to say no. Unless you can figure a way to mitigate that issue (I can only think of bad solutions).


On a different topic, do people feel we should also provide draft-5 tests? Or would omitting them make people less likely to implement and therefore use draft-5 (which I feel is probably desierable given how draft-5 was born).

@epoberezkin
Copy link
Member

@Relequestual I think draft-5 can be safely forgotten - no tests and no meta-schema...

I guess trying to mark tests that should be ignored complicates things... Maybe we can at least add property draft: X to indicate when the test was added (and add it to old tests as well so diffing is still possible)?

The problem I am trying to solve is to avoid running the same tests twice - it's already quite slow (I'm testing against 128 validator instances to test different permutations of options...).

Diffing is ok as a solution to see new tests, but not as trivial in the test-suite when running tests.

@epoberezkin
Copy link
Member

epoberezkin commented Dec 23, 2016

Adding draft property also makes it easier to see (without diffing) if it's a new feature or if it existed before.

@awwright
Copy link
Member Author

The draft-wright-json-schema-00 draft and companions was intended to update the draft-04 meta-schema, it was never actually given a designation like "draft 5". Unfortunately me and others have been calling it that as if it was supposed to get a meta-schema.

But new I-Ds replace old I-Ds in their entirety, so fortunately none of that will really matter.

@Relequestual
Copy link
Member

@awwright You couldn't be more wrong in your last comment...

"draft-wright-json-schema-00" IS draft 5. You didn't have to give it any designation. Previously was draft 4 (I don't care what the IETF URI was), so the next version is draft 5. We agreed this when we updated http://json-schema.org/documentation.html

"draft n" and "v n" are used interchangibly for JSON Schema. The Internet Draft numbering has been reset, but that doesn't matter to the VERSION of JSON Schema.

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

@Relequestual I think what @awwright was getting at with his "none of that will really matter" is that once we have draft 6 out with its meta-schema, there's really no reason to go back and add meta-schema or test support for draft 5. Draft 6 will fully replace it and while it's publication demonstrated that the project is moving again, it has not been widely adopted. So any implementors should be encouraged to move directly to draft 6.

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

So I think it is OK if we focus our test suite efforts on draft 6

@Julian
Copy link
Member

Julian commented Jan 5, 2017

I obviously haven't been following so closely, but the least confusing thing I think is just to ln -s draft4 draft5 it sounds like, just to never have to get asked that question.

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

@Julian there is one change in draft-5 that would technically require a test change. This "format": "uri" test:

            {
                "description": "a valid protocol-relative URI",
                "data": "//foo.bar/?baz=qux#quux",
                "valid": true
            },

Is not valid in draft-5 because "uri" now only validates the full URI production from RFC 3986, and not relative references. Relative references are only valid with "uriref".

Otherwise I believe the draft-4 tests are valid for draft-5. There are larger incompatible changes for hyper-schema but I guess we don't have any tests for that anyway so it doesn't matter.

@Julian
Copy link
Member

Julian commented Jan 5, 2017

A ha, good, OK I'm glad that change was made. cp -R with the change then is what I'd recommend, I think I've been pretty OK with managing copied tests, despite how it sets off warning bells to programmers, I think it works in this case.

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

@Julian makes sense- unlike multiple sustaining release lines, we don't have all that many fixes that need backporting.

Should new test folders be made on master? I see there's a "node" branch as well but that doesn't seem like a place for general purpose work?

@awwright
Copy link
Member Author

awwright commented Jan 5, 2017 via email

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

@awwright Implementations choose behavior based on meta-schema, so numbering tests with meta-schema numbers is appropriate.

@Julian
Copy link
Member

Julian commented Jan 5, 2017

@handrews master yeah, the node branch was someone saying "I want a package.json" and me saying "I don't think language-specific things belong in the repo, but feel free to maintain a separate branch" (I don't think that actually happened though).

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

@Julian cool. Seems like we should decide on the naming scheme though.

@awwright I have no objection to using the IETF numbering, but if we do, we should name the meta-schemas after the IETF numbering as well, e.g. draft-6 meta schemas should be draft-awright-01. I would be fine with that.

@awwright
Copy link
Member Author

awwright commented Jan 5, 2017 via email

@handrews
Copy link
Contributor

handrews commented Jan 5, 2017

@awwright Cool. I guess I'll go ahead and seed a draft-06 folder from the draft-04 suite (unless you or @Julian are working on it). We can backfill draft-05 at our leisure if we decide it's worthwhile. Otherwise we can just encourage folks to move on to draft-06.

@Julian
Copy link
Member

Julian commented Jan 5, 2017

All yours -- if we do change the naming scheme that's obviously fine with me too, but for backwards compatibility I'd leave symlinks.

handrews added a commit to handrews/JSON-Schema-Test-Suite that referenced this issue Jan 5, 2017
We are not currently adding tests for draft5, see issue json-schema-org#136 for
discussion.
@handrews handrews self-assigned this Jan 6, 2017
@Relequestual
Copy link
Member

@awwright I have no objection to using the IETF numbering, but if we do, we should name the meta-schemas after the IETF numbering as well, e.g. draft-6 meta schemas should be draft-awright-01. I would be fine with that.
(@handrews)

I really have a problem with that.

Consider, someone new to JSON Schema comes to the site. They see there are currently 2 different prefixes to the version number, zyp-04 and awright-01. Generally people will assume higher number is more recent. Explaining why this is not the case is going to cause confusion, which is the main reason I want to keep the numbering regardless of the I-D URI, and explain, like is currently on the JOSN Schema site, "draft-5 is awright-00 due to authorship change".

@epoberezkin
Copy link
Member

I've created a checklist in #153 (as a separate issue so we can see how many tests are left to do). There are 5 PRs with the tests.
@handrews @awwright @Relequestual please review.

@Julian
Copy link
Member

Julian commented Jan 26, 2017

Cool, gonna close this issue out then in favor of the specific checklist you've got there, and / or a milestone if we'd like as well, should be a bit easier to manage and figure out when each individual thing should be closed.

@Julian Julian closed this as completed Jan 26, 2017
@epoberezkin
Copy link
Member

@Julian thank you.

Not sure I understand about milestones... They all should be done for draft-06 I think, there is nothing else in #153.

@Relequestual
Copy link
Member

Great work @epoberezkin, I don't have the time to review the list vs the changes sadly =/
BUT, nothing comes to mind that you might of missed so far.

@Julian
Copy link
Member

Julian commented Jan 27, 2017

@epoberezkin I just meant using a milestone tag on any issues specifically related to draft 6 -- if we are already, great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.
Projects
None yet
Development

No branches or pull requests

6 participants