Merge lp:~thumper/juju-core/patch-with-cleanup into lp:~go-bot/juju-core/trunk

[Tim Penhey]

PatchEnvironment and PatchValue on cleanup suite

The Set function was moved from testing/checkers to
testing. It wasn't a checker and it has also been
renamed to PatchValue, and in testing/patch.go. The
PatchEnvironment function has been added to that
file too.

Tests were added for PatchEnvironment.

Methods were added to CleanupSuite for PatchValue
and PatchEnvironment that automatically register
the restore functions as cleanup methods.

Call sites of PatchEnvironment and jc.Set have been
updated (although not necessarily all of them).

https://codereview.appspot.com/13770045/

PatchEnvironment and PatchValue on cleanup suite

The Set function was moved from testing/checkers to
testing. It wasn't a checker and it has also been
renamed to PatchValue, and in testing/patch.go. The
PatchEnvironment function has been added to that
file too.

Tests were added for PatchEnvironment.

Methods were added to CleanupSuite for PatchValue
and PatchEnvironment that automatically register
the restore functions as cleanup methods.

Call sites of PatchEnvironment and jc.Set have been
updated (although not necessarily all of them).

https://codereview.appspot.com/13770045/

[Tim Penhey (thumper)]

Reviewers: mp+186464_code.launchpad.net,

Message:
Please take a look.

Description:
PatchEnvironment and PatchValue on cleanup suite

The Set function was moved from testing/checkers to
testing. It wasn't a checker and it has also been
renamed to PatchValue, and in testing/patch.go. The
PatchEnvironment function has been added to that
file too.

Tests were added for PatchEnvironment.

Methods were added to CleanupSuite for PatchValue
and PatchEnvironment that automatically register
the restore functions as cleanup methods.

Call sites of PatchEnvironment and jc.Set have been
updated (although not necessarily all of them).

https://code.launchpad.net/~thumper/juju-core/patch-with-cleanup/+merge/186464

(do not edit description out of merge proposal)

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

Affected files (+158, -81 lines):
   A [revision details]
   M agent/format-1.16_whitebox_test.go
   M cmd/jujud/machine_test.go
   M cmd/logging_test.go
   M container/lxc/lxc_test.go
   M environs/config/config_test.go
   M instance/address_test.go
   M instance/testing/instance.go
   M provider/azure/environ_test.go
   M testing/cleanup.go
   M testing/cleanup_test.go
   M testing/environ.go
   M testing/patch.go
   M testing/patch_test.go
   M worker/provisioner/lxc-broker_test.go
   M worker/uniter/charm/git_test.go
   M worker/uniter/debug/server_test.go

[Ian Booth (wallyworld)]

LGTM. I didn't see the issue you said exists. I'll assume you'll fix it
before landing :-)

https://codereview.appspot.com/13770045/diff/1/testing/cleanup_test.go
File testing/cleanup_test.go (right):

https://codereview.appspot.com/13770045/diff/1/testing/cleanup_test.go#newcode74
testing/cleanup_test.go:74: // and to avoid calling them twice, clear
out the stack
this comment doesn't really make sense - can it be reworded?

https://codereview.appspot.com/13770045/

[Roger Peppe (rogpeppe)]

I just noticed this.

"The Set function was moved from testing/checkers to
testing. It wasn't a checker and it has also been
renamed to PatchValue, and in testing/patch.go. The
PatchEnvironment function has been added to that
file too."

There was a particular reason that I defined Set inside
testing/checkers rather than in testing - dependencies.
There are various places that can't import testing because
it has a particularly hefty set of dependencies.

It may be that "checkers" is not a great name for a package
that includes both checkers and the patch functionality,
but I think there should be a place for our minimal-dependency
testing-related code, and I think that the package we import
as "jc" (whatever its path is) is a nice place for that.

I quite liked the original "jc.Set" name too - easy to type
and nicely indicative of its purpose.

Would you object too horribly if I reverted that part of this
CL, and moved testing.PatchEnvironment to jc.Setenv ?

https://codereview.appspot.com/13770045/

