Remove controller from unit tests #164

admiyo · 2017-03-23T01:33:42Z

No description provided.

stu-gott · 2017-03-24T12:26:32Z

pkg/virt-controller/watch/job.go

@@ -15,7 +15,7 @@ import (
 	"kubevirt.io/kubevirt/pkg/virt-controller/services"
 )

-func migrationJobSelector() kubeapi.ListOptions {
+func MigrationJobSelector() kubeapi.ListOptions {


What's the rationale behind making this a public function? It's fine to do that, but the "panic" instruction in this code should instead return an error if this is to be a general purpose function.

Returned to private naming

stu-gott · 2017-03-24T12:42:29Z

pkg/virt-controller/watch/migration.go

 				if err != nil {
-					logger.Error().Reason(err).Msg("updating migration state failed")
+					logger.Error().Reason(err).Msg("creating am migration target node failed")


admiyo · 2017-03-27T18:18:50Z

retest this please

stu-gott

Most of my comments are nitpicky. The most important one to look at is the mergeConstraints question.

stu-gott · 2017-03-27T18:38:10Z

pkg/virt-controller/watch/job_test.go

 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/ghttp"
 	"k8s.io/client-go/kubernetes"
+	"net/http"


Looks like your IDE decided to move this line

stu-gott · 2017-03-27T18:43:36Z

pkg/virt-controller/watch/migration.go

 		}
 	}
+	vm.Spec.NodeSelector = merged


Might want to run this line after the "if len(conflicts) > 0" block. This makes a side-effect change since VM was passed in by pointer, so we should check that there are no errors first.

Agreed, that should be after the check.

stu-gott · 2017-03-27T18:48:37Z

pkg/virt-controller/watch/migration_test.go

+			close(done)
+		}, 10)
+
+		It("should fail if confliciting VM and Migration have conflicting Node Selectors", func(done Done) {


typo: "confliciting"

stu-gott · 2017-03-27T19:10:22Z

pkg/virt-handler/domain.go

-		queue.AddRateLimited(key)
-	} else if !vmExists || obj.(*v1.VM).GetObjectMeta().GetUID() != domain.GetObjectMeta().GetUID() {
+	obj, vmExists, _ := d.vmStore.GetByKey(key.(string))
+	// GetByKey cannot return an error, leading the error handling code


Nit: Technically this comment refers to non-existent code.

It explains why the err in the above line is ignored.

stu-gott · 2017-03-27T19:11:44Z

pkg/virt-handler/domain_test.go

 	"kubevirt.io/kubevirt/pkg/logging"
 	. "kubevirt.io/kubevirt/pkg/virt-handler"
 	"kubevirt.io/kubevirt/pkg/virt-handler/virtwrap"
+	"time"


Wrong import order

stu-gott · 2017-03-27T19:15:04Z

pkg/virt-handler/domain_test.go

 			Consistently(vmQueue.Len).Should(Equal(0))
 		})
