Improving Unit Test Logic and Time #59
Conversation
razo7
commented
Jun 21, 2023
razo7
commented
- Separate logical unit-test and order- Test taint and finalizer existence in eventually following an expect rather than one eventually with and a logical OR/AND of there existence. This way we better check the order of their creation. Related to older comment.
- Reduce eventually timeout and polling interval- the values were too high
Test taint and finalizer existence in eventually following an expect rather than one eventually with and a logical OR/AND of there existance. This way we better check the order of their creation
The values were too high
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
| @@ -133,29 +133,37 @@ var _ = Describe("FAR Controller", func() { | |||
| It("should have finalizer and taint", func() { | |||
| farNamespacedName := client.ObjectKey{Name: node01, Namespace: defaultNamespace} | |||
| nodeNamespacedName := client.ObjectKey{Name: node01} | |||
There was a problem hiding this comment.
This is a bit confusing, since namespace isn't relevant in the context of nodes
There was a problem hiding this comment.
I usually use something like nodeKey: it represents the type (ObjectKey) and it doesn't matter if it has a namespace or not 🙂
| @@ -133,29 +133,37 @@ var _ = Describe("FAR Controller", func() { | |||
| It("should have finalizer and taint", func() { | |||
| farNamespacedName := client.ObjectKey{Name: node01, Namespace: defaultNamespace} | |||
| nodeNamespacedName := client.ObjectKey{Name: node01} | |||
| By("Searching for remediation taint") | |||
| Eventually(func() bool { | |||
| Expect(k8sClient.Get(context.Background(), nodeNamespacedName, node)).To(Succeed()) | |||
There was a problem hiding this comment.
I think that checking for the node existence, which is created in the test code (and not in production code) is redundant.
IMO it's roughly the equivalent of:
It("some test", func() {
someVar := true
Expect(someVar).To(BeTrue())
})
There was a problem hiding this comment.
On the same principle I can ignore checking the CR creation, right?
There was a problem hiding this comment.
Even though that I don't agree that I can ignore this line, because without this k8sClient.Get call node variable won't be updated with the node from reconcile (and with taints that we check on line 141)...
There was a problem hiding this comment.
Good point, definitely need to check the taint on the node.
| return controllerutil.ContainsFinalizer(underTestFAR, v1alpha1.FARFinalizer) && utils.TaintExists(node.Spec.Taints, &farNoExecuteTaint) && res | ||
| }, 1*time.Second, 500*time.Millisecond).Should(BeTrue(), "finalizer and taint should be added, and command format is correct") | ||
| return utils.TaintExists(node.Spec.Taints, &farNoExecuteTaint) && res | ||
| }, 20*time.Millisecond, 10*time.Millisecond).Should(BeTrue(), "taint should be added, and command format is correct") |
There was a problem hiding this comment.
Nit: retry of every 10ms over a period of 20ms seems like cutting it really close, which in part defeating the use of Eventually, maybe use a longer period ? (first param)
Improve code's clarity
|
/lgtm |
