Skip to content

[Perf] Suggestion to improve Websocket test reliability  #86

@Unique-Divine

Description

@Unique-Divine

⚠️ Potential issue | 🟠 Major

Avoid relying on external WebSocket services in unit tests

Both tests dial wss://echo.websocket.org, which is a third-party host that has been intermittently unavailable for years and is routinely blocked in CI environments without outbound Internet access. The DNS-only guard still lets the tests run and hang/fail when the TLS handshake or dial is rejected, so this will introduce flakiness into the suite you just sped up. Please replace the external dependency with an in-process echo server (e.g., httptest.NewServer + websocket.Upgrader) or a mock. Doing so also lets you delete the skip helper entirely.

A minimal sketch:

@@
-import (
-	"context"
-	"net"
-	"os"
-	"testing"
-	"time"
-
-	"github.com/rs/zerolog"
-	"github.com/stretchr/testify/require"
-)
+import (
+	"context"
+	"io"
+	"net"
+	"net/http/httptest"
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/gorilla/websocket"
+	"github.com/rs/zerolog"
+	"github.com/stretchr/testify/require"
+)
@@
-func TestWebsocketSuccess(t *testing.T) {
-	if !canConnectToWebsocket("wss://echo.websocket.org") {
-		t.Skip("Skipping test: cannot reach echo.websocket.org (network may be unavailable)")
-	}
-
-	ws := NewWebsocket("wss://echo.websocket.org", []byte("test"), zerolog.New(os.Stderr))
+func TestWebsocketSuccess(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		upgrader := websocket.Upgrader{}
+		conn, err := upgrader.Upgrade(w, r, nil)
+		require.NoError(t, err)
+		defer conn.Close()
+		for {
+			mt, payload, err := conn.ReadMessage()
+			if err != nil {
+				return
+			}
+			require.NoError(t, conn.WriteMessage(mt, payload))
+		}
+	}))
+	defer server.Close()
+
+	wsURL := "ws" + strings.TrimPrefix(server.URL, "http")
+	ws := NewWebsocket(wsURL, []byte("test"), zerolog.New(io.Discard))
@@
-func TestWebsocketExplicitClose(t *testing.T) {
-	if !canConnectToWebsocket("wss://echo.websocket.org") {
-		t.Skip("Skipping test: cannot reach echo.websocket.org (network may be unavailable)")
-	}
-
-	ws := NewWebsocket("wss://echo.websocket.org", []byte("test"), zerolog.New(os.Stderr))
+func TestWebsocketExplicitClose(t *testing.T) {
+	ws := NewWebsocket(wsURL, []byte("test"), zerolog.New(io.Discard))
 	require.NotPanics(t, func() {
 		ws.Close()
 	})
 }

This keeps the tests deterministic, offline-capable, and fast while still exercising the production websocket code path.

[scratchpad_end] -->

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In feeder/eventstream_websocket_test.go around lines 45-95, the tests currently
dial the external host wss://echo.websocket.org which causes flaky CI and
network-dependent failures; replace this external dependency with an in-process
echo server. Remove calls to canConnectToWebsocket and t.Skip; instead create an
httptest.NewServer (or httptest.NewTLSServer) with a websocket.Upgrader that
echoes messages, use its URL (convert http->ws or https->wss as appropriate)
when constructing NewWebsocket, ensure the server is closed via defer after
test, and keep the existing message read/timeout logic and the explicit Close
assertion so the tests remain deterministic, offline-capable, and fast.

Originally posted by @coderabbitai[bot] in #81 (comment)

Metadata

Metadata

Assignees

No one assigned

    Labels

    S-triageStatus: This issue is waiting on initial triage. More Info: https://tinyurl.com/25uty9w5

    Type

    No type

    Projects

    Status

    ⚡ Building 🧱

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions