spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.62k stars 38.13k forks source link

Provide guidance on using `@Nested` within test classes with complex inheritance and annotation arrangements #28466

Open GeorgiPetkov opened 2 years ago

GeorgiPetkov commented 2 years ago

I've provided a minimal JUnit Jupiter test ConcreteServiceTest reproducing the problem. Please check the code first. The issue is that the mock is not reset so the second test fails since there are 2 calls instead of the expected 1. The use case has a common base service and similarly base test class.

Some observations:

Spring Boot version: 2.6.2

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.RepeatedTest;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.context.annotation.Import;
import org.springframework.test.context.junit.jupiter.SpringExtension;

import static org.mockito.Mockito.verify;

@Import(ConcreteService.class)
class ConcreteServiceTest extends BaseServiceTest {
}

@ExtendWith(SpringExtension.class)
abstract class BaseServiceTest {

    @MockBean
    private Runnable dependency;
    @Autowired
    private BaseService service;

    @Nested
    class Suite {

        @RepeatedTest(2)
        void test() {
            service.serve();

            verify(dependency).run();
        }
    }
}

class ConcreteService extends BaseService {
}

abstract class BaseService {

    @Autowired
    private Runnable dependency;

    public void serve() {
        dependency.run();
    }
}
wilkinsona commented 2 years ago

Thanks for the sample. The problem is occurring because, while the mock Runnable is being reset, it isn't the mock that's been injected into the ConcreteServiceTest and ConcreteService instances.

There are two application contexts involved here. The first is created for ConcreteServiceTest and the second is created for Suite. A mock is created and registered with each context. The mock from ConcreteServiceTest's context is injected into the dependency field of both ConcreteServiceTest and ConcreteService but it's the mock from Suite's context that is reset as this is the context that's "active" when executing test() on Suite.

I'm not sure what we can do about this in Boot. @sbrannen, can you please advise?

sbrannen commented 2 years ago

I'm not sure what we can do about this in Boot. @sbrannen, can you please advise?

There's effectively nothing (reasonable) that we can do about it in either Spring Boot or Spring Framework.

The issue is that the Suite class inherits its configuration from its enclosing class which is BaseServiceTest, and BaseServiceTest is not annotated with @Import(ConcreteService.class). Thus, the MergedContextConfiguration is different for Suite and ConcreteServiceTest. This causes those two test classes to have a different ApplicationContext.

The MergedContextConfiguration (which is used as the context cache key in the Spring TestContext Framework (TCF)) is built based on static analysis of the test classes, their super classes, and their enclosing classes. Consequently, there is a disconnect between the enclosing class for Suite and the concrete type of the enclosing instance at runtime. At runtime, the enclosing instance is of type ConcreteServiceTest, but the enclosing class is of type BaseServiceTest.

With the current infrastructure in the TCF, all lookups for the enclosing type are based on the enclosing class (retrieved via java.lang.Class.getEnclosingClass()). In theory it should be possible to use org.junit.jupiter.api.extension.ExtensionContext.getTestInstances() to discover the concrete type of the enclosing instance; however, that would require an exhaustive rewrite of the internals in the TCF, and even if that were done... there would likely be other parts of the Spring portfolio or Spring community where enclosing type lookups are hard coded to use the enclosing class.

Moreover, as far as I know, the standard Java APIs do not support such lookups (for example, discovering the concrete type of an enclosing instance via reflection).

In summary, I would advise against @Nested test class structures where Spring annotations are only present on a subclass of a class whose nested test classes depend on the annotations of that concrete subclass.


For what it's worth, this scenario is not limited to the use of @Import. The same scenario arises if you replace @Import(ConcreteService.class) with @ContextConfiguration(classes = ConcreteService.class) (or similar arrangements of test-related annotations that affect the context cache key).


As a side note, the ApplicationContext loaded for the Suite test class does not even contain a BaseService bean. The only reason it contains a Runnable bean is because @MockBean will create such a bean if it does not exist.

wilkinsona commented 2 years ago

Thanks very much for taking a look, @sbrannen.

Closing based on the recommendations above.

GeorgiPetkov commented 2 years ago

@wilkinsona I'm just wondering if this is the correct handling of the issue. It's obviously a real issue. A recommendation here in a closed issue is almost worthless. Even if this is not fixed any time soon this could be useful in the case of later redesign.

I believe there are still other things that can be done like documenting this somewhere or even better - detecting this case so there's a proper error/warning message for this. I've spent hours trying to figure out what is wrong because the usage was actually correct, but the implementation is unintendedly not supporting it and there is no indication of that whatsoever.

I understand that this is not a top priority issue, but just closing it seems inappropriate to me.

wilkinsona commented 2 years ago

It may be worth documenting this advice from Sam:

I would advise against @Nested test class structures where Spring annotations are only present on a subclass of a class whose nested test classes depend on the annotations of that concrete subclass.

It may even be possible to detect this arrangement and log a warning or throw an exception. It's something that would be best done in Spring Framework as it's general advice for using the Test Context Framework.

We'll transfer this issue to the Spring Framework issue tracker so that they can consider some documentation improvements.

sbrannen commented 2 years ago

Even if this is not fixed any time soon this could be useful in the case of later redesign.

I'm not yet convinced that this is an issue worth "fixing". The more I think about it, the more I consider it an inherent limitation of the @Nested test class programming model in JUnit Jupiter.

For example, the following is based purely on JUnit Jupiter and displays similar behavior.

abstract class BaseTests {

    @Nested
    class NestedTests {

        @Test
        void my_test(TestInfo testInfo) {
            assertEquals("my test (TestInfo)", testInfo.getDisplayName());
        }
    }

    @DisplayNameGeneration(ReplaceUnderscores.class)
    static class ConcreteTests extends BaseTests {

        @Test
        void my_test(TestInfo testInfo) {
            assertEquals("my test (TestInfo)", testInfo.getDisplayName());
        }
    }

}

When running ConcreteTests, the my_test method in NestedTests fails analogous to the nested Suite test class in the Spring-boot based example. The reason is that JUnit Jupiter looks up the @DisplayNameGeneration annotation on the enclosing class determined via Class.getEnclosingClass().

It may be possible to rewrite the DisplayNameUtils.getDisplayNameGenerator() method in JUnit Jupiter to use the type of the enclosing instance (retrieved from the parent TestDescriptor), and it may be possible to rewrite Spring's testing support to utilize Jupiter's ExtensionContext.getTestInstances() feature, but that might not close all of the gaps. For example, there may well be situations where it is not possible to rely on Jupiter's ExtensionContext.getTestInstances() feature (or Jupiter internals), and in such situations static analysis will not tell you that NestedTests should sometimes be treated as if ConcreteTests were its enclosing class.

Another thing that concerns me is that many of the public APIs in the TestContext framework accept the Class<?> testClass to find annotations, etc. And that will always only allow static analysis of the enclosing class. In order to support finding the type of the enclosing instance, we would have to overload a lot of methods in public APIs to pass an additional argument that provides the "enclosing instance types" metadata, and I don't consider that feasible. Plus, this additional metadata currently only makes sense for @Nested support with JUnit Jupiter. Other testing frameworks like TestNG, JUnit 4, etc. would not be able to provide such metadata.

I'll put some more thought into it, but it may be that this limitation is best documented in JUnit 5 and/or the Spring Framework.