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

[Tim Penhey]

Add a base cleaner suite to add cleanup methods

Also made the LoggerSuite embed it, so most existing
tests get the magic.

https://codereview.appspot.com/13633045/

Add a base cleaner suite to add cleanup methods

Also made the LoggerSuite embed it, so most existing
tests get the magic.

https://codereview.appspot.com/13633045/

[Tim Penhey (thumper)]

Reviewers: mp+185199_code.launchpad.net,

Message:
Please take a look.

Description:
Add a base cleaner suite to add cleanup methods

Also made the LoggerSuite embed it, so most existing
tests get the magic.

https://code.launchpad.net/~thumper/juju-core/cleanup-suite/+merge/185199

(do not edit description out of merge proposal)

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

Affected files (+112, -9 lines):
   A [revision details]
   A testing/cleanup.go
   A testing/cleanup_test.go
   M testing/log.go

[Andrew Wilkins (axwalk)]

On 2013/09/12 05:05:17, thumper wrote:
> Please take a look.

LGTM.

https://codereview.appspot.com/13633045/

[Ian Booth (wallyworld)]

LGTM but I'd like some tests to ensure there's no cross contamination
between suite and test cleanup

https://codereview.appspot.com/13633045/

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/cleanup-suite into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.714s
ok launchpad.net/juju-core/agent/tools 0.238s
ok launchpad.net/juju-core/bzr 6.872s
ok launchpad.net/juju-core/cert 2.158s
ok launchpad.net/juju-core/charm 0.571s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.020s
ok launchpad.net/juju-core/cmd 0.249s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddMachineSuite.SetUpTest

[LOG] 98.04033 INFO juju mongod: error command line: unknown option sslOnNormalPorts
[LOG] 98.04037 INFO juju mongod: use --help for help
... Panic: no reachable servers (PC=0x414321)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:198
  in MgoDial
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:205
  in MgoSuite.SetUpTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:138
  in JujuConnSuite.SetUpTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/repo.go:25
  in RepoSuite.SetUpTest

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddMachineSuite.TearDownTest

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x414321)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
/usr/lib/go/src/pkg/runtime/panic.c:487
  in panicstring
/usr/lib/go/src/pkg/runtime/os_linux.c:236
  in sigpanic
/home/tarmac/trees/src/launchpad.net/juju-core/state/state.go:1044
  in State.SetAdminMongoPassword
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:264
  in JujuConnSuite.tearDownConn
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:144
  in JujuConnSuite.TearDownTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/repo.go:44
  in RepoSuite.TearDownTest

----------------------------------------------------------------------
PANIC: addmachine_test.go:82: AddMachineSuite.TestAddContainerToExistingMachine

... Panic: Fixture has panicked (see related PANIC)

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddRelationSuite.SetUpTest

... Panic: no reachable servers (PC=0x414321)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:198
  in MgoDial
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:205
  in MgoSuite.SetUpTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:138
  in JujuConnSuite.SetUpTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/repo.go:25
  in RepoSuite.SetUpTest

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddRelationSuite.TearDownTest

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x414321)

/usr/lib/go/src/pkg/...

[Roger Peppe (rogpeppe)]

LGTM with a couple of minor suggestions.

Embedding this into LoggingSuite makes me
feel twitchy though. This a marriage entirely
of convenience - LoggingSuite previously
was about a single thing only, and now has
this functionality shoehorned in just because
lots of things use LoggingSuite.

I would be more inclined to embed it in some
of the "kitchen-sink" suites: JujuConnSuite;
or rename LoggingSuite to LoggingAndCleanupSuite
perhaps throughout.

I like the functionality in general though.

https://codereview.appspot.com/13633045/diff/1/testing/cleanup.go
File testing/cleanup.go (right):

https://codereview.appspot.com/13633045/diff/1/testing/cleanup.go#newcode14
testing/cleanup.go:14: testStack []func()
One possibility is to use jc.Restorer instead of func() here, which
perhaps makes the purpose of the functions more clear.

It doesn't really make any difference, just a thought.

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

https://codereview.appspot.com/13633045/diff/1/testing/cleanup_test.go#newcode10
testing/cleanup_test.go:10: testing.CleanupSuite
I'm not sure we need to embed this - you can just
declare it in each test, which I think will make the
tests a little clearer as it avoids shared state.

https://codereview.appspot.com/13633045/

[John A Meinel (jameinel)]

The problem with embedding it in each test, is then you have to update your SetUp and TearDown to call yet-another method, etc. So it is "just one more embed" but that is one more of a bunch of other bits.

This is the sort of thing you would put on a "base" suite if we had such a thing. LoggingSuite comes pretty close. We could rename it for clarity if we like that more.

The failure in the test suite was because the bot rebooted and I missed one line. It should land now. I'm going to mark it Approved because I don't want Tim to have to wait on Bot failure. However, please respond to Roger's comments.

