-
Notifications
You must be signed in to change notification settings - Fork 153
This should fix issue #262 #265
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
Conversation
* Included new JSON-LD tests in json-ld.org directory (perhaps rename this to something more W3C specific, as these are the latest tests from W3C) * Starting from `manifest.jsonld` rather than all jsonld files in the test directory; then iterating of the the list of manifest files specified there.
|
Hi @RinkeHoekstra, thanks for your pull request! Some thoughts:
@ansell: Any thoughts on this, in particular about the last two points? |
|
@fsteeg I haven't had a chance to review the code changes. Getting the testsuite up to what the rest of the JSON-LD community are using is a good thing. I like tests in this repository, it gives consistent builds. Nothing worse than having to check out another repository and then rely on it being stable. Having a script to sync them when changes are made upstream would be ideal, but not necessary for this pull request. I don't mind having broken upstream compliance tests on master while they are being worked through, but I haven't seen the degree to which they are failing due to implementation deficiencies here. I wouldn't be surprised if quite a few are failing, given the lack of attention that Tristan and I have been able to give to this codebase over the past few years. If too many are broken I will do a release before merging, given the few minor changes since the last release. |
|
@RinkeHoekstra As Fabian says, the git commit metadata is important to track who is contributing here. We have automated cleanup for code formatting, so don't worry about that too much, but removing commented out code that is being replaced is a good thing. |
|
There are too many failing tests with similar errors to pull this into master at this point. Many of them seem to have similar reasons which should be fairly easy to fix as part of this Pull Request, to isolate those that are genuinely failing due to an out of date implementation or a lack of implementation for new features.
|
|
Pushed some smaller changes to handle some 1.1 constructs. Doesn't help much (+25 passing tests total), but moves many errors to failures: |
|
Hey @fsteeg @ansell @RinkeHoekstra I would like to use 1.1 in my Android project. Please, let me know how can I help to get this PR approved. |
|
@dsemenovsky Thanks for offering help. If you need any specific 1.1 features you could let us know, so we can focus on implementing them first, and you could test the implementation. |
|
Added support for JSON literals (https://w3c.github.io/json-ld-syntax/#json-literals). Current state: |
|
Added different 1.1 tweaks and fixes in 906cdef, current state: I've also set up an option to run the old 1.0 suite in my fork (fsteeg@278dce3), which currently also has a lot of failures. In jsonld.js they support that option too (see bottom of https://github.com/digitalbazaar/jsonld.js/#tests), however these tests fail over there too, so I'm not sure if it makes sense to set that up here. In any case I'd suggest we rename the test suite folder in this branch to |
core/src/test/java/com/github/jsonldjava/core/JsonLdProcessorTest.javato start frommanifest.jsonldrather than all jsonld files in the test directory; then iterating of the the list of manifest files specified there.