From 6d5a1629f032ea412d09247987941ea7bd8dcafa Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Thu, 20 May 2021 22:58:38 -0400 Subject: [PATCH] Address review feedback #2 Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer_test.go | 41 ++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/pkg/finalizer/finalizer_test.go b/pkg/finalizer/finalizer_test.go index f809f52028..d6e49453d0 100644 --- a/pkg/finalizer/finalizer_test.go +++ b/pkg/finalizer/finalizer_test.go @@ -34,18 +34,10 @@ var _ = Describe("TestFinalizer", func() { var f mockFinalizer BeforeEach(func() { pod = &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{"finalizers.sigs.k8s.io/testfinalizer"}, - }, + ObjectMeta: metav1.ObjectMeta{}, } - Expect(pod).NotTo(BeNil()) - finalizers = NewFinalizers() - Expect(finalizers).NotTo(BeNil()) - - f := mockFinalizer{} - Expect(f).NotTo(BeNil()) - + f = mockFinalizer{} }) Describe("Register", func() { It("successfully registers a finalizer", func() { @@ -65,12 +57,6 @@ var _ = Describe("TestFinalizer", func() { }) }) Describe("Finalize", func() { - It("should return no error and return false for needsUpdate if a finalizer is not registered", func() { - ret, err := finalizers.Finalize(context.TODO(), pod) - Expect(err).To(BeNil()) - Expect(ret).To(BeFalse()) - }) - It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() { err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) @@ -81,6 +67,10 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) + // when deletion timestamp is nil and finalizer is not present, the registered finalizer would be added to the obj + Expect(len(pod.Finalizers)).To(Equal(1)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer")) + }) It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() { @@ -95,6 +85,8 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) + // finalizer will be removed from the obj upon successful finalization + Expect(len(pod.Finalizers)).To(Equal(0)) }) It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() { @@ -104,6 +96,8 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(0)) + }) It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() { @@ -114,6 +108,7 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(0)) }) @@ -132,6 +127,7 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) + Expect(len(pod.Finalizers)).To(Equal(0)) }) It("should return needsUpdate as false and a non-nil error", func() { @@ -149,6 +145,8 @@ var _ = Describe("TestFinalizer", func() { Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) Expect(needsUpdate).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(1)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer")) }) It("should return expected needsUpdate and error values when registering multiple finalizers", func() { @@ -170,6 +168,11 @@ var _ = Describe("TestFinalizer", func() { result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(result).To(BeTrue()) + // `finalizers.sigs.k8s.io/testfinalizer1` will be removed from the list + // of finalizers, so length will be 2. + Expect(len(pod.Finalizers)).To(Equal(2)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) + Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) // test for needsUpdate as false, and non-nil error f.needsUpdate = false @@ -181,6 +184,9 @@ var _ = Describe("TestFinalizer", func() { Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) Expect(result).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(2)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) + Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) // test for needsUpdate as true, and non-nil error f.needsUpdate = true @@ -192,6 +198,9 @@ var _ = Describe("TestFinalizer", func() { Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) Expect(result).To(BeTrue()) + Expect(len(pod.Finalizers)).To(Equal(2)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) + Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) }) }) })