[bugfix] Fix method subrouter handler matching (#300)#317
[bugfix] Fix method subrouter handler matching (#300)#317kisielk merged 7 commits intogorilla:masterfrom
Conversation
mux_test.go
Outdated
|
|
||
| // TestMethodsSubrouter tests the correct matching of handlers for | ||
| // subrouters registered after a route's method-matcher is set. | ||
| func TestMethodsSubrouter(t *testing.T) { |
There was a problem hiding this comment.
It may be better to break this up a little, to allow parallel test execution (via t.Parallel()),.
mux_test.go
Outdated
| // TestMethodsSubrouter tests the correct matching of handlers for | ||
| // subrouters registered after a route's method-matcher is set. | ||
| func TestMethodsSubrouter(t *testing.T) { | ||
| flags1 := make([]bool, 3) |
There was a problem hiding this comment.
It isn't obvious (e.g. at first) what the purpose of the slice of bools / flags are for at first. Can you add a comment here to aid future contributors, and/or (better) look at a better way to test these that doesn't require this state tracking?
There was a problem hiding this comment.
Yes, I can try that, would inspecting the response body be better?
There was a problem hiding this comment.
I actually don't quite understand it, is it just to show which methods matched? Why not use a map[string]bool for this?
There was a problem hiding this comment.
The flags are used to check which handler is invoked. I wanted to check sequentially (for when two same routes are registered, but only the first one should be matched), but I can use map[string]bool with [method][number] like get1 and get2.
There was a problem hiding this comment.
I would also call it something like matched_methods instead of just flags. The tests for this package are pretty confusing and convoluted, so anything that helps add some clarity is a big plus.
|
Broke up
|
elithrar
left a comment
There was a problem hiding this comment.
Minor style changes, but otherwise: thanks for refactoring the tests. Much more readable now! 👌
| } | ||
|
|
||
| // TestMethodsSubrouterPathPrefix matches handlers on subrouters created | ||
| // on a router with a path prefix matcher and method matcher. |
mux_test.go
Outdated
| switch test.wantCode { | ||
| case http.StatusMethodNotAllowed: | ||
| if resp.Code != http.StatusMethodNotAllowed { | ||
| t.Errorf("(%s) Expected \"405 Method Not Allowed\", but got %d code", test.title, resp.Code) |
There was a problem hiding this comment.
Use backticks (`) for raw strings so you don't have to escape quotes.
|
The relevant string literals have been updated to use backticks. Thanks for both your helpful feedback! |
mux_test.go
Outdated
| ) | ||
|
|
||
| // Method handlers return the method string. | ||
| var ( |
There was a problem hiding this comment.
I think this would make it more clear:
func methodHandler(method string) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(method))
}
}
That way when you're defining the test fixtures, the string returned by the handler is right there in the fixture as opposed to these global functions.
There was a problem hiding this comment.
Ok, makes sense. I noticed the HTTP methods in other tests are raw strings. Should these constants be replaced, too?
There was a problem hiding this comment.
You could probably just the put the strings inline, yeah. I don't think it helps much to have them as constants here
|
The global method handler variables and constants have been removed. |
|
LGTM! Thanks |
|
Thanks for the contribution @mtso! |
Fix handler match for subrouters created from method-matcher-based routes where the paths match (issue #300).
Route.Match