Merge lp:~axwalk/juju-core/fix-debug-hooks-tests into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

worker/apiuniter/debug: fix debug-hook tests

DebugHooksServerSuite.TestRunHookExceptional has
been changed to no longer be timing-based at all,
which has the added benefit of speeding up the
tests by 1s.

DebugHooksServerSuite.TestRunHook is still timing
based (it shells out, so mocking isn't really
an option here), but the timeout is significantly
increased (to 5s, from 100ms).

Fixes #1224768

https://codereview.appspot.com/13549045/

worker/apiuniter/debug: fix debug-hook tests

DebugHooksServerSuite.TestRunHookExceptional has
been changed to no longer be timing-based at all,
which has the added benefit of speeding up the
tests by 1s.

DebugHooksServerSuite.TestRunHook is still timing
based (it shells out, so mocking isn't really
an option here), but the timeout is significantly
increased (to 5s, from 100ms).

Fixes #1224768

https://codereview.appspot.com/13549045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+185423_code.launchpad.net,

Message:
Please take a look.

Description:
worker/apiuniter/debug: fix debug-hook tests

DebugHooksServerSuite.TestRunHookExceptional has
been changed to no longer be timing-based at all,
which has the added benefit of speeding up the
tests by 1s.

DebugHooksServerSuite.TestRunHook is still timing
based (it shells out, so mocking isn't really
an option here), but the timeout is significantly
increased (to 5s, from 100ms).

Fixes #1224768

https://code.launchpad.net/~axwalk/juju-core/fix-debug-hooks-tests/+merge/185423

(do not edit description out of merge proposal)

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

Affected files (+69, -55 lines):
   A [revision details]
   M worker/apiuniter/debug/server.go
   M worker/apiuniter/debug/server_test.go

[William Reade (fwereade)]

Nice, LGTM

https://codereview.appspot.com/13549045/

[Dimiter Naydenov (dimitern)]

NOT LGTM, please don't land this - worker/apiuniter is gone.

https://codereview.appspot.com/13549045/

[Andrew Wilkins (axwalk)]

On 2013/09/13 10:46:10, dimitern wrote:
> NOT LGTM, please don't land this - worker/apiuniter is gone.

Yep, no worries. I'll redo this on worker/api/debug after the weekend.

https://codereview.appspot.com/13549045/

[Andrew Wilkins (axwalk)]

On 2013/09/13 10:55:20, axw1 wrote:
> On 2013/09/13 10:46:10, dimitern wrote:
> > NOT LGTM, please don't land this - worker/apiuniter is gone.

> Yep, no worries. I'll redo this on worker/api/debug after the weekend.

Er, worker/uniter/debug, but you probably got that.

https://codereview.appspot.com/13549045/

[Andrew Wilkins (axwalk)]

On 2013/09/13 10:55:39, axw1 wrote:
> On 2013/09/13 10:55:20, axw1 wrote:
> > On 2013/09/13 10:46:10, dimitern wrote:
> > > NOT LGTM, please don't land this - worker/apiuniter is gone.
> >
> > Yep, no worries. I'll redo this on worker/api/debug after the
weekend.

> Er, worker/uniter/debug, but you probably got that.

The "debug" directory hadn't changed at all between uniter/apiuniter
(except for jam's changes to skip these breaking tests). My change is
exactly the same as before, but in worker/uniter/debug; I'll go ahead
and land.

https://codereview.appspot.com/13549045/

[Andrew Wilkins (axwalk)]

On 2013/09/13 10:55:39, axw1 wrote:
> On 2013/09/13 10:55:20, axw1 wrote:
> > On 2013/09/13 10:46:10, dimitern wrote:
> > > NOT LGTM, please don't land this - worker/apiuniter is gone.
> >
> > Yep, no worries. I'll redo this on worker/api/debug after the
weekend.

> Er, worker/uniter/debug, but you probably got that.

The "debug" directory hadn't changed at all between uniter/apiuniter
(except for jam's changes to skip these breaking tests). My change is
exactly the same as before, but in worker/uniter/debug; I'll go ahead
and land.

https://codereview.appspot.com/13549045/

=== modified file 'worker/uniter/debug/server.go'
--- worker/uniter/debug/server.go	2013-08-16 04:10:20 +0000
+++ worker/uniter/debug/server.go	2013-09-16 01:57:13 +0000
@@ -27,6 +27,13 @@
 	return s.hooks.IsEmpty() || s.hooks.Contains(hookName)
 }
 
+// waitClientExit executes flock, waiting for the SSH client to exit.
+// This is a var so it can be replaced for testing.
+var waitClientExit = func(s *ServerSession) {
+	path := s.ClientExitFileLock()
+	exec.Command("flock", path, "-c", "true").Run()
+}
+
 // RunHook "runs" the hook with the specified name via debug-hooks.
 func (s *ServerSession) RunHook(hookName, charmDir string, env []string) error {
 	env = append(env, "JUJU_HOOK_NAME="+hookName)
@@ -41,8 +48,7 @@
 		// Wait for the SSH client to exit (i.e. release the flock),
 		// then kill the server hook process in case the client
 		// exited uncleanly.
-		path := s.ClientExitFileLock()
-		exec.Command("flock", path, "-c", "true").Run()
+		waitClientExit(s)
 		proc.Kill()
 	}(cmd.Process)
 	return cmd.Wait()

=== modified file 'worker/uniter/debug/server_test.go'
--- worker/uniter/debug/server_test.go	2013-08-20 01:32:02 +0000
+++ worker/uniter/debug/server_test.go	2013-09-16 01:57:13 +0000
@@ -1,7 +1,7 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package debug_test
+package debug
 
 import (
 	"fmt"
@@ -15,11 +15,10 @@
 	gc "launchpad.net/gocheck"
 
 	jc "launchpad.net/juju-core/testing/checkers"
-	"launchpad.net/juju-core/worker/uniter/debug"
 )
 
 type DebugHooksServerSuite struct {
-	ctx     *debug.HooksContext
+	ctx     *HooksContext
 	fakebin string
 	tmpdir  string
 	oldenv  []string
@@ -51,7 +50,7 @@
 		err := ioutil.WriteFile(filepath.Join(s.fakebin, name), []byte(echocommand), 0777)
 		c.Assert(err, gc.IsNil)
 	}
-	s.ctx = debug.NewHooksContext("foo/8")
+	s.ctx = NewHooksContext("foo/8")
 	s.ctx.FlockDir = s.tmpdir
 	s.setenv("JUJU_UNIT_NAME", s.ctx.Unit)
 }
