move fabric8-config to junit-5 #798

wind57 · 2021-05-20T18:05:59Z

No description provided.

wind57 · 2021-05-20T18:25:40Z

...ework/cloud/kubernetes/fabric8/config/reload/EventBasedConfigurationChangeDetectorTests.java

 import io.fabric8.kubernetes.api.model.DoneableConfigMap;
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.Resource;


the changes here are obvious: move to junit-5 + minor nitpick on a raw type.

wind57 · 2021-05-20T18:26:16Z

.../springframework/cloud/kubernetes/fabric8/config/reload/ConfigurationChangeDetectorTest.java

 import java.util.Map;

-import org.junit.Assert;
-import org.junit.Test;


trivial changes also : junit-5 + assertJ to be consistent

wind57 · 2021-05-20T18:27:54Z

...pringframework/cloud/kubernetes/fabric8/config/reload/ConfigReloadAutoConfigurationTest.java


-	@BeforeClass
-	public static void setUpBeforeClass() {
+	// injected because of @EnableKubernetesMockClient. Because of the way


I hope this comment makes sense of why the configuration has moved in here from the common class. It is just the way @EnableKubernetesMockClient "injects" fields.

wind57 · 2021-05-20T18:29:15Z

...pringframework/cloud/kubernetes/fabric8/config/reload/ConfigReloadAutoConfigurationTest.java


 		HashMap<String, String> data = new HashMap<>();
 		data.put("bean.greeting", "Hello ConfigMap, %s!");
-		server.expect().withPath("/api/v1/namespaces/test/configmaps/" + APPLICATION_NAME)


mock client works slightly differently now and there is no need to mock paths, instead everything happens on the client itself. It is the same exact thing, logically.

wind57 · 2021-05-20T20:30:16Z

.../src/test/java/org/springframework/cloud/kubernetes/fabric8/config/MultipleSecretsTests.java


 		mockClient.secrets().inNamespace(DEFAULT_NAMESPACE).create(secret1);

-		Map<String, String> metadata2 = new HashMap() {


refactor the anti-pattern away

wind57 · 2021-05-20T20:32:05Z

...pringframework/cloud/kubernetes/fabric8/config/reload/ConfigReloadAutoConfigurationTest.java

-		setup("spring.cloud.kubernetes.enabled=true", "spring.cloud.kubernetes.config.enabled=false",
-				"spring.cloud.kubernetes.secrets.enabled=false", "spring.cloud.kubernetes.reload.enabled=true");
+	public void kubernetesReloadEnabledButSecretAndConfigDisabled() {
+		setup(KubernetesClientTestConfiguration.class, "spring.cloud.kubernetes.enabled=true",


setup method was slightly changed to get Class<?> as input, and I pass the KubernetesClientTestConfiguration.class configuration in there. everything else stays the same

wind57 · 2021-05-20T20:38:55Z

...c/test/java/org/springframework/cloud/kubernetes/fabric8/config/MultipleConfigMapsTests.java

+	private static void createConfigmap(KubernetesClient client, String configMapName, String namespace,
 			Map<String, String> data) {

-		server.expect().withPath(String.format("/api/v1/namespaces/%s/configmaps/%s", namespace, configMapName))


no need to mock paths anymore

wind57 · 2021-05-20T21:22:25Z

...t/java/org/springframework/cloud/kubernetes/fabric8/config/ConfigMapsFromFilePathsTests.java

 	protected static final String FIRST_FILE_NAME_DUPLICATED_FULL_PATH = FILES_ROOT_PATH + "/" + FILES_SUB_PATH + "/"
 			+ FIRST_FILE_NAME;

-	@ClassRule


this is now injected by @EnableKubernetesMockClient(crud = true, https = false) so no need for the server. the rest of the changes are trivial, imho, here.

wind57 · 2021-05-20T21:23:34Z

.../src/test/java/org/springframework/cloud/kubernetes/fabric8/config/ConfigMapsMixedTests.java


 	protected static final String APPLICATION_NAME = "configmap-mixed-example";

-	@ClassRule


this is now injected by @EnableKubernetesMockClient(crud = true, https = false) so no need for the server. the rest of the changes are trivial, imho, here.

wind57 · 2021-05-20T21:23:46Z

.../src/test/java/org/springframework/cloud/kubernetes/fabric8/config/ConfigMapsMixedTests.java


 		HashMap<String, String> data = new HashMap<>();
 		data.put("bean.morning", "Buenos Dias ConfigMap, %s");
-		server.expect().withPath("/api/v1/namespaces/test/configmaps/" + APPLICATION_NAME)


no need to mock paths anymore

wind57 · 2021-05-20T21:24:49Z

...config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/ConfigMapsTest.java

-		@SuppressWarnings("unchecked")
-		Map<String, String> data = (Map<String, String>) configMapList.getAdditionalProperties().get("data");
-		assertThat(data.get("KEY")).isEqualTo("123");
+		assertThat(configMapList.getItems().size()).isEqualTo(1);


it tests the same thing, but where to find the "thing" to assert against is in a different place

wind57 · 2021-05-20T21:29:35Z

...config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/ConfigMapsTest.java

 	@Test
 	public void testConfigMapFromSingleApplicationProperties() {
 		String configMapName = "app-properties-test";
-		String namespace = "app-props";


another interesting change is that KubernetesServer used to return a NamespacedKubernetesClient, so you could do : this.server.getClient().inNamespace(namespace), specifically call inNamespace with a certain namespace. KubernetesClient that is injected can't do that so I used it's default namespace test.

As such, I deleted String namespace = "app-props";. Fundamentally, the test does the same assertions.

the same change is across all tests in this class

wind57 · 2021-05-20T21:32:35Z

...bric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/CoreTest.java


-	@Autowired(required = false)
-	private KubernetesClient client;
+	// not a fan of changing the type from KubernetesClient, but because of:


this was already fixed by me, waiting for a review. I've also asked if this will be back-ported. If it is (and you are OK with that, of course), I will upgrade the fabric8 libraries and revert this back.

What version of fabric8 was it changed in? Maybe we should make that change first, before making these?

this is still pending review from my PR here. The good news is that it is targeted in 4.13.4 and not 5.x.x. I am more than OK to wait for 4.13.4 release and only then merge this one.

That being said - I do have a minor question here. Why isn't this project moving towards fabric8 - 5.x.x? thank you

OK cool, its up to you. If you don't mind going back and making that change I am happy to merge this now.

We will move to 5.x.x in our next release train. We just don't want to make a major change like that and cause it to break people in a service release.

I will make the change in a separate PR, as soon as 4.13.4 is released. pinky-promise!

your explanation makes perfect sense, btw. thx

wind57 · 2021-05-20T21:34:13Z

...a/org/springframework/cloud/kubernetes/fabric8/config/KubernetesConfigConfigurationTest.java

+@EnableKubernetesMockClient(crud = true, https = false)
 public class KubernetesConfigConfigurationTest extends KubernetesConfigTestBase {

+	// injected because of @EnableKubernetesMockClient. Because of the way


why this is needed in this manner is explained in a similar class, I hope it makes sense.

wind57 · 2021-05-20T21:34:54Z

.../test/java/org/springframework/cloud/kubernetes/fabric8/config/KubernetesConfigTestBase.java

-		context = new SpringApplicationBuilder(PropertyPlaceholderAutoConfiguration.class,
-				KubernetesClientTestConfiguration.class, BootstrapConfiguration.class,
-				ConfigReloadAutoConfiguration.class, RefreshAutoConfiguration.class)
+	protected void setup(Class<?> mockClientConfiguration, String... env) {


this method take one more argument now

wind57 · 2021-05-20T21:49:19Z

@ryanjbaxter ready to review. The changes are not that big (but I am biased since I stayed quite a while in this code). The one change that I wish I could perform and really can't is to drop all junit-4 dependencies (transitive ones), but because of this issue that is internally used by KubernetesMockServerExtension that is declared on @EnableKubernetesMockClient - I can't. It's not a big deal, but I wish I could have done that...

ryanjbaxter · 2021-05-21T00:09:55Z

Can you mark it ready to review?

wind57 · 2021-05-21T01:58:35Z

done

ryanjbaxter · 2021-05-24T13:22:38Z

...bric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/CoreTest.java


-	@Autowired(required = false)
-	private KubernetesClient client;
+	// not a fan of changing the type from KubernetesClient, but because of:


What version of fabric8 was it changed in? Maybe we should make that change first, before making these?

ryanjbaxter · 2021-05-24T13:24:40Z

Thanks for this, just one minor question!

first attempt

f0f8818

wind57 changed the title ~~first attempt~~ move fabric8-config to junit-5 May 20, 2021

spring-cloud-issues added the waiting-for-triage label May 20, 2021

more changes

91d72a6

wind57 commented May 20, 2021

View reviewed changes

mrged main

0b5432d

wind57 marked this pull request as ready for review May 21, 2021 01:58

ryanjbaxter approved these changes May 24, 2021

View reviewed changes

ryanjbaxter added task waiting for feedback and removed waiting-for-triage waiting for feedback labels May 24, 2021

ryanjbaxter added this to the 2.0.3 milestone May 24, 2021

ryanjbaxter merged commit 82afaca into spring-cloud:main May 24, 2021


		mockClient.secrets().inNamespace(DEFAULT_NAMESPACE).create(secret1);

		Map<String, String> metadata2 = new HashMap() {


		protected static final String APPLICATION_NAME = "configmap-mixed-example";

		@ClassRule

move fabric8-config to junit-5 #798

move fabric8-config to junit-5 #798

Uh oh!

Conversation

wind57 commented May 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wind57 May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wind57 May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wind57 commented May 20, 2021

Uh oh!

ryanjbaxter commented May 21, 2021

Uh oh!

wind57 commented May 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanjbaxter commented May 24, 2021

Uh oh!

Uh oh!

wind57 May 20, 2021 •

edited

Loading

wind57 May 24, 2021 •

edited

Loading