search

Decathlon / tzatziki

Decathlon library to ease and promote Test Driven Development of Java microservices!
Apache License 2.0
62 stars 30 forks source link

Ensure that KafkaInterceptor @Component is in context #394

Open alexandrepa opened 3 weeks ago

alexandrepa commented 3 weeks ago

Using the tzatziki-kafka module, we have seen some users complaining about instability and struggling to test their kafka topics. It was due to the fact that KafkaInterceptor was not in the classpath. Tzatziki could not add some aspectj around kafka consumer. We already added some instructions in the README to avoid that.

Second thing we can do is to check that the component is in the spring context. If it's not the case we could stops the test and log a message.

The purpose of this issue is to discuss how can we assure the KafkaInterceptor is initialized and also how do we want to address it to the user.

To check if KafkaInterceptor is initialized, maybe the best way is to @Autowired (optional) it in KafkaSteps and check in @Before method if its not null.

jak78dkt commented 3 weeks ago

Hey. As I said in previous discussions, I'm a strong supporter of this proposal. However, we have to consider the backward compatibility. Some people might not be happy that a Tzatziki upgrade make their test fail because they need time for undoing all their fixes in their Gherkin files they had to do to stabilize their tests. This is why I would suggest to provide an option to disable this check to give people time to migrate. In that case, the check could log an error instead of failing.

alexandrepa commented 3 weeks ago

I agree with you, seems a good way to operate. By default, the tests will failed if there is no KafkaInterceptor, and the failure should mention the issue and the possibility to disable the failing of the test with a property.

jak78dkt commented 3 weeks ago

Exactly

brian-mulier-p commented 3 weeks ago

Hello ! Maybe this is worth giving a try ? 

We do this by adding the name of the class in the standard file resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports:

com.baeldung.autoconfiguration.MySQLAutoconfiguration

Adding the interceptor as an auto-import candidate should work but I might be wrong and maybe the mechanism comes too late for this case.

alexandrepa commented 3 weeks ago

I just did a test and it seems to be working 😄 thanks !

The only issue is how do we handle the users that are currently wrongly using the kafka lib in their test. Adding this could lead to failing test in their projects.

Maybe creating a major version with a warrant int the README. And add the possibility to disable the KafkaInterceptor.

brian-mulier-p commented 2 weeks ago

I'm not sure why it would fail those and in any case I think it's better in every aspect to auto-inject, otherwise you just don't take this dependency as the whole Kafka module is based-off the interceptor. So IMO just make the change without caring about that (and if some users complain we can find a solution and maybe add a switch but pretty sure it's not needed)

jak78dkt commented 1 week ago

I'm not sure why it would fail

Some projets have adjusted/hacked their expected values in their test just make their tests repeatable, instead of fixing the tests using KafkaInterceptor. For these projects, enabling KafkaInterceptor will bring their test back to a normal behaviour, making their hacks fail. They need time to migrate their legacy tests by removing their hacks

I have changed my mind about the need for a switch: these users can stick to the previous version of Tzatziki while migrating, and upgrade to the latest Tzatziki version when their migration is complete.

jak78dkt commented 1 week ago

Hello ! Maybe this is worth giving a try ?

Thanks @brian-mulier-p that's a great idea!