Update Cruise Control system tests #9574

kyguy · 2024-01-19T19:15:27Z

Type of change

Refactoring

Description

Refactors Cruise Control system tests to remove system tests covered by unit tests and other system tests in attempt to reduce unnecessary test failures and resource usage.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj · 2024-01-24T08:16:15Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlConfigurationST.java

@@ -256,6 +261,23 @@ void testConfigurationPerformanceOptions(ExtensionContext extensionContext) thro
                assertThat(containerConfiguration, hasEntry(key, value.toString())));
    }

+    @ParallelNamespaceTest
+    void testKafkaRebalanceAutoApprovalMechanism(ExtensionContext extensionContext) {


I don't follow why you moved this here. This test seems to not be related to the other tests in this class. It tests the Cruise Control API (which is the KafkaRebalance resource). So it should have stayed where it was?

(Leaving aside that at least some tests in this class look like they should be unit tests and not system tests)

I don't follow why you moved this here. This test seems to not be related to the other tests in this class. It tests the Cruise Control API (which is the KafkaRebalance resource). So it should have stayed where it was?

Whoops, I meant to move that in the CruiseControlST class. It seemed that the CruiseControlST class has tests related to the KafkaRebalance resource where as the CruiseControlApiST class seemed to contain tests for hitting/testing the CC API directly.

(Leaving aside that at least some tests in this class look like they should be unit tests and not system tests)

I'll investigate this class to to see what can be removed

Yeah, that makes sense I guess.

scholzj · 2024-01-24T08:18:01Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlApiST.java

        LOGGER.info("Verifying that {} REST API is available", CRUISE_CONTROL_NAME);
+        CruiseControlUtils.ApiResult response = CruiseControlUtils.callApi(testStorage.getNamespaceName(), CruiseControlUtils.HttpMethod.GET, CruiseControlEndpoints.STATE);


So, what exactly does this whole test test? And why is it not suported in KRaft mode? I guess that question applies to more of the tests in this class.

So, what exactly does this whole test test?

This is just to test basic CC API functionality, that you can get the state, execute rebalances, stop proposal execution, get user task list containing the previously executed tasks

And why is it not suported in KRaft mode?

Appears to be an oversight, let me clean that up

Do we really need to test these? I would expect that those are:

Tested by Cruise Controls own tests

Used in the KafkaRebalance tests

So I would expect here more or less just some check that we unlocked the REST API. But not detailed tests of different CC fetures.

So I would expect here more or less just some check that we unlocked the REST API. But not detailed tests of different CC fetures.

Yeah that sounds fair to me, I'll take a stab at it with the next commit

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj · 2024-01-25T09:40:19Z

/azp run regression

azure-pipelines · 2024-01-25T09:40:31Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

im-konge

Thanks for the clean-up, I have just one nit in indent.

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

Co-authored-by: Lukáš Král <53821852+im-konge@users.noreply.github.com> Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj · 2024-01-26T09:42:35Z

/azp run regression

azure-pipelines · 2024-01-26T09:42:47Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-01-26T23:31:48Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlConfigurationST.java

@@ -62,158 +51,29 @@ public class CruiseControlConfigurationST extends AbstractST {
    private static final Logger LOGGER = LogManager.getLogger(CruiseControlConfigurationST.class);

    @ParallelNamespaceTest
-    void testDeployAndUnDeployCruiseControl(ExtensionContext extensionContext) throws IOException {
+    void testMetricReporterConfigurationInKafka(ExtensionContext extensionContext) throws IOException {


This test seems to be failing. Also, I'm not sure why you made the changes you did here.

The original code seemed to be at least checking that the CC pod is removed and that rolling updates to do it were properly happening here. You now just check a config mpa with generated configuration. That is done in the unit tests already and does not need a system test. So we should either reintroduce the original logic or we can remove it completely.

Reverted to how it was before

scholzj · 2024-01-26T23:36:17Z

...mon/src/main/java/io/strimzi/operator/common/model/cruisecontrol/CruiseControlEndpoints.java

+    /**
+     * metrics
+     */
+    METRICS("/metrics");


metrics => Metrics if we want to keep this. But I'm not sure it is worth to pull this into a production code when it is relevant only to tests.

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj · 2024-01-27T12:03:14Z

/azp run regression

azure-pipelines · 2024-01-27T12:03:25Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-01-27T18:12:00Z

Thanks @kyguy

kyguy added this to the 0.40.0 milestone Jan 19, 2024

kyguy force-pushed the update-cc-st branch 2 times, most recently from af48ba7 to c7e8739 Compare January 24, 2024 00:39

Update CruiseControlApiST tests

0c0ca87

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy force-pushed the update-cc-st branch from c7e8739 to 0c0ca87 Compare January 24, 2024 00:40

kyguy marked this pull request as ready for review January 24, 2024 02:49

kyguy requested review from im-konge and scholzj January 24, 2024 02:49

scholzj reviewed Jan 24, 2024

View reviewed changes

Remove system tests covered by unit tests

04b5670

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy changed the title ~~Update CruiseControlApiST tests~~ Update Cruise Control system tests Jan 25, 2024

Fix ST error

993088b

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

im-konge approved these changes Jan 25, 2024

View reviewed changes

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java Outdated Show resolved Hide resolved

Address feedback - lk

159afd2

Co-authored-by: Lukáš Král <53821852+im-konge@users.noreply.github.com> Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj reviewed Jan 26, 2024

View reviewed changes

kyguy added 2 commits January 26, 2024 18:51

Fix other STs

3e28938

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Addressing feedback -js

8c28b65

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj approved these changes Jan 27, 2024

View reviewed changes

scholzj merged commit d52eafd into strimzi:main Jan 27, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Cruise Control system tests #9574

Update Cruise Control system tests #9574

kyguy commented Jan 19, 2024 •

edited

scholzj Jan 24, 2024

kyguy Jan 24, 2024

scholzj Jan 24, 2024

scholzj Jan 24, 2024

kyguy Jan 24, 2024

scholzj Jan 24, 2024

kyguy Jan 24, 2024

scholzj commented Jan 25, 2024

azure-pipelines bot commented Jan 25, 2024

im-konge left a comment

scholzj commented Jan 26, 2024

azure-pipelines bot commented Jan 26, 2024

scholzj Jan 26, 2024

kyguy Jan 27, 2024

scholzj Jan 26, 2024

kyguy Jan 27, 2024

scholzj commented Jan 27, 2024

azure-pipelines bot commented Jan 27, 2024

scholzj commented Jan 27, 2024

		LOGGER.info("Verifying that {} REST API is available", CRUISE_CONTROL_NAME);
		CruiseControlUtils.ApiResult response = CruiseControlUtils.callApi(testStorage.getNamespaceName(), CruiseControlUtils.HttpMethod.GET, CruiseControlEndpoints.STATE);

Update Cruise Control system tests #9574

Update Cruise Control system tests #9574

Conversation

kyguy commented Jan 19, 2024 • edited

Type of change

Description

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

scholzj commented Jan 25, 2024

azure-pipelines bot commented Jan 25, 2024

im-konge left a comment

Choose a reason for hiding this comment

scholzj commented Jan 26, 2024

azure-pipelines bot commented Jan 26, 2024

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

scholzj commented Jan 27, 2024

azure-pipelines bot commented Jan 27, 2024

scholzj commented Jan 27, 2024

kyguy commented Jan 19, 2024 •

edited