Comments (17)
@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.
from spring-cloud-stream.
@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!
from spring-cloud-stream.
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
from spring-cloud-stream.
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.
from spring-cloud-stream.
@kutmk No worries. We will get back to you on this soon.
from spring-cloud-stream.
@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!
from spring-cloud-stream.
@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.
from spring-cloud-stream.
@kutmk We needed to back out the changes introduced in 4174657 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?
from spring-cloud-stream.
We did try out the 4.1.2-SNAPSHOT
of the spring-cloud-stream-test-binder
and it seems to work. Thanks a lot for the quick help :)
from spring-cloud-stream.
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
from spring-cloud-stream.
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 @EmbeddedKafka
and some using the TestChannelBinderConfiguration
(where Kafka doesn't matter) as pointed out in the docs.
Hope this helps with the decision :)
from spring-cloud-stream.
Maybe introducing a property e.g. spring.cloud.stream.test-binder.enabled-by-default
with the default of true
might be an option π€
from spring-cloud-stream.
@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.)
from spring-cloud-stream.
@kutmk Reopening this issue after further discussion with our team. @olegz - please chime in.
from spring-cloud-stream.
@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.
from spring-cloud-stream.
@olegz @sobychacko
I'm interested and would like to submit a PR.
from spring-cloud-stream.
@kutmk Go ahead! We'll wait
from spring-cloud-stream.
Related Issues (20)
- NPE when sending heterogeneously-typed Arrays via StreamBridge. HOT 6
- StreamsBuilderFactoryBean: Call afterSingletonsInitialized() programmatically in StreamsBuilderFactoryManager
- Current reference doc linked to 4.0.5 HOT 1
- RabbitMQ: spring.cloud.stream.rabbit.bindings.* properties not recognized in Native/AOT builds HOT 5
- Feature Request: Add delivery attempt count to RabbitMQ DLQ messages HOT 21
- BeanCreationNotAllowedException on shutdown HOT 2
- @Component annotated beans composition doesn't work HOT 5
- EmbeddedKafka usage improvements in Kafka Streams binder
- EmbeddedKafka usage improvements in Kafka binder
- Allow disabling particular binder from health HOT 2
- Remove spring.binders for test binder
- Health Check for Binders Always Shows 'UNKNOWN' Status Due to Empty binderHealthContributors Map (Missing Bean Configuration for ReactorKafkaBinderHealthIndicator?) HOT 2
- Kafka Binder Health Indicator consumer group for the metadata consumer
- Ability to re-use the same function in multiple streams HOT 1
- Batch Consumer With ListenerContainerWithDlqAndRetryCustomizer should disable binder DLQ
- Inconsistent behavior when configuring multiple functional stream bindings with kafka streams HOT 6
- Deprecate `BindingServiceProperties#dynamicDestinations` HOT 3
- Unable to invoke `actuator/bindings` when using test binder HOT 3
- ReactorKafkaBinder - not respecting back pressure (identical vanilla ReactorKafka consumer does) HOT 11
- NoSuchMethodError Health.down(java.lang.Exception) exception thrown on actuator /health request HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from spring-cloud-stream.