test: consolidate resp write-error cases, honor lock preconditions

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).
This commit is contained in:
2026-08-20 21:42:40 +01:00
parent aee3908bc4
commit 7621733c88
3 changed files with 57 additions and 24 deletions
+2
View File
@@ -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")
}
+52 -23
View File
@@ -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")
}
+3 -1
View File
@@ -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",