spring-projects / spring-framework

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

Add @SpringTest annotation to bootstrap integration with the type's lifecycle and features similar to standard Spring components #22924

Closed odrotbohm closed 5 years ago

odrotbohm commented 5 years ago

As a follow up of #22286, this ticket is supposed to discuss the idea of a canonical annotation to be used with test classes that constitute Spring integration tests and configure JUnit to treat the class like a Spring component as much as possible. In detail, this includes:

This could effectively look something like this (name TBD):

@SpringJUnitConfig // Bootstrap Spring container
@AutowireTestConstructor // Enable constructor injection
@TestInstance(Lifecycle.PER_CLASS) // Singleton, i.e. one instance for all test method invocations
// other annotations needed
@interface SpringTest { // SpringTest as that's in line with @SpringBootTest
  // add aliases for attributes of above annotations
}

The goal is to minimalize the conceptual differences between a Spring component in production code and a integration test class.

marcphilipp commented 5 years ago

Out of curiosity, why make @TestInstance(Lifecycle.PER_CLASS) the default? It imposes an additional burden on test authors to manage state correctly and prevents test methods within the class from being executed in parallel.

odrotbohm commented 5 years ago

Because singleton is the default scope for Spring components in general. Making this the default for tests, too, is just consistent with that. This creates the incentive to create stateless classes, or classes whose methods do not share state, which not only enables multithreaded execution much easier but is also more efficient as it avoids the need to create new instances of the class for each method invocation. There's no burden to manage state if you don't have any.

I know that JUnit's default has been exactly the other way round, but I've always considered the reinstantiation a workaround to allow state in the class. Coming from a pure Spring mindset it feels odd that we teach users to write stateless, immutable components but then all of a sudden allow keeping state in the test classes. Especially beginners usually don't realize this difference clearly and then start to use the pattern of manipulating instance variables from within production components which leads to subtle, hardly testable problems.

Aligning the testing model with the runtime component model makes problems like these more obvious and discoverable more easily.

sdeleuze commented 5 years ago

Big +1 from me on that one, this combination of annotations is exactly what I need for Kotlin developers that typically use non static @BeforeAll / @AfterAll annotated methods and constructor based injection with non-nullable immutable properties.

snicoll commented 5 years ago

Because singleton is the default scope for Spring components in general

Isn't that going a bit too far? The same way that field autowiring in a test class is okay but doing so in a spring component should definitely be avoided.

Given that the constructor is going to be invoked for each test, there is no "state" per se. I personally like to setup the common infrastructure of a test in the constructor so that it is immutable. If you have a single component, it is easy enough to just initialize an attribute as the first line of your test but: a) this is something that we have to repeat for every test, b) it's getting more annoying if you have more than one thing to initialise.

I guess there are arguments to be made to wrap that in a component or whatever but I think we should be a bit more pragmatic and keep the default of the test framework.

odrotbohm commented 5 years ago

Isn't that going a bit too far? The same way that field autowiring in a test class is okay but doing so in a spring component should definitely be avoided.

How do you mean "too far"? Having to inject dependencies into fields was a necessity as previous JUnit generations didn't support constructors. You're also free to use field injection with tests, the annotation setup doesn't change that. It's all opt-in anyway. The annotation enables constructor injection, it doesn't enforce it. Just like @Component does.

Given that the constructor is going to be invoked for each test, there is no "state" per se. I personally like to setup the common infrastructure of a test in the constructor so that it is immutable. If you have a single component, it is easy enough to just initialize an attribute as the first line of your test but: a) this is something that we have to repeat for every test, b) it's getting more annoying if you have more than one thing to initialise.

I am not sure I can follow. The per-class setting changes that to instantiate the test class only once. If you have an immutable test fixture that's valid for all tests you can still set that up in the constructor. That can then be augmented by additional setup done in the test methods. If you have common logic there, you can still extract methods which you call from some methods and not from others.

It all just creates more incentives to work with immutable test classes, which is a good thing in general and – as indicated before – all of this is optional.

I guess they are arguments to be made to wrap that in a component or whatever but I think we should be a bit more pragmatic and keep the default of the test framework.

Again, I can't follow. It's an annotation that's optional to be used. When you use it, you opt into its features. If you don't want that, don't use the annotation, use the ones you'd like to apply. I think it's convenient – or: pragmatic – to have a one stop shop annotation that expresses: this test class behaves like a Spring component in terms of lifecycle. If I want the defaults, I get them by default anyway. Especially for beginners this removes a lot of cognitive overload because you don't even have to talk about lifecycles and the differences between them in Spring VS. JUnit.

