Added Koperator end to end tests #987

bartam1 · 2023-05-28T14:12:34Z

Description

Adding basic end to end tests for Koperator.
Included tests:

helm uninstallation and check remained resources
kafkacluster, zookeepercluster uninstallation and check remained resources
Internal consumer producer test

Type of Change

Other (test)

Checklist

I have read the contributing guidelines
All new and existing tests pass

tests/e2e/k8s_test.go

Kuvesz

Sorry, got a lot of comments, many of them are repeating though, so it's okay to answer those only once and they shouldn't require too much effort to fix. Overall I've got some suggestions that I'd like to see implemented, but these are suggestions, it's all up for discussion.

Kuvesz · 2023-06-05T11:15:58Z

Makefile

@@ -102,7 +102,11 @@ test: generate fmt vet manifests bin/setup-envtest
 test-e2e:
 	go test github.com/banzaicloud/koperator/tests/e2e \
 		-v \
-		-timeout 5m
+		-timeout 15m \
+		--ginkgo.fail-fast \


I think we agreed the tests should be slow failing, so this line might need to be removed when we're done with testing the tests. Just a note really.

It is now only for testing purposes after Patrik PR I will change it.

tests/e2e/cert_manager_test.go

Kuvesz · 2023-06-05T11:20:59Z

tests/e2e/cert_manager_test.go

+			fmt.Sprintf(managedByHelmLabelTemplate, "cert-manager"),
+			"",
+			kubectlArgGoTemplateKindNameNamespace,
+			"--all-namespaces")


Question: Will this overwrite the namespace in the kubectlOptions?

Yes, the --all-namespaces overwrites the -n namespace parameter effect

tests/e2e/koperator_test.go

tests/e2e/prometheus_test.go

tests/e2e/zookeeper_test.go

panyuenlau

Implementation looks great! Just something very minor

panyuenlau · 2023-06-07T19:17:43Z

tests/e2e/helm_test.go

+	purge bool,
+	extraArgs ...string,
+) {
+	By(fmt.Sprintf("Checking for existing Helm release names %s", helmReleaseName))


We shouldn't use plural since we are checking for one single here

Suggested change

By(fmt.Sprintf("Checking for existing Helm release names %s", helmReleaseName))

By(fmt.Sprintf("Checking for existing Helm release name %s", helmReleaseName))

fixed: b0b2558

panyuenlau · 2023-06-07T19:23:40Z

tests/e2e/helm_test.go

+}
+
+// installHelmChart deploys a Helm chart to the specified kubectl context and
+// namespace using the specified infos, extra arguments can be any of the helm


Suggested change

// namespace using the specified infos, extra arguments can be any of the helm

// namespace using the specified info, extra arguments can be any of the helm

fixed: b0b2558

panyuenlau · 2023-06-07T19:24:00Z

tests/e2e/helm_test.go

+// installHelmChart checks whether the specified named Helm release exists in
+// the provided kubectl context and namespace, logs it if it does and returns or
+// alternatively deploys the Helm chart to the specified kubectl context and
+// namespace using the specified infos, extra arguments can be any of the helm


Suggested change

// namespace using the specified infos, extra arguments can be any of the helm

// namespace using the specified info, extra arguments can be any of the helm

fixed:b0b2558

tests/e2e/utils.go

The base branch was changed.

Kuvesz

LGTM

Kuvesz · 2023-06-15T08:35:52Z

tests/e2e/helm.go

+// uninstallHelmChart uninstall Helm chart from the specified kubectl context and
+// namespace using the specified helm release name. When purge is true, it removes records
+// and make that name free to be reused for another installation.
+// Extra arguments can be any of the helm CLI uninstall flag arguments.


Suggested change

// uninstallHelmChart uninstall Helm chart from the specified kubectl context and

// namespace using the specified helm release name. When purge is true, it removes records

// and make that name free to be reused for another installation.

// Extra arguments can be any of the helm CLI uninstall flag arguments.

// uninstallHelmChart uninstalls a Helm chart from the specified kubectl context and

// namespace using the specified helm release name. When purge is true, it removes records

// and makes that name free to be reused for another installation.

// Extra arguments can be any of the helm CLI uninstall flag arguments.

tests/e2e/test_test.go

tests/e2e/uninstall.go

pregnor

There are multiple files in which the It(...) executed functions should be error-checked. (At some places this is fixed, but you missed a couple. Let me know if I should provide a list of places.)

tests/e2e/const.go

tests/e2e/helm.go

tests/e2e/k8s.go

pregnor · 2023-06-15T13:32:01Z

tests/e2e/uninstall.go

+		k8sResourceKinds, err := listK8sAllResourceKind(kubectlOptions)
+		Expect(err).ShouldNot(HaveOccurred())
+
+		koperatorAvailableResourceKinds := stringSlicesUnion(koperatorCRDs(), k8sResourceKinds)
+		koperatorAvailableResourceKinds = append(koperatorAvailableResourceKinds, basicK8sResourceKinds()...)
+
+		remainedResources, err := getK8sResources(kubectlOptions,
+			koperatorAvailableResourceKinds,
+			fmt.Sprintf(managedByHelmLabelTemplate, koperatorLocalHelmDescriptor.ReleaseName),
+			"",
+			kubectlArgGoTemplateKindNameNamespace,
+			"--all-namespaces")
+
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(remainedResources).Should(BeEmpty())
+	})


Note: This is fine like this for now, but in a follow-up ticket we want to move these kind of checks higher up onto case level when this becomes unnecessary.
Then everything will fold up because the Helm chart uninstallation becomes only 1 line, the Koperator uninstallation becomes only 1 line so we can omit those and put the uninstall Koperator It(...) directly into the uninstall test case.

Same thing I did at the last refactor of the install test case.

Same holds true for the other components.

CC: @panyuenlau @Kuvesz @mihaialexandrescu

I dont understand what do you mean. Please explain in priv.

I know you put in your test_install.go directly those tests under When containers. Somehow I felt that it is clener and maybe more reusable if I kept the original form and put evreything in functions.

I wrote in private.

tests/e2e/helm.go

tests/e2e/uninstall_cluster_test.go

tests/e2e/utils.go

pregnor

Earlier unresolved threads:

The fixes were great, thanks.
There were a couple error checks missed so I'm commenting them by line to make it easier to go through.

Test after my suggestions, I wrote them from the top of my mind.

tests/e2e/kafka.go

pregnor · 2023-06-16T07:10:22Z

tests/e2e/kafka.go

+			},
+		)
+		err := waitK8sResourceCondition(kubectlOptions, kafkaTopicKind, "jsonpath={.status.state}=created", defaultTopicCreationWaitTime, "", topicName)
+		Expect(err).ShouldNot(HaveOccurred())


Note: According to https://pkg.go.dev/github.com/onsi/gomega#Assertion typically expect uses NotTo compared to ShouldNot, but I'm not picky about that, but we should use one of the two throughout the tests for easier human pattern recognition. I don't mind which one we use, if we feel like ShouldNot is easier to read, fine for me. I used NotTo due to being used to that, but no hard feelings.

I saw somewhere (gomega tooltip) that the "Expect(err).ShouldNot(HaveOccurred())" should be used to check error

Could you search around just quickly in 5m what is the official suggestion here?
What I linked is the official gomega docs, so I don't understand why some tooltip would be the direct opposite.
But as I said, I don't care which, whatever is preferred officially is fine for me, I care about not mixing the two.

I personally have no preference in this, both are easy to understand really. My only issue is that it would be nice to be consistent across our code, so we should really pick one. I'm not saying we have to fix this now as all occurrences can be easily replaced at any time but at some point we should I think.

tests/e2e/kafka.go

tests/e2e/produce_consume.go

tests/e2e/uninstall.go

tests/e2e/uninstall_cluster.go

