Merge lp:~niemeyer/juju-core/fix-jujud-tests into lp:~juju/juju-core/trunk

[Gustavo Niemeyer]

cmd/jujud: fix all tests

https://codereview.appspot.com/6565052/

[Gustavo Niemeyer (niemeyer)]

Reviewers: mp+126536_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/jujud: fix all tests

https://code.launchpad.net/~niemeyer/juju-core/fix-jujud-tests/+merge/126536

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/jujud/provisioning_test.go
   M cmd/jujud/unit_test.go
   M cmd/jujud/upgrade_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: cmd/jujud/provisioning_test.go
=== modified file 'cmd/jujud/provisioning_test.go'
--- cmd/jujud/provisioning_test.go 2012-09-25 23:40:11 +0000
+++ cmd/jujud/provisioning_test.go 2012-09-26 20:11:21 +0000
@@ -5,6 +5,7 @@
   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/environs/dummy"
   "launchpad.net/juju-core/juju/testing"
+ "launchpad.net/juju-core/state"
   "reflect"
   "time"
  )
@@ -56,7 +57,7 @@
   c.Assert(err, IsNil)
   units, err := s.Conn.AddUnits(svc, 1)
   c.Assert(err, IsNil)
- c.Check(opRecvTimeout(c, op, dummy.OpStartInstance{}), NotNil)
+ c.Check(s.opRecvTimeout(c, op, dummy.OpStartInstance{}), NotNil)

   // Wait for the instance id to show up in the state.
   id, err := units[0].AssignedMachineId()
@@ -64,7 +65,10 @@
   m, err := s.State.Machine(id)
   c.Assert(err, IsNil)
   w := m.Watch()
+ defer w.Stop()
   for _ = range w.Changes() {
+ err = m.Refresh()
+ c.Assert(err, IsNil)
    _, err := m.InstanceId()
    if state.IsNotFound(err) {
     continue
@@ -75,7 +79,7 @@
   err = units[0].OpenPort("tcp", 999)
   c.Assert(err, IsNil)

- c.Check(opRecvTimeout(c, op, dummy.OpOpenPorts{}), NotNil)
+ c.Check(s.opRecvTimeout(c, op, dummy.OpOpenPorts{}), NotNil)

   err = a.Stop()
   c.Assert(err, IsNil)
@@ -83,14 +87,15 @@
   select {
   case err := <-done:
    c.Assert(err, IsNil)
- case <-time.After(2 * time.Second):
+ case <-time.After(5 * time.Second):
    c.Fatalf("timed out waiting for agent to terminate")
   }
  }

  // opRecvTimeout waits for any of the given kinds of operation to
  // be received from ops, and times out if not.
-func opRecvTimeout(c *C, opc <-chan dummy.Operation,
kinds ...dummy.Operation) dummy.Operation {
+func (s *ProvisioningSuite) opRecvTimeout(c *C, opc <-chan
dummy.Operation, kinds ...dummy.Operation) dummy.Operation {
+ s.State.StartSync()
   for {
    select {
    case op := <-opc:
@@ -100,7 +105,7 @@
      }
     }
     c.Logf("discarding unknown event %#v", op)
- case <-time.After(2 * time.Second):
+ case <-time.After(5 * time.Second):
     c.Fatalf("time out wating for operation")
    }
   }

Index: cmd/jujud/unit_test.go
=== modified file 'cmd/jujud/unit_test.go'
--- cmd/jujud/unit_test.go 2012-09-13 20:32:50 +0000
+++ cmd/jujud/unit_test.go 2012-09-26 19:39:30 +0000
@@ -96,6 +96,9 @@
      continue
     case state.UnitStarted:
      c.Logf("started!")
+ case state.UnitDown:
+ s.State.StartSync()
+ c.Logf("unit i...

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/6565052/

[Gustavo Niemeyer (niemeyer)]

*** Submitted:

cmd/jujud: fix all tests

R=fwereade
CC=
https://codereview.appspot.com/6565052

https://codereview.appspot.com/6565052/

=== modified file 'cmd/jujud/provisioning_test.go'
--- cmd/jujud/provisioning_test.go	2012-09-25 23:40:11 +0000
+++ cmd/jujud/provisioning_test.go	2012-09-26 20:13:27 +0000
@@ -5,6 +5,7 @@
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs/dummy"
 	"launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
 	"reflect"
 	"time"
 )
@@ -56,7 +57,7 @@
 	c.Assert(err, IsNil)
 	units, err := s.Conn.AddUnits(svc, 1)
 	c.Assert(err, IsNil)
-	c.Check(opRecvTimeout(c, op, dummy.OpStartInstance{}), NotNil)
+	c.Check(s.opRecvTimeout(c, op, dummy.OpStartInstance{}), NotNil)
 
 	// Wait for the instance id to show up in the state.
 	id, err := units[0].AssignedMachineId()
@@ -64,7 +65,10 @@
 	m, err := s.State.Machine(id)
 	c.Assert(err, IsNil)
 	w := m.Watch()
+	defer w.Stop()
 	for _ = range w.Changes() {
+		err = m.Refresh()
+		c.Assert(err, IsNil)
 		_, err := m.InstanceId()
 		if state.IsNotFound(err) {
 			continue
@@ -75,7 +79,7 @@
 	err = units[0].OpenPort("tcp", 999)
 	c.Assert(err, IsNil)
 
-	c.Check(opRecvTimeout(c, op, dummy.OpOpenPorts{}), NotNil)
+	c.Check(s.opRecvTimeout(c, op, dummy.OpOpenPorts{}), NotNil)
 
 	err = a.Stop()
 	c.Assert(err, IsNil)
@@ -83,14 +87,15 @@
 	select {
 	case err := <-done:
 		c.Assert(err, IsNil)
-	case <-time.After(2 * time.Second):
+	case <-time.After(5 * time.Second):
 		c.Fatalf("timed out waiting for agent to terminate")
 	}
 }
 
 // opRecvTimeout waits for any of the given kinds of operation to
 // be received from ops, and times out if not.
-func opRecvTimeout(c *C, opc <-chan dummy.Operation, kinds ...dummy.Operation) dummy.Operation {
+func (s *ProvisioningSuite) opRecvTimeout(c *C, opc <-chan dummy.Operation, kinds ...dummy.Operation) dummy.Operation {
+	s.State.StartSync()
 	for {
 		select {
 		case op := <-opc:
@@ -100,7 +105,7 @@
 				}
 			}
 			c.Logf("discarding unknown event %#v", op)
-		case <-time.After(2 * time.Second):
+		case <-time.After(5 * time.Second):
 			c.Fatalf("time out wating for operation")
 		}
 	}

=== modified file 'cmd/jujud/unit_test.go'
--- cmd/jujud/unit_test.go	2012-09-13 20:32:50 +0000
+++ cmd/jujud/unit_test.go	2012-09-26 20:13:27 +0000
@@ -96,6 +96,9 @@
 				continue
 			case state.UnitStarted:
 				c.Logf("started!")
+			case state.UnitDown:
+				s.State.StartSync()
+				c.Logf("unit is still down")
 			default:
 				c.Fatalf("unexpected status %s %s", st, info)
 			}

=== modified file 'cmd/jujud/upgrade_test.go'
--- cmd/jujud/upgrade_test.go	2012-09-17 11:21:13 +0000
+++ cmd/jujud/upgrade_test.go	2012-09-26 20:13:27 +0000
@@ -17,42 +17,30 @@
 	"time"
 )
 
-var _ = Suite(&upgraderSuite{})
+var _ = Suite(&UpgraderSuite{})
 
-type upgraderSuite struct {
+type UpgraderSuite struct {
 	oldVersion version.Binary
 	testing.JujuConnSuite
 }
 
-func (s *upgraderSuite) SetUpTest(c *C) {
+func (s *UpgraderSuite) SetUpTest(c *C) {
 	s.JujuConnSuite.SetUpTest(c)
 	s.oldVersion = version.Current
 }
 
-func (s *upgraderSuite) TearDownTest(c *C) {
+func (s *UpgraderSuite) TearDownTest(c *C) {
 	version.Current = s.oldVersion
 	s.JujuConnSuite.TearDownTest(c)
 }
 
-func (s *upgraderSuite) TestUpgraderError(c *C) {
-	st, err := state.Open(s.StateInfo(c))
-	c.Assert(err, IsNil)
-	// We have no installed tools, so the logic should set the agent
-	// tools anyway, but with no URL.
-	u := startUpgrader(c, st, c.MkDir(), &state.Tools{Binary: version.Current})
-
-	// Close the state under the watcher and check that the upgrader dies.
-	st.Close()
-	waitDeath(c, u, nil, "watcher: cannot get content of node.*")
-}
-
-func (s *upgraderSuite) TestUpgraderStop(c *C) {
+func (s *UpgraderSuite) TestUpgraderStop(c *C) {
 	u := startUpgrader(c, s.State, c.MkDir(), &state.Tools{Binary: version.Current})
 	err := u.Stop()
 	c.Assert(err, IsNil)
 }
 
-func (s *upgraderSuite) proposeVersion(c *C, vers version.Number, development bool) {
+func (s *UpgraderSuite) proposeVersion(c *C, vers version.Number, development bool) {
 	cfg, err := s.State.EnvironConfig()
 	c.Assert(err, IsNil)
 	attrs := cfg.AllAttrs()
@@ -64,7 +52,7 @@
 	c.Assert(err, IsNil)
 }
 
-func (s *upgraderSuite) uploadTools(c *C, vers version.Binary) *state.Tools {
+func (s *UpgraderSuite) uploadTools(c *C, vers version.Binary) *state.Tools {
 	tgz := coretesting.TarGz(
 		coretesting.NewTarFile("juju", 0777, "juju contents "+vers.String()),
 		coretesting.NewTarFile("jujuc", 0777, "jujuc contents "+vers.String()),
@@ -131,7 +119,7 @@
 },
 }
 
-func (s *upgraderSuite) TestUpgrader(c *C) {
+func (s *UpgraderSuite) TestUpgrader(c *C) {
 	dataDir, currentTools := s.primeTools(c, version.MustParseBinary("2.0.0-foo-bar"))
 	// Remove the tools from the storage so that we're sure that the
 	// uploader isn't trying to fetch them.
@@ -171,8 +159,10 @@
 		}
 		if test.propose != "" {
 			s.proposeVersion(c, version.MustParse(test.propose), test.devVersion)
+			s.State.StartSync()
 		}
 		if test.upgradeTo == "" {
+			s.State.StartSync()
 			assertNothingHappens(c, upgraderDone)
 		} else {
 			tools := uploaded[version.MustParse(test.upgradeTo)]
@@ -229,7 +219,7 @@
 },
 }
 
-func (s *upgraderSuite) TestDelayedStop(c *C) {
+func (s *UpgraderSuite) TestDelayedStop(c *C) {
 	defer dummy.SetStorageDelay(0)
 	dataDir, tools := s.primeTools(c, version.MustParseBinary("2.0.3-foo-bar"))
 	s.uploadTools(c, version.MustParseBinary("2.0.5-foo-bar"))
@@ -259,12 +249,12 @@
 	}
 }
 
-func (s *upgraderSuite) poisonVersion(vers version.Binary) {
+func (s *UpgraderSuite) poisonVersion(vers version.Binary) {
 	path := environs.ToolsStoragePath(vers)
 	dummy.Poison(s.Conn.Environ.Storage(), path, fmt.Errorf("poisoned file"))
 }
 
-func (s *upgraderSuite) removeVersion(c *C, vers version.Binary) {
+func (s *UpgraderSuite) removeVersion(c *C, vers version.Binary) {
 	path := environs.ToolsStoragePath(vers)
 	err := s.Conn.Environ.Storage().Remove(path)
 	c.Assert(err, IsNil)
@@ -272,7 +262,7 @@
 
 // primeTools sets up the current version of the tools to vers and
 // makes sure that they're available in the returned dataDir.
-func (s *upgraderSuite) primeTools(c *C, vers version.Binary) (dataDir string, tools *state.Tools) {
+func (s *UpgraderSuite) primeTools(c *C, vers version.Binary) (dataDir string, tools *state.Tools) {
 	dataDir = c.MkDir()
 	// Set up the current version and tools.
 	version.Current = vers