Improved extends and $ref support, added json-schema-test-suite test-cases #39

janmentzel · 2013-04-18T20:38:00Z

I integrated the json-schema-test-suite test cases.
Fixed about 20 tests but still some fail.

$ ./vendor/bin/phpunit 
PHPUnit 3.7.19 by Sebastian Bergmann.

Configuration read from /Users/janmentzel/work/json-schema-php/phpunit.xml.dist

...............................................................  63 / 452 ( 13%)
............................................................... 126 / 452 ( 27%)
...........................................................F... 189 / 452 ( 41%)
.......F....FFF.FFFF.F..FF........F...........FFF.............. 252 / 452 ( 55%)
.....F............F..........................F....EEEEEEEEEEEEE 315 / 452 ( 69%)
.....FEE....................................................... 378 / 452 ( 83%)
..................................F.F.F..FEE................... 441 / 452 ( 97%)
........F..

Time: 1 second, Memory: 17.25Mb

[...]

FAILURES!
Tests: 452, Assertions: 464, Failures: 25, Errors: 17.

For its remote schema tests the testsuite needs a small http server running on http://localhost:1234
Please see README.md for how to install and run.

If you don't want the json-schema-test-suite tests to run, simply comment out the code in test/SuiteTest.php.

…DO: nested refs TODO: Failures: 25, Errors: 16

justinrainbow · 2013-05-28T16:44:58Z

Could you rebase this against the current master?

mchiocca · 2013-05-28T17:43:06Z

Hello. Please take a look at #41, which was merged into master, before you rebase so you'll have a good idea and understanding of all the changes that were included in that Pull Request. Thanks!

janmentzel · 2013-05-28T17:53:32Z

rebased already. had some conflicts. seems we had done equal fixes for the same issues ;)
passed quite some time. seems i have to merge my stuff manually.

mchiocca · 2013-05-28T18:14:47Z

Hello. I fixed all of the draft-03 bugs and issues with the exception of things related to $ref, $schema, and id. So #41 doesn't include any changes to address that functionality. Not all ref tests, remote ref tests, etc. work correctly. If you look at the commits in #41, you'll see what I fixed. There is some overlap with things that you have worked on. The entire test suite that's included with #41 should pass. The tests that I added were based on tests from JSON-Schema-Test-Suite, which you have been working with. I noticed that your work seemed to focus on $ref, etc. so my work focused on fixing all other draft-03 deficiencies. Note that with #41, the entire draft-03 test suite should pass, excluding ref.json, refRemote.json, and a few of the optional tests. Let me know if you have any questions about #41. It significantly improves the json-schema project's functionality with respect to supporting draft-03. Thanks!

janmentzel · 2013-05-28T18:39:54Z

looks good. my fork seems to be obsolete.

my postings on json-schema-test-suite might me misleading. I didn't work on $ref here. At the beginning I thought I could solve some businescases with $ref but later I found out 'extends' is what I was looking for. So my main focus was 'extends' - and the errors concerning the test-suite as well.

edit: to be honest, its been almost two months ago I worked on the pull request here. lost the details.

Did you include all test-cases from the test-suite?

mchiocca · 2013-05-28T19:06:49Z

Hello again. Note that #41 also fixes extends as well. So, I guess you're right; your fork no longer seems to be needed. I apologize. I did not intend on undermining your work. We needed draft-03 compliance and the support for $ref was sufficient in it's current form. I did not add the entire JSON-Schema-Test-Suite with #41. I did incorporate tests from that suite with each of my commits based on the functionality that I was fixing. However, I have included the draft-03 suite in it's entirety in my own fork. It just isn't committed or pushed to my master yet. I don't believe that your approach of using a Git module is the best way to go. Most JSON Schema validation implementations that I've seen incorporate that test suite manually, copying the files into the project in some form. That's what I've done. This way, the project is isolated from changes in the test suite that could potentially break the build. I'd like to push what I have to master because I think having the entire suite adds a lot of value to the json-schema project. And, as I mentioned, the draft-03 suite does pass except for a few .json test files that I have marked as excluded. I hope you find value with all of the changes I made with #41. Thanks!

