Add tests for the Manager plugin #306
Conversation
Signed-off-by: David Chung <david.chung@docker.com>
Signed-off-by: David Chung <david.chung@docker.com>
|
Please sign your commits following these rules: $ git clone -b "manager-tests" git@github.com:chungers/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354169992
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Current coverage is 68.97% (diff: 100%)@@ master #306 diff @@
==========================================
Files 25 27 +2
Lines 1241 1441 +200
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 881 994 +113
- Misses 285 355 +70
- Partials 75 92 +17
|
pkg/manager/manager_test.go
Outdated
| group_rpc "github.com/docker/infrakit/pkg/rpc/group" | ||
| "github.com/docker/infrakit/pkg/rpc/server" | ||
| "github.com/docker/infrakit/pkg/spi/group" | ||
| _ "github.com/docker/infrakit/pkg/store" |
There was a problem hiding this comment.
What's the purpose of the underscore import? If it remains, can you leave a comment so it's clear why it needs to be here?
There was a problem hiding this comment.
removed anonymous import
pkg/manager/manager_test.go
Outdated
| manager1.Start() | ||
| manager2.Start() | ||
|
|
||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
As discussed offline, if you use a custom leader detector for test, you should be able to eliminate (or at least more tightly control) the leadership delay. A nice outcome for this will be to remove all the calls to time.Sleep().
…tion Signed-off-by: David Chung <david.chung@docker.com>
| leader chan string, | ||
| ctrl *gomock.Controller, | ||
| configStore func(*store_mock.MockSnapshot), | ||
| configureGroup func(*group_mock.MockPlugin)) (Backend, server.Stoppable) { |
There was a problem hiding this comment.
I'm not a big fan of function that starts to take on a ton of parameters.
Anyway for us to clean this up via struct or split this until smaller functions?
Perfect example in Kubernetes:
https://github.com/kubernetes/kubernetes/blob/973f2fcd86ef3c3e69f455aaf88553f0906b48bd/pkg/kubelet/kubelet.go#L171
Now refactored to:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet.go#L272
There was a problem hiding this comment.
I think you're referring to the long list of parameters in the function NewManager in manager.go?
This function uses parameters that are of complex type (e.g. discoverry.Plugin, leader.Detector) instead of simple types like strings and bools (seen in the k8s you referenced), so while the function arg list is long, it's hard to mess things up without the compiler complaining.
I could look into doing something with embedding / composition with a struct like a manager.Config; however, my concern is that the fields would be public/exported and can be mutated after a manager instance is initialized. Having the dependencies passed in as function params allow the manager's dependencies (as struct fields) to be unexported and immutable after initialization.
pkg/manager/manager_test.go
Outdated
|
|
||
| defer close(checkpoint2) | ||
|
|
||
| t.Log("m2 leading") |
There was a problem hiding this comment.
Remove debug log statement
pkg/manager/manager_test.go
Outdated
|
|
||
| defer close(checkpoint3) | ||
|
|
||
| t.Log("m1 releasing") |
There was a problem hiding this comment.
Remove debug log statement
Signed-off-by: David Chung <david.chung@docker.com>
5 participants
Closes #298
Changes
There are a bunch of unsigned merge commits which Gordon seems to get confused and complains about. We use squash merge so these should be ok.