Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor fabric8 catalog watch integration tests (1) #1755

Merged
merged 15 commits into from
Oct 15, 2024
Merged

Refactor fabric8 catalog watch integration tests (1) #1755

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
40252b6
started work
wind57 Oct 4, 2024
86d2e8a
dirty
wind57 Oct 5, 2024
0589167
dirty
wind57 Oct 5, 2024
840fc59
fix
wind57 Oct 5, 2024
71d0a5e
Merge branch 'change-delete-sequence' into refactor-integration-tests-1
wind57 Oct 5, 2024
fe07a73
dirty
wind57 Oct 5, 2024
6b3200f
first working
wind57 Oct 5, 2024
e4a88a9
cleanup
wind57 Oct 5, 2024
f88a581
cleanup
wind57 Oct 5, 2024
ed6565c
cleanup
wind57 Oct 5, 2024
8824f14
more improvements
wind57 Oct 6, 2024
7dda032
more improvements
wind57 Oct 6, 2024
947cc43
cleanup
wind57 Oct 6, 2024
b138b42
static import
wind57 Oct 6, 2024
7b134e6
Merge branch '3.1.x' into refactor-integration-tests-1
wind57 Oct 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions spring-cloud-kubernetes-integration-tests/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
</execution>
</executions>
<configuration>
<classesDirectory>${project.build.outputDirectory}</classesDirectory>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why?

Copy link
Contributor Author

@wind57 wind57 Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

<includes>
<include>${testsToRun}</include>
</includes>
Expand Down
16 changes: 16 additions & 0 deletions ...bernetes-integration-tests/spring-cloud-kubernetes-fabric8-client-catalog-watcher/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,22 @@
<filtering>true</filtering>
</resource>
</resources>

<plugins>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Is it because we use BusyBox instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<plugin>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-maven-plugin</artifactId>
<executions>
<execution>
<id>build-image</id>
<configuration>
<skip>true</skip>
</configuration>
</execution>
</executions>
</plugin>
</plugins>

</build>

</project>
75 changes: 75 additions & 0 deletions ...a/org/springframework/cloud/kubernetes/fabric8/catalog/watch/Fabric8CatalogWatchBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright 2012-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.cloud.kubernetes.fabric8.catalog.watch;

import java.util.Map;
import java.util.Set;

import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.extension.ExtendWith;
import org.testcontainers.k3s.K3sContainer;

import org.springframework.boot.test.system.OutputCaptureExtension;
import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties;
import org.springframework.cloud.kubernetes.integration.tests.commons.Commons;
import org.springframework.cloud.kubernetes.integration.tests.commons.fabric8_client.Util;
import org.springframework.test.context.TestPropertySource;

/**
* @author wind57
*/

@TestPropertySource(
properties = { "spring.main.cloud-platform=kubernetes", "spring.cloud.config.import-check.enabled=false",
"spring.cloud.kubernetes.discovery.catalogServicesWatchDelay=2000",
"spring.cloud.kubernetes.client.namespace=default",
"logging.level.org.springframework.cloud.kubernetes.fabric8.discovery=DEBUG" })
@ExtendWith(OutputCaptureExtension.class)
abstract class Fabric8CatalogWatchBase {

protected static final String NAMESPACE = "default";

protected static final String NAMESPACE_A = "a";

protected static final String NAMESPACE_B = "b";

protected static final K3sContainer K3S = Commons.container();

protected static Util util;

@BeforeAll
protected static void beforeAll() {
K3S.start();
util = new Util(K3S);
}

protected static KubernetesDiscoveryProperties discoveryProperties(boolean useEndpointSlices) {
return new KubernetesDiscoveryProperties(true, false, Set.of(NAMESPACE, NAMESPACE_A), true, 60, false, null,
Set.of(443, 8443), Map.of(), null, KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, useEndpointSlices,
false, null);
}

protected static KubernetesClient client() {
String kubeConfigYaml = K3S.getKubeConfigYaml();
Config config = Config.fromKubeconfig(kubeConfigYaml);
return new KubernetesClientBuilder().withConfig(config).build();
}

}
254 changes: 0 additions & 254 deletions ...ava/org/springframework/cloud/kubernetes/fabric8/catalog/watch/Fabric8CatalogWatchIT.java
View file
Open in desktop

This file was deleted.

Loading
Loading