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

@@ -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")
+	t.Helper()
-	if header == "" || header == "0" {
+	require.NoError(t, checkIndex(resp))
-		t.Fatalf("Bad: %v", header)
-	}
 }
 
 // checkIndex is like assertIndex but returns an error

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)
 
+	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()
-	retry.Run(t, func(r *retry.R) {
 		obj, err := a.srv.SessionCreate(resp, req)
-		if err != nil {
+		require.NoError(r, err)
-			r.Fatalf("err: %v", 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)
+
+	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()
-		retry.Run(t, func(r *retry.R) {
 			obj, err := a.srv.SessionCreate(resp, req)
-			if err != nil {
+			require.NoError(r, err)
-				r.Fatalf("err: %v", err)
+			require.Equal(r, resp.Code, http.StatusOK, resp.Body.String())
-			}
-			if obj == nil {
-				r.Fatalf("expected a session")
-			}
 
 			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)
 
+		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()
-		retry.Run(t, func(r *retry.R) {
 			obj, err := a.srv.SessionCreate(resp, req)
-			if err != nil {
+			require.NoError(r, err)
-				r.Fatalf("err: %v", 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)
+
+		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()
-		retry.Run(t, func(r *retry.R) {
 			obj, err := a.srv.SessionCreate(resp, req)
-			if err != nil {
+			require.NoError(r, err)
-				r.Fatalf("err: %v", 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")
 
+		retry.Run(t, func(r *retry.R) {
 			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 {
 				r.Fatalf("err: %v", err)

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 {
+	require.NoError(t, err)
-		t.Fatalf("err: %v", err)
+	require.Equal(t, resp.Code, http.StatusOK)
-	}
-
 	assertIndex(t, resp)
 
 	// Should be 1 node for the server