-
Notifications
You must be signed in to change notification settings - Fork 829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exchanges/margin: Fix marshalling issue #1812
base: master
Are you sure you want to change the base?
exchanges/margin: Fix marshalling issue #1812
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK. Was wondering about the delimiter and maybe we should add in one in a marshaller for Submit if missing between, but out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, nice work 👍
I've raised nits, but none of them stop this being merged as-is.
@@ -23,6 +23,11 @@ func (t *Type) UnmarshalJSON(d []byte) error { | |||
return err | |||
} | |||
|
|||
// MarshalJSON conforms type to the Marshaler interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
// MarshalJSON conforms type to the Marshaler interface | |
// MarshalJSON conforms type to the json.Marshaler interface |
I'd also argue for implements
over conforms
, but that's getting picky.
t.Run(tc.want, func(t *testing.T) { | ||
t.Parallel() | ||
resp, err := json.Marshal(tc.in) | ||
require.NoError(t, err) | ||
assert.Equal(t, tc.want, string(resp)) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinionated Nit:
As someone who runs tests verbose often, I really don't like having t.Run
for trivial cases like this, over just adding tc.want to the end of the 2 checks, because of the increased output if we're always doing this.
I feel like we want t.Run
for tests which are going to take a lot of time, or where we might want to -run
them individually.
The reason I raise this (again) is because if we keep adding it for ubiqutous methods like TestMarshal or TestString then we'll get a lot of this:
=== RUN TestMarshalJSON
=== PAUSE TestMarshalJSON
=== CONT TestMarshalJSON
=== RUN TestMarshalJSON/"isolated"
=== PAUSE TestMarshalJSON/"isolated"
=== RUN TestMarshalJSON/"multi"
=== PAUSE TestMarshalJSON/"multi"
=== RUN TestMarshalJSON/"cash"
=== PAUSE TestMarshalJSON/"cash"
=== RUN TestMarshalJSON/"spot_isolated"
=== PAUSE TestMarshalJSON/"spot_isolated"
=== RUN TestMarshalJSON/"unknown"
=== PAUSE TestMarshalJSON/"unknown"
=== RUN TestMarshalJSON/""
=== PAUSE TestMarshalJSON/""
=== CONT TestMarshalJSON/"isolated"
=== CONT TestMarshalJSON/"cash"
=== CONT TestMarshalJSON/"multi"
=== CONT TestMarshalJSON/"spot_isolated"
=== CONT TestMarshalJSON/""
=== CONT TestMarshalJSON/"unknown"
--- PASS: TestMarshalJSON (0.00s)
--- PASS: TestMarshalJSON/"isolated" (0.00s)
--- PASS: TestMarshalJSON/"cash" (0.00s)
--- PASS: TestMarshalJSON/"multi" (0.00s)
--- PASS: TestMarshalJSON/"spot_isolated" (0.00s)
--- PASS: TestMarshalJSON/"" (0.00s)
--- PASS: TestMarshalJSON/"unknown" (0.00s)
PASS
ok github.com/thrasher-corp/gocryptotrader/exchanges/margin 0.519s
And it'll overwhelm scrollback and make grepping the test output harder.
Price: 1000, | ||
} | ||
jOrderSubmit, err := json.Marshal(orderSubmit) | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.NoError(t, err) | |
require.NoError(t, err, "json.Marshal must not error") |
// Margin-type regression test | ||
orderSubmit.MarginType = margin.Multi | ||
jOrderSubmit, err = json.Marshal(orderSubmit) | ||
require.NoError(t, err) | ||
err = json.Unmarshal(jOrderSubmit, &os2) | ||
require.NoError(t, err) | ||
require.Equal(t, orderSubmit, os2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits:
- Think the regression can be moved into the reason we do the test
- But actually, I don't think we should have this regression test here (in isolation of any other field tests); We didn't fix anything in order.Submit at all, so the regression wouldn't be here.
- Also: Am I missing why we couldn't have just set MarginType in the original orderSumbit and let L2215 handle this?
// Margin-type regression test | |
orderSubmit.MarginType = margin.Multi | |
jOrderSubmit, err = json.Marshal(orderSubmit) | |
require.NoError(t, err) | |
err = json.Unmarshal(jOrderSubmit, &os2) | |
require.NoError(t, err) | |
require.Equal(t, orderSubmit, os2) | |
orderSubmit.MarginType = margin.Multi | |
jOrderSubmit, err = json.Marshal(orderSubmit) | |
require.NoError(t, err, "json.Marshal must not error") | |
err = json.Unmarshal(jOrderSubmit, &os2) | |
require.NoError(t, err) | |
require.Equal(t, orderSubmit, os2, "order.Submit must unmarshal Margin.Type correctly") |
PR Description
Addresses issue #1810
There is a bug from a lack of
MarshalJSON
implementation. When unmarshalling and marshalling an order.Submit for example, it will fail from being unable to parse a string into amargin.Type
Type of change
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