replace nil with stubs

Author: mattdholloway

pkg/github/context_tools_test.go

@@ -96,9 +96,10 @@ func Test_GetMe(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			var deps ToolDependencies
 			if tc.clientErr != "" {
-				deps = stubDeps{clientFn: stubClientFnErr(tc.clientErr)}
+				deps = stubDeps{clientFn: stubClientFnErr(tc.clientErr), obsv: stubExporters()}
 			} else {
-				deps = BaseDeps{Client: github.NewClient(tc.mockedClient)}
+				obs := stubExporters()
+				deps = BaseDeps{Client: github.NewClient(tc.mockedClient), Obsv: obs}
 			}
 			handler := serverTool.Handler(deps)
 
@@ -304,7 +305,7 @@ func Test_GetTeams(t *testing.T) {
 		{
 			name: "getting client fails",
 			makeDeps: func() ToolDependencies {
-				return stubDeps{clientFn: stubClientFnErr("expected test error")}
+				return stubDeps{clientFn: stubClientFnErr("expected test error"), obsv: stubExporters()}
 			},
 			requestArgs:        map[string]any{},
 			expectToolError:    true,
@@ -315,6 +316,7 @@ func Test_GetTeams(t *testing.T) {
 			makeDeps: func() ToolDependencies {
 				return BaseDeps{
 					Client: github.NewClient(httpClientUserFails()),
+					Obsv:   stubExporters(),
 				}
 			},
 			requestArgs:        map[string]any{},
@@ -327,6 +329,7 @@ func Test_GetTeams(t *testing.T) {
 				return stubDeps{
 					clientFn:    stubClientFnFromHTTP(httpClientWithUser()),
 					gqlClientFn: stubGQLClientFnErr("GraphQL client error"),
+					obsv:        stubExporters(),
 				}
 			},
 			requestArgs:        map[string]any{},
@@ -469,7 +472,7 @@ func Test_GetTeamMembers(t *testing.T) {
 		},
 		{
 			name: "getting GraphQL client fails",
-			deps: stubDeps{gqlClientFn: stubGQLClientFnErr("GraphQL client error")},
+			deps: stubDeps{gqlClientFn: stubGQLClientFnErr("GraphQL client error"), obsv: stubExporters()},
 			requestArgs: map[string]any{
 				"org":       "testorg",
 				"team_slug": "testteam",

pkg/github/dependencies.go

@@ -188,17 +188,11 @@ func (d BaseDeps) GetContentWindowSize() int { return d.ContentWindowSize }
 
 // Logger implements ToolDependencies.
 func (d BaseDeps) Logger(_ context.Context) *slog.Logger {
-	if d.Obsv == nil {
-		return slog.New(slog.DiscardHandler)
-	}
 	return d.Obsv.Logger()
 }
 
 // Metrics implements ToolDependencies.
 func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics {
-	if d.Obsv == nil {
-		return metrics.NewNoopMetrics()
-	}
 	return d.Obsv.Metrics(ctx)
 }
 

pkg/github/dependencies_test.go

@@ -3,13 +3,21 @@ package github_test
 import (
 	"context"
 	"errors"
+	"log/slog"
 	"testing"
 
 	"github.com/github/github-mcp-server/pkg/github"
+	"github.com/github/github-mcp-server/pkg/observability"
+	"github.com/github/github-mcp-server/pkg/observability/metrics"
 	"github.com/github/github-mcp-server/pkg/translations"
 	"github.com/stretchr/testify/assert"
 )
 
+func testExporters() observability.Exporters {
+	obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics())
+	return obs
+}
+
 func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
 	t.Parallel()
 
@@ -28,7 +36,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
 		github.FeatureFlags{},
 		0,       // contentWindowSize
 		checker, // featureChecker
-		nil,     // obsv
+		testExporters(),
 	)
 
 	// Test enabled flag
@@ -53,7 +61,7 @@ func TestIsFeatureEnabled_WithoutChecker(t *testing.T) {
 		github.FeatureFlags{},
 		0,   // contentWindowSize
 		nil, // featureChecker (nil)
-		nil, // obsv
+		testExporters(),
 	)
 
 	// Should return false when checker is nil
@@ -78,7 +86,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) {
 		github.FeatureFlags{},
 		0,       // contentWindowSize
 		checker, // featureChecker
-		nil,     // obsv
+		testExporters(),
 	)
 
 	// Should return false for empty flag name
@@ -103,7 +111,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) {
 		github.FeatureFlags{},
 		0,       // contentWindowSize
 		checker, // featureChecker
-		nil,     // obsv
+		testExporters(),
 	)
 
 	// Should return false and log error (not crash)

pkg/github/dynamic_tools_test.go

@@ -136,7 +136,7 @@ func TestDynamicTools_EnableToolset(t *testing.T) {
 	deps := DynamicToolDependencies{
 		Server:    server,
 		Inventory: reg,
-		ToolDeps:  NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil, nil),
+		ToolDeps:  NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil, stubExporters()),
 		T:         translations.NullTranslationHelper,
 	}
 

pkg/github/feature_flags_test.go

@@ -104,7 +104,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) {
 				FeatureFlags{},
 				0,
 				checker,
-				nil,
+				stubExporters(),
 			)
 
 			// Get the tool and its handler