[Andrew Wilkins (axwalk)]

On 2013/09/19 15:15:12, rog wrote:
> I just noticed this.

> "The Set function was moved from testing/checkers to
> testing. It wasn't a checker and it has also been
> renamed to PatchValue, and in testing/patch.go. The
> PatchEnvironment function has been added to that
> file too."

> There was a particular reason that I defined Set inside
> testing/checkers rather than in testing - dependencies.
> There are various places that can't import testing because
> it has a particularly hefty set of dependencies.

> It may be that "checkers" is not a great name for a package
> that includes both checkers and the patch functionality,
> but I think there should be a place for our minimal-dependency
> testing-related code, and I think that the package we import
> as "jc" (whatever its path is) is a nice place for that.

IMHO, if this were in "checkers", then "checkers" needs to change name.
Perhaps a new subpackage is in order for small test utilities. Like,
"testutils" or something. This new subpackage could be required to have
no dependencies other than third-party (gocheck/loggo),
testing/checkers, and stdlib.

> I quite liked the original "jc.Set" name too - easy to type
> and nicely indicative of its purpose.

> Would you object too horribly if I reverted that part of this
> CL, and moved testing.PatchEnvironment to jc.Setenv ?

https://codereview.appspot.com/13770045/

=== modified file 'agent/format-1.16_whitebox_test.go'
--- agent/format-1.16_whitebox_test.go	2013-09-02 22:22:00 +0000
+++ agent/format-1.16_whitebox_test.go	2013-09-19 11:38:26 +0000
@@ -105,13 +105,13 @@
 }
 
 func (s *format_1_16Suite) TestMigrate(c *gc.C) {
-	defer testing.PatchEnvironment(JujuLxcBridge, "lxc bridge")()
-	defer testing.PatchEnvironment(JujuProviderType, "provider type")()
-	defer testing.PatchEnvironment(osenv.JujuContainerType, "container type")()
-	defer testing.PatchEnvironment(JujuStorageDir, "storage dir")()
-	defer testing.PatchEnvironment(JujuStorageAddr, "storage addr")()
-	defer testing.PatchEnvironment(JujuSharedStorageDir, "shared storage dir")()
-	defer testing.PatchEnvironment(JujuSharedStorageAddr, "shared storage addr")()
+	s.PatchEnvironment(JujuLxcBridge, "lxc bridge")
+	s.PatchEnvironment(JujuProviderType, "provider type")
+	s.PatchEnvironment(osenv.JujuContainerType, "container type")
+	s.PatchEnvironment(JujuStorageDir, "storage dir")
+	s.PatchEnvironment(JujuStorageAddr, "storage addr")
+	s.PatchEnvironment(JujuSharedStorageDir, "shared storage dir")
+	s.PatchEnvironment(JujuSharedStorageAddr, "shared storage addr")
 
 	config := s.newConfig(c)
 	s.formatter.migrate(config)
@@ -130,7 +130,7 @@
 }
 
 func (s *format_1_16Suite) TestMigrateOnlySetsExisting(c *gc.C) {
-	defer testing.PatchEnvironment(JujuProviderType, "provider type")()
+	s.PatchEnvironment(JujuProviderType, "provider type")
 
 	config := s.newConfig(c)
 	s.formatter.migrate(config)

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-09-12 14:49:01 +0000
+++ cmd/jujud/machine_test.go	2013-09-19 11:38:26 +0000
@@ -33,7 +33,6 @@
 type MachineSuite struct {
 	agentSuite
 	lxc.TestSuite
-	restoreCacheDir jc.Restorer
 }
 
 var _ = gc.Suite(&MachineSuite{})
@@ -41,11 +40,11 @@
 func (s *MachineSuite) SetUpSuite(c *gc.C) {
 	s.agentSuite.SetUpSuite(c)
 	s.TestSuite.SetUpSuite(c)
-	s.restoreCacheDir = jc.Set(&charm.CacheDir, c.MkDir())
+	restore := testing.PatchValue(&charm.CacheDir, c.MkDir())
+	s.AddSuiteCleanup(func(*gc.C) { restore() })
 }
 
 func (s *MachineSuite) TearDownSuite(c *gc.C) {
-	s.restoreCacheDir()
 	s.TestSuite.TearDownSuite(c)
 	s.agentSuite.TearDownSuite(c)
 }

=== modified file 'cmd/logging_test.go'
--- cmd/logging_test.go	2013-09-17 04:06:28 +0000
+++ cmd/logging_test.go	2013-09-19 11:38:26 +0000
@@ -24,9 +24,8 @@
 var _ = gc.Suite(&LogSuite{})
 
 func (s *LogSuite) SetUpTest(c *gc.C) {
-	restore := testing.PatchEnvironment(osenv.JujuLoggingConfig, "")
+	s.PatchEnvironment(osenv.JujuLoggingConfig, "")
 	s.AddCleanup(func(_ *gc.C) {
-		restore()
 		loggo.ResetLoggers()
 		loggo.ResetWriters()
 	})

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-08-22 07:16:20 +0000
+++ container/lxc/lxc_test.go	2013-09-19 11:38:26 +0000
@@ -17,6 +17,7 @@
 
 	"launchpad.net/juju-core/container/lxc"
 	"launchpad.net/juju-core/instance"
+	instancetest "launchpad.net/juju-core/instance/testing"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
@@ -208,11 +209,11 @@
 
 	result, err := foo.ListContainers()
 	c.Assert(err, gc.IsNil)
-	testing.MatchInstances(c, result, foo1, foo2, foo3)
+	instancetest.MatchInstances(c, result, foo1, foo2, foo3)
 
 	result, err = bar.ListContainers()
 	c.Assert(err, gc.IsNil)
-	testing.MatchInstances(c, result, bar1, bar2)
+	instancetest.MatchInstances(c, result, bar1, bar2)
 }
 
 type NetworkSuite struct {

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2013-09-12 23:37:50 +0000
+++ environs/config/config_test.go	2013-09-19 11:38:26 +0000
@@ -736,9 +736,9 @@
 	}
 }
 