janmentzel · 2013-05-29T09:21:11Z

yeah, it's a pity my pull request has been ignored for so long and you refixed the things i have already done.

i used git submodule to easily staying up to date if there are some changes in the test-suite.
I think breaking the build by failing test-suite tests would only be a cosmetic issue.
I even think being compliant to the most recent test-suite is more important. If tests fail there is an issue to be resolved and not the test to be removed imho.

Installing the test-suite via composer as dev-dependency with - if you like - a fixed version would be the way to go in my opinion - explicit, easy, isolatable, safe.

Question: did you include the remote tests?

mchiocca · 2013-05-29T11:50:25Z

Hello. I think the biggest reason why your pull request was never merged is because it was never fully complete. Your master never passed the build due to the large number of failures and errors in the test suite that you were trying to add. If it were me, I would never merge from a pull request that didn't have a green build.

My pull request had only two commits that weren't green. Those were due to Scrutinizer errors, not test failures. I also fixed and/or added a lot more draft-03 functionality in my request than you did in yours. Plus I included a bunch of new unit tests. Look over the commits in #41 to see what I did that was beyond what you did.

Using a Git submodule can keep you the up-to-date with the latest in the test suite repository. However, failures are possible, and not just because functionality is broken. I filed a bug against one of the tests in the suite, which would have caused an erroneous failure not due to broken functionality, but because of a faulty test. So I don't think using a Git submodule is the best way to go even with a dependency on a fixed version, unless that fixed version is known to be free of defects. The test suite only has one tag, 1.0.0, and that's pretty old.

As I mentioned, all of the JSON Schema validation implementations that I've seen incorporate the test suite into their projects by copying the content of the tests. None of them use a Git submodule to pull in the suite. So there is isolation from the test suite project and any potential breakage.

I've copied the draft-03 suite into my local repository. I have all of the .json files and I've written a small amount of code to read those files and execute the tests. The entire draft-03 test suite passes with the exception of a few test files that I've marked as skipped (e.g. ref.json, refRemote.json, and three .json files in the optional folder.)

As for your pull request, since #41 has rendered it largely obsolete and unnecessary, I would simply close this request and not merge it. You will have lost some effort, but since you said that you haven't actively worked on this pull request for two months, I think closing without merging is probably the best way to go. Hope that's OK.

janmentzel · 2013-05-29T12:58:13Z

Ok, you skipped the remote tests.
So there still seem to be some usefull parts of my work.
Will do another pull request when I find time to work on it.

