Provide only compatible JUnit 5 test engines in RemotePluginTestRunner

[HeikoKlare]

Currently, it is possible that the RemotePluginTestRunner loads bundles with "incompatible" test engines. This means, the engine is version-incompatible with the TestEngine interface. In such a case, the execution will fail, because the ServiceLoader tries to load this engine as a provider for the TestEngine service but fails because it cannot assign the engine to the TestEngine interface. One such case is the execution of tests with Tycho, which can use a different JUnit version than the one used in the JDT test bundles.

This change ensures that bundles providing incompatible engines are not added to the class loader used for executing the test to ensure that service loading for the engines does not fail.

How to Reproduce

An example for the error is the execution of session tests with JUnit 5, as proposed in eclipse-platform/eclipse.platform#1086. There, the session test org.eclipse.core.tests.runtime.jobs.Bug_412138 is ported to JUnit 5, but fails because the org.eclipse.tycho.surefire.osgibooter is found as an engine provider and provides incompatible engine versions. See for example this log:

java.util.ServiceConfigurationError: org.junit.platform.engine.TestEngine: org.junit.jupiter.engine.JupiterTestEngine not a subtype
	at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1244)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1273)
	at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309)
	at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393)
	at java.base/java.lang.Iterable.forEach(Iterable.java:74)
	at org.junit.platform.launcher.core.LauncherFactory.collectTestEngines(LauncherFactory.java:160)

With this fix applied, the test runs fine, as the the engine is not loaded from the org.eclipse.tycho.surefire.osgibooter bundle's class loader, but from the one of the test engine bundles.

[github-actions]

Test Results

   291 files  +   98     291 suites  +98   1h 7m 14s ⏱️ + 34m 52s
 3 526 tests ±    0   3 468 ✅ +    2   58 💤  -  1  0 ❌  - 1 
10 875 runs  +3 677  10 699 ✅ +3 608  176 💤 +70  0 ❌  - 1 

Results for commit 29f596c. ± Comparison against base commit f26df87.

♻️ This comment has been updated with latest results.

[HeikoKlare]

@akurtakov @HannesWell May I ask (one of) you to have a look at this? Having this merged would allow me to proceed working on modernizing (i.e., migrating to JUnit 5) platform's session tests: eclipse-platform/eclipse.platform#1086

[HannesWell]

@akurtakov @HannesWell May I ask (one of) you to have a look at this? Having this merged would allow me to proceed working on modernizing (i.e., migrating to JUnit 5) platform's session tests: eclipse-platform/eclipse.platform#1086

I'll have a look at it over the weekend.
The main point that I want to check is if the new dependency is trivial. The reason is that org.eclipse.pde.junit.runtime runs in JVM's launched from a dev-eclipse together with the bundles from the TP. Therefore they can be (much) older than those currently in use and (probably) not even contain the JUnit-5 bundles.
But I have to check all of that.

[HeikoKlare]

Thanks, Hannes!
I see, that's an interesting point. I can also change the implementation to be more resilient and also work when JUnit 5 bundles are not available. Then the dependency can either be made optional or replaced by using fully qualified names instead of the TestEngine class.

[HannesWell]

OK, I checked the JUnit Plugin test launchers and it looks like as expected that one can launch such test without Junit-5.
But depending on the selected JUnit version in the launch-config there is code to ensure the required bundles for that version are present:

eclipse.pde/ui/org.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java

Lines 535 to 549 in a53dc16

	public static Collection<String> getRequiredJunitRuntimePlugins(ILaunchConfiguration configuration) {
	ITestKind testKind = JUnitLaunchConfigurationConstants.getTestRunnerKind(configuration);
	if (testKind.isNull()) {
	return Collections.emptyList();
	}
	List<String> plugins = new ArrayList<>();
	plugins.add("org.eclipse.pde.junit.runtime"); //$NON-NLS-1$
	if (TestKindRegistry.JUNIT4_TEST_KIND_ID.equals(testKind.getId())) {
	plugins.add("org.eclipse.jdt.junit4.runtime"); //$NON-NLS-1$
	} else if (TestKindRegistry.JUNIT5_TEST_KIND_ID.equals(testKind.getId())) {
	plugins.add("org.eclipse.jdt.junit5.runtime"); //$NON-NLS-1$
	}
	return plugins;
	}

