From 787c44f8f76fb2722213c9d28420013cf8b6322d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 24 Apr 2026 12:09:21 +0000 Subject: [PATCH 1/2] Add tests for server.buildCircuitBreakers and server.getCircuitBreaker These two functions in internal/server/unified.go had no direct test coverage despite containing meaningful branching logic: - buildCircuitBreakers: nil config fast-path, empty servers, single server with custom threshold/cooldown, multiple servers, zero values falling back to defaults, independence between server CBs. - getCircuitBreaker: returns pre-configured CB, lazy creation for unknown server, nil map initialisation, caching (same pointer on repeated calls), different servers get different instances, newly created CB starts in CLOSED state. Tests follow the table-driven and t.Parallel() patterns already used in internal/server/circuit_breaker_test.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../server/circuit_breaker_builder_test.go | 204 ++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 internal/server/circuit_breaker_builder_test.go diff --git a/internal/server/circuit_breaker_builder_test.go b/internal/server/circuit_breaker_builder_test.go new file mode 100644 index 00000000..f8cccd5f --- /dev/null +++ b/internal/server/circuit_breaker_builder_test.go @@ -0,0 +1,204 @@ +package server + +import ( + "testing" + "time" + + "github.com/github/gh-aw-mcpg/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestBuildCircuitBreakers verifies that buildCircuitBreakers creates per-server +// circuit breakers with the configuration values provided in the config. +func TestBuildCircuitBreakers(t *testing.T) { + t.Parallel() + + t.Run("nil config returns empty map", func(t *testing.T) { + t.Parallel() + cbs := buildCircuitBreakers(nil) + assert.Empty(t, cbs, "nil config should produce an empty circuit breaker map") + }) + + t.Run("empty servers returns empty map", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{}, + } + cbs := buildCircuitBreakers(cfg) + assert.Empty(t, cbs, "empty servers map should produce no circuit breakers") + }) + + t.Run("single server uses configured threshold and cooldown", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "github": { + RateLimitThreshold: 5, + RateLimitCooldown: 120, + }, + }, + } + cbs := buildCircuitBreakers(cfg) + require.Len(t, cbs, 1) + + cb, ok := cbs["github"] + require.True(t, ok, "circuit breaker for 'github' should exist") + assert.Equal(t, 5, cb.threshold, "threshold should match server config") + assert.Equal(t, 120*time.Second, cb.cooldown, "cooldown should match server config") + }) + + t.Run("multiple servers each get their own circuit breaker", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "github": { + RateLimitThreshold: 3, + RateLimitCooldown: 60, + }, + "slack": { + RateLimitThreshold: 10, + RateLimitCooldown: 30, + }, + }, + } + cbs := buildCircuitBreakers(cfg) + require.Len(t, cbs, 2) + + ghCB, ok := cbs["github"] + require.True(t, ok) + assert.Equal(t, 3, ghCB.threshold) + assert.Equal(t, 60*time.Second, ghCB.cooldown) + + slackCB, ok := cbs["slack"] + require.True(t, ok) + assert.Equal(t, 10, slackCB.threshold) + assert.Equal(t, 30*time.Second, slackCB.cooldown) + }) + + t.Run("zero threshold falls back to default", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "github": { + RateLimitThreshold: 0, + RateLimitCooldown: 0, + }, + }, + } + cbs := buildCircuitBreakers(cfg) + require.Len(t, cbs, 1) + + cb := cbs["github"] + // newCircuitBreaker replaces zero values with defaults. + assert.Equal(t, DefaultRateLimitThreshold, cb.threshold, "zero threshold should use default") + assert.Equal(t, DefaultRateLimitCooldown, cb.cooldown, "zero cooldown should use default") + }) + + t.Run("each circuit breaker starts in CLOSED state", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "github": {RateLimitThreshold: 5, RateLimitCooldown: 60}, + "slack": {RateLimitThreshold: 5, RateLimitCooldown: 60}, + }, + } + cbs := buildCircuitBreakers(cfg) + for serverID, cb := range cbs { + assert.Equal(t, circuitClosed, cb.State(), "circuit breaker for %s should start CLOSED", serverID) + assert.NoError(t, cb.Allow(), "CLOSED circuit breaker for %s should allow requests", serverID) + } + }) + + t.Run("circuit breakers are independent per server", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "github": {RateLimitThreshold: 1, RateLimitCooldown: 60}, + "slack": {RateLimitThreshold: 3, RateLimitCooldown: 60}, + }, + } + cbs := buildCircuitBreakers(cfg) + + // Open the github circuit breaker by hitting the threshold. + cbs["github"].RecordRateLimit(time.Time{}) + assert.Equal(t, circuitOpen, cbs["github"].State(), "github CB should be open after 1 error (threshold=1)") + + // slack's circuit breaker should still be closed (threshold=3, no errors recorded). + assert.Equal(t, circuitClosed, cbs["slack"].State(), "slack CB should remain CLOSED") + assert.NoError(t, cbs["slack"].Allow(), "slack CB should still allow requests") + }) +} + +// TestGetCircuitBreaker verifies the lazy-initialisation behaviour of getCircuitBreaker. +func TestGetCircuitBreaker(t *testing.T) { + t.Parallel() + + t.Run("returns existing circuit breaker when present", func(t *testing.T) { + t.Parallel() + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "github": {RateLimitThreshold: 5, RateLimitCooldown: 30}, + }, + } + us := &UnifiedServer{ + circuitBreakers: buildCircuitBreakers(cfg), + } + + cb := us.getCircuitBreaker("github") + require.NotNil(t, cb) + assert.Equal(t, 5, cb.threshold, "should return the pre-configured circuit breaker") + assert.Equal(t, 30*time.Second, cb.cooldown) + }) + + t.Run("creates default circuit breaker for unknown server", func(t *testing.T) { + t.Parallel() + us := &UnifiedServer{ + circuitBreakers: map[string]*circuitBreaker{}, + } + + cb := us.getCircuitBreaker("unknown") + require.NotNil(t, cb) + assert.Equal(t, DefaultRateLimitThreshold, cb.threshold, "should use default threshold") + assert.Equal(t, DefaultRateLimitCooldown, cb.cooldown, "should use default cooldown") + }) + + t.Run("nil circuitBreakers map is lazily initialised", func(t *testing.T) { + t.Parallel() + // Simulate a server created without the map (e.g. in unit tests that bypass NewUnified). + us := &UnifiedServer{} + assert.Nil(t, us.circuitBreakers, "precondition: circuitBreakers is nil before first call") + + cb := us.getCircuitBreaker("github") + require.NotNil(t, cb) + assert.NotNil(t, us.circuitBreakers, "circuitBreakers map should be initialised after first call") + assert.Equal(t, DefaultRateLimitThreshold, cb.threshold) + }) + + t.Run("cached: second call returns same instance", func(t *testing.T) { + t.Parallel() + us := &UnifiedServer{} + + cb1 := us.getCircuitBreaker("github") + cb2 := us.getCircuitBreaker("github") + assert.Same(t, cb1, cb2, "repeated calls should return the same circuit breaker instance") + }) + + t.Run("different servers return different instances", func(t *testing.T) { + t.Parallel() + us := &UnifiedServer{} + + cbGitHub := us.getCircuitBreaker("github") + cbSlack := us.getCircuitBreaker("slack") + assert.NotSame(t, cbGitHub, cbSlack, "different server IDs should return different instances") + }) + + t.Run("newly created default CB starts CLOSED", func(t *testing.T) { + t.Parallel() + us := &UnifiedServer{} + cb := us.getCircuitBreaker("new-server") + require.NotNil(t, cb) + assert.Equal(t, circuitClosed, cb.State(), "lazily created CB should start CLOSED") + assert.NoError(t, cb.Allow()) + }) +} From ae6a98e7366c4f4fa54b133c043e81bd0eae49b1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 15:22:48 +0000 Subject: [PATCH 2/2] fix: add safe map access and Len assertions in circuit breaker tests Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/f693fb51-ae62-4beb-b1b6-a61ef74b3502 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/server/circuit_breaker_builder_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/server/circuit_breaker_builder_test.go b/internal/server/circuit_breaker_builder_test.go index f8cccd5f..87299122 100644 --- a/internal/server/circuit_breaker_builder_test.go +++ b/internal/server/circuit_breaker_builder_test.go @@ -89,7 +89,8 @@ func TestBuildCircuitBreakers(t *testing.T) { cbs := buildCircuitBreakers(cfg) require.Len(t, cbs, 1) - cb := cbs["github"] + cb, ok := cbs["github"] + require.True(t, ok, "circuit breaker for 'github' should exist") // newCircuitBreaker replaces zero values with defaults. assert.Equal(t, DefaultRateLimitThreshold, cb.threshold, "zero threshold should use default") assert.Equal(t, DefaultRateLimitCooldown, cb.cooldown, "zero cooldown should use default") @@ -104,6 +105,7 @@ func TestBuildCircuitBreakers(t *testing.T) { }, } cbs := buildCircuitBreakers(cfg) + require.Len(t, cbs, 2, "expected circuit breakers for both 'github' and 'slack'") for serverID, cb := range cbs { assert.Equal(t, circuitClosed, cb.State(), "circuit breaker for %s should start CLOSED", serverID) assert.NoError(t, cb.Allow(), "CLOSED circuit breaker for %s should allow requests", serverID) @@ -119,6 +121,9 @@ func TestBuildCircuitBreakers(t *testing.T) { }, } cbs := buildCircuitBreakers(cfg) + require.Len(t, cbs, 2, "expected circuit breakers for both 'github' and 'slack'") + require.Contains(t, cbs, "github", "circuit breaker for 'github' should exist") + require.Contains(t, cbs, "slack", "circuit breaker for 'slack' should exist") // Open the github circuit breaker by hitting the threshold. cbs["github"].RecordRateLimit(time.Time{})