Lifecycle callback methods in enclosing class are invoked with ExtensionContext for test executing in @Nested test class

[sbrannen]

Overview

A comment in SPR-15366 has made me aware of a potential bug in JUnit Jupiter's handling of @BeforeEach methods with regard to the ExtensionContext supplied to a ParameterResolver registered for an enclosing test class when executing a test method within a @Nested test class.

Specifically, Jupiter invokes a @BeforeEach method in the enclosing class using the ExtensionContext for a @Test method in a @Nested class. The same may be true for other lifecycle callback methods as well, but I have not investigated that.

Analysis

It would appear that the cause of this behavior is due to a combination of how ClassTestDescriptor.invokeMethodInExtensionContext(Method, ExtensionContext, ExtensionRegistry) and TestMethodTestDescriptor.invokeBeforeEachMethods(JupiterEngineExecutionContext) operate. The latter invokes each synthesized BeforeEachMethodAdapter using the ExtensionContext for the current test method instead of supplying the ExtensionContext for the context in which the method is declared. Consequently, if a ParameterResolver is asked to resolve a parameter for a @BeforeEach method in an enclosing class and the resolver needs the test class (or information tied to the test class via annotations) to resolve the parameter, then the resolver gets the test class for the currently executing @Nested test class when invoking extensionContext.getRequiredTestClass().

However, this behavior does not appear to apply to the invocation of TestInstancePostProcessor implementations as can be seen in TestInstancePostProcessorTests.

Similarly, the behavior is different for a ParameterResolver applied to the constructor of an enclosing class for the currently executing @Nested test class.

Deliverables

• [ ] Determine which lifecycle callback methods are affected by this.
  • [ ] Ensure appropriate tests are in place for @BeforeAll
  • [ ] Ensure appropriate tests are in place for @AfterAll
  • [ ] Ensure appropriate tests are in place for @BeforeEach
  • [ ] Ensure appropriate tests are in place for @AfterEach
• [ ] Ensure that extensions registered and invoked for a given lifecycle callback are supplied an appropriate ExtensionContext.

[sbrannen]

In some test code that I have locally, I have modified one of our test parameter resolvers as follows.

public class CustomTypeParameterResolver implements ParameterResolver {

	@Override
	public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
		assertCorrectExtensionContext(parameterContext, extensionContext);

		return parameterContext.getParameter().getType().equals(CustomType.class);
	}

	@Override
	public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
		assertCorrectExtensionContext(parameterContext, extensionContext);

		return new CustomType();
	}

	private static void assertCorrectExtensionContext(ParameterContext parameterContext,
			ExtensionContext extensionContext) {

		// Ensure that this ParameterResolver is being invoked in the correct ExtensionContext
		assertEquals(parameterContext.getDeclaringExecutable().getDeclaringClass(),
			extensionContext.getRequiredTestClass());
	}

}

With the status quo in Jupiter, use of the above resolver fails as outlined in this issue's description.

Here we can see that the declaring class for the current method (e.g., a @BeforeEach method) does not match the "test class" returned by the supplied ExtensionContext.

If one wished to argue that this is not a bug, I suppose one could argue that a ParameterResolver should never rely on the "test class" in the current ExtensionContext but rather only use the "declaring class" of the method for which the resolver is resolving the parameter.

I could possibly change the behavior of the SpringExtension to work like that, but... it feels wrong to me.

More importantly, the behavior described in this issue is in direct contrast to the behavior for TestInstancePostProcessor implementations as well as ParameterResolver extensions applied to constructors.

@junit-team/junit-lambda, Thoughts?

[sbrannen]

@marcphilipp, thanks for adding the "team discussion" label!

I was just about to do that. ;-)

[sbrannen]

FYI: I updated this issue's overview and analysis.

[sbrannen]

Regardless of the fate of this issue, the Javadoc for ExtensionContext.getTestClass():

Get the Class associated with the current test or container, if available.

... is not specific about what "current" means with regard to extensions being applied within a @Nested test class structure.

One could infer that the current test class is the currently executing nested test class, but I think we should at least aim to be more explicit in the documentation with regard to @Nested classes.

[sbrannen]

If one wished to argue that this is not a bug, I suppose one could argue that a ParameterResolver should never rely on the "test class" in the current ExtensionContext but rather only use the "declaring class" of the method for which the resolver is resolving the parameter.

Actually, I take that back.

The "declaring class" of the method may be a superclass or interface. So that is not a viable solution.

[marcphilipp]

You could check the target. If it's absent, the method must be static or it's a constructor in which case the declaring class cannot be a superclass or interface, right?

[sbrannen]

Well.... in case you mean checking the class for the value returned from ExtensionContext.getTestInstance()... that would hopefully be the instance for the currently executing nested test... assuming that is aligned with the "current test class".

Otherwise, wouldn't that imply that we have a bug in the state of the supplied ExtensionContext? 😉

