[junit-platform tester] Sub-request to handle plain DiscoverySelector

[kriegfrj]

The latest version of junit-platform-engine has changed its internal discovery algorithm, and in doing so exposed a bug in the JUnit Platform tester.

The bug relates to the following code:

bnd/biz.aQute.tester.junit-platform/src/aQute/tester/bundle/engine/discovery/BundleSelectorResolver.java

Lines 520 to 533 in 362f0ae

	@Override
	public <T extends DiscoverySelector> List<T> getSelectorsByType(Class<T> selectorType) {
	info(() -> "Getting selectors from sub-request for: " + selectorType);
	if (selectorType.equals(ClassSelector.class) || selectorType.equals(MethodSelector.class)) {
	return selectors.stream()
	.filter(selectorType::isInstance)
	.map(selectorType::cast)
	.collect(toList());
	}
	if (selectorType.equals(BundleSelector.class)) {
	return new ArrayList<>();
	}
	return request.getSelectorsByType(selectorType);
	}

The aim of this method is to resolve any raw ClassSelector or MethodSelector selectors (ie, those specified using Strings for the class names) into the versions that have fully loaded Class instances. This allows it to take care of the OSGi aspects of the classloading (which plain JUnit Platform is not adequately aware of), and it also turns a BundleSelector (which only BundleEngine understands) into ClassSelectors that can be understood by the sub-engines.

Under the older version of JUnit Platform, calls to EngineDiscoveryRequest.getSelectorsByType() were always made using the individual concrete subclasses ClassSelector and MethodSelector. So the code path would go via L524 and return our fully-resolved version of the selectors rather than the original selector instance (which may include a String version).

Under the new version, it first calls getSelectorsByType() with the interface DiscoverySelector.class:

https://github.com/junit-team/junit5/blob/master/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/EngineDiscoveryRequestResolution.java#L80

This causes it to bypass L524 of BundleSelectorResolver and instead delegate to the original request.

In the common use case (running all tests in one or more bundles), the original request will typically comprise one or more BundleSelectors and nothing else. So these BundleSelectors will be returned. These are in turn passed on to the sub-engine for discovery, but the sub-engine, not understanding what a BundleSelector is, will simply ignore them. Thus, it doesn't discover any tests (hence the behaviour @bjhargrave discovered in eclipse-osgi-technology/osgi-test#70).

The fix:

• Instead of selectorType.equals(), in L523, we should use something like selectorType.isAssignableFrom().
• Always delegate to the original request, filter the result to remove BundleSelectors, ClassSelectors and MethodSelector, and add the pre-resolved versions of ClassSelector/MethodSelector instances to the return value if appropriate.

Unfortunately this bug makes junit-platform tester 5.0.0 unusable with JUnit 5.6. I'll look at a fix, and perhaps we can consider cherrypicking it into a 5.0.1 release or something like that.

[bjhargrave]

The fix:

* Instead of `selectorType.equals()`,  in L523, we should use something like `selectorType.isAssignableFrom()`.

I am not sure this will do anything useful. selectorType.isAssignable(ClassSelector.class) and selectorType.isAssignable(MethodSelector.class) will always return true if the method is called with DiscoverySelector.class since that is the supertype.

* Always delegate to the original request, filter the result to remove `BundleSelector`s, `ClassSelector`s and `MethodSelector`, and add the pre-resolved versions of `ClassSelector`/`MethodSelector` instances to the return value if appropriate.

Agree.

Unfortunately this bug makes junit-platform tester 5.0.0 unusable with JUnit 5.6. I'll look at a fix, and perhaps we can consider cherrypicking it into a 5.0.1 release or something like that.

Yes, this definitely seems like a bug we should fix for 5.0.1.

[bjhargrave]

I updated the Known Issues for Bnd 5.0.0 to reference this bug.

[bjhargrave]

I think this may be the fix based upon your suggestion above:

diff --git a/biz.aQute.tester.junit-platform/src/aQute/tester/bundle/engine/discovery/BundleSelectorResolver.java b/biz.aQute.tester.junit-platform/src/aQute/tester/bundle/engine/discovery/BundleSelectorResolver.java
index bf98f6c78..7d951735a 100644
--- a/biz.aQute.tester.junit-platform/src/aQute/tester/bundle/engine/discovery/BundleSelectorResolver.java
+++ b/biz.aQute.tester.junit-platform/src/aQute/tester/bundle/engine/discovery/BundleSelectorResolver.java
@@ -520,16 +520,15 @@ public class BundleSelectorResolver {
                @Override
                public <T extends DiscoverySelector> List<T> getSelectorsByType(Class<T> selectorType) {
                        info(() -> "Getting selectors from sub-request for: " + selectorType);
-                       if (selectorType.equals(ClassSelector.class) || selectorType.equals(MethodSelector.class)) {
-                               return selectors.stream()
-                                       .filter(selectorType::isInstance)
-                                       .map(selectorType::cast)
-                                       .collect(toList());
-                       }
-                       if (selectorType.equals(BundleSelector.class)) {
-                               return new ArrayList<>();
-                       }
-                       return request.getSelectorsByType(selectorType);
+                       return Stream.concat(request.getSelectorsByType(selectorType)
+                               .stream()
+                               .filter(selector -> !(selector instanceof ClassSelector || selector instanceof MethodSelector
+                                       || selector instanceof BundleSelector)),
+                               selectors.stream()
+					.filter(selector -> selectorType.isInstance(selector) && !(selector instanceof BundleSelector)))
+                               .map(selectorType::cast)
+                               .collect(toList());
                }
 
                @Override

[bjhargrave]

I tested the PR fix for eclipse-osgi-technology/osgi-test#70 (comment) and all the tests now run.

[INFO] --- bnd-testing-maven-plugin:5.1.0-SNAPSHOT:testing (testing) @ org.osgi.test.junit5 ---
[INFO] Matching glob *
[INFO] Bnd inferred -runee: JavaSE-1.8

Test run finished after 1865 ms
[         9 containers found      ]
[         0 containers skipped    ]
[         9 containers started    ]
[         0 containers aborted    ]
[         9 containers successful ]
[         0 containers failed     ]
[        26 tests found           ]
[         0 tests skipped         ]
[        26 tests started         ]
[         0 tests aborted         ]
[        26 tests successful      ]
[         0 tests failed          ]

[INFO] No Errors

[bjhargrave]

@kriegfrj Great job in debugging this problem and identifying the solution!