test : Add unit test for old deprecated Config constructor #6157

rohanKanojia · 2024-07-19T11:59:04Z

Description

Prerequisite to #6153

Add unit test for @Deprecated Config constructor. We don't know whether this public constructor is being used by any user. Adding a test just in case to verify that it behaves as expected.
Organize tests in ConfigTest to be grouped under various load sources
- Use AssertJ throughout the test for assertions
- Group common tests sharing common initialization logic in @Nested classes
Add new test ConfigSourcePrecedenceTest for verifying Config source load precedence

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

manusa · 2024-07-24T05:36:47Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+  class KubeConfigSource {
+    @BeforeEach
+    void setUp() {
+      System.setProperty("kubeconfig", Utils.filePath(ConfigSourcePrecedenceTest.class.getResource("/test-kubeconfig")));


I assume this is reusing a .kube/config shared with other tests.

It's better if you create one specific for this test and store it as /config-source-precedence/kube-config or something similar.

manusa · 2024-07-24T05:37:36Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+    @Nested
+    @DisplayName("user configuration overrides KubeConfig attributes")
+    class UserConfigurationOverKubeConfig extends UserConfigurationViaConfigBuilder {
+    }


Not sure what's going on here

manusa · 2024-07-24T05:38:47Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+        System.setProperty("KUBERNETES_SERVICE_HOST", "10.96.0.1");
+        System.setProperty("KUBERNETES_SERVICE_PORT", "443");
+        System.setProperty("kubernetes.auth.serviceAccount.token",
+            Utils.filePath(ConfigSourcePrecedenceTest.class.getResource("/test-serviceaccount/token")));


https://github.com/fabric8io/kubernetes-client/pull/6157/files#r1689164258

manusa · 2024-07-24T05:39:19Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+    }
+
+    @Test
+    @DisplayName("Service Account files provided, then do NOT use it")


What's it in this context?

manusa · 2024-07-24T05:39:38Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+      System.setProperty("KUBERNETES_SERVICE_HOST", "10.96.0.1");
+      System.setProperty("KUBERNETES_SERVICE_PORT", "443");
+      System.setProperty("kubernetes.auth.serviceAccount.token",
+          Utils.filePath(ConfigSourcePrecedenceTest.class.getResource("/test-serviceaccount/token")));


https://github.com/fabric8io/kubernetes-client/pull/6157/files#r1689164258

manusa · 2024-07-24T05:40:46Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+        System.setProperty("KUBERNETES_SERVICE_HOST", "10.96.0.1");
+        System.setProperty("KUBERNETES_SERVICE_PORT", "443");


Why do this look like environment variables?

Yes, Config class uses Utils.getSystemPropertyOrEnvVar to resolve kubernetes apiserver host and port

kubernetes-client/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java

Lines 531 to 532 in 520894a

String masterHost = Utils.getSystemPropertyOrEnvVar(KUBERNETES_SERVICE_HOST_PROPERTY, (String) null);

String masterPort = Utils.getSystemPropertyOrEnvVar(KUBERNETES_SERVICE_PORT_PROPERTY, (String) null);

This method can handle converting property name kubernetes.service.host to environment variable KUBERNETES_SERVICE_HOST; but I think we're passing it incorrectly. At the moment, it only seems to work as both property and env names as KUBERNETES_SERVICE_HOST

kubernetes-client/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java

Lines 121 to 122 in 520894a

public static final String KUBERNETES_SERVICE_HOST_PROPERTY = "KUBERNETES_SERVICE_HOST";

public static final String KUBERNETES_SERVICE_PORT_PROPERTY = "KUBERNETES_SERVICE_PORT";

So maybe report this as a bug?

Also, given your context, not sure if this is the valid test name then?

Umm, I'm not sure if it's a bug as KubernetesClient doesn't document anywhere usage of kubernetes.service.host and kubernetes.service.port properties in https://github.com/fabric8io/kubernetes-client#configuring-the-client

KUBERNETES_SERVICE_HOST/PORT environment variables are something that's exposed via Kubernetes. KubernetesClient is just trying to detect it in auto configuration phase.

I have removed adding KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT to System Properties as they're not System Properties.

manusa · 2024-07-24T05:41:16Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+    @Nested
+    @DisplayName("user configuration overrides ServiceAccount's Config attributes")
+    class UserConfigurationOverServiceAccount extends UserConfigurationViaConfigBuilder {
+    }


https://github.com/fabric8io/kubernetes-client/pull/6157/files#r1689164779

manusa · 2024-07-24T05:42:34Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+            .hasFieldOrPropertyWithValue("masterUrl", "https://user-configuration-override:8443/")
+            .hasFieldOrPropertyWithValue("namespace", "namespace-overridden")
+            .hasFieldOrPropertyWithValue("autoOAuthToken", "token-overridden");


Can you also add an assertion for one of the configurations that's read from the initial source? -> precedence values preserved <-

manusa · 2024-07-24T05:43:00Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+    @Nested
+    @DisplayName("User configuration overrides Config attributes configured via System Properties")
+    class UserConfigurationOverSystemProperties extends UserConfigurationViaConfigBuilder {
+    }


https://github.com/fabric8io/kubernetes-client/pull/6157/files#r1689164779

manusa · 2024-07-24T05:46:56Z

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java

+  private abstract static class UserConfigurationViaConfigBuilder {
+    @SuppressWarnings("unused")
+    @Test
+    @DisplayName("User configuration via builder given most precedence")
+    void whenUserConfigurationOverridesSomeFields_thenUserConfigurationGivenPrecedence() {
+      // Given
+      Config config = new ConfigBuilder()
+          .withMasterUrl("https://user-configuration-override:8443")
+          .withNamespace("namespace-overridden")
+          .withAutoOAuthToken("token-overridden")
+          .build();
+
+      // When + Then
+      assertThat(config)
+          .hasFieldOrPropertyWithValue("masterUrl", "https://user-configuration-override:8443/")
+          .hasFieldOrPropertyWithValue("namespace", "namespace-overridden")
+          .hasFieldOrPropertyWithValue("autoOAuthToken", "token-overridden");
+    }
+  }


It's not worth reusing this test.
It obfuscates things and makes it hard to understand what's going on, especially in case of failure.
In addition, there's no check for precedence (as suggested in one of the other comments). You'd specifically want one assertion to ensure that 1. some values are overridden, and 2. some values are preserved from the precedent config

kubernetes-itests/src/test/java/io/fabric8/kubernetes/DisableAutoConfigurationIT.java

- Add unit test for `@Deprecated` Config constructor. We don't know whether this public constructor is being used by any user. Adding a test just in case to verify that it behaves as expected. - Organize tests in `ConfigTest` to be grouped under various load sources - Add new test `ConfigSourcePrecedenceTest` for verifying Config source load precedence Signed-off-by: Rohan Kumar <[email protected]>

sonarqubecloud · 2024-07-24T11:29:41Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

manusa · 2024-07-24T12:57:51Z

kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java

+    @Test
+    @DisplayName("override via system property should get more precedence over kubeconfig")
+    void testOverrideViaSystemProperties() {


This is redundant, please remove any of these redundant tests in a follow-up PR

manusa

…asses in ConfigTest - Remove `@Nested` class BuildConfigured as it doesn’t have any common setup, teardown blocks - Remove these tests as they were already covered by ConfigSourcePrecedenceTest: - `testOverrideViaSystemProperties` - `testSystemPropertiesAndBuilderGetMorePrecedenceOverKubeconfig` - Rename tests with legacy names starting with `testXXXX` - Add `@DisplayName` annotation for tests wherever applicable - Remove `assertConfig` method and inline assertions in tests wherever it’s getting used - Remove abstract test class introduced in fabric8io#6157 `AutoConfiguredDisabledScenarios`, duplicate tests so that it’s more clear in case of failure Signed-off-by: Rohan Kumar <[email protected]>

…asses in ConfigTest - Remove `@Nested` class BuildConfigured as it doesn’t have any common setup, teardown blocks - Remove these tests as they were already covered by ConfigSourcePrecedenceTest: - `testOverrideViaSystemProperties` - `testSystemPropertiesAndBuilderGetMorePrecedenceOverKubeconfig` - Rename tests with legacy names starting with `testXXXX` - Add `@DisplayName` annotation for tests wherever applicable - Remove `assertConfig` method and inline assertions in tests wherever it’s getting used - Remove abstract test class introduced in fabric8io#6157 `AutoConfiguredDisabledScenarios`, duplicate tests so that it’s more clear in case of failure - Move this test outside of ConfigTest as a separate test class `ConfigDisableAutoConfigurationTest` Signed-off-by: Rohan Kumar <[email protected]>

…sses in ConfigTest - Remove `@Nested` class BuildConfigured as it doesn’t have any common setup, teardown blocks - Remove these tests as they were already covered by ConfigSourcePrecedenceTest: - `testOverrideViaSystemProperties` - `testSystemPropertiesAndBuilderGetMorePrecedenceOverKubeconfig` - Rename tests with legacy names starting with `testXXXX` - Add `@DisplayName` annotation for tests wherever applicable - Remove `assertConfig` method and inline assertions in tests wherever it’s getting used - Remove abstract test class introduced in #6157 `AutoConfiguredDisabledScenarios`, duplicate tests so that it’s more clear in case of failure - Move this test outside of ConfigTest as a separate test class `ConfigDisableAutoConfigurationTest` Signed-off-by: Rohan Kumar <[email protected]>

rohanKanojia force-pushed the tests/config-constructor branch 10 times, most recently from 35dbbce to d2b916a Compare July 23, 2024 13:05

rohanKanojia marked this pull request as ready for review July 23, 2024 14:06

rohanKanojia requested review from manusa, oscerd and shawkins as code owners July 23, 2024 14:06

manusa added this to the 7.0.0 milestone Jul 24, 2024 — with automated-tasks

manusa reviewed Jul 24, 2024

View reviewed changes

kubernetes-itests/src/test/java/io/fabric8/kubernetes/DisableAutoConfigurationIT.java Show resolved Hide resolved

rohanKanojia force-pushed the tests/config-constructor branch 2 times, most recently from ddae5ab to 66c4e8d Compare July 24, 2024 10:19

rohanKanojia force-pushed the tests/config-constructor branch from 66c4e8d to 7a94b68 Compare July 24, 2024 10:59

manusa self-requested a review July 24, 2024 11:58

manusa reviewed Jul 24, 2024

View reviewed changes

manusa approved these changes Jul 24, 2024

View reviewed changes

manusa merged commit 00db1be into fabric8io:main Jul 24, 2024
20 of 21 checks passed

rohanKanojia deleted the tests/config-constructor branch July 24, 2024 13:12

manusa mentioned this pull request Jul 24, 2024

Release Fabric8 Kubernetes Client 6.13.2 #6097

Closed

18 tasks

rohanKanojia mentioned this pull request Jul 24, 2024

test (kubernetes-client-api) : Remove redundant methods and nested classes in ConfigTest #6194

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test : Add unit test for old deprecated Config constructor #6157

test : Add unit test for old deprecated Config constructor #6157

rohanKanojia commented Jul 19, 2024 •

edited

Loading

manusa Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

rohanKanojia Jul 24, 2024

manusa Jul 24, 2024

rohanKanojia Jul 24, 2024

rohanKanojia Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

manusa Jul 24, 2024

sonarqubecloud bot commented Jul 24, 2024

manusa Jul 24, 2024

manusa left a comment

		System.setProperty("KUBERNETES_SERVICE_HOST", "10.96.0.1");
		System.setProperty("KUBERNETES_SERVICE_PORT", "443");

	String masterHost = Utils.getSystemPropertyOrEnvVar(KUBERNETES_SERVICE_HOST_PROPERTY, (String) null);
	String masterPort = Utils.getSystemPropertyOrEnvVar(KUBERNETES_SERVICE_PORT_PROPERTY, (String) null);

	public static final String KUBERNETES_SERVICE_HOST_PROPERTY = "KUBERNETES_SERVICE_HOST";
	public static final String KUBERNETES_SERVICE_PORT_PROPERTY = "KUBERNETES_SERVICE_PORT";

test : Add unit test for old deprecated Config constructor #6157

test : Add unit test for old deprecated Config constructor #6157

Conversation

rohanKanojia commented Jul 19, 2024 • edited Loading

Description

Type of change

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 24, 2024

Quality Gate passed

Choose a reason for hiding this comment

manusa left a comment

Choose a reason for hiding this comment

rohanKanojia commented Jul 19, 2024 •

edited

Loading