* Improve performance - don't loop over everything if already valid * Don't coerce already-valid types (fixes #379) * Add remaining coercion cases & rewrite tests * Add all remaining coercion cases from ajv matrix * Rewrite the coercion tests to tidy things up a bit * Add CHECK_MODE_EARLY_COERCE If set, falls back to the old behavior of coercing to the first compatible type, regardless of whether another already-valid type might be available. * Add multiple-type test that requires coercion * \JSON_PRETTY_PRINT doesn't exist in PHP-5.3, so work around this * Various PR cleanup stuff * Fix whitespace * Turn $early into $extraFlags * Change "string" to "ABC" in string test * Update README.md description of CHECK_MODE_EARLY_COERCE * Move loop after complex tests definition * Move test #39 to grid #15

* Improve performance - don't loop over everything if already valid * Don't coerce already-valid types (fixes jsonrainbow#379) * Add remaining coercion cases & rewrite tests * Add all remaining coercion cases from ajv matrix * Rewrite the coercion tests to tidy things up a bit * Add CHECK_MODE_EARLY_COERCE If set, falls back to the old behavior of coercing to the first compatible type, regardless of whether another already-valid type might be available. * Add multiple-type test that requires coercion * \JSON_PRETTY_PRINT doesn't exist in PHP-5.3, so work around this * Various PR cleanup stuff * Fix whitespace * Turn $early into $extraFlags * Change "string" to "ABC" in string test * Update README.md description of CHECK_MODE_EARLY_COERCE * Move loop after complex tests definition * Move test jsonrainbow#39 to grid jsonrainbow#15

janmentzel added 17 commits April 15, 2013 11:40

using json-schema-suite tests. 30 tests still fail.

25fa3a9

docu: composer via curl

8171348

comment

9f0f3ac

typo in readme

2b14e49

testserver for testsuite remote tests; remote ref working basicly, TO…

5e07c66

…DO: nested refs TODO: Failures: 25, Errors: 16

fix package name

d3bb6e4

Update composer.json

efb377b

Merge branch 'master' of github.com:hypercharge/json-schema

0206ae6

fiddling around with uri resolving

5f8f4c5

revoked validateRegex; resolve extends with uri; removed debug echos

a9eaf93

add support for extends with array

fe4897c

draft not needed anymore

aa48b8c

testserver install howto

dc2be6e

fix incremental id concat, fix regexp tests

8ca98f9

SuiteTest: nicer test picking and auto verbose handling.

1478819

UriRetriever.resolve() should handle https://, file:// as well

2954e4c

refix id resolving in RefResolver

0ba684e

janmentzel closed this May 29, 2013

erayd added a commit to erayd/json-schema that referenced this pull request Mar 14, 2017

Move test jsonrainbow#39 to grid jsonrainbow#15

15102c8

erayd mentioned this pull request Mar 14, 2017

Improvements to type coercion #384

Merged

erayd added a commit to erayd/json-schema that referenced this pull request Mar 17, 2017

Move test jsonrainbow#39 to grid jsonrainbow#15

7df164c

erayd added a commit to erayd/json-schema that referenced this pull request Mar 17, 2017

Move test jsonrainbow#39 to grid jsonrainbow#15

98b6516

erayd added a commit to erayd/json-schema that referenced this pull request Mar 21, 2017

Move test jsonrainbow#39 to grid jsonrainbow#15

e64fd9e

erayd added a commit to erayd/json-schema that referenced this pull request Mar 21, 2017

Move test jsonrainbow#39 to grid jsonrainbow#15

a21741d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improved extends and $ref support, added json-schema-test-suite test-cases #39

Improved extends and $ref support, added json-schema-test-suite test-cases #39

Uh oh!

janmentzel commented Apr 18, 2013

Uh oh!

justinrainbow commented May 28, 2013

Uh oh!

mchiocca commented May 28, 2013

Uh oh!

janmentzel commented May 28, 2013

Uh oh!

mchiocca commented May 28, 2013

Uh oh!

janmentzel commented May 28, 2013

Uh oh!

mchiocca commented May 28, 2013

Uh oh!

janmentzel commented May 29, 2013

Uh oh!

mchiocca commented May 29, 2013

Uh oh!

janmentzel commented May 29, 2013

Uh oh!

Uh oh!

Improved extends and $ref support, added json-schema-test-suite test-cases #39

Improved extends and $ref support, added json-schema-test-suite test-cases #39

Uh oh!

Conversation

janmentzel commented Apr 18, 2013

Uh oh!

justinrainbow commented May 28, 2013

Uh oh!

mchiocca commented May 28, 2013

Uh oh!

janmentzel commented May 28, 2013

Uh oh!

mchiocca commented May 28, 2013

Uh oh!

janmentzel commented May 28, 2013

Uh oh!

mchiocca commented May 28, 2013

Uh oh!

janmentzel commented May 29, 2013

Uh oh!

mchiocca commented May 29, 2013

Uh oh!

janmentzel commented May 29, 2013

Uh oh!

Uh oh!