Merge lp:~wallyworld/goose/control-test-doubles into lp:~gophers/goose/trunk

[Ian Booth]

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://codereview.appspot.com/7204055/

[Ian Booth (wallyworld)]

Reviewers: mp+144838_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/7204055/

Affected files:
   A [revision details]
   M testservices/novaservice/service.go
   M testservices/openstack/openstack.go
   M testservices/service.go
   A testservices/service_test.go
   M tools/secgroup-delete-all/main_test.go

[John A Meinel (jameinel)]

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#newcode50
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_test.go
File tools/secgroup-delete-all/main_test.go (left):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#oldcode47
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_test.go
File tools/secgroup-delete-all/main_test.go (right):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#newcode75
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_test.go#newcode84
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...

[lp:~wallyworld/goose/control-test-doubles]

54. By Ian Booth on 2013-01-29

Code review fixes

[Ian Booth (wallyworld)]

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#newcode50
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_test.go
File tools/secgroup-delete-all/main_test.go (left):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#oldcode47
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_test.go
File tools/secgroup-delete-all/main_test.go (right):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#newcode75
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.

> ...

[John A Meinel (jameinel)]

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

I'm not terribly concerned about the name of the module. "openstackservice" is fine with me.

What I'm more concerned with is that we don't name a variable that matches the name of an imported module.

The line in question was:

openstack = openstack.New(creds)

Note that this isn't "s.openstack = " or anything like that. Just doing "os = openstack.New()" would be enough for me.

+var doNotDelete *nova.SecurityGroup

I generally prefer to use a closure (eg func() ...) rather than global state like this.

Otherwise LGTM. Just a note that you need to use 'lbox propose' again to update the reitveld diff. It will also push to launchpad to update the MP.

review: Approve

[John A Meinel (jameinel)]

I must have clicked on the wrong link, it looks like the reitveld diff is updated.

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/7204055/

[Ian Booth (wallyworld)]

*** 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

https://codereview.appspot.com/7204055/

=== modified file 'client/local_test.go'
--- client/local_test.go	2013-01-24 03:15:19 +0000
+++ client/local_test.go	2013-01-29 00:47:20 +0000
@@ -6,7 +6,7 @@
 	"launchpad.net/goose/testing/httpsuite"
 	"launchpad.net/goose/testservices"
 	"launchpad.net/goose/testservices/identityservice"
-	"launchpad.net/goose/testservices/openstack"
+	"launchpad.net/goose/testservices/openstackservice"
 )
 
 func registerLocalTests(authMethods []identity.AuthMethod) {
@@ -43,7 +43,7 @@
 		panic("Invalid authentication method")
 	case identity.AuthUserPass:
 		// The openstack test service sets up userpass authentication.
-		s.service = openstack.New(s.cred)
+		s.service = openstackservice.New(s.cred)
 	case identity.AuthLegacy:
 		legacy := identityservice.NewLegacy()
 		legacy.AddUser(s.cred.User, s.cred.Secrets, s.cred.TenantName)

=== modified file 'nova/local_test.go'
--- nova/local_test.go	2013-01-24 01:01:35 +0000
+++ nova/local_test.go	2013-01-29 00:47:20 +0000
@@ -7,7 +7,7 @@
 	"launchpad.net/goose/errors"
 	"launchpad.net/goose/identity"
 	"launchpad.net/goose/nova"
-	"launchpad.net/goose/testservices/openstack"
+	"launchpad.net/goose/testservices/openstackservice"
 	"log"
 	"net/http"
 	"net/http/httptest"
@@ -45,7 +45,7 @@
 		Region:     "some region",
 		TenantName: "tenant",
 	}
-	openstack := openstack.New(s.cred)
+	openstack := openstackservice.New(s.cred)
 	openstack.SetupHTTP(s.Mux)
 
 	s.LiveTests.SetUpSuite(c)

=== modified file 'swift/local_test.go'
--- swift/local_test.go	2013-01-24 01:01:35 +0000
+++ swift/local_test.go	2013-01-29 00:47:20 +0000
@@ -4,7 +4,7 @@
 	. "launchpad.net/gocheck"
 	"launchpad.net/goose/identity"
 	"launchpad.net/goose/testing/httpsuite"
-	"launchpad.net/goose/testservices/openstack"
+	"launchpad.net/goose/testservices/openstackservice"
 )
 
 func registerLocalTests() {
@@ -18,7 +18,7 @@
 	LiveTestsPublicContainer
 	// The following attributes are for using testing doubles.
 	httpsuite.HTTPSuite
-	openstack *openstack.Openstack
+	openstack *openstackservice.Openstack
 }
 
 func (s *localLiveSuite) SetUpSuite(c *C) {
@@ -33,7 +33,7 @@
 		TenantName: "tenant",
 	}
 	s.LiveTestsPublicContainer.cred = s.LiveTests.cred
