Code review comment for lp:~niemeyer/juju-core/fix-jujud-tests

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 is still down")
     default:
      c.Fatalf("unexpected status %s %s", st, info)
     }

Index: cmd/jujud/upgrade_test.go
=== 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 19:39:30 +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