pregnor · 2023-06-16T07:25:41Z

tests/e2e/utils.go

Optional: I MAY consider renaming this file to string.go for now.
Generic util packages and files are an anti-pattern because they are used as dumps instead of thinking through the organization of the units, currently this only has a string function so I would use this as string function collector file, in the future we can figure out if there is anything else to add.

Im sure that we will have things what will need to be added into the utils.go later.
Creating a new file for one function I think is not worth.
Because it is optional from you I would keep it as it is.

Ok, consider this resolved, feel free to mark the conversation as resolved.

Context: The problem is, when you are looking for something in a structured way util.go is difficult to decipher without looking into it and if there are multiple not connected functions in it, it will become a mess internally.
Instead if we structurize these functions based on their nature/topic/etc. such as string modification functions it is easier to look at the files list in the package and quickly guess where you should look for different top level functions.
That was behind the helm.go, k8s.go files as well.

pregnor · 2023-06-19T09:37:16Z

tests/e2e/test_uninstall.go

+			It("Setting globals", func() {
+				err := dependencyCRDs.Initialize(kubectlOptions)
+				Expect(err).NotTo(HaveOccurred())
+			})


Note: This needs to be moved to the per case before once we handle that ticket. Nothing to do now.

pregnor

Couple optional things and arguable recommendations, otherwise looks ok.

tests/e2e/types.go

pregnor · 2023-06-19T09:41:03Z

tests/e2e/types.go

@@ -0,0 +1,52 @@
+package e2e


(For some reason my file level commend does not work this morning.)

Optional: IMO this other than the version type which I commented, the rest of the file would fit into k8s.go with the dependencyCRDsType and the functions because we are doing k8s related ops with those.

I'm fine to leave this here if others agree it's more clear this way, but it's weird for me.

Kuvesz

Still looks good to me overall

bartam1 requested a review from a team as a code owner May 28, 2023 14:12

bartam1 changed the base branch from master to test/de2e-koperator-install May 28, 2023 14:12

Kuvesz reviewed Jun 1, 2023

View reviewed changes

tests/e2e/k8s_test.go Outdated Show resolved Hide resolved

bartam1 changed the title ~~Added Koperator uninstallation end to end test~~ Added Koperator end to end tests Jun 5, 2023

Kuvesz reviewed Jun 5, 2023

View reviewed changes

pregnor force-pushed the test/de2e-koperator-install branch 2 times, most recently from 1f98ba9 to 9db5a56 Compare June 6, 2023 11:48

panyuenlau previously approved these changes Jun 7, 2023

View reviewed changes

Base automatically changed from test/de2e-koperator-install to master June 8, 2023 11:07

bartam1 force-pushed the test/de2e-koperator-uninstall branch from b905456 to 56b6b8a Compare June 14, 2023 15:21

Kuvesz previously approved these changes Jun 15, 2023

View reviewed changes

pregnor requested changes Jun 15, 2023

View reviewed changes

bartam1 dismissed Kuvesz’s stale review via 1a732a8 June 16, 2023 01:34

bartam1 force-pushed the test/de2e-koperator-uninstall branch from c31e4dd to 1a732a8 Compare June 16, 2023 01:34

pregnor and others added 9 commits June 16, 2023 03:37

test(docs/install): added install e2e test

6df5501

test(docs/install): added localversion 4 Koperator

bd41f3b

feat(e2e): added uninstall koperator and dep

bdce649

Refactor after Pregnor PR merge

d9331f2

fix based on review Panyuenlau

b796dcd

fix: _kubectlRemoveWarnings

9d78d96

add: description for utils

2cd5d0d

refactor filenames and utils

5dd0266

update: go.mod

46b2d71

bartam1 force-pushed the test/de2e-koperator-uninstall branch from 1a732a8 to 46b2d71 Compare June 16, 2023 01:38

bartam1 added 5 commits June 16, 2023 04:35

fix: _test suffix and listAllK8sResources

