From ea29a15cf090779f5627ef54ce02ebde5c438118 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Tue, 17 Nov 2015 13:33:56 -0500 Subject: [PATCH 1/4] Extract code to acquire temp file name and make sure the file is closed so os.Remove works on Windows. --- client/driver/spawn/spawn_test.go | 138 ++++++++++++------------------ 1 file changed, 53 insertions(+), 85 deletions(-) diff --git a/client/driver/spawn/spawn_test.go b/client/driver/spawn/spawn_test.go index eb013db0de2c..7c25e710b2d4 100644 --- a/client/driver/spawn/spawn_test.go +++ b/client/driver/spawn/spawn_test.go @@ -12,26 +12,20 @@ import ( ) func TestSpawn_NoCmd(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) if err := spawn.Spawn(nil); err == nil { t.Fatalf("Spawn() with no user command should fail") } } func TestSpawn_InvalidCmd(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("foo")) if err := spawn.Spawn(nil); err == nil { t.Fatalf("Spawn() with no invalid command should fail") @@ -46,23 +40,17 @@ func TestSpawn_SetsLogs(t *testing.T) { t.Skip("Test fails on windows; unknown reason. Skipping") } - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) exp := "foo" spawn.SetCommand(exec.Command("echo", exp)) // Create file for stdout. - stdout, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(stdout.Name()) - spawn.SetLogs(&Logs{Stdout: stdout.Name()}) + stdout := tempFileName(t) + defer os.Remove(stdout) + spawn.SetLogs(&Logs{Stdout: stdout}) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed: %v", err) @@ -72,7 +60,7 @@ func TestSpawn_SetsLogs(t *testing.T) { t.Fatalf("Wait() returned %v, %v; want 0, nil", res.ExitCode, res.Err) } - stdout2, err := os.Open(stdout.Name()) + stdout2, err := os.Open(stdout) if err != nil { t.Fatalf("Open() failed: %v", err) } @@ -89,13 +77,10 @@ func TestSpawn_SetsLogs(t *testing.T) { } func TestSpawn_Callback(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("sleep", "1")) called := false @@ -115,13 +100,10 @@ func TestSpawn_Callback(t *testing.T) { } func TestSpawn_ParentWaitExited(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -135,13 +117,10 @@ func TestSpawn_ParentWaitExited(t *testing.T) { } func TestSpawn_ParentWait(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("sleep", "2")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -153,13 +132,10 @@ func TestSpawn_ParentWait(t *testing.T) { } func TestSpawn_NonParentWaitExited(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -175,13 +151,10 @@ func TestSpawn_NonParentWaitExited(t *testing.T) { } func TestSpawn_NonParentWait(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("sleep", "2")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -204,11 +177,8 @@ func TestSpawn_NonParentWait(t *testing.T) { } func TestSpawn_DeadSpawnDaemon_Parent(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) var spawnPid int cb := func(pid int) error { @@ -216,7 +186,7 @@ func TestSpawn_DeadSpawnDaemon_Parent(t *testing.T) { return nil } - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("sleep", "5")) if err := spawn.Spawn(cb); err != nil { t.Fatalf("Spawn() errored: %v", err) @@ -241,11 +211,8 @@ func TestSpawn_DeadSpawnDaemon_Parent(t *testing.T) { } func TestSpawn_DeadSpawnDaemon_NonParent(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) var spawnPid int cb := func(pid int) error { @@ -253,7 +220,7 @@ func TestSpawn_DeadSpawnDaemon_NonParent(t *testing.T) { return nil } - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("sleep", "2")) if err := spawn.Spawn(cb); err != nil { t.Fatalf("Spawn() errored: %v", err) @@ -280,13 +247,10 @@ func TestSpawn_DeadSpawnDaemon_NonParent(t *testing.T) { } func TestSpawn_Valid_TaskRunning(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("sleep", "2")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -302,13 +266,10 @@ func TestSpawn_Valid_TaskRunning(t *testing.T) { } func TestSpawn_Valid_TaskExit_ExitCode(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } - defer os.Remove(f.Name()) + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -324,12 +285,10 @@ func TestSpawn_Valid_TaskExit_ExitCode(t *testing.T) { } func TestSpawn_Valid_TaskExit_NoExitCode(t *testing.T) { - f, err := ioutil.TempFile("", "") - if err != nil { - t.Fatalf("TempFile() failed") - } + tempFile := tempFileName(t) + defer os.Remove(tempFile) - spawn := NewSpawner(f.Name()) + spawn := NewSpawner(tempFile) spawn.SetCommand(exec.Command("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) @@ -340,9 +299,18 @@ func TestSpawn_Valid_TaskExit_NoExitCode(t *testing.T) { } // Delete the file so that it can't find the exit code. - os.Remove(f.Name()) + os.Remove(tempFile) if err := spawn.Valid(); err == nil { t.Fatalf("Valid() should have failed") } } + +func tempFileName(t *testing.T) string { + f, err := ioutil.TempFile("", "") + if err != nil { + t.Fatalf("TempFile() failed") + } + defer f.Close() + return f.Name() +} From 230080d0282ba2da246bb12754b143bfa8ed8c95 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Tue, 17 Nov 2015 17:21:56 -0500 Subject: [PATCH 2/4] Use TestMain to provide portable echo and sleep commands. --- client/driver/spawn/spawn_test.go | 74 ++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 21 deletions(-) diff --git a/client/driver/spawn/spawn_test.go b/client/driver/spawn/spawn_test.go index 7c25e710b2d4..771c687b6f29 100644 --- a/client/driver/spawn/spawn_test.go +++ b/client/driver/spawn/spawn_test.go @@ -5,12 +5,45 @@ import ( "io/ioutil" "os" "os/exec" - "runtime" "strings" "testing" "time" ) +func TestMain(m *testing.M) { + switch os.Getenv("TEST_MAIN") { + case "app": + appMain() + default: + os.Exit(m.Run()) + } +} + +func appMain() { + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "no command provided") + os.Exit(1) + } + switch cmd := os.Args[1]; cmd { + case "echo": + fmt.Println(strings.Join(os.Args[2:], " ")) + case "sleep": + if len(os.Args) != 3 { + fmt.Fprintln(os.Stderr, "expected 3 args") + os.Exit(1) + } + dur, err := time.ParseDuration(os.Args[2]) + if err != nil { + fmt.Fprintf(os.Stderr, "could not parse sleep time: %v", err) + os.Exit(1) + } + time.Sleep(dur) + default: + fmt.Fprintln(os.Stderr, "unknown command:", cmd) + os.Exit(1) + } +} + func TestSpawn_NoCmd(t *testing.T) { tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -26,26 +59,19 @@ func TestSpawn_InvalidCmd(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("foo")) + spawn.SetCommand(exec.Command("foo")) // non-existent command if err := spawn.Spawn(nil); err == nil { - t.Fatalf("Spawn() with no invalid command should fail") + t.Fatalf("Spawn() with an invalid command should fail") } } func TestSpawn_SetsLogs(t *testing.T) { - // TODO: Figure out why this test fails. If the spawn-daemon directly writes - // to the opened stdout file it works but not the user command. Maybe a - // flush issue? - if runtime.GOOS == "windows" { - t.Skip("Test fails on windows; unknown reason. Skipping") - } - tempFile := tempFileName(t) defer os.Remove(tempFile) spawn := NewSpawner(tempFile) exp := "foo" - spawn.SetCommand(exec.Command("echo", exp)) + spawn.SetCommand(testCommand("echo", exp)) // Create file for stdout. stdout := tempFileName(t) @@ -81,7 +107,7 @@ func TestSpawn_Callback(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("sleep", "1")) + spawn.SetCommand(testCommand("sleep", "1s")) called := false cbErr := fmt.Errorf("ERROR CB") @@ -104,7 +130,7 @@ func TestSpawn_ParentWaitExited(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("echo", "foo")) + spawn.SetCommand(testCommand("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -121,7 +147,7 @@ func TestSpawn_ParentWait(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("sleep", "2")) + spawn.SetCommand(testCommand("sleep", "2s")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -136,7 +162,7 @@ func TestSpawn_NonParentWaitExited(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("echo", "foo")) + spawn.SetCommand(testCommand("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -155,7 +181,7 @@ func TestSpawn_NonParentWait(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("sleep", "2")) + spawn.SetCommand(testCommand("sleep", "2s")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -187,7 +213,7 @@ func TestSpawn_DeadSpawnDaemon_Parent(t *testing.T) { } spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("sleep", "5")) + spawn.SetCommand(testCommand("sleep", "5s")) if err := spawn.Spawn(cb); err != nil { t.Fatalf("Spawn() errored: %v", err) } @@ -221,7 +247,7 @@ func TestSpawn_DeadSpawnDaemon_NonParent(t *testing.T) { } spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("sleep", "2")) + spawn.SetCommand(testCommand("sleep", "2s")) if err := spawn.Spawn(cb); err != nil { t.Fatalf("Spawn() errored: %v", err) } @@ -251,7 +277,7 @@ func TestSpawn_Valid_TaskRunning(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("sleep", "2")) + spawn.SetCommand(testCommand("sleep", "2s")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -270,7 +296,7 @@ func TestSpawn_Valid_TaskExit_ExitCode(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("echo", "foo")) + spawn.SetCommand(testCommand("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -289,7 +315,7 @@ func TestSpawn_Valid_TaskExit_NoExitCode(t *testing.T) { defer os.Remove(tempFile) spawn := NewSpawner(tempFile) - spawn.SetCommand(exec.Command("echo", "foo")) + spawn.SetCommand(testCommand("echo", "foo")) if err := spawn.Spawn(nil); err != nil { t.Fatalf("Spawn() failed %v", err) } @@ -314,3 +340,9 @@ func tempFileName(t *testing.T) string { defer f.Close() return f.Name() } + +func testCommand(args ...string) *exec.Cmd { + cmd := exec.Command(os.Args[0], args...) + cmd.Env = append(os.Environ(), "TEST_MAIN=app") + return cmd +} From 1451cc6441c69a588b543191d6a64c6e481765b3 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Tue, 17 Nov 2015 23:39:23 -0500 Subject: [PATCH 3/4] Move defer f.Close after error check. --- client/driver/spawn/spawn.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/client/driver/spawn/spawn.go b/client/driver/spawn/spawn.go index 32a27e11c570..5c7795f38be1 100644 --- a/client/driver/spawn/spawn.go +++ b/client/driver/spawn/spawn.go @@ -73,10 +73,10 @@ func (s *Spawner) Spawn(cb func(pid int) error) error { } exitFile, err := os.OpenFile(s.StateFile, os.O_CREATE|os.O_WRONLY, 0666) - defer exitFile.Close() if err != nil { return fmt.Errorf("Error opening file to store exit status: %v", err) } + defer exitFile.Close() config, err := s.spawnConfig() if err != nil { @@ -87,17 +87,17 @@ func (s *Spawner) Spawn(cb func(pid int) error) error { // Capture stdout spawnStdout, err := spawn.StdoutPipe() - defer spawnStdout.Close() if err != nil { return fmt.Errorf("Failed to capture spawn-daemon stdout: %v", err) } + defer spawnStdout.Close() // Capture stdin. spawnStdin, err := spawn.StdinPipe() - defer spawnStdin.Close() if err != nil { return fmt.Errorf("Failed to capture spawn-daemon stdin: %v", err) } + defer spawnStdin.Close() if err := spawn.Start(); err != nil { return fmt.Errorf("Failed to call spawn-daemon on nomad executable: %v", err) @@ -264,10 +264,10 @@ func (s *Spawner) pollWait() *structs.WaitResult { // returns an error if the file can't be read. func (s *Spawner) readExitCode() *structs.WaitResult { f, err := os.Open(s.StateFile) - defer f.Close() if err != nil { return structs.NewWaitResult(-1, 0, fmt.Errorf("Failed to open %v to read exit code: %v", s.StateFile, err)) } + defer f.Close() stat, err := f.Stat() if err != nil { @@ -289,7 +289,7 @@ func (s *Spawner) readExitCode() *structs.WaitResult { // Valid checks that the state of the Spawner is valid and that a subsequent // Wait could be called. This is useful to call when reopening a Spawner -// throught client restarts. If Valid a nil error is returned. +// through client restarts. If Valid a nil error is returned. func (s *Spawner) Valid() error { // If the spawner is still alive, then the task is running and we can wait // on it. From 21285ba9cb7c9e6a1c862d2e452df9a758568e65 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Tue, 17 Nov 2015 23:42:34 -0500 Subject: [PATCH 4/4] Speed up tests by allowing parallel execution. --- client/driver/spawn/spawn_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/client/driver/spawn/spawn_test.go b/client/driver/spawn/spawn_test.go index 771c687b6f29..62323bf8389a 100644 --- a/client/driver/spawn/spawn_test.go +++ b/client/driver/spawn/spawn_test.go @@ -45,6 +45,7 @@ func appMain() { } func TestSpawn_NoCmd(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -55,6 +56,7 @@ func TestSpawn_NoCmd(t *testing.T) { } func TestSpawn_InvalidCmd(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -66,6 +68,7 @@ func TestSpawn_InvalidCmd(t *testing.T) { } func TestSpawn_SetsLogs(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -103,6 +106,7 @@ func TestSpawn_SetsLogs(t *testing.T) { } func TestSpawn_Callback(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -126,6 +130,7 @@ func TestSpawn_Callback(t *testing.T) { } func TestSpawn_ParentWaitExited(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -143,6 +148,7 @@ func TestSpawn_ParentWaitExited(t *testing.T) { } func TestSpawn_ParentWait(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -158,6 +164,7 @@ func TestSpawn_ParentWait(t *testing.T) { } func TestSpawn_NonParentWaitExited(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -177,6 +184,7 @@ func TestSpawn_NonParentWaitExited(t *testing.T) { } func TestSpawn_NonParentWait(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -203,6 +211,7 @@ func TestSpawn_NonParentWait(t *testing.T) { } func TestSpawn_DeadSpawnDaemon_Parent(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -237,6 +246,7 @@ func TestSpawn_DeadSpawnDaemon_Parent(t *testing.T) { } func TestSpawn_DeadSpawnDaemon_NonParent(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -273,6 +283,7 @@ func TestSpawn_DeadSpawnDaemon_NonParent(t *testing.T) { } func TestSpawn_Valid_TaskRunning(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -292,6 +303,7 @@ func TestSpawn_Valid_TaskRunning(t *testing.T) { } func TestSpawn_Valid_TaskExit_ExitCode(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile) @@ -311,6 +323,7 @@ func TestSpawn_Valid_TaskExit_ExitCode(t *testing.T) { } func TestSpawn_Valid_TaskExit_NoExitCode(t *testing.T) { + t.Parallel() tempFile := tempFileName(t) defer os.Remove(tempFile)