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

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.

The existing e2e test still manually create the namespace on setup and remove it on teardown as mentioned above. Is it necessary or not?
Generally, I suggest to following existing e2e pattern. Any improvement can be done later in separate PR.

I think we can add it as of now (it does throw an error that the namespace does not exist), and in a later PR we can remove it in case we feel it is necessary.

I've added it in the latest commit to this PR.

test/e2e/e2e_suite_test.go

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

The existing e2e test works because it uses the Run() function mentioned in the utils file. I don't use that specific function because it uses Gingko. I've used the RunCommand() function given by the e2e-framework. Since we wish to completely remove the use of Gingko, I suggest we use this. Below is the function used in the existing tests:

func Run(cmd *exec.Cmd) (string, error) { dir, _ := GetProjectDir() cmd.Dir = dir if err := os.Chdir(cmd.Dir); err != nil { _, _ = fmt.Fprintf(GinkgoWriter, "chdir dir: %s\n", err) } cmd.Env = append(os.Environ(), "GO111MODULE=on") command := strings.Join(cmd.Args, " ") _, _ = fmt.Fprintf(GinkgoWriter, "running: %s\n", command) output, err := cmd.CombinedOutput() if err != nil { return string(output), fmt.Errorf("%s failed with error: (%v) %s", command, err, string(output)) } return string(output), nil }

I suggest we also update the test/utils/utils.go file, because it refers to Gingko. I can take it up in another PR.

The existing e2e test works because it uses the Run() function mentioned in the utils file. I don't use that specific function because it uses Gingko. I've used the RunCommand() function given by the e2e-framework.

This isn't convincing. Shouldn't RunCommand() and command.Run() have the same or similar effect? I don't think the root cause is clear. Resolving a problem without knowing the root cause is usually dangerous, nor a recommended way.

I understand what you're saying Benjamin, and technically they should, but they are using different methods to solve the problem. The Run() function takes the input of type exec.Cmd while the RunCommand() function takes in a string and then uses the gexe library to process the command. I'd be happy to hop on a call and go through this together.

cc @ahrtr @hakman @ArkaSaha30

I just spend about 5 minutes to read the source code of the existing e2e test.

Is there any difficulty to follow the same pattern ( reuse the GetProjectDir)?

etcd-operator/test/utils/utils.go

Lines 195 to 202 in 3879c6e

func GetProjectDir() (string, error) {

wd, err := os.Getwd()

if err != nil {

return wd, err

}

wd = strings.Replace(wd, "/test/e2e", "", -1)

return wd, nil

}

Nope, no issues. I just thought implementing the e2e-framework's approach was simpler. However, I get your idea to just replicate the Gingko tests for now, and we can improve in the future.

No worries, I'll update it.

An alternative would be to replace the make calls with make -f../../Makefile. But that would require the Makefile to be agnostic of where it's being executed, and although doable, it will make it dirtier.

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?

I don't think changing this would be a good idea unless we want to change the Kustomize stack that comes from the scaffolding. But, if we want to replace it, I believe you'll need to change the instances from the Makefile:

etcd-operator/Makefile

Line 132 in 3879c6e

cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}

etcd-operator/Makefile

Line 151 in 3879c6e

cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}

So do we just wanna leave the image name to be controller?

Is there a particular reason not to? That file is usually just a template. When running make (the target that calls line 151), it replaces it with the actual location of the image (i.e., gcr.io/etcd-development/etcd-operator:v1.2.3).

But we can leave it as you did and later address this when we do the first release.

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?

Updated the version. Do we want to use a separate go.mod? We can consider doing this in a future PR too.
cc @ahrtr

test/e2e/e2e_suite_test.go

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

I don't think spinning a cluster with the latest k8s version should be a problem?

I think being able to set the version makes sense in cases where the controller doesn't work with the latest version yet. That way, we could set it to the latest supported version.

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.

test/e2e/e2e_suite_test.go

ivanvc · 2025-02-11T04:43:15Z

@abdurrehman107, could you please rebase/sync your branch with the remote main, so it's easier to review without the go.mod changes? Thanks :)

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

abdurrehman107 · 2025-02-11T10:48:20Z

@abdurrehman107, could you please rebase/sync your branch with the remote main, so it's easier to review without the go.mod changes? Thanks :)

The PR keeps failing without the go.mod @ivanvc. We add a new framework, the etcd-framework which adds to the go.mod file. I think it is necessary for the PR.

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

ivanvc · 2025-02-12T04:51:54Z

test/e2e/e2e_suite_test.go

@@ -6,7 +6,6 @@ you may not use this file except in compliance with the License.
 You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0
-


nit: I don't think we want to remove this line. It comes from the Apache license.

ivanvc · 2025-02-12T06:55:09Z

test/e2e/e2e_suite_test.go

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


I think being able to set the version makes sense in cases where the controller doesn't work with the latest version yet. That way, we could set it to the latest supported version.

ivanvc · 2025-02-12T06:57:41Z

test/e2e/e2e_suite_test.go

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


I think you'll be removing the change in directories, so this may not be relevant. But, it is not required here because there are no calls to make.

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

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

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

ArkaSaha30 reviewed Feb 10, 2025

View reviewed changes

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

ArkaSaha30 reviewed Feb 10, 2025

View reviewed changes

ahrtr reviewed Feb 10, 2025

View reviewed changes

test/e2e/e2e_suite_test.go Show resolved Hide resolved

Convert existing e2e tests from Gingko to e2e framework

5283edd

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

abdurrehman107 force-pushed the abdur-rehman/convert-gingko-to-e2e-framework branch from 00191c4 to 5283edd Compare February 11, 2025 06:25

address suggestions

f8c247e

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

abdurrehman107 requested review from ArkaSaha30 and ahrtr February 11, 2025 10:48

use make for docker-build

118d633

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

ivanvc reviewed Feb 12, 2025

View reviewed changes

	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)
	})

	func GetProjectDir() (string, error) {
	wd, err := os.Getwd()
	if err != nil {
	return wd, err
	}
	wd = strings.Replace(wd, "/test/e2e", "", -1)
	return wd, nil
	}

		@@ -6,7 +6,6 @@ you may not use this file except in compliance with the License.
		You may obtain a copy of the License at

		http://www.apache.org/licenses/LICENSE-2.0

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

abdurrehman107 Feb 11, 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

abdurrehman107 Feb 11, 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

abdurrehman107 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdurrehman107 Feb 11, 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

ahrtr commented Feb 10, 2025

ivanvc commented Feb 11, 2025

abdurrehman107 commented Feb 11, 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

abdurrehman107 Feb 11, 2025 •

edited

Loading

abdurrehman107 Feb 11, 2025 •

edited

Loading

abdurrehman107 Feb 11, 2025 •

edited

Loading

abdurrehman107 Feb 11, 2025 •

edited

Loading