Add ginkgo tests for machineset controller #544

alexander-demicev · 2020-04-01T12:09:15Z

This PR adds ginkgo unit tests for machineset controller and one reconcile test, the tests are similar to upstream https://github.com/kubernetes-sigs/cluster-api/blob/09f87a2aadcd590952e17684f5c05259337e4da9/controllers/machineset_controller_test.go

JoelSpeed · 2020-04-01T12:20:50Z

pkg/controller/machineset/machineset_controller_suite_test.go

+// SetupTestReconcile returns a reconcile.Reconcile implementation that delegates to inner and
+// writes the request to requests after Reconcile is finished.
+func SetupTestReconcile(inner reconcile.Reconciler) reconcile.Reconciler {
+	fn := reconcile.Func(func(req reconcile.Request) (reconcile.Result, error) {
+		result, err := inner.Reconcile(req)
+		return result, err
+	})
+	return fn


I don't think this actually does anything for us? I've seen this pattern before where it writes to a channel, but I always found relying on that to be annoying and not help us to write good tests, my suggestion would be to remove this

👍 I thought about removing it too

JoelSpeed · 2020-04-01T12:23:37Z

pkg/controller/machineset/machineset_controller_suite_test.go

+	By("setting up a new manager")
+	mgr, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"})
+	Expect(err).NotTo(HaveOccurred())

-// SetupTestReconcile returns a reconcile.Reconcile implementation that delegates to inner and
-// writes the request to requests after Reconcile is finished.
-func SetupTestReconcile(inner reconcile.Reconciler) (reconcile.Reconciler, chan reconcile.Request) {
-	requests := make(chan reconcile.Request)
-	fn := reconcile.Func(func(req reconcile.Request) (reconcile.Result, error) {
-		result, err := inner.Reconcile(req)
-		requests <- req
-		return result, err
-	})
-	return fn, requests
-}
+	k8sClient = mgr.GetClient()

-// StartTestManager adds recFn
-func StartTestManager(mgr manager.Manager, t *testing.T) (chan struct{}, chan error) {
-	t.Helper()
+	By("setting up a new reconciler")
+	r := newReconciler(mgr)
+	reconciler = SetupTestReconcile(r)

-	stop := make(chan struct{})
-	errs := make(chan error, 1)
+	err = add(mgr, reconciler, r.MachineToMachineSets)
+	Expect(err).NotTo(HaveOccurred())

+	By("starting the manager")
 	go func() {
-		errs <- mgr.Start(stop)
+		Expect(mgr.Start(doneMgr)).To(Succeed())
 	}()


This could potentially be done in a BeforeEach within the test suite, that way we start a new manager and reconciler for each test and there is less potential for side effects between individual tests

JoelSpeed · 2020-04-01T12:24:23Z

pkg/controller/machineset/machineset_controller_suite_test.go

 	go func() {
-		errs <- mgr.Start(stop)
+		Expect(mgr.Start(doneMgr)).To(Succeed())
 	}()


Separate go routines calling gomega expectations require a defer GinkgoRecover() at the top

JoelSpeed · 2020-04-01T12:26:45Z

pkg/controller/machineset/machineset_controller_test.go

+	AfterEach(func() {
+		By("Deleting the namespace")
+		Expect(k8sClient.Delete(ctx, namespace)).To(Succeed())
+	})


There's no GC running so you'll need to delete all Machines and MachineSets too, probably worth a helper function that does this (list all machines/machinesets, iterate through and delete). Doing this way means you don't have to clean up within your test IT's, just rely on the suite to clean everything

JoelSpeed · 2020-04-01T12:28:46Z

pkg/controller/machineset/machineset_controller_test.go

+		Eventually(func() int {
+			if err := k8sClient.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil {
+				return -1
+			}
+			return len(machines.Items)
+		}, timeout).Should(BeEquivalentTo(replicas))


You can return a second parameter here (I learnt this recently) and Eventually will check the non-first params are their zero values/nil, that way you don't have to make up "error" values as we have done before. Might be worth doing the same on the others so that errors are surfaced properly

Suggested change

Eventually(func() int {

if err := k8sClient.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil {

return -1

}

return len(machines.Items)

}, timeout).Should(BeEquivalentTo(replicas))

Eventually(func() (int, error) {

if err := k8sClient.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil {

return 0, err

}

return len(machines.Items), nil

}, timeout).Should(BeEquivalentTo(replicas))

JoelSpeed · 2020-04-01T14:27:48Z

pkg/controller/machineset/controller_test.go

@@ -262,3 +266,67 @@ func TestAdoptOrphan(t *testing.T) {
 		}
 	}
 }
