veyron/services/mgmt/node/impl: take care of a few low hanging TODOs in the unit
test.
Change-Id: Iabd6e6f024bae4a855a911da78492109905c532e
diff --git a/services/mgmt/node/impl/impl_test.go b/services/mgmt/node/impl/impl_test.go
index b3012f5..961fded 100644
--- a/services/mgmt/node/impl/impl_test.go
+++ b/services/mgmt/node/impl/impl_test.go
@@ -262,32 +262,6 @@
}
}
-// TODO(caprita): Move this to util_test.go, and add a flag to allow persisting
-// the root dir even when the test succeeds (helpful while developing tests).
-
-// setupRootDir sets up and returns the local filesystem location that the node
-// manager is told to use, as well as a cleanup function.
-func setupRootDir(t *testing.T) (string, func()) {
- rootDir, err := ioutil.TempDir("", "nodemanager")
- if err != nil {
- t.Fatalf("Failed to set up temporary dir for test: %v", err)
- }
- // On some operating systems (e.g. darwin) os.TempDir() can return a
- // symlink. To avoid having to account for this eventuality later,
- // evaluate the symlink.
- rootDir, err = filepath.EvalSymlinks(rootDir)
- if err != nil {
- vlog.Fatalf("EvalSymlinks(%v) failed: %v", rootDir, err)
- }
- return rootDir, func() {
- if t.Failed() {
- t.Logf("You can examine the node manager workspace at %v", rootDir)
- } else {
- os.RemoveAll(rootDir)
- }
- }
-}
-
// readPID waits for the "ready:<PID>" line from the child and parses out the
// PID of the child.
func readPID(t *testing.T, c *blackbox.Child) int {
@@ -323,11 +297,9 @@
root, cleanup := setupRootDir(t)
defer cleanup()
- // Current link does not have to live in the root dir.
- // TODO(caprita): This should be in a unique test tempdir.
- currLink := filepath.Join(os.TempDir(), "testcurrent")
- os.Remove(currLink) // Start out with a clean slate.
- defer os.Remove(currLink)
+ // Current link does not have to live in the root dir, but it's
+ // convenient to put it there so we have everything in one place.
+ currLink := filepath.Join(root, "current_link")
// Set up the initial version of the node manager, the so-called
// "factory" version.
@@ -509,6 +481,22 @@
p <- arg
}
+// setupPingServer creates a server listening for a ping from a child app; it
+// returns a channel on which the app's ping message is returned, and a cleanup
+// function.
+func setupPingServer(t *testing.T) (<-chan string, func()) {
+ server, _ := newServer()
+ pingCh := make(chan string, 1)
+ if err := server.Serve("pingserver", ipc.LeafDispatcher(pingServerDisp(pingCh), nil)); err != nil {
+ t.Fatalf("Serve(%q, <dispatcher>) failed: %v", "pingserver", err)
+ }
+ return pingCh, func() {
+ if err := server.Stop(); err != nil {
+ t.Fatalf("Stop() failed: %v", err)
+ }
+ }
+}
+
func verifyAppWorkspace(t *testing.T, root, appID, instanceID string) {
// HACK ALERT: for now, we peek inside the node manager's directory
// structure (which ought to be opaque) to check for what the app has
@@ -575,12 +563,8 @@
readPID(t, nm)
// Create the local server that the app uses to let us know it's ready.
- server, _ := newServer()
- defer server.Stop()
- pingCh := make(chan string, 1)
- if err := server.Serve("pingserver", ipc.LeafDispatcher(pingServerDisp(pingCh), nil)); err != nil {
- t.Fatalf("Serve(%q, <dispatcher>) failed: %v", "pingserver", err)
- }
+ pingCh, cleanup := setupPingServer(t)
+ defer cleanup()
// Create an envelope for a first version of the app.
app := blackbox.HelperCommand(t, "app", "appV1")
@@ -820,13 +804,8 @@
generateSuidHelperScript(t, root)
// Create the local server that the app uses to let us know it's ready.
- // TODO(caprita): Factor this code snippet out, it's pretty common.
- server, _ := newServer()
- defer server.Stop()
- pingCh := make(chan string, 1)
- if err := server.Serve("pingserver", ipc.LeafDispatcher(pingServerDisp(pingCh), nil)); err != nil {
- t.Fatalf("Serve(%q, <dispatcher>) failed: %v", "pingserver", err)
- }
+ pingCh, cleanup := setupPingServer(t)
+ defer cleanup()
// Start an instance of the app.
instanceID := startApp(t, appID)
@@ -946,8 +925,9 @@
root, cleanup := setupRootDir(t)
defer cleanup()
- currLink := filepath.Join(os.TempDir(), "testcurrent")
- os.Remove(currLink) // Start out with a clean slate.
+ // Current link does not have to live in the root dir, but it's
+ // convenient to put it there so we have everything in one place.
+ currLink := filepath.Join(root, "current_link")
// We create this command not to run it, but to harvest the flags and
// env vars from it. We need these to pass to the installer, to ensure
@@ -1009,12 +989,8 @@
generateSuidHelperScript(t, root)
// Create the local server that the app uses to let us know it's ready.
- server, _ := newServer()
- defer server.Stop()
- pingCh := make(chan string, 1)
- if err := server.Serve("pingserver", ipc.LeafDispatcher(pingServerDisp(pingCh), nil)); err != nil {
- t.Fatalf("Serve(%q, <dispatcher>) failed: %v", "pingserver", err)
- }
+ pingCh, cleanup := setupPingServer(t)
+ defer cleanup()
// Create the envelope for the first version of the app.
app := blackbox.HelperCommand(t, "app", "appV1")
diff --git a/services/mgmt/node/impl/util_test.go b/services/mgmt/node/impl/util_test.go
index 37d3cfb..ea0fed8 100644
--- a/services/mgmt/node/impl/util_test.go
+++ b/services/mgmt/node/impl/util_test.go
@@ -2,7 +2,9 @@
import (
"fmt"
+ "io/ioutil"
"os"
+ "path/filepath"
"testing"
"veyron.io/veyron/veyron2"
@@ -20,6 +22,11 @@
mtlib "veyron.io/veyron/veyron/services/mounttable/lib"
)
+// Setting this environment variable to any non-empty value avoids removing the
+// node manager's workspace for successful test runs (for failed test runs, this
+// is already the case). This is useful when developing test cases.
+const preserveNMWorkspaceEnv = "VEYRON_TEST_PRESERVE_NM_WORKSPACE"
+
// TODO(caprita): I've had to write one too many of these, let's move it to some
// central utility library.
@@ -78,6 +85,29 @@
}
}
+// setupRootDir sets up and returns the local filesystem location that the node
+// manager is told to use, as well as a cleanup function.
+func setupRootDir(t *testing.T) (string, func()) {
+ rootDir, err := ioutil.TempDir("", "nodemanager")
+ if err != nil {
+ t.Fatalf("Failed to set up temporary dir for test: %v", err)
+ }
+ // On some operating systems (e.g. darwin) os.TempDir() can return a
+ // symlink. To avoid having to account for this eventuality later,
+ // evaluate the symlink.
+ rootDir, err = filepath.EvalSymlinks(rootDir)
+ if err != nil {
+ vlog.Fatalf("EvalSymlinks(%v) failed: %v", rootDir, err)
+ }
+ return rootDir, func() {
+ if t.Failed() || os.Getenv(preserveNMWorkspaceEnv) != "" {
+ t.Logf("You can examine the node manager workspace at %v", rootDir)
+ } else {
+ os.RemoveAll(rootDir)
+ }
+ }
+}
+
func newServer() (ipc.Server, string) {
server, err := rt.R().NewServer()
if err != nil {