Someone creates an instance, calls all @Test methods on it, done. Not: someone creates an instance, calls one method, but then – because you might have changed state with that method – has to recreate the instance, execute all preparation again, for each method. The latter might seem obvious and easy to us, because we're so very familiar with it. That doesn't mean it's simple though.

sbrannen commented 5 years ago

Because singleton is the default scope for Spring components in general. Making this the default for tests, too, is just consistent with that. This creates the incentive to create stateless classes, or classes whose methods do not share state, which not only enables multithreaded execution much easier but is also more efficient as it avoids the need to create new instances of the class for each method invocation. There's no burden to manage state if you don't have any.

The reason @marcphilipp posed that question is that parallel execution is disabled by default in JUnit Jupiter for methods in test classes configured with per-class test instance lifecycle semantics.

Excerpt from the JUnit 5 User Guide:

The default execution mode is applied to all nodes of the test tree with a few notable exceptions, namely test classes that use the Lifecycle.PER_CLASS mode or a MethodOrderer (except for Random). In the former case, test authors have to ensure that the test class is thread-safe; in the latter, concurrent execution might conflict with the configured execution order. Thus, in both cases, test methods in such test classes are only executed concurrently if the @Execution(CONCURRENT) annotation is present on the test class or method.

marcphilipp commented 5 years ago

@odrotbohm Thanks for the detailed explanation!

@sbrannen Thanks for providing additional background to what I wrote!

sbrannen commented 5 years ago

@philwebb and @wilkinsona, do either of you have any input from the Boot side?

Would Spring Boot be more suitable to host such an opinionated test annotation?

philwebb commented 5 years ago

Regardless of if Spring Boot or Spring Framework would be a better home, there are a few things about the annotation that make me uneasy. Whilst I can see that it is useful in some situations, the subtle differences between @SpringTest and @SpringBootTest are likely to catch out users.

Currently @SpringBootTest has the following definition:

@BootstrapWith(SpringBootTestContextBootstrapper.class)
@ExtendWith(SpringExtension.class)
public @interface SpringBootTest {
...

This means that you can use it with JUnit 5 directly, or with JUnit 4 (as long as you also have @RunWith(SpringRunner.class)). It doesn't have @ContextConfiguration as a meta-annotation.

The @AutowireTestConstructor looks like it landed as @TestConstructor(autowire = true) in the end. We don't currently apply that automatically either. That one is a tough call, I'd very much like to be able to just inject beans into the test constructor, but I also want to be able to use other extensions (like OutputCaptureExtension.class) and mix the constructor parameters. It would be nice if we checked other ParameterResolvers first, then only autowire ones that aren't covered (I've raised #23224 to consider changing the boolean in case we want to support that in the future). I'm not sure if it would be better to opt-in to constructor autowiring or to opt-out if you need a different ParameterResolvers.

With regards to @TestInstance(Lifecycle.PER_CLASS), I personally think this would be a mistake as It adds another subtle inconsistency for very little gain. The fact that all other JUnit tests behave differently to @SpringTest is pretty confusing. I think it's more likely that a user will be caught out by this behavior, than actually need it. We may prefer Lifecycle.PER_CLASS ourselves, but I think we should stay consistent with the JUnit defaults.

So to summarize:

The leaves us with an annotation that's basically just @SpringJUnitConfig:

@SpringJUnitConfig 
@interface SpringTest {
}
wilkinsona commented 5 years ago

Thanks for asking, Sam. I don't really have much to add to what Phil's already written above. However, I would like to specifically echo his feelings about Lifecycle.PER_CLASS. I agree wholeheartedly with "the fact that all other JUnit tests behave differently to @SpringTest is pretty confusing". I think it would be a mistake to deviate from JUnit's well-established defaults here.

sbrannen commented 5 years ago

@philwebb and @wilkinsona, thanks for chiming in and providing feedback. Much appreciated.

I tend to share the same sentiments.

FWIW, I may find that I personally use such a composed annotation for my own projects, and I can see how it would be beneficial in certain scenarios, especially for Kotlin developers. However, I am not yet convinced that this particular combination of configuration options would be suitable for mainstream Spring users.

Generally speaking, the core Spring Framework is rarely (if ever) strongly opinionated in such matters. For example, we introduced @GetMapping, PostMapping, etc. as specializations of @RequestMapping, but we intentionally did not introduce opinionated variants of @RequestMapping that preconfigure (or hard code) the consumes and produces attributes -- for example, something like a @GetJson annotation. I think that rationale is applicable here as well: I see @SpringJUnitConfig as an appropriate specialization but something like the proposed @SpringTest as potentially too opinionated.

In the end, it's easy enough to implement such a custom composed annotation on a per-project basis. So I'm inclined to omit such an annotation for the time being and let the community speak up over time to voice more interest in such a dedicated annotation.

odrotbohm commented 5 years ago

I'm a bit confused about the aspect of confusion. We teach newbies that the following declaration is the way we recommend writing Spring components:

@Component // or more concrete stereotype
class MyComponent {

