Skip to content

Commit

Permalink
[test] Avoid logging to testing.T from server goroutine (jaegertracin…
Browse files Browse the repository at this point in the history
…g#4546)

## Which problem is this PR solving?
- Resolves jaegertracing#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>
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
  • Loading branch information
yurishkuro authored and kjschnei001 committed Jun 29, 2023
1 parent ad14797 commit e2b944f
Showing 1 changed file with 9 additions and 23 deletions.
32 changes: 9 additions & 23 deletions cmd/collector/app/server/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -156,8 +149,6 @@ func TestSpanCollectorHTTPS(t *testing.T) {
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
},
expectError: false,
expectServerFail: false,
expectClientError: true,
},
{
Expand All @@ -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",
Expand All @@ -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{}),
Expand All @@ -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)
Expand Down Expand Up @@ -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{}),
Expand Down

0 comments on commit e2b944f

Please sign in to comment.