+
+func TestMachineSetReconcile(t *testing.T) {


Any reason not to use Ginkgo for these two tests as well?

var _ = Describe("MachineSet Reconcile", func() { var r reconciler var result reconile.Result var reconcileErr error BeforeEach(func() { // set up reconciler as r with scheme and new fake recorder }) JustBeforeEach(func() { result, reconcileErr = r.Reconcile(request) }) Context("with a valid machineset", func() { BeforeEach(func() { // set up machinset // set up fake client // override r.client }) It("has the expected result", func() { Expect(result).To(Equal()) }) It("has the expected error", func() { Expect(reconcileErr).To(Equal()) }) It("did something with events", func() { g.Eventually(r.recorder.Events).Should(Receive()) }) }) Context("with an invalid machineset", func() { // This should look basically the same as above }) })

JoelSpeed · 2020-04-01T14:36:40Z

pkg/controller/machineset/machineset_controller_test.go

+		By("Deleting the namespace")
+		Expect(k8sClient.Delete(ctx, namespace)).To(Succeed())
+
+		By("Deleting the machinesets")
+		cleanResources()


I would swap the order of these around so you clean the resources before deleting the namespace

JoelSpeed · 2020-04-01T14:39:30Z

pkg/controller/machineset/machineset_controller_test.go

+				return 0, err
+			}
+			return len(machines.Items), nil
+		}, timeout).Should(BeEquivalentTo(replicas))


I assume there's a type mismatch if you use Equal here instead?

JoelSpeed · 2020-04-01T14:51:08Z

pkg/controller/machineset/machineset_controller_test.go

+		// Verify that the Machine has been deleted.
+		Eventually(func() bool {
+			key := client.ObjectKey{Name: machineToBeDeleted.Name, Namespace: machineToBeDeleted.Namespace}
+			if err := k8sClient.Get(ctx, key, &machineToBeDeleted); apierrors.IsNotFound(err) || !machineToBeDeleted.DeletionTimestamp.IsZero() {


Is there anything adding a finalizer to the Machines? Do you expect them not to be deleted straight away?, I'd be tempted to remove the second check and see if this still passes?

You are right, the check can be removed

JoelSpeed · 2020-04-01T14:52:12Z

pkg/controller/machineset/machineset_controller_test.go

+		Eventually(func() (ready int) {
+			if err := k8sClient.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil {
+				return -1
+			}


Suggested change

Eventually(func() (ready int) {

if err := k8sClient.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil {

return -1

}

Eventually(func() (int, error) {

ready := 0

if err := k8sClient.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil {

return 0, err

}

JoelSpeed · 2020-04-01T14:58:09Z

pkg/controller/machineset/machineset_controller_test.go

+	for _, machineSet := range machineSets.Items {
+		if err := k8sClient.Delete(ctx, &machineSet); err != nil {
+			return err
+		}


Taking the address of a loop variable in go is asking for trouble! 😅 Do this instead to avoid confusion later 😉

Suggested change

for _, machineSet := range machineSets.Items {

if err := k8sClient.Delete(ctx, &machineSet); err != nil {

return err

}

for _, machineSet := range machineSets.Items {

ms := machineSet

if err := k8sClient.Delete(ctx, &ms); err != nil {

return err

}

While I don't think it makes a problem here, it can in certain cases so it's best to just avoid it

Have a read of https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable for more info

Please also fix the one below

JoelSpeed · 2020-04-01T14:59:10Z

We aren't reducing the test cases right, as far as I can see the new file with the tests in that call Reconcile directly match two of the cases removed from the ones converted to Ginkgo?

alexander-demicev · 2020-04-01T16:07:23Z

@JoelSpeed Yes, we are not reducing coverage and all comments are fixed now.

JoelSpeed

In terms of refactoring, looks great! Added a couple of suggestions for improving the test naming and also potentially for adding some extra cases but I'm not fussed about those going in this PR, just ideas for future improvements

JoelSpeed · 2020-04-01T16:09:57Z

pkg/controller/machineset/controller_test.go

+			Expect(result).To(Equal(reconcile.Result{}))
+		})
+
+		It("has the expected error", func() {


Suggested change

It("has the expected error", func() {

It("does not return an error", func() {

JoelSpeed · 2020-04-01T16:10:29Z

pkg/controller/machineset/controller_test.go

+		It("did something with events", func() {
+			Eventually(rec.Events).Should(Receive())
+		})


Is it possible to also check the result and error here? Do you know what they're expected to be? /could you work it out from the code? (Could be a follow up)

That should be possible with Eventually(rec.Events).Should(Receive(Equal("..")))

JoelSpeed · 2020-04-01T16:10:52Z

pkg/controller/machineset/controller_test.go

+		It("has the expected result", func() {
+			Expect(result).To(Equal(reconcile.Result{}))
+		})


Should there be any events in this case? Could check those? (Could be a follow up)

No, in this case, there shouldn't

JoelSpeed · 2020-04-01T16:11:15Z

pkg/controller/machineset/controller_test.go

+			r.Client = fake.NewFakeClientWithScheme(scheme.Scheme, ms)
+		})
+
+		It("has the expected result", func() {


Suggested change

It("has the expected result", func() {

It("returns an empty result", func() {

alexander-demicev · 2020-04-01T16:44:22Z

@JoelSpeed Can one more review round?

JoelSpeed

/lgtm

Maybe worth a follow up to improve the reconcile test but that's another concern

enxebre · 2020-04-03T11:59:28Z

/approve

openshift-ci-robot · 2020-04-03T12:00:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

openshift-bot · 2020-04-03T12:07:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-03T12:18:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

enxebre · 2020-04-06T07:58:58Z

/lgtm

openshift-bot · 2020-04-06T08:07:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-06T09:12:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-06T10:15:51Z

@alexander-demichev: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure	`5947d94`	link	`/test e2e-azure`
ci/prow/e2e-aws-scaleup-rhel7	`5947d94`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

openshift-bot · 2020-04-06T10:17:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2020

openshift-ci-robot requested review from JoelSpeed and michaelgugino April 1, 2020 12:09

JoelSpeed reviewed Apr 1, 2020

View reviewed changes

alexander-demicev force-pushed the machisettests branch from 32e6558 to ec2b70a Compare April 1, 2020 13:31

alexander-demicev changed the title ~~[WIP]Add ginkgo tests for machineset controller~~ Add ginkgo tests for machineset controller Apr 1, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2020

alexander-demicev force-pushed the machisettests branch from d6c8739 to 5feae08 Compare April 1, 2020 14:10

JoelSpeed reviewed Apr 1, 2020

View reviewed changes

alexander-demicev force-pushed the machisettests branch from 5feae08 to 536ca27 Compare April 1, 2020 16:06

JoelSpeed reviewed Apr 1, 2020

View reviewed changes

alexander-demicev force-pushed the machisettests branch from 536ca27 to 74e9ded Compare April 1, 2020 16:43

JoelSpeed reviewed Apr 1, 2020

View reviewed changes

openshift-ci-robot assigned JoelSpeed Apr 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2020

Alexander Demichev added 3 commits April 3, 2020 14:28

Update dependencies

1c0ff15

Add ginkgo tests for machineset controller

cec871f

Add machineset reconcile unit test

5947d94

alexander-demicev force-pushed the machisettests branch from 74e9ded to 5947d94 Compare April 3, 2020 12:29

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2020

openshift-ci-robot assigned enxebre Apr 6, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020

openshift-merge-robot merged commit 79ccf91 into openshift:master Apr 6, 2020

	It("has the expected error", func() {
	It("does not return an error", func() {

	It("has the expected result", func() {
	It("returns an empty result", func() {

Add ginkgo tests for machineset controller #544

Add ginkgo tests for machineset controller #544

Conversation

alexander-demicev commented Apr 1, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Apr 1, 2020

alexander-demicev commented Apr 1, 2020

JoelSpeed 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

alexander-demicev commented Apr 1, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

enxebre commented Apr 3, 2020

openshift-ci-robot commented Apr 3, 2020

openshift-bot commented Apr 3, 2020

openshift-bot commented Apr 3, 2020

enxebre commented Apr 6, 2020

openshift-bot commented Apr 6, 2020

openshift-bot commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020 • edited

openshift-bot commented Apr 6, 2020

alexander-demicev commented Apr 1, 2020 •

edited

openshift-ci-robot commented Apr 6, 2020 •

edited