@@ -118,18 +117,20 @@
 	err = session.RunHook("myhook", s.tmpdir, os.Environ())
 	c.Assert(err, gc.ErrorMatches, "signal: killed")
 
-	// Run the hook in debug mode with the exit flock held.
-	// This simulates the client process starting but not
-	// cleanly exiting (normally the .pid file is updated,
-	// and the server waits on the client process' death).
-	cmd := exec.Command("flock", s.ctx.ClientExitFileLock(), "-c", "sleep 1s")
-	c.Assert(cmd.Start(), gc.IsNil)
-	expected := time.Now().Add(time.Second)
+	// Run the hook in debug mode, simulating the holding
+	// of the exit flock. This simulates the client process
+	// starting but not cleanly exiting (normally the .pid
+	// file is updated, and the server waits on the client
+	// process' death).
+	ch := make(chan bool)
+	var clientExited bool
+	defer jc.Set(&waitClientExit, func(*ServerSession) {
+		clientExited = <-ch
+	})()
+	go func() { ch <- true }()
 	err = session.RunHook("myhook", s.tmpdir, os.Environ())
-	after := time.Now()
-	c.Assert(after, jc.TimeBetween(expected.Add(-100*time.Millisecond), expected.Add(100*time.Millisecond)))
+	c.Assert(clientExited, jc.IsTrue)
 	c.Assert(err, gc.ErrorMatches, "signal: killed")
-	c.Assert(cmd.Wait(), gc.IsNil)
 }
 
 func (s *DebugHooksServerSuite) TestRunHook(c *gc.C) {
@@ -151,46 +152,53 @@
 	go func() {
 		ch <- session.RunHook(hookName, s.tmpdir, os.Environ())
 	}()
+
+	// Wait until either we find the debug dir, or the flock is released.
+	ticker := time.Tick(10 * time.Millisecond)
 	var debugdir os.FileInfo
-	for i := 0; i < 10; i++ {
-		tmpdir, err := os.Open(s.tmpdir)
-		if err != nil {
-			c.Fatalf("Failed to open $TMPDIR: %s", err)
-		}
-		fi, err := tmpdir.Readdir(-1)
-		if err != nil {
-			c.Fatalf("Failed to read $TMPDIR: %s", err)
-		}
-		tmpdir.Close()
-		for _, fi := range fi {
-			if fi.IsDir() {
-				hooksh := filepath.Join(s.tmpdir, fi.Name(), "hook.sh")
-				if _, err = os.Stat(hooksh); err == nil {
-					debugdir = fi
-					break
+	for debugdir == nil {
+		select {
+		case err = <-ch:
+			// flock was released before we found the debug dir.
+			c.Error("could not find hook.sh")
+
+		case <-ticker:
+			tmpdir, err := os.Open(s.tmpdir)
+			if err != nil {
+				c.Fatalf("Failed to open $TMPDIR: %s", err)
+			}
+			fi, err := tmpdir.Readdir(-1)
+			if err != nil {
+				c.Fatalf("Failed to read $TMPDIR: %s", err)
+			}
+			tmpdir.Close()
+			for _, fi := range fi {
+				if fi.IsDir() {
+					hooksh := filepath.Join(s.tmpdir, fi.Name(), "hook.sh")
+					if _, err = os.Stat(hooksh); err == nil {
+						debugdir = fi
+						break
+					}
 				}
 			}
-		}
-		if debugdir != nil {
-			break
-		}
-		time.Sleep(10 * time.Millisecond)
-	}
-	if debugdir == nil {
-		c.Error("could not find hook.sh")
-	} else {
-		envsh := filepath.Join(s.tmpdir, debugdir.Name(), "env.sh")
-		s.verifyEnvshFile(c, envsh, hookName)
-
-		hookpid := filepath.Join(s.tmpdir, debugdir.Name(), "hook.pid")
-		err = ioutil.WriteFile(hookpid, []byte("not a pid"), 0777)
-		c.Assert(err, gc.IsNil)
-
-		// RunHook should complete without waiting to be
-		// killed, and despite the exit lock being held.
-		err = <-ch
-		c.Assert(err, gc.IsNil)
-	}
+			if debugdir != nil {
+				break
+			}
+			time.Sleep(10 * time.Millisecond)
+		}
+	}
+
+	envsh := filepath.Join(s.tmpdir, debugdir.Name(), "env.sh")
+	s.verifyEnvshFile(c, envsh, hookName)
+
+	hookpid := filepath.Join(s.tmpdir, debugdir.Name(), "hook.pid")
+	err = ioutil.WriteFile(hookpid, []byte("not a pid"), 0777)
+	c.Assert(err, gc.IsNil)
+
+	// RunHook should complete without waiting to be
+	// killed, and despite the exit lock being held.
+	err = <-ch
+	c.Assert(err, gc.IsNil)
 	cmd.Process.Kill() // kill flock
 }