Skip to content

Conversation

@fsteeg
Copy link
Member

@fsteeg fsteeg commented Apr 28, 2020

As announced in #277 (comment), this PR contains only the changes in the test setup for 1.1:

See README for updating, running, and report generation:

https://github.com/jsonld-java/jsonld-java/blob/1.1-tests/README.md#update-local-spec-tests

This is the report summary for 1.1 compliance w/o any 1.1 specific code (see 2318376):

tests pass
Compaction 86/238 (36.1%)
Expansion 84/366 (23.0%)
Flattening 45/55 (81.8%)
Framing 26/89 (29.2%)
HTML 3/49 (6.1%)
Remote document 3/18 (16.7%)
Transform JSON-LD to RDF 152/442 (34.4%)
Transform RDF to JSON-LD 30/51 (58.8%)

I will open a second pull request with our current 1.1 implementation changes from #277.

RinkeHoekstra and others added 21 commits June 27, 2019 10:02
* 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.
- Set up tests for json-ld-api, json-ld-framing, and json-ld-1.0
- Run only json-ld-1.0 suite in Maven build (by naming convention)
- Run all suites for development of 1.1 features (AllSuites.java)
- Skip only failing tests to make sure we run 1.1 code paths in CI
- Use resolvable URLs in skip files, they can act like TODO lists
- Delete `*-skip` files to re-generate them during next test run

Now `mvn test` runs all tests that are not listed in the files.
- be more verbose in some error messages
- add some more comments pointing to the specs
- add a blank node condition according to spec 16.4
Focusing on expansion and context/term processing
@fsteeg fsteeg requested a review from dr0i April 28, 2020 08:50
@coveralls
Copy link

coveralls commented Apr 28, 2020

Coverage Status

Coverage increased (+1.1%) to 90.397% when pulling a1d62e1 on 1.1-tests into e94efdf on develop.

@@ -0,0 +1,66 @@
package com.github.jsonldjava.specs;
Copy link
Member

@dr0i dr0i Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is almost identical to the JsonLd1Tests. Can the code be reused ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at this, but I think avoiding the redundancy here would lead to too much complexity. First, the actual code is already extracted in static methods of SuiteUtils. The redundant part is only the JUnit Parameters/BeforeClass/AfterClass setup. These are static methods, so simply extracting them to a superclass won't work. An option would be a custom JUnit Rule (see https://www.baeldung.com/junit-4-rules#defining-a-custom-junit-rule), but I'm not sure if/how that would work with Parameterized, and I think it might actually make the code less readable.

@@ -0,0 +1,66 @@
package com.github.jsonldjava.specs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -0,0 +1,106 @@
# Introduction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this is a copy of https://github.com/w3c/json-ld-api/blob/master/tests/README.md.
Will add a remark in a commit - I think it is helpful to know the source?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed helpful to know the source. However, the workflow for updating the tests (as documented in the README, 'Update local spec tests') involves copying json-ld-api/tests/* to json-ld-api-tests/, which replaces the README. Maybe we could add a separate README to src/test/resources, describing the origins of the spec tests?

@dr0i dr0i assigned fsteeg and unassigned dr0i Apr 28, 2020
@ansell
Copy link
Member

ansell commented May 3, 2020

Rather than pulling in partially implemented code to master, I will retarget this at a new develop branch, which can contain all of the 1.1 development without worrying about whether it is currently passing all tests. That way we can still use master for releases of bug fixes if necessary while the 1.1 development occurs.

@ansell ansell changed the base branch from master to develop May 3, 2020 02:53
elahrvivaz and others added 20 commits July 8, 2020 11:12
* Don't minimize guava shading, causes NoClassDefFoundError
Current state of 1.1 development
Complements the last commit.
The ignored test would test an expected behaviour of java.net.URL.HttpURLConnection,
i.e. not to automatically follow redirects when this involves a protocol switching
(e.g. from HTTP to HTTPS).

See #289.
- remove unused imports
- sorted alphabetically
This avoids a possible endless loop.

See #292 (comment).
It should be checked if an Entity has a contentType, not if an Entity has
content.

See #292 (comment) and
#292 (comment).
- remove superfluous check (was always false)
- remove superfluous IOException
- remove logger as it is no more needed

See #292 (comment).
Signed-off-by: Peter Ansell <p_ansell@yahoo.com>
Signed-off-by: Peter Ansell <p_ansell@yahoo.com>
Signed-off-by: Peter Ansell <p_ansell@yahoo.com>
Signed-off-by: Peter Ansell <p_ansell@yahoo.com>
@fsteeg
Copy link
Member Author

fsteeg commented Sep 10, 2020

I've updated this with the current master, making the tests pass. There is some confusion here however, as this now contains what is described in #284 (current state of 1.1 implementation) plus what is described here (just the test setup). I shouldn't have opened #284 with a base branch that I didn't intend to be merged into, sorry. But this might be obsolete anyways (see #284 (comment)).

@ansell ansell mentioned this pull request Oct 13, 2020
@dr0i dr0i merged commit 6030f25 into develop Mar 4, 2021
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.

8 participants