and

eclipse.pde/ui/org.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java

Lines 501 to 505 in a53dc16

	if (fAllBundles.containsKey("junit-platform-runner") || fAllBundles.containsKey("org.junit.platform.runner")) { //$NON-NLS-1$ //$NON-NLS-2$
	// add launcher and jupiter.engine to support @RunWith(JUnitPlatform.class)
	requiredPlugins.add("junit-platform-launcher"); //$NON-NLS-1$
	requiredPlugins.add("junit-jupiter-engine"); //$NON-NLS-1$
	}

So yes, the code should be prepared to handle a completely unavailable TestEngine class and should not add a mandatory requirement to its package. I added explicit suggestions below, based on the FQN approach.

But in general I'm surprised this code is used directly in Tycho.

[HeikoKlare]

@HannesWell Thanks for the in-depth review and your suggestions. I have incorporated the changes in f4e9f33 so that no strict dependency to JUnit 5 is required anymore.

But in general I'm surprised this code is used directly in Tycho.

Just for clarificaiton: It is not used "directly" in Tycho, but rather "indirectly". One consumer is the session test framework, which starts a remote test run via the RemotePluginTestRunner from within a test case, and that test case is executed by Tycho. Maybe it would also be possible to "cleanup" the classloader in the session test framework when starting a remote test run, but I thought there may be other situations than a Tycho run in which incompatible engine versions are on the classpath, so it makes sense to make the runner capable of dealing with that situation.

[HannesWell]

Maybe it would also be possible to "cleanup" the classloader in the session test framework when starting a remote test run, but I thought there may be other situations than a Tycho run in which incompatible engine versions are on the classpath, so it makes sense to make the runner capable of dealing with that situation.

Thanks for the clarification. I think it is ok to do it here. This should not harm normal situations.

In general the change now looks good, I just have one small point open.

[HannesWell]

It looks like Class.forName() never returns null.
Further I assume there are no class loading tricks where a different TestEngine class is a super class of this bundle's one? Therefore an equals (maybe even identity?) check should be sufficient, shouldn't it?

Suggested change

	return thisTestEngine != null && bundleTestEngine != null && thisTestEngine.isAssignableFrom(bundleTestEngine);
	return thisTestEngine.equals(bundleTestEngine);

[HeikoKlare]

Yes, an identity comparison is sufficient here (fixed with 19e3dd5). I had two false assumptions:

• I accidentally expected isAssignableFrom() to return true for two instances of the same class loaded by different class loaders. Since that's the operation used by the ServiceLoader that fails when incompatible engine versions are used, I reused it here.
• I expected Class.forName() to return null if the engine is not on the classpath, but it will of course throw an exception. A scenario where I would expect this happens to is as follows: execute the runner without any JUnit 5 dependencies, but use the argument -runasjunit5 and have some bundle specifying a service provider for "org.junit.platform.engine.TestEngine".

[HannesWell]

A scenario where I would expect this happens to is as follows: execute the runner without any JUnit 5 dependencies, but use the argument -runasjunit5 and have some bundle specifying a service provider for "org.junit.platform.engine.TestEngine".

I assume each bundle providing a TestEngine requires that class to implement it. And therefore, if the OSGi metadata are correct, it should depend on the providing bundle/package.
So I don't expect that to happen, but we'll see what specialties occur. ^^

[HeikoKlare]

So I don't expect that to happen

Me neither 🙂 I should have mentioned that the situation may only occur with invalid metadata (i.e., the declaration of a service that is actually not provided). And even for such a very unlikely, accidental case it might be good to fail in a more "useful" way than throwing an NPE.

And, of course, thanks for merging!

[HannesWell]

Looks good now, thank you.