@@ -167,7 +167,7 @@ func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) {
 				FeatureFlags{InsidersMode: tt.insidersMode},
 				0,
 				nil,
-				nil,
+				stubExporters(),
 			)
 
 			// Get the tool and its handler

pkg/github/server_test.go

@@ -65,19 +65,20 @@ func (s stubDeps) GetFlags(_ context.Context) FeatureFlags           { return s.
 func (s stubDeps) GetContentWindowSize() int                         { return s.contentWindowSize }
 func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false }
 func (s stubDeps) Logger(_ context.Context) *slog.Logger {
-	if s.obsv != nil {
-		return s.obsv.Logger()
-	}
-	return slog.New(slog.DiscardHandler)
+	return s.obsv.Logger()
 }
 func (s stubDeps) Metrics(ctx context.Context) metrics.Metrics {
-	if s.obsv != nil {
-		return s.obsv.Metrics(ctx)
-	}
-	return metrics.NewNoopMetrics()
+	return s.obsv.Metrics(ctx)
 }
 
 // Helper functions to create stub client functions for error testing
+
+// stubExporters returns a discard-logger + noop-metrics Exporters for tests.
+func stubExporters() observability.Exporters {
+	obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics())
+	return obs
+}
+
 func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*gogithub.Client, error) {
 	return func(_ context.Context) (*gogithub.Client, error) {
 		return gogithub.NewClient(httpClient), nil
@@ -141,7 +142,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) {
 		InsidersMode:      false,
 	}
 
-	deps := stubDeps{}
+	deps := stubDeps{obsv: stubExporters()}
 
 	// Build inventory
 	inv, err := NewInventory(cfg.Translator).

pkg/observability/observability.go

@@ -22,14 +22,18 @@ type exporters struct {
 
 // NewExporters creates an Exporters bundle. Pass a configured *slog.Logger
 // (with whatever slog.Handler you need) and a Metrics implementation.
-// The logger must not be nil; use slog.New(slog.DiscardHandler) if logging is unwanted.
-func NewExporters(logger *slog.Logger, metrics metrics.Metrics) (Exporters, error) {
+// Neither may be nil; use slog.New(slog.DiscardHandler) and metrics.NewNoopMetrics()
+// if logging or metrics are unwanted.
+func NewExporters(logger *slog.Logger, m metrics.Metrics) (Exporters, error) {
 	if logger == nil {
 		return nil, errors.New("logger must not be nil: use slog.New(slog.DiscardHandler) to discard logs")
 	}
+	if m == nil {
+		return nil, errors.New("metrics must not be nil: use metrics.NewNoopMetrics() to discard metrics")
+	}
 	return &exporters{
 		logger:  logger,
-		metrics: metrics,
+		metrics: m,
 	}, nil
 }
 

pkg/observability/observability_test.go

@@ -23,16 +23,24 @@ func TestNewExporters(t *testing.T) {
 }
 
 func TestNewExporters_WithNilLogger(t *testing.T) {
-	_, err := NewExporters(nil, nil)
+	_, err := NewExporters(nil, metrics.NewNoopMetrics())
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "logger must not be nil")
 }
 
+func TestNewExporters_WithNilMetrics(t *testing.T) {
+	_, err := NewExporters(slog.New(slog.DiscardHandler), nil)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "metrics must not be nil")
+}
+
 func TestNewExporters_WithDiscardLogger(t *testing.T) {
 	logger := slog.New(slog.DiscardHandler)
-	exp, err := NewExporters(logger, nil)
+	m := metrics.NewNoopMetrics()
+	exp, err := NewExporters(logger, m)
 
 	require.NoError(t, err)
 	assert.NotNil(t, exp)
 	assert.Equal(t, logger, exp.Logger())
+	assert.Equal(t, m, exp.Metrics(context.Background()))
 }