16f2904

fix: using --output not -o

2b05149

remove: checkExistenceOfK8sResource

c315b37

fix: fn description and extend log

1d66fe3

fix: check returned error for waitK8sResourceCondition

d68b2ca

bartam1 added 2 commits June 16, 2023 05:37

refactor: topic and consume produce

53694b9

fix: union to instersect

cc6b1fc

pregnor requested changes Jun 16, 2023

View reviewed changes

bartam1 added 4 commits June 16, 2023 09:35

refactor: uninstall into helmDescriptior

63ed2cd

refactor: using dynamically dependencies CRDs

3addfae

fix: missing error checks

3efc5f9

formatting: waitK8sResourceCondition

abea29c

pregnor reviewed Jun 19, 2023

View reviewed changes

pregnor previously approved these changes Jun 19, 2023

View reviewed changes

bartam1 added 2 commits June 19, 2023 14:00

refactor: move version type into const.go

b6683a2

formatting: waitK8sResourceCondition

917afad

bartam1 dismissed pregnor’s stale review via 917afad June 19, 2023 12:01

Kuvesz approved these changes Jun 19, 2023

View reviewed changes

pregnor approved these changes Jun 19, 2023

View reviewed changes

bartam1 merged commit 24ec257 into master Jun 19, 2023
3 checks passed

bartam1 deleted the test/de2e-koperator-uninstall branch June 19, 2023 15:24

mihaialexandrescu mentioned this pull request Jun 27, 2023

[e2e tests] use --wait and --cascade=foreground in helm uninstall #1004

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Koperator end to end tests #987

Added Koperator end to end tests #987

bartam1 commented May 28, 2023 •

edited

Loading

Kuvesz left a comment

Kuvesz Jun 5, 2023 •

edited

Loading

bartam1 Jun 6, 2023

Kuvesz Jun 5, 2023

bartam1 Jun 6, 2023

panyuenlau left a comment

panyuenlau Jun 7, 2023

bartam1 Jun 14, 2023

panyuenlau Jun 7, 2023

bartam1 Jun 14, 2023

panyuenlau Jun 7, 2023

bartam1 Jun 14, 2023

Kuvesz left a comment

Kuvesz Jun 15, 2023

pregnor left a comment

pregnor Jun 15, 2023

bartam1 Jun 15, 2023 •

edited

Loading

bartam1 Jun 16, 2023

pregnor Jun 16, 2023

pregnor left a comment •

edited

Loading

pregnor Jun 16, 2023

bartam1 Jun 19, 2023 •

edited

Loading

pregnor Jun 19, 2023

Kuvesz Jun 19, 2023

pregnor Jun 16, 2023

bartam1 Jun 19, 2023

pregnor Jun 19, 2023

pregnor Jun 19, 2023

pregnor left a comment

pregnor Jun 19, 2023

Kuvesz left a comment

	By(fmt.Sprintf("Checking for existing Helm release names %s", helmReleaseName))
	By(fmt.Sprintf("Checking for existing Helm release name %s", helmReleaseName))

	// namespace using the specified infos, extra arguments can be any of the helm
	// namespace using the specified info, extra arguments can be any of the helm

Added Koperator end to end tests #987

Added Koperator end to end tests #987

Conversation

bartam1 commented May 28, 2023 • edited Loading

Description

Type of Change

Checklist

Kuvesz left a comment

Choose a reason for hiding this comment

Kuvesz Jun 5, 2023 • edited Loading

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

panyuenlau left a 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

Kuvesz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pregnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartam1 Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pregnor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartam1 Jun 19, 2023 • edited Loading

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

pregnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kuvesz left a comment

Choose a reason for hiding this comment

bartam1 commented May 28, 2023 •

edited

Loading

Kuvesz Jun 5, 2023 •

edited

Loading

bartam1 Jun 15, 2023 •

edited

Loading

pregnor left a comment •

edited

Loading

bartam1 Jun 19, 2023 •

edited

Loading