adevinta / zoe

The Kafka CLI for humans
https://adevinta.github.io/zoe
MIT License
287 stars 21 forks source link

rework testcontainer usage #6

Closed mgmeiner closed 4 years ago

mgmeiner commented 4 years ago

When i implemented the other pull request i noticed that the tests clashed with my local running kafka + zookeeper and the schema-registry clashed with a completely different service running on that port (8081). So i thought fixing this by choosing a random and non-clashing ports which is done by testcontainers out of the box.

This removes the testcontainers setup with docker-compose and replaces it with testcontainer's container definition by code.

For applying the random and non-clashing ports to the EnvConfig during startup I introduced a customizer for the config which executes when instantiating it for the Koin module. I'm new to Koin so there might be a better solution to this problem 🙂.

I also updated the used kafka version for tests to: 5.5.1.

wlezzar commented 4 years ago

nice! Thanks a lot for the PR : ) . I will review this ASAP

wlezzar commented 4 years ago

@mgmeiner nice! : ) . I will take a look at these chabnes today (sorry I have been off these last couple of days). I will give you some feedback later today 👍

mgmeiner commented 4 years ago

I reverted everything to my first solution which customizes the EnvConfig with a simple kotlin function (EnvConfig) -> EnvConfig and refactored it slightly.

The private customize function looks for beans/functions, which implement this function-pattern: (T) -> T, and if one or more exists it invokes them + reassigns the result as the actual bean.
I skipped creating a dedicated interface for the customizer but I'm not sure if its better to have one or to simply use the kotlin function definition as it is now. This solution now would basically work for every bean without the need for custom 'Customizer'-Interfaces but might not be obvious at first sight.

I hope this is what you're looking for 😊.

wlezzar commented 4 years ago

I'm ready to merge it if you are done @mgmeiner ?

mgmeiner commented 4 years ago

@wlezzar Yeah :) But one more thing: We could speedup tests by not starting and stopping the Containers for every Spec and instead just start the Containers once (with init on the Testcontainers Context object). The teardown is handled automatically by Testcontainers. If you like i could add that too unless you want a fresh kafka instance per Spec.

wlezzar commented 4 years ago

@wlezzar Yeah :) But one more thing: We could speedup tests by not starting and stopping the Containers for every Spec and instead just start the Containers once (with init on the Testcontainers Context object). The teardown is handled automatically by Testcontainers. If you like i could add that too unless you want a fresh kafka instance per Spec.

I actually had this in my todo list, so yes completely agree ^^ . The tests are designed to not impact each others, so one cluster for all the tests would be enough. So if you would like to do it in this PR, would be super cool : )

mgmeiner commented 4 years ago

Ok i squashed the previous commits and added another one for the "starting-only-once". Thanks for your review and support on this PR 😊

wlezzar commented 4 years ago

Thanks a lot @mgmeiner : ) . And sorry it took so much time..