Add test for HandleHTTP #9
Conversation
groyoh
force-pushed
the
add_test_for_handle_http
branch
from
0d3da04
to
5f97c2a
Compare
groyoh
force-pushed
the
add_test_for_handle_http
branch
from
5f97c2a
to
1e1f7b4
Compare
|
Changes Unknown when pulling 1e1f7b4 on groyoh:add_test_for_handle_http into * on vinxi:master*. |
| req := &http.Request{Method: "OPTIONS", Header: make(http.Header)} | ||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| fmt.Fprint(w, "Hi") | ||
| st.Expect(t, r.Header.Get("foo"), "") |
There was a problem hiding this comment.
Just one point: it's not supposed that you have intercepted the request adding the foo header? Why then the test asserts it as empty string? It shouldn't be present?
The response handler is called since the body is properly defined, but the header is not.
There was a problem hiding this comment.
When a Header key is not present, calling Get on it, returns an empty string. Ref. https://golang.org/pkg/net/http/#Header.Get
I just realized that Header is actually a map so I could probably use _, ok := Header["foo"] and check the value of ok. Does that makes more sense to you?
There was a problem hiding this comment.
No no, it's fine as it is. This is the expected behavior since Go it's strict in terms of type enforcement.
I was pointing that out just because I think the test is not working, since the request should be intercepted and the header should be added, but seems like it doesn't. In other words, seems like modifierFunc is not called.
Maybe I'm wrong because it's a bit late and my brain could not work very well :P
There was a problem hiding this comment.
That's the expected behavior for HEAD and OPTIONS (or at least the behavior you wrote :D) https://github.com/vinxi/intercept/blob/master/request.go#L185
There was a problem hiding this comment.
Damn, I suppose I was forgetting something 👍 .
In any case, this opened another discussion. Do you think it makes sense to ignore those request methods? HEAD method is not too much used, but useful in REST or RPC models to perform health checks or similar verification only requests. OPTIONS could be useful to modify CORS headers and perform further checks. Since now the devs can add custom filters or even they can use mux package to compose middleware based on specific filters in order to ignore certain traffic, so this could be useless and limiting for some devs. Opinions?
There was a problem hiding this comment.
I also think that we should not ignore these methods by default. I actually planned to open an issue for this but I wanted to take a look again at the HTTP specs to make sure how these two methods work 😄
There was a problem hiding this comment.
Updated the PR to remove the HEAD/OPTIONS check.
|
I guess this is only for |
|
Yes, that's only for the request. I don't want to add too much at one cause I'm not always sure how to handle things. |
| m.Header.Set("foo", "bar") | ||
| }) | ||
| w := utils.NewWriterStub() | ||
| req := &http.Request{Method: "HEAD", Header: make(http.Header)} | ||
| interceptor.HandleHTTP(w, req, handler) | ||
| st.Expect(t, string(w.Body), "Hi") | ||
| } | ||
|
|
||
| func TestHandleHTTPOPTIONSRequest(t *testing.T) { |
There was a problem hiding this comment.
I would rename test functions into TestHandleHTTPOptionsRequest.
It might be just a personal style preference, but for me the HTTP verb name is upper case mostly inside the HTTP protocol domain & spec, not strictly required in program semantics, such as methods/function names.
HTTP keyword instead is an acronym and the idiomatic Go also points about keeping acronyms with original case notations.
There was a problem hiding this comment.
Yeah I agree. I thought about it when I was reviewing the PR but decided against because I didn't know what's the idiomatic way to write it in Go.
|
Changes Unknown when pulling 58123eb on groyoh:add_test_for_handle_http into * on vinxi:master*. |
@h2non I'm not sure if these tests were properly handled. IMO it properly tests every part (writer, request, interception) of the method but there may be a cleaner/better way to do that.
I'll start working on the filters for the request once this is merged.