Remove coupling on Goa middleware

goadesign · Jan 7, 2024 · cbd111b · cbd111b
1 parent 7a4d79c
commit cbd111b
Show file tree

Hide file tree

Showing 10 changed files with 179 additions and 207 deletions.
diff --git a/clue/log.go b/clue/log.go
diff --git a/clue/log_test.go b/clue/log_test.go
diff --git a/clue/options_test.go b/clue/options_test.go
@@ -5,73 +5,61 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"go.opentelemetry.io/otel/propagation"
 	"goa.design/clue/log"
 )
 
 func TestOptions(t *testing.T) {
 	ctx := log.Context(context.Background())
 	cases := []struct {
-		name    string
-		options []Option
-		want    *options
+		name   string
+		option Option
+		want   func(*options) // mutate default options
 	}{
 		{
 			name: "default",
-			want: defaultOptions(ctx),
 		},
 		{
-			name:    "with reader interval",
-			options: []Option{WithReaderInterval(10)},
-			want: func() *options {
-				o := defaultOptions(ctx)
-				o.readerInterval = 10
-				return o
-			}(),
+			name:   "with reader interval",
+			option: WithReaderInterval(1000),
+			want:   func(o *options) { o.readerInterval = 1000 },
 		},
 		{
-			name:    "with max sampling rate",
-			options: []Option{WithMaxSamplingRate(10)},
-			want: func() *options {
-				o := defaultOptions(ctx)
-				o.maxSamplingRate = 10
-				return o
-			}(),
+			name:   "with max sampling rate",
+			option: WithMaxSamplingRate(1000),
+			want:   func(o *options) { o.maxSamplingRate = 1000 },
 		},
 		{
-			name:    "with sample size",
-			options: []Option{WithSampleSize(10)},
-			want: func() *options {
-				o := defaultOptions(ctx)
-				o.sampleSize = 10
-				return o
-			}(),
+			name:   "with sample size",
+			option: WithSampleSize(1000),
+			want:   func(o *options) { o.sampleSize = 1000 },
 		},
 		{
-			name:    "with propagator",
-			options: []Option{WithPropagators(nil)},
-			want: func() *options {
-				o := defaultOptions(ctx)
-				o.propagators = nil
-				return o
-			}(),
+			name:   "with propagator",
+			option: WithPropagators(propagation.TraceContext{}),
+			want:   func(o *options) { o.propagators = propagation.TraceContext{} },
 		},
 		{
-			name:    "with error handler",
-			options: []Option{WithErrorHandler(dummyErrorHandler{})},
-			want: func() *options {
-				o := defaultOptions(ctx)
-				o.errorHandler = dummyErrorHandler{}
-				return o
-			}(),
+			name:   "with error handler",
+			option: WithErrorHandler(dummyErrorHandler{}),
+			want:   func(o *options) { o.errorHandler = dummyErrorHandler{} },
 		},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
 			got := defaultOptions(ctx)
-			for _, o := range c.options {
-				o(got)
+			if c.option != nil {
+				c.option(got)
 			}
-			assert.Equal(t, c.want, got)
+			want := defaultOptions(ctx)
+			if c.want != nil {
+				c.want(want)
+			}
+			assert.Equal(t, want.maxSamplingRate, got.maxSamplingRate)
+			assert.Equal(t, want.sampleSize, got.sampleSize)
+			assert.Equal(t, want.readerInterval, got.readerInterval)
+			assert.Equal(t, want.propagators, got.propagators)
+			assert.IsType(t, want.errorHandler, got.errorHandler)
 		})
 	}
 }
diff --git a/debug/grpc_test.go b/debug/grpc_test.go
@@ -7,6 +7,7 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"google.golang.org/grpc"
 
 	"goa.design/clue/internal/testsvc"
@@ -18,7 +19,7 @@ func TestUnaryServerInterceptor(t *testing.T) {
 	ctx := log.Context(context.Background(), log.WithOutput(&buf), log.WithFormat(logKeyValsOnly))
 	cli, stop := testsvc.SetupGRPC(t,
 		testsvc.WithServerOptions(grpc.ChainUnaryInterceptor(
-			log.UnaryServerInterceptor(ctx, log.WithDisableCallLogging()),
+			log.UnaryServerInterceptor(ctx, log.WithDisableCallLogging(), log.WithDisableCallID()),
 			UnaryServerInterceptor())),
 		testsvc.WithUnaryFunc(logUnaryMethod))
 	defer stop()
@@ -28,10 +29,10 @@ func TestUnaryServerInterceptor(t *testing.T) {
 	defer ts.Close()
 
 	steps := []struct {
-		name         string
-		on           bool
-		off          bool
-		expectedLogs string
+		name    string
+		on      bool
+		off     bool
+		wantLog string
 	}{
 		{"start", false, false, ""},
 		{"turn debug logs on", true, false, "debug=message "},
@@ -47,12 +48,8 @@ func TestUnaryServerInterceptor(t *testing.T) {
 			makeRequest(t, ts.URL+"/debug?debug-logs=off")
 		}
 		_, err := cli.GRPCMethod(context.Background(), nil)
-		if err != nil {
-			t.Errorf("unexpected error: %v", err)
-		}
-		if buf.String() != step.expectedLogs {
-			t.Errorf("expected log %q, got %q", step.expectedLogs, buf.String())
-		}
+		assert.NoError(t, err)
+		assert.Equal(t, step.wantLog, buf.String())
 		buf.Reset()
 	}
 }
@@ -62,14 +59,14 @@ func TestStreamServerInterceptor(t *testing.T) {
 	ctx := log.Context(context.Background(), log.WithOutput(&buf), log.WithFormat(logKeyValsOnly))
 	cli, stop := testsvc.SetupGRPC(t,
 		testsvc.WithServerOptions(grpc.ChainStreamInterceptor(
-			log.StreamServerInterceptor(ctx, log.WithDisableCallLogging()),
+			log.StreamServerInterceptor(ctx, log.WithDisableCallLogging(), log.WithDisableCallID()),
 			StreamServerInterceptor())),
 		testsvc.WithStreamFunc(echoMethod))
 	defer stop()
 	steps := []struct {
 		name            string
 		enableDebugLogs bool
-		expectedLogs    string
+		wantLog         string
 	}{
 		{"no debug logs", false, ""},
 		{"debug logs", true, "debug=message "},
@@ -78,19 +75,13 @@ func TestStreamServerInterceptor(t *testing.T) {
 	for _, step := range steps {
 		debugLogs = step.enableDebugLogs
 		stream, err := cli.GRPCStream(context.Background())
-		if err != nil {
-			t.Errorf("%s: unexpected error: %v", step.name, err)
-		}
+		assert.NoError(t, err)
 		defer stream.Close()
-		if err = stream.Send(&testsvc.Fields{}); err != nil {
-			t.Errorf("%s: unexpected send error: %v", step.name, err)
-		}
-		if _, err = stream.Recv(); err != nil {
-			t.Errorf("%s: unexpected recv error: %v", step.name, err)
-		}
-		if buf.String() != step.expectedLogs {
-			t.Errorf("%s: unexpected log %q", step.name, buf.String())
-		}
+		err = stream.Send(&testsvc.Fields{})
+		assert.NoError(t, err)
+		_, err = stream.Recv()
+		assert.NoError(t, err)
+		assert.Equal(t, step.wantLog, buf.String())
 		buf.Reset()
 	}
 }

diff --git a/debug/http_test.go b/debug/http_test.go
@@ -7,13 +7,17 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"goa.design/clue/log"
 )
 
 func TestHTTP(t *testing.T) {
 	// Create log context
 	var buf bytes.Buffer
-	ctx := log.Context(context.Background(), log.WithOutput(&buf), log.WithFormat(logKeyValsOnly))
+	ctx := log.Context(context.Background(),
+		log.WithOutput(&buf),
+		log.WithFormat(logKeyValsOnly))
 	log.FlushAndDisableBuffering(ctx)
 
 	// Create HTTP handler
@@ -32,25 +36,26 @@ func TestHTTP(t *testing.T) {
 	// Mount debug handler and log middleware
 	MountDebugLogEnabler(mux)
 	handler = HTTP()(handler)
-	handler = log.HTTP(ctx, log.WithDisableRequestLogging())(handler)
+	handler = log.HTTP(ctx,
+		log.WithDisableRequestLogging(),
+		log.WithDisableRequestID())(handler)
 
 	// Start test server
 	mux.Handle("/", handler)
 	ts := httptest.NewServer(mux)
 	defer ts.Close()
 
 	steps := []struct {
-		name         string
-		on           bool
-		off          bool
-		expectedResp string
-		expectedLogs string
+		name    string
+		on      bool
+		off     bool
+		wantLog string
 	}{
-		{"start", false, false, "", "test=info "},
-		{"turn debug logs on", true, false, `{"debug-logs":true}`, "test=info test=debug "},
-		{"with debug logs on", false, false, `{"debug-logs":true}`, "test=info test=debug "},
-		{"turn debug logs off", false, true, `{"debug-logs":false}`, "test=info "},
-		{"with debug logs off", false, false, `{"debug-logs":false}`, "test=info "},
+		{"start", false, false, "test=info "},
+		{"turn debug logs on", true, false, "test=info test=debug "},
+		{"with debug logs on", false, false, "test=info test=debug "},
+		{"turn debug logs off", false, true, "test=info "},
+		{"with debug logs off", false, false, "test=info "},
 	}
 	for _, step := range steps {
 		if step.on {
@@ -62,15 +67,9 @@ func TestHTTP(t *testing.T) {
 
 		status, resp := makeRequest(t, ts.URL)
 
-		if status != http.StatusOK {
-			t.Errorf("%s: got status %d, expected %d", step.name, status, http.StatusOK)
-		}
-		if resp != "OK" {
-			t.Errorf("%s: got body %q, expected %q", step.name, resp, "OK")
-		}
-		if buf.String() != step.expectedLogs {
-			t.Errorf("%s: got logs %q, expected %q", step.name, buf.String(), step.expectedLogs)
-		}
+		assert.Equal(t, http.StatusOK, status)
+		assert.Equal(t, "OK", resp)
+		assert.Equal(t, step.wantLog, buf.String())
 		buf.Reset()
 	}
 }