From 6d43d24edbd98274c925cbaf2b18005a37dbd7e9 Mon Sep 17 00:00:00 2001 From: Freddy Date: Tue, 14 Aug 2018 12:08:33 -0400 Subject: [PATCH] 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 | 4 + agent/session_endpoint_test.go | 131 +++++++++++++++++---------------- agent/txn_endpoint_test.go | 3 + command/lock/lock_test.go | 70 ++++++++++-------- 4 files changed, 112 insertions(+), 96 deletions(-) diff --git a/agent/kvs_endpoint_test.go b/agent/kvs_endpoint_test.go index 023dff9ebc..71054f25d7 100644 --- a/agent/kvs_endpoint_test.go +++ b/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)) diff --git a/agent/session_endpoint_test.go b/agent/session_endpoint_test.go index a66f6c42bd..2730715921 100644 --- a/agent/session_endpoint_test.go +++ b/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) { - t.Helper() - +func verifySession(r *retry.R, a *TestAgent, want structs.Session) { 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 - body := bytes.NewBuffer(nil) - enc := json.NewEncoder(body) - raw := map[string]interface{}{ - "Name": "my-cool-session", - "Node": a.Config.NodeName, - "Checks": []types.CheckID{structs.SerfCheckID, "consul"}, - "LockDelay": "20s", - } - enc.Encode(raw) + retry.Run(t, func(r *retry.R) { + var out struct{} + if err := a.RPC("Catalog.Register", args, &out); err != nil { + r.Fatalf("err: %v", err) + } - req, _ := http.NewRequest("PUT", "/v1/session/create", body) - resp := httptest.NewRecorder() - obj, err := a.srv.SessionCreate(resp, req) - if err != nil { - t.Fatalf("err: %v", err) - } + // Associate session with node and 2 health checks + body := bytes.NewBuffer(nil) + enc := json.NewEncoder(body) + raw := map[string]interface{}{ + "Name": "my-cool-session", + "Node": a.Config.NodeName, + "Checks": []types.CheckID{structs.SerfCheckID, "consul"}, + "LockDelay": "20s", + } + enc.Encode(raw) - want := structs.Session{ - 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(t, a, want) + req, _ := http.NewRequest("PUT", "/v1/session/create", body) + resp := httptest.NewRecorder() + obj, err := a.srv.SessionCreate(resp, req) + if err != nil { + r.Fatalf("err: %v", err) + } + + want := structs.Session{ + 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) diff --git a/agent/txn_endpoint_test.go b/agent/txn_endpoint_test.go index 35ec3f44ac..7ffdf07d3e 100644 --- a/agent/txn_endpoint_test.go +++ b/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. diff --git a/command/lock/lock_test.go b/command/lock/lock_test.go index b04be5e875..0002264ebb 100644 --- a/command/lock/lock_test.go +++ b/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) { - ui := cli.NewMockUi() - c := New(ui) - args := []string{"-http-addr=" + a.HTTPAddr(), "-child-exit-code", "test/prefix", "sh", "-c", "exit", "0"} - if got, want := c.Run(args), 0; got != want { - t.Fatalf("got %d want %d", got, want) - } - }) + tt := []struct { + name string + args []string + want int + }{ + { + 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) { - ui := cli.NewMockUi() - c := New(ui) - args := []string{"-http-addr=" + a.HTTPAddr(), "-child-exit-code", "test/prefix", "exit", "1"} - if got, want := c.Run(args), 2; got != want { - t.Fatalf("got %d want %d", got, want) - } - }) + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + ui := cli.NewMockUi() + c := New(ui) - t.Run("not propagated", func(t *testing.T) { - ui := cli.NewMockUi() - 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) - } - }) + if got := c.Run(tc.args); got != tc.want { + t.Fatalf("got %d want %d", got, tc.want) + } + }) + } }