Tests: Various race fixes and move TestFixtureToDataHandler #1534

gbjk · 2024-05-05T03:30:18Z

Fix a instance pair manager race intermittently causing Bitfinex and Kraken to fail
Fix PairsManager.Load breaking matcher
Fix UpdatePairsOnce not working across instances
Move and Simplify FixtureToDataHandler
Rename PairStore.copy to PairStore.clone and replace various builtin copy with slices.Clone
Add Fullstore.clone and PairFormat.clone

Type of change

Test fixes and improvements

How has this been tested

go test ./... -race
golangci-lint run

shazbert

Looks good to me, only some pedantic things, that you can resolve at your choosing. Nice.

currency/manager.go

currency/pair.go

currency/manager.go

codecov · 2024-05-06T01:30:11Z

Codecov Report

Attention: Patch coverage is 74.79675% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 37.87%. Comparing base (34ef09d) to head (93fee81).

❗ Current head 93fee81 differs from pull request most recent head 0708926. Consider uploading reports for the commit 0708926 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1534      +/-   ##
==========================================
+ Coverage   35.89%   37.87%   +1.97%     
==========================================
  Files         409      412       +3     
  Lines      177453   147950   -29503     
==========================================
- Hits        63699    56031    -7668     
+ Misses     105899    84057   -21842     
- Partials     7855     7862       +7

Files	Coverage Δ
currency/pair.go	`86.90% <100.00%> (+2.12%)`	⬆️
exchanges/okx/okx_websocket.go	`35.69% <100.00%> (+3.18%)`	⬆️
exchanges/sharedtestvalues/sharedtestvalues.go	`70.23% <100.00%> (-1.74%)`	⬇️
exchanges/exchange.go	`79.74% <80.00%> (+3.52%)`	⬆️
exchanges/gateio/gateio.go	`16.34% <60.00%> (+2.15%)`	⬆️
exchanges/okx/okx.go	`22.53% <60.00%> (+2.38%)`	⬆️
currency/manager.go	`94.96% <95.58%> (+1.53%)`	⬆️
exchanges/okx/okx_wrapper.go	`33.29% <42.85%> (+2.58%)`	⬆️
internal/testing/exchange/exchange.go	`47.32% <0.00%> (-3.49%)`	⬇️

... and 382 files with indirect coverage changes

shazbert

Just some 🏃 racey features left over here

gbjk · 2024-05-06T04:56:43Z

Fixed with 2e9e7d6 and 1481244
In both cases I've shied away from going and changing everything to remove the (very) thin wrappers around CurrencyManager methods
Also haven't gone hunting for other examples of intrusion (yet)

shazbert

Good stuff, thanks for addressing my change requests. There's a couple of error on the CI build

currency/manager.go

exchanges/gateio/gateio.go

gbjk · 2024-05-07T12:28:21Z

@shazbert Shout when you're done with this (and/or resolve threads).
I'll rebase it on master and resolve the conflict before @gloriousCode starts his review.

shazbert

Changes look good 👍 can proceed with rebase.

currency/manager_test.go

gbjk · 2024-05-08T02:10:42Z

@shazbert Following discussion with @thrasher- I've switched it back to using a new PairsManager.IsAssetSupported.

Fixed 93fee81

Everything else was a rebase of what you've already seen.

shazbert

Thanks for the changes. tACK! 🚀

shazbert

Changes ACK.

gloriousCode

tACK!

gloriousCode · 2024-05-16T01:10:39Z

currency/manager.go

 	if ps == nil {
-		return nil, errPairStoreIsNil
+		return nil


The removal of the error here and making returning nil an acceptable situation is something that could lead to panics as not much code is prepared for that.

At present it looks like only a botched test via Load will lead to this which is acceptable and addressable by the offending user, but I still need to point out because 🦕

Understood.

Calling StorePairs on global instance can lead to race

This should delegate entirely to PairManager's IsAssetSupported

TestGetCurrencyTradeURL would fail sometimes due to sequencing of enabling futures but not having pairs for it.

thrasher-

tACK, nice work! NOTE: BTSE issues are being worked on in a separate branch.

gbjk added the test(s) fix This is to denote the PR is fixing a build test issue label May 5, 2024

gbjk self-assigned this May 5, 2024

gbjk requested review from gloriousCode, thrasher- and shazbert May 5, 2024 03:33

gbjk added the review me This pull request is ready for review label May 5, 2024

shazbert requested changes May 6, 2024

View reviewed changes

currency/manager.go Show resolved Hide resolved

currency/manager.go Outdated Show resolved Hide resolved

currency/pair.go Outdated Show resolved Hide resolved

currency/manager.go Show resolved Hide resolved

currency/manager.go Outdated Show resolved Hide resolved

gbjk requested a review from shazbert May 6, 2024 01:52

shazbert requested changes May 6, 2024

View reviewed changes

currency/manager.go Outdated Show resolved Hide resolved

exchanges/gateio/gateio.go Show resolved Hide resolved

gbjk force-pushed the feature/squirter branch 3 times, most recently from 752fe2e to 907d016 Compare May 7, 2024 11:10

gbjk requested a review from shazbert May 7, 2024 12:27

shazbert requested changes May 7, 2024

View reviewed changes

currency/manager_test.go Show resolved Hide resolved

gbjk force-pushed the feature/squirter branch 3 times, most recently from 0b928df to 93fee81 Compare May 8, 2024 02:09

gbjk requested a review from shazbert May 8, 2024 02:10

shazbert approved these changes May 9, 2024

View reviewed changes

shazbert approved these changes May 10, 2024

View reviewed changes

gbjk mentioned this pull request May 15, 2024

Bybit: Fix WS ticker processing #1538

Open

4 tasks

gloriousCode approved these changes May 16, 2024

View reviewed changes

shazbert added the szrc shazbert review complete label May 16, 2024

gbjk added 2 commits May 16, 2024 10:48

Tests: Move and simplify TestFixtureToDataHandler

03c7efe

Currency: Fix PairsManager.Load breaking matcher

7c1ca69

gbjk added 9 commits May 16, 2024 10:48

Tests: Add multi-instance cache to UpdatePairsOnce

3ed71bd

Kraken: Fix TestUpdateTickers race error

48c8ced

Calling StorePairs on global instance can lead to race

Bitfinex: Fix TestUpdateTickers racing intermittently

7d4c965

Currency: Fix concurrent access to PM formats

558a9af

Currency: Fix SupportsAsset implementation

3eaf2a7

This should delegate entirely to PairManager's IsAssetSupported

Okx: Fix PM intrusion, rm GetPairFromInstrumentID

c66d15c

Exchange: Fix SetGlobalPairsManager to set asset enabled

18e53e8

Bitflyer: Fix race on set TestGetCurrURL

1f6fd1b

TestGetCurrencyTradeURL would fail sometimes due to sequencing of enabling futures but not having pairs for it.

Tests: Simplify usage pattern for FixtureToDH

0708926

gbjk force-pushed the feature/squirter branch from 9398c3f to 0708926 Compare May 16, 2024 03:53

thrasher- approved these changes May 16, 2024

View reviewed changes

thrasher- merged commit 7d1eecf into thrasher-corp:master May 16, 2024
5 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: Various race fixes and move TestFixtureToDataHandler #1534

Tests: Various race fixes and move TestFixtureToDataHandler #1534

gbjk commented May 5, 2024 •

edited

shazbert left a comment

codecov bot commented May 6, 2024 •

edited

shazbert left a comment

gbjk commented May 6, 2024

shazbert left a comment

gbjk commented May 7, 2024

shazbert left a comment

gbjk commented May 8, 2024

shazbert left a comment

shazbert left a comment

gloriousCode left a comment

gloriousCode May 16, 2024

gbjk May 16, 2024

thrasher- left a comment

Tests: Various race fixes and move TestFixtureToDataHandler #1534

Tests: Various race fixes and move TestFixtureToDataHandler #1534

Conversation

gbjk commented May 5, 2024 • edited

Type of change

How has this been tested

shazbert left a comment

Choose a reason for hiding this comment

codecov bot commented May 6, 2024 • edited

Codecov Report

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented May 6, 2024

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented May 7, 2024

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented May 8, 2024

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

gloriousCode May 16, 2024

Choose a reason for hiding this comment

gbjk May 16, 2024

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

gbjk commented May 5, 2024 •

edited

codecov bot commented May 6, 2024 •

edited