[Enhancement] Refactor common component tests into Abstract test suite

[samuel-hawker]

Is your feature request related to a problem? Please describe. Many components of Strimzi follow the same underlying configuration patterns, such as using 'templates' to allow for a user to configure things such as: images, envars, annotations, labels, tolerations, affinities etc for each pod/container.

These are tested throughout the components, but most often when a new component is added these tests are copied and pasted., this is not only more code to maintain, but often variables, and comments are incorrectly modified or not changed at all, making the tests less useful or informative.

Some of the classes with these copy and pasted tests are: cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaConnectS2IClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaBridgeClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaExporterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaConnectClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/EntityOperatorTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaMirrorMakerClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/JmxTransTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaMirrorMaker2ClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/ZookeeperClusterTest.java cluster-operator/src/test/java/io/strimzi/operator/cluster/model/CruiseControlTest.java

Describe the solution you'd like Having an AbstractComponentTest class would be useful for mandating a certain set of tests for each component. This at first will enforce all components to follow these tests. If a test doesn't apply to a component this method could then be overriden with an empty test and a documented comment explaining why the component is exempt would be supplied.

As a more ambitious further goal of this, defining a test such as the much copied testTemplate() test:

    @Test
    public void testTemplate() {
        Map<String, String> depLabels = TestUtils.map("l1", "v1", "l2", "v2");
        Map<String, String> depAnots = TestUtils.map("a1", "v1", "a2", "v2");

        Map<String, String> podLabels = TestUtils.map("l3", "v3", "l4", "v4");
        Map<String, String> podAnots = TestUtils.map("a3", "v3", "a4", "v4");

        Map<String, String> svcLabels = TestUtils.map("l5", "v5", "l6", "v6");
        Map<String, String> svcAnots = TestUtils.map("a5", "v5", "a6", "v6");

        Affinity affinity = new AffinityBuilder()
                .withNewNodeAffinity()
                    .withNewRequiredDuringSchedulingIgnoredDuringExecution()
                        .withNodeSelectorTerms(new NodeSelectorTermBuilder()
                                .addNewMatchExpression()
                                    .withNewKey("key1")
                                    .withNewOperator("In")
                                    .withValues("value1", "value2")
                                .endMatchExpression()
                                .build())
                    .endRequiredDuringSchedulingIgnoredDuringExecution()
                .endNodeAffinity()
                .build();

        List<Toleration> tolerations = singletonList(new TolerationBuilder()
                .withEffect("NoExecute")
                .withKey("key1")
                .withOperator("Equal")
                .withValue("value1")
                .build());

        Kafka resource = new KafkaBuilder(ResourceUtils.createKafkaCluster(namespace, cluster, replicas, image, healthDelay, healthTimeout))
                .editSpec()
                    .withNewCruiseControl()
                        .withImage(ccImage)
                        .withNewTemplate()
                            .withNewDeployment()
                                .withNewMetadata()
                                    .withLabels(depLabels)
                                    .withAnnotations(depAnots)
                                .endMetadata()
                            .endDeployment()
                            .withNewPod()
                                .withNewMetadata()
                                    .withLabels(podLabels)
                                    .withAnnotations(podAnots)
                                .endMetadata()
                                .withNewPriorityClassName("top-priority")
                                .withNewSchedulerName("my-scheduler")
                                .withAffinity(affinity)
                                .withTolerations(tolerations)
                            .endPod()
                            .withNewApiService()
                                .withNewMetadata()
                                    .withLabels(svcLabels)
                                    .withAnnotations(svcAnots)
                                .endMetadata()
                            .endApiService()
                        .endTemplate()
                    .endCruiseControl()
                .endSpec()
                .build();

        CruiseControl cc = CruiseControl.fromCrd(resource, VERSIONS);

        // Check Deployment
        Deployment dep = cc.generateDeployment(true, depAnots, null, null);
        depLabels.putAll(expectedLabels());
        assertThat(dep.getMetadata().getLabels(), is(depLabels));
        assertThat(dep.getMetadata().getAnnotations(), is(depAnots));

        // Check Pods
        podLabels.putAll(expectedLabels());
        assertThat(dep.getSpec().getTemplate().getMetadata().getLabels(), is(podLabels));
        assertThat(dep.getSpec().getTemplate().getMetadata().getAnnotations(), is(podAnots));
        assertThat(dep.getSpec().getTemplate().getSpec().getPriorityClassName(), is("top-priority"));
        assertThat(dep.getSpec().getTemplate().getSpec().getSchedulerName(), is("my-scheduler"));
        assertThat(dep.getSpec().getTemplate().getSpec().getAffinity(), is(affinity));
        assertThat(dep.getSpec().getTemplate().getSpec().getTolerations(), is(tolerations));

        // Check Service
        svcLabels.putAll(expectedLabels());
        Service svc = cc.generateService();
        assertThat(svc.getMetadata().getLabels(), is(svcLabels));
        assertThat(svc.getMetadata().getAnnotations(),  is(svcAnots));
    }

Could be made into a single test shared by all implementing classes and converted into something like:

    @Test
    public void testTemplate() {
        Resource resource = resourceWithComponentTemplates()

        AbstractModel model = model(resource);

        assertState(model)
    }

where these abstract methods have no implementation but must be implemented by the implementing class, meaning the differing code between tests is only the component specific code.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

[samuel-hawker]

This issue probably should be split into multiple sub issues, more specifically the first part of this issue should be an investigation/plan on how to refactor these tests, whether it's feasible and whether it provides enough value to validate its implementation.

[scholzj]

Triaged on 7.6.2022: Every resource has its own set of template properties. So it probably cannot be completely unified. It would always need some custom tests for the custom fields unique to the different resources. It was not worked on for almost 2 years, so it is probably not too important.