[test] Add tests for server.buildCircuitBreakers and server.getCircuitBreaker

[github-actions]

Test Coverage Improvement: buildCircuitBreakers and getCircuitBreaker

Function Analyzed

• Package: internal/server
• Functions: buildCircuitBreakers, getCircuitBreaker
• File: internal/server/unified.go
• Previous Coverage: 0% (no direct tests existed)
• Complexity: Medium — multiple branching paths, lazy initialisation, caching behaviour

Why These Functions?

buildCircuitBreakers and getCircuitBreaker contained meaningful branching logic — nil-config fast-path, zero-value fallbacks to defaults, map lazy initialisation, caching semantics — yet had zero direct test coverage. They were identified by scanning all source files against existing *_test.go files and confirmed by grepping test files for their names.

Tests Added

New file: internal/server/circuit_breaker_builder_test.go

TestBuildCircuitBreakers (6 sub-tests)

• ✅ nil config returns empty map — fast-path guard
• ✅ empty servers returns empty map — zero-length iteration
• ✅ single server uses configured threshold and cooldown — happy path with custom values
• ✅ multiple servers each get their own circuit breaker — N-server map, each keyed correctly
• ✅ zero threshold falls back to default — covers the newCircuitBreaker default-fill branch
• ✅ each circuit breaker starts in CLOSED state — functional state verification
• ✅ circuit breakers are independent per server — tripping one CB does not affect another

TestGetCircuitBreaker (6 sub-tests)

• ✅ returns existing circuit breaker when present — pre-configured CB returned unchanged
• ✅ creates default circuit breaker for unknown server — lazy creation path
• ✅ nil circuitBreakers map is lazily initialised — covers us.circuitBreakers == nil branch
• ✅ cached: second call returns same instance — pointer identity check
• ✅ different servers return different instances — map isolation
• ✅ newly created default CB starts CLOSED — functional state verification

Test Execution

Tests are structured following existing project patterns (t.Parallel(), assert/require from testify, table-driven sub-tests). CI will run full verification.

---

Generated by Test Coverage Improver
Next run will target the next most complex under-tested function

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by Test Coverage Improver · ● 15.5M · ◷

[Copilot]

Pull request overview

Adds direct unit tests in internal/server to cover the previously untested buildCircuitBreakers and (*UnifiedServer).getCircuitBreaker logic (defaults, per-server config mapping, and lazy initialization/caching).

Changes:

• Introduces new tests for buildCircuitBreakers across nil/empty configs, multi-server configs, and default fallback behavior.
• Adds tests for getCircuitBreaker covering existing entries, lazy map initialization, default creation, and caching semantics.

Show a summary per file

File	Description
internal/server/circuit_breaker_builder_test.go	Adds unit tests for circuit breaker construction from config and lazy retrieval/creation/caching on UnifiedServer.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

internal/server/circuit_breaker_builder_test.go:129

• This subtest indexes cbs["github"] / cbs["slack"] and calls methods on the result without asserting the keys exist. If buildCircuitBreakers regresses and omits one entry, the test will panic instead of producing an actionable assertion failure. Add require.Contains/require.NotNil checks (and ideally require.Len(t, cbs, 2)) before dereferencing.

		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")

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Copilot]

The loop only asserts properties for entries returned by ranging over cbs. If buildCircuitBreakers mistakenly returns an empty map, this subtest would pass without running any assertions. Add a require.Len(t, cbs, 2) (or at least require.NotEmpty) before the loop to ensure the test actually exercises the expected circuit breakers.

[Copilot]

cb := cbs["github"] can be nil if the map doesn't contain the expected key, which would cause a panic when accessing cb.threshold/cb.cooldown. Prefer require.Contains(t, cbs, "github") (or require.NotNil(t, cb)) before dereferencing so failures are reported cleanly.

This issue also appears on line 121 of the same file.

[lpcox]

@copilot address this review feedback #4466 (review)

[Copilot]

@copilot address this review feedback #4466 (review)

Addressed in ae6a98e:

1. zero threshold falls back to default (line 92): replaced cb := cbs["github"] with the comma-ok pattern + require.True(t, ok, ...) so a missing key reports a clean failure instead of a nil-dereference panic. Same pattern already applied on line 121.
2. each circuit breaker starts in CLOSED state (line 106): added require.Len(t, cbs, 2, ...) before the range loop so the test fails explicitly if the map is empty rather than silently passing with no assertions executed.
3. circuit breakers are independent per server (line 121): added require.Len(t, cbs, 2) + require.Contains for both "github" and "slack" keys before the direct map accesses.