Randomize Unit Tests #108

razo7 · 2023-11-26T04:32:17Z

Randomize unit tests
Ensure unit tests are independent for randomly running the specs

openshift-ci · 2023-11-26T04:32:22Z

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

mshitrit · 2023-11-26T08:26:25Z

/test ?

openshift-ci · 2023-11-26T08:26:29Z

@mshitrit: The following commands are available to trigger required jobs:

/test 4.12-ci-bundle-my-bundle
/test 4.12-images
/test 4.12-openshift-e2e
/test 4.12-test
/test 4.13-ci-bundle-my-bundle
/test 4.13-images
/test 4.13-openshift-e2e
/test 4.13-test
/test 4.14-ci-bundle-my-bundle
/test 4.14-images
/test 4.14-openshift-e2e
/test 4.14-test
/test 4.15-ci-bundle-my-bundle
/test 4.15-images
/test 4.15-openshift-e2e
/test 4.15-test

Use /test all to run all jobs.

In response to this:

/test ?

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/test-infra repository.

mshitrit · 2023-11-26T08:26:55Z

/test 4.12-openshift-e2e

clobrano · 2023-11-29T11:00:48Z

controllers/fenceagentsremediation_controller_test.go

@@ -285,7 +286,7 @@ func cleanupTestedResources(va1, va2 *storagev1.VolumeAttachment, pod *corev1.Po
 	podTest := &corev1.Pod{}
 	if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(pod), podTest); err == nil {
 		log.Info("Cleanup: clean pod", "pod name", podTest.Name)
-		Expect(k8sClient.Delete(context.Background(), podTest)).To(Succeed())
+		Expect(k8sClient.Delete(context.Background(), podTest, forced)).To(Succeed())


Would you do the same for "test-pod" in e2e tests?

Yes, and thanks for pointing this out.
But I prefer to do that in a follow-up PR as this one is more about running unit tests in a randomized way.

So, maybe doing both in the next PR? Using this one only for randomization?

SGTM :D #110

slintes · 2023-12-07T14:22:51Z

controllers/fenceagentsremediation_controller_test.go

@@ -312,22 +315,21 @@ func cliCommandsEquality(far *v1alpha1.FenceAgentsRemediation) (bool, error) {

 // testPodDeletion tests whether the pod no longer exist for successful FAR CR
 // and consistently check if the pod exist and was not deleted
-func testPodDeletion(podName string, resourceDeletionWasTriggered bool) {
+func testPodDeletion(podName string, wasDeleted bool) {


The func name with the wasDeleted arg is confusing. I'd prefer 2 functions for clarity. E.g. verifyPodExists and verifyPodDeleted

In my other PR, I opted for checkPodIsNotFound(podname, boolean).
I think this might improve readability

checkPodIsNotFound somePod true, if pod was deleted

checkPodIsNotFound somePod false, if pod was not deleted

wdyt?

I think Marc's suggestion for two different functions seems more readable

eventually, the two functions can simply call this one, since the code is basically the same

func verifyPodDeleted(podname string) { checkPodIsNotFound(podname, true) } func checkPodIsNotFound(podName string, expected bool) { podKey := client.ObjectKey{ Namespace: defaultNamespace, Name: podName, } ConsistentlyWithOffset(1, func() bool { pod := &corev1.Pod{} err := k8sClient.Get(context.Background(), podKey, pod) return apierrors.IsNotFound(err) }, timeoutDeletion, pollInterval).Should(Equal(expected)) }

the same

They are close to being the same. One function uses ConsistentlyWithOffset and the other uses EventuallyWithOffset. ~~But they can both use ConsistentlyWithOffset. SGTM~~

Edit: I will stick with the current functions.

oh, I missed that. Maybe that's why I got that problem in my e2e 😅

slintes · 2023-12-07T15:17:46Z

lgtm

but did we actually agree that we want to have this? I remember some discussion in chat... 🤔

razo7 · 2023-12-07T15:38:49Z

but did we actually agree that we want to have this? I remember some discussion in chat... 🤔

IIRC we agreed not to do a repetition, but the concept of randomization was accepted.

clobrano · 2023-12-11T16:44:16Z

controllers/fenceagentsremediation_controller_test.go

 			})
 			It("should have finalizer, taint, while the two VAs and one pod will be deleted", func() {
 				By("Searching for remediation taint")
+				nodeKey.Name = workerNode


Modifying variables on the fly is confusing when reading the test
Why not using directly the client.ObjectKey?

client.ObjectKey{Name: workerNode, Namespace: defaultNamespace}

I have no strong preference, I thought it looked easier this way, but I will change it.

Randomize all the specs per suite to test them in diffetnet orders of specs.

Needed for randomly running the specs. Set nodeKey and farNamespacedName using client.ObjectKey for better understanding the tests

Split testPodDeletion into two functions - verifyPodExists and verifyPodDeleted

openshift-ci · 2023-12-12T08:29:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

razo7 · 2023-12-12T10:31:22Z

/retest

razo7 · 2023-12-12T11:41:30Z

/retest

openshift-ci bot added do-not-merge/work-in-progress approved labels Nov 26, 2023

razo7 force-pushed the randomize-ut branch from 8b5cbc5 to 892b97b Compare November 26, 2023 04:35

razo7 marked this pull request as ready for review November 29, 2023 00:29

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

openshift-ci bot requested review from beekhof and mshitrit November 29, 2023 00:29

razo7 changed the title ~~Randomize Unit Tests and Run them Twice~~ Randomize Unit Tests Nov 29, 2023

clobrano reviewed Nov 29, 2023

View reviewed changes

slintes requested changes Dec 7, 2023

View reviewed changes

openshift-ci bot assigned slintes Dec 7, 2023

razo7 force-pushed the randomize-ut branch from 1736460 to beee785 Compare December 11, 2023 15:17

clobrano reviewed Dec 11, 2023

View reviewed changes

razo7 added 3 commits December 12, 2023 10:07

Randomize unit tests

1e0562f

Randomize all the specs per suite to test them in diffetnet orders of specs.

Ensure unit tests are independent

e81ac65

Needed for randomly running the specs. Set nodeKey and farNamespacedName using client.ObjectKey for better understanding the tests

Simplifies testing pod deletion and existance

93c1272

Split testPodDeletion into two functions - verifyPodExists and verifyPodDeleted

razo7 force-pushed the randomize-ut branch from beee785 to 93c1272 Compare December 12, 2023 08:16

clobrano approved these changes Dec 12, 2023

View reviewed changes

openshift-ci bot assigned clobrano Dec 12, 2023

openshift-ci bot added the lgtm label Dec 12, 2023

openshift-merge-bot bot merged commit b404e39 into medik8s:main Dec 12, 2023
18 checks passed

clobrano mentioned this pull request Dec 17, 2023

Shorten Timeouts for E2e Tests #113

Merged

razo7 deleted the randomize-ut branch January 7, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Randomize Unit Tests #108

Randomize Unit Tests #108

razo7 commented Nov 26, 2023 •

edited

Loading

openshift-ci bot commented Nov 26, 2023

mshitrit commented Nov 26, 2023

openshift-ci bot commented Nov 26, 2023

mshitrit commented Nov 26, 2023

clobrano Nov 29, 2023

razo7 Dec 7, 2023

clobrano Dec 11, 2023

razo7 Dec 11, 2023

slintes Dec 7, 2023

clobrano Dec 11, 2023

razo7 Dec 11, 2023

clobrano Dec 11, 2023 •

edited

Loading

razo7 Dec 11, 2023 •

edited

Loading

clobrano Dec 11, 2023

slintes commented Dec 7, 2023

razo7 commented Dec 7, 2023

clobrano Dec 11, 2023

razo7 Dec 12, 2023

openshift-ci bot commented Dec 12, 2023

razo7 commented Dec 12, 2023

razo7 commented Dec 12, 2023

Randomize Unit Tests #108

Randomize Unit Tests #108

Conversation

razo7 commented Nov 26, 2023 • edited Loading

openshift-ci bot commented Nov 26, 2023

mshitrit commented Nov 26, 2023

openshift-ci bot commented Nov 26, 2023

mshitrit commented Nov 26, 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

clobrano Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

razo7 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slintes commented Dec 7, 2023

razo7 commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 12, 2023

razo7 commented Dec 12, 2023

razo7 commented Dec 12, 2023

razo7 commented Nov 26, 2023 •

edited

Loading

clobrano Dec 11, 2023 •

edited

Loading

razo7 Dec 11, 2023 •

edited

Loading