Compute junit5 runtime Plug-ins dynamically in RequiredPlugins-Container and reference junit and hamcrest only via Import-Package

[HannesWell]

This PR does two things, mainly with the goal to not require a specific hamcrest bundle name:

1. Compute junit5 runtime Plug-ins dynamically in RequiredPlugins-Container
   And add support for old junit bundle names from Orbit.
2. Reference junit and hamcrest only via Import-Package
   This requires to import the hamcrest packages, if they are used and those test-plugins don't rely on the re-export of hamcrest by org.junit anymore.

Additionally only use junit-4 assertion methods and replace use of junit-3 assertions.
And remove unnecessary hard-coded references to the hamcrest bundle name.

@merks what do you think?

[HannesWell]

I wonder why org.junit (i.e. the Junit-4 bundle is added), if a Test-Plugin references the JUnit-5 API?
Can anyone tell?

[laeubi]

You can use JUnit 4 API in JUnit5 tests via the junit-vintage-engine (even though it is not really a JUnit5 test then) so one is able to mix JUnit 3/4/5 ...

[HannesWell]

Yes, but then one imports the org.junit packge or requires that bundle?
Or is the intention to make it available without any further addo once one imports the Jupiter-API?

[laeubi]

Actually require it to importpackage/requirebundle junit is bad, why should I ever want that? Beside that you should look at the JUnit Container at JDT that defines the set of junit jars and I think PDE is inspired by that.

Actually one wants to define the used engine (like in BND Tests or Surefire) and then it might be enough to determine transitive requirements.

[HannesWell]

Actually require it to importpackage/requirebundle junit is bad, why should I ever want that?

I more think, why I should ever not want that? All other dependencies needed for compilation have to be specified, why should a Junit dependency used for compilation (not runtime), be different in this case?
We have only dedicated 'test-plugins/fragments' in PDE, so there is IMHO no great need to treat them differently.
And as said in another comment, I have the impression that this can lead to compilation failures in Tycho because in the IDE the classpath sees more jars than in the build.

[github-actions]

Test Results

     246 files  +       6       246 suites  +6   1h 1m 30s ⏱️ + 16m 10s
  3 298 tests  -        1    3 265 ✔️  -        8  24 💤 ±  0  7 ❌ +6  2 🔥 +1 
10 191 runs  +2 721  10 110 ✔️ +2 690  72 💤 +24  7 ❌ +6  2 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit 758234c. ± Comparison against base commit 7dd52e7.

This pull request removes 1 test.

AllPDEMinimalTests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test

♻️ This comment has been updated with latest results.

[laeubi]

If you are already working in that area, it would be great if PDE would not only check imports/require bundles but also the Precence of the JUnit3/4/5 container on the classpath what actually is a much better way than using imports.

[merks]

It looks promising. Of course one has to wonder the origin of the access restriction:

Error: D:\a\eclipse.pde\eclipse.pde\apitools\org.eclipse.pde.api.tools.tests\src\org\eclipse\pde\api\tools\tests\ApiToolsPerformanceTestSuite.java:[17]
Error: import org.junit.runner.RunWith;
Error: ^^^^^^^^^^^^^^^^^^^^^^^^
Error: Access restriction: The type 'RunWith' is not API (restriction on classpath entry 'C:\Users\runneradmin.m2\repository\p2\osgi\bundle\org.junit\4.13.2.v20230725-0701\org.junit-4.13.2.v20230725-0701.jar')

The bundle has this