-func (*ConfigSuite) TestConfigAttrs(c *gc.C) {
+func (s *ConfigSuite) TestConfigAttrs(c *gc.C) {
 	// Normally this is handled by testing.FakeHome
-	defer testing.PatchEnvironment(osenv.JujuLoggingConfig, "")()
+	s.PatchEnvironment(osenv.JujuLoggingConfig, "")
 	attrs := map[string]interface{}{
 		"type":                      "my-type",
 		"name":                      "my-name",
@@ -930,10 +930,10 @@
 	c.Assert(config.LoggingConfig(), gc.Equals, logConfig)
 }
 
-func (*ConfigSuite) TestLoggingConfigFromEnvironment(c *gc.C) {
+func (s *ConfigSuite) TestLoggingConfigFromEnvironment(c *gc.C) {
 	defer makeFakeHome(c).Restore()
 	logConfig := "<root>=INFO"
-	defer testing.PatchEnvironment(osenv.JujuLoggingConfig, logConfig)()
+	s.PatchEnvironment(osenv.JujuLoggingConfig, logConfig)
 
 	config := newTestConfig(c, nil)
 	c.Assert(config.LoggingConfig(), gc.Equals, logConfig)

=== modified file 'instance/address_test.go'
--- instance/address_test.go	2013-09-18 04:40:29 +0000
+++ instance/address_test.go	2013-09-19 11:38:26 +0000
@@ -9,10 +9,12 @@
 
 	gc "launchpad.net/gocheck"
 
-	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing"
 )
 
-type AddressSuite struct{}
+type AddressSuite struct {
+	testing.LoggingSuite
+}
 
 var _ = gc.Suite(&AddressSuite{})
 
@@ -151,14 +153,14 @@
 	}
 }
 
-func (*AddressSuite) TestHostAddresses(c *gc.C) {
+func (s *AddressSuite) TestHostAddresses(c *gc.C) {
 	// Mock the call to net.LookupIP made from HostAddresses.
 	var lookupIPs []net.IP
 	var lookupErr error
 	lookupIP := func(addr string) ([]net.IP, error) {
 		return append([]net.IP{}, lookupIPs...), lookupErr
 	}
-	defer jc.Set(&netLookupIP, lookupIP).Restore()
+	s.PatchValue(&netLookupIP, lookupIP)
 
 	// err is only non-nil if net.LookupIP fails.
 	addrs, err := HostAddresses("")

=== added directory 'instance/testing'
=== renamed file 'testing/instance.go' => 'instance/testing/instance.go'
=== modified file 'provider/azure/environ_test.go'
--- provider/azure/environ_test.go	2013-09-19 00:22:15 +0000
+++ provider/azure/environ_test.go	2013-09-19 11:38:26 +0000
@@ -703,8 +703,7 @@
 }
 
 func (s *environSuite) setServiceDeletionConcurrency(nbGoroutines int) {
-	restore := jc.Set(&maxConcurrentDeletes, nbGoroutines)
-	s.AddCleanup(func(*gc.C) { restore() })
+	s.PatchValue(&maxConcurrentDeletes, nbGoroutines)
 }
 
 func (s *environSuite) TestStopInstancesDestroysMachines(c *gc.C) {

=== modified file 'testing/cleanup.go'
--- testing/cleanup.go	2013-09-17 02:46:26 +0000
+++ testing/cleanup.go	2013-09-19 11:38:26 +0000
@@ -40,10 +40,31 @@
 	}
 }
 
+// AddCleanup pushes the cleanup function onto the stack of functions to be
+// called during TearDownTest.
 func (s *CleanupSuite) AddCleanup(cleanup CleanupFunc) {
 	s.testStack = append(s.testStack, cleanup)
 }
 
+// AddSuiteCleanup pushes the cleanup function onto the stack of functions to
+// be called during TearDownSuite.
 func (s *CleanupSuite) AddSuiteCleanup(cleanup CleanupFunc) {
 	s.suiteStack = append(s.suiteStack, cleanup)
 }
+
+// PatchEnvironment sets the environment variable 'name' the the value passed
+// in. The old value is saved and returned to the original value at test tear
+// down time using a cleanup function.
+func (s *CleanupSuite) PatchEnvironment(name, value string) {
+	restore := PatchEnvironment(name, value)
+	s.AddCleanup(func(*gc.C) { restore() })
+}
+
+// PatchValue sets the 'dest' variable the the value passed in. The old value
+// is saved and returned to the original value at test tear down time using a
+// cleanup function. The value must be assignable to the element type of the
+// destination.
+func (s *CleanupSuite) PatchValue(dest, value interface{}) {
+	restore := PatchValue(dest, value)
+	s.AddCleanup(func(*gc.C) { restore() })
+}

=== modified file 'testing/cleanup_test.go'
--- testing/cleanup_test.go	2013-09-17 02:46:26 +0000
+++ testing/cleanup_test.go	2013-09-19 11:38:26 +0000
@@ -1,6 +1,8 @@
 package testing_test
 
 import (
+	"os"
+
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/testing"
@@ -36,7 +38,8 @@
 	s.TearDownSuite(c)
 	c.Assert(order, gc.DeepEquals, []string{"second", "first"})
 
-	// and to avoid calling them twice, clear out the stack.
+	// SetUpSuite resets the cleanup stack, this stops the cleanup functions
+	// being called again.
 	s.SetUpSuite(c)
 }
 
@@ -52,6 +55,58 @@
 	s.TearDownTest(c)
 	c.Assert(order, gc.DeepEquals, []string{"second", "first"})
 
-	// and to avoid calling them twice, clear out the stack.
+	// SetUpTest resets the cleanup stack, this stops the cleanup functions
+	// being called again.
+	s.SetUpTest(c)
+}
+
+func (s *cleanupSuite) TestPatchEnvironment(c *gc.C) {
+	const envName = "TESTING_PATCH_ENVIRONMENT"
+	// remember the old value, and set it to something we can check
+	oldValue := os.Getenv(envName)
+	os.Setenv(envName, "initial")
+
+	s.PatchEnvironment(envName, "new value")
+	// Using check to make sure the environment gets set back properly in the test.
+	c.Check(os.Getenv(envName), gc.Equals, "new value")
+
+	s.TearDownTest(c)
+	c.Check(os.Getenv(envName), gc.Equals, "initial")
+
+	// SetUpTest resets the cleanup stack, this stops the cleanup functions
+	// being called again.
+	s.SetUpTest(c)
+	// explicitly return the envName to the old value
+	os.Setenv(envName, oldValue)
+}
+
+func (s *cleanupSuite) TestPatchValueInt(c *gc.C) {
+	i := 42
+	s.PatchValue(&i, 0)
+	c.Assert(i, gc.Equals, 0)
+
+	s.TearDownTest(c)
+	c.Assert(i, gc.Equals, 42)
+
+	// SetUpTest resets the cleanup stack, this stops the cleanup functions
+	// being called again.
+	s.SetUpTest(c)
+}
+
+func (s *cleanupSuite) TestPatchValueFunction(c *gc.C) {
+	function := func() string {
+		return "original"
+	}
+
+	s.PatchValue(&function, func() string {
+		return "patched"
+	})
+	c.Assert(function(), gc.Equals, "patched")
+
+	s.TearDownTest(c)
+	c.Assert(function(), gc.Equals, "original")
+
+	// SetUpTest resets the cleanup stack, this stops the cleanup functions
+	// being called again.
 	s.SetUpTest(c)
 }

