TestChannelBinder is exposed without including TestChannelBinderConfiguration

[Laksuh]

Description The TestChannelBinder is initialized in spring context during tests even though it was never configured to be. This issue is there since 4.1.1 (we upgraded from spring cloud 2023.0.0 to 2023.0.1). The docs clearly say, that you have to add the TestChannelBinderConfiguration in order to activate the TestChannelBinder. In an integration test where we wanted to test with an EmbeddedKafka (or a local one), we were not able to make our consumer connect to our topics. We found out that the TestChannelBinder ist exposed and used, which was why we could not consume any messages.

To Reproduce Steps to reproduce the behavior: We have setup an example project which is able to reproduce the issue: https://github.com/Akkjon/kafka-bug-demo

1. clone the example project
2. check if you have a local instance of kafka on port 9092 or use the embedded Kafka in the test class uncomment in Line 21 to use the embedded Kafka
3. run mvn clean test and see that the test will fail
4. change the version of spring cloud from 2023.0.1 back to 2023.0.0
5. run mvn clean test again and see the tests succeeding

Version of the framework Spring Boot 3.2.4, Spring Cloud 2023.0.1, JDK 21

Expected behavior Since we did not include the TestChannelBinderConfiguration in the test class (nor anywhere in the code), we would expect the KafkaMessageChannelBinder to be used.

@Akkjon

[sobychacko]

@Laksuh Thanks for reporting this issue. It looks like a regression introduced in the previous version.

@kutmk The fixes introduced for this issue is potentially causing a regression. Having an unconditional auto-configuration for TestChannelBinderConfiguration prevents integration tests from using a real binder with the embedded Kafka or other Kafka clusters.

[sobychacko]

@kutmk We needed to back out the changes introduced in https://github.com/spring-cloud/spring-cloud-stream/commit/4174657a356478cdb1d7e648a0793dd6d87af19c due to this regression issue. Thinking it over again, since other binders (Kafka binder, for example) are not using the normal auto-configuration strategies by using @AutoConfiguration, we may run into unforeseen issues like the one reported. I tried just to bring back the @ConditionalOnMissingBean(Binder.class) on TestChannelBinderConfiguration, which did not work due to the fact that the Kafka binder was not using the regular auto-configuration but rather using an internal mechanism that only instantiates the binder after the fact. Please rely on the workarounds you mentioned in the issue until we devise a different plan.

@Laksuh, could you test it with the latest snapshot and verify that it works now?

[Laksuh]

We did try out the 4.1.2-SNAPSHOTof the spring-cloud-stream-test-binder and it seems to work. Thanks a lot for the quick help :)

[kutmk]

Thank you @sobychacko for letting me know. I understand about the back out.

By the way, can’t this issue be resolved by removing the spring-cloud-stream-test-binder from the dependencies? I understand that the Test Binder is activated when the spring-cloud-stream-test-binder is added to the dependencies, and if you want to test with Kafka Binder or similar, you have to remove it from the dependencies. Special Note on Mixing Test Binder and Regular Middleware Binder for Testing

[sobychacko]

@kutmk Although it is given as a recommendation in the docs (it was added way back when the test binder was not added by default via the initializr), in practice, many people won't remember to remove the dependency since it may have been added by default via initializr for example. Yes, if you are diligent about proactively removing it, this would work. However, since other components in spring-cloud depend on the fact that the test binder is not autoconfigured by default, we need to do a full evaluation to ensure no other regression issues. That was another motivation to back out this change. We may reintroduce something along these lines in the 4.2.0 version.

@Laksuh Do you have a hard reason for not removing the test binder dependency?

Thanks!

[Laksuh]

Our services do have a lot of integration tests and we do not want to setup an embedded kafka for each integration test class. Also the binding of kafka with authentication via kerberos takes way more time, than "mocking" the whole thing away with the TestchannelBinder for tests, where the messaging queue does not even matter. So we have some test classes using the @EmbeddedKafkaand some using the TestChannelBinderConfiguration (where Kafka doesn't matter) as pointed out in the docs. Hope this helps with the decision :)

[Laksuh]

Maybe introducing a property e.g. spring.cloud.stream.test-binder.enabled-by-default with the default of true might be an option 🤔

[Stexxen]

This caused us some problems in integrations testing as well. We fixed some tests with @EnableAutoConfiguration(exclude = org.springframework.cloud.stream.binder.test.TestChannelBinderConfiguration.class)

but eventually we found this issue, and decided to roll back to 4.1.0 while we wait for 4.1.2

[sobychacko]

After some further internal discussions on this matter, we are leaving the configuration of the test channel binder as it is today, i.e., you need to manually opt-in by using @Import. This prevents some unforeseen issues that applications may encounter when they include the test binder with the regular binders in the same module.

[kutmk]

@sobychacko Just to confirm, do you mean to back out Issue #2566? Or do you mean to back out only Issue #2893? (Note: #2893 respected the opinion of #2566 and added the missing functionality.)

[sobychacko]

@kutmk Reopening this issue after further discussion with our team. @olegz - please chime in.

[kutmk]

@sobychacko Thank you. To avoid any misunderstanding, I want to clarify that I do not intend to refuse the decision. I asked because I might not fully understand the situation as English is not my native language. I asked to know whether @Import is necessary in all cases when using TestBinder, or if it is still necessary in some cases. p.s. I think @Import is sufficient, but if @Import is necessary in all cases, I would like to suggest the introduction of a meta-annotation like @EnableTestBinder as one opinion.

[sobychacko]

@kutmk No worries. We will get back to you on this soon.

[sobychacko]

@kutmk We had an internal discussion with the project lead @olegz on this issue. We like the idea of introducing the meta-annotation @EnableTestBinder - as you suggested. Would you be interested in submitting a PR for that change? Please let us know; otherwise, we will try to work on it. Thanks!

[olegz]

@kutmk First, thank you for your participation in this discussion and a great suggestion for improvement. As @sobychacko suggested please let us know if you are interested in contributing, otherwise we'll do it. I also want to answer your questions. . . The @Import allows us to bring TestChannelBinder on-demand (when needed). Making it auto-configured, on the other hand, would always bring it when running in test scope. The issue with auto-configuration (always bringing test binder) is that a developer like yourself may also want to have a test case in the same project running embedded Kafka or other binder - effectively mixing unit tests and integration tests (which is common). This would require adding such binder to the classpath. If we were to bring test channel binder as auto-configuration than every test in your project would have to be treated as multi-binder test since two+ binder auto-configurations would be available in the classpath. That would be very inefficient and cumbersome. But by requiring @Import or as you suggested @EnableTestBinder would allow you to only bring test binder when you intend to use it for the current test. And internally we already have a mechanism to ensure there is no collision with another binder when you do that.

[kutmk]

@olegz @sobychacko I'm interested and would like to submit a PR.

[olegz]

@kutmk Go ahead! We'll wait