Simplify stripPrefix and stripPrefixRegex tests #1699

ldez · 2017-06-01T20:12:36Z

Description

Simplify stripPrefix and stripPrefixRegex tests.
typo in whitelister file name.

fix flaky tests:

--- FAIL: TestStripPrefix (0.00s)
    --- FAIL: TestStripPrefix/general_prefix_on_matching_path (0.00s)
    	assertions.go:226:                      
	Error Trace:	stripPrefix_test.go:97
	Error:      	Received unexpected error:
	            	Get http://127.0.0.1:52739/stat/: net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called
	Messages:   	Failed to send GET request

timoreimann · 2017-06-03T06:24:14Z

middlewares/replace_path_test.go

@@ -27,8 +28,8 @@ func TestReplacePath(t *testing.T) {
 				}),
 			}

-			req, err := http.NewRequest("GET", "http://localhost"+path, nil)
-			assert.NoError(t, err, "%s: unexpected error.", path)
+			req, err := http.NewRequest(http.MethodGet, "http://localhost"+path, nil)


We can use testhelpers.MustNewRequest here and below.

timoreimann · 2017-06-03T09:44:36Z

It is conventional in Go that all Must* functions / methods panic. The assumption here is that when you call them, they must only ever fail when you made a logical programming error. When that happens, you have broken an implicit invariant that you want to fix immediately like a compiler error. That's the reason for the immediate panic which will show up right in your face (as opposed to being handled somewhat gracefully when doing t.Fatal).

(See regexp.MustCompile or Prometheus' MustRegister for similar examples.)

Can we return to the original signature of MustNewRequest?

ldez · 2017-06-03T10:15:38Z

@timoreimann or we can remove the Must prefix: it's a test helper, use t seems normal. FYT?

emilevauge

LGTM

timoreimann

Nice cleanup!

Left a few last comments.

timoreimann · 2017-06-06T19:12:05Z

healthcheck/healthcheck_test.go

@@ -182,12 +111,12 @@ func TestSetBackendsConfiguration(t *testing.T) {

 			lb.Lock()
 			defer lb.Unlock()
-			if lb.numRemovedServers != test.wantNumRemovedServers {
-				t.Errorf("got %d removed servers, wanted %d", lb.numRemovedServers, test.wantNumRemovedServers)
+			if test.wantNumRemovedServers != lb.numRemovedServers {


The order in Go (for stdlib testing) is got first, want second; both in the comparison expression and the error message.

timoreimann · 2017-06-06T19:12:50Z

healthcheck/healthcheck_test.go

 	"github.com/vulcand/oxy/roundrobin"
 )

 const healthCheckInterval = 100 * time.Millisecond

-type testLoadBalancer struct {


Any particular reason why you moved this to the bottom? Does any convention exist that defines the order?

It's a conventional way: when you read test, you beginning by read the test and after you read some test helpers. To read test you don't need to known the test implementation details.

You could also argue that you do need to know the implementation details in order to understand the test.

I don't mind too much either way though, was more interested in the motivation.

you do need to know the implementation details in order to understand the test

It's a bad test for me.
A test must clear, readable and understandable by itself.

My main motivation: test readability, always the readability 😃

timoreimann · 2017-06-06T19:14:12Z

middlewares/addPrefix_test.go

+		}),
+	}
+
+	req := testhelpers.MustNewRequest("GET", "http://localhost"+path, nil)


One unused http.MethodGet slipped through here. :-)

timoreimann · 2017-06-06T19:17:21Z

middlewares/stripPrefixRegex_test.go


-			assert.Equal(t, test.expectedStatusCode, resp.StatusCode, "%s: unexpected status code.", test.path)
+			assert.Equal(t, test.expectedStatusCode, resp.Code, "%s: unexpected status code.", test.path)


You don't need to include test.path in the error description; the sub-test (which we parameterized with the path) will do that for you.

I need to do that because I don't see the context case (sub-test) when a test failed.

/home/ldez/.gvm/gos/go1.8.3/bin/go test -c -i -o /tmp/TestStripPrefixRegex_in_stripPrefixRegex_test_gogo github.com/containous/traefik/middlewares /tmp/TestStripPrefixRegex_in_stripPrefixRegex_test_gogo -test.v -test.run ^TestStripPrefixRegex$ Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 201 received: 200 Messages: /b/api2/test2: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 405 received: 404 Messages: /a/test: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 201 received: 200 Messages: /b/api/: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 201 received: 200 Messages: /c/api/123/test3: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 201 received: 200 Messages: /c/api/123/: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 201 received: 200 Messages: /a/api/test: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 405 received: 404 Messages: /c/api/abc/test4: unexpected status code. Error Trace: stripPrefixRegex_test.go:83 Error: Not equal: expected: 201 received: 200 Messages: /b/api/test1: unexpected status code.

It's a side effect when I run unit tests in Gogland... 😞

Hmm, I'd rather not pollute our tests with duplicate identifiers due to one Go IDE being buggy. On all of Visual Studio Code, Atom, and vim-go, this is not a problem. Might it be acceptable to switch to the terminal and run go test manually for cases where tests fail on Gogland until this gets fixed?

You pasted a go test text output. Looking at this screenshot from a Gogland issue, it looks like the IDE does show the sub-test name on the left pane?

Yes, you can see sub-test names but not the log related to only one sub-test.
You can show all log (without the tests "headers") or nothing 😞

Run tests manually it's not acceptable for me, I need a tool which run continuously my tests after each code changes.

I agree to "not pollute" our tests due to one IDE but it will be hard to me to analyze failed tests. 😞 😞 😞

AFAICS, you can still see which tests failed but cannot see the details of individual tests.

I think there are a few alternatives:

Remove all but the failing tests if you want to see the error details.

Switch to the terminal for failing tests.

Switch to a different IDE/editor, either for the failing tests or permanently.

Visual Studio Code (my IDE of choice) is pretty good, though not perfect. There are a few things which cannot be done as nicely as with the Go CLI toolchain; yet, I refrain from letting those shortcomings leak into the code base.

I'll approve the PR since this is my only concern left and I don't want to block it from getting merged any longer (assuming that @emilevauge is fine with it too). Nevertheless, I'm still very much against this change, and even more when it starts to spread throughout the code base over time.

(Side note: Could you please consider filing a bug report on the Gogland issue tacker? IMHO, JetBrains should fix this problem ASAP.)

timoreimann · 2017-06-06T19:18:13Z

testhelpers/helpers.go

@@ -19,3 +20,12 @@ func MustNewRequest(method, urlStr string, body io.Reader) *http.Request {
 	}
 	return request
 }
+
+// MustParseURL parse an URL or panics if it can't


nit-picks:

parse -> parses

an URL -> a URL (the U is not a vowel sound)

refactor: typo in whitelister file name.

ldez added area/rules kind/enhancement a new or improved feature. status/2-needs-review labels Jun 1, 2017

ldez added this to the 1.4 milestone Jun 1, 2017

ldez force-pushed the refactor/simplify-stripprefix-tests branch 4 times, most recently from 4a1b052 to 6022700 Compare June 1, 2017 21:22

timoreimann self-requested a review June 3, 2017 06:25

timoreimann reviewed Jun 3, 2017

View reviewed changes

ldez force-pushed the refactor/simplify-stripprefix-tests branch from 2a7ea11 to e8c49bf Compare June 3, 2017 09:30

ldez force-pushed the refactor/simplify-stripprefix-tests branch 2 times, most recently from 85516fb to eb8a978 Compare June 3, 2017 10:07

ldez force-pushed the refactor/simplify-stripprefix-tests branch 8 times, most recently from 04475eb to 0c86fd9 Compare June 3, 2017 13:17

emilevauge approved these changes Jun 6, 2017

View reviewed changes

timoreimann suggested changes Jun 6, 2017

View reviewed changes

ldez force-pushed the refactor/simplify-stripprefix-tests branch 5 times, most recently from 966abf2 to 2cc5e4c Compare June 7, 2017 14:21

ldez added 6 commits June 7, 2017 20:25

test: simplify stripPrefix* tests.

86c3ce5

refactor: fix typos.

ae176b5

refactor: typo in whitelister file name.

refactor: ordering imports.

f2d990e

test: use MustNewRequest.

7bfdaac

test: HealthCheck review.

77b28a2

test: add AddPrefix test.

9a97d33

ldez force-pushed the refactor/simplify-stripprefix-tests branch from 2cc5e4c to 9a97d33 Compare June 7, 2017 18:25

refactor: Use Statefull interface in access log.

6e828b3

timoreimann approved these changes Jun 9, 2017

View reviewed changes

ldez merged commit 7399a83 into traefik:master Jun 9, 2017

ldez deleted the refactor/simplify-stripprefix-tests branch June 9, 2017 21:55

ldez mentioned this pull request Jun 12, 2017

Error when requesting an url with double slashes, right after hostname #1323

Closed

ldez removed the status/2-needs-review label Jun 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify stripPrefix and stripPrefixRegex tests #1699

Simplify stripPrefix and stripPrefixRegex tests #1699

ldez commented Jun 1, 2017 •

edited

Loading

timoreimann Jun 3, 2017

timoreimann commented Jun 3, 2017 •

edited

Loading

ldez commented Jun 3, 2017 •

edited

Loading

emilevauge left a comment

timoreimann left a comment

timoreimann Jun 6, 2017 •

edited

Loading

timoreimann Jun 6, 2017

ldez Jun 6, 2017

timoreimann Jun 6, 2017 •

edited

Loading

ldez Jun 7, 2017

timoreimann Jun 6, 2017

timoreimann Jun 6, 2017

ldez Jun 6, 2017

ldez Jun 6, 2017

timoreimann Jun 6, 2017

ldez Jun 6, 2017

timoreimann Jun 9, 2017

timoreimann Jun 6, 2017


		assert.Equal(t, test.expectedStatusCode, resp.StatusCode, "%s: unexpected status code.", test.path)
		assert.Equal(t, test.expectedStatusCode, resp.Code, "%s: unexpected status code.", test.path)

Simplify stripPrefix and stripPrefixRegex tests #1699

Simplify stripPrefix and stripPrefixRegex tests #1699

Conversation

ldez commented Jun 1, 2017 • edited Loading

Description

Choose a reason for hiding this comment

timoreimann commented Jun 3, 2017 • edited Loading

ldez commented Jun 3, 2017 • edited Loading

emilevauge left a comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented Jun 1, 2017 •

edited

Loading

timoreimann commented Jun 3, 2017 •

edited

Loading

ldez commented Jun 3, 2017 •

edited

Loading

timoreimann Jun 6, 2017 •

edited

Loading

timoreimann Jun 6, 2017 •

edited

Loading