proxy: Refactor tests (#660)
- use YAML for test configurations - remove most uses of simple-responder, opting to use process.testHandler Fixes #655
This commit is contained in:
Benson Wong
GitHub
@@ -0,0 +1,183 @@
|
||||
# Improve Testability (#655)
|
||||
|
||||
## Current Pain Points
|
||||
|
||||
1. **Tests bypass config loading** - ~80% of tests build `config.Config` structs directly, skipping YAML parsing, env var substitution, macro expansion, and `${PORT}` assignment. Config bugs in those paths go untested.
|
||||
|
||||
2. **simple-responder is everywhere** - Every proxy/routing test launches a real subprocess, waits for health checks (~healthCheckTimeout: 15), and manages process lifecycle just to test HTTP routing. Most of that overhead is wasted.
|
||||
|
||||
3. **Port counter is fragile** - A global `nextTestPort` counter starting at 12000 with a mutex. Parallel tests or leftover processes can collide.
|
||||
|
||||
## Stages
|
||||
|
||||
### Stage 1: YAML-based test config helper
|
||||
|
||||
**Goal:** Tests go through the real `LoadConfigFromReader` path instead of hand-building structs.
|
||||
|
||||
**Effort:** Low | **Impact:** Config bugs caught earlier | **Risk:** None
|
||||
|
||||
Create a test helper in `proxy/helpers_test.go`:
|
||||
|
||||
```go
|
||||
// testConfigFromYAML substitutes simple-responder paths and loads through
|
||||
// the real config pipeline (env vars, macros, port assignment, etc.)
|
||||
func testConfigFromYAML(t *testing.T, yamlTmpl string) config.Config {
|
||||
t.Helper()
|
||||
yamlStr := strings.ReplaceAll(yamlTmpl, "{{RESPONDER}}", filepath.ToSlash(simpleResponderPath))
|
||||
cfg, err := config.LoadConfigFromReader(strings.NewReader(yamlStr))
|
||||
require.NoError(t, err)
|
||||
return cfg
|
||||
}
|
||||
```
|
||||
|
||||
Tests would then look like:
|
||||
|
||||
```go
|
||||
func TestProxyManager_SwapProcessCorrectly(t *testing.T) {
|
||||
config := testConfigFromYAML(t, `
|
||||
healthCheckTimeout: 15
|
||||
logLevel: error
|
||||
models:
|
||||
model1:
|
||||
cmd: {{RESPONDER}} --port ${PORT} -silent -respond model1
|
||||
model2:
|
||||
cmd: {{RESPONDER}} --port ${PORT} -silent -respond model2
|
||||
`)
|
||||
proxy := New(config)
|
||||
// ... same assertions
|
||||
}
|
||||
```
|
||||
|
||||
**Why this stage first:** Zero production code changes. Pure test-side refactoring. Can be done incrementally - migrate tests one at a time. Each migrated test now validates the full config pipeline.
|
||||
|
||||
**Scope:** ~20-30 tests in `proxymanager_test.go`, `processgroup_test.go`, `peerproxy_test.go`.
|
||||
|
||||
### Stage 2: Injected test handler (eliminate simple-responder for routing tests)
|
||||
|
||||
**Goal:** Replace simple-responder subprocess launches with an injected `http.Handler` for tests that don't specifically test process lifecycle.
|
||||
|
||||
**Effort:** Medium | **Impact:** 10-100x faster routing tests | **Risk:** Low (additive, no existing code broken)
|
||||
|
||||
Add a `testHandler http.Handler` field to `Process`. When set, `ProxyRequest` delegates directly to this handler instead of going through the reverse proxy. No subprocess, no health checks, no TCP roundtrip.
|
||||
|
||||
**2a. Add testHandler to Process:**
|
||||
|
||||
```go
|
||||
// In Process struct (process.go):
|
||||
testHandler http.Handler // set only in tests; bypasses subprocess and reverse proxy
|
||||
```
|
||||
|
||||
In `Process.Start()`, skip subprocess + health check when handler is set:
|
||||
|
||||
```go
|
||||
func (p *Process) start() error {
|
||||
if p.testHandler != nil {
|
||||
p.setState(StateReady)
|
||||
return nil
|
||||
}
|
||||
// existing subprocess logic...
|
||||
}
|
||||
```
|
||||
|
||||
In `Process.ProxyRequest()`, delegate directly to the handler:
|
||||
|
||||
```go
|
||||
// Before the reverseProxy.ServeHTTP call:
|
||||
if p.testHandler != nil {
|
||||
p.testHandler.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
```
|
||||
|
||||
**2b. Test helper to create the handler:**
|
||||
|
||||
```go
|
||||
// newTestHandler returns an http.Handler that mimics llama.cpp's API
|
||||
// (same endpoints as simple-responder).
|
||||
func newTestHandler(respond string) http.Handler {
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/v1/chat/completions", func(w http.ResponseWriter, r *http.Request) { ... })
|
||||
mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) { ... })
|
||||
// ... other endpoints
|
||||
return mux
|
||||
}
|
||||
```
|
||||
|
||||
Tests for routing/auth/CORS/streaming then become:
|
||||
|
||||
```go
|
||||
func TestProxyManager_AuthRequired(t *testing.T) {
|
||||
handler := newTestHandler("model1")
|
||||
|
||||
config := testConfigFromYAML(t, `
|
||||
healthCheckTimeout: 15
|
||||
logLevel: error
|
||||
requiredAPIKeys: [test-key]
|
||||
models:
|
||||
model1:
|
||||
cmd: {{RESPONDER}} --port ${PORT} -silent -respond model1
|
||||
`)
|
||||
pm := NewProxyManager(config)
|
||||
// inject handler — skips subprocess, health check, port allocation
|
||||
pm.processGroups["model1"].process.testHandler = handler
|
||||
}
|
||||
```
|
||||
|
||||
**Why this matters:** The handler is called directly in-process. No subprocess spawn, no health check timeout, no port allocation, no TCP roundtrip, no reverse proxy overhead. Routing tests go from ~100ms each (process startup + health check) to ~1ms. Unlike an `httptest.Server` approach, there are zero network hops.
|
||||
|
||||
**Why not blank-cmd + proxy URL:** A blank `cmd` with a `proxy` field pointing at `httptest.Server` still requires a real TCP roundtrip through the reverse proxy and introduces "external process" semantics to the config schema. Injecting the handler directly keeps it purely a test concern with no config changes.
|
||||
|
||||
**Scope:** Most tests in `proxymanager_test.go` (auth, CORS, model listing, streaming, peer proxy), `peerproxy_test.go`, `metrics_monitor_test.go`.
|
||||
|
||||
### Stage 3: Migrate tests incrementally
|
||||
|
||||
**Goal:** Convert existing tests to use the Stage 1 + Stage 2 helpers.
|
||||
|
||||
**Effort:** Medium | **Impact:** Cleaner, more reliable tests | **Risk:** None
|
||||
|
||||
Priority order:
|
||||
1. `proxymanager_test.go` routing tests (highest count, most repetition)
|
||||
2. `peerproxy_test.go` (straightforward, all HTTP routing)
|
||||
3. `metrics_monitor_test.go` (capture logic doesn't need real processes)
|
||||
4. `processgroup_test.go` swap tests (keep simple-responder for actual swap lifecycle tests)
|
||||
|
||||
Tests that **must keep simple-responder:**
|
||||
- Process lifecycle: start/stop, SIGKILL, SIGTERM, TTL expiry, health check failures, failed start counting
|
||||
- ProcessGroup swap concurrency (the port-collision test in `TestProcessGroup_ProxyRequestSwapIsTrueParallel`)
|
||||
|
||||
**Scope:** ~60-70% of tests can drop simple-responder.
|
||||
|
||||
### Stage 4 (optional): Process interface for ProcessGroup
|
||||
|
||||
**Goal:** Enable pure unit tests of ProcessGroup's swap/exclusive/concurrency logic without any HTTP server at all.
|
||||
|
||||
**Effort:** High | **Impact:** Pure unit tests possible | **Risk:** Medium (refactor core code)
|
||||
|
||||
```go
|
||||
type ProcessController interface {
|
||||
Start() error
|
||||
Stop(StopStrategy)
|
||||
ProxyRequest(http.ResponseWriter, *http.Request) error
|
||||
CurrentState() ProcessState
|
||||
ID() string
|
||||
SetState(ProcessState) // for test setup
|
||||
}
|
||||
```
|
||||
|
||||
This requires:
|
||||
- Extracting the interface
|
||||
- A `MockProcess` implementation
|
||||
- Refactoring `ProcessGroup` to use the interface instead of `*Process`
|
||||
|
||||
**Recommendation:** Only do this if ProcessGroup grows significantly more complex. Stages 1-3 give 80% of the benefit for 20% of the effort.
|
||||
|
||||
## Effort/Impact Summary
|
||||
|
||||
| Stage | Effort | Impact | Risk |
|
||||
|-------|--------|--------|------|
|
||||
| 1. YAML config helper | Low | Config bugs caught earlier | None |
|
||||
| 2. Injected test handler | Medium | 10-100x faster routing tests | Low |
|
||||
| 3. Migrate tests | Medium | Cleaner, more reliable tests | None |
|
||||
| 4. Process interface | High | Pure unit tests possible | Medium |
|
||||
|
||||
**Recommended approach:** Do stages 1-3 in order. Each stage is independently valuable and can ship on its own. Stage 4 is deferred unless there's a specific need.
|
||||
Reference in New Issue
Block a user