Skip to content

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Dec 16, 2022

No description provided.

@wind57
Copy link
Contributor Author

wind57 commented Dec 19, 2022

After writing some integration tests I've realized that we do quite a lot of boilerplate code in each of them. In this PR, I've tried to simplify and hide all the complexity of an integration test set-up to a single class Util. I've also clean-ed up tests quite a lot, moved wiremock/kafka/rabbitmq/zookeeper related code to a single package. imho, now the integration test reads far simpler and it's easier to reason on what is going on.

What is more important is that this touched integration tests only, no other code. It does not change the logical flow of them, just makes it simpler. I know there are many files to look at, but imho, this is needed. The more tests we add, the more complex each of them becomes, and it should not be like that at all.

I honestly hoped that for the fabric8 cases, things would be even simple and nicer since we upgraded to 6.2 and they have some neat features for integration tests. But after trying a few of them, they are limited (at least for the time being) and do not fit our needs. I'll raise a defect on their side and ask a few questions and see where it goes.

@wind57 wind57 changed the title Simplify integration tests Simplify kubernetes native integration tests Dec 22, 2022
@wind57 wind57 changed the title Simplify kubernetes native integration tests Simplify integration tests Dec 22, 2022
@wind57 wind57 marked this pull request as ready for review December 24, 2022 12:41
private static void deleteManifests() {
private static void manifests(Phase phase) {

try {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanjbaxter here is the idea of my PR, for example. In almost all of our integration tests, we create a deployment,service,ingress (and may be configmaps and secrets), and they do the exact same thing. First create them, do the testing, then deleted them. So why can't we simplify this and hide all the complexity of waiting for the resources to be created and deleted via proper methods? In such a case, reading the code of the integration test becomes far simpler to reason about. I've done this in all the tests + did some clean-up here and there.

Copy link
Contributor

@ryanjbaxter ryanjbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the kubernetes java client catalog watch implementation included in this PR?

@wind57
Copy link
Contributor Author

wind57 commented Jan 4, 2023

hey, welcome back. It's a simple reason, I hoped that the k8s watcher implementation would go in first, before this one

@ryanjbaxter
Copy link
Contributor

Ah OK, I missed that other PR. I just merged it, so you should now be able to resolve the conflicts here

@wind57
Copy link
Contributor Author

wind57 commented Jan 4, 2023

@ryanjbaxter ready to be looked at. thank you

@ryanjbaxter
Copy link
Contributor

This is great! Thank you!

@ryanjbaxter ryanjbaxter merged commit 66801c9 into spring-cloud:main Jan 4, 2023
@ryanjbaxter ryanjbaxter added this to the 3.0.1 milestone Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants