-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Enhance rules tests. #1679
Enhance rules tests. #1679
Conversation
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.
👍
LGTM
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.
Some very smallish comments. :-)
@@ -16,7 +16,7 @@ BIND_DIR := "dist" | |||
TRAEFIK_MOUNT := -v "$(CURDIR)/$(BIND_DIR):/go/src/github.com/containous/traefik/$(BIND_DIR)" | |||
|
|||
GIT_BRANCH := $(subst heads/,,$(shell git rev-parse --abbrev-ref HEAD 2>/dev/null)) | |||
TRAEFIK_DEV_IMAGE := traefik-dev$(if $(GIT_BRANCH),:$(GIT_BRANCH)) | |||
TRAEFIK_DEV_IMAGE := traefik-dev$(if $(GIT_BRANCH),:$(subst /,-,$(GIT_BRANCH))) |
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.
Nice, this one has bothered me before!
middlewares/replace_path_test.go
Outdated
if oldPath != path { | ||
t.Fatalf("old path should be '%s'", path) | ||
} | ||
assert.Equal(t, replacementPath, expectedPath, "%s: unexpected path.", path) |
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.
Expected must go before actual or otherwise the labeling will be wrong in assert.Equal
's output.
middlewares/stripPrefixRegex_test.go
Outdated
t.Errorf("Expected '%s' : '%s'\n", test.expectedResponse, response) | ||
} | ||
test := test | ||
t.Run(test.desc, func(t *testing.T) { |
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.
For the test cases at hand, it would also suffice to pass path
to t.Run
as it's disambiguating already, and the desc
riptions aren't overly -- descriptive. :)
a6c1a0d
to
1328bdb
Compare
@timoreimann It's fixed! |
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.
Aaaaaand... it's approved!
- refactor: change incorrect package. - refactor: test readability.
72ffc48
to
9ac43e5
Compare
Description
Homogenization of the tests.
The naming of cases is not perfect for
stripPrefixRegex_test
but it is more maintainable.Use
r.Header.Add("MY-HEADER", value)
instead ofr.Header["MY-HEADER"] = []string{value}
.Bonus:
/
likefoo/bar
.