net/http: remove arbitrary timeouts in tests of Server.ErrorLog

This also allows us to remove the chanWriter helper from the test,
using a simpler strings.Builder instead, relying on
clientServerTest.close for synchronization.
(I don't think this runs afoul of #38370, because the handler
functions themselves in these tests should never be executed,
let alone result in an asynchronous write to the error log.)

Fixes #57599.

Change-Id: I45c6cefca0bb218f6f9a9659de6bde454547f704
Reviewed-on: https://go-review.googlesource.com/c/go/+/539436
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index 0fe555af38f08a3b06101f11f2edb4a96d9e458d..df2a670aee6d8e4efa600550e8401d8d7fe2a1c4 100644 (file)
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -60,13 +60,6 @@ func pedanticReadAll(r io.Reader) (b []byte, err error) {
        }
 }
 
-type chanWriter chan string
-
-func (w chanWriter) Write(p []byte) (n int, err error) {
-       w <- string(p)
-       return len(p), nil
-}
-
 func TestClient(t *testing.T) { run(t, testClient) }
 func testClient(t *testing.T, mode testMode) {
        ts := newClientServerTest(t, mode, robotsTxtHandler).ts
@@ -827,12 +820,12 @@ func TestClientInsecureTransport(t *testing.T) {
        run(t, testClientInsecureTransport, []testMode{https1Mode, http2Mode})
 }
 func testClientInsecureTransport(t *testing.T, mode testMode) {
-       ts := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+       cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
                w.Write([]byte("Hello"))
-       })).ts
-       errc := make(chanWriter, 10) // but only expecting 1
-       ts.Config.ErrorLog = log.New(errc, "", 0)
-       defer ts.Close()
+       }))
+       ts := cst.ts
+       errLog := new(strings.Builder)
+       ts.Config.ErrorLog = log.New(errLog, "", 0)
 
        // TODO(bradfitz): add tests for skipping hostname checks too?
        // would require a new cert for testing, and probably
@@ -851,15 +844,10 @@ func testClientInsecureTransport(t *testing.T, mode testMode) {
                }
        }
 
-       select {
-       case v := <-errc:
-               if !strings.Contains(v, "TLS handshake error") {
-                       t.Errorf("expected an error log message containing 'TLS handshake error'; got %q", v)
-               }
-       case <-time.After(5 * time.Second):
-               t.Errorf("timeout waiting for logged error")
+       cst.close()
+       if !strings.Contains(errLog.String(), "TLS handshake error") {
+               t.Errorf("expected an error log message containing 'TLS handshake error'; got %q", errLog)
        }
-
 }
 
 func TestClientErrorWithRequestURI(t *testing.T) {
@@ -897,9 +885,10 @@ func TestClientWithIncorrectTLSServerName(t *testing.T) {
        run(t, testClientWithIncorrectTLSServerName, []testMode{https1Mode, http2Mode})
 }
 func testClientWithIncorrectTLSServerName(t *testing.T, mode testMode) {
-       ts := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {})).ts
-       errc := make(chanWriter, 10) // but only expecting 1
-       ts.Config.ErrorLog = log.New(errc, "", 0)
+       cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {}))
+       ts := cst.ts
+       errLog := new(strings.Builder)
+       ts.Config.ErrorLog = log.New(errLog, "", 0)
 
        c := ts.Client()
        c.Transport.(*Transport).TLSClientConfig.ServerName = "badserver"
@@ -910,13 +899,10 @@ func testClientWithIncorrectTLSServerName(t *testing.T, mode testMode) {
        if !strings.Contains(err.Error(), "127.0.0.1") || !strings.Contains(err.Error(), "badserver") {
                t.Errorf("wanted error mentioning 127.0.0.1 and badserver; got error: %v", err)
        }
-       select {
-       case v := <-errc:
-               if !strings.Contains(v, "TLS handshake error") {
-                       t.Errorf("expected an error log message containing 'TLS handshake error'; got %q", v)
-               }
-       case <-time.After(5 * time.Second):
-               t.Errorf("timeout waiting for logged error")
+
+       cst.close()
+       if !strings.Contains(errLog.String(), "TLS handshake error") {
+               t.Errorf("expected an error log message containing 'TLS handshake error'; got %q", errLog)
        }
 }
 

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 00230020e7a2fef2b4460570cf6b8cf5784c7287..0c76f1bcc4e44d201bc5a7b11f66482255ab2fd2 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -1400,27 +1400,28 @@ func TestTLSHandshakeTimeout(t *testing.T) {
        run(t, testTLSHandshakeTimeout, []testMode{https1Mode, http2Mode})
 }
 func testTLSHandshakeTimeout(t *testing.T, mode testMode) {
-       errc := make(chanWriter, 10) // but only expecting 1
-       ts := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {}),
+       errLog := new(strings.Builder)
+       cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {}),
                func(ts *httptest.Server) {
                        ts.Config.ReadTimeout = 250 * time.Millisecond
-                       ts.Config.ErrorLog = log.New(errc, "", 0)
+                       ts.Config.ErrorLog = log.New(errLog, "", 0)
                },
-       ).ts
+       )
+       ts := cst.ts
+
        conn, err := net.Dial("tcp", ts.Listener.Addr().String())
        if err != nil {
                t.Fatalf("Dial: %v", err)
        }
-       defer conn.Close()
-
        var buf [1]byte
        n, err := conn.Read(buf[:])
        if err == nil || n != 0 {
                t.Errorf("Read = %d, %v; want an error and no bytes", n, err)
        }
+       conn.Close()
 
-       v := <-errc
-       if !strings.Contains(v, "timeout") && !strings.Contains(v, "TLS handshake") {
+       cst.close()
+       if v := errLog.String(); !strings.Contains(v, "timeout") && !strings.Contains(v, "TLS handshake") {
                t.Errorf("expected a TLS handshake timeout error; got %q", v)
        }
 }