GH-4592 Replace a Junit4 Suite with v5 DynamicTest #4593

JervenBolleman · 2023-05-22T12:54:45Z

Also replaces a static so more tests should be runnable in parallel in the future.

GitHub issue closes #4592

PR Author Checklist (see the contributor guidelines for more details):

my pull request is self-contained
I've added tests for the changes I made
I've applied code formatting (you can use mvn process-resources to format from the command line)
I've squashed my commits where necessary
every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

JervenBolleman · 2023-05-22T20:41:41Z

@abrokenjester I think I got the upgrade of the manifest based tests to junit5 done. I removed some old deprecated test classes now that we are going to go to v5 as they are not compatible. They would need a similar non compatible rewrite to make work.

hmottestad · 2023-10-31T20:41:21Z

@JervenBolleman I'll try to take at look at this as soon as I can.

JervenBolleman · 2023-10-31T20:44:23Z

@hmottestad I am rebasing this so that you don't need to deal with the mergeconflicts.

hmottestad · 2023-10-31T20:47:04Z

@hmottestad I am rebasing this so that you don't need to deal with the mergeconflicts.

Thanks!

JervenBolleman · 2023-11-01T08:28:55Z

@hmottestad the verify/integration-tests stage passes on my laptop.

However, it does fail on my desktop with the same error as given by the github action :(

JervenBolleman · 2023-11-03T08:42:08Z

@kenwenzel would you mind having a look at this failure. The tests pass for native and memory on all my systems. However, on my desktop and the github action I have strange errors. One difference is that my laptop is fedora while desktop and github action are ubuntu based.

kenwenzel · 2023-11-03T10:26:10Z

@kenwenzel would you mind having a look at this failure. The tests pass for native and memory on all my systems. However, on my desktop and the github action I have strange errors. One difference is that my laptop is fedora while desktop and github action are ubuntu based.

Hi @JervenBolleman,

the NPEs are due to lazy values. Maybe the hash code function tries to initialize these values after the store has already been closed.
I've improved the handling of lazy values in this commit:
9b0622a

This should ensure that values do not leave the SAIL in an uninitialized state.
It may be the case that I've overseen something.

JervenBolleman · 2023-11-03T10:27:13Z

@kenwenzel @hmottestad I found that part of the reason is that these tests now run concurrently and where sharing state. I fixed just this and will push a new commit.

hmottestad · 2023-11-10T11:46:09Z

...src/main/java/org/eclipse/rdf4j/testsuite/query/algebra/geosparql/GeoSPARQLManifestTest.java

-		tests.toArray(result);
-
-		return result;
+	public Collection<DynamicTest> tests() {


I believe that this needs the @testfactory annotation. IntelliJ doesn't seem to pick up the annotation on the abstract method.

Probably applies to other similar methods too.

I think it is actually an error in the manifest file itself. There is a query that looks for mf:include but the manifest file for geosparql uses mf:entities and even a different IRI prefix for mf:. Let me have a look.

Looking at a current develop integration test job I think these tests are not run at this time either...

[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] --- maven-failsafe-plugin:3.1.2:verify (verify) @ rdf4j-geosparql-compliance ---

Reenabled these tests. Lucky they all pass :)

There are several other similar methods that were missing the @testfactory annotation. Did you look over the rest of the code?

hmottestad · 2023-11-10T11:57:07Z

This is pretty sweet! I can debug a single test in IntelliJ!

hmottestad · 2023-11-10T12:35:42Z

testsuites/sparql/src/main/java/org/eclipse/rdf4j/testsuite/sparql/tests/BindTest.java

+
+	public Stream<DynamicTest> tests() {
+		return Stream.of(makeTest("GH4499BindFilterNotExist2", this::testGH4499BindFilterNotExist2),
+				makeTest("GH4499BindFilterNotExist1", this::testGH4499BindFilterNotExist1),
+				makeTest("GH3696Bind", this::testGH3696Bind), makeTest("SelectBindOnly", this::testSelectBindOnly),
+				makeTest("SES2250BindErrorsInPath", this::testSES2250BindErrorsInPath),
+				makeTest("SES2250BindErrors", this::testSES2250BindErrors),
+				makeTest("BindScopeUnion", this::testBindScopeUnion), makeTest("BindScope", this::testBindScope),
+				makeTest("BindError", this::testBindError));
+	}


Is there no way to stick with the regular @test annotation? Or maybe generate this list of tests in a more automatic way?

I couldn't figure it out from the documentation :(

ChatGPT recommends this:

import java.lang.reflect.Method; import java.util.Arrays; import java.util.stream.Stream; import org.eclipse.rdf4j.repository.Repository; import org.eclipse.rdf4j.repository.RepositoryConnection; import org.junit.jupiter.api.DynamicTest; public class BindTest extends AbstractComplianceTest { // ... existing methods ... public Stream<DynamicTest> tests() { // You may need to adjust how the repository connection is obtained Repository repo = getRepository(); // Or however you obtain a repository instance RepositoryConnection conn = repo.getConnection(); return Arrays.stream(BindTest.class.getDeclaredMethods()) .filter(method -> shouldRunAsTest(method)) .map(method -> createDynamicTestFromMethod(method, conn)); } private boolean shouldRunAsTest(Method method) { // Check if the method is a test method and takes a RepositoryConnection as argument return method.getName().startsWith("test") && method.getParameterCount() == 1 && method.getParameterTypes()[0] == RepositoryConnection.class; } private DynamicTest createDynamicTestFromMethod(Method method, RepositoryConnection conn) { return DynamicTest.dynamicTest(method.getName(), () -> { try { method.invoke(BindTest.this, conn); } catch (Exception e) { throw e.getCause(); } }); } private Repository getRepository() { // Implement this method to return your Repository instance return ...; } }

Dunno how pretty that is though.

That's going back to junit v2 ;) I don't mind, the official way is to generate some kind of test template. But it is a lot of work for a little gain. @hmottestad Is it ok if we merge as is and leave that as a todo?

I guess we can always change it up later, and your approach does allow IntelliJ to run a single test instead of having to run the entire suite.

Also replaces a static so more tests should be runnable in parallel in the future. Signed-off-by: Jerven Bolleman <[email protected]>

Also delete some deprecated tests classes that are no longer compatible with the junit5 way of doing things. Signed-off-by: Jerven Bolleman <[email protected]>

Signed-off-by: Jerven Bolleman <[email protected]>

Allow for non DAWG approved tests to run. Adds a manifest-all.ttl file for geosparql like sparql 1.1. compliance. Also remove some unused imports. Signed-off-by: Jerven Bolleman <[email protected]>

JervenBolleman · 2023-11-10T15:43:46Z

@hmottestad sorry for the force push to undo the merge commit :(

JervenBolleman · 2023-11-10T18:07:50Z

I believe that the missing annotation was not the problem. For me other classes with eclipse it works with just the annotation on the super super class. I am going to make sure they ran in the logs to be sure.

…

On Fri, Nov 10, 2023, 17:30 Håvard M. Ottestad ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In testsuites/geosparql/src/main/java/org/eclipse/rdf4j/testsuite/query/algebra/geosparql/GeoSPARQLManifestTest.java <#4593 (comment)>: > - manifests.add( - GeoSPARQLManifestTest.class.getClassLoader() - .getResource("testcases-geosparql/functions/manifest.ttl") - .toExternalForm()); - while (!manifests.isEmpty()) { - String pop = manifests.pop(); - SPARQLQueryTestManifest manifest = new SPARQLQueryTestManifest(pop, null, false); - tests.addAll(manifest.getTests()); - manifests.addAll(manifest.getSubManifests()); - } - - Object[][] result = new Object[tests.size()][6]; - tests.toArray(result); - - return result; + public Collection<DynamicTest> tests() { There are several other similar methods that were missing the @testfactory <https://github.com/testfactory> annotation. Did you look over the rest of the code? — Reply to this email directly, view it on GitHub <#4593 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHQYFJ77D5YYXEWO5T5AALYDZJDRAVCNFSM6AAAAAAYKM6VKCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRVGE2TINRUHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

JervenBolleman · 2023-11-10T20:00:15Z

@hmottestad you where completely right I had missed a @testfactory and it was needed! thanks and have a good weekend.

hmottestad

@JervenBolleman thanks for these fixes! You can go ahead and merge when you want.

JervenBolleman self-assigned this May 22, 2023

JervenBolleman added this to the 5.0.0 milestone May 22, 2023

JervenBolleman linked an issue May 22, 2023 that may be closed by this pull request

Migrate RepositorySPARQLComplianceTestSuite to JUnit5 #4592

Closed

JervenBolleman requested a review from abrokenjester May 22, 2023 20:40

JervenBolleman added the ⛔ Not backwards compatible A change that breaks backwards compatibility and needs a major release label May 22, 2023

JervenBolleman requested review from hmottestad and removed request for abrokenjester October 31, 2023 19:39

JervenBolleman force-pushed the GH-4592-junit5-repository-sparql-compliance branch from ef38ad4 to 8c33d62 Compare October 31, 2023 21:48

JervenBolleman force-pushed the GH-4592-junit5-repository-sparql-compliance branch from e9d57aa to f289202 Compare November 3, 2023 10:28

hmottestad reviewed Nov 10, 2023

View reviewed changes

JervenBolleman added 5 commits November 10, 2023 16:42

GH-4592 Replace a Junit4 Suite with v5 DynamicTest

21fb00e

Also replaces a static so more tests should be runnable in parallel in the future. Signed-off-by: Jerven Bolleman <[email protected]>

GH-4592 Replace more Junit4 Suite with v5 DynamicTest

b7fc16b

Also delete some deprecated tests classes that are no longer compatible with the junit5 way of doing things. Signed-off-by: Jerven Bolleman <[email protected]>

GH-4592 Make sure tests do not share state as they run concurrently.

940e316

Signed-off-by: Jerven Bolleman <[email protected]>

GH-4592 Reduce code duplication and difference to the develop branch.

7beed16

Signed-off-by: Jerven Bolleman <[email protected]>

GH-4592 Reenable geosparql junit testing.

da3e01e

Allow for non DAWG approved tests to run. Adds a manifest-all.ttl file for geosparql like sparql 1.1. compliance. Also remove some unused imports. Signed-off-by: Jerven Bolleman <[email protected]>

JervenBolleman force-pushed the GH-4592-junit5-repository-sparql-compliance branch from 4f3ba48 to da3e01e Compare November 10, 2023 15:43

GH-4592 Fix missing @testfactory

04143fb

hmottestad removed the ⛔ Not backwards compatible A change that breaks backwards compatibility and needs a major release label Nov 22, 2023

hmottestad requested review from hmottestad and removed request for hmottestad November 22, 2023 19:00

hmottestad approved these changes Nov 22, 2023

View reviewed changes

JervenBolleman merged commit bb7ac22 into develop Nov 23, 2023
9 checks passed

JervenBolleman deleted the GH-4592-junit5-repository-sparql-compliance branch November 23, 2023 09:10

hmottestad added the M3 label Jun 5, 2024

hmottestad removed this from the 5.0.0 milestone Jun 5, 2024

hmottestad removed the M3 label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-4592 Replace a Junit4 Suite with v5 DynamicTest #4593

GH-4592 Replace a Junit4 Suite with v5 DynamicTest #4593

JervenBolleman commented May 22, 2023

JervenBolleman commented May 22, 2023

hmottestad commented Oct 31, 2023

JervenBolleman commented Oct 31, 2023

hmottestad commented Oct 31, 2023

JervenBolleman commented Nov 1, 2023 •

edited

Loading

JervenBolleman commented Nov 3, 2023

kenwenzel commented Nov 3, 2023

JervenBolleman commented Nov 3, 2023

hmottestad Nov 10, 2023 •

edited

Loading

hmottestad Nov 10, 2023

JervenBolleman Nov 10, 2023

JervenBolleman Nov 10, 2023

JervenBolleman Nov 10, 2023

hmottestad Nov 10, 2023

hmottestad commented Nov 10, 2023

hmottestad Nov 10, 2023

JervenBolleman Nov 10, 2023

hmottestad Nov 10, 2023 •

edited

Loading

hmottestad Nov 10, 2023

JervenBolleman Nov 22, 2023

hmottestad Nov 22, 2023

JervenBolleman commented Nov 10, 2023

JervenBolleman commented Nov 10, 2023 via email

JervenBolleman commented Nov 10, 2023

hmottestad left a comment

GH-4592 Replace a Junit4 Suite with v5 DynamicTest #4593

GH-4592 Replace a Junit4 Suite with v5 DynamicTest #4593

Conversation

JervenBolleman commented May 22, 2023

JervenBolleman commented May 22, 2023

hmottestad commented Oct 31, 2023

JervenBolleman commented Oct 31, 2023

hmottestad commented Oct 31, 2023

JervenBolleman commented Nov 1, 2023 • edited Loading

JervenBolleman commented Nov 3, 2023

kenwenzel commented Nov 3, 2023

JervenBolleman commented Nov 3, 2023

hmottestad Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmottestad commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmottestad Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JervenBolleman commented Nov 10, 2023

JervenBolleman commented Nov 10, 2023 via email

JervenBolleman commented Nov 10, 2023

hmottestad left a comment

Choose a reason for hiding this comment

JervenBolleman commented Nov 1, 2023 •

edited

Loading

hmottestad Nov 10, 2023 •

edited

Loading

hmottestad Nov 10, 2023 •

edited

Loading