Skip to content

Conversation

@RinkeHoekstra
Copy link

  • 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)
  • Updated core/src/test/java/com/github/jsonldjava/core/JsonLdProcessorTest.java to start from manifest.jsonld rather than all jsonld files in the test directory; then iterating of the the list of manifest files specified there.

* 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.
@fsteeg
Copy link
Member

fsteeg commented Jun 28, 2019

Hi @RinkeHoekstra, thanks for your pull request! Some thoughts:

  • It looks like the email used in your local Git config is not known to GitHub, so your commit here is not connected to your account
  • Your changes are conflicting with the current master branch due to changes in master after you branched off. You should merge the current master into your branch or rebase your branch on top of the current master, and resolve the conflicts (in toRdf-manifest.jsonld)
  • If you mention the related issue in the initial pull request comment (as you did in the commit), a useful link to the pull request is created in the issue
  • We should not keep commented-out code around, it's in the Git history so we can always check that if we need the old code
  • I'm not sure if it's a good idea to include a copy of the test suites in our repo. Maybe we could go with the approach that jsonld.js takes, which is to expect local checked out repos for the tests (and as a second step, automate checking out these repos), see https://github.com/digitalbazaar/jsonld.js/#tests
  • As many of these tests currently fail with our implementation, we need a way to specify which tests to skip, like the skip entries in https://github.com/digitalbazaar/jsonld.js/blob/master/tests/test-common.js. Perhaps for this pull request, you could make running against the new suite an option, and we keep running the old suite as the default. As a next step we can then implement skipping specific tests, and then switch the default to the new suite.

@ansell: Any thoughts on this, in particular about the last two points?

@ansell
Copy link
Member

ansell commented Jun 29, 2019

@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.

@ansell
Copy link
Member

ansell commented Jun 29, 2019

@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.

@ansell
Copy link
Member

ansell commented Jul 29, 2019

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.

JsonLd invalid term definition: 1.1 and java.lang.ClassCastException: java.util.ArrayList cannot be cast to java.lang.String are two errors that appear often in the results and probably should be investigated before merging this.

[ERROR] Tests run: 929, Failures: 204, Errors: 220, Skipped: 12

@fsteeg
Copy link
Member

fsteeg commented Oct 29, 2019

Pushed some smaller changes to handle some 1.1 constructs.

Doesn't help much (+25 passing tests total), but moves many errors to failures:

[ERROR] Tests run: 929, Failures: 293, Errors: 106, Skipped: 12

@dsemenovsky
Copy link

dsemenovsky commented Nov 20, 2019

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.

@fsteeg
Copy link
Member

fsteeg commented Nov 29, 2019

@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.

@fsteeg
Copy link
Member

fsteeg commented Nov 29, 2019

Added support for JSON literals (https://w3c.github.io/json-ld-syntax/#json-literals). Current state:

[ERROR] Tests run: 929, Failures: 296, Errors: 80, Skipped: 12

fsteeg added a commit to fsteeg/jsonld-java that referenced this pull request Dec 6, 2019
@fsteeg
Copy link
Member

fsteeg commented Dec 6, 2019

Added different 1.1 tweaks and fixes in 906cdef, current state:

[ERROR] Tests run: 930, Failures: 355, Errors: 14, Skipped: 12

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 json-ld-api-tests to avoid confusion with the old json-ld.org suite.

@fsteeg
Copy link
Member

fsteeg commented Apr 28, 2020

Replaced by / moved to #283 & #284, closing.

@fsteeg fsteeg closed this Apr 28, 2020
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