exchanges/gateio: Add websocket order batch [SPOT] support with context mocking #1778
Conversation
…larity on purpose. Change connections map to point to candidate to track subscriptions for future dynamic connections holder and drop struct ConnectionDetails.
…rror but websocket frame error or anything really makes the reader routine return and then connection never cycles and the buffer gets filled. * Handle reconnection via an errors.Is check which is simpler and in that scope allow for quick disconnect reconnect without waiting for connection cycle. * Dial now uses code from DialContext but just calls context.Background() * Don't allow reader to return on parse binary response error. Just output error and return a non nil response
…would hang connection reader for eternity.
… item to nil for systems that do not require rate limiting; add glorious nit
Co-authored-by: Scott <[email protected]>
shazbert
added
the
nomerge requires dependency
shazbert
changed the title
shazbert
force-pushed
the
ws_batch_spot
branch
from
c5b9558 to
068bec7
Compare
shazbert
force-pushed
the
ws_batch_spot
branch
from
068bec7 to
b65fc6b
Compare
| var mockResponseFlag = struct{ name string }{name: "mockResponse"} | ||
|
|
||
| // IsMockResponse returns true if the request has a mock response set | ||
| func IsMockResponse(ctx context.Context) bool { |
There was a problem hiding this comment.
| func IsMockResponse(ctx context.Context) bool { | |
| func HasMockResponse(ctx context.Context) bool { |
| func getRESTResponseFromMock(ctx context.Context) *http.Response { | ||
| mockResp := GetMockResponse(ctx) | ||
| if len(mockResp) != 1 { | ||
| panic("mock REST response invalid, requires exactly one response") |
There was a problem hiding this comment.
Let's not panic. This can be called from anywhere given WithMockResponse is exported. The function where this is used already has error handling after this function is called
| @@ -1274,6 +1275,10 @@ func (w *Websocket) GetConnection(messageFilter any) (Connection, error) { | |||
| return nil, errMessageFilterNotSet | |||
| } | |||
|
|
|||
| if request.IsMockResponse(ctx) { | |||
| return newMockWebsocketConnection(), nil | |||
There was a problem hiding this comment.
This placement does not appear the most appropriate. GetConnection creating a whole new one while existing functionality does not?
Why not have a new function instead of this? Reduces the need for inspecting context on every call as well. Even if you don't make a new function, I think it should be removed from this one
I don't see any benefit to this over just assigning a websocket with stream.MockWebsocketConnection inside test code
edit: your pr description makes me think you're wanting to hot-swap test code at times maybe? If so I'll mull over how to improve this
| } | ||
| return &http.Response{ | ||
| StatusCode: http.StatusOK, | ||
| Body: io.NopCloser(io.Reader(io.LimitReader(bytes.NewBuffer(mockResp[0]), drainBodyLimit))), |
There was a problem hiding this comment.
| Body: io.NopCloser(io.Reader(io.LimitReader(bytes.NewBuffer(mockResp[0]), drainBodyLimit))), | |
| Body: io.NopCloser(io.LimitReader(bytes.NewBuffer(mockResp[0]), drainBodyLimit)), |
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestMockResponse(t *testing.T) { |
There was a problem hiding this comment.
I would prefer testing to be done at a per function level. Perhaps these ones are exceptional and I'll allow it, but I'm just not a fan of OneBigTest style testing
| require.Nil(t, resp) | ||
| singleResp, err := got.SendMessageReturnResponse(request.WithMockResponse(context.Background(), []byte("test")), 0, nil, nil) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, singleResp) | ||
| resp, err = got.SendMessageReturnResponsesWithInspector(request.WithMockResponse(context.Background(), []byte("test")), 0, nil, nil, 0, nil) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, resp) |
There was a problem hiding this comment.
I think the functionality is important enough to actually check that the result is what you expect. As well as including multi-byte-slice scenario too
PR Description
WebsocketSubmitBatchOrdersWebsocketSubmitBatchOrdersfor spot onlyFutures not done just to keep this as small as possible and I haven't done any trading batch wise to verify it yet.
Requires #1603
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist