From 53a34be7929683f50ceac8bc540673246d1a2107 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Tue, 20 Jan 2026 13:02:19 +0000 Subject: [PATCH] Run the server as a standalone process Previously it was instantiating the server in-process. Use the full binary to ensure that the tests are realistic. --- test/e2e/api_helpers.go | 144 ++++++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 65 deletions(-) diff --git a/test/e2e/api_helpers.go b/test/e2e/api_helpers.go index 5ce35c68f1..b5a65edb8b 100644 --- a/test/e2e/api_helpers.go +++ b/test/e2e/api_helpers.go @@ -4,15 +4,16 @@ package e2e import ( "context" "fmt" - "net" "net/http" + "os/exec" + "strconv" + "strings" "time" . "github.com/onsi/ginkgo/v2" //nolint:staticcheck // Standard practice for Ginkgo . "github.com/onsi/gomega" //nolint:staticcheck // Standard practice for Gomega - "github.com/stacklok/toolhive/pkg/api" - "github.com/stacklok/toolhive/pkg/container" + "github.com/stacklok/toolhive/pkg/networking" ) // ServerConfig holds configuration for the API server in tests @@ -33,83 +34,89 @@ func NewServerConfig() *ServerConfig { } } -// Server represents a running API server instance for testing +// Server represents a running API server instance for testing. +// It runs `thv serve` as a subprocess. type Server struct { config *ServerConfig baseURL string + cmd *exec.Cmd ctx context.Context cancel context.CancelFunc - serverErr chan error - done chan struct{} httpClient *http.Client + port int + stderr *strings.Builder + stdout *strings.Builder } -// NewServer creates and starts a new API server instance +// NewServer creates and starts a new API server instance by running `thv serve` as a subprocess. func NewServer(config *ServerConfig) (*Server, error) { - ctx, cancel := context.WithCancel(context.Background()) + testConfig := NewTestConfig() - // Create a temporary listener to get a free port - listener, err := net.Listen("tcp", config.Address) + // Find a free port + port, err := networking.FindOrUsePort(0) if err != nil { + return nil, fmt.Errorf("failed to find free port: %w", err) + } + + // Create temporary config directory (similar to CLI tests) + tempXdgConfigHome := GinkgoT().TempDir() + tempHome := GinkgoT().TempDir() + + ctx, cancel := context.WithCancel(context.Background()) + + // Create string builders to capture output + var stdout, stderr strings.Builder + + // Create the command: thv serve --host 127.0.0.1 --port + //nolint:gosec // Intentional for e2e testing + cmd := exec.CommandContext( + ctx, + testConfig.THVBinary, + "serve", + "--host", + "127.0.0.1", + "--port", + strconv.Itoa(port), + ) + // Set environment variables including temporary config paths + cmd.Env = append([]string{ + "TOOLHIVE_DEV=true", + fmt.Sprintf("XDG_CONFIG_HOME=%s", tempXdgConfigHome), + fmt.Sprintf("HOME=%s", tempHome), + }, cmd.Env...) + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + // Start the server process + if err := cmd.Start(); err != nil { cancel() - return nil, fmt.Errorf("failed to create listener: %w", err) + return nil, fmt.Errorf("failed to start thv serve: %w", err) } - actualAddr := listener.Addr().String() - // Close the listener immediately as the server will create its own - _ = listener.Close() server := &Server{ - config: config, - baseURL: fmt.Sprintf("http://%s", actualAddr), - ctx: ctx, - cancel: cancel, - serverErr: make(chan error, 1), - done: make(chan struct{}), + config: config, + baseURL: fmt.Sprintf("http://127.0.0.1:%d", port), + cmd: cmd, + ctx: ctx, + cancel: cancel, httpClient: &http.Client{ Timeout: config.RequestTimeout, }, + port: port, + stdout: &stdout, + stderr: &stderr, } - // Start the server in a goroutine - go func() { - defer close(server.done) - // Create container runtime for the API server - containerRuntime, err := container.NewFactory().Create(ctx) - if err != nil { - server.serverErr <- fmt.Errorf("failed to create container runtime: %w", err) - return - } - - builder := api.NewServerBuilder(). - WithAddress(actualAddr). - WithUnixSocket(false). - WithDebugMode(config.DebugMode). - WithDocs(false). - WithOIDCConfig(nil). - WithContainerRuntime(containerRuntime) - - apiServer, err := api.NewServer(ctx, builder) - if err != nil { - server.serverErr <- fmt.Errorf("failed to create API server: %w", err) - return - } - - if err := apiServer.Start(ctx); err != nil { - server.serverErr <- err - return - } - }() - // Wait for server to be ready if err := server.WaitForReady(); err != nil { - server.Stop() + _ = server.Stop() return nil, err } return server, nil } -// WaitForReady waits for the API server to be ready to accept requests +// WaitForReady waits for the API server to be ready to accept requests. func (s *Server) WaitForReady() error { ctx, cancel := context.WithTimeout(context.Background(), s.config.StartTimeout) defer cancel() @@ -120,9 +127,9 @@ func (s *Server) WaitForReady() error { for { select { case <-ctx.Done(): - return fmt.Errorf("timeout waiting for API server to be ready") - case err := <-s.serverErr: - return fmt.Errorf("API server failed to start: %w", err) + // Include server logs in the error message for debugging + return fmt.Errorf("timeout waiting for API server to be ready on port %d.\nStdout: %s\nStderr: %s", + s.port, s.stdout.String(), s.stderr.String()) case <-ticker.C: // Try to connect to the health endpoint req, err := http.NewRequestWithContext(ctx, http.MethodGet, s.baseURL+"/health", nil) @@ -136,7 +143,7 @@ func (s *Server) WaitForReady() error { } _ = resp.Body.Close() - // Server is ready if we get the expected response. + // Server is ready if we get the expected response if resp.StatusCode == http.StatusNoContent { return nil } @@ -144,14 +151,21 @@ func (s *Server) WaitForReady() error { } } -// Stop stops the API server -func (s *Server) Stop() { - s.cancel() - // Wait for server to shut down gracefully - <-s.done +// Stop stops the API server subprocess. +func (s *Server) Stop() error { + if s.cancel != nil { + s.cancel() + } + + if s.cmd != nil && s.cmd.Process != nil { + // Wait for the process to exit + _ = s.cmd.Wait() + } + + return nil } -// Get performs a GET request to the specified path +// Get performs a GET request to the specified path. func (s *Server) Get(path string) (*http.Response, error) { req, err := http.NewRequestWithContext(s.ctx, http.MethodGet, s.baseURL+path, nil) if err != nil { @@ -160,7 +174,7 @@ func (s *Server) Get(path string) (*http.Response, error) { return s.httpClient.Do(req) } -// GetWithHeaders performs a GET request with custom headers +// GetWithHeaders performs a GET request with custom headers. func (s *Server) GetWithHeaders(path string, headers map[string]string) (*http.Response, error) { req, err := http.NewRequestWithContext(s.ctx, http.MethodGet, s.baseURL+path, nil) if err != nil { @@ -174,7 +188,7 @@ func (s *Server) GetWithHeaders(path string, headers map[string]string) (*http.R return s.httpClient.Do(req) } -// BaseURL returns the base URL of the API server +// BaseURL returns the base URL of the API server. func (s *Server) BaseURL() string { return s.baseURL } @@ -187,7 +201,7 @@ func StartServer(config *ServerConfig) *Server { // Register cleanup DeferCleanup(func() { - server.Stop() + _ = server.Stop() }) return server