Skip to content

Commit

Permalink
Merge pull request juju#5677 from mjs/MM-migrationmaster-watcher-cont…
Browse files Browse the repository at this point in the history 
…ruction

api/migrationmaster: Inject watcher factory

Injecting the factory which creates client-side NotifyWatchers greatly simplifies testing.

Also added some missing test coverage for WatchMinionReports.

(Review request: http://reviews.vapour.ws/r/5121/)
  • Loading branch information
@jujubot
jujubot authored Jun 21, 2016
2 parents 070d12d + 6ad0851 commit 1564435
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 100 deletions.
20 changes: 12 additions & 8 deletions api/migrationmaster/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,27 @@ import (
"gopkg.in/juju/names.v2"

"github.com/juju/juju/api/base"
apiwatcher "github.com/juju/juju/api/watcher"
"github.com/juju/juju/apiserver/params"
"github.com/juju/juju/core/migration"
"github.com/juju/juju/watcher"
)

// NewWatcherFunc exists to let us unit test Facade without patching.
type NewWatcherFunc func(base.APICaller, params.NotifyWatchResult) watcher.NotifyWatcher

// NewClient returns a new Client based on an existing API connection.
func NewClient(caller base.APICaller) *Client {
return &Client{base.NewFacadeCaller(caller, "MigrationMaster")}
func NewClient(caller base.APICaller, newWatcher NewWatcherFunc) *Client {
return &Client{
caller: base.NewFacadeCaller(caller, "MigrationMaster"),
newWatcher: newWatcher,
}
}

// Client describes the client side API for the MigrationMaster facade
// (used by the migrationmaster worker).
type Client struct {
caller base.FacadeCaller
caller base.FacadeCaller
newWatcher NewWatcherFunc
}

// Watch returns a watcher which reports when a migration is active
Expand All @@ -37,8 +43,7 @@ func (c *Client) Watch() (watcher.NotifyWatcher, error) {
if result.Error != nil {
return nil, result.Error
}
w := apiwatcher.NewNotifyWatcher(c.caller.RawAPICaller(), result)
return w, nil
return c.newWatcher(c.caller.RawAPICaller(), result), nil
}

// GetMigrationStatus returns the details and progress of the latest
Expand Down Expand Up @@ -138,8 +143,7 @@ func (c *Client) WatchMinionReports() (watcher.NotifyWatcher, error) {
if result.Error != nil {
return nil, result.Error
}
w := apiwatcher.NewNotifyWatcher(c.caller.RawAPICaller(), result)
return w, nil
return c.newWatcher(c.caller.RawAPICaller(), result), nil
}

// GetMinionReports returns details of the reports made by migration
Expand Down
140 changes: 49 additions & 91 deletions api/migrationmaster/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
package migrationmaster_test

import (
"time"

"github.com/juju/errors"
jujutesting "github.com/juju/testing"
jc "github.com/juju/testing/checkers"
Expand All @@ -14,12 +12,12 @@ import (
gc "gopkg.in/check.v1"
"gopkg.in/juju/names.v2"

"github.com/juju/juju/api/base"
apitesting "github.com/juju/juju/api/base/testing"
"github.com/juju/juju/api/migrationmaster"
"github.com/juju/juju/apiserver/params"
"github.com/juju/juju/core/migration"
coretesting "github.com/juju/juju/testing"
"github.com/juju/juju/worker"
"github.com/juju/juju/watcher"
)

type ClientSuite struct {
Expand All @@ -32,55 +30,30 @@ func (s *ClientSuite) TestWatch(c *gc.C) {
var stub jujutesting.Stub
apiCaller := apitesting.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
stub.AddCall(objType+"."+request, id, arg)
switch request {
case "Watch":
*(result.(*params.NotifyWatchResult)) = params.NotifyWatchResult{
NotifyWatcherId: "abc",
}
case "Next":
return errors.New("boom")
case "Stop":
*(result.(*params.NotifyWatchResult)) = params.NotifyWatchResult{
NotifyWatcherId: "123",
}
return nil
})
expectWatch := &struct{ watcher.NotifyWatcher }{}
newWatcher := func(caller base.APICaller, result params.NotifyWatchResult) watcher.NotifyWatcher {
c.Check(caller, gc.NotNil)
c.Check(result, jc.DeepEquals, params.NotifyWatchResult{NotifyWatcherId: "123"})
return expectWatch
}
client := migrationmaster.NewClient(apiCaller, newWatcher)

client := migrationmaster.NewClient(apiCaller)
w, err := client.Watch()
c.Assert(err, jc.ErrorIsNil)
defer worker.Stop(w)

errC := make(chan error)
go func() {
errC <- w.Wait()
}()

select {
case err := <-errC:
c.Assert(err, gc.ErrorMatches, "boom")
expectedCalls := []jujutesting.StubCall{
{"MigrationMaster.Watch", []interface{}{"", nil}},
{"NotifyWatcher.Next", []interface{}{"abc", nil}},
{"NotifyWatcher.Stop", []interface{}{"abc", nil}},
}
// The Stop API call happens in a separate goroutine which
// might execute after the worker has exited so wait for the
// expected calls to arrive.
for a := coretesting.LongAttempt.Start(); a.Next(); {
if len(stub.Calls()) >= len(expectedCalls) {
return
}
}
stub.CheckCalls(c, expectedCalls)
case <-time.After(coretesting.LongWait):
c.Fatal("timed out waiting for watcher to die")
}
c.Check(err, jc.ErrorIsNil)
c.Check(w, gc.Equals, expectWatch)
stub.CheckCalls(c, []jujutesting.StubCall{{"MigrationMaster.Watch", []interface{}{"", nil}}})
}

func (s *ClientSuite) TestWatchErr(c *gc.C) {
func (s *ClientSuite) TestWatchCallError(c *gc.C) {
apiCaller := apitesting.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
return errors.New("boom")
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.Watch()
c.Assert(err, gc.ErrorMatches, "boom")
}
Expand All @@ -106,7 +79,7 @@ func (s *ClientSuite) TestGetMigrationStatus(c *gc.C) {
}
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)

status, err := client.GetMigrationStatus()
c.Assert(err, jc.ErrorIsNil)
Expand All @@ -130,7 +103,7 @@ func (s *ClientSuite) TestSetPhase(c *gc.C) {
stub.AddCall(objType+"."+request, id, arg)
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
err := client.SetPhase(migration.QUIESCE)
c.Assert(err, jc.ErrorIsNil)
expectedArg := params.SetMigrationPhaseArgs{Phase: "QUIESCE"}
Expand All @@ -143,7 +116,7 @@ func (s *ClientSuite) TestSetPhaseError(c *gc.C) {
apiCaller := apitesting.APICallerFunc(func(string, int, string, string, interface{}, interface{}) error {
return errors.New("boom")
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
err := client.SetPhase(migration.QUIESCE)
c.Assert(err, gc.ErrorMatches, "boom")
}
Expand All @@ -163,7 +136,7 @@ func (s *ClientSuite) TestExport(c *gc.C) {
}
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
out, err := client.Export()
c.Assert(err, jc.ErrorIsNil)
stub.CheckCalls(c, []jujutesting.StubCall{
Expand All @@ -182,7 +155,7 @@ func (s *ClientSuite) TestExportError(c *gc.C) {
apiCaller := apitesting.APICallerFunc(func(string, int, string, string, interface{}, interface{}) error {
return errors.New("blam")
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.Export()
c.Assert(err, gc.ErrorMatches, "blam")
}
Expand All @@ -193,7 +166,7 @@ func (s *ClientSuite) TestReap(c *gc.C) {
stub.AddCall(objType+"."+request, id, arg)
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
err := client.Reap()
c.Check(err, jc.ErrorIsNil)
stub.CheckCalls(c, []jujutesting.StubCall{
Expand All @@ -205,7 +178,7 @@ func (s *ClientSuite) TestReapError(c *gc.C) {
apiCaller := apitesting.APICallerFunc(func(string, int, string, string, interface{}, interface{}) error {
return errors.New("blam")
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
err := client.Reap()
c.Assert(err, gc.ErrorMatches, "blam")
}
Expand All @@ -214,48 +187,33 @@ func (s *ClientSuite) TestWatchMinionReports(c *gc.C) {
var stub jujutesting.Stub
apiCaller := apitesting.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
stub.AddCall(objType+"."+request, id, arg)
switch request {
case "WatchMinionReports":
*(result.(*params.NotifyWatchResult)) = params.NotifyWatchResult{
NotifyWatcherId: "abc",
}
case "Next":
return errors.New("boom")
case "Stop":
*(result.(*params.NotifyWatchResult)) = params.NotifyWatchResult{
NotifyWatcherId: "123",
}
return nil
})

client := migrationmaster.NewClient(apiCaller)
w, err := client.WatchMinionReports()
c.Assert(err, jc.ErrorIsNil)
defer worker.Stop(w)
expectWatch := &struct{ watcher.NotifyWatcher }{}
newWatcher := func(caller base.APICaller, result params.NotifyWatchResult) watcher.NotifyWatcher {
c.Check(caller, gc.NotNil)
c.Check(result, jc.DeepEquals, params.NotifyWatchResult{NotifyWatcherId: "123"})
return expectWatch
}
client := migrationmaster.NewClient(apiCaller, newWatcher)

errC := make(chan error)
go func() {
errC <- w.Wait()
}()
w, err := client.WatchMinionReports()
c.Check(err, jc.ErrorIsNil)
c.Check(w, gc.Equals, expectWatch)
stub.CheckCalls(c, []jujutesting.StubCall{{"MigrationMaster.WatchMinionReports", []interface{}{"", nil}}})
}

select {
case err := <-errC:
c.Assert(err, gc.ErrorMatches, "boom")
expectedCalls := []jujutesting.StubCall{
{"MigrationMaster.WatchMinionReports", []interface{}{"", nil}},
{"NotifyWatcher.Next", []interface{}{"abc", nil}},
{"NotifyWatcher.Stop", []interface{}{"abc", nil}},
}
// The Stop API call happens in a separate goroutine which
// might execute after the worker has exited so wait for the
// expected calls to arrive.
for a := coretesting.LongAttempt.Start(); a.Next(); {
if len(stub.Calls()) >= len(expectedCalls) {
return
}
}
stub.CheckCalls(c, expectedCalls)
case <-time.After(coretesting.LongWait):
c.Fatal("timed out waiting for watcher to die")
}
func (s *ClientSuite) TestWatchMinionReportsError(c *gc.C) {
apiCaller := apitesting.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
return errors.New("boom")
})
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.WatchMinionReports()
c.Assert(err, gc.ErrorMatches, "boom")
}

func (s *ClientSuite) TestGetMinionReports(c *gc.C) {
Expand All @@ -281,7 +239,7 @@ func (s *ClientSuite) TestGetMinionReports(c *gc.C) {
}
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
out, err := client.GetMinionReports()
c.Assert(err, jc.ErrorIsNil)
stub.CheckCalls(c, []jujutesting.StubCall{
Expand All @@ -303,7 +261,7 @@ func (s *ClientSuite) TestGetMinionReportsFailedCall(c *gc.C) {
apiCaller := apitesting.APICallerFunc(func(string, int, string, string, interface{}, interface{}) error {
return errors.New("blam")
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.GetMinionReports()
c.Assert(err, gc.ErrorMatches, "blam")
}
Expand All @@ -316,7 +274,7 @@ func (s *ClientSuite) TestGetMinionReportsInvalidPhase(c *gc.C) {
}
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.GetMinionReports()
c.Assert(err, gc.ErrorMatches, `invalid phase: "BLARGH"`)
}
Expand All @@ -330,7 +288,7 @@ func (s *ClientSuite) TestGetMinionReportsBadUnknownTag(c *gc.C) {
}
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.GetMinionReports()
c.Assert(err, gc.ErrorMatches, `processing unknown agents: "carl" is not a valid tag`)
}
Expand All @@ -344,7 +302,7 @@ func (s *ClientSuite) TestGetMinionReportsBadFailedTag(c *gc.C) {
}
return nil
})
client := migrationmaster.NewClient(apiCaller)
client := migrationmaster.NewClient(apiCaller, nil)
_, err := client.GetMinionReports()
c.Assert(err, gc.ErrorMatches, `processing failed agents: "dave" is not a valid tag`)
}
3 changes: 2 additions & 1 deletion worker/migrationmaster/shim.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import (
"github.com/juju/errors"
"github.com/juju/juju/api/base"
"github.com/juju/juju/api/migrationmaster"
"github.com/juju/juju/api/watcher"
"github.com/juju/juju/worker"
)

func NewFacade(apiCaller base.APICaller) (Facade, error) {
facade := migrationmaster.NewClient(apiCaller)
facade := migrationmaster.NewClient(apiCaller, watcher.NewNotifyWatcher)
return facade, nil
}

Expand Down

0 comments on commit 1564435

Please sign in to comment.