Merge lp:~dave-cheney/juju-core/033-testing-mgo-panic into lp:~juju/juju-core/trunk

[Dave Cheney]

testing: prefer Assert over panic

https://codereview.appspot.com/6775079/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/6775079/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6775079/diff/1/environs/dummy/environs.go#newcode30
environs/dummy/environs.go:30: "time"
nice

https://codereview.appspot.com/6775079/diff/1/environs/dummy/environs.go#newcode173
environs/dummy/environs.go:173: testing.MgoReset(new(gocheck.C))
this seems wrong. if we haven't got a gocheck.C, panic is the only
viable alternative. there's no guarantee that new(gocheck.C) gives us a
working *C value.

one possibility would be to change MgoReset to accept an interface that
implements Fatalf, and write an implementation here that turns Fatalf
into panic.

https://codereview.appspot.com/6775079/diff/1/testing/mgo.go
File testing/mgo.go (right):

https://codereview.appspot.com/6775079/diff/1/testing/mgo.go#newcode86
testing/mgo.go:86: panic("MgoSuite tests must be run with
MgoTestPackage")
if you're gonna prefer Assert over panic then calling Assert rather than
panic might be a good plan. but in general i think this all works ok,
and we can leave the panics alone.

https://codereview.appspot.com/6775079/diff/1/testing/mgo.go#newcode117
testing/mgo.go:117: panic(err)
ditto

https://codereview.appspot.com/6775079/diff/1/testing/mgo.go#newcode122
testing/mgo.go:122: panic(err)
ditto

https://codereview.appspot.com/6775079/diff/1/testing/mgo.go#newcode130
testing/mgo.go:130: panic(fmt.Errorf("Cannot drop MongoDB database %v:
%v", name, err))
ditto

https://codereview.appspot.com/6775079/

Unmerged revisions

700. By Dave Cheney on 2012-10-30

pass C to MgoReset

699. By Dave Cheney on 2012-10-30

store: prefer c.Assert to panic

Updates #1073125.

Replace a bunch of panics with Asserts to make the underlying issue more obvious.

R=rog
CC=
https://codereview.appspot.com/6818053

698. By Dave Cheney on 2012-10-30

worker/provisioner: fix race on config reload

Bug #1064144 was cause by a race between the PA reloading it's config -or-
observing a change to the machines collection. This proposal adds an observer
that the tests can use to wait until the PA has ack'd the configuration
change.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6742049

697. By William Reade on 2012-10-30

state: drop obsolete relation lifecycle

drop:

* Relation.EnsureDying
* Relation.EnsureDead
* State.RemoveRelation

...and use Relation.Destroy and RelationUnit.LeaveScope instead throughout.

R=niemeyer
CC=
https://codereview.appspot.com/6812047

696. By William Reade on 2012-10-30

state: Relation.Destroy

...and `juju remove-relation`, because it's trivial now that Destroy exists.

Points to note:

* Relation.EnsureDying, Relation.EnsureDead, and State.RemoveRelation are
  now obsolete, and will be remove entirely in a followup CL; omitted here
  due to volume of change.
* It's still called remove-relation, instead of destroy-relation + alias,
  because it seems more sensible to fix remove-unit at the same time and it
  also deserves a separate CL IMO.

R=niemeyer
CC=
https://codereview.appspot.com/6783051

695. By Roger Peppe on 2012-10-30

worker/uniter: make logger predictable

The stdout pipe is closed too early by os/exec;
do the work ourselves to prevent this.

R=dfc, niemeyer
CC=
https://codereview.appspot.com/6774050

694. By William Reade on 2012-10-29

juju:implement add-relation

R=niemeyer
CC=
https://codereview.appspot.com/6761044

693. By William Reade on 2012-10-29

upstart: use better Out value

We now use the same value for upstart conf Out fields as we do --log-file
params. No idea what I did to lead myself to think it didn't work...

Also, made log file names more consistent (ie $kind-$name.log, with no
explicit "-agent")

R=rog, niemeyer
CC=
https://codereview.appspot.com/6766051

692. By William Reade on 2012-10-29

uniter/charm: verify upgrade symlink overwrite

Verifies non-existence of lp:988115 in juju-core

R=niemeyer
CC=
https://codereview.appspot.com/6760046

691. By Frank Mueller on 2012-10-29

firewaller: integrated global mode

Firewaller now recognizes the global mode. It dies a
port usage counting usage counting and opens each
used port only once and closes it after the last using unit
has gone. The handling of ports in case of a restart has
been removed from this CL to a later one.

R=
CC=
https://codereview.appspot.com/6713054

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-10-19 09:16:42 +0000
+++ environs/dummy/environs.go	2012-10-30 15:39:22 +0000
@@ -22,6 +22,14 @@
 import (
 	"errors"
 	"fmt"
+	"net"
+	"net/http"
+	"os"
+	"strings"
+	"sync"
+	"time"
+
+	"launchpad.net/gocheck"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
@@ -30,12 +38,6 @@
 	"launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/trivial"
 	"launchpad.net/juju-core/version"
-	"net"
-	"net/http"
-	"os"
-	"strings"
-	"sync"
-	"time"
 )
 
 // stateInfo returns a *state.Info which allows clients to connect to the
@@ -168,7 +170,7 @@
 	providerInstance.ops = discardOperations
 	providerInstance.state = make(map[string]*environState)
 	if testing.MgoAddr != "" {
-		testing.MgoReset()
+		testing.MgoReset(new(gocheck.C))
 	}
 }
 
@@ -471,7 +473,7 @@
 	defer e.state.mu.Unlock()
 	e.state.ops <- OpDestroy{Env: e.state.name}
 	if testing.MgoAddr != "" {
-		testing.MgoReset()
+		testing.MgoReset(new(gocheck.C))
 	}
 	e.state.bootstrapped = false
 	e.state.storage.files = make(map[string][]byte)

=== modified file 'state/ssh_test.go'
--- state/ssh_test.go	2012-09-21 12:01:53 +0000
+++ state/ssh_test.go	2012-10-30 15:39:22 +0000
@@ -325,7 +325,7 @@
 	c.Assert(food[0]["spam"], Equals, "lots")
 	c.Assert(food[1]["eggs"], Equals, "fried")
 
-	testing.MgoReset()
+	testing.MgoReset(c)
 	morefood := make([]map[string]string, 0)
 	err = menu.Find(nil).All(&morefood)
 	c.Assert(err, IsNil)

=== modified file 'testing/mgo.go'
--- testing/mgo.go	2012-10-12 16:03:12 +0000
+++ testing/mgo.go	2012-10-30 15:39:22 +0000
@@ -91,22 +91,20 @@
 func (s *MgoSuite) TearDownSuite(c *C) {}
 
 // MgoDial returns a new connection to the shared MongoDB server.
-func MgoDial() *mgo.Session {
+func MgoDial(c *C) *mgo.Session {
 	session, err := mgo.Dial(MgoAddr)
-	if err != nil {
-		panic(err)
-	}
+	c.Assert(err, IsNil)
 	return session
 }
 
 func (s *MgoSuite) SetUpTest(c *C) {
 	mgo.ResetStats()
-	s.Session = MgoDial()
+	s.Session = MgoDial(c)
 }
 
 // MgoReset deletes all content from the shared MongoDB server.
-func MgoReset() {
-	session := MgoDial()
+func MgoReset(c *C) {
+	session := MgoDial(c)
 	defer session.Close()
 	dbnames, err := session.DatabaseNames()
 	if isUnauthorized(err) {
@@ -145,8 +143,10 @@
 }
 
 func (s *MgoSuite) TearDownTest(c *C) {
-	MgoReset()
-	s.Session.Close()
+	MgoReset(c)
+	if s.Session != nil {
+		s.Session.Close()
+	}
 	for i := 0; ; i++ {
 		stats := mgo.GetStats()
 		if stats.SocketsInUse == 0 && stats.SocketsAlive == 0 {

=== modified file 'testing/mgo_test.go'
--- testing/mgo_test.go	2012-10-10 12:41:22 +0000
+++ testing/mgo_test.go	2012-10-30 15:39:22 +0000
@@ -39,7 +39,7 @@
 }
 
 func (s *mgoSuite) TestMgoResetWhenUnauthorized(c *C) {
-	session := testing.MgoDial()
+	session := testing.MgoDial(c)
 	defer session.Close()
 	err := session.DB("admin").AddUser("admin", "foo", false)
 	if err != nil && err.Error() != "need to login" {
@@ -51,7 +51,7 @@
 func (s *mgoSuite) TestMgoStartAndClean(c *C) {
 	c.Assert(testing.MgoAddr, Not(Equals), "")
 
-	session := testing.MgoDial()
+	session := testing.MgoDial(c)
 	defer session.Close()
 	menu := session.DB("food").C("menu")
 	err := menu.Insert(
@@ -66,7 +66,7 @@
 	c.Assert(food[0]["spam"], Equals, "lots")
 	c.Assert(food[1]["eggs"], Equals, "fried")
 
-	testing.MgoReset()
+	testing.MgoReset(c)
 	morefood := make([]map[string]string, 0)
 	err = menu.Find(nil).All(&morefood)
 	c.Assert(err, IsNil)