PatchValue (AddCleanup) unsafe with non pointer receiver test

[John Meinel]

I came across this in the LXD test suite today, which was hard to track
down, so I figured I'd let everyone know about it.

We have a nice helper in testing.IsolationSuite with "PatchValue()" that
will change a global for you during the test, and then during TearDown()
will cleanup the patch it made.
It turns out that if your test doesn't have a pointer receiver this fails,
because the "suite" object is a copy, so when PatchValue calls AddCleanup
to do s.testStack = append(...) the suite object goes away before TearDown
is called.

You can see this with the attached test suite.

Example:

func (s mySuite) TestFoo(c *gc.C) {
 // This is unsafe because s.PatchValue ends up modifying s.testStack but
that attribute only exists
 // for the life of the TestFoo function
  s.PatchValue(&globalVar, "newvalue")
}

I tried adding the attached patch so that we catch places we are using
AddCleanup unsafely, but it fails a few tests I wasn't expecting, so I'm
not sure if I'm actually doing the right thing.

John
=:->
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160317/8d773d82/attachment.html>
-------------- next part --------------
diff --git a/cleanup.go b/cleanup.go
index 48a48bb..2b88f7e 100644
--- a/cleanup.go
+++ b/cleanup.go
@@ -18,10 +18,13 @@ type cleanupStack []CleanupFunc
 type CleanupSuite struct {
 	testStack  cleanupStack
 	suiteStack cleanupStack
+	suiteSuite *CleanupSuite
+	testSuite  *CleanupSuite
 }
 
 func (s *CleanupSuite) SetUpSuite(c *gc.C) {
 	s.suiteStack = nil
+	s.suiteSuite = s
 }
 
 func (s *CleanupSuite) TearDownSuite(c *gc.C) {
@@ -30,6 +33,7 @@ func (s *CleanupSuite) TearDownSuite(c *gc.C) {
 
 func (s *CleanupSuite) SetUpTest(c *gc.C) {
 	s.testStack = nil
+	s.testSuite = s
 }
 
 func (s *CleanupSuite) TearDownTest(c *gc.C) {
@@ -45,12 +49,18 @@ func (s *CleanupSuite) callStack(c *gc.C, stack cleanupStack) {
 // AddCleanup pushes the cleanup function onto the stack of functions to be
 // called during TearDownTest.
 func (s *CleanupSuite) AddCleanup(cleanup CleanupFunc) {
+	if s != s.testSuite {
+		panic("testSuite changed from SetUp to TearDown")
+	}
 	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) {
+	if s != s.testSuite {
+		panic("test suite changed from SetUpSuite before AddSuiteCleanup")
+	}
 	s.suiteStack = append(s.suiteStack, cleanup)
 }
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cleanup_test.go
Type: text/x-go
Size: 827 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160317/8d773d82/attachment.bin>