Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+61 −3
Open

exchanges/margin: Fix marshalling issue #1812

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f653a3
Fixes issue with marshalling margin types
gloriousCode Feb 24, 2025
0f2b490
Update exchanges/order/order_test.go
gloriousCode Mar 3, 2025
0bee885
Update exchanges/margin/margin.go
gloriousCode Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions exchanges/margin/margin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ func (t *Type) UnmarshalJSON(d []byte) error {
return err
}

// MarshalJSON conforms type to the Marshaler interface
func (t Type) MarshalJSON() ([]byte, error) {
return json.Marshal(t.String())
}

// String returns the string representation of the margin type in lowercase
// the absence of a lower func should hopefully highlight that String is lower
func (t Type) String() string {
Expand All @@ -37,10 +42,9 @@ func (t Type) String() string {
return spotIsolatedStr
case NoMargin:
return cashStr
case Unknown:
default:
return unknownStr
}
return ""
}

// Upper returns the upper case string representation of the margin type
Expand Down
27 changes: 26 additions & 1 deletion exchanges/margin/margin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package margin

import (
"fmt"
"strings"
"testing"

Expand Down Expand Up @@ -33,6 +34,7 @@ func TestUnmarshalJSON(t *testing.T) {
"spotIsolated": {`{"margin":"spot_isolated"}`, SpotIsolated, nil},
"invalid": {`{"margin":"hello moto"}`, Unknown, ErrInvalidMarginType},
"unset": {`{"margin":""}`, Unset, nil},
"": {`{"margin":""}`, Unset, nil},
} {
t.Run(name, func(t *testing.T) {
t.Parallel()
Expand All @@ -46,6 +48,28 @@ func TestUnmarshalJSON(t *testing.T) {
}
}

func TestMarshalJSON(t *testing.T) {
t.Parallel()
for _, tc := range []struct {
in Type
want string
}{
{Isolated, fmt.Sprintf(`%q`, isolatedStr)},
{Multi, fmt.Sprintf(`%q`, multiStr)},
{NoMargin, fmt.Sprintf(`%q`, cashStr)},
{SpotIsolated, fmt.Sprintf(`%q`, spotIsolatedStr)},
{Type(uint8(123)), fmt.Sprintf(`%q`, unknownStr)},
{Unset, fmt.Sprintf(`%q`, unsetStr)},
} {
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))
})
Comment on lines +64 to +69
Copy link
Collaborator

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.

Copy link
Collaborator Author

@gloriousCode gloriousCode Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be changing test design to suit dislike of the kind of verbose output when verbose and because of increased scrolling. The issue is much closer to "golang outputs subtests in an ugly manner" than "this test design has negative impacts". I understand your desire and you can and do design your tests without it. I do utilise the sub test functionality, its especially handy when one fails, to debug into just one of them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll yield. I'll just stop using verbose as much as I do currently.
BTW, you noted already I assumed that there's obviously a performance hit for using t.Run, which you're accepting as "worth it".
It's only 0.05-0.1s for me, though, and pales in comparison to our tests in general.

}
}

func TestString(t *testing.T) {
t.Parallel()
assert.Equal(t, unknownStr, Unknown.String())
Expand All @@ -54,7 +78,8 @@ func TestString(t *testing.T) {
assert.Equal(t, unsetStr, Unset.String())
assert.Equal(t, spotIsolatedStr, SpotIsolated.String())
assert.Equal(t, cashStr, NoMargin.String())
assert.Equal(t, "", Type(30).String())
assert.Equal(t, unknownStr, Type(30).String())
assert.Equal(t, "", Unset.String())
}

func TestUpper(t *testing.T) {
Expand Down
29 changes: 29 additions & 0 deletions exchanges/order/order_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/currency"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
"github.com/thrasher-corp/gocryptotrader/exchanges/margin"
"github.com/thrasher-corp/gocryptotrader/exchanges/protocol"
"github.com/thrasher-corp/gocryptotrader/exchanges/validate"
)
Expand Down Expand Up @@ -2192,3 +2193,31 @@ func TestTrackingModeString(t *testing.T) {
require.Equal(t, v, k.String())
}
}

func TestUnmarshalOrder(t *testing.T) {
t.Parallel()
btx := currency.NewBTCUSDT()
btx.Delimiter = "-"
orderSubmit := Submit{
Exchange: "test",
Pair: btx,
AssetType: asset.Spot,
Side: Buy,
Type: Market,
Amount: 1,
Price: 1000,
}
jOrderSubmit, err := json.Marshal(orderSubmit)
require.NoError(t, err)
var os2 Submit
err = json.Unmarshal(jOrderSubmit, &os2)
require.NoError(t, err)
require.Equal(t, orderSubmit, os2)
// 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)
Comment on lines +2216 to +2222
Copy link
Collaborator

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?
Suggested change
// 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")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know what you want me to do with this comment sorry. Do you want me to remove the whole test because its resolved in the margin package?

}
Loading