Improve reliability of tests with TestAgent (#4525)

- Add WaitForTestAgent to tests flaky due to missing serfHealth registration

- Fix bug in retries calling Fatalf with *testing.T

- Convert TestLockCommand_ChildExitCode to table driven test

agent/kvs_endpoint_test.go

@@ -8,6 +8,8 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/hashicorp/consul/testrpc"
+
 	"github.com/hashicorp/consul/agent/structs"
 )
 
@@ -342,6 +344,8 @@ func TestKVSEndpoint_AcquireRelease(t *testing.T) {
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
 
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
+
 	// Acquire the lock
 	id := makeTestSession(t, a.srv)
 	req, _ := http.NewRequest("PUT", "/v1/kv/test?acquire="+id, bytes.NewReader(nil))

agent/session_endpoint_test.go

@@ -16,19 +16,17 @@ import (
 	"github.com/pascaldekloe/goe/verify"
 )
 
-func verifySession(t *testing.T, a *TestAgent, want structs.Session) {
+func verifySession(r *retry.R, a *TestAgent, want structs.Session) {
-	t.Helper()
-
 	args := &structs.SessionSpecificRequest{
 		Datacenter: "dc1",
 		Session:    want.ID,
 	}
 	var out structs.IndexedSessions
 	if err := a.RPC("Session.Get", args, &out); err != nil {
-		t.Fatalf("err: %v", err)
+		r.Fatalf("err: %v", err)
 	}
 	if len(out.Sessions) != 1 {
-		t.Fatalf("bad: %#v", out.Sessions)
+		r.Fatalf("bad: %#v", out.Sessions)
 	}
 
 	// Make a copy so we don't modify the state store copy for an in-mem
@@ -36,14 +34,15 @@ func verifySession(t *testing.T, a *TestAgent, want structs.Session) {
 	got := *(out.Sessions[0])
 	got.CreateIndex = 0
 	got.ModifyIndex = 0
-	verify.Values(t, "", got, want)
+	verify.Values(r, "", got, want)
 }
 
 func TestSessionCreate(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	// Create a health check
 	args := &structs.RegisterRequest{
@@ -58,45 +57,48 @@ func TestSessionCreate(t *testing.T) {
 			Status:    api.HealthPassing,
 		},
 	}
-	var out struct{}
-	if err := a.RPC("Catalog.Register", args, &out); err != nil {
-		t.Fatalf("err: %v", err)
-	}
 
-	// Associate session with node and 2 health checks
+	retry.Run(t, func(r *retry.R) {
-	body := bytes.NewBuffer(nil)
+		var out struct{}
-	enc := json.NewEncoder(body)
+		if err := a.RPC("Catalog.Register", args, &out); err != nil {
-	raw := map[string]interface{}{
+			r.Fatalf("err: %v", err)
-		"Name":      "my-cool-session",
+		}
-		"Node":      a.Config.NodeName,
-		"Checks":    []types.CheckID{structs.SerfCheckID, "consul"},
-		"LockDelay": "20s",
-	}
-	enc.Encode(raw)
 
-	req, _ := http.NewRequest("PUT", "/v1/session/create", body)
+		// Associate session with node and 2 health checks
-	resp := httptest.NewRecorder()
+		body := bytes.NewBuffer(nil)
-	obj, err := a.srv.SessionCreate(resp, req)
+		enc := json.NewEncoder(body)
-	if err != nil {
+		raw := map[string]interface{}{
-		t.Fatalf("err: %v", err)
+			"Name":      "my-cool-session",
-	}
+			"Node":      a.Config.NodeName,
+			"Checks":    []types.CheckID{structs.SerfCheckID, "consul"},
+			"LockDelay": "20s",
+		}
+		enc.Encode(raw)
 
-	want := structs.Session{
+		req, _ := http.NewRequest("PUT", "/v1/session/create", body)
-		ID:        obj.(sessionCreateResponse).ID,
+		resp := httptest.NewRecorder()
-		Name:      "my-cool-session",
+		obj, err := a.srv.SessionCreate(resp, req)
-		Node:      a.Config.NodeName,
+		if err != nil {
-		Checks:    []types.CheckID{structs.SerfCheckID, "consul"},
+			r.Fatalf("err: %v", err)
-		LockDelay: 20 * time.Second,
+		}
-		Behavior:  structs.SessionKeysRelease,
+
-	}
+		want := structs.Session{
-	verifySession(t, a, want)
+			ID:        obj.(sessionCreateResponse).ID,
+			Name:      "my-cool-session",
+			Node:      a.Config.NodeName,
+			Checks:    []types.CheckID{structs.SerfCheckID, "consul"},
+			LockDelay: 20 * time.Second,
+			Behavior:  structs.SessionKeysRelease,
+		}
+		verifySession(r, a, want)
+	})
 }
 
 func TestSessionCreate_Delete(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	// Create a health check
 	args := &structs.RegisterRequest{
@@ -114,7 +116,7 @@ func TestSessionCreate_Delete(t *testing.T) {
 	retry.Run(t, func(r *retry.R) {
 		var out struct{}
 		if err := a.RPC("Catalog.Register", args, &out); err != nil {
-			t.Fatalf("err: %v", err)
+			r.Fatalf("err: %v", err)
 		}
 
 		// Associate session with node and 2 health checks, and make it delete on session destroy
@@ -133,7 +135,7 @@ func TestSessionCreate_Delete(t *testing.T) {
 		resp := httptest.NewRecorder()
 		obj, err := a.srv.SessionCreate(resp, req)
 		if err != nil {
-			t.Fatalf("err: %v", err)
+			r.Fatalf("err: %v", err)
 		}
 
 		want := structs.Session{
@@ -144,7 +146,7 @@ func TestSessionCreate_Delete(t *testing.T) {
 			LockDelay: 20 * time.Second,
 			Behavior:  structs.SessionKeysDelete,
 		}
-		verifySession(t, a, want)
+		verifySession(r, a, want)
 	})
 }
 
@@ -152,7 +154,7 @@ func TestSessionCreate_DefaultCheck(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	// Associate session with node and 2 health checks
 	body := bytes.NewBuffer(nil)
@@ -169,7 +171,7 @@ func TestSessionCreate_DefaultCheck(t *testing.T) {
 	retry.Run(t, func(r *retry.R) {
 		obj, err := a.srv.SessionCreate(resp, req)
 		if err != nil {
-			t.Fatalf("err: %v", err)
+			r.Fatalf("err: %v", err)
 		}
 
 		want := structs.Session{
@@ -180,7 +182,7 @@ func TestSessionCreate_DefaultCheck(t *testing.T) {
 			LockDelay: 20 * time.Second,
 			Behavior:  structs.SessionKeysRelease,
 		}
-		verifySession(t, a, want)
+		verifySession(r, a, want)
 	})
 }
 
@@ -206,7 +208,7 @@ func TestSessionCreate_NoCheck(t *testing.T) {
 	retry.Run(t, func(r *retry.R) {
 		obj, err := a.srv.SessionCreate(resp, req)
 		if err != nil {
-			t.Fatalf("err: %v", err)
+			r.Fatalf("err: %v", err)
 		}
 
 		want := structs.Session{
@@ -217,7 +219,7 @@ func TestSessionCreate_NoCheck(t *testing.T) {
 			LockDelay: 20 * time.Second,
 			Behavior:  structs.SessionKeysRelease,
 		}
-		verifySession(t, a, want)
+		verifySession(r, a, want)
 	})
 }
 
@@ -307,7 +309,7 @@ func TestSessionDestroy(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	id := makeTestSession(t, a.srv)
 
@@ -329,7 +331,8 @@ func TestSessionCustomTTL(t *testing.T) {
 		session_ttl_min = "250ms"
 	`)
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
+
 	retry.Run(t, func(r *retry.R) {
 		id := makeTestSessionTTL(t, a.srv, ttl.String())
 
@@ -337,17 +340,17 @@ func TestSessionCustomTTL(t *testing.T) {
 		resp := httptest.NewRecorder()
 		obj, err := a.srv.SessionGet(resp, req)
 		if err != nil {
-			t.Fatalf("err: %v", err)
+			r.Fatalf("err: %v", err)
 		}
 		respObj, ok := obj.(structs.Sessions)
 		if !ok {
-			t.Fatalf("should work")
+			r.Fatalf("should work")
 		}
 		if len(respObj) != 1 {
-			t.Fatalf("bad: %v", respObj)
+			r.Fatalf("bad: %v", respObj)
 		}
 		if respObj[0].TTL != ttl.String() {
-			t.Fatalf("Incorrect TTL: %s", respObj[0].TTL)
+			r.Fatalf("Incorrect TTL: %s", respObj[0].TTL)
 		}
 
 		time.Sleep(ttl*structs.SessionTTLMultiplier + ttl)
@@ -356,11 +359,11 @@ func TestSessionCustomTTL(t *testing.T) {
 		resp = httptest.NewRecorder()
 		obj, err = a.srv.SessionGet(resp, req)
 		if err != nil {
-			t.Fatalf("err: %v", err)
+			r.Fatalf("err: %v", err)
 		}
 		respObj, ok = obj.(structs.Sessions)
 		if len(respObj) != 0 {
-			t.Fatalf("session '%s' should have been destroyed", id)
+			r.Fatalf("session '%s' should have been destroyed", id)
 		}
 	})
 }
@@ -372,7 +375,7 @@ func TestSessionTTLRenew(t *testing.T) {
 		session_ttl_min = "250ms"
 	`)
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	id := makeTestSessionTTL(t, a.srv, ttl.String())
 
@@ -450,21 +453,21 @@ func TestSessionGet(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
-		testrpc.WaitForLeader(t, a.RPC, "dc1")
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		req, _ := http.NewRequest("GET", "/v1/session/info/adf4238a-882b-9ddc-4a9d-5b6758e4159e", nil)
 		resp := httptest.NewRecorder()
 		retry.Run(t, func(r *retry.R) {
 			obj, err := a.srv.SessionGet(resp, req)
 			if err != nil {
-				t.Fatalf("err: %v", err)
+				r.Fatalf("err: %v", err)
 			}
 			respObj, ok := obj.(structs.Sessions)
 			if !ok {
-				t.Fatalf("should work")
+				r.Fatalf("should work")
 			}
 			if respObj == nil || len(respObj) != 0 {
-				t.Fatalf("bad: %v", respObj)
+				r.Fatalf("bad: %v", respObj)
 			}
 		})
 	})
@@ -472,7 +475,7 @@ func TestSessionGet(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
-		testrpc.WaitForLeader(t, a.RPC, "dc1")
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		id := makeTestSession(t, a.srv)
 
@@ -497,7 +500,7 @@ func TestSessionList(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
-		testrpc.WaitForLeader(t, a.RPC, "dc1")
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		req, _ := http.NewRequest("GET", "/v1/session/list", nil)
 		resp := httptest.NewRecorder()
@@ -517,7 +520,7 @@ func TestSessionList(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
-		testrpc.WaitForLeader(t, a.RPC, "dc1")
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		var ids []string
 		for i := 0; i < 10; i++ {
@@ -545,7 +548,7 @@ func TestSessionsForNode(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
-		testrpc.WaitForLeader(t, a.RPC, "dc1")
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		req, _ := http.NewRequest("GET", "/v1/session/node/"+a.Config.NodeName, nil)
 		resp := httptest.NewRecorder()
@@ -565,7 +568,7 @@ func TestSessionsForNode(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
-		testrpc.WaitForLeader(t, a.RPC, "dc1")
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		var ids []string
 		for i := 0; i < 10; i++ {
@@ -592,7 +595,7 @@ func TestSessionDeleteDestroy(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	id := makeTestSessionDelete(t, a.srv)
 

agent/txn_endpoint_test.go

@@ -9,6 +9,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/consul/testrpc"
+
 	"github.com/hashicorp/consul/agent/structs"
 )
 
@@ -130,6 +132,7 @@ func TestTxnEndpoint_KV_Actions(t *testing.T) {
 	t.Run("", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), "")
 		defer a.Shutdown()
+		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 		// Make sure all incoming fields get converted properly to the internal
 		// RPC format.

command/lock/lock_test.go

@@ -70,7 +70,7 @@ func TestLockCommand_NoShell(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -95,7 +95,7 @@ func TestLockCommand_TryLock(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -129,7 +129,7 @@ func TestLockCommand_TrySemaphore(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -163,7 +163,7 @@ func TestLockCommand_MonitorRetry_Lock_Default(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -198,7 +198,7 @@ func TestLockCommand_MonitorRetry_Semaphore_Default(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -233,7 +233,7 @@ func TestLockCommand_MonitorRetry_Lock_Arg(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -268,7 +268,7 @@ func TestLockCommand_MonitorRetry_Semaphore_Arg(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	ui := cli.NewMockUi()
 	c := New(ui)
@@ -303,32 +303,38 @@ func TestLockCommand_ChildExitCode(t *testing.T) {
 	a := agent.NewTestAgent(t.Name(), ``)
 	defer a.Shutdown()
 
-	testrpc.WaitForLeader(t, a.RPC, "dc1")
+	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
-	t.Run("clean exit", func(t *testing.T) {
+	tt := []struct {
-		ui := cli.NewMockUi()
+		name string
-		c := New(ui)
+		args []string
-		args := []string{"-http-addr=" + a.HTTPAddr(), "-child-exit-code", "test/prefix", "sh", "-c", "exit", "0"}
+		want int
-		if got, want := c.Run(args), 0; got != want {
+	}{
-			t.Fatalf("got %d want %d", got, want)
+		{
-		}
+			name: "clean exit",
-	})
+			args: []string{"-http-addr=" + a.HTTPAddr(), "-child-exit-code", "test/prefix", "sh", "-c", "exit", "0"},
+			want: 0,
+		},
+		{
+			name: "error exit",
+			args: []string{"-http-addr=" + a.HTTPAddr(), "-child-exit-code", "test/prefix", "exit", "1"},
+			want: 2,
+		},
+		{
+			name: "not propagated",
+			args: []string{"-http-addr=" + a.HTTPAddr(), "test/prefix", "sh", "-c", "exit", "1"},
+			want: 0,
+		},
+	}
 
-	t.Run("error exit", func(t *testing.T) {
+	for _, tc := range tt {
-		ui := cli.NewMockUi()
+		t.Run(tc.name, func(t *testing.T) {
-		c := New(ui)
+			ui := cli.NewMockUi()
-		args := []string{"-http-addr=" + a.HTTPAddr(), "-child-exit-code", "test/prefix", "exit", "1"}
+			c := New(ui)
-		if got, want := c.Run(args), 2; got != want {
-			t.Fatalf("got %d want %d", got, want)
-		}
-	})
 
-	t.Run("not propagated", func(t *testing.T) {
+			if got := c.Run(tc.args); got != tc.want {
-		ui := cli.NewMockUi()
+				t.Fatalf("got %d want %d", got, tc.want)
-		c := New(ui)
+			}
-		args := []string{"-http-addr=" + a.HTTPAddr(), "test/prefix", "sh", "-c", "exit", "1"}
+		})
-		if got, want := c.Run(args), 0; got != want {
+	}
-			t.Fatalf("got %d want %d", got, want)
-		}
-	})
 }