Consolidate and fix VirtualMCP E2E tests (#2978)

* Fix VirtualMCP e2e test port accessibility checks

Enhance test helpers to verify MCP server is actually listening before
attempting client connections, preventing EOF errors.

The original helpers only checked Kubernetes resource status (Ready
condition and NodePort assignment) but didn't verify the actual pods
were running or the port was accessible. This timing gap caused tests
to fail with EOF errors when connecting to localhost:30020.

* fix ci

---------

Co-authored-by: taskbot <[email protected]>

Author: yrobla

test/e2e/thv-operator/virtualmcp/helpers.go

@@ -1,15 +1,20 @@
+// Package virtualmcp provides helper functions for VirtualMCP E2E tests.
 package virtualmcp
 
 import (
 	"bytes"
 	"context"
 	"fmt"
 	"io"
+	"net"
+	"net/http"
 	"os"
+	"strings"
 	"time"
 
 	mcpclient "github.com/mark3labs/mcp-go/client"
 	"github.com/mark3labs/mcp-go/mcp"
+	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -25,7 +30,14 @@ import (
 )
 
 // WaitForVirtualMCPServerReady waits for a VirtualMCPServer to reach Ready status
-func WaitForVirtualMCPServerReady(ctx context.Context, c client.Client, name, namespace string, timeout time.Duration) {
+// and ensures the associated pods are actually running and ready
+func WaitForVirtualMCPServerReady(
+	ctx context.Context,
+	c client.Client,
+	name, namespace string,
+	timeout time.Duration,
+	pollingInterval time.Duration,
+) {
 	vmcpServer := &mcpv1alpha1.VirtualMCPServer{}
 
 	gomega.Eventually(func() error {
@@ -39,13 +51,63 @@ func WaitForVirtualMCPServerReady(ctx context.Context, c client.Client, name, na
 		for _, condition := range vmcpServer.Status.Conditions {
 			if condition.Type == "Ready" {
 				if condition.Status == "True" {
+					// Also check that the pods are actually running and ready
+					labels := map[string]string{
+						"app.kubernetes.io/name":     "virtualmcpserver",
+						"app.kubernetes.io/instance": name,
+					}
+					if err := checkPodsReady(ctx, c, namespace, labels); err != nil {
+						return fmt.Errorf("VirtualMCPServer ready but pods not ready: %w", err)
+					}
 					return nil
 				}
 				return fmt.Errorf("ready condition is %s: %s", condition.Status, condition.Message)
 			}
 		}
 		return fmt.Errorf("ready condition not found")
-	}, timeout, 5*time.Second).Should(gomega.Succeed())
+	}, timeout, pollingInterval).Should(gomega.Succeed())
+}
+
+// checkPodsReady checks if all pods matching the given labels are ready
+func checkPodsReady(ctx context.Context, c client.Client, namespace string, labels map[string]string) error {
+	podList := &corev1.PodList{}
+	if err := c.List(ctx, podList,
+		client.InNamespace(namespace),
+		client.MatchingLabels(labels)); err != nil {
+		return fmt.Errorf("failed to list pods: %w", err)
+	}
+
+	if len(podList.Items) == 0 {
+		return fmt.Errorf("no pods found with labels %v", labels)
+	}
+
+	for _, pod := range podList.Items {
+		if pod.Status.Phase != corev1.PodRunning {
+			return fmt.Errorf("pod %s is in phase %s", pod.Name, pod.Status.Phase)
+		}
+
+		containerReady := false
+		podReady := false
+
+		for _, condition := range pod.Status.Conditions {
+			if condition.Type == corev1.ContainersReady {
+				containerReady = condition.Status == corev1.ConditionTrue
+			}
+
+			if condition.Type == corev1.PodReady {
+				podReady = condition.Status == corev1.ConditionTrue
+			}
+		}
+
+		if !containerReady {
+			return fmt.Errorf("pod %s containers not ready", pod.Name)
+		}
+
+		if !podReady {
+			return fmt.Errorf("pod %s not ready", pod.Name)
+		}
+	}
+	return nil
 }
 
 // InitializedMCPClient holds an initialized MCP client with its associated context
@@ -163,47 +225,17 @@ func GetVirtualMCPServerPods(ctx context.Context, c client.Client, vmcpServerNam
 }
 
 // WaitForPodsReady waits for all pods matching labels to be ready
-func WaitForPodsReady(ctx context.Context, c client.Client, namespace string, labels map[string]string, timeout time.Duration) {
+func WaitForPodsReady(
+	ctx context.Context,
+	c client.Client,
+	namespace string,
+	labels map[string]string,
+	timeout time.Duration,
+	pollingInterval time.Duration,
+) {
 	gomega.Eventually(func() error {
-		podList := &corev1.PodList{}
-		if err := c.List(ctx, podList,
-			client.InNamespace(namespace),
-			client.MatchingLabels(labels)); err != nil {
-			return err
-		}
-
-		if len(podList.Items) == 0 {
-			return fmt.Errorf("no pods found with labels %v", labels)
-		}
-
-		for _, pod := range podList.Items {
-			if pod.Status.Phase != corev1.PodRunning {
-				return fmt.Errorf("pod %s is in phase %s", pod.Name, pod.Status.Phase)
-			}
-
-			containerReady := false
-			podReady := false
-
-			for _, condition := range pod.Status.Conditions {
-				if condition.Type == corev1.ContainersReady {
-					containerReady = condition.Status == corev1.ConditionTrue
-				}
-
-				if condition.Type == corev1.PodReady {
-					podReady = condition.Status == corev1.ConditionTrue
-				}
-			}
-
-			if !containerReady {
-				return fmt.Errorf("pod %s containers not ready", pod.Name)
-			}
-
-			if !podReady {
-				return fmt.Errorf("pod %s not ready", pod.Name)
-			}
-		}
-		return nil
-	}, timeout, 5*time.Second).Should(gomega.Succeed())
+		return checkPodsReady(ctx, c, namespace, labels)
+	}, timeout, pollingInterval).Should(gomega.Succeed())
 }
 
 // GetMCPGroupBackends returns the list of backend MCPServers in an MCPGroup
@@ -269,6 +301,7 @@ func WaitForCondition(
 	conditionType string,
 	expectedStatus string,
 	timeout time.Duration,
+	pollingInterval time.Duration,
 ) {
 	gomega.Eventually(func() error {
 		vmcpServer := &mcpv1alpha1.VirtualMCPServer{}
@@ -284,7 +317,7 @@ func WaitForCondition(
 		}
 
 		return fmt.Errorf("condition %s not found with status %s", conditionType, expectedStatus)
-	}, timeout, 5*time.Second).Should(gomega.Succeed())
+	}, timeout, pollingInterval).Should(gomega.Succeed())
 }
 
 // OIDC Testing Helpers
@@ -680,7 +713,8 @@ func CreateMCPServerAndWait(
 	return backend
 }
 
-// GetVMCPNodePort waits for the VirtualMCPServer service to have a NodePort assigned and returns it.
+// GetVMCPNodePort waits for the VirtualMCPServer service to have a NodePort assigned
+// and verifies the port is accessible.
 func GetVMCPNodePort(
 	ctx context.Context,
 	c client.Client,
@@ -703,12 +737,114 @@ func GetVMCPNodePort(
 			return fmt.Errorf("nodePort not assigned for vmcp service %s", serviceName)
 		}
 		nodePort = service.Spec.Ports[0].NodePort
+
+		// Verify the TCP port is accessible
+		if err := checkPortAccessible(nodePort, 1*time.Second); err != nil {
+			return fmt.Errorf("nodePort %d assigned but not accessible: %w", nodePort, err)
+		}
+
+		// Verify the HTTP server is ready to handle requests
+		if err := checkHTTPHealthReady(nodePort, 2*time.Second); err != nil {
+			return fmt.Errorf("nodePort %d accessible but HTTP server not ready: %w", nodePort, err)
+		}
+
 		return nil
-	}, timeout, pollingInterval).Should(gomega.Succeed(), "NodePort should be assigned")
+	}, timeout, pollingInterval).Should(gomega.Succeed(), "NodePort should be assigned and HTTP server ready")
 
 	return nodePort
 }
 
+// checkPortAccessible verifies that the port is open and accepting TCP connections.
+// This is a lightweight check that completes in milliseconds instead of the seconds
+// required for a full MCP session initialization.
+func checkPortAccessible(nodePort int32, timeout time.Duration) error {
+	address := fmt.Sprintf("localhost:%d", nodePort)
+	conn, err := net.DialTimeout("tcp", address, timeout)
+	if err != nil {
+		return fmt.Errorf("port %d not accessible: %w", nodePort, err)
+	}
+	// Port is accessible - close connection (ignore errors as port accessibility is confirmed)
+	_ = conn.Close()
+	return nil
+}
+
+// checkHTTPHealthReady verifies the HTTP server is ready by checking the /health endpoint.
+// This is more reliable than just TCP check as it ensures the application is serving requests.
+func checkHTTPHealthReady(nodePort int32, timeout time.Duration) error {
+	httpClient := &http.Client{Timeout: timeout}
+	url := fmt.Sprintf("http://localhost:%d/health", nodePort)
+
+	resp, err := httpClient.Get(url)
+	if err != nil {
+		return fmt.Errorf("health check failed for port %d: %w", nodePort, err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("health check returned status %d for port %d", resp.StatusCode, nodePort)
+	}
+
+	return nil
+}
+
+// TestToolListingAndCall is a shared helper that creates an MCP client, lists tools,
+// finds a tool matching the pattern, calls it, and verifies the response.
+// This eliminates the duplicate "create client → list → call" pattern found in most tests.
+func TestToolListingAndCall(vmcpNodePort int32, clientName string, toolNamePattern string, testInput string) {
+	ginkgo.By("Creating and initializing MCP client")
+	mcpClient, err := CreateInitializedMCPClient(vmcpNodePort, clientName, 30*time.Second)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred())
+	defer mcpClient.Close()
+
+	ginkgo.By("Listing tools from VirtualMCPServer")
+	listRequest := mcp.ListToolsRequest{}
+	tools, err := mcpClient.Client.ListTools(mcpClient.Ctx, listRequest)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred())
+	gomega.Expect(tools.Tools).ToNot(gomega.BeEmpty())
+
+	// Find a tool matching the pattern
+	var targetToolName string
+	for _, tool := range tools.Tools {
+		if strings.Contains(tool.Name, toolNamePattern) {
+			targetToolName = tool.Name
+			break
+		}
+	}
+	gomega.Expect(targetToolName).ToNot(gomega.BeEmpty(), fmt.Sprintf("Should find a tool matching pattern: %s", toolNamePattern))
+
+	ginkgo.By(fmt.Sprintf("Calling tool: %s", targetToolName))
+	callRequest := mcp.CallToolRequest{}
+	callRequest.Params.Name = targetToolName
+	callRequest.Params.Arguments = map[string]any{
+		"input": testInput,
+	}
+
+	result, err := mcpClient.Client.CallTool(mcpClient.Ctx, callRequest)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred(), "Should successfully call tool")
+	gomega.Expect(result).ToNot(gomega.BeNil())
+	gomega.Expect(result.Content).ToNot(gomega.BeEmpty(), "Should have content in response")
+
+	fmt.Fprintf(ginkgo.GinkgoWriter, "✓ Successfully called tool %s\n", targetToolName)
+}
+
+// TestToolListing is a shared helper that creates an MCP client and lists tools.
+// Returns the list of tools for further assertions.
+func TestToolListing(vmcpNodePort int32, clientName string) []mcp.Tool {
+	ginkgo.By("Creating and initializing MCP client")
+	mcpClient, err := CreateInitializedMCPClient(vmcpNodePort, clientName, 30*time.Second)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred())
+	defer mcpClient.Close()
+
+	ginkgo.By("Listing tools from VirtualMCPServer")
+	listRequest := mcp.ListToolsRequest{}
+	toolsResult, err := mcpClient.Client.ListTools(mcpClient.Ctx, listRequest)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred())
+	gomega.Expect(toolsResult.Tools).ToNot(gomega.BeEmpty())
+
+	fmt.Fprintf(ginkgo.GinkgoWriter, "Listed %d tools from VirtualMCPServer\n", len(toolsResult.Tools))
+	return toolsResult.Tools
+}
+
 // InstrumentedBackendScript is an instrumented backend script that tracks Bearer tokens
 const InstrumentedBackendScript = `
 pip install --quiet flask && python3 - <<'PYTHON_SCRIPT'

test/e2e/thv-operator/virtualmcp/virtualmcp_aggregation_filtering_test.go

@@ -76,7 +76,7 @@ var _ = Describe("VirtualMCPServer Aggregation Filtering", Ordered, func() {
 		Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed())
 
 		By("Waiting for VirtualMCPServer to be ready")
-		WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout)
+		WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval)
 
 		By("Getting NodePort for VirtualMCPServer")
 		vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval)
@@ -159,40 +159,8 @@ var _ = Describe("VirtualMCPServer Aggregation Filtering", Ordered, func() {
 		})
 
 		It("should still allow calling filtered tools", func() {
-			By("Creating and initializing MCP client for VirtualMCPServer")
-			mcpClient, err := CreateInitializedMCPClient(vmcpNodePort, "toolhive-filtering-test", 30*time.Second)
-			Expect(err).ToNot(HaveOccurred())
-			defer mcpClient.Close()
-
-			By("Listing available tools")
-			listRequest := mcp.ListToolsRequest{}
-			tools, err := mcpClient.Client.ListTools(mcpClient.Ctx, listRequest)
-			Expect(err).ToNot(HaveOccurred())
-
-			// Find the backend1 echo tool
-			var targetToolName string
-			for _, tool := range tools.Tools {
-				if strings.Contains(tool.Name, backend1Name) && strings.Contains(tool.Name, "echo") {
-					targetToolName = tool.Name
-					break
-				}
-			}
-			Expect(targetToolName).ToNot(BeEmpty(), "Should find echo tool from backend1")
-
-			By(fmt.Sprintf("Calling filtered echo tool: %s", targetToolName))
-			testInput := "filtered123"
-			callRequest := mcp.CallToolRequest{}
-			callRequest.Params.Name = targetToolName
-			callRequest.Params.Arguments = map[string]any{
-				"input": testInput,
-			}
-
-			result, err := mcpClient.Client.CallTool(mcpClient.Ctx, callRequest)
-			Expect(err).ToNot(HaveOccurred(), "Should be able to call filtered tool")
-			Expect(result).ToNot(BeNil())
-			Expect(result.Content).ToNot(BeEmpty(), "Should have content in response")
-
-			GinkgoWriter.Printf("Filtered tool call successful: %s\n", targetToolName)
+			// Use shared helper to test tool listing and calling
+			TestToolListingAndCall(vmcpNodePort, "toolhive-filtering-test", "echo", "filtered123")
 		})
 	})
 

test/e2e/thv-operator/virtualmcp/virtualmcp_aggregation_overrides_test.go

@@ -76,7 +76,7 @@ var _ = Describe("VirtualMCPServer Tool Overrides", Ordered, func() {
 		Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed())
 
 		By("Waiting for VirtualMCPServer to be ready")
-		WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout)
+		WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval)
 
 		By("Getting NodePort for VirtualMCPServer")
 		vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval)
@@ -166,28 +166,8 @@ var _ = Describe("VirtualMCPServer Tool Overrides", Ordered, func() {
 		})
 
 		It("should allow calling the renamed tool", func() {
-			By("Creating and initializing MCP client for VirtualMCPServer")
-			mcpClient, err := CreateInitializedMCPClient(vmcpNodePort, "toolhive-overrides-test", 30*time.Second)
-			Expect(err).ToNot(HaveOccurred())
-			defer mcpClient.Close()
-
-			renamedToolFullName := fmt.Sprintf("%s_%s", backendName, renamedToolName)
-			By(fmt.Sprintf("Calling renamed tool: %s", renamedToolFullName))
-
-			testInput := "override_test_123"
-			callRequest := mcp.CallToolRequest{}
-			callRequest.Params.Name = renamedToolFullName
-			callRequest.Params.Arguments = map[string]any{
-				"input": testInput,
-			}
-
-			result, err := mcpClient.Client.CallTool(mcpClient.Ctx, callRequest)
-			Expect(err).ToNot(HaveOccurred(), "Should be able to call renamed tool")
-			Expect(result).ToNot(BeNil())
-			Expect(result.Content).ToNot(BeEmpty(), "Should have content in response")
-
-			// Yardstick echo tool echoes back the input
-			GinkgoWriter.Printf("Renamed tool call result: %+v\n", result.Content)
+			// Use shared helper to test tool listing and calling
+			TestToolListingAndCall(vmcpNodePort, "toolhive-overrides-test", renamedToolName, "override_test_123")
 		})
 	})