Validate utils Package and Taint Functionality #61

razo7 · 2023-06-29T10:31:15Z

Add one unit test for taint functionality on control-plane node with adding and removing FAR NoExecute taint
Use worker label from Commons rather than declaring it in FAR's repoistory
Run make go-verify
Create new suite test and UT file for utils package, then move out the 3 UT for Utils functionality to utils_test, and update makefile to test the new directory.

openshift-ci · 2023-06-29T10:31:19Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

razo7 · 2023-06-29T10:34:47Z

Tests will fail since the PR uses common repo which depends on newer Kubernetes API, thus #56 should be merged, and then after rebasing the PR could be tested.

mshitrit · 2023-07-02T11:25:35Z

I think at the moment go.mod/go.sum/vendor aren't in sync.
This can be resolved by go mod tidy/vendor.
However once done it will uncover a more serious issue which I've also encountered in a separate PR.

slintes · 2023-07-03T07:30:49Z

controllers/fenceagentsremediation_controller_test.go

+					By("having one control-plane-role taint")
+					taintedNode := &corev1.Node{}
+					Expect(k8sClient.Get(context.Background(), nodeKey, taintedNode)).To(Succeed())
+					Expect(utils.TaintExists(taintedNode.Spec.Taints, &controlPlaneRoleTaint)).To(BeTrue())


this looks like a test for the utils package to me, so it should be there IMHO

Add UT to check whether append and remove taint functions append and remove properly taints while the other taints remain

Use common for k8s node labels

Add missing and remove unused modules, make vendored copy of dependencies, and verify dependencies have expected content

razo7 · 2023-07-03T14:29:45Z

pkg/utils/nodes.go

+// used for making new node object for test and have a unique resourceVersion
+// GetNode returns a node object with the name nodeName based on the nodeType input
+func GetNode(nodeType, nodeName string) *corev1.Node {
+	if nodeType == "control-plane" {
+		return &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nodeName,
+			},
+			Spec: corev1.NodeSpec{
+				Taints: []corev1.Taint{
+					{
+						Key:    medik8sLabels.ControlPlaneRole,
+						Effect: corev1.TaintEffectNoExecute,
+					},
+				},
+			},
+		}
+	} else {
+		return &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nodeName,
+			},
+		}
+	}
+}


I think it makes more sense to move this function which is used for unit-testing to another file. Maybe under test?

Or even having two local function for getting the node object? One in fenceagentsremediation_controller_test.go , and one in utils_test.go

Good point. I would put it into pkg/utils/nodes_test.go

But then controllers/fenceagentsremediation_controller_test.go could not find the GetNode function from a test code pkg/utils/nodes_test.go.

ah crap, I always forget about this limitation...
then I would just leave it here 🤷🏼‍♂️

razo7 · 2023-07-03T15:12:29Z

/test 4.13-openshift-e2e

slintes · 2023-07-04T07:43:13Z

pkg/utils/utils_suite_test.go

@@ -0,0 +1,99 @@
+/*


filename: you can skip "utils", it's redundant (there should be one suite per package)

slintes · 2023-07-04T07:44:52Z

pkg/utils/utils_test.go

@@ -0,0 +1,78 @@
+package utils


nit: I would name this file nodes_test.go, so you can have separate test files when you add new util files later on.

But nodes_test.go wouldn't be interfered as the test file for nodes.go, while this file shares a test for the taint file as well? ATM it has tests for two utils files, taint.go, and nodes.go
Going with your direction maybe it would make more sense to move Context("Taint functioninality test",... logic to taint_test.go, and have nodes_test.go for the other tests that utils.go has.
WDYT?

yes, ideally every xyz.go file should have its own xyz_test.go file. But I won't block on this, for these few methods / tests

Sounds good to me :)

slintes · 2023-07-04T07:46:41Z

pkg/utils/nodes.go

+// used for making new node object for test and have a unique resourceVersion
+// GetNode returns a node object with the name nodeName based on the nodeType input
+func GetNode(nodeType, nodeName string) *corev1.Node {
+	if nodeType == "control-plane" {
+		return &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nodeName,
+			},
+			Spec: corev1.NodeSpec{
+				Taints: []corev1.Taint{
+					{
+						Key:    medik8sLabels.ControlPlaneRole,
+						Effect: corev1.TaintEffectNoExecute,
+					},
+				},
+			},
+		}
+	} else {
+		return &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nodeName,
+			},
+		}
+	}
+}


Good point. I would put it into pkg/utils/nodes_test.go

mshitrit · 2023-07-04T08:18:01Z

controllers/fenceagentsremediation_controller_test.go

@@ -132,7 +114,7 @@ var _ = Describe("FAR Controller", func() {

 		When("creating valid FAR CR", func() {
 			BeforeEach(func() {
-				node = getNode(node01)
+				node = utils.GetNode("", node01)


IIUC node is always initialized to the same value.
If this is indeed the case I suggest:

initialize node value on var declaration

move the node creation to a share BeforeEach block

IIUC node is always initialized to the same value.

Correct, but if it is initialized once then the second test would fail as the instance might be deleted or already created while the second test tries to create it (an error due race condition). To overcome it we initialize an independent instance of the node (same spec and name, but different resource version) in a different BeforeEach block.

mshitrit · 2023-07-04T08:24:31Z

pkg/utils/utils_test.go

+	Context("FAR CR and Node Names Validity test", func() {
+		BeforeEach(func() {
+			node = GetNode("", node01)
+			DeferCleanup(k8sClient.Delete, context.Background(), node)


Nit: defer should be after the Create

mshitrit · 2023-07-04T08:35:19Z

pkg/utils/utils_test.go

+				By("having one control-plane-role taint")
+				taintedNode := &corev1.Node{}
+				Expect(k8sClient.Get(context.Background(), nodeKey, taintedNode)).To(Succeed())
+				Expect(TaintExists(taintedNode.Spec.Taints, &controlPlaneRoleTaint)).To(BeTrue())


IMO this is redundant since we added this taint in the test code

These test are unrelated to controller package, thus they should reside under utils directory

razo7 · 2023-07-04T08:56:04Z

/test 4.13-openshift-e2e

Every xyz.go file should have its own xyz_test.go file, and not a general one as utils_test.go

razo7 · 2023-07-05T07:33:44Z

/test 4.13-openshift-e2e

razo7 · 2023-07-05T08:44:46Z

/retry

razo7 · 2023-07-05T08:44:56Z

/retest

razo7 · 2023-07-05T08:46:50Z

I didn't though that /retest will trigger all the jobs, since the PR is still considered as draft... But now I know :D

razo7 · 2023-07-05T10:30:16Z

/retest

slintes

two nits, other than that lgtm

slintes · 2023-07-05T19:11:07Z

pkg/utils/nodes.go

@@ -33,3 +34,29 @@ func IsNodeNameValid(r client.Reader, nodeName string) (bool, error) {
 	}
 	return true, nil
 }
+
+// used for making new node object for test and have a unique resourceVersion


comments should start with the function name, in the 1st line of the comment 😉

slintes · 2023-07-05T19:11:41Z

pkg/utils/nodes.go

+
+// used for making new node object for test and have a unique resourceVersion
+// GetNode returns a node object with the name nodeName based on the nodeType input
+func GetNode(nodeType, nodeName string) *corev1.Node {


IMHO nodeRole wold be a better name than nodeType?

Var name and function declaration should begin in the first line of comments

razo7 · 2023-07-06T11:55:35Z

/test 4.13-openshift-e2e

openshift-ci · 2023-07-06T13:17:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7, slintes

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~~ [razo7,slintes]

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

openshift-ci bot added the do-not-merge/work-in-progress label Jun 29, 2023

openshift-ci bot added the approved label Jun 29, 2023

slintes reviewed Jul 3, 2023

View reviewed changes

razo7 added 3 commits July 3, 2023 16:43

Test taint functionailty on control-plane node

95ebbb9

Add UT to check whether append and remove taint functions append and remove properly taints while the other taints remain

Use worker label from Commons

02f6fb1

Use common for k8s node labels

Run make go-verify

3e6f458

Add missing and remove unused modules, make vendored copy of dependencies, and verify dependencies have expected content

razo7 force-pushed the taint-ut branch from 8565d2f to 26e6a36 Compare July 3, 2023 14:22

razo7 changed the title ~~Unit Test Taint Functionality~~ Validate utils Package and Taint Functionality Jul 3, 2023

razo7 commented Jul 3, 2023

View reviewed changes

razo7 force-pushed the taint-ut branch from 26e6a36 to 4f22495 Compare July 3, 2023 14:33

slintes reviewed Jul 4, 2023

View reviewed changes

mshitrit reviewed Jul 4, 2023

View reviewed changes

Validate utils functions with UT under utils directory

6ac73e7

These test are unrelated to controller package, thus they should reside under utils directory

razo7 force-pushed the taint-ut branch from 4f22495 to 6ac73e7 Compare July 4, 2023 08:51

Separate unit-test in utils package

3fa86ca

Every xyz.go file should have its own xyz_test.go file, and not a general one as utils_test.go

slintes reviewed Jul 5, 2023

View reviewed changes

Typo fix

3ee5066

Var name and function declaration should begin in the first line of comments

slintes approved these changes Jul 6, 2023

View reviewed changes

openshift-ci bot assigned slintes Jul 6, 2023

openshift-ci bot added the lgtm label Jul 6, 2023

razo7 marked this pull request as ready for review July 6, 2023 13:19

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 6, 2023

openshift-ci bot requested review from mshitrit and slintes July 6, 2023 13:19

openshift-merge-robot merged commit a50d9f6 into medik8s:main Jul 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate utils Package and Taint Functionality #61

Validate utils Package and Taint Functionality #61

razo7 commented Jun 29, 2023 •

edited

Loading

openshift-ci bot commented Jun 29, 2023

razo7 commented Jun 29, 2023

mshitrit commented Jul 2, 2023

slintes Jul 3, 2023

razo7 Jul 3, 2023

razo7 Jul 3, 2023

slintes Jul 4, 2023

razo7 Jul 4, 2023

slintes Jul 4, 2023

razo7 commented Jul 3, 2023

slintes Jul 4, 2023

slintes Jul 4, 2023

razo7 Jul 4, 2023

slintes Jul 4, 2023

razo7 Jul 5, 2023

slintes Jul 4, 2023

mshitrit Jul 4, 2023

razo7 Jul 4, 2023

mshitrit Jul 4, 2023

mshitrit Jul 4, 2023

razo7 commented Jul 4, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

slintes left a comment

slintes Jul 5, 2023

slintes Jul 5, 2023

razo7 Jul 6, 2023

razo7 commented Jul 6, 2023

openshift-ci bot commented Jul 6, 2023

Validate utils Package and Taint Functionality #61

Validate utils Package and Taint Functionality #61

Conversation

razo7 commented Jun 29, 2023 • edited Loading

openshift-ci bot commented Jun 29, 2023

razo7 commented Jun 29, 2023

mshitrit commented Jul 2, 2023

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

razo7 commented Jul 3, 2023

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

razo7 commented Jul 4, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

razo7 commented Jul 5, 2023

slintes 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

razo7 commented Jul 6, 2023

openshift-ci bot commented Jul 6, 2023

razo7 commented Jun 29, 2023 •

edited

Loading