=== modified file 'testing/environ.go'
--- testing/environ.go	2013-09-15 22:35:59 +0000
+++ testing/environ.go	2013-09-19 11:38:26 +0000
@@ -225,12 +225,3 @@
 func MakeMultipleEnvHome(c *gc.C) *FakeHome {
 	return MakeFakeHome(c, MultipleEnvConfig, SampleCertName, "erewhemos-2")
 }
-
-// PatchEnvironment provides a test a simple way to override a single
-// environment variable. A function is returned that will return the
-// environment to what it was before.
-func PatchEnvironment(name, value string) func() {
-	oldValue := os.Getenv(name)
-	os.Setenv(name, value)
-	return func() { os.Setenv(name, oldValue) }
-}

=== renamed file 'testing/checkers/set.go' => 'testing/patch.go'
--- testing/checkers/set.go	2013-09-10 17:36:09 +0000
+++ testing/patch.go	2013-09-19 11:38:26 +0000
@@ -1,6 +1,7 @@
-package checkers
+package testing
 
 import (
+	"os"
 	"reflect"
 )
 
@@ -13,12 +14,10 @@
 	r()
 }
 
-// Set sets the value pointed to by the given
-// destination to the given value, and returns
-// a function to restore it to its original value.
-// The value must be assignable to the element
-// type of the destination.
-func Set(dest, value interface{}) Restorer {
+// PatchValue sets the value pointed to by the given destination to the given
+// value, and returns a function to restore it to its original value.  The
+// value must be assignable to the element type of the destination.
+func PatchValue(dest, value interface{}) Restorer {
 	destv := reflect.ValueOf(dest).Elem()
 	oldv := reflect.New(destv.Type()).Elem()
 	oldv.Set(destv)
@@ -33,3 +32,14 @@
 		destv.Set(oldv)
 	}
 }
