[bugfix] Fix method subrouter handler matching (#300)

[mtso]

Fix handler match for subrouters created from method-matcher-based routes where the paths match (issue #300).

• Add a test case for routers with methods-based subrouters and matching paths
• Pass the test case with an addition to Route.Match

[elithrar]

It may be better to break this up a little, to allow parallel test execution (via t.Parallel()),.

[mtso]

Ok, I can try that.

[elithrar]

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?

[mtso]

Yes, I can try that, would inspecting the response body be better?

[kisielk]

I actually don't quite understand it, is it just to show which methods matched? Why not use a map[string]bool for this?

[mtso]

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.

[kisielk]

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.

[mtso]

Broke up TestMethodsSubrouter into five separate test cases based on the router initialized for each one. t.Parallel is also invoked in each one, so they run concurrently. Breaking them up further without t.Run would make the router initialization strategy more verbose.

methodsSubrouterTest has also been made stateless so the handler-match assertions are cleaner.

[elithrar]

Minor style changes, but otherwise: thanks for refactoring the tests. Much more readable now! 👌

[elithrar]

👍

[elithrar]

Use backticks (`) for raw strings so you don't have to escape quotes.

[mtso]

The relevant string literals have been updated to use backticks. Thanks for both your helpful feedback!

[kisielk]

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.

[mtso]

Ok, makes sense. I noticed the HTTP methods in other tests are raw strings. Should these constants be replaced, too?

[kisielk]

You could probably just the put the strings inline, yeah. I don't think it helps much to have them as constants here

[mtso]

The global method handler variables and constants have been removed.

[kisielk]

LGTM! Thanks

[elithrar]

Thanks for the contribution @mtso!