Add possibility to register launcher-discovery listeners via SPI #2457

filiphr · 2020-10-21T18:06:15Z

LauncherDiscoveryListeners can now be registered via Java's ServiceLoader
mechanism in addition to those
passed as part of each LauncherDiscoveryRequest and the default one.

Overview

The idea of this PR is to enable registering LauncherDiscoveryListener through an SPI. I am creating this PR in order to hear the opinion of the JUnit Team. Is something like this acceptable for you? If it is I am going to add add the documentation and finish whatever needs to be finished from your feedback.

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

`LauncherDiscoveryListeners` can now be registered via Java's `ServiceLoader` mechanism in addition to those passed as part of each `LauncherDiscoveryRequest` and the default one.

codecov · 2020-10-21T19:06:48Z

Codecov Report

Merging #2457 (95b2cad) into main (a8d08e0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2457      +/-   ##
============================================
+ Coverage     90.39%   90.41%   +0.02%     
- Complexity     4572     4584      +12     
============================================
  Files           399      400       +1     
  Lines         11292    11324      +32     
  Branches        909      910       +1     
============================================
+ Hits          10207    10239      +32     
  Misses          808      808              
  Partials        277      277

Impacted Files	Coverage Δ	Complexity Δ
.../junit/platform/launcher/core/DefaultLauncher.java	`96.87% <100.00%> (+0.10%)`	`11.00 <1.00> (+1.00)`
.../platform/launcher/core/DefaultLauncherConfig.java	`100.00% <100.00%> (ø)`	`8.00 <1.00> (+1.00)`
...orm/launcher/core/EngineDiscoveryOrchestrator.java	`100.00% <100.00%> (ø)`	`25.00 <3.00> (+2.00)`
...g/junit/platform/launcher/core/LauncherConfig.java	`100.00% <100.00%> (ø)`	`1.00 <0.00> (ø)`
...launcher/core/LauncherDiscoveryRequestBuilder.java	`100.00% <100.00%> (ø)`	`19.00 <0.00> (ø)`
.../junit/platform/launcher/core/LauncherFactory.java	`100.00% <100.00%> (ø)`	`12.00 <2.00> (+5.00)`
...erviceLoaderLauncherDiscoveryListenerRegistry.java	`100.00% <100.00%> (ø)`	`4.00 <4.00> (?)`
...g/junit/platform/testkit/engine/EngineTestKit.java	`75.34% <100.00%> (ø)`	`17.00 <1.00> (ø)`
...junit/platform/launcher/TestExecutionListener.java	`85.71% <0.00%> (-14.29%)`	`6.00% <0.00%> (-1.00%)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8d08e0...95b2cad. Read the comment docs.

marcphilipp · 2020-10-23T10:00:19Z

Tentatively slated for 5.8 M1 solely for the purpose of team discussion.

filiphr · 2020-10-24T10:08:13Z

Just FYI doing something like this allows to have an SPI change the current thread context classloader and actually can help with #201 and #806. I needed to use a different classloader for loading the classes that are discovered and adding my own classloader before the discovery is started is one way to achieve this.

I am sure that this is not the only use case for this, and the use case I have does not really fit this, I am sure that there are other more suitable use cases for this.

marcphilipp · 2020-10-25T16:30:04Z

...launcher/src/main/java/org/junit/platform/launcher/core/LauncherDiscoveryRequestBuilder.java

-		if (discoveryListeners.contains(defaultDiscoveryListener)) {
+		if (discoveryListeners.contains(defaultDiscoveryListener) && serviceLoaderListeners.isEmpty()) {
 			return LauncherDiscoveryListeners.composite(discoveryListeners);
 		}
 		List<LauncherDiscoveryListener> allDiscoveryListeners = new ArrayList<>(discoveryListeners);
-		allDiscoveryListeners.add(defaultDiscoveryListener);
+		allDiscoveryListeners.addAll(serviceLoaderListeners);
+		if (!allDiscoveryListeners.contains(defaultDiscoveryListener)) {
+			allDiscoveryListeners.add(defaultDiscoveryListener);
+		}
 		return LauncherDiscoveryListeners.composite(allDiscoveryListeners);


I don't think this optimization is necessary. Let's get rid of the first if.

Sure no problem, I was not entirely sure about this part as well, it didn't feel quite right anyways. I'll make a small optimisation so that the list is created with a correct size at least (to avoid resizes)

Frontrider · 2020-10-28T01:48:08Z

So this would let us stuff like changing the class path for the tests?

filiphr · 2020-10-28T07:07:26Z

I want to be really clear that the goal of this PR is not to fix the issues that need different classpath for the tests. I don't want the JUnit team to reject this PR solely on the fact of allowing users to change the classloader.

However, it indeed allows implementors of this interface to change the current thread context classloader in LauncherDiscoveryListener#LauncherDiscoveryListener. This only works when the discovery selectors are created without the class already loaded, i.e. only with the class name. JUnit Jupiter the uses this constructor to load the class for the tests.

marcphilipp

Looks good! Could you please add this to the user guide and release notes as well?

...launcher/src/main/java/org/junit/platform/launcher/core/LauncherDiscoveryRequestBuilder.java

…and user guide

filiphr · 2020-10-28T21:46:57Z

Looks good! Could you please add this to the user guide and release notes as well?

Thanks. I just pushed the update to the user guide and release notes.

One thing that I just remembered. What would you like the default flag to be set to. Currently discovering LauncherDiscoveryListener(s) is an opt-in. Users have to explicitly enable it via junit.platform.discovery.listener.autodetection.enabled

marcphilipp · 2020-10-30T11:32:12Z

Team decision: Let's include this feature in 5.8.

Whether discovery listeners are picked up via ServiceLoader is now configurable via `LauncherConfig` for consistency with all other APIs potentially loaded that way. Moreover, discovery listeners are now registered via ServiceLoader by default.

…to-detection

marcphilipp

@filiphr I took the liberty of reworking a few parts based on your implementation. I hope you don't mind!

filiphr · 2020-12-21T14:55:04Z

Really appreciated @marcphilipp. Thanks for the work on it.

One small comment though, I think that you missed a @since 1.8 on the isLauncherDiscoveryListenerAutoRegistrationEnabled() in the LauncherConfig. I saw that you added some missing @since 1.7 and wanted to let you know.

junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/LauncherConfig.java

marcphilipp · 2020-12-21T16:42:13Z

I merged this manuaally in 8b71ee6 to resolve the merge conflict. I'm glad this is now in place. Thanks for the contribution and patience, @filiphr! 👍

filiphr · 2020-12-21T18:09:05Z

That's great news @marcphilipp. Looking forward in using this and finalising the migration to JUnit 5 in the MapStruct project.

Add possibility to register launcher-discovery listeners via SPI

68b2244

`LauncherDiscoveryListeners` can now be registered via Java's `ServiceLoader` mechanism in addition to those passed as part of each `LauncherDiscoveryRequest` and the default one.

filiphr force-pushed the launcher-discovery-auto-detection branch from 5a8c228 to 68b2244 Compare October 21, 2020 18:55

marcphilipp added the status: team discussion label Oct 23, 2020

marcphilipp added this to the 5.8 M1 milestone Oct 23, 2020

marcphilipp reviewed Oct 25, 2020

View reviewed changes

Apply review feedback

0973542

filiphr force-pushed the launcher-discovery-auto-detection branch from 73349b8 to 0973542 Compare October 26, 2020 21:25

marcphilipp self-requested a review October 27, 2020 07:07

marcphilipp requested changes Oct 28, 2020

View reviewed changes

...launcher/src/main/java/org/junit/platform/launcher/core/LauncherDiscoveryRequestBuilder.java Outdated Show resolved Hide resolved

Add LauncherDiscoveryListener detection via SPI to the release notes …

8fb0fd4

…and user guide

marcphilipp removed the status: team discussion label Oct 30, 2020

filiphr mentioned this pull request Nov 12, 2020

Introduce extension API for providing a custom ClassLoader (e.g., for Powermock) #201

Closed

marcphilipp added 2 commits December 21, 2020 13:29

Register discovery listeners by default

5657b1f

Whether discovery listeners are picked up via ServiceLoader is now configurable via `LauncherConfig` for consistency with all other APIs potentially loaded that way. Moreover, discovery listeners are now registered via ServiceLoader by default.

Merge remote-tracking branch 'origin/main' into launcher-discovery-au…

eef7f5c

…to-detection

marcphilipp approved these changes Dec 21, 2020

View reviewed changes

marcphilipp reviewed Dec 21, 2020

View reviewed changes

junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/LauncherConfig.java Show resolved Hide resolved

mpkorstanje mentioned this pull request Dec 21, 2020

Introduce junit-platform-suite-engine #2416

Merged

7 tasks

marcphilipp and others added 2 commits December 21, 2020 16:20

Fix integration test

e201503

Add missing since tag

95b2cad

marcphilipp force-pushed the launcher-discovery-auto-detection branch from 994d405 to 95b2cad Compare December 21, 2020 16:09

marcphilipp closed this in 8b71ee6 Dec 21, 2020

filiphr deleted the launcher-discovery-auto-detection branch December 21, 2020 18:09

sbrannen added component: Platform type: enhancement theme: discovery labels Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to register launcher-discovery listeners via SPI #2457

Add possibility to register launcher-discovery listeners via SPI #2457

filiphr commented Oct 21, 2020

codecov bot commented Oct 21, 2020 •

edited

Loading

marcphilipp commented Oct 23, 2020

filiphr commented Oct 24, 2020

marcphilipp Oct 25, 2020

filiphr Oct 26, 2020

Frontrider commented Oct 28, 2020

filiphr commented Oct 28, 2020

marcphilipp left a comment

filiphr commented Oct 28, 2020

marcphilipp commented Oct 30, 2020

marcphilipp left a comment

filiphr commented Dec 21, 2020

marcphilipp commented Dec 21, 2020

filiphr commented Dec 21, 2020

Add possibility to register launcher-discovery listeners via SPI #2457

Add possibility to register launcher-discovery listeners via SPI #2457

Conversation

filiphr commented Oct 21, 2020

Overview

Definition of Done

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

marcphilipp commented Oct 23, 2020

filiphr commented Oct 24, 2020

marcphilipp Oct 25, 2020

Choose a reason for hiding this comment

filiphr Oct 26, 2020

Choose a reason for hiding this comment

Frontrider commented Oct 28, 2020

filiphr commented Oct 28, 2020

marcphilipp left a comment

Choose a reason for hiding this comment

filiphr commented Oct 28, 2020

marcphilipp commented Oct 30, 2020

marcphilipp left a comment

Choose a reason for hiding this comment

filiphr commented Dec 21, 2020

marcphilipp commented Dec 21, 2020

filiphr commented Dec 21, 2020

codecov bot commented Oct 21, 2020 •

edited

Loading