From 587dcebfcac0e9abe29a7bd55174d77fc33a9cb7 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 14 Aug 2020 16:54:44 -0400 Subject: [PATCH 1/2] Use t.Helper in testutil/retry --- sdk/testutil/retry/retry.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sdk/testutil/retry/retry.go b/sdk/testutil/retry/retry.go index 53c05a2b05..603c47346a 100644 --- a/sdk/testutil/retry/retry.go +++ b/sdk/testutil/retry/retry.go @@ -23,6 +23,8 @@ import ( // Failer is an interface compatible with testing.T. type Failer interface { + Helper() + // Log is called for the final test output Log(args ...interface{}) @@ -116,6 +118,7 @@ func dedup(a []string) string { func run(r Retryer, t Failer, f func(r *R)) { rr := &R{} fail := func() { + t.Helper() out := dedup(rr.output) if out != "" { t.Log(out) From a4b201af36384e6d5ac3028dc854dcd127e6081e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 13 Aug 2020 18:39:58 -0400 Subject: [PATCH 2/2] testing: Improve session_endpoint_test While working on another change I caused a bunch of these tests to fail. Unfortunately the failure messages were not super helpful at first. One problem was that the request and response were created outside of the retry. This meant that when the second attempt happened, the request body was empty (because the buffer had been consumed), and so the request was not actually being retried. This was fixed by moving more of the request creation into the retry block. Another problem was that these functions can return errors in two ways, and are not consistent about which way they use. Some errors are returned to the response writer, but the tests were not checking those errors, which was causing a panic later on. This was fixed by adding a check for the response code. Also adds some missing t.Helper(), and has assertIndex use checkIndex so that it is clear these are the same implementation. --- agent/http_test.go | 6 +-- agent/session_endpoint_test.go | 95 ++++++++++++++++------------------ agent/ui_endpoint_test.go | 6 +-- 3 files changed, 50 insertions(+), 57 deletions(-) diff --git a/agent/http_test.go b/agent/http_test.go index 9136c1eddd..d5c4d827bb 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -1298,10 +1298,8 @@ func TestAllowedNets(t *testing.T) { // assertIndex tests that X-Consul-Index is set and non-zero func assertIndex(t *testing.T, resp *httptest.ResponseRecorder) { - header := resp.Header().Get("X-Consul-Index") - if header == "" || header == "0" { - t.Fatalf("Bad: %v", header) - } + t.Helper() + require.NoError(t, checkIndex(resp)) } // checkIndex is like assertIndex but returns an error diff --git a/agent/session_endpoint_test.go b/agent/session_endpoint_test.go index 144aa26a3f..4690ebfbb7 100644 --- a/agent/session_endpoint_test.go +++ b/agent/session_endpoint_test.go @@ -97,7 +97,7 @@ func TestSessionCreate(t *testing.T) { "Checks": []types.CheckID{"consul"}, "LockDelay": "20s", } - enc.Encode(raw) + require.NoError(r, enc.Encode(raw)) req, _ := http.NewRequest("PUT", "/v1/session/create", body) resp := httptest.NewRecorder() @@ -158,7 +158,7 @@ func TestSessionCreate_NodeChecks(t *testing.T) { "NodeChecks": []types.CheckID{structs.SerfCheckID}, "LockDelay": "20s", } - enc.Encode(raw) + require.NoError(r, enc.Encode(raw)) req, _ := http.NewRequest("PUT", "/v1/session/create", body) resp := httptest.NewRecorder() @@ -216,7 +216,7 @@ func TestSessionCreate_Delete(t *testing.T) { "LockDelay": "20s", "Behavior": structs.SessionKeysDelete, } - enc.Encode(raw) + require.NoError(r, enc.Encode(raw)) req, _ := http.NewRequest("PUT", "/v1/session/create", body) resp := httptest.NewRecorder() @@ -244,23 +244,21 @@ func TestSessionCreate_DefaultCheck(t *testing.T) { defer a.Shutdown() testrpc.WaitForTestAgent(t, a.RPC, "dc1") - // 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, "LockDelay": "20s", } - enc.Encode(raw) - req, _ := http.NewRequest("PUT", "/v1/session/create", body) - resp := httptest.NewRecorder() retry.Run(t, func(r *retry.R) { + body := bytes.NewBuffer(nil) + enc := json.NewEncoder(body) + require.NoError(r, enc.Encode(raw)) + 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) - } + require.NoError(r, err) + require.Equal(r, resp.Code, http.StatusOK) want := structs.Session{ ID: obj.(sessionCreateResponse).ID, @@ -281,26 +279,23 @@ func TestSessionCreate_NoCheck(t *testing.T) { testrpc.WaitForTestAgent(t, a.RPC, "dc1") - t.Run("no check fields should yield default serfHealth", func(t *testing.T) { - body := bytes.NewBuffer(nil) - enc := json.NewEncoder(body) - raw := map[string]interface{}{ - "Name": "my-cool-session", - "Node": a.Config.NodeName, - "LockDelay": "20s", - } - enc.Encode(raw) + raw := map[string]interface{}{ + "Name": "my-cool-session", + "Node": a.Config.NodeName, + "LockDelay": "20s", + } - req, _ := http.NewRequest("PUT", "/v1/session/create", body) - resp := httptest.NewRecorder() + t.Run("no check fields should yield default serfHealth", func(t *testing.T) { retry.Run(t, func(r *retry.R) { + body := bytes.NewBuffer(nil) + enc := json.NewEncoder(body) + require.NoError(r, enc.Encode(raw)) + + 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) - } - if obj == nil { - r.Fatalf("expected a session") - } + require.NoError(r, err) + require.Equal(r, resp.Code, http.StatusOK, resp.Body.String()) want := structs.Session{ ID: obj.(sessionCreateResponse).ID, @@ -315,23 +310,22 @@ func TestSessionCreate_NoCheck(t *testing.T) { }) t.Run("overwrite nodechecks to associate with no checks", func(t *testing.T) { - body := bytes.NewBuffer(nil) - enc := json.NewEncoder(body) raw := map[string]interface{}{ "Name": "my-cool-session", "Node": a.Config.NodeName, "NodeChecks": []string{}, "LockDelay": "20s", } - enc.Encode(raw) - req, _ := http.NewRequest("PUT", "/v1/session/create", body) - resp := httptest.NewRecorder() retry.Run(t, func(r *retry.R) { + body := bytes.NewBuffer(nil) + enc := json.NewEncoder(body) + require.NoError(r, enc.Encode(raw)) + 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) - } + require.NoError(r, err) + require.Equal(r, resp.Code, http.StatusOK) want := structs.Session{ ID: obj.(sessionCreateResponse).ID, @@ -346,23 +340,23 @@ func TestSessionCreate_NoCheck(t *testing.T) { }) t.Run("overwrite checks to associate with no checks", func(t *testing.T) { - body := bytes.NewBuffer(nil) - enc := json.NewEncoder(body) raw := map[string]interface{}{ "Name": "my-cool-session", "Node": a.Config.NodeName, "Checks": []string{}, "LockDelay": "20s", } - enc.Encode(raw) - req, _ := http.NewRequest("PUT", "/v1/session/create", body) - resp := httptest.NewRecorder() retry.Run(t, func(r *retry.R) { + body := bytes.NewBuffer(nil) + enc := json.NewEncoder(body) + require.NoError(r, enc.Encode(raw)) + + 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) - } + require.NoError(r, err) + require.Equal(r, resp.Code, http.StatusOK) want := structs.Session{ ID: obj.(sessionCreateResponse).ID, @@ -379,6 +373,7 @@ func TestSessionCreate_NoCheck(t *testing.T) { } func makeTestSession(t *testing.T, srv *HTTPServer) string { + t.Helper() url := "/v1/session/create" req, _ := http.NewRequest("PUT", url, nil) resp := httptest.NewRecorder() @@ -391,13 +386,14 @@ func makeTestSession(t *testing.T, srv *HTTPServer) string { } func makeTestSessionDelete(t *testing.T, srv *HTTPServer) string { + t.Helper() // Create Session with delete behavior body := bytes.NewBuffer(nil) enc := json.NewEncoder(body) raw := map[string]interface{}{ "Behavior": "delete", } - enc.Encode(raw) + require.NoError(t, enc.Encode(raw)) url := "/v1/session/create" req, _ := http.NewRequest("PUT", url, body) @@ -411,13 +407,14 @@ func makeTestSessionDelete(t *testing.T, srv *HTTPServer) string { } func makeTestSessionTTL(t *testing.T, srv *HTTPServer, ttl string) string { + t.Helper() // Create Session with TTL body := bytes.NewBuffer(nil) enc := json.NewEncoder(body) raw := map[string]interface{}{ "TTL": ttl, } - enc.Encode(raw) + require.NoError(t, enc.Encode(raw)) url := "/v1/session/create" req, _ := http.NewRequest("PUT", url, body) @@ -586,9 +583,9 @@ func TestSessionGet(t *testing.T) { defer a.Shutdown() 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) { + req, _ := http.NewRequest("GET", "/v1/session/info/adf4238a-882b-9ddc-4a9d-5b6758e4159e", nil) + resp := httptest.NewRecorder() obj, err := a.srv.SessionGet(resp, req) if err != nil { r.Fatalf("err: %v", err) diff --git a/agent/ui_endpoint_test.go b/agent/ui_endpoint_test.go index eab3c4193c..d9bdc3d9b4 100644 --- a/agent/ui_endpoint_test.go +++ b/agent/ui_endpoint_test.go @@ -155,10 +155,8 @@ func TestUiNodeInfo(t *testing.T) { req, _ := http.NewRequest("GET", fmt.Sprintf("/v1/internal/ui/node/%s", a.Config.NodeName), nil) resp := httptest.NewRecorder() obj, err := a.srv.UINodeInfo(resp, req) - if err != nil { - t.Fatalf("err: %v", err) - } - + require.NoError(t, err) + require.Equal(t, resp.Code, http.StatusOK) assertIndex(t, resp) // Should be 1 node for the server