Optionally provide test-scoped ExtensionContext for test-specific callbacks in extensions

[JojOatXGME]

I am currently writing a small extension which implements ParameterResolver to provide a few parameters which need to be shut down after the test. After reading the documentation, my first intuition was to write the extension like this:

final class MyExtension implements ParameterResolver {
    private static final Namespace NAMESPACE = Namespace.create(MyExtension.class);

    @Override
    public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
        return parameterContext.getParameter().getType() == Parameter.class;
    }

    @Override
    public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
        return extensionContext
                .getStore(NAMESPACE)
                .getOrComputeIfAbsent(ParameterWrapper.class)
                .parameter;
    }

    private static final class ParameterWrapper implements CloseableResource {
        private final Parameter parameter;

        private ParameterWrapper() {
            this.parameter = new Parameter();
        }

        @Override
        public void close() {
            parameter.close();
        }
    }
}

The problem with that solution is that the ExtensionContext is scoped for the whole test class when resolving arguments for the test class constructor, although the test uses TestInstance.Lifecycle.PER_METHOD. As a consequence, my example above will use the same parameter for all tests, instead of creating a new one per test. The documentation does not describe a lot of details about the lifecycle of each ExtensionContext, therefore I cannot declare it as a bug, but this behavior is very unexpected in my opinion. Since the callbacks are on the level of the individual tests, I expected the ExtensionContext to be scoped to the test as well. The same problem applies to TestInstancePreConstructCallback. I haven't tested TestInstanceFactory, but I guess it is also affected.

The obvious workaround is to also add a TestInstancePreDestroyCallback which closes the parameter and removes it from the store. Note that the implementation of that method is not trivial because in contrast to the methods above, this callback receives a child store which is scoped to the test. Unfortunately, this means that calling remove on the given store directly instead of iterating over the parents would not work.

    @Override
    public void preDestroyTestInstance(ExtensionContext context) {
        ParameterWrapper wrapper = null;
        do {
            wrapper = context.getStore(NAMESPACE).remove(ParameterWrapper.class, ParameterWrapper.class);
            context = context.getParent().orElse(null);
        } while (context != null && wrapper == null);
        if (wrapper != null) wrapper.close();
    }

Deliverables

• Test-scoped ExtensionContext for InvocationInterceptor.interceptTestClassConstructor(…)
• Test-scoped ExtensionContext for ParameterResolver while resolving constructor parameters
• Test-scoped ExtensionContext for TestInstancePreConstructCallback
• Test-scoped ExtensionContext for TestInstancePostProcessor
• Test-scoped ExtensionContext for TestInstanceFactory
• Consider Expose test method to test instance constructor extensions #3670 in the implementation

-- or --

• Documentation of the limitation in the Javadoc of the interfaces or callbacks.

Related issues

• Cannot get executing test method in ParameterResolver of @MethodSource factory method #3323
• Expose test method to test instance constructor extensions #3670

[marcphilipp]

We agree that this is a valid use case that should be simpler to accomplish. We'd like to investigate whether it's feasible to inject the method-level ExtensionContext if Lifecycle.PER_METHOD is used and, if so, for which extension interfaces.

[JojOatXGME]

Great to hear. If there is anything I can help with, I am happy to hear it.

[victorherraiz]

TestInstancePreConstructCallback and TestInstancePreDestroyCallback are not symmetric, the context of the latter is the child of the former. Therefore, removecalls from the latter fails, the store is in the parent.
TestInstancePreConstructCallback recives a context that outlives the test using the default config when the extenxion is configured in the test class.
In my opinion, ParameterResolver and TestInstancePreConstructCallback should receive a context just for the current test if the life cycle is configured per method.

[JojOatXGME]

I just wanted to make my extension work with @Nested test classes, but the asymmetry makes it a bit tricky. It is not trivial to identify which call of preDestroyTestInstance needs to shut down the extension.

[JojOatXGME]

I created a PR at #4032. Also note that I added TestInstancePostProcessor to the “deliverables” in the issue description, as this callback was affected as well.

[marcphilipp]

@JojOatXGME Thanks for the PR! I don't think this can be changed in general since it could potentially break existing extensions (as you've pointed out in #1568 (comment)). The reason these extension method are always called with the class-scoped ExtensionContext is to make them agnostic of the configured test instance lifecycle.

Instead, I think we need to provide a way for an extension to opt-in to getting the closest extension context or even failing if the test instance lifecycle is PER_CLASS. This could be achieved by an annotation on the ExtensionContext parameter of the implementing method or on the extension class. For example:

final class MyExtension implements ParameterResolver {

    @Override
    public Object resolveParameter(
		ParameterContext parameterContext,
		@TestScoped ExtensionContext extensionContext
	) {
        return extensionContext
                .getStore(NAMESPACE)
                .getOrComputeIfAbsent(ParameterWrapper.class)
                .parameter;
    }

	// ...
}

I'm open to other suggestions, though.

[JojOatXGME]

@marcphilipp Sorry for my long message, I just had a lot to say to this topic. 😅

I could probably implement such annotation as you have suggested, but it might add noticeable complexity to the implementation. I also want to highlight that I think that the current behavior is rather problematic, and that the new behavior should feel more like the new default.

Current Issues

The reason these extension method are always called with the class-scoped ExtensionContext is to make them agnostic of the configured test instance lifecycle.

While this is a desirable goal, I don't think that it has worked out in this case. ExtensionContext.getElement() and ExtensionContext.getTestClass() seem to be the only methods which benefit from the harmonized behavior. Both of these methods could be replaced 1-to-1 by TestInstanceFactoryContext.getTestClass(). In other words, the gain seems very minimal to me. Extensions would just have to call a different existing method. At the same time, the current approach has various noticeable drawbacks.

