From 3ed09289837c45a89e2bb97c6c270d79f3b88065 Mon Sep 17 00:00:00 2001
From: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Date: Thu, 22 Jun 2023 13:04:52 -0400
Subject: [PATCH] [test] Avoid logging to testing.T from server goroutine
 (#4546)

## Which problem is this PR solving?
- Resolves #4497

## Short description of the changes
- Do not use `zaptest.NewLogger(t)` because it causes race condition
shown in the ticket when the server goroutine tries to log something
that is being forwarded to `testing.T` while the test is being shutdown
due to panic.
- I was not able to get to the root cause why this happens, since the
test is properly shutting down the server. This may indicate an issue in
testing itself in how it handles panic.

Signed-off-by: Yuri Shkuro <github@ysh.us>
---
 cmd/collector/app/server/http_test.go | 32 ++++++++-------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/cmd/collector/app/server/http_test.go b/cmd/collector/app/server/http_test.go
index 6f2742d233e..acfd4a80a11 100644
--- a/cmd/collector/app/server/http_test.go
+++ b/cmd/collector/app/server/http_test.go
@@ -26,7 +26,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
-	"go.uber.org/zap/zaptest"
 
 	"github.com/jaegertracing/jaeger/cmd/collector/app/handler"
 	"github.com/jaegertracing/jaeger/internal/metricstest"
@@ -94,7 +93,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 		clientTLS         tlscfg.Options
 		expectError       bool
 		expectClientError bool
-		expectServerFail  bool
 	}{
 		{
 			name: "should fail with TLS client to untrusted TLS server",
@@ -109,7 +107,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 			},
 			expectError:       true,
 			expectClientError: true,
-			expectServerFail:  false,
 		},
 		{
 			name: "should fail with TLS client to trusted TLS server with incorrect hostname",
@@ -125,7 +122,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 			},
 			expectError:       true,
 			expectClientError: true,
-			expectServerFail:  false,
 		},
 		{
 			name: "should pass with TLS client to trusted TLS server with correct hostname",
@@ -139,9 +135,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 				CAPath:     testCertKeyLocation + "/example-CA-cert.pem",
 				ServerName: "example.com",
 			},
-			expectError:       false,
-			expectClientError: false,
-			expectServerFail:  false,
 		},
 		{
 			name: "should fail with TLS client without cert to trusted TLS server requiring cert",
@@ -156,8 +149,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 				CAPath:     testCertKeyLocation + "/example-CA-cert.pem",
 				ServerName: "example.com",
 			},
-			expectError:       false,
-			expectServerFail:  false,
 			expectClientError: true,
 		},
 		{
@@ -175,9 +166,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 				CertPath:   testCertKeyLocation + "/example-client-cert.pem",
 				KeyPath:    testCertKeyLocation + "/example-client-key.pem",
 			},
-			expectError:       false,
-			expectServerFail:  false,
-			expectClientError: false,
 		},
 		{
 			name: "should fail with TLS client without cert to trusted TLS server requiring cert from a different CA",
@@ -194,15 +182,15 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 				CertPath:   testCertKeyLocation + "/example-client-cert.pem",
 				KeyPath:    testCertKeyLocation + "/example-client-key.pem",
 			},
-			expectError:       false,
-			expectServerFail:  false,
 			expectClientError: true,
 		},
 	}
 
 	for _, test := range testCases {
 		t.Run(test.name, func(t *testing.T) {
-			logger := zaptest.NewLogger(t)
+			// Cannot reliably use zaptest.NewLogger(t) because it causes race condition
+			// See https://github.com/jaegertracing/jaeger/issues/4497.
+			logger := zap.NewNop()
 			params := &HTTPServerParams{
 				HostPort:       fmt.Sprintf(":%d", ports.CollectorHTTP),
 				Handler:        handler.NewJaegerSpanHandler(logger, &mockSpanProcessor{}),
@@ -214,14 +202,12 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 			}
 
 			server, err := StartHTTPServer(params)
-
-			if test.expectServerFail {
-				require.Error(t, err)
-			}
-			defer server.Close()
-
 			require.NoError(t, err)
-			clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop())
+			defer func() {
+				assert.NoError(t, server.Close())
+			}()
+
+			clientTLSCfg, err0 := test.clientTLS.Config(logger)
 			require.NoError(t, err0)
 			dialer := &net.Dialer{Timeout: 2 * time.Second}
 			conn, clientError := tls.DialWithDialer(dialer, "tcp", "localhost:"+fmt.Sprintf("%d", ports.CollectorHTTP), clientTLSCfg)
@@ -260,7 +246,7 @@ func TestSpanCollectorHTTPS(t *testing.T) {
 }
 
 func TestStartHTTPServerParams(t *testing.T) {
-	logger := zaptest.NewLogger(t)
+	logger := zap.NewNop()
 	params := &HTTPServerParams{
 		HostPort:          fmt.Sprintf(":%d", ports.CollectorHTTP),
 		Handler:           handler.NewJaegerSpanHandler(logger, &mockSpanProcessor{}),