+
+		It("should create not requeue even if domain reference if it is not in the cache", func() {


Should not create?

dropped the "create"

stu-gott · 2017-03-27T19:18:05Z

pkg/virt-controller/watch/migration_test.go

+			close(done)
+		}, 10)
+
+		It("Should mark Migation failed if VM not running ", func(done Done) {


Typo: "Migation"

stu-gott · 2017-03-27T19:25:55Z

pkg/virt-controller/watch/pod_test.go


-			Expect(len(server.ReceivedRequests())).To(Equal(5))
+			Expect(len(server.ReceivedRequests())).To(Equal(7))


I wish there were a better way to test this (I'm not saying there is). The number of API calls made by any given section of code is a good candidate for something likely to change--so this test could unexpectedly fail in the future.

I think that is all right, since we have to mock this layer, there is no way around it. I would not like to have rest calls lying aroung which are not even used.

But: Instead of checking this explicitly I would add one check in the AfterEach section which makes sure that the amount of registered calls equals the amount of expected calls. The nwe can remove this line from the test itself.

Yeah, but there is no way to get a count of the registered handlers, so the logic here is as simple as we can get.

rmohr · 2017-03-28T07:18:46Z

pkg/virt-controller/watch/job.go

@@ -55,12 +50,9 @@ type JobDispatch struct {

 func (jd *JobDispatch) Execute(store cache.Store, queue workqueue.RateLimitingInterface, key interface{}) {
 	// Fetch the latest Job state from cache
-	obj, exists, err := store.GetByKey(key.(string))
+	// Cannot return an error
+	obj, exists, _ := store.GetByKey(key.(string))


The controller loop has no idea which cache implementation is used, so I would say that it is not it's job to guess about the internals.

rmohr · 2017-03-28T07:22:44Z

pkg/virt-controller/watch/migration.go

@@ -55,12 +50,9 @@ func (md *MigrationDispatch) Execute(store cache.Store, queue workqueue.RateLimi
 	}

 	// Fetch the latest Migration state from cache
-	obj, exists, err := store.GetByKey(key.(string))
+	// error is always nil
+	obj, exists, _ := store.GetByKey(key.(string))


Again, I would expect the error although, the current used store does not throw one ...

rmohr · 2017-03-28T07:24:34Z

pkg/virt-controller/watch/migration.go

 		}
 	}
+	vm.Spec.NodeSelector = merged


Agreed, that should be after the check.

rmohr · 2017-03-28T07:28:12Z

pkg/virt-controller/watch/migration_test.go

+		expectedMigration := obj.(*v1.Migration)
+		expectedMigration.Status.Phase = expectedStatus
+		expectedMigration.Kind = "Migration"
+		expectedMigration.APIVersion = "kubevirt.io/v1alpha1"


Setting all these fields looks strange. I think it is better if you only explicitly set fields which are relevant for the test.

Self link, ... are not something we need to check here ...

They are set by the calling code, and we need to confirm a match. Any other logic would be more complicated.

What I meant is that you are copying the object anyway already. So you are overriding for the test unimportant fields with the value which it already contains (like self link, ...).

Since you are copying it already, just overriding the fields with the expected changes should do ...

rmohr

The checks need a few cleanups:

On requeue we need to check for the history of the key too
We always have to check if the queue is empty after a call which should be successful

rmohr · 2017-03-28T07:32:15Z

pkg/virt-controller/watch/job_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(2))
+			Eventually(jobQueue.Len()).Should(Equal(1))


A check if we have a history of the key is missing: queue.NumRequeues(key) == 1

rmohr · 2017-03-28T07:32:52Z

pkg/virt-controller/watch/job_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(3))
+			Eventually(jobQueue.Len()).Should(Equal(1))


A check if we have a history of the key is missing

rmohr · 2017-03-28T07:32:58Z

pkg/virt-controller/watch/job_test.go

 			Expect(len(server.ReceivedRequests())).To(Equal(4))
+			Eventually(jobQueue.Len()).Should(Equal(1))


A check if we have a history of the key is missing

rmohr · 2017-03-28T07:33:50Z

pkg/virt-controller/watch/job_test.go

+			)
+
+			doExecute()
+			Expect(len(server.ReceivedRequests())).To(Equal(4))


I think you are not testing if the queue is now empty.

rmohr · 2017-03-28T07:36:52Z

pkg/virt-controller/watch/migration_test.go

-			// Create the expected VM after the update
-			obj, err := conversion.NewCloner().DeepCopy(vm)
-			Expect(err).ToNot(HaveOccurred())
+			Expect(len(server.ReceivedRequests())).To(Equal(4))


We need to check if the queue is empty.

rmohr · 2017-03-28T07:39:25Z

pkg/virt-controller/watch/migration_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(1))
+			Eventually(queue.Len()).Should(Equal(1))


rmohr · 2017-03-28T07:39:31Z

pkg/virt-controller/watch/migration_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(3))
+			Eventually(queue.Len()).Should(Equal(1))


rmohr · 2017-03-28T07:39:39Z

pkg/virt-controller/watch/migration_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(3))
+			Eventually(queue.Len()).Should(Equal(1))


rmohr · 2017-03-28T07:39:45Z

pkg/virt-controller/watch/migration_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(3))
+			Eventually(queue.Len()).Should(Equal(1))


rmohr · 2017-03-28T07:39:52Z

pkg/virt-controller/watch/migration_test.go

+			doExecute()
+
+			Expect(len(server.ReceivedRequests())).To(Equal(3))
+			Eventually(queue.Len()).Should(Equal(1))


rmohr · 2017-03-28T08:16:36Z

@admiyo one of the functional tests failed, could you check if there is something behind it?

Increases Unit test coverage

admiyo · 2017-03-28T15:36:11Z

That is just the rebase: have not addressed comments yet.

rmohr · 2017-03-28T17:54:49Z

pkg/virt-handler/domain.go

-		return
-	}
+	obj, exists, _ := indexer.GetByKey(key.(string))
+	// GetByKey can never return anything but nil for the error


@admiyo , you could use your own store implementation (A store is an interface) which can throw an error, if that is your line of agrumentation. Every code in kubernetes itself is also taking this error into account.

Its a bug, then, in the library we are using.

Why? There is an interface which says, return an error if there is one, but that does not mean that every implementation necessarily is written in a way that there can happen an error to report.

The most extreme examples: Think of an in memory map, which can't have an error when looking up a key. It is a valid implementation of a store and when accessed through the interface, you always return nil as an error. Now think of a file based store. When looking something there up, it can fail.

The interface is the contract I would want to implement. I don't think it is nice, to assume a concrete implementation behind an interface.

I think that, if I add it back in, we'll still be under the threshold for untestable code. Let's see.

of course this can be tested. create a cache class that throws an error.

fix travis-9

admiyo force-pushed the remove-controller-from-unit-tests branch from ce89023 to 6d17b9c Compare March 23, 2017 15:30

stu-gott reviewed Mar 24, 2017

View reviewed changes

admiyo force-pushed the remove-controller-from-unit-tests branch 2 times, most recently from 371b3d5 to c95ce1e Compare March 24, 2017 18:18

admiyo force-pushed the remove-controller-from-unit-tests branch from c95ce1e to e3f0293 Compare March 27, 2017 18:28

stu-gott requested changes Mar 27, 2017

View reviewed changes

rmohr requested changes Mar 28, 2017

View reviewed changes

admiyo added 5 commits March 28, 2017 11:32

Remove controller from tests

3bd8bf8

Renamed Factory functions

4d43cdb

Increase unit test coverage for virt-handler

7241db6

increase test covereage for job

1bcd1ab

Fix Migration selector merge

8fc7024

Increases Unit test coverage

admiyo force-pushed the remove-controller-from-unit-tests branch from e3f0293 to 8fc7024 Compare March 28, 2017 15:35

admiyo added 2 commits March 28, 2017 13:16

Check number of requeues in unit tests

9db140d

Check for 0 requeues on successful job tests

d7c5d69

rmohr reviewed Mar 28, 2017

View reviewed changes

Add back in error handlers for store.GetByKey

0b7a627

rmohr merged commit 0b7a627 into kubevirt:master Mar 30, 2017

admiyo deleted the remove-controller-from-unit-tests branch April 7, 2017 17:17

kubevirt-bot pushed a commit to kubevirt-bot/kubevirt that referenced this pull request Nov 6, 2020

Merge pull request kubevirt#164 from screeley44/travis-9

4053bff

fix travis-9


		Expect(len(server.ReceivedRequests())).To(Equal(5))
		Expect(len(server.ReceivedRequests())).To(Equal(7))

		Expect(len(server.ReceivedRequests())).To(Equal(4))
		Eventually(jobQueue.Len()).Should(Equal(1))

Remove controller from unit tests #164

Remove controller from unit tests #164

Conversation

admiyo commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

admiyo commented Mar 27, 2017

stu-gott 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

admiyo Mar 28, 2017 • 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

rmohr 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

rmohr commented Mar 28, 2017

admiyo commented Mar 28, 2017 • 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

admiyo Mar 28, 2017 •

edited

admiyo commented Mar 28, 2017 •

edited