Ignore HTTPKeepAlive in leaktest #29
Conversation
|
Thanks for the PR! Pretty fair case to me, can you add a two tests, one that passes without this patch (i.e. using DisableKeepAlive) and another that fails unless this patch is applied (without using DisableKeepAlive) ? Not sure why travis didn't run on this PR at all either |
|
Sure, I’ll pick this up tomorrow. Webhooks where disabled while GH had their storage issues today so guessing the build will run when I push the tests up. |
lawrencegripper
force-pushed
the
lg/ignorehttpkeepalive
branch
from
81ab8df to
8c96954
Compare
|
@fortytw2 I've refactored the tests a bit so I can add those in easily. In order to test nicely (without calling externally) I create a server in the test and call it, this means I've had to remove go 1.7 from the test as its server didn't support the IdleTimeout field - hope that's ok? |
|
@fortytw2 Did you have a chance to look at the PR, happy to fix up any feedback around the additional tests I added. |
|
I totally missed your update to the PR, my bad 🙈 let me get right on it |
leaktest_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // TestSlowTest verifies that the timeout works on slow tests: it should | ||
| // be based on time after the test finishes rather than time after the test's | ||
| // start. | ||
| func TestSlowTest(t *testing.T) { | ||
| defer CheckTimeout(t, 1000 * time.Millisecond)() | ||
| defer CheckTimeout(t, 1000*time.Millisecond)() |
There was a problem hiding this comment.
Yes it looks like gofmt is picking that one up.
gofmt -d leaktest_test.go
diff -u leaktest_test.go.orig leaktest_test.go
--- leaktest_test.go.orig 2018-11-06 16:10:23.301798204 +0000
+++ leaktest_test.go 2018-11-06 16:10:23.305798204 +0000
@@ -138,7 +138,7 @@
// be based on time after the test finishes rather than time after the test's
// start.
func TestSlowTest(t *testing.T) {
- defer CheckTimeout(t, 1000 * time.Millisecond)()
+ defer CheckTimeout(t, 1000*time.Millisecond)()
go time.Sleep(1500 * time.Millisecond)
time.Sleep(750 * time.Millisecond)
I've rolled it back to keep things simple.
leaktest_utils_test.go
Outdated
| router.Handle("/", index()) | ||
|
|
||
| server := &http.Server{ | ||
| Addr: ":8091", |
There was a problem hiding this comment.
Can you use httptest.NewServer here, so we're not forced to free up :8091 on any machine running this?
Also kill the log statements :)
There was a problem hiding this comment.
Oooh cool, I'd not worked with the httptest stuff before that has made this much simpler!
|
@fortytw2 np, hopefully fixed those up. Thanks for the pointers, |
|
@lawrencegripper thanks for the fixes! I'll pull it locally this afternoon and double check, then get it all nice and merged! (need to cut a new tag since we're dropping 1.7 support) |
Fix #28
See: cockroachdb/cockroach@e5e6dde#diff-fdc7f75e12e2453349457c6742ff532f