+
+// PatchEnvironment provides a test a simple way to override a single
+// environment variable. A function is returned that will return the
+// environment to what it was before.
+func PatchEnvironment(name, value string) Restorer {
+	oldValue := os.Getenv(name)
+	os.Setenv(name, value)
+	return func() {
+		os.Setenv(name, oldValue)
+	}
+}

=== renamed file 'testing/checkers/set_test.go' => 'testing/patch_test.go'
--- testing/checkers/set_test.go	2013-09-13 14:48:13 +0000
+++ testing/patch_test.go	2013-09-19 11:38:26 +0000
@@ -1,55 +1,73 @@
-package checkers_test
+package testing_test
 
 import (
 	"errors"
+	"os"
 
 	gc "launchpad.net/gocheck"
 
-	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing"
 )
 
-type SetSuite struct{}
-
-var _ = gc.Suite(&SetSuite{})
-
-func (*SetSuite) TestSetInt(c *gc.C) {
+type PatchValueSuite struct{}
+
+var _ = gc.Suite(&PatchValueSuite{})
+
+func (*PatchValueSuite) TestSetInt(c *gc.C) {
 	i := 99
-	restore := jc.Set(&i, 88)
+	restore := testing.PatchValue(&i, 88)
 	c.Assert(i, gc.Equals, 88)
 	restore()
 	c.Assert(i, gc.Equals, 99)
 }
 
-func (*SetSuite) TestSetError(c *gc.C) {
+func (*PatchValueSuite) TestSetError(c *gc.C) {
 	oldErr := errors.New("foo")
 	newErr := errors.New("bar")
 	err := oldErr
-	restore := jc.Set(&err, newErr)
+	restore := testing.PatchValue(&err, newErr)
 	c.Assert(err, gc.Equals, newErr)
 	restore()
 	c.Assert(err, gc.Equals, oldErr)
 }
 
-func (*SetSuite) TestSetErrorToNil(c *gc.C) {
+func (*PatchValueSuite) TestSetErrorToNil(c *gc.C) {
 	oldErr := errors.New("foo")
 	err := oldErr
-	restore := jc.Set(&err, nil)
+	restore := testing.PatchValue(&err, nil)
 	c.Assert(err, gc.Equals, nil)
 	restore()
 	c.Assert(err, gc.Equals, oldErr)
 }
 
-func (*SetSuite) TestSetMapToNil(c *gc.C) {
+func (*PatchValueSuite) TestSetMapToNil(c *gc.C) {
 	oldMap := map[string]int{"foo": 1234}
 	m := oldMap
-	restore := jc.Set(&m, nil)
+	restore := testing.PatchValue(&m, nil)
 	c.Assert(m, gc.IsNil)
 	restore()
 	c.Assert(m, gc.DeepEquals, oldMap)
 }
 
-func (*SetSuite) TestSetPanicsWhenNotAssignable(c *gc.C) {
+func (*PatchValueSuite) TestSetPanicsWhenNotAssignable(c *gc.C) {
 	i := 99
 	type otherInt int
-	c.Assert(func() { jc.Set(&i, otherInt(88)) }, gc.PanicMatches, `reflect\.Set: value of type checkers_test\.otherInt is not assignable to type int`)
+	c.Assert(func() { testing.PatchValue(&i, otherInt(88)) }, gc.PanicMatches, `reflect\.Set: value of type testing_test\.otherInt is not assignable to type int`)
+}
+
+type PatchEnvironmentSuite struct{}
+
+var _ = gc.Suite(&PatchEnvironmentSuite{})
+
+func (*PatchEnvironmentSuite) TestPatchEnvironment(c *gc.C) {
+	const envName = "TESTING_PATCH_ENVIRONMENT"
+	// remember the old value, and set it to something we can check
+	oldValue := os.Getenv(envName)
+	os.Setenv(envName, "initial")
+	restore := testing.PatchEnvironment(envName, "new value")
+	// Using check to make sure the environment gets set back properly in the test.
+	c.Check(os.Getenv(envName), gc.Equals, "new value")
+	restore()
+	c.Check(os.Getenv(envName), gc.Equals, "initial")
+	os.Setenv(envName, oldValue)
 }

=== modified file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	2013-08-30 05:21:55 +0000
+++ worker/provisioner/lxc-broker_test.go	2013-09-19 11:38:26 +0000
@@ -20,6 +20,7 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
+	instancetest "launchpad.net/juju-core/instance/testing"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/provider"
@@ -163,7 +164,7 @@
 func (s *lxcBrokerSuite) assertInstances(c *gc.C, inst ...instance.Instance) {
 	results, err := s.broker.AllInstances()
 	c.Assert(err, gc.IsNil)
-	coretesting.MatchInstances(c, results, inst...)
+	instancetest.MatchInstances(c, results, inst...)
 }
 
 func (s *lxcBrokerSuite) lxcContainerDir(inst instance.Instance) string {

=== modified file 'worker/uniter/charm/git_test.go'
--- worker/uniter/charm/git_test.go	2013-08-16 02:59:08 +0000
+++ worker/uniter/charm/git_test.go	2013-09-19 11:38:26 +0000
@@ -22,7 +22,6 @@
 type GitDirSuite struct {
 	testing.GitSuite
 	LoggingSuite testing.LoggingSuite
-	resetLcAll   func()
 }
 
 var _ = gc.Suite(&GitDirSuite{})
@@ -30,11 +29,10 @@
 func (s *GitDirSuite) SetUpTest(c *gc.C) {
 	s.GitSuite.SetUpTest(c)
 	s.LoggingSuite.SetUpTest(c)
-	s.resetLcAll = testing.PatchEnvironment("LC_ALL", "en_US")
+	s.LoggingSuite.PatchEnvironment("LC_ALL", "en_US")
 }
 
 func (s *GitDirSuite) TearDownTest(c *gc.C) {
-	s.resetLcAll()
 	s.LoggingSuite.TearDownTest(c)
 	s.GitSuite.TearDownTest(c)
 }

=== modified file 'worker/uniter/debug/server_test.go'
--- worker/uniter/debug/server_test.go	2013-09-16 01:55:29 +0000
+++ worker/uniter/debug/server_test.go	2013-09-19 11:38:26 +0000
@@ -14,14 +14,15 @@
 
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type DebugHooksServerSuite struct {
+	testing.LoggingSuite
 	ctx     *HooksContext
 	fakebin string
 	tmpdir  string
-	oldenv  []string
 }
 
 var _ = gc.Suite(&DebugHooksServerSuite{})
@@ -35,33 +36,19 @@
 
 var fakecommands = []string{"tmux"}
 
-func (s *DebugHooksServerSuite) setenv(key, value string) {
-	s.oldenv = append(s.oldenv, key, os.Getenv(key))
-	os.Setenv(key, value)
-}
-
 func (s *DebugHooksServerSuite) SetUpTest(c *gc.C) {
 	s.fakebin = c.MkDir()
 	s.tmpdir = c.MkDir()
-	s.setenv("PATH", s.fakebin+":"+os.Getenv("PATH"))
-	s.setenv("TMPDIR", s.tmpdir)
-	s.setenv("TEST_RESULT", "")
+	s.PatchEnvironment("PATH", s.fakebin+":"+os.Getenv("PATH"))
+	s.PatchEnvironment("TMPDIR", s.tmpdir)
+	s.PatchEnvironment("TEST_RESULT", "")
 	for _, name := range fakecommands {
 		err := ioutil.WriteFile(filepath.Join(s.fakebin, name), []byte(echocommand), 0777)
 		c.Assert(err, gc.IsNil)
 	}
 	s.ctx = NewHooksContext("foo/8")
 	s.ctx.FlockDir = s.tmpdir
-	s.setenv("JUJU_UNIT_NAME", s.ctx.Unit)
-}
-
-func (s *DebugHooksServerSuite) TearDownTest(c *gc.C) {
-	if len(s.oldenv) > 0 {
-		for i := len(s.oldenv) - 2; i >= 0; i = i - 2 {
-			os.Setenv(s.oldenv[i], s.oldenv[i+1])
-		}
-		s.oldenv = nil
-	}
+	s.PatchEnvironment("JUJU_UNIT_NAME", s.ctx.Unit)
 }
 
 func (s *DebugHooksServerSuite) TestFindSession(c *gc.C) {
@@ -124,9 +111,9 @@
 	// process' death).
 	ch := make(chan bool)
 	var clientExited bool
-	defer jc.Set(&waitClientExit, func(*ServerSession) {
+	s.PatchValue(&waitClientExit, func(*ServerSession) {
 		clientExited = <-ch
-	})()
+	})
 	go func() { ch <- true }()
 	err = session.RunHook("myhook", s.tmpdir, os.Environ())
 	c.Assert(clientExited, jc.IsTrue)