net/http: refactor tests to run most in HTTP/1 and HTTP/2 modes

Replace the ad-hoc approach to running tests in HTTP/1 and HTTP/2
modes with a 'run' function that executes a test in various modes.
By default, these modes are HTTP/1 and HTTP/2, but tests can
opt-in to HTTPS/1 as well.

The 'run' function also takes care of post-test cleanup (running the
afterTest function).

The 'run' function runs tests in parallel by default. Tests which
can't run in parallel (generally because they use global test hooks)
pass a testNotParallel option to disable parallelism.

Update clientServerTest to use t.Cleanup to clean up after itself,
rather than leaving this up to tests to handle.

Drop an unnecessary mutex in SetReadLoopBeforeNextReadHook.
Test hooks can't be set in parallel, and we want the race detector
to notify us if two simultaneous tests try to set a hook.

Fixes #56032

Change-Id: I16be64913c426fc93d84abc6ad85dbd3bc191224
Reviewed-on: https://go-review.googlesource.com/c/go/+/438137
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: David Chase <drchase@google.com>

1 files changed, 16 insertions, 44 deletions

diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go
index 27e9eb30ee..686a8699fb 100644
--- a/src/net/http/request_test.go
+++ b/src/net/http/request_test.go
@@ -15,7 +15,6 @@ import (
 	"math"
 	"mime/multipart"
 	. "net/http"
-	"net/http/httptest"
 	"net/url"
 	"os"
 	"reflect"
@@ -289,10 +288,11 @@ Content-Type: text/plain
 // the payload size and the internal leeway buffer size of 10MiB overflows, that we
 // correctly return an error.
 func TestMaxInt64ForMultipartFormMaxMemoryOverflow(t *testing.T) {
-	defer afterTest(t)
-
+	run(t, testMaxInt64ForMultipartFormMaxMemoryOverflow)
+}
+func testMaxInt64ForMultipartFormMaxMemoryOverflow(t *testing.T, mode testMode) {
 	payloadSize := 1 << 10
-	cst := httptest.NewServer(HandlerFunc(func(rw ResponseWriter, req *Request) {
+	cst := newClientServerTest(t, mode, HandlerFunc(func(rw ResponseWriter, req *Request) {
 		// The combination of:
 		//      MaxInt64 + payloadSize + (internal spare of 10MiB)
 		// triggers the overflow. See issue https://golang.org/issue/40430/
@@ -300,8 +300,7 @@ func TestMaxInt64ForMultipartFormMaxMemoryOverflow(t *testing.T) {
 			Error(rw, err.Error(), StatusBadRequest)
 			return
 		}
-	}))
-	defer cst.Close()
+	})).ts
 	fBuf := new(bytes.Buffer)
 	mw := multipart.NewWriter(fBuf)
 	mf, err := mw.CreateFormFile("file", "myfile.txt")
@@ -329,11 +328,9 @@ func TestMaxInt64ForMultipartFormMaxMemoryOverflow(t *testing.T) {
 	}
 }
 
-func TestRedirect_h1(t *testing.T) { testRedirect(t, h1Mode) }
-func TestRedirect_h2(t *testing.T) { testRedirect(t, h2Mode) }
-func testRedirect(t *testing.T, h2 bool) {
-	defer afterTest(t)
-	cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
+func TestRequestRedirect(t *testing.T) { run(t, testRequestRedirect) }
+func testRequestRedirect(t *testing.T, mode testMode) {
+	cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
 		switch r.URL.Path {
 		case "/":
 			w.Header().Set("Location", "/foo/")
@@ -344,7 +341,6 @@ func testRedirect(t *testing.T, h2 bool) {
 			w.WriteHeader(StatusBadRequest)
 		}
 	}))
-	defer cst.close()
 
 	var end = regexp.MustCompile("/foo/$")
 	r, err := cst.c.Get(cst.ts.URL)
@@ -1035,19 +1031,10 @@ func TestRequestCloneTransferEncoding(t *testing.T) {
 	}
 }
 
-func TestNoPanicOnRoundTripWithBasicAuth_h1(t *testing.T) {
-	testNoPanicWithBasicAuth(t, h1Mode)
-}
-
-func TestNoPanicOnRoundTripWithBasicAuth_h2(t *testing.T) {
-	testNoPanicWithBasicAuth(t, h2Mode)
-}
-
 // Issue 34878: verify we don't panic when including basic auth (Go 1.13 regression)
-func testNoPanicWithBasicAuth(t *testing.T, h2 bool) {
-	defer afterTest(t)
-	cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {}))
-	defer cst.close()
+func TestNoPanicOnRoundTripWithBasicAuth(t *testing.T) { run(t, testNoPanicWithBasicAuth) }
+func testNoPanicWithBasicAuth(t *testing.T, mode testMode) {
+	cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {}))
 
 	u, err := url.Parse(cst.ts.URL)
 	if err != nil {
@@ -1328,11 +1315,6 @@ Host: localhost:8080
 `)
 }
 
-const (
-	withTLS = true
-	noTLS   = false
-)
-
 func BenchmarkFileAndServer_1KB(b *testing.B) {
 	benchmarkFileAndServer(b, 1<<10)
 }
@@ -1360,16 +1342,12 @@ func benchmarkFileAndServer(b *testing.B, n int64) {
 		b.Fatalf("Failed to copy %d bytes: %v", n, err)
 	}
 
-	b.Run("NoTLS", func(b *testing.B) {
-		runFileAndServerBenchmarks(b, noTLS, f, n)
-	})
-
-	b.Run("TLS", func(b *testing.B) {
-		runFileAndServerBenchmarks(b, withTLS, f, n)
-	})
+	run(b, func(b *testing.B, mode testMode) {
+		runFileAndServerBenchmarks(b, mode, f, n)
+	}, []testMode{http1Mode, https1Mode, http2Mode})
 }
 
-func runFileAndServerBenchmarks(b *testing.B, tlsOption bool, f *os.File, n int64) {
+func runFileAndServerBenchmarks(b *testing.B, mode testMode, f *os.File, n int64) {
 	handler := HandlerFunc(func(rw ResponseWriter, req *Request) {
 		defer req.Body.Close()
 		nc, err := io.Copy(io.Discard, req.Body)
@@ -1382,14 +1360,8 @@ func runFileAndServerBenchmarks(b *testing.B, tlsOption bool, f *os.File, n int6
 		}
 	})
 
-	var cst *httptest.Server
-	if tlsOption == withTLS {
-		cst = httptest.NewTLSServer(handler)
-	} else {
-		cst = httptest.NewServer(handler)
-	}
+	cst := newClientServerTest(b, mode, handler).ts
 
-	defer cst.Close()
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		// Perform some setup.