Spring Framework
  1. Spring Framework
  2. SPR-4632

Load dedicated child ApplicationContext for test instance in the TestContext framework

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.5.2
    • Fix Version/s: 4.1 RC1
    • Component/s: Test
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      Status Quo

      The loadContext(*) methods in AbstractGenericContextLoader and AbstractGenericWebContextLoader invoke AnnotationConfigUtils.registerAnnotationConfigProcessors(context).

      The result is that @Autowired, @Resource, @Inject, etc. all work by default, even if annotation-driven dependency injection is not explicitly configured in the test's ApplicationContext. Consequently, if a developer is not aware of this, then errors related to the lack of explicit annotation-driven DI support in production configuration will not be noticed until after testing which is typically undesirable.

      Note, however, that this unexpected behavior only applies to XML-based configuration. With JavaConfig, an AnnotatedBeanDefinitionReader (used internally by AnnotationConfigContextLoader and AnnotationConfigWebContextLoader) automatically registers the annotation config processors.

      Furthermore, BeanPostProcessors may inadvertently be applied to the test instance – even though the test is not truly a bean in the ApplicationContext. This leads to potentially problematic behavior such as accidental proxying of the test instance, as described in SPR-9478.


      Deliverables

      1. As suggested in comments for this issue, create an ApplicationContext dedicated solely to the test instance (for the purpose of dependency injection and bean initialization) and set the parent of that context to the ApplicationContext loaded by the TCF.
      2. Stop invoking AnnotationConfigUtils.registerAnnotationConfigProcessors() in:
        • AbstractGenericContextLoader
        • AbstractGenericWebContextLoader
      3. Ensure that child contexts are properly closed with regard to context caching in the TCF.

      Original Issue Summary

      Do not enable annotation-driven DI for the entire ApplicationContext in the TestContext framework

      Original Proposal

      It would be better if the TestContext framework (TCF) only enabled annotation-driven dependency injection for test instances and not for the entire ApplicationContext.

        Issue Links

          Activity

          Hide
          Juergen Hoeller added a comment -

          This has actually been debated before but was decided to be the better tradeoff overall. We also consider enabling annotation processing by default in forthcoming special environments, so that doesn't necessarily apply to test environments only.

          Actually, integration test XML files are not necessarily supposed to be an exact match for production bean definition files in the first place. There are always going to be some configuration differences, typically isolated into a specific XML file that's part of a config set. Since it's sufficient to have a <context:annotation-config> entry in one of the files in the config set, I don't think that the defaults in the test environment are bad there... Maybe not immediately expected but still not causing serious hassle either.

          Juergen

          Show
          Juergen Hoeller added a comment - This has actually been debated before but was decided to be the better tradeoff overall. We also consider enabling annotation processing by default in forthcoming special environments, so that doesn't necessarily apply to test environments only. Actually, integration test XML files are not necessarily supposed to be an exact match for production bean definition files in the first place. There are always going to be some configuration differences, typically isolated into a specific XML file that's part of a config set. Since it's sufficient to have a <context:annotation-config> entry in one of the files in the config set, I don't think that the defaults in the test environment are bad there... Maybe not immediately expected but still not causing serious hassle either. Juergen
          Hide
          Tuomas Kiviaho added a comment -

          Automatic post processor registration lead to intense debugging session for me since I wasn't aware of it before and I have a custom CommonAnnotationBeanPostProcessor which lead to double post processing causing latter to crash due to it's default configuration.

          As workaround I just had to name my post processor identically to the default one ( (org.springframework.context.annotation.internalCommonAnnotationProcessor) preventing it from being registered.

          One solution could be to use org.springframework.beans.factory.ListableBeanFactory#getBeanNamesForType to ensure that beanFactory doesn't already have post processors instead of name comparison.

          Show
          Tuomas Kiviaho added a comment - Automatic post processor registration lead to intense debugging session for me since I wasn't aware of it before and I have a custom CommonAnnotationBeanPostProcessor which lead to double post processing causing latter to crash due to it's default configuration. As workaround I just had to name my post processor identically to the default one ( (org.springframework.context.annotation.internalCommonAnnotationProcessor) preventing it from being registered. One solution could be to use org.springframework.beans.factory.ListableBeanFactory#getBeanNamesForType to ensure that beanFactory doesn't already have post processors instead of name comparison.
          Hide
          Tuomas Kiviaho added a comment -

          I noticed that it is possible to register processors just for test class via extendingDependencyInjectionTestExecutionListener by using test container application context as parent of a test application context which sole purpose is to instantiate test.

          By moving the AnnotationConfigUtils.registerAnnotationConfigProcessors from AbstractGenericContextLoader to DependencyInjectionTestExecutionListener is one way to solve this issue without API changes. By moving hooks as well from AbstractGenericContextLoader the extendability stays about the same.

          Show
          Tuomas Kiviaho added a comment - I noticed that it is possible to register processors just for test class via extendingDependencyInjectionTestExecutionListener by using test container application context as parent of a test application context which sole purpose is to instantiate test. By moving the AnnotationConfigUtils.registerAnnotationConfigProcessors from AbstractGenericContextLoader to DependencyInjectionTestExecutionListener is one way to solve this issue without API changes. By moving hooks as well from AbstractGenericContextLoader the extendability stays about the same.
          Hide
          Dave Syer added a comment - - edited

          I agree with Tuomas: the test case should live in a child context, so that the behaviour of the parent context is the same as it would be in production. Can we get this into the 3.0.x roadmap?

          Show
          Dave Syer added a comment - - edited I agree with Tuomas: the test case should live in a child context, so that the behaviour of the parent context is the same as it would be in production. Can we get this into the 3.0.x roadmap?
          Hide
          Neale Upstone added a comment -

          Is there any hope of this making 3.2 now? It looks like there are enough related changes going in with spring-test-mvc that this should go too...

          Show
          Neale Upstone added a comment - Is there any hope of this making 3.2 now? It looks like there are enough related changes going in with spring-test-mvc that this should go too...
          Hide
          Sam Brannen added a comment -

          Hi Neal,

          Pending completion of related issues SPR-5613 and SPR-9863, it may be possible to address this issue in the 3.2 time frame, but we'll just have to see how things play out.

          Regards,

          Sam

          Show
          Sam Brannen added a comment - Hi Neal, Pending completion of related issues SPR-5613 and SPR-9863 , it may be possible to address this issue in the 3.2 time frame, but we'll just have to see how things play out. Regards, Sam
          Hide
          Sam Brannen added a comment -

          FYI: I have created a branch as a sort of playground for investigating options that do not include creating a child ApplicationContext. This code may well be thrown away at some point, but in case you're interested, you can check it out here for now: https://github.com/sbrannen/spring-framework/commits/SPR-4632

          Regards,

          Sam

          Show
          Sam Brannen added a comment - FYI: I have created a branch as a sort of playground for investigating options that do not include creating a child ApplicationContext . This code may well be thrown away at some point, but in case you're interested, you can check it out here for now: https://github.com/sbrannen/spring-framework/commits/SPR-4632 Regards, Sam
          Hide
          Neale Upstone added a comment -

          Thanks. I've had a quick look, and do wonder if it's an approach that might lead to maintenance headaches in future. Are there drawbacks to the child context approach?

          Show
          Neale Upstone added a comment - Thanks. I've had a quick look, and do wonder if it's an approach that might lead to maintenance headaches in future. Are there drawbacks to the child context approach?
          Hide
          Sam Brannen added a comment -

          Hi Neale,

          Thanks for taking a look and more importantly for the quick feedback!

          The work in the branch is really just an exploration of one possible option, but you're right: the child context approach may well prove to be more pragmatic and elegant in the long run. In addition, introducing a child context may make the issue raised in SPR-9478 a moot point, thus potentially killing two (or more) birds with one stone. So I'll continue investigating.

          Cheers,

          Sam

          Show
          Sam Brannen added a comment - Hi Neale, Thanks for taking a look and more importantly for the quick feedback! The work in the branch is really just an exploration of one possible option, but you're right: the child context approach may well prove to be more pragmatic and elegant in the long run. In addition, introducing a child context may make the issue raised in SPR-9478 a moot point, thus potentially killing two (or more) birds with one stone. So I'll continue investigating. Cheers, Sam

            People

            • Assignee:
              Sam Brannen
              Reporter:
              Eberhard Wolff
              Last updater:
              Sam Brannen
            • Votes:
              5 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Days since last comment:
                10 weeks, 6 days ago