  private final MyDependency dependency;

  MyComponent(MyDependency dependency) {
    this.dependency = dependency;
  }

  // business methods go here
}

To an overwhelming degree that design suggestion does not have anything to do with Spring Framework, but general ideas of decent object-oriented design: immutability, exposing required dependencies on constructors. Spring is able to work with classes designed that way. In fact, it creates incentives to design the class that way, as following that convention avoids additional configuration (e.g. explicit annotations to document: I want to autowire the constructor). @Component is a single-annotation way to express all of that to the framework.

This is also understandable to newbie users. If we now move on and discuss, how you'd write a test class that's also subject to framework handling, I'd argue that anything but the same level of simplicity (a single annotation enabling the above described feature) has to pass the test of asking "Why does it need to be more complicated than that?".

Why do I need to write this:

@SpringJUnitConfig
@TestConstructor(autowire = true)
@TestInstance(Lifecycle.PER_CLASS)
class MyComponentTest {

}

to get the same behavior as above?

We can now start to argue different defaults in lifecycle and injecting behavior, legacy expectations created from using previous JUnit generations. The unspoilt mind of a newbie to the platform considers this leaky abstraction, implementation details and – to come back to the start - confusing. This topic has come up every time I teach Spring to university students and it's always been considered a seam in the otherwise very consistent component design story we have. Up until JUnit 4, I was able to argue that the framework itself doesn't allow unifying this into a single annotation. That has changed with JUnit 5.

Nobody is asking for some defaults to change, but merely a mechanism just as simple as @Component to actively opt into the behavior one gets to learn for Spring application components in the first place. The way of executing a test significantly changes depending on which Boot test annotation you use (MockMVC, real server, slices only) and you're facing the challenge having to understand which parts get bootstrapped why in which case. We accept that as it's clearly documented in the Javadoc. How is this any different in this case?

The annotation would allow to just skip an entire world of discussion as we can resort to "Just works like for @Component.". And just like with @Component you might have good reasons to diverge from the default behavior if needed and the consequences are understood.

In the end, it's easy enough to implement such a custom composed annotation on a per-project basis.

Of course you can have a custom annotation in your project to make this work. That still leaves me to explain why everyone has to bridge that conceptual gap.

I'd love to see @jhoeller's opinion on that topic here as in our discussion at Spring I/O it felt he saw some appeal in something like this.

philwebb commented 5 years ago

Unless I'm misunderstanding what the annotation does, isn't it quite possible to write a test in the same way without using @TestInstance(Lifecycle.PER_CLASS)? Can't we still encourage constructor based injection and final member variables without deviating from the JUnit defaults? To me that annotation feels a bit like @Scope in the Spring world. It's just that Spring has a default of singleton and JUnit is more like @Scope("request"). What benefit does Lifecycle.PER_CLASS bring? Why does it need to be used by default?

With regards to @TestConstructor(autowire = true), I think that until https://github.com/junit-team/junit5/issues/1945 has been reviewed, we shouldn't rush to add it. It really feels like disadvantage that I get Spring injection only at the expense of any other ParameterResolver implementations. If we can get that issue fixed, I'd be much happier with a composed annotation that was:

@ExtendWith(SpringExtension.class)
@TestConstructor(Autowire.REMAINING)
class SpringTest {
}

You could then do:

@SpringTest
class MyTest {

