Adds a test to ensure URL paths are lower case #68
Conversation
pkg/frontend/frontend_test.go
Outdated
| f := &frontend{} | ||
|
|
||
| fakeRouter := mux.NewRouter() | ||
| f.unauthenticatedRoutes(fakeRouter.NewRoute().Subrouter()) |
There was a problem hiding this comment.
I would be up for seeing the router setup in frontend.Run pulled out into a nonexported method to avoid the duplication here, wdyt?
pkg/frontend/frontend_test.go
Outdated
|
|
||
| // Ignore the error: it can occur when a route has no path, | ||
| // but there is no way to check it here | ||
| if err == nil { |
There was a problem hiding this comment.
nit: if err != nil { continue } reduces indentation
There was a problem hiding this comment.
It's not a loop, but I changed it in a similar way with return nil
pkg/frontend/frontend_test.go
Outdated
| if err == nil { | ||
| cleanPathTemplate := varCleanupRe.ReplaceAllString(pathTemplate, "") | ||
| if cleanPathTemplate != strings.ToLower(cleanPathTemplate) { | ||
| t.Fatal(pathTemplate) |
There was a problem hiding this comment.
nit t.Error() will allow someone to see all the faulty routes in one go.
m1kola
force-pushed
the
URL_paths_are_lower_case
branch
from
ed5c33e
to
6861068
Compare
| f.s = &http.Server{ | ||
| Handler: middleware.Lowercase(r), | ||
| Handler: middleware.Lowercase(f.setupRouter()), |
There was a problem hiding this comment.
nit for next time, might be worth getting middleware.Lowercase() in setupRouter too.
There was a problem hiding this comment.
@jim-minter yeah, I was thinking about it, but decided to keep it here, because I wasn't sure why it wasn't in router itself initially. My guess was: a middleware added like r.Use(middleware.Lowercase) will only be called on a route match, but we might want lowercase 404s.
Fixes #64