-	s.openstack = openstack.New(s.LiveTests.cred)
+	s.openstack = openstackservice.New(s.LiveTests.cred)
 
 	s.LiveTests.SetUpSuite(c)
 	s.LiveTestsPublicContainer.SetUpSuite(c)

=== modified file 'testservices/novaservice/service.go'
--- testservices/novaservice/service.go	2013-01-24 03:15:19 +0000
+++ testservices/novaservice/service.go	2013-01-29 00:47:20 +0000
@@ -91,6 +91,7 @@
 			VersionPath:     versionPath,
 			TenantId:        tenantId,
 			Region:          region,
+			ControlHooks:    make(map[string]testservices.ControlProcessor),
 		},
 	}
 	if identityService != nil {
@@ -340,6 +341,9 @@
 
 // removeSecurityGroup deletes an existing group.
 func (n *Nova) removeSecurityGroup(groupId int) error {
+	if err := n.ProcessControlHook("", n, groupId); err != nil {
+		return err
+	}
 	if _, err := n.securityGroup(groupId); err != nil {
 		return err
 	}

=== renamed directory 'testservices/openstack' => 'testservices/openstackservice'
=== modified file 'testservices/openstackservice/openstack.go'
--- testservices/openstack/openstack.go	2013-01-24 01:01:35 +0000
+++ testservices/openstackservice/openstack.go	2013-01-29 00:47:20 +0000
@@ -1,4 +1,4 @@
-package openstack
+package openstackservice
 
 import (
 	"launchpad.net/goose/identity"
@@ -10,29 +10,29 @@
 
 // Openstack provides an Openstack service double implementation.
 type Openstack struct {
-	identity identityservice.IdentityService
-	nova     *novaservice.Nova
-	swift    *swiftservice.Swift
+	Identity identityservice.IdentityService
+	Nova     *novaservice.Nova
+	Swift    *swiftservice.Swift
 }
 
 // New creates an instance of a full Openstack service double.
 // An initial user with the specified credentials is registered with the identity service.
 func New(cred *identity.Credentials) *Openstack {
 	openstack := Openstack{
-		identity: identityservice.NewUserPass(),
+		Identity: identityservice.NewUserPass(),
 	}
-	userInfo := openstack.identity.AddUser(cred.User, cred.Secrets, cred.TenantName)
+	userInfo := openstack.Identity.AddUser(cred.User, cred.Secrets, cred.TenantName)
 	if cred.TenantName == "" {
 		panic("Openstack service double requires a tenant to be specified.")
 	}
-	openstack.nova = novaservice.New(cred.URL, "v2", userInfo.TenantId, cred.Region, openstack.identity)
-	openstack.swift = swiftservice.New(cred.URL, "v1", userInfo.TenantId, cred.Region, openstack.identity)
+	openstack.Nova = novaservice.New(cred.URL, "v2", userInfo.TenantId, cred.Region, openstack.Identity)
+	openstack.Swift = swiftservice.New(cred.URL, "v1", userInfo.TenantId, cred.Region, openstack.Identity)
 	return &openstack
 }
 
 // setupHTTP attaches all the needed handlers to provide the HTTP API for the Openstack service..
 func (openstack *Openstack) SetupHTTP(mux *http.ServeMux) {
-	openstack.identity.SetupHTTP(mux)
-	openstack.nova.SetupHTTP(mux)
-	openstack.swift.SetupHTTP(mux)
+	openstack.Identity.SetupHTTP(mux)
+	openstack.Nova.SetupHTTP(mux)
+	openstack.Swift.SetupHTTP(mux)
 }

=== modified file 'testservices/service.go'
--- testservices/service.go	2013-01-24 03:15:19 +0000
+++ testservices/service.go	2013-01-29 00:47:20 +0000
@@ -3,6 +3,8 @@
 import (
 	"launchpad.net/goose/testservices/identityservice"
 	"net/http"
+	"runtime"
+	"strings"
 )
 
 // An HttpService provides the HTTP API for a service double.
@@ -13,9 +15,78 @@
 // A ServiceInstance is an Openstack module, one of nova, swift, glance.
 type ServiceInstance struct {
 	identityservice.ServiceProvider
+	ServiceControl
 	IdentityService identityservice.IdentityService
 	Hostname        string
 	VersionPath     string
 	TenantId        string
 	Region          string
+	// Hooks to run when specified control points are reached in the service business logic.
+	ControlHooks map[string]ControlProcessor
+}
+
+// ControlProcessor defines a function that is run when a specified control point is reached in the service
+// business logic. The function receives the service instance so internal state can be inspected, plus for any
+// arguments passed to the currently executing service function.
+type ControlProcessor func(sc ServiceControl, args ...interface{}) error
+
+// ControlHookCleanup defines a function used to remove a control hook.
+type ControlHookCleanup func()
+
+// ServiceControl instances allow hooks to be registered for execution at the specified point of execution.
+// The control point name can be a function name or a logical execution point meaningful to the service.
+// If name is "", the hook for the currently executing function is executed.
+// Returns a function which can be used to remove the hook.
+type ServiceControl interface {
+	RegisterControlPoint(name string, controller ControlProcessor) ControlHookCleanup
+}
+
+// currentServiceMethodName returns the method executing on the service when ProcessControlHook was invoked.
+func (s *ServiceInstance) currentServiceMethodName() string {
+	pc, _, _, ok := runtime.Caller(2)
+	if !ok {
+		panic("current method name cannot be found")
+	}
+	return unqualifiedMethodName(pc)
+}
+
+func unqualifiedMethodName(pc uintptr) string {
+	f := runtime.FuncForPC(pc)
+	fullName := f.Name()
+	nameParts := strings.Split(fullName, ".")
+	return nameParts[len(nameParts)-1]
+}
+
+// ProcessControlHook retrieves the ControlProcessor for the specified hook name and runs it, returning any error.
+// Use it like this with a "" hookName to invoke a hook registered for the current function:
+// if err := n.ProcessControlHook("", <serviceinstance>, <somearg1>, <somearg2>); err != nil {
+//     return err
+// }
+//
+// Use it like this to invoke a hook registered for some arbitrary control point:
+// if err := n.ProcessControlHook("foobar", <serviceinstance>, <somearg1>, <somearg2>); err != nil {
+//     return err
+// }
+func (s *ServiceInstance) ProcessControlHook(hookName string, sc ServiceControl, args ...interface{}) error {
+	if hookName == "" {
+		hookName = s.currentServiceMethodName()
+	}
+	if hook, ok := s.ControlHooks[hookName]; ok {
+		return hook(sc, args...)
+	}
+	return nil
+}
+
+// RegisterControlPoint assigns the specified controller to the named hook. If nil, any existing controller for the
+// hook is removed.
+// hookName is the name of a function on the service or some arbitrarily named control point.
+func (s *ServiceInstance) RegisterControlPoint(hookName string, controller ControlProcessor) ControlHookCleanup {
+	if controller == nil {
+		delete(s.ControlHooks, hookName)
+	} else {
+		s.ControlHooks[hookName] = controller
+	}
+	return func() {
+		s.RegisterControlPoint(hookName, nil)
+	}
 }

=== added file 'testservices/service_test.go'
--- testservices/service_test.go	1970-01-01 00:00:00 +0000
+++ testservices/service_test.go	2013-01-29 00:47:20 +0000
@@ -0,0 +1,105 @@
+package testservices
+
+import (
+	"fmt"
+	. "launchpad.net/gocheck"
+	"testing"
+)
+
+func Test(t *testing.T) {
+	TestingT(t)
+}
+
+var _ = Suite(&ServiceSuite{})
+
+type ServiceSuite struct {
+	ts *testService
+}
+
+func (s *ServiceSuite) SetUpTest(c *C) {
+	s.ts = newTestService()
+	// This hook is called based on the function name.
+	s.ts.RegisterControlPoint("foo", functionControlHook)
+	// This hook is called based on a user specified hook name.
+	s.ts.RegisterControlPoint("foobar", namedControlHook)
+}
+
+type testService struct {
+	ServiceInstance
+	label string
+}
+
+func newTestService() *testService {
+	return &testService{
+		ServiceInstance: ServiceInstance{
+			ControlHooks: make(map[string]ControlProcessor),
+		},
+	}
+}
+
+func functionControlHook(s ServiceControl, args ...interface{}) error {
+	label := args[0].(string)
+	returnError := args[1].(bool)
+	if returnError {
+		return fmt.Errorf("An error occurred")
+	}
+	s.(*testService).label = label
+	return nil
+}
+
+func namedControlHook(s ServiceControl, args ...interface{}) error {
+	s.(*testService).label = "foobar"
+	return nil
+}
+
+func (s *testService) foo(label string, returnError bool) error {
+	if err := s.ProcessControlHook("", s, label, returnError); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (s *testService) bar() error {
+	if err := s.ProcessControlHook("foobar", s); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (s *ServiceSuite) TestFunctionHookNoError(c *C) {
+	err := s.ts.foo("success", false)
+	c.Assert(err, IsNil)
+	c.Assert(s.ts.label, Equals, "success")
+}
+
+func (s *ServiceSuite) TestHookWithError(c *C) {
+	err := s.ts.foo("success", true)
+	c.Assert(err, Not(IsNil))
+	c.Assert(s.ts.label, Equals, "")
+}
+
+func (s *ServiceSuite) TestNamedHook(c *C) {
+	err := s.ts.bar()
+	c.Assert(err, IsNil)
+	c.Assert(s.ts.label, Equals, "foobar")
+}
+
+func (s *ServiceSuite) TestHookCleanup(c *C) {
+	// Manually delete the existing control point.
+	s.ts.RegisterControlPoint("foo", nil)
+	// Register a new hook and ensure it works.
+	cleanup := s.ts.RegisterControlPoint("foo", functionControlHook)
+	err := s.ts.foo("cleanuptest", false)
+	c.Assert(err, IsNil)
+	c.Assert(s.ts.label, Equals, "cleanuptest")
+	// Use the cleanup func to remove the hook and check the result.
+	cleanup()
+	err = s.ts.foo("again", false)
+	c.Assert(err, IsNil)
+	c.Assert(s.ts.label, Equals, "cleanuptest")
+	// Ensure that only the specified hook was removed and the other remaining one still works.
+	err = s.ts.bar()
+	c.Assert(err, IsNil)
+	c.Assert(s.ts.label, Equals, "foobar")
+
+}

=== modified file 'tools/secgroup-delete-all/main_test.go'
--- tools/secgroup-delete-all/main_test.go	2013-01-24 01:01:35 +0000
+++ tools/secgroup-delete-all/main_test.go	2013-01-29 00:47:20 +0000
@@ -2,12 +2,14 @@
 
 import (
 	"bytes"
+	"fmt"
 	. "launchpad.net/gocheck"
 	"launchpad.net/goose/client"
 	"launchpad.net/goose/identity"
 	"launchpad.net/goose/nova"
 	"launchpad.net/goose/testing/httpsuite"
-	"launchpad.net/goose/testservices/openstack"
+	"launchpad.net/goose/testservices"
+	"launchpad.net/goose/testservices/openstackservice"
 	tool "launchpad.net/goose/tools/secgroup-delete-all"
 	"testing"
 )
@@ -36,7 +38,7 @@
 	return nova.New(osc)
 }
 
-func (s *ToolSuite) makeServices(c *C) *nova.Client {
+func (s *ToolSuite) makeServices(c *C) (*openstackservice.Openstack, *nova.Client) {
 	creds := &identity.Credentials{
 		URL:        s.Server.URL,
 		User:       username,
@@ -44,13 +46,13 @@
 		Region:     region,
 		TenantName: tenant,
 	}
-	openstack := openstack.New(creds)
+	openstack := openstackservice.New(creds)
 	openstack.SetupHTTP(s.Mux)
-	return createNovaClient(creds)
+	return openstack, createNovaClient(creds)
 }
 
 func (s *ToolSuite) TestNoGroups(c *C) {
-	nova := s.makeServices(c)
+	_, nova := s.makeServices(c)
 	var buf bytes.Buffer
 	err := tool.DeleteAll(&buf, nova)
 	c.Assert(err, IsNil)
@@ -58,7 +60,7 @@
 }
 
 func (s *ToolSuite) TestTwoGroups(c *C) {
-	nova := s.makeServices(c)
+	_, nova := s.makeServices(c)
 	nova.CreateSecurityGroup("group-a", "A group")
 	nova.CreateSecurityGroup("group-b", "Another group")
 	var buf bytes.Buffer
@@ -67,5 +69,27 @@
 	c.Assert(string(buf.Bytes()), Equals, "2 security groups deleted.\n")
 }
 
-// GZ 2013-01-21: Should also test undeleteable groups, but can't induce
-//                novaservice errors currently.
+// This group is one for which we will simulate a deletion error in the following test.
+var doNotDelete *nova.SecurityGroup
+
+// deleteGroupError hook raises an error if a group with id 2 is deleted.
+func deleteGroupError(s testservices.ServiceControl, args ...interface{}) error {
+	groupId := args[0].(int)
+	if groupId == doNotDelete.Id {
+		return fmt.Errorf("cannot delete group %d", groupId)
+	}
+	return nil
+}
+
+func (s *ToolSuite) TestUndeleteableGroup(c *C) {
+	os, nova := s.makeServices(c)
+	nova.CreateSecurityGroup("group-a", "A group")
+	doNotDelete, _ = nova.CreateSecurityGroup("group-b", "Another group")
+	nova.CreateSecurityGroup("group-c", "Yet another group")
+	cleanup := os.Nova.RegisterControlPoint("removeSecurityGroup", deleteGroupError)
+	defer cleanup()
+	var buf bytes.Buffer
+	err := tool.DeleteAll(&buf, nova)
+	c.Assert(err, IsNil)
+	c.Assert(string(buf.Bytes()), Equals, "2 security groups deleted.\n1 security groups could not be deleted.\n")
+}