[refactor] Test endpoints through ServeHTTP, not by raw handler

[kisunji]

Background

In many places in our tests, when we want to make API calls to handlers, we tend to use the raw handler (e.g. a.srv.AgentToken(resp, req)).

This is problematic because it skips the middleware that real HTTP requests go through, which includes (but is not limited to) multiplexing based on URL patterns, blocking not-allowed HTTP methods, and translating application-level errors to HTTP error codes and messages.

This means that developer errors like this may not get caught by the test suite:

// http_register.go
// registerEndpoint("/v1/agent/join/", []string{"PUT"}, (*HTTPHandlers).AgentJoin)

req, _ := http.NewRequest("GET", "/v1/agent/node", nil) // wrong method and endpoint

resp := httptest.NewRecorder()
_, err := a.srv.AgentJoin(resp, req) // does not immediately reject the request

AgentJoin is not responsible for validating the request's URL or method and may fail in unpredictable ways, wasting developer time or misleading devs to make incorrect test assertions.

Solution

The proposed solution is to start using Agent.srv.h.ServeHTTP(resp, req) to handle requests generically instead of specifying a typed handler.

See examples here.

Related PRs

#11445 (thanks @Mathew-Estafanous)

[Mathew-Estafanous]

Hi @kisunji, I’m interested in attempting to work on this issue. I just have a couple questions as I am working on it.

1. Do we need to replace every test case that makes direct raw calls through *HTTPHandler? So this includes test cases like a.srv.AgentHealthServiceByID(resp, req), obj, err := a.srv.Txn(resp, req), etc. which all need to go through the proper a.srv.h.ServeHTTP(resp, req) handler.

2. Secondly I’ve noticed that some of these test cases are checking for specific internal errors. The following are some examples of what I’m referring to. How should I approach these cases?
   

   consul/agent/agent_endpoint_test.go

   Lines 5400 to 5404 in ab425e3

   	_, err := a.srv.AgentToken(resp, req)
   	if tt.expectedErr != "" {
   	require.EqualError(t, err, tt.expectedErr)
   	return
   	}

   
   ^^ In this case, I just checked the http status code that is returned by the http error.
   Below is an example of what I was thinking of doing.

a.srv.h.ServeHTTP(resp, req)
require.Equal(t, tt.code, resp.Code)

then I removed expectedErr and in turn made those test cases check the status code.

tests := []struct {
		name        string
		method, url string
		body        io.Reader
		code        int
		init        tokens
		raw         tokens
		effective   tokens
	}{
		{
			name:   "bad token name",
			method: "PUT",
			url:    "nope?token=root",
			body:   body("X"),
			code:   http.StatusNotFound,
		},
		{
			name:   "bad JSON",
			method: "PUT",
			url:    "acl_token?token=root",
			body:   badJSON(),
			code:   http.StatusBadRequest,
		},

---

Now for this case it is checking for a specific internal error errPermissionDenied so I assume I could do something similar to what I did previously and just check for a 404 Forbidden status code.

consul/agent/agent_endpoint_test.go

Lines 5435 to 5436 in ab425e3

	_, err := a.srv.AgentToken(nil, req)
	require.True(t, acl.IsErrPermissionDenied(err))

Is it ok that I’m no longer checking for specific internal errors and only checking for the response code? If not, how could I approach checking the internal error while also calling ServeHTTP?

---

Here is another case where I’m not exactly sure how I could handle if I were to change to using ServeHTTP. There are several other cases where they are checking internal objects, etc.

consul/agent/agent_endpoint_test.go

Lines 66 to 70 in ab425e3

	tokInf, err := srv.ACLTokenCreate(resp, req)
	require.NoError(t, err)
	svcToken, ok := tokInf.(*structs.ACLToken)
	require.True(t, ok)
	return svcToken.SecretID

[kisunji]

Hi @Mathew-Estafanous, thank you for your interest in contributing!

Given that the scope of this issue is fairly large, we encourage you to start with one or two tests to keep the PR small (helps the team during review as well).

1. Yes, ideally we'd like to replace every call. Practically speaking, there may be many exceptions or places that are confusing / unclear on what is being asserted. It is okay to skip those (maybe label with a // TODO(GH#11396)).
2. Since we are testing HTTP handlers, we want to test 1) the HTTP response code; and 2) the response body which should contain the application error message. It should be okay to replace error comparisons with string comparisons (or even require.Contains).

Regarding your last point about tests that check for typed objects, here is an example of a struct metrics.MetricsSummary being decoded from the response body:

consul/agent/agent_endpoint_test.go

Lines 1499 to 1502 in 01e9740

	decoder := json.NewDecoder(resp.Body)
	var summary metrics.MetricsSummary
	err = decoder.Decode(&summary)
	require.NoError(t, err)

Minor caveat

Our team is discussing the nature of these HTTP-handler tests and what they should assert; there are some shortcomings in converting the json response back to a typed go struct that we need to address internally.

There is a chance our goalposts may change throughout the process but we welcome your PR to help kickstart that conversation.