Convert existing e2e tests from Gingko to e2e framework #62

abdurrehman107 · 2025-02-04T19:49:14Z

The above adds the default e2e tests written using the e2e framework. These were earlier written in Gingko. The PR creates a Kind Cluster and installs Prometheus, Cert Manager and Controller manager.

The e2e_test.go file is left empty so I can include tests in it. Most of the work done in Gingko earlier has been replicated in the e2e_suite_test.go.

There are a couple things such as displaying logs at the end of each test (AfterAll() in Gingko) which I cannot add in e2e framework until we begin writing the tests. I have a few more tests ready, and we can begin on them once this PR is merged.

k8s-ci-robot · 2025-02-04T19:49:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abdurrehman107
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-02-04T19:49:23Z

Hi @abdurrehman107. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ahrtr · 2025-02-04T19:57:11Z

/ok-to-test

abdurrehman107 · 2025-02-04T20:04:23Z

/retest

abdurrehman107 · 2025-02-04T20:28:44Z

/retest

test/e2e/e2e_suite_test.go

ahrtr · 2025-02-05T15:41:53Z

The high level structure looks ok, but there is some overlaps between the e2e test suite and the Makefile as pointed out the above comments. The most straightforward way is to reuse the existing Makefile targets. Any refactoring or improvement should be in separate PRs.

@hakman @gdasson @ArkaSaha30 PTAL

The etcdcluster_controller_test.go needs to be updated as well. It can be addressed in a separate PR.

abdurrehman107 · 2025-02-05T19:58:22Z

The etcdcluster_controller_test.go needs to be updated as well. It can be addressed in a separate PR

Right. I'll take this up in the PR right after the e2e-tests.go PR.

test/e2e/e2e_suite_test.go

ahrtr · 2025-02-06T13:26:26Z

Suggest to follow the same order to setup & teardown the env. All e2e tests will be executed in between.

Setup:

Create a Kind cluster
- either in workflow check or in e2e test case as mentioned in Convert existing e2e tests from Gingko to e2e framework #62 (comment)
prepare the resources
- generate files
- generate manifests
- build operator images
install dependencies (optional for now)
- install prometheus operator
- install certificate maanger
set up environment
- create namespace
- install CRD
- deploy etcd operator

Teardown

cleanup environment
- undeploy etcd-operator
- uninstall CRD
- remove namespace
remove dependencies (optional)
- uninstall prometheus operator
- uninstall certificate manager

Also please always keep in mind that one PR is supposed to do only one thing. Please move unrelated change (i.e. etcd version bumping) into separate PR.

abdurrehman107 · 2025-02-07T09:40:11Z

Please move unrelated change (i.e. etcd version bumping) into separate PR.

@ahrtr do you want me to remove the go.mod file in my PR, that'll stop from bumping the etcd version but it'll also remove all the new libraries. Or do you want me to only remove the specific lines referring to

ahrtr · 2025-02-07T10:01:31Z

@ahrtr do you want me to remove the go.mod file in my PR, that'll stop from bumping the etcd version but it'll also remove all the new libraries. Or do you want me to only remove the specific lines referring to

I won't insist on it this time. Please keep in mind next time.

Please organize & order the setup & teardown stuff based on my comment above.

abdurrehman107 · 2025-02-07T19:41:11Z

@ahrtr, implemented your suggestions. Can you please review again?

test/e2e/e2e_suite_test.go

Signed-off-by: Abdur Rehman <[email protected]>

abdurrehman107 · 2025-02-07T22:59:25Z

Addressed feedback @ahrtr, can you please review again?

ahrtr · 2025-02-10T10:53:58Z

test/e2e/e2e_suite_test.go