• getUniqueId(), getDisplayName(), getTags() and getTestMethod() are less specific, or not available.
• Using the Store in any of the affected callbacks tends to leak data between test methods. (Note that if the context wants to store any data related to the created instance, this issue also makes it harder to create agnostic extensions, as they would otherwise only work with PER_CLASS by default.)
• Parameter injection is not supported by most extensions in my experience, probably due to this very issue.
• The Store of the affected callbacks doesn't see data previously stored by TestTemplateInvocationContext. (New extension with test templates #2970)
• The current behavior seems quite unintuitive to me. Who expects that the creation of a test specific instance is not scoped to the test? Also note that TestInstancePreDestroyCallback is actually scoped to the test, showing that even JUnit itself is not consistent with that reasoning.

Regarding my points about the Store, I hope I could show that it becomes very hard to use the Store in the affected callbacks for anything but caching of immutable resources. Data which is changed by the tests should usually not leak between tests, which is currently only possible by disabling parallel execution AND a workaround similar to what I have written in the ticket description. Note that my workaround still breaks for @Nested tests.

Alternative suggestion

As I have written above, I would like to have a solution which would feel more natural, i.e. more like the new default. Unfortunately, I don't have a very good idea to achieve that. However, I would at least propose to create the annotation on class level. This has the benefit that you wouldn't have to repeat the annotation an every affected callback you are using.

@MethodContextOnInstanceCreation // Not sure about a good name
final class MyExtension implements ParameterResolver {
    ...
}

Alternatively, we could also introduce a marker interface.

final class MyExtension implements Extension2, ParameterResolver {
    ...
}

I also want to note that ParameterResolver is probably the least affected callback. For this specific callback, we could probably just provide the new context. After all, this callback already needs to work with test-specific scopes when it is used on methods.

even failing if the test instance lifecycle is PER_CLASS.

I would rather favor some method on the ExtensionContext to allow for easy preconditions. Note that some extensions might actually be about supporting the instance creation, in this case you don't want to distinguish between the two lifecycles.

final class MyExtension implements Extension2, ParameterResolver {
    @Override
    public Object resolveParameter(
        ParameterContext parameterContext,
        ExtensionContext extensionContext
    ) {
        if (extensionContext.getScope().isBelowTest()) {
            throw new IllegalStateException();
        }
        ...
    }
}

Assessing the Risk

When I started to write this message, I was also not sure whether it might be worth to risk “breaking” the existing behavior. I am still not entirely sure about that, but I see that it would be quite risky. I put “breaking” in quotes because the current behavior isn't documented. Based on the documentation, my implementation actually feels like the more natural interpretation to me. Anyway, here are my considerations while I thought about it, just in case someone is interested.

Click here to expand ...

JUnit made a very similar change in #3905 concerning ParameterResolver in combination with @MethodSource. Of course, there was less risk involved as the feature was only introduced in version 5.9. I also pointed out already that ParameterResolver is probably the least risky callback. Besides, let's see, what are the actual changes which could break an extension?

• getElement() would return the method, not the class. This is probably the most problematic change. I did usually avoid using this method due to its ambiguity. (I cannot think of a use case where it is not important where I am looking for the annotation.) However, it is fair to assume that various extensions have started to assume that this method always returns the test class currently instantiated, although it was never specified or documented like that. TestInstanceFactoryContext.getTestClass() would continue to work for these use cases.

• getTestClass() would return the most-inner test class, not the test class currently instantiated. While there is TestInstanceFactoryContext.getTestClass() which continues to return the instantiated class, some extensions might have treated these methods interchangeably. Note that this will only affect tests with @Nested test classes.

There are some more methods which would change their behavior, but I think they are way less problematic.

• getParent() would return the extension context of the most inner test class. I am not entirely sure about its use cases, but the only use cases that come to mind would require an iteration over all parents, and would therefore not really be affected.
• getUniqueId() would return the ID for the test method, not the class. Not sure what an extension may do with that besides writing it into reports. I don't expect many tests to rely on this method in a very significant way.
• getDisplayName() would return the display name of the specific test. Extensions shouldn't use the name for any significant logic.
• getTags() would see all tags. I doubt that not hiding tags will break any extension, except if they did something really hacky.
• getTestMethod() would no-longer be empty.
• getStore(Namespace) would become much more usable for the affected callback. Maybe we could break some extensions if they applied workarounds for the current behavior.

[marcphilipp]

Thanks for explaining your thought process so thoroughly! 👍

We'll discuss this in the team and will get back to you.

I tend to agree that the new behavior should be the default. Rather than making it opt-in via an annotation, we could make it opt-out via a configuration parameter that would specify a comma-separated list of class name patterns (to support use case where the code of the problematic extension can't be edited, e.g. when it's in a third-party dependency). This could cause breakages when updating but there would be a switch that allows reverting to the old behavior and supports gradually updating one's extensions.

[JojOatXGME]

I tend to agree that the new behavior should be the default. [...] we could make it opt-out via a configuration parameter

Thanks. I also like the idea with the configuration parameter.

I also just realized that we could also still make it opt-in at first, but already document that it will become the default. This way, extension vendors would have some time to adapt their extensions. Hopefully, it would encourage them to opt-in for future compatibility. However, I guess there might be many extensions where the authors are not actively monitoring the change log of JUnit, so not sure how much this would actually help. After the default is changed, the annotation (or what ever we used to opt-in) would become a no-op.

[marcphilipp]

Team decision: Make the new behavior opt-in via a new class-level annotation (@MethodLevelExtensionContextAware or similar) and document that the new behavior will become the default in 6.0.

@JojOatXGME Do you have time to change your PR to add the annotation or should I take a stab at it?

[JojOatXGME]

@marcphilipp I can try to implement it over the next week.