websocket, internal/testing: support multiconn WS helpers#2205
websocket, internal/testing: support multiconn WS helpers#2205thrasher- wants to merge 2 commits intothrasher-corp:masterfrom
Conversation
thrasher-
added
jarvis
thrasher-
requested review from
cranktakular,
gloriousCode,
samuael and
shazbert
There was a problem hiding this comment.
Pull request overview
This PR fixes shared websocket test helpers (SetupWs and MockWsInstance) to work with exchanges that use multi-connection websocket management. Previously, these helpers assumed single-connection behavior, which broke for exchanges using UseMultiConnectionManagement.
Changes:
- Added
SetSubscriptionsNotRequired()andSetAllConnectionURLs()methods to the websocketManagerto support both single and multi-connection modes. - Updated
MockWsInstanceandSetupWstest helpers to use the new multi-connection-safe methods. - Added unit tests for the new manager methods and integration-level regression tests including a Poloniex proof-of-concept.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| exchange/websocket/manager.go | Added SetSubscriptionsNotRequired() and SetAllConnectionURLs() methods to Manager |
| internal/testing/exchange/exchange.go | Updated MockWsInstance and SetupWs to use the new multi-connection-safe helpers |
| exchange/websocket/manager_test.go | Added unit tests for SetSubscriptionsNotRequired and SetAllConnectionURLs |
| internal/testing/exchange/exchange_test.go | Added regression tests for multi-connection support in MockWsInstance and SetupWs |
| exchanges/poloniex/poloniex_websocket_helper_test.go | Added Poloniex-specific proof-of-concept test for multi-connection SetupWs |
| for _, ws := range m.connectionManager { | ||
| if ws.setup == nil { | ||
| ws.setup = &ConnectionSetup{} | ||
| } | ||
| ws.setup.SubscriptionsNotRequired = true | ||
| } |
There was a problem hiding this comment.
Creating a new ConnectionSetup{} for a nil ws.setup silently masks a programming error — a websocket entry in connectionManager should always have been initialized via SetupNewConnection. This guard could hide bugs where the connection manager is in an inconsistent state. Consider returning an error or logging a warning instead of silently initializing an empty setup.
There was a problem hiding this comment.
Jarvis: I’m keeping this guard as-is. In normal flow multi-connection entries are created via SetupNewConnection with a non-nil setup, so this is a cheap defensive safeguard rather than a primary code path, and I’d rather keep the helper resilient here than widen it into a broader error-return change.
| for _, ws := range m.connectionManager { | ||
| if ws.setup == nil { | ||
| ws.setup = &ConnectionSetup{} | ||
| } | ||
| ws.setup.URL = u | ||
| for _, conn := range ws.connections { | ||
| conn.SetURL(u) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same concern as in SetSubscriptionsNotRequired: silently initializing a nil ws.setup with &ConnectionSetup{} could mask an invalid state. Both methods should handle this consistently — consider extracting the nil-guard into a shared helper or removing it in favor of treating a nil setup as an error.
There was a problem hiding this comment.
Jarvis: Same reasoning here — I’m keeping the guard. The nil setup case should not occur in normal multi-connection setup flow, and the defensive check keeps the two helper methods consistent without changing behaviour for valid managers.
| // Exchanges which don't support subscription conf; Can be removed when all exchanges support sub conf | ||
| b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil } |
There was a problem hiding this comment.
In MockWsInstance, GenerateSubs is still explicitly overridden on line 126, even though SetSubscriptionsNotRequired() on line 127 already sets GenerateSubs to return an empty list for single-connection managers. This makes the two lines redundant for the single-connection path. Consider removing the explicit GenerateSubs override or adding a comment explaining why both are needed (e.g., if SetSubscriptionsNotRequired doesn't override GenerateSubs for multi-connection managers and the old override is kept as a safety net for the single-connection case).
| // Exchanges which don't support subscription conf; Can be removed when all exchanges support sub conf | |
| b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil } | |
| // Ensure no default subscriptions are required for tests, including exchanges without subscription configuration support. |
There was a problem hiding this comment.
Jarvis: Done in thrasher-@669594f — removed the redundant GenerateSubs override so MockWsInstance now relies on SetSubscriptionsNotRequired() as the single source of that behaviour.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of supporting multi-connection websocket helpers for testing. The introduction of SetSubscriptionsNotRequired and SetAllConnectionURLs in the websocket manager provides a clean way to configure test environments, and the updates to MockWsInstance and SetupWs correctly leverage these new helpers. The testing strategy is thorough, with new unit tests, regression tests for multi-connection exchanges, and a proof-of-concept test for Poloniex. I have one minor suggestion to improve code clarity in a test helper, as detailed in the specific comment.
| b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil } | ||
| b.Websocket.SetSubscriptionsNotRequired() |
There was a problem hiding this comment.
The call to b.Websocket.SetSubscriptionsNotRequired() on line 127 makes the assignment on line 126 redundant. The SetSubscriptionsNotRequired function already handles setting GenerateSubs for single-connection managers. To improve maintainability and avoid confusion, you can remove this redundant line. The preceding comment on line 125 also appears to be obsolete with this change.
| b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil } | |
| b.Websocket.SetSubscriptionsNotRequired() | |
| b.Websocket.SetSubscriptionsNotRequired() |
There was a problem hiding this comment.
Jarvis: Done in thrasher-@669594f — removed the redundant GenerateSubs assignment and the now-obsolete extra comment so this helper only uses SetSubscriptionsNotRequired() for that behaviour.
PR Description
Fix internal testing websocket helpers so they work with multi-connection managers.
This addresses issue #2151 by updating the shared websocket test helpers instead of relying on per-exchange workarounds.
What changed
SetSubscriptionsNotRequired()support for multi-connection managersSetAllConnectionURLs()so test helpers can redirect all managed websocket connections to a single mock serverinternal/testing/exchange.SetupWsto use the multi-connection-safe helper pathinternal/testing/exchange.MockWsInstanceto use the multi-connection-safe helper pathMockWsInstancewith a real multi-connection exchangeSetupWswith a synthetic multi-connection setupWhy
SetupWsandMockWsInstancepreviously assumed single-connection websocket behaviour. That broke down once exchanges usedUseMultiConnectionManagement, because empty generated subscriptions prevented connections from being established and test helpers had no clean way to mark subscriptions as not required.This change keeps the fix in shared test infrastructure so exchanges do not need ad-hoc helper logic.
Type of change
How has this been tested
go test ./exchange/websocket -count=1go test ./internal/testing/exchange -count=1 -vgo test ./exchanges/bybit -count=1go test ./exchanges/poloniex -count=1golangci-lint run ./exchange/websocket ./internal/testing/exchange ./exchanges/poloniexcodespellgofumptCloses #2151