=== added file 'testing/cleanup.go'
--- testing/cleanup.go	1970-01-01 00:00:00 +0000
+++ testing/cleanup.go	2013-09-12 05:03:44 +0000
@@ -0,0 +1,46 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	gc "launchpad.net/gocheck"
+)
+
+// CleanupSuite adds the ability to add cleanup functions that are called
+// during either test tear down or suite tear down depending on the method
+// called.
+type CleanupSuite struct {
+	testStack  []func()
+	suiteStack []func()
+}
+
+func (s *CleanupSuite) SetUpSuite(c *gc.C) {
+	s.suiteStack = nil
+}
+
+func (s *CleanupSuite) TearDownSuite(c *gc.C) {
+	s.callStack(s.suiteStack)
+}
+
+func (s *CleanupSuite) SetUpTest(c *gc.C) {
+	s.testStack = nil
+}
+
+func (s *CleanupSuite) TearDownTest(c *gc.C) {
+	s.callStack(s.testStack)
+}
+
+func (s *CleanupSuite) callStack(stack []func()) {
+	for i := len(stack) - 1; i >= 0; i-- {
+		stack[i]()
+	}
+}
+
+func (s *CleanupSuite) AddCleanup(cleanup func()) {
+	s.testStack = append(s.testStack, cleanup)
+}
+
+func (s *CleanupSuite) AddSuiteCleanup(cleanup func()) {
+	s.suiteStack = append(s.suiteStack, cleanup)
+}

=== added file 'testing/cleanup_test.go'
--- testing/cleanup_test.go	1970-01-01 00:00:00 +0000
+++ testing/cleanup_test.go	2013-09-12 05:03:44 +0000
@@ -0,0 +1,57 @@
+package testing_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/testing"
+)
+
+type cleanupSuite struct {
+	testing.CleanupSuite
+}
+
+var _ = gc.Suite(&cleanupSuite{})
+
+func (s *cleanupSuite) TestTearDownSuiteEmpty(c *gc.C) {
+	// The suite stack is empty initially, check we can tear that down.
+	s.TearDownSuite(c)
+	s.SetUpSuite(c)
+}
+
+func (s *cleanupSuite) TestTearDownTestEmpty(c *gc.C) {
+	// The test stack is empty initially, check we can tear that down.
+	s.TearDownTest(c)
+	s.SetUpTest(c)
+}
+
+func (s *cleanupSuite) TestAddSuiteCleanup(c *gc.C) {
+	order := []string{}
+	s.AddSuiteCleanup(func() {
+		order = append(order, "first")
+	})
+	s.AddSuiteCleanup(func() {
+		order = append(order, "second")
+	})
+
+	s.TearDownSuite(c)
+	c.Assert(order, gc.DeepEquals, []string{"second", "first"})
+
+	// and to avoid calling them twice, clear out the stack.
+	s.SetUpSuite(c)
+}
+
+func (s *cleanupSuite) TestAddCleanup(c *gc.C) {
+	order := []string{}
+	s.AddCleanup(func() {
+		order = append(order, "first")
+	})
+	s.AddCleanup(func() {
+		order = append(order, "second")
+	})
+
+	s.TearDownTest(c)
+	c.Assert(order, gc.DeepEquals, []string{"second", "first"})
+
+	// and to avoid calling them twice, clear out the stack.
+	s.SetUpTest(c)
+}

=== modified file 'testing/log.go'
--- testing/log.go	2013-08-28 17:20:01 +0000
+++ testing/log.go	2013-09-12 05:03:44 +0000
@@ -14,7 +14,7 @@
 // LoggingSuite redirects the juju logger to the test logger
 // when embedded in a gocheck suite type.
 type LoggingSuite struct {
-	restoreLog func()
+	CleanupSuite
 }
 
 type gocheckWriter struct {
@@ -27,21 +27,19 @@
 }
 
 func (t *LoggingSuite) SetUpSuite(c *gc.C) {
+	t.CleanupSuite.SetUpSuite(c)
 	t.setUp(c)
-}
-
-func (t *LoggingSuite) TearDownSuite(c *gc.C) {
-	loggo.ResetLoggers()
-	loggo.ResetWriters()
+	t.AddSuiteCleanup(func() {
+		loggo.ResetLoggers()
+		loggo.ResetWriters()
+	})
 }
 
 func (t *LoggingSuite) SetUpTest(c *gc.C) {
+	t.CleanupSuite.SetUpTest(c)
 	t.setUp(c)
 }
 
-func (t *LoggingSuite) TearDownTest(c *gc.C) {
-}
-
 func (t *LoggingSuite) setUp(c *gc.C) {
 	loggo.ResetWriters()
 	loggo.ReplaceDefaultWriter(&gocheckWriter{c})