test: refactoring e2e #227

riya-singhal31 · 2022-07-06T11:46:02Z

Removed the Deploy Manager package as it was being used by e2e only.
Created and accessed the clients, configs and all the required functions in the e2e folder itself.

Signed-off-by: riya-singhal31 [email protected]

sp98 · 2022-07-13T08:07:41Z

e2e/config.go

 // SuiteFailed indicates whether any test in the current suite has failed
 var SuiteFailed = false
+var scheme = runtime.NewScheme()
+var CrClient crclient.Client


is the variable used outside the package? If not, then it should start with small case. Also, why the name CrClient?

Yes, used this client in pvc_test.go
So that's why initialised it with upper case.

do we need a different package for pvc? Can everything be done in the e2e pkg?

Okay, so inside lvm only lvm_suite_test.go should reside right?

Yes, it can be done. Updating

Do we need the lvm pkg at all? Can everything be in a single e2e pkg?

Updated, everything inside the same package.

sp98 · 2022-07-13T08:08:20Z

e2e/config.go

+	utilruntime.Must(snapapi.AddToScheme(scheme))
+
+	kubeconfig := os.Getenv("KUBECONFIG")
+	config, _ := getKubeconfig(kubeconfig)


error is not handled.

sp98 · 2022-07-13T08:09:12Z

e2e/config.go

+
+	kubeconfig := os.Getenv("KUBECONFIG")
+	config, _ := getKubeconfig(kubeconfig)
+	CrClient, _ = crclient.New(config, crclient.Options{Scheme: scheme})


error is not handled.

sp98 · 2022-07-13T08:15:32Z

e2e/helper.go

-	if err != nil && !errors.IsNotFound(err) {
-		return err
-	}
+	err := CrClient.Delete(context.TODO(), ns)


Need to pass proper context rather than context.TODO() for every function.

context.TODO() is still being used in a lot of places.

sp98 · 2022-07-13T08:27:15Z

e2e/config.go

 // SuiteFailed indicates whether any test in the current suite has failed
 var SuiteFailed = false
+var scheme = runtime.NewScheme()


instead of having var declared in each line, it can be grouped together. For example:
var ( )

Grouped all the vars and const.

sp98 · 2022-07-13T08:29:01Z

e2e/config.go

@@ -23,11 +32,10 @@ var LvmSubscriptionChannel string
 // DiskInstall indicates whether disks are needed to be installed.
 var DiskInstall bool


Please recheck all these variables. The ones within the scope of the package should be in small case. Like DiskInstall.

sp98 · 2022-07-13T08:31:20Z

e2e/helper.go

-		return err
-	}
+	err := CrClient.Create(context.TODO(), ns)
+	gomega.Expect(err).To(gomega.BeNil())


earlier it was validating against two errors ( not nil and IsAlreadyExists). But looks like the change (gomega error handling) does not cover IsAlreadyExists

sp98 · 2022-07-13T09:18:58Z

e2e/subscription.go

@@ -150,7 +150,7 @@ func (t *DeployManager) waitForLVMCatalogSource() error {
 }

 // WaitForLVMOperator waits for the lvm-operator to come online.


Suggested change

// WaitForLVMOperator waits for the lvm-operator to come online.

// waitForLVMOperator waits for the lvm-operator to come online.

sp98 · 2022-07-13T09:19:51Z

e2e/subscription.go

@@ -275,11 +275,11 @@ func (t *DeployManager) DeployLVMWithOLM(lvmCatalogImage string, subscriptionCha
 }

 // DeleteClusterObjects deletes remaining operator manifests.


Suggested change

// DeleteClusterObjects deletes remaining operator manifests.

// deleteClusterObjects deletes remaining operator manifests.

sp98 · 2022-07-13T09:20:46Z

e2e/subscription.go

@@ -275,11 +275,11 @@ func (t *DeployManager) DeployLVMWithOLM(lvmCatalogImage string, subscriptionCha
 }

 // DeleteClusterObjects deletes remaining operator manifests.


Suggested change

// DeleteClusterObjects deletes remaining operator manifests.

// deleteClusterObjects deletes remaining operator manifests.

sp98 · 2022-07-13T09:21:46Z

e2e/subscription.go

@@ -306,15 +306,15 @@ func (t *DeployManager) deleteClusterObjects(co *clusterObjects) error {
 }

 // UninstallLVM uninstalls lvm operator.
-func (t *DeployManager) UninstallLVM(lvmCatalogImage string, subscriptionChannel string) error {
+func UninstallLVM(lvmCatalogImage string, subscriptionChannel string) error {


start function with lower case if its not used outside the package. Please check this for all the other functions as well.

riya-singhal31 · 2022-07-15T09:25:45Z

Thanks @sp98 , @nbalacha for the reviews.
Addressed all of them.

sp98 · 2022-07-15T09:41:02Z

e2e/disk_setup.go

@@ -53,7 +53,7 @@ func diskSetup() error {
 func diskRemoval() error {
 	// get nodes
 	nodeList := &corev1.NodeList{}
-	err := DeployManagerObj.GetCrClient().List(context.TODO(), nodeList, client.HasLabels{labelNodeRoleWorker})
+	err := crClient.List(context.TODO(), nodeList, client.HasLabels{labelNodeRoleWorker})


uses context.TODO()

sp98 · 2022-07-15T09:41:40Z

e2e/pvc_check.go

@@ -29,184 +25,183 @@ func PVCTest() {
 		var clonePod *k8sv1.Pod
 		var restorePvc *k8sv1.PersistentVolumeClaim
 		var restorePod *k8sv1.Pod
-		client := tests.DeployManagerObj.GetCrClient()
 		ctx := context.TODO()


using context.TODO()

sp98 · 2022-07-18T05:54:03Z

e2e/LVMCluster.go

-	return t.crClient.Create(context.TODO(), lvmClusterRes)
+func startLVMCluster() error {
+	lvmClusterRes := generateLVMCluster()
+	return crClient.Create(context.Background(), lvmClusterRes)
 }

 // Deletes a sample CR.


as per golang coding convention, doc comment for an exported function should start with the function name. Please fix this for other functions as well.

This function is used only in this package(not an exported func), so will correct the name of function with lower case.

sp98 · 2022-07-21T06:11:18Z

discussed some more improvements that can be made in the PR offline. Adding them here:

Don't create context for each function. Have a single parent context that can be used by all the functions. Possible ways are:
- Function receiver that holds the context.
- Or pass the context as function argument.
Avoid using package level variables that are used for all the functions, for example, the way the crClient being used.
A good read on why package level variables are not recommended.

nbalacha · 2022-07-21T06:07:42Z

e2e/config.go

-// SuiteFailed indicates whether any test in the current suite has failed
-var SuiteFailed = false
+const (
+	testNamespace    = "lvm-endtoendtest"


Please add comments describing the constants and variables.

nbalacha · 2022-07-21T06:08:26Z

e2e/config.go

+	kubeconfig := os.Getenv("KUBECONFIG")
+	config, err := getKubeconfig(kubeconfig)
+	if err != nil {
+		panic(fmt.Sprintf("Failed to set kubecofig: %v", err))


Nit:

Suggested change

panic(fmt.Sprintf("Failed to set kubecofig: %v", err))

panic(fmt.Sprintf("Failed to set kubeconfig: %v", err))

nbalacha · 2022-07-21T06:13:40Z

e2e/subscription.go

 type clusterObjects struct {
 	namespaces     []k8sv1.Namespace
 	operatorGroups []v1.OperatorGroup
 	catalogSources []v1alpha1.CatalogSource
 	subscriptions  []v1alpha1.Subscription
 }

-// Generating the cluster objects.
-func (t *DeployManager) generateClusterObjects(lvmCatalogImage string, subscriptionChannel string) *clusterObjects {
+// generateClusterObjects generates the cluster objects.


Suggested change

// generateClusterObjects generates the cluster objects.

// generateClusterObjects generates the cluster objects required for deploying the operator using OLM.

riya-singhal31 · 2022-07-22T05:33:07Z

Thanks @sp98, @nbalacha for the reviews.
Addressed them all.

Signed-off-by: riya-singhal31 <[email protected]>

nbalacha · 2022-07-25T04:56:58Z

e2e/config.go

+	lvmSubscriptionChannel string
+	// diskInstall indicates whether disks are needed to be installed.
+	diskInstall          bool
+	lvmOperatorInstall   bool


Missing comments for lvmOperatorInstall and lvmOperatorUninstall.

openshift-ci · 2022-07-25T05:01:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nbalacha, riya-singhal31

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nbalacha]

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

riya-singhal31 force-pushed the refactoring branch from cf3b34b to d34b024 Compare July 12, 2022 08:09

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2022

riya-singhal31 force-pushed the refactoring branch 4 times, most recently from e5e3b7b to 3639d7b Compare July 12, 2022 09:00

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2022

riya-singhal31 changed the title ~~[WIP]test: refactoring e2e~~ test: refactoring e2e Jul 12, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2022

riya-singhal31 force-pushed the refactoring branch from 3639d7b to c25fcc8 Compare July 12, 2022 09:15

riya-singhal31 requested review from nbalacha and sp98 July 12, 2022 11:13

sp98 suggested changes Jul 13, 2022

View reviewed changes

openshift-ci bot assigned sp98 Jul 13, 2022

sp98 reviewed Jul 13, 2022

View reviewed changes

sp98 suggested changes Jul 13, 2022

View reviewed changes

riya-singhal31 force-pushed the refactoring branch from c25fcc8 to c91726c Compare July 15, 2022 09:23

riya-singhal31 force-pushed the refactoring branch from c91726c to 76cf3ab Compare July 15, 2022 09:29

riya-singhal31 requested a review from sp98 July 15, 2022 09:30

sp98 reviewed Jul 15, 2022

View reviewed changes

riya-singhal31 force-pushed the refactoring branch 2 times, most recently from 822e48d to 52e44df Compare July 15, 2022 10:51

riya-singhal31 requested a review from sp98 July 15, 2022 10:55

riya-singhal31 force-pushed the refactoring branch 2 times, most recently from 98f2e7e to 091755c Compare July 15, 2022 11:00

sp98 suggested changes Jul 18, 2022

View reviewed changes

riya-singhal31 force-pushed the refactoring branch 2 times, most recently from a62500c to 61101c6 Compare July 18, 2022 07:09

riya-singhal31 requested a review from sp98 July 18, 2022 07:10

nbalacha suggested changes Jul 21, 2022

View reviewed changes

openshift-ci bot assigned nbalacha Jul 21, 2022

riya-singhal31 force-pushed the refactoring branch from 61101c6 to c798b13 Compare July 22, 2022 05:26

riya-singhal31 requested a review from nbalacha July 22, 2022 05:32

test: refactoring e2e

28ef252

Signed-off-by: riya-singhal31 <[email protected]>

riya-singhal31 force-pushed the refactoring branch from c798b13 to 28ef252 Compare July 22, 2022 09:37

nbalacha reviewed Jul 25, 2022

View reviewed changes

nbalacha approved these changes Jul 25, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2022

openshift-merge-robot merged commit cc63237 into openshift:main Jul 25, 2022

		@@ -23,11 +32,10 @@ var LvmSubscriptionChannel string
		// DiskInstall indicates whether disks are needed to be installed.
		var DiskInstall bool

		@@ -150,7 +150,7 @@ func (t *DeployManager) waitForLVMCatalogSource() error {
		}

		// WaitForLVMOperator waits for the lvm-operator to come online.

		@@ -275,11 +275,11 @@ func (t *DeployManager) DeployLVMWithOLM(lvmCatalogImage string, subscriptionCha
		}

		// DeleteClusterObjects deletes remaining operator manifests.

	panic(fmt.Sprintf("Failed to set kubecofig: %v", err))
	panic(fmt.Sprintf("Failed to set kubeconfig: %v", err))

	// generateClusterObjects generates the cluster objects.
	// generateClusterObjects generates the cluster objects required for deploying the operator using OLM.

test: refactoring e2e #227

test: refactoring e2e #227

Conversation

riya-singhal31 commented Jul 6, 2022 • 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

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

riya-singhal31 commented Jul 15, 2022

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

sp98 commented Jul 21, 2022

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

riya-singhal31 commented Jul 22, 2022

Choose a reason for hiding this comment

openshift-ci bot commented Jul 25, 2022

riya-singhal31 commented Jul 6, 2022 •

edited

Loading