Error testing infrastructure for service doubles

Error testing infrastructure for service doubles

This branch provides infrastructure to allow tests to specify that the service
doubles should return an error at various points.
The basic premise is that the test provides a hook which is run at the specfied point in time eg when a specific function runs. 
The hook is passed the service double and any function arguments. It can inspect the state of the service and the function args and
can decide to return an error if it wants to.

The diff conatins a practical example of this being used - 3 security groups are
created and an error is raised when the second one is deleted.

This work doesn't have to be used to raise an error - the hook can also be used to modify the sate of the service double and simply exit without error.

https://code.launchpad.net/~wallyworld/goose/control-test-doubles/+merge/144838

(do not edit description out of merge proposal)

[wallyworld, 2013-01-25 02:06:56]

Please take a look.

[jameinel, 2013-01-28 06:13:58]

overall LGTM.

A couple small things that we can tweak, but I like the overall idea of a hook
point that you can use to trace what is going on, and to inject the logic you
want.

https://codereview.appspot.com/7204055/diff/1/testservices/service.go
File testservices/service.go (right):

https://codereview.appspot.com/7204055/diff/1/testservices/service.go#newcode65
testservices/service.go:65: // }
I can't help but feel that something like "nil" would be clearer than the empty
string. But I think we're stuck with what we have.

Alternatively, if most of the time we're just going to be grabbing the function
name, maybe we want to make that the obvious one, and add a separate function
when we have to?

https://codereview.appspot.com/7204055/diff/1/testservices/service_test.go
File testservices/service_test.go (right):

