Refactor fabric8 catalog watch integration tests (1) #1755

wind57 · 2024-10-05T21:26:21Z

No description provided.

wind57 · 2024-10-05T21:31:35Z

The main idea here is that I took an existing package of integration tests and re-factored it:

it does not need an image
it does not need ingress
it connects directly to a running cluster

Logically, the tests have not changed at all, I changed the way we set-up thing.

On my laptop, running the previous version (time mvn clean install)

   ________________________________________________________
Executed in  378.78 secs    fish           external
   usr time   18.56 secs  104.00 micros   18.56 secs
   sys time    6.51 secs  708.00 micros    6.51 secs

and the current version:

Executed in  149.69 secs    fish           external
   usr time   23.64 secs   77.00 micros   23.64 secs
   sys time    5.78 secs  800.00 micros    5.78 secs

wind57 · 2024-10-05T21:32:59Z

...-kubernetes-integration-tests/spring-cloud-kubernetes-fabric8-client-catalog-watcher/pom.xml

@@ -42,6 +42,22 @@
 				<filtering>true</filtering>
 			</resource>
 		</resources>
+
+		<plugins>


we do not need to build an image anymore, thus skip this

Why? Is it because we use BusyBox instead?

sorry, I should have been more verbose here.

So, until now, we were building an image out of each integration test maven module. For example, this maven module is called: spring-cloud-kubernetes-fabric8-client-catalog-watcher; using spring-boot-maven-plugin we were building a docker image out of it.

This image was used as a deployment and put in K3s. We also used ingress to hit some endpoints needed for the test itself.

I am proposing that instead of doing that, we still spin an instance of K3s, but now connect to it directly:

protected static final K3sContainer K3S = Commons.container(); .... protected static KubernetesClient client() { String kubeConfigYaml = K3S.getKubeConfigYaml(); Config config = Config.fromKubeconfig(kubeConfigYaml); return new KubernetesClientBuilder().withConfig(config).build(); } ..... @TestConfiguration static class TestConfig { @Bean @Primary KubernetesClient kubernetesClient() { return client(); } }

So if we do that, we do not need an image, we do not need ingress, we do not need a deployment.

The test itself still asserts all the previous conditions, nothing changes in this regard, but this time its a lot faster.

I mean this just configures the underlying KubernetesClient. Before we would deploy and app and then use that app to test if the feature we were concerned about worked the way we expect. How does overriding the KubernetesClient with this configuration allow us to test the feature without the need for an app?

Let me try to explain. In this test, all we care about it what catalog watcher posts as an event. All our testing logic is around that and for that we :

have a listener that would catch that event

have a rest controller that would allow us to get that event

have a deployment and service to deploy the app

have ingress so that the rest controller is exposed externally

Then, from the test, we would make changes (deploy busybox for example), then call that rest endpoint to see that the result is correct (via ingress).

But, that catalog watcher does not have to be deployed. All it needs, is a KubernetesClient connected to the cluster and that's it. The rest of the infrastructure can be local (like the controller) and can be provided to the running test via a simple:

@SpringBootTest(class=Application.class)

and that Application.class will take care to load all needed beans.

So instead of going to the ingress path, we now go to a simple :

WebClient client = builder().baseUrl("http://localhost:" + port + "/result").build();

and the value that /result provides to us for assertions, still comes from the cluster.

I guess I can see how that might be possible for this specific test but can this approach be generalized for other tests? For example, if we are testing that updating a config map causes a refresh to be sent to an app via the configuration watcher, I don't see how we don't get around deploy one (or more ) apps to verify that happens.

Spot on sir! It works here and in the discovery for example ( my next PR just like this one ). For reload, I am currently testing to see if and how its possible.

Even moving a few tests in this manner, would be beneficial, imo.

so the idea is not to refactor all the tests, but only some where such changes would be possible. The example you mention with config watcher would not undergo such refactoring for the reason you mention

wind57 · 2024-10-05T21:36:31Z

spring-cloud-kubernetes-integration-tests/pom.xml

@@ -77,6 +77,7 @@
 					</execution>
 				</executions>
 				<configuration>
+					<classesDirectory>${project.build.outputDirectory}</classesDirectory>


without this, I was getting:

