From 7621733c88c68084af2aaea9c1f2668cf732ee4c Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 20 Aug 2026 21:42:40 +0100 Subject: [PATCH] test: consolidate resp write-error cases, honor lock preconditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes (Task 8, second pass): - Fold TestRESPClient_AttemptPipelineLocked_WriteError and TestRESPClient_HandshakeLocked_WriteError into one table, TestRESPClient_WriteError_BrokenPipe — both exercised the identical broken-pipe shape on a pre-set fakeConn. - Take c.mu.Lock()/Unlock() around every direct call to an internal "*Locked" method (ensureConnLocked, attemptPipelineLocked x2, handshakeLocked) per each method's own "callers must hold c.mu" precondition, even though every such test is single-goroutine. - TestUnifiedCostMicros case 2: replace a want that called costMicros (the function under indirect test) with the literal 11_250_000, hand- derived from builtinPricing["gpt-5"] (InputPerM 1.25, OutputPerM 10) at 1M prompt + 1M completion tokens. - TestRedisStore_Get_DownServer_ReturnsError: assert err.Error() contains "redisStore: get", not just that an error occurred. Report corrected: the earlier claim that all new tests were table-driven was inaccurate (actual split 8/16 at review time); documented as a deliberate choice, not an oversight — six single-scenario tests stay flat since a one-row table adds structure without adding coverage. Gate: go test ./... -count=1 (pass), -race (pass), gofmt (clean), vet (clean). --- redis_store_test.go | 2 ++ resp_test.go | 75 +++++++++++++++++++++++++++++------------- routes_unified_test.go | 4 ++- 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/redis_store_test.go b/redis_store_test.go index 97a59f5..a71097d 100644 --- a/redis_store_test.go +++ b/redis_store_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -181,4 +182,5 @@ func TestRedisStore_Get_DownServer_ReturnsError(t *testing.T) { store := newRedisStore(newRESPClient(deadAddr, "", 0)) _, err := store.get("k") require.Error(t, err) + assert.Contains(t, err.Error(), "redisStore: get") } diff --git a/resp_test.go b/resp_test.go index 55c3165..07de6cd 100644 --- a/resp_test.go +++ b/resp_test.go @@ -721,36 +721,61 @@ func TestRESPClient_EnsureConnLocked_Errors(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - err := c.client().ensureConnLocked(c.deadline) + client := c.client() + // ensureConnLocked documents "callers must hold c.mu" — honor + // that precondition even though this test is single-goroutine. + client.mu.Lock() + err := client.ensureConnLocked(c.deadline) + client.mu.Unlock() require.Error(t, err) assert.Contains(t, err.Error(), c.wantErrContains) }) } } -// TestRESPClient_AttemptPipelineLocked_WriteError covers the command-write -// failure attemptPipelineLocked returns when an already-connected socket -// refuses a write (a broken pipe) — distinct from a dial failure, which -// never reaches this code path. -func TestRESPClient_AttemptPipelineLocked_WriteError(t *testing.T) { - c := &respClient{ - conn: fakeConn{writeErr: errors.New("stub: broken pipe")}, - r: bufio.NewReader(strings.NewReader("")), +// TestRESPClient_WriteError_BrokenPipe covers the identical broken-pipe +// shape shared by attemptPipelineLocked's command write and +// handshakeLocked's own write: both fail deterministically on an +// already-connected socket that refuses a write — distinct from a dial +// failure, which never reaches either code path. +func TestRESPClient_WriteError_BrokenPipe(t *testing.T) { + cases := []struct { + run func(c *respClient) error + name string + wantErrContains string + }{ + { + name: "attemptPipelineLocked: command write fails", + run: func(c *respClient) error { + _, err := c.attemptPipelineLocked([][]string{{"GET", "k"}}, time.Now().Add(time.Second)) + return err + }, + wantErrContains: "write", + }, + { + name: "handshakeLocked: AUTH write fails", + run: func(c *respClient) error { + return c.handshakeLocked("AUTH", "pw") + }, + wantErrContains: "AUTH", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + client := &respClient{ + conn: fakeConn{writeErr: errors.New("stub: broken pipe")}, + r: bufio.NewReader(strings.NewReader("")), + } + // attemptPipelineLocked/handshakeLocked document "callers must + // hold c.mu" — honor that precondition even though this test + // is single-goroutine. + client.mu.Lock() + err := c.run(client) + client.mu.Unlock() + require.Error(t, err) + assert.Contains(t, err.Error(), c.wantErrContains) + }) } - _, err := c.attemptPipelineLocked([][]string{{"GET", "k"}}, time.Now().Add(time.Second)) - require.Error(t, err) - assert.Contains(t, err.Error(), "write") -} - -// TestRESPClient_HandshakeLocked_WriteError covers handshakeLocked's own -// write failure, the same broken-pipe shape as -// TestRESPClient_AttemptPipelineLocked_WriteError but for the AUTH/SELECT -// handshake command specifically. -func TestRESPClient_HandshakeLocked_WriteError(t *testing.T) { - c := &respClient{conn: fakeConn{writeErr: errors.New("stub: broken pipe")}} - err := c.handshakeLocked("AUTH", "pw") - require.Error(t, err) - assert.Contains(t, err.Error(), "AUTH") } // TestRESPClient_AuthHandshakeFailure_ErrorReplySurfaces covers @@ -768,7 +793,11 @@ func TestRESPClient_AuthHandshakeFailure_ErrorReplySurfaces(t *testing.T) { }) c := newRESPClient(ln.Addr().String(), "wrong-pw", 0) + // attemptPipelineLocked documents "callers must hold c.mu" — honor that + // precondition even though this test is single-goroutine. + c.mu.Lock() _, err := c.attemptPipelineLocked([][]string{{"GET", "k"}}, time.Now().Add(2*time.Second)) + c.mu.Unlock() require.Error(t, err) assert.Contains(t, err.Error(), "AUTH failed") } diff --git a/routes_unified_test.go b/routes_unified_test.go index bf5c0f4..ff020f1 100644 --- a/routes_unified_test.go +++ b/routes_unified_test.go @@ -1640,7 +1640,9 @@ func TestUnifiedCostMicros(t *testing.T) { canonical: "unknown-provider/gpt-5", bare: "gpt-5", overrides: nil, - want: costMicros("gpt-5", u, nil), + // builtinPricing["gpt-5"] = {InputPerM: 1.25, OutputPerM: 10}: + // (1M prompt * $1.25/M) + (1M completion * $10/M), in micros. + want: 11_250_000, }, { name: "neither id priced returns zero",