+	testEnv.Setup(
+		// create KinD cluster and namespace
+		envfuncs.CreateCluster(kindCluster, kindClusterName),
+		envfuncs.CreateNamespace(namespace),


Pls move it into "setup up environment" as the first step.

And remove the namespace in "cleanup environment" as the last step.

@ahrtr make undeploy deletes the namespace earlier. So deleting it manually errors out. I think we should also remove the step to create the namespace, since make deploy creates it eitherway.

The existing e2e test create the namespace and deploy the controller in BeforeAll, and undeploy the controller and remove the namespace in AfterAll. Can you sort out why the existing e2e test doesn't run into any issue?

etcd-operator/test/e2e/e2e_test.go

Lines 50 to 85 in 3879c6e

BeforeAll(func() {

By("creating manager namespace")

cmd := exec.Command("kubectl", "create", "ns", namespace)

_, err := utils.Run(cmd)

Expect(err).NotTo(HaveOccurred(), "Failed to create namespace")

By("installing CRDs")

cmd = exec.Command("make", "install")

_, err = utils.Run(cmd)

Expect(err).NotTo(HaveOccurred(), "Failed to install CRDs")

By("deploying the controller-manager")

cmd = exec.Command("make", "deploy", fmt.Sprintf("IMG=%s", projectImage))

_, err = utils.Run(cmd)

Expect(err).NotTo(HaveOccurred(), "Failed to deploy the controller-manager")

})

// After all tests have been executed, clean up by undeploying the controller, uninstalling CRDs,

// and deleting the namespace.

AfterAll(func() {

By("cleaning up the curl pod for metrics")

cmd := exec.Command("kubectl", "delete", "pod", "curl-metrics", "-n", namespace)

_, _ = utils.Run(cmd)

By("undeploying the controller-manager")

cmd = exec.Command("make", "undeploy")

_, _ = utils.Run(cmd)

By("uninstalling CRDs")

cmd = exec.Command("make", "uninstall")

_, _ = utils.Run(cmd)

By("removing manager namespace")

cmd = exec.Command("kubectl", "delete", "ns", namespace)

_, _ = utils.Run(cmd)

})

Just manually ran the Gingko tests by commenting out the part where they delete the namespace. It still worked. From my understanding it is meant to be a fault tolerant way to ensure that the namespace does not remain not-deleted at the end of the test.

However, I don't think this is needed in our case. Because we dont have to manually pre-create a Kind cluster like with Gingko (we must have a kind cluster already there to run the e2e test in Gingko). In our case we create and destroy the entire Kind cluster, all within the e2e test. So we don't need a "fault-tolerant" method, because the cluster would get deleted anyways.

Just manually ran the Gingko tests by commenting out the part where they delete the namespace. It still worked.

It works doesn't means it's correct. You create it, then you need to remove it.

It seems that the namespace etcd-operator-system automatically applied? The existing e2e test still manually create the namespace on setup and remove it on teardown as mentioned above. Is it necessary or not? @ArkaSaha30

etcd-operator/config/default/kustomization.yaml

Line 2 in 3879c6e

namespace: etcd-operator-system

Generally, I suggest to following existing e2e pattern. Any improvement can be done later in separate PR.

ahrtr · 2025-02-10T10:54:31Z

test/e2e/e2e_suite_test.go

+			if err := os.Chdir("../../"); err != nil {
+				log.Printf("Unable to set working directory: %s", err)
+				return ctx, err
+			}


We expect users to run the e2e test from the root directory. go test ./test/e2e/

Suggested change

if err := os.Chdir("../../"); err != nil {

log.Printf("Unable to set working directory: %s", err)

return ctx, err

}

ahrtr · 2025-02-10T10:54:54Z

test/e2e/e2e_suite_test.go

+			if err := os.Chdir("../../"); err != nil {
+				log.Printf("Unable to set working directory: %s", err)
+				return ctx, err
+			}


We expect users to run the e2e test from the root directory. go test ./test/e2e/

Suggested change

if err := os.Chdir("../../"); err != nil {

log.Printf("Unable to set working directory: %s", err)

return ctx, err

}

@ahrtr, I did manually test using go test ./test/e2e from the root directory, the tests fail if I do not move out of the directory. The go test ./test/e2e/ makes them start from the directory test/e2e.

Again, why the existing e2e test work without this?

cc @ArkaSaha30 @hakman

ahrtr · 2025-02-10T10:56:40Z

config/manager/manager.yaml

@@ -63,7 +63,7 @@ spec:
        args:
          - --leader-elect
          - --health-probe-bind-address=:8081
-        image: controller:latest
+        image: etcd-operator-controller:v0.1


You change "latest" to "v0.1.0", it means that we will have to maintain it each time when we bump the version. Suggest not to change it, and let's always add tag "latest" for the image as well.

@ahrtr we are building the image locally. If I put the tag latest, it for some reason defaults to trying to pull the image from a registry (the default one being DockerHub). I used the tag current and it works. Sounds good?

Signed-off-by: Abdur Rehman <[email protected]>

ArkaSaha30 · 2025-02-10T17:39:36Z

go.mod

 	go.uber.org/zap v1.27.0
 	k8s.io/api v0.32.1
 	k8s.io/apimachinery v0.32.1
 	k8s.io/client-go v0.32.1
 	k8s.io/klog/v2 v2.130.1
 	k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738
 	sigs.k8s.io/controller-runtime v0.20.1
+	sigs.k8s.io/e2e-framework v0.5.0


Can we maintain a separate go.mod for test/e2e ...similar to etcd:https://github.com/etcd-io/etcd/tree/main/tests ?
Also, a newer v0.6.0 is available, can we use that?

ArkaSaha30 · 2025-02-10T17:39:44Z

test/e2e/e2e_suite_test.go


-	"go.etcd.io/etcd-operator/test/utils"
+	kubebuilder_utils "go.etcd.io/etcd-operator/test/utils"


Does it make more sense to name it as e2e_utils or test_utils?

ArkaSaha30 · 2025-02-10T17:46:42Z

test/e2e/e2e_suite_test.go

+func TestMain(m *testing.M) {
+	testEnv = env.New()
+	kindClusterName := "etcd-cluster"
+	kindCluster := kind.NewCluster(kindClusterName)


Should we keep the option for which version of Kubernetes the kind cluster should spin up with? Default can use the latest available.
Ref: https://github.com/kubernetes-sigs/e2e-framework/blob/main/support/kind/kind.go#L28

ahrtr · 2025-02-10T19:31:28Z

I suggest to add a very simple e2e test to do some simple sanity test to ensure the environment is healthy.

ahrtr · 2025-02-10T19:32:22Z

test/e2e/e2e_suite_test.go

+			return ctx, nil
+		},
+
+		// set up environment


Suggested change

// set up environment

// create namespace and deploy the etcd-operator

k8s-ci-robot added needs-ok-to-test size/XL labels Feb 4, 2025

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 9ee93c7 to 2477fcf Compare February 4, 2025 19:49

abdurrehman107 mentioned this pull request Feb 4, 2025

Convert existing e2e tests from Gingko to e2e framework #61

Closed

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 4, 2025

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 0635056 to 8108e75 Compare February 4, 2025 20:04

ahrtr requested review from gdasson and hakman February 4, 2025 20:15

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from d6c967c to 48b58ec Compare February 4, 2025 20:28

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from c8133bf to 0d14085 Compare February 5, 2025 15:06

ahrtr reviewed Feb 5, 2025

View reviewed changes

test/e2e/e2e_suite_test.go Outdated Show resolved Hide resolved

test/e2e/e2e_suite_test.go Outdated Show resolved Hide resolved

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch 2 times, most recently from 40c08a3 to 4e63d28 Compare February 5, 2025 19:56

abdurrehman107 requested a review from ahrtr February 5, 2025 19:57

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch 3 times, most recently from 1f83aa1 to dd1123b Compare February 5, 2025 21:22

ahrtr reviewed Feb 6, 2025

View reviewed changes

test/e2e/e2e_suite_test.go Outdated Show resolved Hide resolved

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 8cb7c25 to 366396a Compare February 7, 2025 19:18

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 366396a to 24f0672 Compare February 7, 2025 19:21

abdurrehman107 requested a review from ahrtr February 7, 2025 19:41

ahrtr reviewed Feb 7, 2025

View reviewed changes

test/e2e/e2e_suite_test.go Outdated Show resolved Hide resolved

test/e2e/e2e_suite_test.go Show resolved Hide resolved

test/e2e/e2e_suite_test.go Show resolved Hide resolved

Convert existing e2e tests from Gingko to e2e framework

053e196

Signed-off-by: Abdur Rehman <[email protected]>

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 24f0672 to 053e196 Compare February 7, 2025 22:47

abdurrehman107 requested a review from ahrtr February 7, 2025 22:59

ahrtr reviewed Feb 10, 2025

View reviewed changes

add recommended changes

0f69247

Signed-off-by: Abdur Rehman <[email protected]>

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 112a460 to 0f69247 Compare February 10, 2025 16:50

ahrtr requested a review from ivanvc February 10, 2025 16:58

lint

cb6347e

Signed-off-by: Abdur Rehman <[email protected]>

ArkaSaha30 reviewed Feb 10, 2025

View reviewed changes

ahrtr reviewed Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert existing e2e tests from Gingko to e2e framework #62

Convert existing e2e tests from Gingko to e2e framework #62

abdurrehman107 commented Feb 4, 2025

k8s-ci-robot commented Feb 4, 2025

k8s-ci-robot commented Feb 4, 2025

ahrtr commented Feb 4, 2025

abdurrehman107 commented Feb 4, 2025

abdurrehman107 commented Feb 4, 2025

ahrtr commented Feb 5, 2025

abdurrehman107 commented Feb 5, 2025

ahrtr commented Feb 6, 2025

abdurrehman107 commented Feb 7, 2025

ahrtr commented Feb 7, 2025

abdurrehman107 commented Feb 7, 2025

abdurrehman107 commented Feb 7, 2025

ahrtr Feb 10, 2025

abdurrehman107 Feb 10, 2025

ahrtr Feb 10, 2025

abdurrehman107 Feb 10, 2025 •

edited

Loading

ahrtr Feb 10, 2025

ahrtr Feb 10, 2025

ahrtr Feb 10, 2025

abdurrehman107 Feb 10, 2025

ahrtr Feb 10, 2025

ahrtr Feb 10, 2025

abdurrehman107 Feb 10, 2025

ArkaSaha30 Feb 10, 2025

ArkaSaha30 Feb 10, 2025

ArkaSaha30 Feb 10, 2025

ahrtr commented Feb 10, 2025

ahrtr Feb 10, 2025

	BeforeAll(func() {
	By("creating manager namespace")
	cmd := exec.Command("kubectl", "create", "ns", namespace)
	_, err := utils.Run(cmd)
	Expect(err).NotTo(HaveOccurred(), "Failed to create namespace")

	By("installing CRDs")
	cmd = exec.Command("make", "install")
	_, err = utils.Run(cmd)
	Expect(err).NotTo(HaveOccurred(), "Failed to install CRDs")

	By("deploying the controller-manager")
	cmd = exec.Command("make", "deploy", fmt.Sprintf("IMG=%s", projectImage))
	_, err = utils.Run(cmd)
	Expect(err).NotTo(HaveOccurred(), "Failed to deploy the controller-manager")
	})

	// After all tests have been executed, clean up by undeploying the controller, uninstalling CRDs,
	// and deleting the namespace.
	AfterAll(func() {
	By("cleaning up the curl pod for metrics")
	cmd := exec.Command("kubectl", "delete", "pod", "curl-metrics", "-n", namespace)
	_, _ = utils.Run(cmd)

	By("undeploying the controller-manager")
	cmd = exec.Command("make", "undeploy")
	_, _ = utils.Run(cmd)

	By("uninstalling CRDs")
	cmd = exec.Command("make", "uninstall")
	_, _ = utils.Run(cmd)

	By("removing manager namespace")
	cmd = exec.Command("kubectl", "delete", "ns", namespace)
	_, _ = utils.Run(cmd)
	})


		"go.etcd.io/etcd-operator/test/utils"
		kubebuilder_utils "go.etcd.io/etcd-operator/test/utils"

	// set up environment
	// create namespace and deploy the etcd-operator

Convert existing e2e tests from Gingko to e2e framework #62

Are you sure you want to change the base?

Convert existing e2e tests from Gingko to e2e framework #62

Conversation

abdurrehman107 commented Feb 4, 2025

k8s-ci-robot commented Feb 4, 2025

k8s-ci-robot commented Feb 4, 2025

ahrtr commented Feb 4, 2025

abdurrehman107 commented Feb 4, 2025

abdurrehman107 commented Feb 4, 2025

ahrtr commented Feb 5, 2025

abdurrehman107 commented Feb 5, 2025

ahrtr commented Feb 6, 2025

Setup:

Teardown

abdurrehman107 commented Feb 7, 2025

ahrtr commented Feb 7, 2025

abdurrehman107 commented Feb 7, 2025

abdurrehman107 commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdurrehman107 Feb 10, 2025 • 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

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

ahrtr commented Feb 10, 2025

Choose a reason for hiding this comment

abdurrehman107 Feb 10, 2025 •

edited

Loading