    void MyTest(CapturedOutput out, MyBean bean) {
    }

}

and both parameters would be injected correctly.

odrotbohm commented 5 years ago

It is possible, of course. It just causes the class to be re-instantiated and the code in the constructor being run for each test method, which makes it something like @BeforeEach.

As indicated before, the main intention is to align the way things work with production code. A constructor of a singleton Spring bean is called once, the instance is then used for all method invocations. There's simply no need to recreate the instance if you're working with immutable test classes. Not having to explain any differences is the value here.

I like what's suggested in junit-team/junit5#1945. What I am looking for is the general ability to inject dependencies into the constructor. That in a context of test execution other injection candidates can be available is totally fine with me.

Personally, I guess I'd prefer CapturedOutput in particular as test method argument as it's quite likely to be used to assert on the output of code invoked in a particular test method.

philwebb commented 5 years ago

Personally, I guess I'd prefer CapturedOutput in particular as test method argument as it's quite likely to be used to assert on the output of code invoked in a particular test method.

Indeed, that was just an example. Although the same problem exists there as well if you want to inject it along with a bean.

philwebb commented 5 years ago

@sbrannen Is there a way to switch @TestInstance(Lifecycle.PER_CLASS) on globally in JUnit 5? It feels like a pattern that that would be useful to switch on for all tests, even if they don't use the SpringExtension.

philwebb commented 5 years ago

Answering my own question, it seems like JupiterConfiguration.DEFAULT_TEST_INSTANCE_LIFECYCLE_PROPERTY_NAME might do the trick. So perhaps we should just update start.spring.io to generate a junit-platform.properties file containing:

junit.jupiter.testinstance.lifecycle.default=PER_CLASS

That way all tests get the default we recommend and @SpringTest wouldn't be special.

wilkinsona commented 5 years ago

That way all tests get the default we recommend and @SpringTest wouldn't be special.

Why would we recommend a non-standard default for all tests? I think changing the lifecycle at all will confuse people and even more so when it's applied to plain unit tests that have nothing to do with Spring.

odrotbohm commented 5 years ago

I agree with Andy. If at all, a change in the overall defaults should probably a separate discussion. The dedicated annotation would allow us to explicitly declare the changes in default and the rationale behind it as well as allow the user to opt into those decision per test, so that she has control over whether a particular test class is supposed to follow those altered conventions and that tweak is documented right at the test class through the annotation.

philwebb commented 5 years ago

Unfortunately I just don't think we're going to reach a consensus on using Lifecycle.PER_CLASS.

There's simply no need to recreate the instance if you're working with immutable test classes. Not having to explain any differences is the value here.

I still don't see any harm in recreating the test instance, and the "not having to explain any differences is the value here" works equally well when comparing a @SpringTest to a regular test.

odrotbohm commented 5 years ago

I still don't see any harm in recreating the test instance

It's work done, that doesn't need to be done. A single instance used for all invocations is a great starting point to explain why it makes sense to use immutable classes in the first place. I'm puzzled to hear someone arguing it's just fine to create multiple instances of a class when one would just work fine.

the "not having to explain any differences is the value here" works equally well when comparing a @SpringTest to a regular test.

Can you elaborate? My point was that there's way of writing production components we actively recommend. It's hard to explain, why there's no simple (or not an as simple) way to get integration test classes work the same way. "Here's the annotation that works with the same component model." is both consistent and an easy to understand answer, that doesn't spark any new questions.

philwebb commented 5 years ago

I don't disagree that it's extra work to create new test instances, or that a single instance might be technically better. What I want to focus on is the fact that we're deviating from the JUnit default, and I think the bar for that should be higher than if we were starting a new test framework from scratch.

I'm puzzled to hear someone arguing it's just fine to create multiple instances of a class when one would just work fine.

Clearly it is fine to create multiple instances, because that's exactly what happens currently. You can still write immutable classes, you can still explain why it makes sense to do that, you can even change the setting yourself if you're so inclined.

It's hard to explain, why there's no simple (or not an as simple) way to get integration test classes work the same way

I don't see why the fact that the test instance gets created once, or per-method really changes things here. I think it's perfectly fine for JUnit to remain in charge of the test lifecycle. I think we need a really really strong benefit to change from the default, and so far I've not been convinced that we've found one.

Something else that we should consider is if we're going to want to offer support for annotations like @TestPropertySource or @MockBean on a per-method basis in the future.

philwebb commented 5 years ago

I don't think I have much more to add to the conversation. I think I've made all the points I want to, so @sbrannen and @jhoeller will have to make the decision. We'll most probably align @SpringBootTest with whatever they decide.

sbrannen commented 5 years ago

Team Decision: After having put considerable thought into the proposal, debating the pros and cons and in spite of the benefits such a constellation provides, the core Spring Framework team has decided that we are not convinced that the introduction of such an opinionated test annotation would be the right choice for the core framework. As pointed out in this issue's description, it is already possible for developers to implement their own composed annotation. As such, we recommend that developers do exactly that if such a composed annotation meets their needs.