all: prefer AddCleanup to TearDown

A common failure mode of our tests is when an issue occurs during the SetUp phase of the suite or test, not only does the test fail, but the Tear down phase blows up because it is rarely written to handle the case where the setup phase did not complete. This PR is a toe in the water to experiment with applying a philosophy of moving work from the TearDown phase of the test suite to a 'defer-like' AddCleanup style.
sdg0919 · Apr 7, 2016 · 4b82e76 · 4b82e76
1 parent 64d8cdc
commit 4b82e76
Show file tree

Hide file tree

Showing 14 changed files with 35 additions and 82 deletions.
diff --git a/apiserver/addresser/addresser_test.go b/apiserver/addresser/addresser_test.go
@@ -58,11 +58,7 @@ func (s *AddresserSuite) SetUpTest(c *gc.C) {
 	var err error
 	s.api, err = addresser.NewAddresserAPI(nil, s.resources, s.authoriser)
 	c.Assert(err, jc.ErrorIsNil)
-}
-
-func (s *AddresserSuite) TearDownTest(c *gc.C) {
-	dummy.Reset(c)
-	s.BaseSuite.TearDownTest(c)
+	s.AddCleanup(dummy.Reset)
 }
 
 func (s *AddresserSuite) TestCanDeallocateAddressesEnabled(c *gc.C) {

diff --git a/apiserver/authentication/user_test.go b/apiserver/authentication/user_test.go
@@ -142,22 +142,13 @@ func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) {
 
 type macaroonAuthenticatorSuite struct {
 	jujutesting.JujuConnSuite
-	discharger *bakerytest.Discharger
 	// username holds the username that will be
 	// declared in the discharger's caveats.
 	username string
 }
 
 var _ = gc.Suite(&macaroonAuthenticatorSuite{})
 
-func (s *macaroonAuthenticatorSuite) SetUpTest(c *gc.C) {
-	s.discharger = bakerytest.NewDischarger(nil, s.Checker)
-}
-
-func (s *macaroonAuthenticatorSuite) TearDownTest(c *gc.C) {
-	s.discharger.Close()
-}
-
 func (s *macaroonAuthenticatorSuite) Checker(req *http.Request, cond, arg string) ([]checkers.Caveat, error) {
 	return []checkers.Caveat{checkers.DeclaredCaveat("username", s.username)}, nil
 }
@@ -207,19 +198,21 @@ var authenticateSuccessTests = []struct {
 }}
 
 func (s *macaroonAuthenticatorSuite) TestMacaroonAuthentication(c *gc.C) {
+	discharger := bakerytest.NewDischarger(nil, s.Checker)
+	defer discharger.Close()
 	for i, test := range authenticateSuccessTests {
 		c.Logf("\ntest %d; %s", i, test.about)
 		s.username = test.dischargedUsername
 
 		svc, err := bakery.NewService(bakery.NewServiceParams{
-			Locator: s.discharger,
+			Locator: discharger,
 		})
 		c.Assert(err, jc.ErrorIsNil)
 		mac, err := svc.NewMacaroon("", nil, nil)
 		c.Assert(err, jc.ErrorIsNil)
 		authenticator := &authentication.ExternalMacaroonAuthenticator{
 			Service:          svc,
-			IdentityLocation: s.discharger.Location(),
+			IdentityLocation: discharger.Location(),
 			Macaroon:         mac,
 		}
 

diff --git a/container/lxc/lxc_test.go b/container/lxc/lxc_test.go
@@ -95,12 +95,10 @@ func (s *LxcSuite) SetUpTest(c *gc.C) {
 	s.PatchValue(&lxc.TemplateLockDir, c.MkDir())
 	s.PatchValue(&lxc.TemplateStopTimeout, 500*time.Millisecond)
 	s.loopDeviceManager = mockLoopDeviceManager{}
-}
-
-func (s *LxcSuite) TearDownTest(c *gc.C) {
-	s.TestSuite.ContainerFactory.RemoveListener(s.events)
-	close(s.events)
-	s.TestSuite.TearDownTest(c)
+	s.AddCleanup(func(*gc.C) {
+		s.TestSuite.ContainerFactory.RemoveListener(s.events)
+		close(s.events)
+	})
 }
 
 func (t *LxcSuite) TestPreferFastLXC(c *gc.C) {

diff --git a/environs/config/authkeys_test.go b/environs/config/authkeys_test.go
@@ -30,17 +30,16 @@ func (s *AuthKeysSuite) SetUpTest(c *gc.C) {
 	old := utils.Home()
 	newhome := c.MkDir()
 	utils.SetHome(newhome)
-	s.AddCleanup(func(*gc.C) { utils.SetHome(old) })
+	s.AddCleanup(func(*gc.C) {
+		ssh.ClearClientKeys()
+		utils.SetHome(old)
+	})
+
 	s.dotssh = filepath.Join(newhome, ".ssh")
 	err := os.Mkdir(s.dotssh, 0755)
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *AuthKeysSuite) TearDownTest(c *gc.C) {
-	ssh.ClearClientKeys()
-	s.BaseSuite.TearDownTest(c)
-}
-
 func (s *AuthKeysSuite) TestReadAuthorizedKeysErrors(c *gc.C) {
 	_, err := config.ReadAuthorizedKeys("")
 	c.Assert(err, gc.ErrorMatches, "no public ssh keys found")

diff --git a/environs/config_test.go b/environs/config_test.go
@@ -20,9 +20,9 @@ type suite struct {
 
 var _ = gc.Suite(&suite{})
 
-func (s *suite) TearDownTest(c *gc.C) {
-	dummy.Reset(c)
-	s.FakeJujuXDGDataHomeSuite.TearDownTest(c)
+func (s *suite) SetUpTest(c *gc.C) {
+	s.FakeJujuXDGDataHomeSuite.SetUpTest(c)
+	s.AddCleanup(dummy.Reset)
 }
 
 // dummySampleConfig returns the dummy sample config without

diff --git a/featuretests/api_model_test.go b/featuretests/api_model_test.go
@@ -30,15 +30,11 @@ type apiEnvironmentSuite struct {
 
 func (s *apiEnvironmentSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
-	var err error
 	s.client = s.APIState.Client()
-	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(s.client, gc.NotNil)
-}
-
-func (s *apiEnvironmentSuite) TearDownTest(c *gc.C) {
-	s.client.ClientFacade.Close()
-	s.JujuConnSuite.TearDownTest(c)
+	s.AddCleanup(func(*gc.C) {
+		s.client.ClientFacade.Close()
+	})
 }
 
 func (s *apiEnvironmentSuite) TestGrantModel(c *gc.C) {

diff --git a/featuretests/block_test.go b/featuretests/block_test.go
@@ -22,11 +22,9 @@ func (s *blockSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
 	s.blockClient = block.NewClient(s.APIState)
 	c.Assert(s.blockClient, gc.NotNil)
-}
-
-func (s *blockSuite) TearDownTest(c *gc.C) {
-	s.blockClient.ClientFacade.Close()
-	s.JujuConnSuite.TearDownTest(c)
+	s.AddCleanup(func(*gc.C) {
+		s.blockClient.ClientFacade.Close()
+	})
 }
 
 func (s *blockSuite) TestBlockFacadeCall(c *gc.C) {

diff --git a/featuretests/cloudimagemetadata_test.go b/featuretests/cloudimagemetadata_test.go
@@ -17,20 +17,16 @@ import (
 
 type cloudImageMetadataSuite struct {
 	testing.JujuConnSuite
-
 	client *imagemetadata.Client
 }
 
 func (s *cloudImageMetadataSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
-
 	s.client = imagemetadata.NewClient(s.APIState)
 	c.Assert(s.client, gc.NotNil)
-}
-
-func (s *cloudImageMetadataSuite) TearDownTest(c *gc.C) {
-	s.client.ClientFacade.Close()
-	s.JujuConnSuite.TearDownTest(c)
+	s.AddCleanup(func(*gc.C) {
+		s.client.ClientFacade.Close()
+	})
 }
 
 func (s *cloudImageMetadataSuite) TestSaveAndFindAndDeleteMetadata(c *gc.C) {

diff --git a/juju/deploy_test.go b/juju/deploy_test.go
@@ -43,12 +43,7 @@ var _ = gc.Suite(&DeployLocalSuite{})
 func (s *DeployLocalSuite) SetUpSuite(c *gc.C) {
 	s.JujuConnSuite.SetUpSuite(c)
 	s.repo = &charmrepo.LocalRepository{Path: testcharms.Repo.Path()}
-	s.oldCacheDir, charmrepo.CacheDir = charmrepo.CacheDir, c.MkDir()
-}
-
-func (s *DeployLocalSuite) TearDownSuite(c *gc.C) {
-	charmrepo.CacheDir = s.oldCacheDir
-	s.JujuConnSuite.TearDownSuite(c)
+	s.PatchValue(&charmrepo.CacheDir, c.MkDir())
 }
 
 func (s *DeployLocalSuite) SetUpTest(c *gc.C) {

diff --git a/provider/cloudsigma/client_test.go b/provider/cloudsigma/client_test.go
@@ -34,11 +34,9 @@ var _ = gc.Suite(&clientSuite{})
 func (s *clientSuite) SetUpSuite(c *gc.C) {
 	s.BaseSuite.SetUpSuite(c)
 	mock.Start()
-}
-
-func (s *clientSuite) TearDownSuite(c *gc.C) {
-	mock.Stop()
-	s.BaseSuite.TearDownSuite(c)
+	s.AddCleanup(func(*gc.C) {
+		mock.Stop()
+	})
 }
 
 func (s *clientSuite) SetUpTest(c *gc.C) {

diff --git a/watcher/legacy/stringsworker_test.go b/watcher/legacy/stringsworker_test.go
@@ -51,11 +51,7 @@ func newStringsHandlerWorker(c *gc.C, setupError, handlerError, teardownError er
 func (s *stringsWorkerSuite) SetUpTest(c *gc.C) {
 	s.BaseSuite.SetUpTest(c)
 	s.actor, s.worker = newStringsHandlerWorker(c, nil, nil, nil)
-}
-
-func (s *stringsWorkerSuite) TearDownTest(c *gc.C) {
-	s.stopWorker(c)
-	s.BaseSuite.TearDownTest(c)
+	s.AddCleanup(s.stopWorker)
 }
 
 type stringsHandler struct {

diff --git a/watcher/notify_test.go b/watcher/notify_test.go
@@ -48,11 +48,7 @@ func newNotifyHandlerWorker(c *gc.C, setupError, handlerError, teardownError err
 func (s *notifyWorkerSuite) SetUpTest(c *gc.C) {
 	s.BaseSuite.SetUpTest(c)
 	s.actor, s.worker = newNotifyHandlerWorker(c, nil, nil, nil)
-}
-
-func (s *notifyWorkerSuite) TearDownTest(c *gc.C) {
-	s.stopWorker(c)
-	s.BaseSuite.TearDownTest(c)
+	s.AddCleanup(s.stopWorker)
 }
 
 type notifyHandler struct {

diff --git a/watcher/strings_test.go b/watcher/strings_test.go
@@ -48,11 +48,7 @@ func newStringsHandlerWorker(c *gc.C, setupError, handlerError, teardownError er
 func (s *stringsWorkerSuite) SetUpTest(c *gc.C) {
 	s.BaseSuite.SetUpTest(c)
 	s.actor, s.worker = newStringsHandlerWorker(c, nil, nil, nil)
-}
-
-func (s *stringsWorkerSuite) TearDownTest(c *gc.C) {
-	s.stopWorker(c)
-	s.BaseSuite.TearDownTest(c)
+	s.AddCleanup(s.stopWorker)
 }
 
 type stringsHandler struct {

diff --git a/wrench/wrench_test.go b/wrench/wrench_test.go
@@ -38,15 +38,11 @@ func (s *wrenchSuite) SetUpTest(c *gc.C) {
 	s.AddCleanup(func(*gc.C) {
 		s.logWriter.Clear()
 		loggo.RemoveWriter("wrench-tests")
+		// Ensure the wrench is turned off when these tests are done.
+		wrench.SetEnabled(false)
 	})
 }
 
-func (s *wrenchSuite) TearDownSuite(c *gc.C) {
-	s.BaseSuite.TearDownSuite(c)
-	// Ensure the wrench is turned off when these tests are done.
-	wrench.SetEnabled(false)
-}
-
 func (s *wrenchSuite) createWrenchDir(c *gc.C) {
 	s.wrenchDir = c.MkDir()
 	s.PatchValue(wrench.WrenchDir, s.wrenchDir)