https://codereview.appspot.com/7204055/diff/1/testservices/service_test.go#ne...
testservices/service_test.go:50: func nonFunctionControlHook(s ServiceControl,
args ...interface{}) error {
I don't quite understand why this is "nonFunctionControlHook", is it meant to be
"named" hook, or something like that?

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
File tools/secgroup-delete-all/main_test.go (left):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
tools/secgroup-delete-all/main_test.go:47: openstack := openstack.New(creds)
I'd like us to generally avoid using variable names that match module names. It
gets confusing to figure out which "openstack" thing is being operated on.

Not that you added this, but I've seen it in a couple places, and was a bit
confused by it.

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
File tools/secgroup-delete-all/main_test.go (right):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
tools/secgroup-delete-all/main_test.go:75: if groupId == 2 {
This seems a bit risky to have it always work (comparing an interface{} to an
integer).
Also, it would be nicer if we at least compared to the group name, rather than
to an integer id. Is it possible to get more context for the hook? I realize
this is what we have today, but it seems like it would be really easy to
accidentally break (changing the numbering of the groups, adding a default
group, etc.)

Maybe if we did it using a local function, so we can use the result from
nova.CreateSecurityGroup rather than just '2'.

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
tools/secgroup-delete-all/main_test.go:84: nova.CreateSecurityGroup("group-b",
"Another group")
I think this would end up looking like:

doNotDelete = nova.CreateSecurityGroup("group-b", "Another group")

deleteError = func(s testServices.ServiceControl, args ...interface{}) error {
  groupId := args[0] 
  if groupId == doNotDelete.Id {
    return fmt.Errorf("cannot delete group %d", groupId)
  }
  return nil
}
os.Nova.RegisterControlPoint("removeSecurityGroup", deleteError)
defer os.Nova.RegisterControlPoint("removeSecurityGroup", nil)

I also wonder if it would be nice to have os.Nova.RegisterControlPoint() return
a callable that you can use to cleanup. So it would look like:

cleanup := os.Nova.RegisterControlPoint("removeSecurityGroup", deleteError)
defer cleanup()

It's nice in python, I'm not sure if it is nice in go.

[wallyworld, 2013-01-29 00:46:34]

Please take a look.

https://codereview.appspot.com/7204055/diff/1/testservices/service.go
File testservices/service.go (right):

https://codereview.appspot.com/7204055/diff/1/testservices/service.go#newcode65
testservices/service.go:65: // }
On 2013/01/28 06:13:58, jameinel wrote:
> I can't help but feel that something like "nil" would be clearer than the
empty
> string. But I think we're stuck with what we have.
> 

Welcome to Go :-(

> Alternatively, if most of the time we're just going to be grabbing the
function
> name, maybe we want to make that the obvious one, and add a separate function
> when we have to?

I thought about it (and cursed and lamented the lack of default parameters in
Go). The Go Way seems to be though to stick to a single method and pass in
default parameter values all the time, from what I can tell.

https://codereview.appspot.com/7204055/diff/1/testservices/service_test.go
File testservices/service_test.go (right):

https://codereview.appspot.com/7204055/diff/1/testservices/service_test.go#ne...
testservices/service_test.go:50: func nonFunctionControlHook(s ServiceControl,
args ...interface{}) error {
On 2013/01/28 06:13:58, jameinel wrote:
> I don't quite understand why this is "nonFunctionControlHook", is it meant to
be
> "named" hook, or something like that?

"namedControlHook" is much nicer, will use that

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
File tools/secgroup-delete-all/main_test.go (left):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
tools/secgroup-delete-all/main_test.go:47: openstack := openstack.New(creds)
On 2013/01/28 06:13:58, jameinel wrote:
> I'd like us to generally avoid using variable names that match module names.
It
> gets confusing to figure out which "openstack" thing is being operated on.
> 
> Not that you added this, but I've seen it in a couple places, and was a bit
> confused by it.

No, it was me who added it in a previous branch from memory.
I'll change the variable name. Perhaps I should have called the package
"testopenstack" but then there's already a "testservices" in the import path and
it seems redundant. We call the nova double "novaservice" but "openstackservice"
seems too long. But in the juju-core code, the production openstack provider
package is just called "openstack" and so disambiguation is needed there. So
just "openstack" in this case does seem like it is not the best choice in
hindsight.

I'll change it to "openstackservice" and see if I get agreement on that.

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
File tools/secgroup-delete-all/main_test.go (right):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
tools/secgroup-delete-all/main_test.go:75: if groupId == 2 {
On 2013/01/28 06:13:58, jameinel wrote:
> This seems a bit risky to have it always work (comparing an interface{} to an
> integer).

I've added a cast. Note though that my expectation here is that the author of
the hook is expected to know the signature of the function being processed and
hence what the arguments will be.

> Also, it would be nicer if we at least compared to the group name, rather than
> to an integer id. Is it possible to get more context for the hook? I realize
> this is what we have today, but it seems like it would be really easy to
> accidentally break (changing the numbering of the groups, adding a default
> group, etc.)

Fair point. I've adopted the solution you suggested below.

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_...
tools/secgroup-delete-all/main_test.go:84: nova.CreateSecurityGroup("group-b",
"Another group")
On 2013/01/28 06:13:58, jameinel wrote:
> 
> I also wonder if it would be nice to have os.Nova.RegisterControlPoint()
return
> a callable that you can use to cleanup. So it would look like:
> 
> cleanup := os.Nova.RegisterControlPoint("removeSecurityGroup", deleteError)
> defer cleanup()
> 
> It's nice in python, I'm not sure if it is nice in go.

I like this suggestion and implemented it.

[jameinel, 2013-01-29 11:56:57]

LGTM

[wallyworld, 2013-01-29 13:15:37]

*** Submitted:

Error testing infrastructure for service doubles

This branch provides infrastructure to allow tests to specify that the service
doubles should return an error at various points.
The basic premise is that the test provides a hook which is run at the specfied
point in time eg when a specific function runs. 
The hook is passed the service double and any function arguments. It can inspect
the state of the service and the function args and
can decide to return an error if it wants to.

The diff conatins a practical example of this being used - 3 security groups are
created and an error is raised when the second one is deleted.

This work doesn't have to be used to raise an error - the hook can also be used
to modify the sate of the service double and simply exit without error.

R=jameinel
CC=
https://codereview.appspot.com/7204055