Archiver-Version                        Plexus Archiver
Automatic-Module-Name                   junit
Build-Jdk                               1.6.0_65
Built-By                                marc
Bundle-ManifestVersion                  2
Bundle-Name                             Bundle junit : junit
Bundle-SymbolicName                     org.junit
Bundle-Version                          4.13.2.v20230725-0701
Created-By                              Apache Maven 3.1.1
Eclipse-Wrapped-Bundle                  junit:junit:4.13.2
Export-Package                          junit.extensions;version="4.13.2";uses:="junit.framework"
                                        junit.framework;version="4.13.2";uses:="org.junit.runner,org.junit.runner.manipulation,org.junit.runner.notification"
                                        junit.runner;version="4.13.2";uses:="junit.framework"
                                        junit.textui;version="4.13.2";uses:="junit.framework,junit.runner"
                                        org.junit.experimental.categories;version="4.13.2";uses:="org.junit.runner,org.junit.runner.manipulation,org.junit.runners,org.junit.runners.model,org.junit.validator"
                                        org.junit.experimental.max;version="4.13.2";uses:="org.junit.runner,org.junit.runner.notification"
                                        org.junit.experimental.results;version="4.13.2";uses:="org.hamcrest,org.junit.runner,org.junit.runner.notification"
                                        org.junit.experimental.runners;version="4.13.2";uses:="org.junit.runners,org.junit.runners.model"
                                        org.junit.experimental.theories.internal;x-internal:=true;version="4.13.2";uses:="org.junit.experimental.theories,org.junit.runners.model"
                                        org.junit.experimental.theories.suppliers;version="4.13.2";uses:="org.junit.experimental.theories"
                                        org.junit.experimental.theories;version="4.13.2";uses:="org.junit.experimental.theories.internal,org.junit.internal,org.junit.runners,org.junit.runners.model"
                                        org.junit.experimental;version="4.13.2";uses:="org.junit.runner,org.junit.runners.model"
                                        org.junit.function;version="4.13.2"
                                        org.junit.internal.builders;x-internal:=true;version="4.13.2";uses:="org.junit.runner,org.junit.runner.notification,org.junit.runners.model"
                                        org.junit.internal.management;x-internal:=true;version="4.13.2"
                                        org.junit.internal.matchers;x-internal:=true;version="4.13.2";uses:="org.hamcrest"
                                        org.junit.internal.requests;x-internal:=true;version="4.13.2";uses:="org.junit.runner,org.junit.runner.manipulation"
                                        org.junit.internal.runners.model;x-internal:=true;version="4.13.2";uses:="org.junit.internal,org.junit.runner,org.junit.runner.notification,org.junit.runners.model"
                                        org.junit.internal.runners.rules;x-internal:=true;version="4.13.2";uses:="org.junit.runners.model"
                                        org.junit.internal.runners.statements;x-internal:=true;version="4.13.2";uses:="org.junit.runners.model"
                                        org.junit.internal.runners;x-internal:=true;version="4.13.2";uses:="junit.framework,org.junit.runner,org.junit.runner.manipulation,org.junit.runner.notification"
                                        org.junit.internal;x-internal:=true;version="4.13.2";uses:="org.hamcrest,org.junit.runner,org.junit.runner.notification"
                                        org.junit.matchers;version="4.13.2";uses:="org.hamcrest,org.hamcrest.core"
                                        org.junit.rules;version="4.13.2";uses:="org.hamcrest,org.junit,org.junit.function,org.junit.internal,org.junit.runner,org.junit.runners.model"
                                        org.junit.runner.manipulation;version="4.13.2";uses:="org.junit.runner"
                                        org.junit.runner.notification;version="4.13.2";uses:="org.junit.runner"
                                        org.junit.runner;version="4.13.2";uses:="junit.framework,org.junit.runner.manipulation,org.junit.runner.notification,org.junit.runners.model,org.junit.validator"
                                        org.junit.runners.model;version="4.13.2";uses:="org.junit.runner"
                                        org.junit.runners.parameterized;version="4.13.2";uses:="org.junit.runner,org.junit.runner.notification,org.junit.runners,org.junit.runners.model"
                                        org.junit.runners;version="4.13.2";uses:="org.junit.internal.runners,org.junit.rules,org.junit.runner,org.junit.runner.manipulation,org.junit.runner.notification,org.junit.runners.model,org.junit.runners.parameterized"
                                        org.junit.validator;version="4.13.2";uses:="org.junit.runners.model"
                                        org.junit;version="4.13.2";uses:="org.hamcrest,org.junit.function,org.junit.internal,org.junit.runners"
Implementation-Title                    JUnit
Implementation-URL                      http://junit.org
Implementation-Vendor                   JUnit
Implementation-Vendor-Id                junit
Implementation-Version                  4.13.2
Import-Package                          org.hamcrest.core;version="1.3"
                                        org.hamcrest;version="1.3"
Manifest-Version                        1.0
Originally-Created-By                   Apache Maven 3.1.1
Require-Bundle                          org.hamcrest.core;bundle-version="1.3.0";resolution:=optional;visibility:=reexport;x-installation:=greedy
Require-Capability                      osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.5))"

So only org.junit.internal packages are marked internal.

[HannesWell]

Of course one has to wonder the origin of the access restriction:

The problem is that PDE seem not to be worried about missing imports of those junit packages.

If you are already working in that area, it would be great if PDE would not only check imports/require bundles but also the Precence of the JUnit3/4/5 container on the classpath what actually is a much better way than using imports.

Sure that's not a problem. Did that alraedy, but I'm not sure what you want to do in that case. Since that container also adds the junit-5 jars, I would say that the addition through PDE can be disabled in this case.
In general I wonder why this is even done? I would say that for compilation the launcher etc is not necessary.
And when the test-runtime is launched the classpath is not relevant.

