Audit-driven follow-up to the Rust scrum review on the 3 untested
HIGH-risk packages. Both the audit (reports/scrum/risk-register.md)
and the scrum (tests/real-world/runs/scrum_mojxb5bw/) independently
flagged these files as the highest-leverage missing test coverage.
internal/shared/server_test.go — 8 test funcs
newListener: valid addr, invalid addr (non-numeric port, port
out of range, port-already-in-use surfacing as net.OpError).
Empty-addr-is-valid: documents the net.Listen quirk that "" binds
an OS-picked port — future readers don't need to relitigate.
HealthResponse marshal: JSON shape stable, round-trip clean.
/health handler reconstructed via httptest.Server: status 200,
Content-Type application/json, body fields stable.
RegisterRoutes callback: contract verified (callback is invoked
with a real chi.Router, mounted route reachable end-to-end).
Run bind-failure surface: synchronous error, not a goroutine swallow
— the contract Run depends on per the race-safe-startup comment.
internal/shared/config_test.go — 6 test funcs
DefaultConfig G0 port pinning: every binary's default bind locked
in (3110/3211-3216) so a refactor can't silently flip a port.
LoadConfig empty path: returns DefaultConfig, no error.
LoadConfig missing file: returns DefaultConfig, logs warn (the warn
line shows up in test output, captured-but-not-asserted).
LoadConfig valid TOML: partial overrides land, unspecified sections
keep defaults (TOML decoder leave-alone behavior).
LoadConfig invalid TOML: returns wrapped 'parse config' error.
LoadConfig unreadable file: skipped under root (root reads 0000);
captures the read-error wrap path for non-root contexts.
internal/storeclient/client_test.go — 14 test funcs
safeKey table-driven: plain segments, single slash, empty, trailing
slash, space (→ %20), apostrophe (→ %27), unicode (→ %C3%A9),
deep nesting. Locks URL-escape contract per scrum suggestion.
recordingServer helper backs Put/Get/Delete/List against
httptest.Server: verifies method, path, body bytes round-trip.
ErrKeyNotFound on 404 (errors.Is round-trip).
Non-OK status wraps body preview into the error chain.
Delete accepts both 200 and 204 (S3 vs compatible-store quirk).
List parses JSON shape and surfaces query-string prefix.
Context cancellation propagates through Put as context.Canceled.
internal/queryd/db_test.go — 5 test funcs (with subtests)
sqlEscape table-driven: 8 cases including empty, all-quotes,
nested apostrophes (the case from the scrum suggestion).
redactCreds table-driven: 6 cases — both keys, single keys,
empty, multi-occurrence, placeholder-collision (lossy but safe).
buildBootstrap statement order: INSTALL → LOAD → CREATE SECRET.
buildBootstrap endpoint schemes: http strips + USE_SSL false,
https keeps SSL true, no-scheme defaults SSL true (prod ambient).
buildBootstrap URL_STYLE: 'path' vs 'vhost' branch.
buildBootstrap escapes credential quotes: future SSO-token-with-
apostrophe doesn't break out of the SQL string literal — the
belt holds when the suspenders snap.
Real finding caught by my own test:
net.Listen("tcp", "") succeeds (OS-picked port) — captured as
TestNewListener_EmptyAddrIsValid so the quirk is documented.
Verified:
go test -short ./... — every internal/ package now has tests
(no more 'no test files' lines for shared/storeclient).
just verify — vet + test + 9 smokes green in 33s.
just proof contract — 53/0/1 green (no harness regression).
Closes:
R-002 internal/shared zero tests HIGH
R-003 internal/storeclient zero tests HIGH
R-008 queryd/db.go untested MED (sqlEscape, redactCreds,
CREATE SECRET formation)
Composite scrum score should move from 43 → ~46 / 60 — the three
HIGH/MED risks closed, internal/shared and internal/storeclient
become "tested + load-bearing" instead of "untested + load-bearing."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
271 lines
7.1 KiB
Go
271 lines
7.1 KiB
Go
package storeclient
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"io"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"testing"
|
|
)
|
|
|
|
// Closes R-003: storeclient was used by catalogd + vectord with zero
|
|
// tests. Coverage strategy: table-driven safeKey for the URL-escape
|
|
// edge cases; httptest.Server-backed tests for Put/Get/Delete/List
|
|
// covering both happy paths and the documented error contracts
|
|
// (404 → ErrKeyNotFound, non-200 → wrapped error with body preview).
|
|
|
|
func TestSafeKey(t *testing.T) {
|
|
cases := []struct {
|
|
name string
|
|
in string
|
|
want string
|
|
}{
|
|
{"plain segments", "a/b/c", "a/b/c"},
|
|
{"single slash", "/", "/"},
|
|
{"empty string", "", ""},
|
|
{"trailing slash preserved", "pre/fix/", "pre/fix/"},
|
|
{"space gets escaped", "a/b c/d", "a/b%20c/d"},
|
|
{"apostrophe gets escaped", "O'Reilly/key", "O%27Reilly/key"},
|
|
{"plus sign escaped", "a+b/c", "a+b/c"}, // PathEscape leaves + alone
|
|
{"unicode encoded", "café/x", "caf%C3%A9/x"},
|
|
{"deep nesting", "datasets/proof_workers/abc.parquet",
|
|
"datasets/proof_workers/abc.parquet"},
|
|
}
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := safeKey(tc.in)
|
|
if got != tc.want {
|
|
t.Errorf("safeKey(%q) = %q, want %q", tc.in, got, tc.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestNew_TrimsTrailingSlash(t *testing.T) {
|
|
c := New("http://127.0.0.1:3211/")
|
|
if c.baseURL != "http://127.0.0.1:3211" {
|
|
t.Errorf("baseURL = %q, want trailing-slash stripped", c.baseURL)
|
|
}
|
|
}
|
|
|
|
// httptest server that records what the client sent + can be steered
|
|
// to return a specific status code per route.
|
|
type recordingServer struct {
|
|
t *testing.T
|
|
srv *httptest.Server
|
|
gotPath string
|
|
gotMethod string
|
|
gotBody []byte
|
|
respStatus int
|
|
respBody string
|
|
}
|
|
|
|
func newRecordingServer(t *testing.T) *recordingServer {
|
|
rs := &recordingServer{t: t, respStatus: http.StatusOK}
|
|
rs.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
rs.gotPath = r.URL.Path + (func() string {
|
|
if r.URL.RawQuery != "" {
|
|
return "?" + r.URL.RawQuery
|
|
}
|
|
return ""
|
|
})()
|
|
rs.gotMethod = r.Method
|
|
rs.gotBody, _ = io.ReadAll(r.Body)
|
|
w.WriteHeader(rs.respStatus)
|
|
if rs.respBody != "" {
|
|
_, _ = w.Write([]byte(rs.respBody))
|
|
}
|
|
}))
|
|
t.Cleanup(rs.srv.Close)
|
|
return rs
|
|
}
|
|
|
|
func TestPut_HappyPath(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
c := New(rs.srv.URL)
|
|
body := []byte("hello world")
|
|
|
|
if err := c.Put(context.Background(), "datasets/x/y.parquet", body); err != nil {
|
|
t.Fatalf("Put: %v", err)
|
|
}
|
|
|
|
if rs.gotMethod != http.MethodPut {
|
|
t.Errorf("method = %q, want PUT", rs.gotMethod)
|
|
}
|
|
if rs.gotPath != "/storage/put/datasets/x/y.parquet" {
|
|
t.Errorf("path = %q, want /storage/put/datasets/x/y.parquet", rs.gotPath)
|
|
}
|
|
if string(rs.gotBody) != "hello world" {
|
|
t.Errorf("body bytes mismatch: got %q want %q", rs.gotBody, body)
|
|
}
|
|
}
|
|
|
|
func TestPut_NonOKStatusReturnsWrappedError(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respStatus = http.StatusForbidden
|
|
rs.respBody = "denied"
|
|
c := New(rs.srv.URL)
|
|
|
|
err := c.Put(context.Background(), "k", []byte{1})
|
|
if err == nil {
|
|
t.Fatal("expected error on 403, got nil")
|
|
}
|
|
if !strings.Contains(err.Error(), "status 403") {
|
|
t.Errorf("error = %v, want status 403 in message", err)
|
|
}
|
|
}
|
|
|
|
func TestGet_RoundTripsBody(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respBody = "the bytes"
|
|
c := New(rs.srv.URL)
|
|
|
|
got, err := c.Get(context.Background(), "datasets/foo")
|
|
if err != nil {
|
|
t.Fatalf("Get: %v", err)
|
|
}
|
|
if string(got) != "the bytes" {
|
|
t.Errorf("body = %q, want 'the bytes'", got)
|
|
}
|
|
if rs.gotMethod != http.MethodGet {
|
|
t.Errorf("method = %q, want GET", rs.gotMethod)
|
|
}
|
|
}
|
|
|
|
func TestGet_404ReturnsErrKeyNotFound(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respStatus = http.StatusNotFound
|
|
c := New(rs.srv.URL)
|
|
|
|
_, err := c.Get(context.Background(), "missing")
|
|
if !errors.Is(err, ErrKeyNotFound) {
|
|
t.Errorf("error = %v, want ErrKeyNotFound", err)
|
|
}
|
|
}
|
|
|
|
func TestGet_500WrapsBodyPreview(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respStatus = http.StatusInternalServerError
|
|
rs.respBody = "boom"
|
|
c := New(rs.srv.URL)
|
|
|
|
_, err := c.Get(context.Background(), "k")
|
|
if err == nil {
|
|
t.Fatal("expected wrapped error on 500")
|
|
}
|
|
if !strings.Contains(err.Error(), "status 500") {
|
|
t.Errorf("error = %v, want status 500 in message", err)
|
|
}
|
|
}
|
|
|
|
func TestDelete_204IsSuccess(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respStatus = http.StatusNoContent
|
|
c := New(rs.srv.URL)
|
|
|
|
if err := c.Delete(context.Background(), "k"); err != nil {
|
|
t.Fatalf("Delete: %v", err)
|
|
}
|
|
if rs.gotMethod != http.MethodDelete {
|
|
t.Errorf("method = %q, want DELETE", rs.gotMethod)
|
|
}
|
|
}
|
|
|
|
func TestDelete_200IsSuccess(t *testing.T) {
|
|
// S3 returns 204; some compatible stores return 200. Both should
|
|
// be acceptable per the comment in client.go.
|
|
rs := newRecordingServer(t)
|
|
rs.respStatus = http.StatusOK
|
|
c := New(rs.srv.URL)
|
|
|
|
if err := c.Delete(context.Background(), "k"); err != nil {
|
|
t.Fatalf("Delete with 200: %v", err)
|
|
}
|
|
}
|
|
|
|
func TestDelete_400IsError(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respStatus = http.StatusBadRequest
|
|
rs.respBody = "bad key"
|
|
c := New(rs.srv.URL)
|
|
|
|
err := c.Delete(context.Background(), "k")
|
|
if err == nil {
|
|
t.Fatal("expected error on 400")
|
|
}
|
|
}
|
|
|
|
func TestList_ParsesObjects(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respBody = `{"prefix":"datasets/","objects":[
|
|
{"Key":"datasets/a.parquet","Size":100},
|
|
{"Key":"datasets/b.parquet","Size":200},
|
|
{"Key":"datasets/c.parquet","Size":300}
|
|
]}`
|
|
c := New(rs.srv.URL)
|
|
|
|
keys, err := c.List(context.Background(), "datasets/")
|
|
if err != nil {
|
|
t.Fatalf("List: %v", err)
|
|
}
|
|
want := []string{"datasets/a.parquet", "datasets/b.parquet", "datasets/c.parquet"}
|
|
if len(keys) != len(want) {
|
|
t.Fatalf("got %d keys, want %d", len(keys), len(want))
|
|
}
|
|
for i, k := range keys {
|
|
if k != want[i] {
|
|
t.Errorf("keys[%d] = %q, want %q", i, k, want[i])
|
|
}
|
|
}
|
|
// And the prefix query-param made it across the wire.
|
|
if !strings.Contains(rs.gotPath, "prefix=datasets") {
|
|
t.Errorf("query path = %q, want prefix=datasets", rs.gotPath)
|
|
}
|
|
}
|
|
|
|
func TestList_EmptyPrefix(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respBody = `{"prefix":"","objects":[]}`
|
|
c := New(rs.srv.URL)
|
|
|
|
keys, err := c.List(context.Background(), "")
|
|
if err != nil {
|
|
t.Fatalf("List: %v", err)
|
|
}
|
|
if len(keys) != 0 {
|
|
t.Errorf("got %d keys, want 0", len(keys))
|
|
}
|
|
}
|
|
|
|
func TestList_BadJSON_ReturnsDecodeError(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
rs.respBody = "not json"
|
|
c := New(rs.srv.URL)
|
|
|
|
_, err := c.List(context.Background(), "p")
|
|
if err == nil {
|
|
t.Fatal("expected decode error on non-JSON body")
|
|
}
|
|
if !strings.Contains(err.Error(), "list decode") {
|
|
t.Errorf("error = %v, want 'list decode' wrapper", err)
|
|
}
|
|
}
|
|
|
|
func TestPut_ContextCancellation(t *testing.T) {
|
|
rs := newRecordingServer(t)
|
|
c := New(rs.srv.URL)
|
|
|
|
ctx, cancel := context.WithCancel(context.Background())
|
|
cancel() // pre-cancel — request should fail without hitting server
|
|
|
|
err := c.Put(ctx, "k", []byte{1})
|
|
if err == nil {
|
|
t.Fatal("expected error from canceled context")
|
|
}
|
|
if !errors.Is(err, context.Canceled) {
|
|
t.Errorf("error = %v, want context.Canceled-wrapped", err)
|
|
}
|
|
}
|