[sbrannen]

Oh wait... I guess you actually meant ParameterContext.getTarget().

Mea culpa. 😇

[sbrannen]

So, yeah, I guess that would work for a @BeforeEach method or a non-static @BeforeAll method.

But... it wouldn't help for a static @BeforeAll method declared in a superclass or interface -- right?

[sbrannen]

In any case, I'm slowly starting to think that this is perhaps not a bug per se but rather just "the nature of the beast"... even if it's sometimes not that intuitive.

[sbrannen]

In the end, it might just be that extensions have to provide their own support for configuration "inheritance" within nested class structures to overcome situations like the one described in the linked Spring JIRA issue.

But let's have that offline discussion we talked about anyway, just to hash things out. 😉

[bechte]

@sbrannen, I would also like to "pin" my thoughts into this issue here.

First of all I'd like to divide this issue into two cases:

1. A typical hierarchical test (not using parameter resolvers)
2. A hierarchical test using parameter resolvers (no matter where)

If we forget about ParameterResolvers for a moment, I feel that the behaviour is valid. An extension is called within the context of declaration, i.e. at the point in time when the actual hierarchy level is reached. So to speak: a @BeforeAll in the main class is executed with the context of that class while a @BeforeAll of a @Nested test class receives the context of the nested class. This looks good and valid to me. I hope we are together here. 😃

With ParameterResolvers, the game somehow changes, but I wonder why. We clearly need to define, what the expectations of a resolver is. Normally, I would state that a ParameterResolver in a higher hierarchy level should not know anything about the @Nested context classes and, therefore, shall not respond on those. This is what I understand in your statement "the nature of the beast". To me, a resolver on a higher level of the hierarchy is only able to respond on events and state on that level or above. In the end a ParameterResolver is a helper for the actual method, e.g. @BeforeAll to fetch the required data in order to perform its work. And those methods "live" on a certain level of the hierarchy and they should only look up. Looking into deeper levels of the hierarchy feels like using Reflections or using a "crystal ball" to foresee the "future".

Having said that, I really think there is some kind of "bug" here in JUnit Jupiter, because the ParameterResolver in the SpringExtension gets called with the @Nested context, which I think is not natural and should be considered a bug.

Still, this would mean a breaking change, and we have to discuss the effect of this change...

[marcphilipp]

Just so we’re on the same page, here are a couple observations/clarifications about the current behavior (which should actually be in the Javadoc or User Guide).

The ExtensionContext passed to a ParameterResolver describes the currently executing test. Its hierarchy represents the test tree.

For example, let's consider the following test class:

class A {
  @BeforeEach void setup(Object x) {}
  @Test void a() {}
  @Nested class B {
    @Test void b() {}
  }
}

There are five ExtensionContext instances present here:

junit-jupiter
└── A
    ├── a()
    └── B
        └── b()

A ParameterResolver registered with A would be called for the setup(Object) method twice:

1. When executing a() with the ExtensionContext of a()
2. When executing b() with the ExtensionContext of b()

Why isn’t it always called with the ExtensionContext of A? First, I think we can all agree that the execution of a @BeforeEach method belongs to the execution of a test, not a test class. Second, it would make it impossible put an object into the Store that’s only relevant for the execution of a() or b(), e.g. to clean up a parameter in an AfterEachCallback.

---

Now, if I understood it correctly, Spring’s requirement is to get access to the test class setup(Object) is called for in its implementation of ParameterResolver.

In the simple example above, one could call parameterContext.getDeclaringExecutable().getDeclaringClass(). However, the setup(Object) method could be declared in a superclass of A so that wouldn’t work reliably. Instead, one could use getTarget().map(Object::getClass).orElse(null) to derive the test class from the test class instance. However, that wouldn’t work for static methods and constructors. To solve this dilemma, we could introduce ParameterContext.getTestClass() that would return test class on behalf of which it is being called.

However, I’m wondering whether the Spring extension couldn’t just store a reference to the application context in the Store of the ExtensionContext that is passed to its TestInstancePostProcessor implementation and then retrieve it from there in its ParameterResolver (and other callbacks) without having to reason about the test class at all. Moreover, that would also make it possible to check if there’s already an application context when instantiating a nested class and failing/extending the existing application context, if necessary.

@sbrannen Would it be possible to implement it this way?

[bechte]

Why isn’t it always called with the ExtensionContext of A? First, I think we can all agree that the execution of a @BeforeEach method belongs to the execution of a test, not a test class. Second, it would make it impossible put an object into the Store that’s only relevant for the execution of a() or b(), e.g. to clean up a parameter in an AfterEachCallback.

I do not share this though. Test hierarchies are more or less a possibility for grouping and scoping tests and the variables used. On the opposite, the behavior described above is what I would expect from a real class hierarchy, i.e. when the methods are inherited onto the context the test are executed in. This is the primary difference between test class inheritance and test class hierarchies:

