diff --git a/workspace-server/internal/memory/wiring/wiring_test.go b/workspace-server/internal/memory/wiring/wiring_test.go index 5ce2c274..ee7a6840 100644 --- a/workspace-server/internal/memory/wiring/wiring_test.go +++ b/workspace-server/internal/memory/wiring/wiring_test.go @@ -2,7 +2,9 @@ package wiring import ( "context" - "errors" + "net/http" + "net/http/httptest" + "sync" "testing" "github.com/DATA-DOG/go-sqlmock" @@ -56,57 +58,103 @@ func TestNamespaceCleanupFn_NilPlugin(t *testing.T) { } } -// TestNamespaceCleanupFn_NamespaceFormat pins that the closure -// computes the right namespace string for a workspace-id input -// ("workspace:") — this is the contract the plugin expects and -// the I5 fixup tests rely on. +// TestNamespaceCleanupFn_HitsPluginAtCorrectNamespace is the real +// integration gate for the closure: it spins up an httptest.Server +// that records every DELETE request, points MEMORY_PLUGIN_URL at it, +// runs Build(), then invokes the returned closure and asserts the +// server saw `DELETE /v1/namespaces/workspace:`. // -// We can't easily inject a mock plugin into Build's output (it -// constructs a real *mclient.Client). Instead we verify behavior -// indirectly: a closure-returning helper with a stub plugin would be -// ideal, but the goal here is to pin the namespace-string format so -// future refactors don't silently break it. Direct test of the -// underlying string is the cheapest gate. -func TestNamespaceCleanupFn_NamespaceFormat(t *testing.T) { - // Build a closure that records the namespace it was called with. - // We test the FORMAT directly because the closure inside - // NamespaceCleanupFn is an internal-only string concatenation. - want := "workspace:abc-123" - got := "workspace:" + "abc-123" - if got != want { - t.Errorf("namespace format drift: got %q, want %q", got, want) +// This replaces two earlier tests that exercised parallel +// implementations rather than the production closure (caught in +// self-review). +func TestNamespaceCleanupFn_HitsPluginAtCorrectNamespace(t *testing.T) { + var ( + mu sync.Mutex + gotPaths []string + gotMethods []string + ) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + gotPaths = append(gotPaths, r.URL.Path) + gotMethods = append(gotMethods, r.Method) + mu.Unlock() + switch r.URL.Path { + case "/v1/health": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"status":"ok","version":"1.0.0","capabilities":[]}`)) + default: + w.WriteHeader(http.StatusNoContent) + } + })) + t.Cleanup(srv.Close) + + t.Setenv("MEMORY_PLUGIN_URL", srv.URL) + db, _, _ := sqlmock.New() + defer db.Close() + + bundle := Build(db) + if bundle == nil { + t.Fatal("Build returned nil with MEMORY_PLUGIN_URL set") + } + cleanup := bundle.NamespaceCleanupFn() + if cleanup == nil { + t.Fatal("NamespaceCleanupFn returned nil with non-nil Plugin") + } + + cleanup(context.Background(), "abc-123") + + mu.Lock() + defer mu.Unlock() + // Two requests expected: /v1/health probe at Boot + DELETE for cleanup. + foundDelete := false + for i, p := range gotPaths { + if gotMethods[i] == "DELETE" && p == "/v1/namespaces/workspace:abc-123" { + foundDelete = true + } + } + if !foundDelete { + t.Errorf("expected DELETE /v1/namespaces/workspace:abc-123, got %v", + pathsAndMethods(gotPaths, gotMethods)) } } -// stubPluginCallTracker is for the integration-shaped test below. -type stubPluginCallTracker struct { - called []string - err error -} +// TestNamespaceCleanupFn_PluginErrorDoesNotPanic exercises the failure +// path for real: server returns 500 on DELETE; the closure must log +// and return without propagating. Replaces the parallel-implementation +// version that didn't actually test the production code. +func TestNamespaceCleanupFn_PluginErrorDoesNotPanic(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v1/health" { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"status":"ok","version":"1.0.0","capabilities":[]}`)) + return + } + http.Error(w, "boom", http.StatusInternalServerError) + })) + t.Cleanup(srv.Close) -func (s *stubPluginCallTracker) Delete(_ context.Context, ns string) error { - s.called = append(s.called, ns) - return s.err -} + t.Setenv("MEMORY_PLUGIN_URL", srv.URL) + db, _, _ := sqlmock.New() + defer db.Close() -// TestNamespaceCleanupFn_FailureLogsButReturns: simulate the -// production behavior where a plugin DeleteNamespace fails. The -// closure logs and returns; it MUST NOT panic or propagate. -// -// Direct test of the bundle's closure isn't feasible without -// dependency injection at the Bundle struct level. We test the -// behavioral contract via a parallel implementation instead — the -// callsite in workspace_crud.go calls the closure unconditionally, -// so any panic would be production-visible. -func TestNamespaceCleanupFn_FailureLogsButReturns(t *testing.T) { - tracker := &stubPluginCallTracker{err: errors.New("plugin dead")} - // Mirror the closure's logic against the stub. - cleanup := func(ctx context.Context, workspaceID string) { - ns := "workspace:" + workspaceID - _ = tracker.Delete(ctx, ns) // production logs but doesn't propagate - } + bundle := Build(db) + cleanup := bundle.NamespaceCleanupFn() + + // Must not panic, must not propagate the 500. Recovering with + // defer is belt-and-suspenders — production calls this from a + // for-loop in workspace_crud.go that has no recover. + defer func() { + if r := recover(); r != nil { + t.Errorf("cleanup panicked on plugin 500: %v", r) + } + }() cleanup(context.Background(), "ws-1") - if len(tracker.called) != 1 || tracker.called[0] != "workspace:ws-1" { - t.Errorf("called = %v, want [workspace:ws-1]", tracker.called) - } +} + +func pathsAndMethods(paths, methods []string) []string { + out := make([]string, len(paths)) + for i := range paths { + out[i] = methods[i] + " " + paths[i] + } + return out }