Regression in SHACL W3C Compliance Tests

[panayot-panayotov-onto]

There is a significant regression in SHACL W3C compliance in RDF4J 3.2.0-M1 build vs 3.1.0 version:

• passed: 35 (3.2.0-M1) vs 52 (3.1.0);
• failed: 12 (3.2.0-M1) vs 29 (3.1.0)
• warrning: 56 (3.2.0-M1) vs 56 (3.1.0);

[hmottestad]

I think you might be looking at the test suite designed by James, which doesn't take into account shapes that are inherently invalid (eg. just loading the shape will cause a violation). I am currently not maintaining that test suite since it is impossible to debug in intellij. I have a simpler to use version of the compliance tests here: https://github.com/eclipse/rdf4j/blob/develop/core/sail/shacl/src/test/java/org/eclipse/rdf4j/sail/shacl/W3cComplianceTest.java

That suite shows 53 passing tests.

This is the PR that caused the change: https://github.com/eclipse/rdf4j/pull/2020

I propose we proceed by adding documentation to the old compliance test (https://github.com/eclipse/rdf4j/blob/master/compliance/shacl/src/test/java/org/eclipse/rdf4j/sail/shacl/SHACLComplianceTest.java) so that people don't rely on it.

[abrokenjester]

I think you might be looking at the test suite designed by James, which doesn't take into account shapes that are inherently invalid (eg. just loading the shape will cause a violation). I am currently not maintaining that test suite since it is impossible to debug in intellij. I have a simpler to use version of the compliance tests here: https://github.com/eclipse/rdf4j/blob/develop/core/sail/shacl/src/test/java/org/eclipse/rdf4j/sail/shacl/W3cComplianceTest.java

Can you explain what is different about your compliance test suite that makes it easier to use in IntelliJ (and indeed how these tests are easier to use/debug)? Curious as it might be potentially relevant for our other manifest-based compliance tests.

I propose we proceed by adding documentation to the old compliance test (https://github.com/eclipse/rdf4j/blob/master/compliance/shacl/src/test/java/org/eclipse/rdf4j/sail/shacl/SHACLComplianceTest.java) so that people don't rely on it.

If it's a matter of getting rid of these old compliance tests, we can just delete the module - the compliance modules are not officially part of our release artifacts, and are not covered by semver concerns (though keep in mind that the testsuite modules are part of our release!). As a courtesy we could of course mark it deprecated first. Also: if these tests are no longer relevant according to the SHACL implementation, can you ensure that they are not actually run when executing the full project build (if you haven't already)?

Perhaps a better alternative would be if we moved your new test suite into the shacl-testsuite module (so others can reuse), getting rid of the old one James designed.

[hmottestad]

My one is parameterized. IntelliJ understands this and lets you debug a single test.

I don’t really want to delete the one that James made, its a lot less “custom” than mine and reuses the manifest code we already have.

I also don’t want to move mine at the moment.

Once we have full compliance with SHACL we can fix the one James wrote to load shapes and data at the same time and then delete mine.

[abrokenjester]

My one is parameterized. IntelliJ understands this and lets you debug a single test.

Ah I see what you mean. This is not unique to IntelliJ by the way (Eclipse supports the same thing), and so would be a great benefit in general.

I don’t really want to delete the one that James made, its a lot less “custom” than mine and reuses the manifest code we already have.

We don't have to clean this up right now but the existing manifest test code is ancient and unwieldy, and I'd be very much in favor of properly refactoring it to using parametrized tests.

But we'll separate that out from this particular task. For now, let's just document that the test suite is out of date. I'll see if I can spend some spare time on doing some refactoring later.