[HannesWell]

Of course one has to wonder the origin of the access restriction:

The problem is that PDE seem not to be worried about missing imports of those junit packages.

I have the Impression that this Problem is caused/hidden by the Auto-Magic addition of the junit5-runtime/junit4 bundles to the CP.

[laeubi]

And when the test-runtime is launched the classpath is not relevant.

I really doubt that ;-)

PDE currently looks for "junit" in the manifest and then adds "magic stuff" to the classpath (and therefore runtime!) of the project, so if you only add the JUnit container the compilation works but running as plugintest fails because no JUnit "bundles" are found, so PDE should simply interpret the presence of JUnit container as "we require the bundles", where the jars of the bundle are already bundles, the only thing is that as far as I know they are from the installation and not from the target platform!

[HannesWell]

And when the test-runtime is launched the classpath is not relevant.

I really doubt that ;-)

Are you referring to a plain Junit-Test or a JUnit Plugin Test? Because for the latter PDE usually just adds all bundles from the TP to the test runtime and if the launcher is not present it adds the bundles from the running IDE (hoping they work together).
For a plain JUnit-Test you are probably right (but I'm not 100% sure how the test-runtime classpath is assembled in that case).

PDE currently looks for "junit" in the manifest and then adds "magic stuff" to the classpath (and therefore runtime!) of the project, so if you only add the JUnit container the compilation works but running as plugintest fails because no JUnit "bundles" are found, so PDE should simply interpret the presence of JUnit container as "we require the bundles", where the jars of the bundle are already bundles, the only thing is that as far as I know they are from the installation and not from the target platform!

So as far as I understand, the JUnit-Container adds the launcher jars from the running IDE (installation), while this PDE features adds the junit-launcher jars from the TP?

[HannesWell]

@laeubi I think the current state is what you have described?
But together with the JUnit-5 CP container the junit-deps are potentially added with different versions and one has to hope that all combinations work well together.

[laeubi]

Tycho has a rich detection of the required JUnit Framework and supports the JUnit Classpathcontainer beside that one can configure that explicitly if required. Still JDT users seem to be happy enough with "one JUnit" shipped with the IDE.

So PDE should either require to import every requirement or should not require it at all (if junit container is there), but looking for some require bundle and then adding others is simply a hack for the lazy ;-)

[laeubi]

Are you referring to a plain Junit-Test or a JUnit Plugin Test? Because for the latter PDE usually just adds all bundles from the TP to the test runtime and if the launcher is not present it adds the bundles from the running IDE (hoping they work together).
For a plain JUnit-Test you are probably right (but I'm not 100% sure how the test-runtime classpath is assembled in that case).

Just try the attached example in an empty workspace: test.junit.zip

1. Everything compiles
2. Run as JUnit test > works
3. Run as Plugin test > org.junit.runners.model.InvalidTestClassError: Invalid test class 'test.junit.TestCase': No runnable methods
4. Add Require-Bundle: org.junit;bundle-version="4.13.2" to the manifest > works

So even though the junit is in the bundle and selected for the run the test does not work without the imports in PDE.

If you look at the classpath the same bundles are there for the JUnit Container and for Plugin Dependencies

I tried adding a while back some patches for that but they where not accepted to I leave this alone but this makes using JUnit way more complex with PDE than necessary and prevents one from using Plugin tests in the same bundle or need to use hacks like optional imports of junit.

[HannesWell]

So PDE should either require to import every requirement or should not require it at all (if junit container is there), but looking for some require bundle and then adding others is simply a hack for the lazy ;-)

Fully agree and I never said that adding the launcher jars to the class-path is a great thing. :)
And thinking about it again, instead of adding the launcher jars to the classpath they should be supplied during the launch, so this is actually another use-case of eclipse-jdt/eclipse.jdt.core#1183.

That being said, actually the content of the jars just added to the classpath by this 'feature/hack' should not be accessible if the package is not imported or the bundle not required in the Manifest, because the jars are added with the access-rule IAccessRule.K_NON_ACCESSIBLE and I tested it with a simple case and there I get a compile-error if I use @org.junit.Test and only have the package org.junit.jupiter.api imported. So actually this should not change the result, but somehow I noticited when converting some of PDE's test plugins that in some cases I only got a discouraged access warning and later the build failed.
So I assume something else is happening in more complex cases.

Nevertheless I think the discussion lost a bit its focus and I would like to complete this for the actual intend now, i.e. compute the launcher bundles dynamically and support the old BSN's and make PDE ready for a newer hamcrest version.
We can still discuss this further in a separate issue and enhance/change this later. @laeubi do you want to create an issue for the case you have discribed?