Show dialog if JUnit 5 plug-in test launch detects unrestricted junit-jupiter-api in plug-in dependencies

[trancexpress]

See: #2013 (comment)

In essence, we have a problem when JUnit 5 and JUnit 6 are in the target platform (which will soon be the case for the Eclipse SDK, see: eclipse-jdt/eclipse.jdt.ui#2535).

Given a MANIFEST.MF like this:

Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: TestPlugin
Bundle-SymbolicName: TestPlugin
Bundle-Version: 1.0.0.qualifier
Require-Bundle: org.junit,
 junit-jupiter-api
Automatic-Module-Name: TestPlugin
Bundle-RequiredExecutionEnvironment: JavaSE-21

Running a JUnit 5 plug-in test launch configuration will result in an odd error in the launched JVM:

Caused by: java.util.ServiceConfigurationError: org.junit.platform.engine.TestEngine: org.junit.jupiter.engine.JupiterTestEngine not a subtype

This is due to adding JUnit 5 to the launch (we fix this in #2013), but the launch already having JUnit 6 due to the unrestricted junit-jupiter-api in the MANIFEST.MF.

We opted for aborting the launch with a specific error message, which indicates what the problem is and how to fix it.

See also the long discussion here for more context: #2013 (comment)

The problem is avoided by restricting the junit-jupiter-api version to [5,6) (thanks to #2013), which the new error should indicate.

[HannesWell]

The problem is avoided by restricting the junit-jupiter-api version to [5,6) (thanks to #2013), which the new error should indicate.

Yes this would be very good to have. But I want to emphazise that we could make it clear that it's generally a missmtach between the bundles in the runtime and the JUnit version kind specified in the launch configuration.

So while it's correct that one can restrict the required/imported version of the corresponding junit-jupiter bundles (and I would generally advice to do so). Another solution is to update the JUnit version/kind in the launch configuration. The error message should make this clear.
For some users it might be easier to just update the launch configuration or to just delete the existing one and recreate it.
Newly created launch configuration should of course detect the correct JUnit version out of the box.

Furthermore we should error if there are two (different major) versions of junit-jupiter bundles present in the runtime (maybe checking for the jupiter-engine is sufficient). That will probably always cause problems at runtime.
Altough the most common case will be the junit-jupiter-engine in version 6 in a launch config configured to use JUnit-5.

You might find inspiration from the dialog that pops up when you have only JUnit-5 bundles in the runtime but have the launch config set up for JUnit-4:

This text is defined in

eclipse.pde/ui/org.eclipse.pde.unittest.junit/src/org/eclipse/pde/unittest/junit/launcher/Messages.properties

Line 17 in 1d67ed6

JUnitPluginLaunchConfigurationDelegate_error_notests_kind=No tests found with test runner ''{0}''.

and you can just check the callers (but admittedly it works a bit differently).

[laeubi]

Furthermore we should error if there are two (different major) versions of junit-jupiter bundles present in the runtime (maybe checking for the jupiter-engine is sufficient). That will probably always cause problems at runtime.

Basically having Junit 5+6 in the launch config and even in the tests should not be a problem, I thought that was the goal of the recent changes in PDE? If not wee need to further adapt this! The problem is often more to have a mixture of JUnit 5 versions because then you get resolution errors but thats the case for other libraries as well and we can't fix the world.

Beside that unrestricted imports are a general problem and we should enhance PDE at all places to use ranges whenever possible and give errors/warnings if the are not used, this is not specific to JUnit.

[trancexpress]

Basically having Junit 5+6 in the launch config and even in the tests should not be a problem, I thought that was the goal of the recent changes in PDE?

No:

With #2013 we allowed to be able to restrict the requirements in MANIFEST.MF and have a working JUnit 5 launch.

That didn't work - the code that added JUnit bundles, if junit-jupiter-api is required, wasn't adding specific JUnit versions. So e.g. you could not launch JUnit 5 tests using JUnit 5 bundles and Java 11, the PDE code would always add JUnit 6 to the launch.

And there were other bugs, such as the validation dialog popping during tests.

In #2011 PDE was again adding JUnit 6, so a JUnit test launch with JRE 11 would not work.

If not wee need to further adapt this! The problem is often more to have a mixture of JUnit 5 versions because then you get resolution errors but thats the case for other libraries as well and we can't fix the world.

I'm not sure we can do anything here, other than what this ticket proposes - explain the error to the user.

In #2013 I proposed removing JUnit 6 that would be pulled in the launch with a manifest like this, but this has problems:

E.g. suppose a plug-in with a manifest like in the description, suppose JUnit 6 has some new API that is also used by code in the plug-in - compilation will be fine, since the unrestricted junit-jupiter-api version will mean using JUnit 6. In the launch we see a JUnit 5 type launch, so we add JUnit 5 bundles. Suppose we do remove JUnit 6, then the test code using the new API doesn't work.

The user has to be made aware that MANIFEST.MF and launch type need to be in sync, due to having both JUnit 5 and JUnit 6 as options.

Let me know if you have other suggestions though, it would be great if we can do more without harm.

[laeubi]

That didn't work - the code that added JUnit bundles, if junit-jupiter-api is required, wasn't adding specific JUnit versions.

Then it has to be fixed, but

The user has to be made aware that MANIFEST.MF and launch type need to be in sync, due to having both JUnit 5 and JUnit 6 as options.

I don't think this is completely true, if I have no restriction in the Manifest then both versions are allowed, according to the selected runtime I then pull in that version and OSGi should resolve that, the problem is more that there is no relation between the test-probe (bundle) and the runtime (launcher), this can be fixed with some PDE magic and I did already do that somehow with OSGi testing support by generate an additional fragment for the test-probe that adds missing requirements.

So unless JUnit 5+6 are binary compatible from the API there is no issue compiling against JUnit 6 and running it with JUnit 5...

[trancexpress]

So unless JUnit 5+6 are binary compatible from the API there is no issue compiling against JUnit 6 and running it with JUnit 5...

I assume you mean binary incompatible. Right now, probably no. In future, probably yes (e.g. new APIs).

if I have no restriction in the Manifest then both versions are allowed

But without a version restriction, always the highest version of the bundle is picked for everything?

[trancexpress]

I've set a PR with my proposal:

#2055

Whatever we put in the message of the new exception will be what is in the error dialog:

When we have a JUnit 6 type launch, this should be extended to cover that mixed case too.

An alternative, replace JUnit 6 bundles with JUnit 5, we discussed here:

#2013 (comment)

@merks also had a similar suggestion: d1fe8d3

The code is hopefully still somewhere, if we want to go that route. The problem I see is potentially obscure errors the user has to deal with, due to having some forgotten old launch still on JUnit 5. Personally I prefer failing early with a clear message.

Furthermore we should error if there are two (different major) versions of junit-jupiter bundles present in the runtime (maybe checking for the jupiter-engine is sufficient).

Lets focus on fixing the issues we create with PDE code. It will definitely be nice to tell the user that their manifest(s) have broken requirements, but to me this is a nice-to-have.

[HannesWell]

Beside that unrestricted imports are a general problem and we should enhance PDE at all places to use ranges whenever possible

That's right. In the past, steps have already been made in this direction but there are still places missing. But in general that's another problem, I suggest to handle at a different issue/discussion.
For now lets just assume somebody has a dependency to junit-6, maybe even explicitly specified in the MANIFEST.MF and tries to launch a configuration specifing still JUnit-5, maybe it was simply forgotten to update it.

That didn't work - the code that added JUnit bundles, if junit-jupiter-api is required, wasn't adding specific JUnit versions.

Then it has to be fixed, but

That was fixed in #2013.

I don't think this is completely true, if I have no restriction in the Manifest then both versions are allowed, according to the selected runtime I then pull in that version and OSGi should resolve that

While OSGi should resolve it, you can see with the described scenario, that it fails when creating the engine because different org.junit.platform.engine.TestEngine may be used at runtime, as it was desribed in #2013 (comment).

So unless JUnit 5+6 are binary compatible from the API there is no issue compiling against JUnit 6 and running it with JUnit 5...

I assume you mean binary incompatible. Right now, probably no. In future, probably yes (e.g. new APIs).

I share this.

if I have no restriction in the Manifest then both versions are allowed

But without a version restriction, always the highest version of the bundle is picked for everything?

Not necessarily. In general the resolve tries to reduce the number of resolved wires and used bundles. So if a bundle in the runtime has a strict requirement to junit-jupiter 5 and another one has an unbound requirement to junit-jupiter, then it might be that only 5 is wired. But most of the time it means the latest version.

An alternative, replace JUnit 6 bundles with JUnit 5, we discussed here:

#2013 (comment)

The code is hopefully still somewhere, if we want to go that route. The problem I see is potentially obscure errors the user has to deal with, due to having some forgotten old launch still on JUnit 5. Personally I prefer failing early with a clear message.

I prefer this too.