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.

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

Huobi: Fix false failure on NW date #1809

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

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Feb 22, 2025

Looks like "Within 2 weeks" fails on 22nd November during leap years. On 2025-02-21 we saw NW come up with 2025-03-07 for a few hours, and I'm guessing it's because they use local time. Added 1 day leeway to account for that timezone difference.

Type of change

  • Test fix (non-breaking change which fixes an issue)

Looks like "Within 2 weeks" fails on 22nd November during leap years.
On 2025-02-21 we saw NW come up with 2025-03-07 for a few hours,
and I'm guessing it's because they use local time.
Added 1 day leeway to account for that timezone difference.
@gbjk gbjk added review me This pull request is ready for review test(s) fix This is to denote the PR is fixing a build test issue labels Feb 22, 2025
@gbjk gbjk requested a review from thrasher- February 22, 2025 03:38
@gbjk gbjk self-assigned this Feb 22, 2025
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

question/suggestion: I don't know times very well (so this is probably completely wrong) but shouldn't we load the expiration as cst then use add date because it accounts for leaps? We might need a mock for this as well.

diff --git a/exchanges/huobi/huobi_test.go b/exchanges/huobi/huobi_test.go
index d3053550c..af7c2bf33 100644
--- a/exchanges/huobi/huobi_test.go
+++ b/exchanges/huobi/huobi_test.go
@@ -1811,14 +1811,16 @@ func TestPairFromContractExpiryCode(t *testing.T) {
                require.True(t, ok, "%s type must be in contractExpiryNames", cType)
                assert.Equal(t, currency.BTC, p.Base, "pair Base should be the same")
                assert.Equal(t, exp, p.Quote, "pair Quote should be the same")
-               d, err := time.Parse("060102", p.Quote.String())
+               cst, err := time.LoadLocation("Asia/Shanghai")
+               require.NoError(t, err)
+               d, err := time.ParseInLocation("060102", p.Quote.String(), cst)
                require.NoError(t, err, "currency code must be a parsable date")
                require.Falsef(t, d.Before(n), "%s expiry must be today or after", cType)
                switch cType {
                case "CW", "NW":
-                       require.Truef(t, d.Before(n.Add(24*time.Hour*15)), "%s expiry must be within 15 days; Got: `%s`", cType, d)
+                       require.Truef(t, d.Before(n.AddDate(0, 0, 14)), "%s expiry must be within 15 days; Got: `%s`", cType, d)
                case "CQ", "NQ":
-                       require.Truef(t, d.Before(n.Add(24*time.Hour*181)), "%s expiry must be within 181 days; Got: `%s`", cType, d)
+                       require.Truef(t, d.Before(n.AddDate(0, 3, 0)), "%s expiry must be within 181 days; Got: `%s`", cType, d)
                }
        }
 }

@gbjk
Copy link
Collaborator Author

gbjk commented Mar 5, 2025

question/suggestion: I don't know times very well (so this is probably completely wrong) but shouldn't we load the expiration as cst then use add date because it accounts for leaps? We might need a mock for this as well.

I mean ... technically they're based in the Seychelles 😂
But actually I'm pretty sure their main base is Singapore now.

Yeah. I like your solution.
Hard pass to mocking this, though 😛
If it fails again, we'll re-evaluate. RoI on mocking is super low.

Fixed gbjk@ef5e85856b

@gbjk gbjk requested a review from shazbert March 5, 2025 07:37
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Changes ACK. one suggestion.

Comment on lines 1814 to 1816
cst, err := time.LoadLocation("Asia/Singapore") // Huobi HQ and apparent local time for when codes become effective
require.NoError(t, err, "LoadLocation must not error")
d, err := time.ParseInLocation("060102", p.Quote.String(), cst)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Going from this

Suggested change
cst, err := time.LoadLocation("Asia/Singapore") // Huobi HQ and apparent local time for when codes become effective
require.NoError(t, err, "LoadLocation must not error")
d, err := time.ParseInLocation("060102", p.Quote.String(), cst)
sgt, err := time.LoadLocation("Asia/Singapore") // Huobi HQ and apparent local time for when codes become effective
require.NoError(t, err, "LoadLocation must not error")
d, err := time.ParseInLocation("060102", p.Quote.String(), sgt)

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 think when we see a mistake happened because of unnecessary naming, best not to double down on it.

Fixed gbjk@0f841230be

@shazbert shazbert added the szrc shazbert review complete label Mar 10, 2025
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me This pull request is ready for review szrc shazbert review complete test(s) fix This is to denote the PR is fixing a build test issue
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants