Skip to content

Introduce mechanism for programmatic extension management #506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
1 task
sbrannen opened this issue Sep 10, 2016 · 23 comments
Open
1 task

Introduce mechanism for programmatic extension management #506

sbrannen opened this issue Sep 10, 2016 · 23 comments

Comments

@sbrannen
Copy link
Member

sbrannen commented Sep 10, 2016

Background

This issue addresses some of the topics discussed in #112.

Status Quo

It is currently possible to register extensions declaratively via @ExtendWith on types and methods, and #497 will support programmatic extension registration via fields. However...

  • Developers cannot alter the order of registered extensions.
  • Developers cannot remove a registered extension.
  • Developers cannot insert an extension into the list of registered extensions -- for example, at the front or somewhere in the middle.

Rationale

In order to avoid the introduction of an overly complex declarative model for sorting, appending, prepending, inserting, and removing extensions, we have opted for a programmatic means to achieve all of the above. Providing a programmatic means to achieve these goals frees developers to manage registered extensions as they see fit without any unnecessary restrictions imposed on them by the framework itself.

Considerations

Such a mechanism could itself be an Extension registered declaratively via @ExtendWith; however, the current thinking in the team is that there should be one and only one such component registered at any given time. Since this is such a special case which affects all extensions which have been registered declaratively, it is therefore considered best to introduce a new declarative mechanism for registering this single component. Similarly, the API for such a component should therefore not extend Extension since doing so would allow users to incorrectly register it via @ExtendWith (in which case it would simply be silently ignored).

Proposal

  • Introduce an ExtensionManager API that receives a list of all registered extensions as input and returns a list of extensions.
  • Introduce a @ManageExtensionsWith annotation that allows the user to declare a single class reference for an implementation of ExtensionManager.

Related Issues

Deliverables

  • Introduce a mechanism for programmatic extension management.
@sormuras
Copy link
Member

sormuras commented Dec 4, 2017

Gist from https://stackoverflow.com/questions/47633819/injecting-test-classes-created-by-platform-launcher ... allow extension instances be part of a LauncherDiscoveryRequest:

    LauncherDiscoveryRequest request = LauncherDiscoveryRequestBuilder
            .request()
            .selectors(...)
            .extendWith(TestInstancePostProcessor.class, new MyExtension("local value", what, ever))
            .build();

@sbrannen
Copy link
Member Author

sbrannen commented Dec 7, 2017

@sormuras, the proposed extendWith() method in the launcher API does not address the issue of extension management. Rather, it simply provides another means for programmatic extension registration.

So, unless I'm missing something, the proposal is not relevant to this GitHub issue.

Furthermore, I am not a fan of registering extensions as mere objects at the level of the launcher API: I think extension registration should be left to the individual test engine implementations as it is now. In any case, that is a topic best suited for a different GitHub issue.

@marcphilipp marcphilipp modified the milestones: 5.1 M2, 5.1 Backlog Jan 8, 2018
@tomblench
Copy link

I'd just like to add that we have existing JUnit 4 tests which used RuleChain and it would be very useful for us to programatically declare extensions in a certain order because there are dependencies between the extensions and therefore the order in which their callbacks run is important.

I can see that if I change the name of my extensions their callbacks will run in the desired order, but this is clearly not sustainable as this behaviour is likely to be JVM-dependent.

My guess is that somewhere behind the scenes, these extensions are being fetched with Class. getFields() where "The elements in the returned array are not sorted and are not in any particular order" (from the javadoc).

@tomblench
Copy link

In the meantime I've written a custom extension which runs the callbacks in the order the extensions were passed in to the constructor: https://gist.github.com/tomblench/999634f126e215132dcad2b0eccec870

@sormuras
Copy link
Member

Looks like a plain and simple solution. Do you register an instance of MultiExtension via 5.1's @RegisterExtension in your test class?

@tomblench
Copy link

@sormuras that's correct

@filiphr
Copy link
Contributor

filiphr commented Aug 15, 2018

I am not sure that I completely understood the proposal. From what I can see this would mean that users will have to register the ordering explicitly. However, as a library writer I would like to have control on the way my extensions are ordered, maybe even control how they are ordered in relation to other libraries (SpringExtension).

Proposal

Why not allow Extension to have a single default method getOrder() with some default value 0. This would allow new extensions to be written and have the order implemented to provide custom order and go in front or after others.

For example we have a base extension that provides the basic support for what we need. Additionally we want to offer support for Spring, i.e. get the required beans from the ApplicationContext and provide them to our extension.

@sbrannen
Copy link
Member Author

sbrannen commented Aug 15, 2018

@filiphr, thanks for the feedback.

The getOrder() method you are proposing is actually what I did for extensions in Spring. AbstractTestExecutionListener implements Spring's Ordered interface which defines a getOrder() method.

That seems to work OK for the Spring ecosystem, since most of the key events are defined/supported by TestExecutionListener implementations from Core Spring.

We discussed such an approach for JUnit Jupiter but in the end decided against it... since most extensions used with JUnit Jupiter will in fact not come from the JUnit Team.

In other words, it would be difficult for various, competing third-party extensions to decide on a common number scheme to use for values returned from getOrder().

Of course, we are open to ideas and brainstorming on how to solve such a problem in the greater JUnit ecosystem.

@sbrannen
Copy link
Member Author

For example we have a base extension that provides the basic support for what we need. Additionally we want to offer support for Spring, i.e. get the required beans from the ApplicationContext and provide them to our extension

There's likely no need to "order" your extension relative to the SpringExtension.

If you need beans from the Spring ApplicationContext, you can simply delegate to the SpringExtension via the static getApplicationContext(ExtensionContext) method. 😉

@sbrannen
Copy link
Member Author

sbrannen commented Aug 15, 2018

@tomblench, that's a pretty nice workaround for the time being.

However, such an approach has a few drawbacks.

  • it does not have proper exception handling and/or guarantees about which callbacks are invoked in case of failure.
  • it invokes "after" callbacks in the order in which they are registered instead of in reverse order.
    • in other words, proper "wrapping" is not enforced.

@tomblench
Copy link

That's a good point on the "wrapping", I guess I didn't notice because for my limited use case it didn't matter.

@sbrannen
Copy link
Member Author

sbrannen commented Aug 15, 2018

Yeah, we take great care to ensure proper wrapping.

Note the use of registry.getReversedExtensions(AfterAllCallback.class) below and in other such cases for extensions that are invoked "after" an extension point.

private void invokeAfterAllCallbacks(JupiterEngineExecutionContext context) {
ExtensionRegistry registry = context.getExtensionRegistry();
ExtensionContext extensionContext = context.getExtensionContext();
ThrowableCollector throwableCollector = context.getThrowableCollector();
registry.getReversedExtensions(AfterAllCallback.class)//
.forEach(extension -> throwableCollector.execute(() -> extension.afterAll(extensionContext)));
}

@filiphr
Copy link
Contributor

filiphr commented Aug 16, 2018

There's likely no need to "order" your extension relative to the SpringExtension.

Yes the SpringExtension was a bad example (you've done a good job on it for the order not to matter).

In other words, it would be difficult for various, competing third-party extensions to decide on a common number scheme to use for values returned from getOrder().

I don't see why it would be difficult. Me as an extension writer will write an extension that has the default order, i.e. I don't care that much about it. Then I can have another extension that actually requires a different 3rd party extension that should run before my first one and after the 3rd party. Knowing this I can define a public static int field in my extension that would have it's order, this way me as an extension provider have control over my own and support for other extensions. I don't see a case where there would be cyclic dependencies between extensions, if there is really such need then those 2 teams would need to work together to come up with a better solution 😄.

@sbrannen
Copy link
Member Author

sbrannen commented Aug 16, 2018

We're not concerned about cyclic dependencies.

Rather, we are concerned about extension authors knowing what a valid/reasonable "order value" would be for a particular type of extension.

The problem for the community is that such order values would typically be static in nature (e.g., hard coded as you suggested by public static int). An extension author could of course programmatically/dynamically decide on what the "order value" should be right now, but for that to work, extensions would have to provided information regarding all currently registered extensions -- in order to make an informed decision.

But even in the case of the latter, how "informed" can an extension be?

It obviously cannot know about every extension that has ever been developed or will be developed.

@sbrannen
Copy link
Member Author

In the end, even if JUnit Jupiter provides a mechanism for extensions to decide on their own "order value", there will be scenarios that require user intervention in order to override the extension-supplied order value due to incompatibilities with competing third-party extensions that know nothing of each other.

@sbrannen
Copy link
Member Author

Update: I've added a Deliverable for consideration of support for @Order in conjunction with @RegisterExtension.

@sebersole
Copy link

Being able to have JUnit show the "effective extensions" (which are applied, and what order) would be a good preliminary step in helping to work around some of these concerns.

@sebersole
Copy link

Another quick thought that would work (at least for my use cases, not sure about generally)... How about the ability for an extension to register that it depends on another extension? E.g.

@ExtendWith( ShouldBeSecond.class, dependsOn={ShouldBeFirst.class} )
@ExtendWith( ShouldBeFirst.class )
class MyTest ...

I know in this trivial example I can just re-order the @ExtendWith declarations. In my real use-case however, the extensions are registered across different "meta annotations" which I have found to be very difficult to get the ordering just right.

@stale
Copy link

stale bot commented May 13, 2021

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the status: stale label May 13, 2021
@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically closed due to inactivity. If you have a good use case for this feature, please feel free to reopen the issue.

@sbrannen
Copy link
Member Author

sbrannen commented Aug 3, 2021

@sbrannen sbrannen reopened this Aug 3, 2021
@stale stale bot removed the status: stale label Aug 3, 2021
@stale
Copy link

stale bot commented Aug 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the status: stale label Aug 3, 2022
@jbduncan
Copy link
Contributor

jbduncan commented Aug 5, 2022

This issue still seems useful to have around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants