Merge lp:~axwalk/juju-core/lp1206195-mock-deployer-init-dir into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/jujud: override deployer init-dir in tests

worker/deployer is (mostly) hard coded to look for unit
jobs in /etc/init, and recall/remove those it doesn't know
about.

The worker/deployer tests specify a temporary directory
instead of the default; this CL updates cmd/jujud tests
to do the same.

Fixes lp:1206195

https://codereview.appspot.com/61610044/

cmd/jujud: override deployer init-dir in tests

worker/deployer is (mostly) hard coded to look for unit
jobs in /etc/init, and recall/remove those it doesn't know
about.

The worker/deployer tests specify a temporary directory
instead of the default; this CL updates cmd/jujud tests
to do the same.

Fixes lp:1206195

https://codereview.appspot.com/61610044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+205907_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/jujud: override deployer init-dir in tests

worker/deployer is (mostly) hard coded to look for unit
jobs in /etc/init, and recall/remove those it doesn't know
about.

The worker/deployer tests specify a temporary directory
instead of the default; this CL updates cmd/jujud tests
to do the same.

Fixes lp:1206195

https://code.launchpad.net/~axwalk/juju-core/lp1206195-mock-deployer-init-dir/+merge/205907

(do not edit description out of merge proposal)

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

Affected files (+21, -2 lines):
   [revision details]
   cmd/jujud/machine_test.go
   cmd/jujud/main_test.go
   worker/deployer/simple.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: tarmac-20140212024202-e5i2ten0rh1ph2d2
+New revision: <email address hidden>

Index: cmd/jujud/machine_test.go
=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go 2014-01-31 05:38:42 +0000
+++ cmd/jujud/machine_test.go 2014-02-12 07:24:47 +0000
@@ -181,7 +181,6 @@
  }

  func (s *MachineSuite) TestDyingMachine(c *gc.C) {
- c.Skip("Disabled as breaks test isolation somehow, see lp:1206195")
   m, _, _ := s.primeAgent(c, state.JobHostUnits)
   a := s.newAgent(c, m)
   done := make(chan error)

Index: cmd/jujud/main_test.go
=== modified file 'cmd/jujud/main_test.go'
--- cmd/jujud/main_test.go 2014-01-14 05:33:34 +0000
+++ cmd/jujud/main_test.go 2014-02-12 07:24:47 +0000
@@ -20,11 +20,20 @@
   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/environs"
   "launchpad.net/juju-core/testing"
+ "launchpad.net/juju-core/worker/deployer"
   "launchpad.net/juju-core/worker/uniter/jujuc"
  )

  var caCertFile string

+func mkdtemp(prefix string) string {
+ d, err := ioutil.TempDir("", prefix)
+ if err != nil {
+ panic(err)
+ }
+ return d
+}
+
  func mktemp(prefix string, content string) string {
   f, err := ioutil.TempFile("", prefix)
   if err != nil {
@@ -39,6 +48,11 @@
  }

  func TestPackage(t *stdtesting.T) {
+ // Change the default init dir in worker/deployer,
+ // so the deployer doesn't try to remove upstart
+ // jobs from tests.
+ deployer.InitDir = mkdtemp("juju-worker-deployer")
+
   // Change the path to "juju-run", so that the
   // tests don't try to write to /usr/local/bin.
   jujuRun = mktemp("juju-run", "")

Index: worker/deployer/simple.go
=== modified file 'worker/deployer/simple.go'
--- worker/deployer/simple.go 2014-01-22 22:48:54 +0000
+++ worker/deployer/simple.go 2014-02-12 07:24:47 +0000
@@ -21,6 +21,10 @@
   "launchpad.net/juju-core/version"
  )

+// InitDir is the default upstart init directory.
+// This is a var so it can be overridden by tests.
+var InitDir = "/etc/init"
+
  // APICalls defines the interface to the API that the simple context needs.
  type APICalls interface {
   ConnectionInfo() (params.DeployerConnectionValues, error)
@@ -65,7 +69,7 @@
   return &SimpleContext{
    api: api,
    agentConfig: agentConfig,...

[Dimiter Naydenov (dimitern)]

Please repropose, the diff is missing.

https://codereview.appspot.com/61610044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/61610044/

[William Reade (fwereade)]

LGTM with suggested tweak.

https://codereview.appspot.com/61610044/diff/20001/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/61610044/diff/20001/cmd/jujud/main_test.go#newcode54
cmd/jujud/main_test.go:54: deployer.InitDir =
mkdtemp("juju-worker-deployer")
This is a bit of a big hammer, but I guess at least it will continue to
work as the tests change. Just to guard against thoughtless future
refactorings, though, please use testbase.PatchValue and defer a
restore.

https://codereview.appspot.com/61610044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/61610044/diff/20001/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/61610044/diff/20001/cmd/jujud/main_test.go#newcode54
cmd/jujud/main_test.go:54: deployer.InitDir =
mkdtemp("juju-worker-deployer")
On 2014/02/17 10:04:11, fwereade wrote:
> This is a bit of a big hammer, but I guess at least it will continue
to work as
> the tests change. Just to guard against thoughtless future
refactorings, though,
> please use testbase.PatchValue and defer a restore.

Done.

https://codereview.appspot.com/61610044/

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2014-01-31 05:38:42 +0000
+++ cmd/jujud/machine_test.go	2014-02-18 02:41:56 +0000
@@ -181,7 +181,6 @@
 }
 
 func (s *MachineSuite) TestDyingMachine(c *gc.C) {
-	c.Skip("Disabled as breaks test isolation somehow, see lp:1206195")
 	m, _, _ := s.primeAgent(c, state.JobHostUnits)
 	a := s.newAgent(c, m)
 	done := make(chan error)

=== modified file 'cmd/jujud/main_test.go'
--- cmd/jujud/main_test.go	2014-01-14 05:33:34 +0000
+++ cmd/jujud/main_test.go	2014-02-18 02:41:56 +0000
@@ -20,11 +20,21 @@
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/worker/deployer"
 	"launchpad.net/juju-core/worker/uniter/jujuc"
 )
 
 var caCertFile string
 
+func mkdtemp(prefix string) string {
+	d, err := ioutil.TempDir("", prefix)
+	if err != nil {
+		panic(err)
+	}
+	return d
+}
+
 func mktemp(prefix string, content string) string {
 	f, err := ioutil.TempFile("", prefix)
 	if err != nil {
@@ -39,6 +49,12 @@
 }
 
 func TestPackage(t *stdtesting.T) {
+	// Change the default init dir in worker/deployer,
+	// so the deployer doesn't try to remove upstart
+	// jobs from tests.
+	restore := testbase.PatchValue(&deployer.InitDir, mkdtemp("juju-worker-deployer"))
+	defer restore()
+
 	// Change the path to "juju-run", so that the
 	// tests don't try to write to /usr/local/bin.
 	jujuRun = mktemp("juju-run", "")

=== modified file 'worker/deployer/simple.go'
--- worker/deployer/simple.go	2014-01-22 22:48:54 +0000
+++ worker/deployer/simple.go	2014-02-18 02:41:56 +0000
@@ -21,6 +21,10 @@
 	"launchpad.net/juju-core/version"
 )
 
+// InitDir is the default upstart init directory.
+// This is a var so it can be overridden by tests.
+var InitDir = "/etc/init"
+
 // APICalls defines the interface to the API that the simple context needs.
 type APICalls interface {
 	ConnectionInfo() (params.DeployerConnectionValues, error)
@@ -65,7 +69,7 @@
 	return &SimpleContext{
 		api:         api,
 		agentConfig: agentConfig,
-		initDir:     "/etc/init",
+		initDir:     InitDir,
 		logDir:      "/var/log/juju",
 	}
 }