New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test for HandleHTTP #9
Changes from 1 commit
018a501
672ab39
1153dbb
076ed32
1e1f7b4
58123eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -365,3 +365,18 @@ func TestHandleHTTPHEADRequest(t *testing.T) { | |
interceptor.HandleHTTP(w, req, handler) | ||
st.Expect(t, string(w.Body), "Hi") | ||
} | ||
|
||
func TestHandleHTTPOPTIONSRequest(t *testing.T) { | ||
modifierFunc := func(m *RequestModifier) { | ||
m.Header.Set("foo", "bar") | ||
} | ||
w := utils.NewWriterStub() | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just one point: it's not supposed that you have intercepted the request adding the The response handler is called since the body is properly defined, but the header is not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a Header key is not present, calling I just realized that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the PR to remove the |
||
}) | ||
interceptor := Request(modifierFunc) | ||
interceptor.HandleHTTP(w, req, handler) | ||
st.Expect(t, string(w.Body), "Hi") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.