[test-improver] Improve tests for proxy handler (#8251)

lpcox · web-flow · commit 1c1ac577c223 · 2026-06-28T20:12:50.000-07:00
# Test Improvements: `handler_test.go` ## File Analyzed - **Test File**: `internal/proxy/handler_test.go` - **Package**: `internal/proxy` - **Lines Added**: +80 lines (3 new tests + 1 helper type) ## Improvements Made ### 1. Increased Coverage Three previously uncovered code paths in `handler.go` are now exercised: | Function | Before | After | |---|---|---| | `handleUnrecognizedPassthrough` | 0% | **100%** | | `forwardAndReadBody` | partial | **100%** | | `handleWithDIFC` | ~86% | **95.1%** | | **Package total** | **91.4%** | **92.2%** | ### 2. New Tests Added #### `TestForwardAndReadBody_BodyReadError` - Covers lines 432–435 of `handler.go`: the `io.ReadAll` error path inside `forwardAndReadBody` - Uses a new `bodyErrorTransport` (custom `http.RoundTripper`) that returns a valid HTTP response whose body is an `io.Pipe` immediately closed with an error - Verifies the handler writes a `502 bad_gateway` / `"failed to read upstream response"` JSON error #### `TestHandleUnrecognizedPassthrough_UpstreamFails` - Covers lines 147–149 of `handler.go`: the `if resp == nil { return }` early exit in `handleUnrecognizedPassthrough` - Points the proxy at `(127.0.0.1/redacted) (unreachable) with an unrecognized path `/v1/unknown/endpoint` - Verifies the handler writes a `502` error and returns without panicking on nil response #### `TestHandleWithDIFC_RateLimitDetected` - Covers lines 230–237 of `handler.go`: the rate-limit span annotation block inside `handleWithDIFC` - Upstream returns `429 Too Many Requests` with `X-Ratelimit-Remaining: 0` and a valid `X-Ratelimit-Reset` Unix timestamp - Exercises both the outer `rateLimited` branch and the inner `!resetAt.IsZero()` branch - Verifies the `429` is passed through and `Retry-After` is injected by `writeResponse` ### 3. Helper Type Added `bodyErrorTransport` — a minimal `http.RoundTripper` that synthesises a response with a failing body reader (via `io.Pipe`). Clean, no network I/O required, no TCP hijacking. ## Test Execution ``` ok github.com/github/gh-aw-mcpg/internal/proxy 0.088s coverage: 92.5% of statements ``` All 160+ tests across all packages pass. ## Why These Changes? `handler.go` had three distinct code paths that were entirely untested: 1. The body-read failure in `forwardAndReadBody` — requires a transport-level mock since `httptest.Server` bodies never fail 2. The nil-response early return in `handleUnrecognizedPassthrough` — requires an unreachable upstream for a non-DIFC path 3. The rate-limit tracing annotations in `handleWithDIFC` — requires a `429` response through the full DIFC pipeline All three paths are now covered, and the tests are self-contained, fast, and deterministic. --- *Generated by Test Improver Workflow* *Focuses on better patterns, increased coverage, and more stable tests* > [!WARNING] > <details> > <summary>Firewall blocked 1 domain</summary> > > The following domain was blocked by the firewall during workflow execution: > > - `index.crates.io` >> To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "index.crates.io" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details> > Generated by [Test Improver](https://github.com/github/gh-aw-mcpg/actions/runs/28340521640) · 2K AIC · ⊞ 29.6K · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-improver%22&type=pullrequests)
diff --git a/internal/proxy/handler_test.go b/internal/proxy/handler_test.go
@@ -5,6 +5,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -20,6 +21,20 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// bodyErrorTransport is an http.RoundTripper that returns a valid response whose
+// body fails when read, exercising the io.ReadAll error path in forwardAndReadBody.
+type bodyErrorTransport struct{}
+
+func (b *bodyErrorTransport) RoundTrip(_ *http.Request) (*http.Response, error) {
+	pr, pw := io.Pipe()
+	pw.CloseWithError(errors.New("simulated body read error"))
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Header:     make(http.Header),
+		Body:       pr,
+	}, nil
+}
+
 // newTestServer creates a minimal proxy Server for unit testing.
 // It uses a NoopGuard so no WASM file is needed, and points
 // upstream at the given URL (use a httptest.Server URL).
@@ -745,3 +760,71 @@ func TestHandleWithDIFC_StrictModeBlocksFilteredItems(t *testing.T) {
 	// NoopGuard returns nil LabelResponse, so no items are filtered → 200 OK
 	assert.Equal(t, http.StatusOK, w.Code)
 }
+
+// ─── forwardAndReadBody: body read failure ────────────────────────────────────
+
+// TestForwardAndReadBody_BodyReadError covers the io.ReadAll error path in
+// forwardAndReadBody (lines 431-435 of handler.go). The upstream request
+// succeeds but the response body fails when read.
+func TestForwardAndReadBody_BodyReadError(t *testing.T) {
+	s := newTestServer(t, "http://example.com")
+	s.httpClient = &http.Client{Transport: &bodyErrorTransport{}}
+	h := &proxyHandler{server: s}
+
+	w := httptest.NewRecorder()
+	resp, body := h.forwardAndReadBody(w, context.Background(), http.MethodGet, "/repos/org/repo/issues", nil, "", "")
+
+	assert.Nil(t, resp)
+	assert.Nil(t, body)
+	assertJSONErrorResponse(t, w.Result(), http.StatusBadGateway, "bad_gateway", "failed to read upstream response")
+}
+
+// ─── handleUnrecognizedPassthrough: upstream failure returns early ────────────
+
+// TestHandleUnrecognizedPassthrough_UpstreamFails covers the resp == nil early
+// return in handleUnrecognizedPassthrough (line 147-149 of handler.go). When the
+// upstream is unreachable for an unrecognized path, the handler writes a 502
+// error itself (via forwardAndReadBody) and returns without further processing.
+func TestHandleUnrecognizedPassthrough_UpstreamFails(t *testing.T) {
+	// Use a closed httptest server URL to deterministically force a connection failure.
+	closed := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}))
+	upstreamURL := closed.URL
+	closed.Close()
+	s := newTestServer(t, upstreamURL)
+	h := &proxyHandler{server: s}
+
+	req := httptest.NewRequest(http.MethodGet, "/v1/unknown/endpoint", nil)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+
+	assertJSONErrorResponse(t, w.Result(), http.StatusBadGateway, "bad_gateway", "upstream request failed")
+}
+
+// ─── handleWithDIFC: rate limit detection ────────────────────────────────────
+
+// TestHandleWithDIFC_RateLimitDetected covers the rate-limit tracing code path
+// in handleWithDIFC (lines 230-237 of handler.go). An upstream 429 response with
+// a non-zero X-Ratelimit-Reset header exercises both the outer rateLimited branch
+// and the inner !resetAt.IsZero() branch.
+func TestHandleWithDIFC_RateLimitDetected(t *testing.T) {
+	resetAt := time.Now().Add(60 * time.Second)
+	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.Header().Set("X-Ratelimit-Remaining", "0")
+		w.Header().Set("X-Ratelimit-Reset", fmt.Sprintf("%d", resetAt.Unix()))
+		w.WriteHeader(http.StatusTooManyRequests)
+		w.Write([]byte(`{"message":"API rate limit exceeded"}`)) //nolint:errcheck
+	}))
+	defer upstream.Close()
+
+	s := newTestServer(t, upstream.URL)
+	h := &proxyHandler{server: s}
+
+	req := httptest.NewRequest(http.MethodGet, "/repos/org/repo/issues", nil)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+
+	// 429 response is passed through; writeResponse injects Retry-After
+	assert.Equal(t, http.StatusTooManyRequests, w.Code)
+	assert.NotEmpty(t, w.Header().Get("Retry-After"))
+}