junit-jupiter
└── A
    ├── a()
    └── B
        └── b()

Referring to your example above, in a hierarchy, everything is defined on a level (refer to a execution stack) and lives on that level (like a local stack variable). Therefore, the @BeforeEach method defined for class A should always be executed with the context of class A, as it does not know anything about the @Nested context classes (e.g. B). There is no semantical connection, neither does the Java programming language defines this. Executing the @BeforeEach method on class A with the context of the test b() just feels wrong in my point of view (inner classes know about the enclosing class, but not the other way around).

Having said that, I think we really should modify the way the extensions are called today. If the @BeforeEach methods shall be inherited, one could use inheritance (or default methods and implementing the defining interfaces). But this is a different use-case that should not be covered by @Nested test hierarchies. At least this was never intended when I came up with the idea while inventing the HierarchicalContextRunner...

In the case of the SpringExtension, I do understand why people think that the Spring application context should be available in the @Nested class context. The application context is representing a state, and state is normally expressed by variables on the level. Those variables can be accessed from within the @Nested class context, because they are defined in a enclosing class. The same holds for the "state of the application context". So we have a semantically related pattern here, that awakens expectations. But this is something the SpringExtension has to solve on its own, because it is a very special use-case. Still, in order to implement things right, it would be great if those @BeforeEach methods are called at the right context level, so the required and expected information is available. 😃

Does that make it a bit more clear? Otherwise, just let us talk...

[sbrannen]

To solve this dilemma, we could introduce ParameterContext.getTestClass() that would return test class on behalf of which it is being called.

Team Decision: the team agrees that the introduction of such a method is the bare minimum that should be done in order to provide extension authors enough information to make appropriate decisions.

However, how that method should be named is up for debate, and the team agrees that getTestClass() would not be a good name.

[sbrannen]

I did a bit of brainstrorming and came up with an idea for providing further information to extension authors -- in case we decided the proposed new method is insufficient.

We could introduce a new NestingContext interface that is returned by ExtensionContext.getNestingContext():

Optional<NestingContext> getNestingContext();

It would likely be Optional since there is no nesting context for a top-level or static nested class.

The NestingContext could provide additional information (names and details not yet clear) -- for example:

• Class<?> getOutermostEnclosingClass(): the outermost enclosing class for the current nested test class hierarchy
• Class<?> getCurrentNestedClass(): the nested class within the nested test class hierarchy that is currently being evaluated/used -- analogous to the proposed ParameterContext.getTestClass() (which will be named something other than getTestClass())
• Class<?> getTestClass(): the same thing as getTestClass() in ExtensionContext thus likely unnecessary to have duplicated
• List<Class<?>> getClassesInNestedHierarchy(): get all classes within the currently executing nested class hierarchy, including the outermost top-level class (which technically is not nested), preserving nested order (top-down)

The list of classes supplied in the last method could of course be determined iteratively using reflection by starting with the ExtensionContext.getRequiredTestClass() and traversing up the enclosing class hierarchy until you reach a non-nested class (i.e., the outermost top-level class).

So.... just "food for thought". 😉

[Klaboe]

Not sure if this falls in the same category as the described issue, but mocking around with Nested tests, i observed that a defined BeforeAllCallback is called twice when using @Nested

Simplified example:

I have defined an Extension that starts a Wiremock-server (with a fixed port), using an overidden beforAll from the BeforeAllCallback interface.

Extending my testclass with this Extension and staring the test will result in an java.net.BindException: Address already in use: bind when using @Nested. I.e the Wiremock-server is started twice, and the second time it fails because an instance is already running on the same port.

So it looks like the beforeAll is runned twice; once for the encapsulating class, and once more for the inner (nested) class.

Does this confirm

The same may be true for other lifecycle callback methods as well, but I have not investigated that.

@sbrannen ?

[marcphilipp]

@Klaboe You're describing the expected behavior. If you want to start it only once, you can do sth. like this:

import org.junit.jupiter.api.extension.BeforeAllCallback;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
import org.junit.jupiter.api.extension.ExtensionContext.Store.CloseableResource;

public class WireMockExtension extends BeforeAllCallback {

    @Override
    public void beforeAll(ExtensionContext context)  {
        context.getStore(Namespace.create(WireMockExtension.class))
                .getOrComputeIfAbsent("server", key -> new StartedWireMockServer());
    }

    static class StartedWireMockServer extends CloseableResource {
        private final WireMockServer server;

        public StartedWireMockServer() {
            server = new WireMockServer();
            server.start();
        }

        @Override
        public void close() {
            server.stop();
        }
    }

}

[sbrannen]

Note that the proposal in https://github.com/junit-team/junit5/issues/1332#issuecomment-379234762 is closely related to the TestInstances API introduced in conjunction with #1618.

[stale[bot]]

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[bot]]

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.