[INFO] Running org.springframework.cloud.kubernetes.fabric8.catalog.watch.Fabric8CatalogWatchWithEndpointSlicesIT [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.14 s <<< FAILURE! - in org.springframework.cloud.kubernetes.fabric8.catalog.watch.Fabric8CatalogWatchWithEndpointSlicesIT [ERROR] org.springframework.cloud.kubernetes.fabric8.catalog.watch.Fabric8CatalogWatchWithEndpointSlicesIT Time elapsed: 0.139 s <<< ERROR! java.lang.IllegalStateException: Failed to find merged annotation for @org.springframework.test.context.BootstrapWith(org.springframework.boot.test.context.SpringBootTestContextBootstrapper.class)

I was searching a bit and found this one

When was this error occurring? All the time? Or is it because of the changes here?

only because of the changes I did in this PR, we were not getting this one until now.

Do you know why?

honestly, no, I did not even bother to investigate too much. I was so hyped by the much lower testing times that I almost forgot about this change at all :D

wind57 · 2024-10-06T12:46:25Z

...java/org/springframework/cloud/kubernetes/integration/tests/commons/fabric8_client/Util.java

@@ -406,10 +406,12 @@ private void waitForDeploymentToBeDeleted(String namespace, Deployment deploymen

 		Map<String, String> matchLabels = deployment.getSpec().getSelector().getMatchLabels();

+		long start = System.currentTimeMillis();


minor debugging here, eventually I will get rid of these statements too, but they do not hurt anyway

wind57 · 2024-10-06T12:47:44Z

...ort/src/main/java/org/springframework/cloud/kubernetes/integration/tests/commons/Images.java

@@ -79,7 +79,9 @@ public static String wiremockVersion() {
 	}

 	public static void loadBusybox(K3sContainer container) {
-		Commons.load(container, BUSYBOX_TAR, BUSYBOX, busyboxVersion());
+		if (!imageAlreadyInK3s(container, BUSYBOX_TAR)) {


another thing that I realized: we might already have the image in K3S (only the ones we use to tests like busybox or wiremock), so there is no need to tar it and then move it to the cluster, since its already there. As I progress with refactor, this method will happen for all images

wind57 · 2024-10-06T13:18:55Z

@ryanjbaxter first IT refactor

wind57 · 2024-10-15T07:22:14Z

so what do you think? should I be looking at other integration tests that can be refactored in this manner?

ryanjbaxter · 2024-10-15T14:21:44Z

Yes! Sorry your response got lost in my email I think. Appreciate the effort here!

wind57 added 7 commits October 4, 2024 19:31

started work

40252b6

dirty

86d2e8a

dirty

0589167

fix

840fc59

Merge branch 'change-delete-sequence' into refactor-integration-tests-1

71d0a5e

dirty

fe07a73

first working

6b3200f

wind57 changed the base branch from main to 3.1.x October 5, 2024 21:26

cleanup

e4a88a9

spring-cloud-issues added the waiting-for-triage label Oct 5, 2024

wind57 commented Oct 5, 2024

View reviewed changes

wind57 added 2 commits October 6, 2024 00:34

cleanup

f88a581

cleanup

ed6565c

wind57 commented Oct 5, 2024

View reviewed changes

wind57 changed the title ~~Refactor integration tests 1~~ Refactor catalog watch integration tests Oct 5, 2024

wind57 changed the title ~~Refactor catalog watch integration tests~~ Refactor fabric8 catalog watch integration tests Oct 5, 2024

wind57 added 5 commits October 6, 2024 12:13

more improvements

8824f14

more improvements

7dda032

cleanup

947cc43

static import

b138b42

Merge branch '3.1.x' into refactor-integration-tests-1

7b134e6

wind57 commented Oct 6, 2024

View reviewed changes

wind57 marked this pull request as ready for review October 6, 2024 13:18

wind57 changed the title ~~Refactor fabric8 catalog watch integration tests~~ Refactor fabric8 catalog watch integration tests (1) Oct 9, 2024

ryanjbaxter added task and removed waiting-for-triage labels Oct 10, 2024

wind57 requested a review from ryanjbaxter October 11, 2024 07:08

ryanjbaxter added this to the 3.1.4 milestone Oct 15, 2024

ryanjbaxter merged commit a27bf65 into spring-cloud:3.1.x Oct 15, 2024
13 checks passed

wind57 mentioned this pull request Nov 29, 2024

Refactor fabric8 discovery integration tests (2) #1756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fabric8 catalog watch integration tests (1) #1755

Refactor fabric8 catalog watch integration tests (1) #1755

wind57 commented Oct 5, 2024

wind57 commented Oct 5, 2024 •

edited

Loading

wind57 Oct 5, 2024

ryanjbaxter Oct 10, 2024

wind57 Oct 11, 2024

ryanjbaxter Oct 11, 2024

wind57 Oct 11, 2024

ryanjbaxter Oct 11, 2024

wind57 Oct 11, 2024

wind57 Oct 12, 2024

wind57 Oct 5, 2024

ryanjbaxter Oct 10, 2024

wind57 Oct 11, 2024

ryanjbaxter Oct 11, 2024

wind57 Oct 11, 2024 •

edited

Loading

wind57 Oct 6, 2024

wind57 Oct 6, 2024

wind57 commented Oct 6, 2024

wind57 commented Oct 15, 2024

ryanjbaxter commented Oct 15, 2024

		@@ -406,10 +406,12 @@ private void waitForDeploymentToBeDeleted(String namespace, Deployment deploymen

		Map<String, String> matchLabels = deployment.getSpec().getSelector().getMatchLabels();

		long start = System.currentTimeMillis();

Refactor fabric8 catalog watch integration tests (1) #1755

Refactor fabric8 catalog watch integration tests (1) #1755

Conversation

wind57 commented Oct 5, 2024

wind57 commented Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wind57 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wind57 commented Oct 6, 2024

wind57 commented Oct 15, 2024

ryanjbaxter commented Oct 15, 2024

wind57 commented Oct 5, 2024 •

edited

Loading

wind57 Oct 11, 2024 •

edited

Loading