Test subnet controller #177

LimKianAn · 2021-10-21T14:19:33Z

Closes #176

LimKianAn · 2021-10-22T18:55:36Z

controllers/network/suite_test.go

GrigoriyMikhalkin · 2021-10-22T20:30:53Z

controllers/network/suite_test.go

+	RegisterFailHandler(Fail)
+
+	RunSpecsWithDefaultAndCustomReporters(t,
+		"Controller Suite",


Change the name to "Network Controller Suite"? Because we have multiple subpackages in controllers package.

controllers/network/subnet_controller_test.go

GrigoriyMikhalkin · 2021-10-22T20:32:44Z

controllers/network/subnet_controller_test.go

+	"reflect"
+	"time"
+
+	"github.com/onmetal/onmetal-api/apis/common/v1alpha1"


Let's use goimports for the code with --local github.com/onmetal/onmetal-api flag. what you think?

Agreed. For VSCode users, add the following to settings.json.

"go.formatTool": "goimports", "gopls": { "formatting.local": "github.com/onmetal/onmetal-api", },

GrigoriyMikhalkin · 2021-10-22T20:33:28Z

controllers/network/subnet_controller_test.go

+)
+
+var _ = Describe("subnet controller", func() {
+	Context("reconcileExists", func() {


I would merge it with "Reconcile" context.

My idea was:
Context: function name
It: what the function does
By: steps in the function

Like here it's more like a function name to me.

Sill open to discussion.

The whole point of using Ginkgo is to implement behavior driven testing. So we don't need to use function names in organizational units(i.e. Context, It, etc). Instead we mention behavior. Like Context("Reconcile"), which, at least usually, is top level definition of testable behavior in controller tests.

P.S. I also, would say, that unit testing is undesirable, as theoretically it can be broken without breaking desired behavior. It's always preferred to test general behavior of the component)

Normally function names describe behaviors in short and people do use function names in Ginkgo's constructs, like here, here and here.

What do you mean by this?

it can be broken without breaking desired behavior

What do you mean by this?

it can be broken without breaking desired behavior

For example, some refactoring is done, part of functionality moved to another function called from calling function. Unit test will be broken, general behavior will not.

Point is, we test behavior not functions)

A function name should describe the behaviour in short. I don't see a conflict here. Yet, since we don't have finalizers here and don't have to do some clean-up after the deletion, the context reconcileExists is indeed too thin.

adracus

Some required changes. In general, try to avoid Ginkgo assertions that yield bool: If they fail, you'll get an error message like

expected 'true' to be 'false'

or else, which isn't very helpful. The richer Ginkgo assertions give back understandable messages which makes troubleshooting a lot easier. If you ever can't get around boolean assertions, make sure to include a hint in the matcher, like

Expect(myResult).To(BeTrue(), "the 'my-operation' operation did not succeed")

adracus · 2021-10-25T07:36:36Z

controllers/network/subnet_controller_test.go

+
+			Eventually(func() bool {
+				rngGot := &nw.IPAMRange{}
+				Expect(k8sClient.Get(ctx, objectKey(ipamRange), rngGot))


.To(Succeed()) assertion is missing

adracus · 2021-10-25T07:37:49Z

controllers/network/subnet_controller_test.go

+			Eventually(func() bool {
+				rngGot := &nw.IPAMRange{}
+				Expect(k8sClient.Get(ctx, objectKey(ipamRange), rngGot))
+				return rngGot.Spec.CIDRs == nil


I'd do

return rngGot.Spec.CIDRs

And then as assertion, instead of BeTrue BeEmpty or BeNil. In case the slice is not empty, there will be a better error message what the contained items are.

adracus · 2021-10-25T07:40:06Z

controllers/network/subnet_controller_test.go

+		It("finishes reconciliation early if the instance is being deleted", func() {
+			subnet := newSubnet("early-finished")
+			subnet.DeletionTimestamp = now()
+			ipamRange := newIPAMRange(subnet)


We should not create the IPAMRange ourselves and instead wait for the Subnet controller to create its IPAMRange object (it does so via server-side apply).

OK! I didn't get this bit and thought the IPAMRange is already generated and only get patched by Subnet controller. Thanks!

adracus · 2021-10-25T07:42:15Z

controllers/network/subnet_controller_test.go

+	Context("reconcileExists", func() {
+		It("finishes reconciliation early if the instance is being deleted", func() {
+			subnet := newSubnet("early-finished")
+			subnet.DeletionTimestamp = now()


Instead of creating it w/ a deletion timestamp set to now, we should create it, wait for the IPAMRange to be created and then delete it to see if it's gone afterwards (it should be gone due to us correctly setting up owner references).

Due to the limit of envtest only the owner reference of the owned IPAMRange is checked.

adracus · 2021-10-25T07:42:51Z

controllers/network/subnet_controller_test.go

+			subnet := newSubnet("no-parant")
+			ipamRange := newIPAMRange(subnet)
+
+			Expect(k8sClient.Create(ctx, ipamRange)).Should(Succeed())


We shouldn't create the IPAMRange manually and instead let the controller do this.

adracus · 2021-10-25T08:21:41Z

controllers/network/subnet_controller_test.go

+			Expect(k8sClient.Create(ctx, parent)).Should(Succeed())
+			Expect(k8sClient.Create(ctx, child)).Should(Succeed())
+
+			By("patching the spec of the owned IPAMRange")


By("waiting for the parent IPAMRange to have an allocation by the child IPAMRange")

adracus · 2021-10-25T08:24:02Z

controllers/network/subnet_controller_test.go

+				return func() bool {
+					parentRng := rngGot.Spec.Parent
+					parentSubnet := child.Spec.Parent
+					if parentRng == nil || parentSubnet == nil {
+						return false
+					}
+
+					// Check if the IPAMRange is patched
+					return parentRng.Name == nw.SubnetIPAMName(parentSubnet.Name) &&
+						rngGot.Spec.CIDRs == nil &&
+						reflect.DeepEqual(rngGot.Spec.Requests[0], nw.IPAMRangeRequest{CIDR: &child.Spec.Ranges[0].CIDR})
+
+				}()
+			}, timeout, interval).Should(BeTrue())


Instead of this block, just use

return rngGot.Spec.Requests }, timeout, interval).Should(ContainElement(child.Spec.Ranges[0].CIDR})

adracus · 2021-10-25T08:24:48Z

controllers/network/subnet_controller_test.go

+				}()
+			}, timeout, interval).Should(BeTrue())
+
+			By("patching the status of the Subnet")


By("waiting for the subnet to report status 'up'") + modify the assertion as described in the parent-less example.

adracus · 2021-10-25T08:25:21Z

controllers/network/suite_test.go

+func TestAPIs(t *testing.T) {
+	RegisterFailHandler(Fail)
+
+	RunSpecsWithDefaultAndCustomReporters(t,


Use RunSpecs and remove the custom reporter. That feature won't be available in the latest Ginkgo version anymore.

adracus · 2021-10-25T08:25:55Z

controllers/network/suite_test.go

+var _ = BeforeSuite(func() {
+	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
+
+	ctx, cancel = context.WithCancel(context.TODO())


context.TODO() -> context.Background()

GrigoriyMikhalkin · 2021-10-26T09:28:37Z

controllers/network/suite_test.go

+	}).SetupWithManager(k8sManager)
+	Expect(err).ToNot(HaveOccurred())
+
+	err = (&IPAMRangeReconciler{


Just a question not a call to action) Is IPAMRangeReconciler needed for testing SubnetReconciler?

At the moment, no, but eventually, yes. The reconciler of Subnet will check the status of the IPAMRange and update the Subnet accordingly. (here) There might be a bug here which I will address in another issue.

adracus

Some minor changes, otherwise looks pretty good!

adracus · 2021-10-26T15:07:22Z

controllers/network/subnet_controller_test.go

+
+			ipamRng := newIPAMRange(subnet)
+			Eventually(func() error {
+				return k8sClient.Get(ctx, objectKey(ipamRng), ipamRng)


Two problems here:

If the Get ever fails, the ipamRng object might be reset, deleting its metadata, resulting in the objectKey returning an empty client.ObjectKey, resulting in always getting Not Found. The key should be created outside the func().

On errors other than apierrors.IsNotFound, the test should error early, as that's a non-recoverable error.
Please make sure to check all Get + Eventually combinations.

adracus · 2021-10-26T15:09:04Z

controllers/network/subnet_controller_test.go

+			ipamRng := newIPAMRange(subnet)
+			Eventually(func() error {
+				return k8sClient.Get(ctx, objectKey(ipamRng), ipamRng)
+			}, timeout, interval).Should(BeNil())


BeNil() -> Succeed()

LimKianAn self-assigned this Oct 21, 2021

github-actions bot added size/L network labels Oct 21, 2021

LimKianAn changed the title ~~Set up suite~~ WIP Test subnet controller Oct 21, 2021

LimKianAn force-pushed the 176-test-subnet-controller branch from 21be879 to 73eca72 Compare October 21, 2021 14:25

Set up suite

06ac011

LimKianAn force-pushed the 176-test-subnet-controller branch from 73eca72 to 06ac011 Compare October 21, 2021 14:31

Test patching IPAMRange by Subnet reconciliation

dfb8d81

LimKianAn force-pushed the 176-test-subnet-controller branch from 1cdad42 to dfb8d81 Compare October 21, 2021 20:30

LimKianAn added 15 commits October 22, 2021 16:27

Test early finished reconciliation

9ba3018

Evaluate against the spec of IPAMRange instead

5ea4b1d

Expect no error, otherwiese the test fails

ba7033e

Better naming

2f36a6d

Create IPAMRange first, which is more realistic

9175554

Add the steps to pass the test

bc6bc67

Add expectation

b90ea31

Better the description of behaviors

b80a609

Better the name of the subnet

7a57d29

Add contents to the behavior

e5508f3

Order alphabetically

8c9af21

Comment not-exported const

aa683fc

Better comments

2870a93

Better naming

0aea737

Use the existing util func

5ad9e4c

LimKianAn requested a review from adracus October 22, 2021 18:54

LimKianAn changed the title ~~WIP Test subnet controller~~ Test subnet controller Oct 22, 2021

GrigoriyMikhalkin reviewed Oct 22, 2021

View reviewed changes

adracus suggested changes Oct 25, 2021

View reviewed changes

Use goimports with the flag local

e22aee8

LimKianAn added 13 commits October 25, 2021 10:57

Better the name of the suite

7f6f308

Add the expectation

8939067

Follow the convention of a pkg alias

1783e36

Test if the owner reference is set

cc5ba08

Fix the bug of creating IPAMRange redundantly

68822a8

Shorter the var name

198bd3d

Remove unused func

53948ac

Better descriptions

60d74fc

Use gomega Equal and better naming

26f4601

Use gomega Equal to simplify tests

ef6d465

Use gomega ContainElement

72ca1a2

Remove custom reporter

c5c4f3d

Use Backgound as top-level Context

ef3a0f7

LimKianAn requested a review from adracus October 26, 2021 09:15

GrigoriyMikhalkin reviewed Oct 26, 2021

View reviewed changes

adracus suggested changes Oct 26, 2021

View reviewed changes

LimKianAn added 4 commits October 27, 2021 10:36

Define object keys outside Eventually

28c4fbf

Replace BeNil with Succeed

0208758

Screen errors

ed1d4b9

Arrange func alphabetically

f6d0bb4

LimKianAn requested a review from adracus October 27, 2021 09:54

adracus approved these changes Oct 28, 2021

View reviewed changes

adracus merged commit e83d486 into main Oct 28, 2021

adracus deleted the 176-test-subnet-controller branch October 28, 2021 09:14

afritzler added the enhancement New feature or request label Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test subnet controller #177

Test subnet controller #177

LimKianAn commented Oct 21, 2021

LimKianAn commented Oct 22, 2021

GrigoriyMikhalkin Oct 22, 2021

LimKianAn Oct 25, 2021

GrigoriyMikhalkin Oct 22, 2021 •

edited

Loading

LimKianAn Oct 25, 2021

GrigoriyMikhalkin Oct 22, 2021

LimKianAn Oct 25, 2021

GrigoriyMikhalkin Oct 25, 2021

GrigoriyMikhalkin Oct 25, 2021

LimKianAn Oct 25, 2021

LimKianAn Oct 25, 2021

GrigoriyMikhalkin Oct 25, 2021 •

edited

Loading

GrigoriyMikhalkin Oct 25, 2021

LimKianAn Oct 25, 2021

adracus left a comment

adracus Oct 25, 2021

adracus Oct 25, 2021

adracus Oct 25, 2021

LimKianAn Oct 25, 2021

adracus Oct 25, 2021

LimKianAn Oct 25, 2021

adracus Oct 25, 2021

adracus Oct 25, 2021

adracus Oct 25, 2021

adracus Oct 25, 2021

adracus Oct 25, 2021

adracus Oct 25, 2021

GrigoriyMikhalkin Oct 26, 2021

LimKianAn Oct 26, 2021

adracus left a comment

adracus Oct 26, 2021

adracus Oct 26, 2021

Test subnet controller #177

Test subnet controller #177

Conversation

LimKianAn commented Oct 21, 2021

LimKianAn commented Oct 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrigoriyMikhalkin Oct 22, 2021 • edited Loading

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

GrigoriyMikhalkin Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adracus 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

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

Choose a reason for hiding this comment

adracus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrigoriyMikhalkin Oct 22, 2021 •

edited

Loading

GrigoriyMikhalkin Oct 25, 2021 •

edited

Loading