Skip to content

Commit

Permalink
Simplify error check in tests
Browse files Browse the repository at this point in the history 
Replace this patter:
```golang
err := someFuncRetOnlyErr()
Expect(err).ToNot(HaveOccurred()
```
With
```golang
Expect(someFuncRetOnlyErr()).To(Succeed())
```

Also, use the `MatchError` gomega matcher when checking errors.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
  • Loading branch information
@nunnatsa
nunnatsa committed Jan 10, 2024
1 parent f93ce10 commit b85b9e7
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 147 deletions.
35 changes: 12 additions & 23 deletions conditions/conditions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,21 @@ var _ = Describe("Condition", func() {
}

// Create Operator Condition
err := os.Setenv(operatorCondEnvVar, "operator-condition-test")
Expect(err).NotTo(HaveOccurred())
Expect(os.Setenv(operatorCondEnvVar, "operator-condition-test")).To(Succeed())
readNamespace = func() (string, error) {
return ns, nil
}

// create a new client
sch := runtime.NewScheme()
err = apiv2.AddToScheme(sch)
Expect(err).NotTo(HaveOccurred())
Expect(apiv2.AddToScheme(sch)).To(Succeed())
cl = fake.NewClientBuilder().WithScheme(sch).WithStatusSubresource(operatorCond).Build()

// create an operator Condition resource
err = cl.Create(ctx, operatorCond)
Expect(err).NotTo(HaveOccurred())
Expect(cl.Create(ctx, operatorCond)).To(Succeed())

// Update its status
err = cl.Status().Update(ctx, operatorCond)
Expect(err).NotTo(HaveOccurred())
Expect(cl.Status().Update(ctx, operatorCond)).To(Succeed())
})

AfterEach(func() {
Expand All @@ -107,14 +103,12 @@ var _ = Describe("Condition", func() {
Expect(err).NotTo(HaveOccurred())

con, err := c.Get(ctx)
Expect(err).To(HaveOccurred())
Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("conditionType %v not found", conditionBar))))
Expect(con).To(BeNil())
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("conditionType %v not found", conditionBar)))
})

It("should error when operator Condition is not present in cluster", func() {
err := os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")
Expect(err).NotTo(HaveOccurred())
Expect(os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")).To(Succeed())

By("setting the status of a new condition")
c, err := NewCondition(cl, conditionFoo)
Expand All @@ -131,13 +125,11 @@ var _ = Describe("Condition", func() {
By("setting the status of an existing condition")
c, err := NewCondition(cl, conditionFoo)
Expect(err).NotTo(HaveOccurred())
err = c.Set(ctx, metav1.ConditionFalse, WithReason("not_in_foo_state"), WithMessage("test"))
Expect(err).NotTo(HaveOccurred())
Expect(c.Set(ctx, metav1.ConditionFalse, WithReason("not_in_foo_state"), WithMessage("test"))).To(Succeed())

By("fetching the condition from cluster")
op := &apiv2.OperatorCondition{}
err = cl.Get(ctx, objKey, op)
Expect(err).NotTo(HaveOccurred())
Expect(cl.Get(ctx, objKey, op)).To(Succeed())

By("checking if the condition has been updated")
res := op.Spec.Conditions[0]
Expand All @@ -150,22 +142,19 @@ var _ = Describe("Condition", func() {
By("setting the status of a new condition")
c, err := NewCondition(cl, conditionBar)
Expect(err).NotTo(HaveOccurred())
err = c.Set(ctx, metav1.ConditionTrue, WithReason("in_bar_state"), WithMessage("test"))
Expect(err).NotTo(HaveOccurred())
Expect(c.Set(ctx, metav1.ConditionTrue, WithReason("in_bar_state"), WithMessage("test"))).To(Succeed())

By("fetching the condition from cluster")
op := &apiv2.OperatorCondition{}
err = cl.Get(ctx, objKey, op)
Expect(err).NotTo(HaveOccurred())
Expect(cl.Get(ctx, objKey, op)).To(Succeed())

By("checking if the condition has been updated")
res := op.Spec.Conditions
Expect(len(res)).To(BeEquivalentTo(2))
Expect(res).To(HaveLen(2))
Expect(meta.IsStatusConditionTrue(res, string(conditionBar))).To(BeTrue())
})
It("should error when operator Condition is not present in cluster", func() {
err := os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")
Expect(err).NotTo(HaveOccurred())
Expect(os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")).To(Succeed())

By("setting the status of a new condition")
c, err := NewCondition(cl, conditionBar)
Expand Down
30 changes: 10 additions & 20 deletions conditions/factory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ var _ = Describe("NewCondition", func() {
var cl client.Client
BeforeEach(func() {
sch := runtime.NewScheme()
err := apiv2.AddToScheme(sch)
Expect(err).NotTo(HaveOccurred())
Expect(apiv2.AddToScheme(sch)).To(Succeed())
cl = fake.NewClientBuilder().WithScheme(sch).Build()
})

Expand All @@ -56,8 +55,7 @@ var _ = Describe("InClusterFactory", func() {

BeforeEach(func() {
sch := runtime.NewScheme()
err := apiv2.AddToScheme(sch)
Expect(err).NotTo(HaveOccurred())
Expect(apiv2.AddToScheme(sch)).To(Succeed())
cl = fake.NewClientBuilder().WithScheme(sch).Build()
f = InClusterFactory{cl}
})
Expand All @@ -73,8 +71,7 @@ var _ = Describe("InClusterFactory", func() {

func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) {
It("should create a new condition", func() {
err := os.Setenv(operatorCondEnvVar, "test-operator-condition")
Expect(err).NotTo(HaveOccurred())
Expect(os.Setenv(operatorCondEnvVar, "test-operator-condition")).To(Succeed())
readNamespace = func() (string, error) {
return "default", nil
}
Expand All @@ -85,8 +82,7 @@ func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) {
})

It("should error when namespacedName cannot be found", func() {
err := os.Unsetenv(operatorCondEnvVar)
Expect(err).NotTo(HaveOccurred())
Expect(os.Unsetenv(operatorCondEnvVar)).To(Succeed())

c, err := fn(conditionFoo)
Expect(err).To(HaveOccurred())
Expand All @@ -96,32 +92,27 @@ func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) {

func testGetNamespacedName(fn func() (*types.NamespacedName, error)) {
It("should error when name of the operator condition cannot be found", func() {
err := os.Unsetenv(operatorCondEnvVar)
Expect(err).NotTo(HaveOccurred())
Expect(os.Unsetenv(operatorCondEnvVar)).To(Succeed())

objKey, err := fn()
Expect(err).To(HaveOccurred())
Expect(err).To(MatchError(ContainSubstring("could not determine operator condition name")))
Expect(objKey).To(BeNil())
Expect(err.Error()).To(ContainSubstring("could not determine operator condition name"))
})

It("should error when object namespace cannot be found", func() {
err := os.Setenv(operatorCondEnvVar, "test")
Expect(err).NotTo(HaveOccurred())
Expect(os.Setenv(operatorCondEnvVar, "test")).To(Succeed())

readNamespace = func() (string, error) {
return "", os.ErrNotExist
}

objKey, err := fn()
Expect(err).To(HaveOccurred())
Expect(err).To(MatchError(ContainSubstring("get operator condition namespace: file does not exist")))
Expect(objKey).To(BeNil())
Expect(err.Error()).To(ContainSubstring("get operator condition namespace: file does not exist"))
})

It("should return the right namespaced name from SA namespace file", func() {
err := os.Setenv(operatorCondEnvVar, "test")
Expect(err).NotTo(HaveOccurred())
Expect(os.Setenv(operatorCondEnvVar, "test")).To(Succeed())

readNamespace = func() (string, error) {
return "testns", nil
Expand All @@ -135,6 +126,5 @@ func testGetNamespacedName(fn func() (*types.NamespacedName, error)) {
}

func deleteCondition(ctx context.Context, client client.Client, obj client.Object) {
err := client.Delete(ctx, obj)
Expect(err).NotTo(HaveOccurred())
Expect(client.Delete(ctx, obj)).To(Succeed())
}
24 changes: 8 additions & 16 deletions handler/enqueue_annotation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {

podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"})

err := SetOwnerAnnotations(podOwner, pod)
Expect(err).ToNot(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed())
instance = EnqueueRequestForAnnotation{
Type: schema.GroupKind{
Group: "",
Expand Down Expand Up @@ -92,8 +91,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
},
}

err := SetOwnerAnnotations(podOwner, repl)
Expect(err).ToNot(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner, repl)).To(Succeed())

evt := event.CreateEvent{
Object: repl,
Expand Down Expand Up @@ -248,8 +246,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
newPod.Name = pod.Name + "2"
newPod.Namespace = pod.Namespace + "2"

err := SetOwnerAnnotations(podOwner, pod)
Expect(err).ToNot(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed())

evt := event.UpdateEvent{
ObjectOld: pod,
Expand Down Expand Up @@ -337,8 +334,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
newPod.Name = pod.Name + "2"
newPod.Namespace = pod.Namespace + "2"

err := SetOwnerAnnotations(podOwner, pod)
Expect(err).ToNot(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed())

var podOwner2 = &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -348,8 +344,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
}
podOwner2.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"})

err = SetOwnerAnnotations(podOwner2, newPod)
Expect(err).ToNot(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner2, newPod)).To(Succeed())

evt := event.UpdateEvent{
ObjectOld: pod,
Expand Down Expand Up @@ -389,8 +384,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
},
}

err := SetOwnerAnnotations(podOwner, nd)
Expect(err).ToNot(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner, nd)).To(Succeed())

expected := map[string]string{
"my-test-annotation": "should-keep",
Expand All @@ -409,8 +403,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
}

podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""})
err := SetOwnerAnnotations(podOwner, nd)
Expect(err).To(HaveOccurred())
Expect(SetOwnerAnnotations(podOwner, nd)).ToNot(Succeed())
})
It("should return an error when the owner Name is not set", func() {
nd := &corev1.Node{
Expand All @@ -426,8 +419,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
}

ownerNew.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""})
err := SetOwnerAnnotations(ownerNew, nd)
Expect(err).To(HaveOccurred())
Expect(SetOwnerAnnotations(ownerNew, nd)).ToNot(Succeed())
})
})
})
32 changes: 10 additions & 22 deletions leader/leader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package leader

import (
"context"
"errors"
"os"

. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -65,27 +64,23 @@ var _ = Describe("Leader election", func() {
})
It("should return an error when POD_NAME is not set", func() {
os.Unsetenv("POD_NAME")
err := Become(context.TODO(), "leader-test")
Expect(err).Should(HaveOccurred())
Expect(Become(context.TODO(), "leader-test")).ShouldNot(Succeed())
})
It("should return an ErrNoNamespace", func() {
os.Setenv("POD_NAME", "leader-test")
readNamespace = func() (string, error) {
return "", ErrNoNamespace
}
err := Become(context.TODO(), "leader-test", WithClient(client))
Expect(err).Should(HaveOccurred())
Expect(err).To(Equal(ErrNoNamespace))
Expect(errors.Is(err, ErrNoNamespace)).To(BeTrue())
Expect(err).Should(MatchError(ErrNoNamespace))
})
It("should not return an error", func() {
os.Setenv("POD_NAME", "leader-test")
readNamespace = func() (string, error) {
return "testns", nil
}

err := Become(context.TODO(), "leader-test", WithClient(client))
Expect(err).ShouldNot(HaveOccurred())
Expect(Become(context.TODO(), "leader-test", WithClient(client))).To(Succeed())
})
})
Describe("isPodEvicted", func() {
Expand Down Expand Up @@ -195,13 +190,11 @@ var _ = Describe("Leader election", func() {
})
It("should return an error if no node is found", func() {
node := corev1.Node{}
err := getNode(context.TODO(), client, "", &node)
Expect(err).Should(HaveOccurred())
Expect(getNode(context.TODO(), client, "", &node)).ToNot(Succeed())
})
It("should return the node with the given name", func() {
node := corev1.Node{}
err := getNode(context.TODO(), client, "mynode", &node)
Expect(err).ShouldNot(HaveOccurred())
Expect(getNode(context.TODO(), client, "mynode", &node)).Should(Succeed())
Expect(node.TypeMeta.APIVersion).To(Equal("v1"))
Expect(node.TypeMeta.Kind).To(Equal("Node"))
})
Expand Down Expand Up @@ -293,28 +286,23 @@ var _ = Describe("Leader election", func() {
})
It("should return an error if existing is not found", func() {
client = fake.NewClientBuilder().WithObjects(pod).Build()
err := deleteLeader(context.TODO(), client, pod, configmap)
Expect(err).Should(HaveOccurred())
Expect(deleteLeader(context.TODO(), client, pod, configmap)).ToNot(Succeed())
})
It("should return an error if pod is not found", func() {
client = fake.NewClientBuilder().WithObjects(configmap).Build()
err := deleteLeader(context.TODO(), client, pod, configmap)
Expect(err).Should(HaveOccurred())
Expect(deleteLeader(context.TODO(), client, pod, configmap)).ToNot(Succeed())
})
It("should return an error if pod is nil", func() {
client = fake.NewClientBuilder().WithObjects(pod, configmap).Build()
err := deleteLeader(context.TODO(), client, nil, configmap)
Expect(err).Should(HaveOccurred())
Expect(deleteLeader(context.TODO(), client, nil, configmap)).ToNot(Succeed())
})
It("should return an error if configmap is nil", func() {
client = fake.NewClientBuilder().WithObjects(pod, configmap).Build()
err := deleteLeader(context.TODO(), client, pod, nil)
Expect(err).Should(HaveOccurred())
Expect(deleteLeader(context.TODO(), client, pod, nil)).ToNot(Succeed())
})
It("should return nil if pod and configmap exists and configmap's owner is the pod", func() {
client = fake.NewClientBuilder().WithObjects(pod, configmap).Build()
err := deleteLeader(context.TODO(), client, pod, configmap)
Expect(err).ShouldNot(HaveOccurred())
Expect(deleteLeader(context.TODO(), client, pod, configmap)).To(Succeed())
})

})
Expand Down

0 comments on commit b85b9e7

Please sign in to comment.