GHA: Add additional checks for common issues (#1922)

* GHA, tests: Add additional checks for common issues

These checks include:
- Ensuring that all testify funcs use their formatted variants (e.g., `assert.Equalf(t, expected, actual)` instead of `assert.Equal(t, expected, actual)`).
- Replacing `%s` with %q
- Enforcing consistent usage of should/must wording for testify assert/require messages

* Add support for checking backticked string format specifiers and fix issues

* tests: Fix error comparisons

* tests: Replace errors.Is(err, nil) usage with testify and automate check

* refactor: Rename ExtractPort to ExtractPortOrDefault

* tests: Replace assert with require for error handling in multiple test files

* tests: Replace assert with require for error handling and improve assertions in data tests

* tests: Fix typo in assertion message for StreamVol test

* OKX: Fix GetOpenInterestAndVolumeStrike test with instrument selection and improved assertions

* OKX: Revert intentional error check

* Improve error message for expiry time check in GetOpenInterestAndVolumeStrike test
This commit is contained in:
Adrian Gallagher
2025-05-28 12:26:51 +10:00
committed by GitHub
parent 1e5739dffa
commit a5b638bfb7
165 changed files with 2565 additions and 4626 deletions

View File

@@ -212,8 +212,8 @@ func TestGetPlatformStatus(t *testing.T) {
func TestGetTickerBatch(t *testing.T) {
t.Parallel()
ticks, err := b.GetTickerBatch(t.Context())
require.NoError(t, err, "GetTickerBatch should not error")
require.NotEmpty(t, ticks, "GetTickerBatch should return some ticks")
require.NoError(t, err, "GetTickerBatch must not error")
require.NotEmpty(t, ticks, "GetTickerBatch must return some ticks")
require.Contains(t, ticks, "tBTCUSD", "Ticker batch must contain tBTCUSD")
checkTradeTick(t, ticks["tBTCUSD"])
require.Contains(t, ticks, "fUSD", "Ticker batch must contain fUSD")
@@ -223,7 +223,7 @@ func TestGetTickerBatch(t *testing.T) {
func TestGetTicker(t *testing.T) {
t.Parallel()
tick, err := b.GetTicker(t.Context(), "tBTCUSD")
require.NoError(t, err, "GetTicker should not error")
require.NoError(t, err, "GetTicker must not error")
checkTradeTick(t, tick)
}
@@ -239,7 +239,7 @@ func TestTickerFromResp(t *testing.T) {
assert.ErrorContains(t, err, "tBTCUSD", "tickerFromResp should error correctly")
tick, err := tickerFromResp("tBTCUSD", []any{1.1, 2.2, 3.3, 4.4, 5.5, 6.6, 7.7, 8.8, 9.9, 10.10})
require.NoError(t, err, "tickerFromResp should error correctly")
require.NoError(t, err, "tickerFromResp must error correctly")
assert.Equal(t, 1.1, tick.Bid, "Tick Bid should be correct")
assert.Equal(t, 2.2, tick.BidSize, "Tick BidSize should be correct")
assert.Equal(t, 3.3, tick.Ask, "Tick Ask should be correct")
@@ -268,7 +268,7 @@ func TestTickerFromFundingResp(t *testing.T) {
assert.ErrorContains(t, err, "fBTC", "tickerFromFundingResp should error correctly")
tick, err := tickerFromFundingResp("fBTC", []any{1.1, 2.2, 3.0, 4.4, 5.5, 6.0, 7.7, 8.8, 9.9, 10.10, 11.11, 12.12, 13.13, nil, nil, 15.15})
require.NoError(t, err, "tickerFromFundingResp should error correctly")
require.NoError(t, err, "tickerFromFundingResp must error correctly")
assert.Equal(t, 1.1, tick.FlashReturnRate, "Tick FlashReturnRate should be correct")
assert.Equal(t, 2.2, tick.Bid, "Tick Bid should be correct")
assert.Equal(t, int64(3), tick.BidPeriod, "Tick BidPeriod should be correct")
@@ -288,7 +288,7 @@ func TestTickerFromFundingResp(t *testing.T) {
func TestGetTickerFunding(t *testing.T) {
t.Parallel()
tick, err := b.GetTicker(t.Context(), "fUSD")
require.NoError(t, err, "GetTicker should not error")
require.NoError(t, err, "GetTicker must not error")
checkFundingTick(t, tick)
}
@@ -537,13 +537,13 @@ func TestUpdateTickers(t *testing.T) {
for _, a := range b.GetAssetTypes(true) {
avail, err := b.GetAvailablePairs(a)
require.NoError(t, err, "GetAvailablePairs should not error")
require.NoError(t, err, "GetAvailablePairs must not error")
err = b.CurrencyPairs.StorePairs(a, avail, true)
require.NoError(t, err, "StorePairs should not error")
require.NoError(t, err, "StorePairs must not error")
err = b.UpdateTickers(t.Context(), a)
require.NoError(t, common.ExcludeError(err, ticker.ErrBidEqualsAsk), "UpdateTickers may only error about locked markets")
require.NoError(t, common.ExcludeError(err, ticker.ErrBidEqualsAsk), "UpdateTickers must only error about locked markets")
// Bitfinex leaves delisted pairs in Available info/conf endpoints
// We want to assert that most pairs are valid, so we'll check that no more than 5% are erroring
@@ -557,7 +557,7 @@ func TestUpdateTickers(t *testing.T) {
okay++
}
}
if !assert.Greater(t, okay/float64(len(avail))*100.0, acceptableThreshold, "At least %.f%% of %s tickers should not error", acceptableThreshold, a) {
if !assert.Greaterf(t, okay/float64(len(avail))*100.0, acceptableThreshold, "At least %.f%% of %s tickers should not error", acceptableThreshold, a) {
assert.NoError(t, errs, "Collection of all the ticker errors")
}
}
@@ -1108,7 +1108,7 @@ func TestWSAuth(t *testing.T) {
t.Skip("Authentecated API support not enabled")
}
testexch.SetupWs(t, b)
require.True(t, b.Websocket.CanUseAuthenticatedEndpoints(), "CanUseAuthenticatedEndpoints should be turned on")
require.True(t, b.Websocket.CanUseAuthenticatedEndpoints(), "CanUseAuthenticatedEndpoints must be turned on")
var resp map[string]any
catcher := func() (ok bool) {
@@ -1135,7 +1135,7 @@ func TestGenerateSubscriptions(t *testing.T) {
b.Websocket.SetCanUseAuthenticatedEndpoints(true)
require.True(t, b.Websocket.CanUseAuthenticatedEndpoints(), "CanUseAuthenticatedEndpoints must return true")
subs, err := b.generateSubscriptions()
require.NoError(t, err, "generateSubscriptions should not error")
require.NoError(t, err, "generateSubscriptions must not error")
exp := subscription.List{}
for _, baseSub := range b.Features.Subscriptions {
for _, a := range b.GetAssetTypes(true) {
@@ -1180,7 +1180,7 @@ func TestWSSubscribe(t *testing.T) {
require.NoError(t, testexch.Setup(b), "TestInstance must not error")
testexch.SetupWs(t, b)
err := b.Subscribe(subscription.List{{Channel: subscription.TickerChannel, Pairs: currency.Pairs{currency.NewBTCUSD()}, Asset: asset.Spot}})
require.NoError(t, err, "Subrcribe should not error")
require.NoError(t, err, "Subrcribe must not error")
catcher := func() (ok bool) {
i := <-b.Websocket.ToRoutine
_, ok = i.(*ticker.Price)
@@ -1189,11 +1189,11 @@ func TestWSSubscribe(t *testing.T) {
assert.Eventually(t, catcher, sharedtestvalues.WebsocketResponseDefaultTimeout, time.Millisecond*10, "Ticker response should arrive")
subs, err := b.GetSubscriptions()
require.NoError(t, err, "GetSubscriptions should not error")
require.Len(t, subs, 1, "We should only have 1 subscription; subID subscription should have been Removed by subscribeToChan")
require.NoError(t, err, "GetSubscriptions must not error")
require.Len(t, subs, 1, "We must only have 1 subscription; subID subscription must have been Removed by subscribeToChan")
err = b.Subscribe(subscription.List{{Channel: subscription.TickerChannel, Pairs: currency.Pairs{currency.NewBTCUSD()}, Asset: asset.Spot}})
require.ErrorContains(t, err, "subscribe: dup (code: 10301)", "Duplicate subscription should error correctly")
require.ErrorContains(t, err, "subscribe: dup (code: 10301)", "Duplicate subscription must error correctly")
assert.EventuallyWithT(t, func(t *assert.CollectT) {
i := <-b.Websocket.ToRoutine
@@ -1203,8 +1203,8 @@ func TestWSSubscribe(t *testing.T) {
}, sharedtestvalues.WebsocketResponseDefaultTimeout, time.Millisecond*10, "error response should go to ToRoutine")
subs, err = b.GetSubscriptions()
require.NoError(t, err, "GetSubscriptions should not error")
require.Len(t, subs, 1, "We should only have one subscription after an error attempt")
require.NoError(t, err, "GetSubscriptions must not error")
require.Len(t, subs, 1, "We must only have one subscription after an error attempt")
err = b.Unsubscribe(subs)
assert.NoError(t, err, "Unsubscribing should not error")
@@ -1702,9 +1702,7 @@ func TestFixCasing(t *testing.T) {
}
_, err = b.fixCasing(currency.NewPair(currency.BTC, currency.EMPTYCODE), asset.MarginFunding)
if !errors.Is(err, nil) {
t.Fatalf("received: '%v' but expected: '%v'", err, nil)
}
require.NoError(t, err)
_, err = b.fixCasing(currency.EMPTYPAIR, asset.MarginFunding)
if !errors.Is(err, currency.ErrCurrencyPairEmpty) {
@@ -1938,9 +1936,7 @@ func TestGetSiteListConfigData(t *testing.T) {
}
pairs, err := b.GetSiteListConfigData(t.Context(), bitfinexSecuritiesPairs)
if !errors.Is(err, nil) {
t.Fatalf("received: %v, expected: %v", err, nil)
}
require.NoError(t, err)
if len(pairs) == 0 {
t.Fatal("expected pairs")
@@ -1951,9 +1947,8 @@ func TestGetSiteInfoConfigData(t *testing.T) {
t.Parallel()
for _, assetType := range []asset.Item{asset.Spot, asset.Futures} {
pairs, err := b.GetSiteInfoConfigData(t.Context(), assetType)
if !errors.Is(err, nil) {
t.Errorf("Error from GetSiteInfoConfigData for %s type received: %v, expected: %v", assetType, err, nil)
}
assert.NoError(t, err)
if len(pairs) == 0 {
t.Errorf("GetSiteInfoConfigData returned no pairs for %s", assetType)
}
@@ -2030,8 +2025,8 @@ func TestGetCurrencyTradeURL(t *testing.T) {
testexch.UpdatePairsOnce(t, b)
for _, a := range b.GetAssetTypes(false) {
pairs, err := b.CurrencyPairs.GetPairs(a, false)
require.NoError(t, err, "cannot get pairs for %s", a)
require.NotEmpty(t, pairs, "no pairs for %s", a)
require.NoErrorf(t, err, "cannot get pairs for %s", a)
require.NotEmptyf(t, pairs, "no pairs for %s", a)
resp, err := b.GetCurrencyTradeURL(t.Context(), a, pairs[0])
require.NoError(t, err)
assert.NotEmpty(t, resp)

View File

@@ -962,7 +962,7 @@ func (b *Bitfinex) handleWSAllTrades(s *subscription.Subscription, respRaw []byt
return fmt.Errorf("%w `tradesSnapshot`: %w", common.ErrParsingWSField, err)
}
default:
return fmt.Errorf("%w `tradesUpdate[1]`: %w `%s`", common.ErrParsingWSField, jsonparser.UnknownValueTypeError, valueType)
return fmt.Errorf("%w `tradesUpdate[1]`: %w %q", common.ErrParsingWSField, jsonparser.UnknownValueTypeError, valueType)
}
trades := make([]trade.Data, len(wsTrades))
for _, w := range wsTrades {

View File

@@ -51,13 +51,13 @@ func (b *Bitfinex) SetDefaults() {
ps.ConfigFormat.Delimiter = ":"
}
if err := b.SetAssetPairStore(a, ps); err != nil {
log.Errorf(log.ExchangeSys, "%s error storing `%s` default asset formats: %s", b.Name, a, err)
log.Errorf(log.ExchangeSys, "%s error storing %q default asset formats: %s", b.Name, a, err)
}
}
// Margin WS Currently not fully implemented and causes subscription collisions with spot
if err := b.DisableAssetWebsocketSupport(asset.Margin); err != nil {
log.Errorf(log.ExchangeSys, "%s error disabling `%s` asset type websocket support: %s", b.Name, asset.Margin, err)
log.Errorf(log.ExchangeSys, "%s error disabling %q asset type websocket support: %s", b.Name, asset.Margin, err)
}
// TODO: Implement Futures and Securities asset types.
@@ -703,9 +703,7 @@ func (b *Bitfinex) parseOrderToOrderDetail(o *Order) (*order.Detail, error) {
var timestamp float64
timestamp, err = strconv.ParseFloat(o.Timestamp, 64)
if err != nil {
log.Warnf(log.ExchangeSys,
"%s Unable to convert timestamp '%s', leaving blank",
b.Name, o.Timestamp)
log.Warnf(log.ExchangeSys, "%s Unable to convert timestamp %q, leaving blank", b.Name, o.Timestamp)
}
var pair currency.Pair