agent/http: use httptest.Server for handler tests
#308
Conversation
| resp := recorder.Result() | ||
| proxyServer := httptest.NewServer(handler) | ||
|
|
||
| req, err := http.NewRequest(tc.method, proxyServer.URL+tc.path, bytes.NewReader(tc.body)) |
There was a problem hiding this comment.
Reading this in the diff, it is not fully clear to me whether tc.body was intended to be the request body in the first place, specially given that all requests are (currently) GET requests. Should I get rid of this and add a new requestBody field to the test table?
There was a problem hiding this comment.
IIRC the body was part of the response generated by the fake client:
Body: io.NopCloser(strings.NewReader(string(f.body)))
Passing it as part of the request was an error in the test code. What I don't understand is your proposal of getting rid of it, as it is should be returned by the upstream server's handler.
There was a problem hiding this comment.
If I understand the code correctly, the request we're performing here would be the client request, meaning the first request of the flow. Am I right? If this is the case, I believe the body for that request belongs to the test case (if we want to add it).
There was a problem hiding this comment.
I've renamed the fields to clarify these are used to control the upstream server response, and fixed the bug where they were being sent by the client 371bb82
Does this align with what you said?
| handler := &httpHandler{ | ||
| upstreamURL: *upstreamURL, | ||
| client: client, | ||
| client: http.DefaultClient, |
There was a problem hiding this comment.
client was introduced only for testing. If we are now always using the DefaultClient, maybe we could simply remove this field.
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| t.Fatalf("making request to proxy: %v", err) | ||
| } | ||
|
|
||
| if tc.expectedStatus != resp.StatusCode { | ||
| t.Errorf("expected status code '%d' but '%d' received ", tc.expectedStatus, resp.StatusCode) |
There was a problem hiding this comment.
As you are using t.Fatal above, shouldn't we use it uniformly at least in this test?
Description
Fixes #305
This PR makes minor changes to http proxy handler tests so they are performed with
httptest.Serverinstead of the fake client. This aligns existing tests with the newly added metric tests, and provides more coverage as implementation details of the whole Go HTTP stack is tested, instead of just the handler.Checklist:
make lint) and all checks pass.make test) and all tests pass.make e2e-xxxforagent,disruptors,kubernetesorclusterrelated changes)