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

Jump to bottom

Fixes race in testsales #995

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Fixes race in testsales #995

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
47c2293
Adds isWaiting to mockClock to remove sleep in testsales
benbierens Nov 21, 2024
a697db1
Merge branch 'master' into bug/fixes-racecondition-testsales
benbierens Nov 25, 2024
30e5d95
Merge branch 'master' into bug/fixes-racecondition-testsales
benbierens Nov 25, 2024
478de31
Merge branch 'master' into bug/fixes-racecondition-testsales
benbierens Nov 26, 2024
08a1900
Review comments by Eric. Also replaced two more sleeps with check-eve…
benbierens Nov 26, 2024
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
14 changes: 10 additions & 4 deletions tests/codex/sales/testsales.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import pkg/codex/sales/slotqueue
import pkg/codex/stores/repostore
import pkg/codex/blocktype as bt
import pkg/codex/node
import pkg/codex/utils/asyncstatemachine
import ../../asynctest
import ../helpers
import ../helpers/mockmarket
Expand Down Expand Up @@ -188,9 +189,14 @@ asyncchecksuite "Sales":
await repoTmp.destroyDb()
await metaTmp.destroyDb()

proc isInState(idx: int, state: string): Future[bool] {.async.} =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes: check-eventually is needed to give sales time to load the agent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but shouldn't we just early return if sales.agent.len == 0? I don't think there's a need for check eventually in the isInState proc, since its usage is used in a check eventually isInState....

proc description(state: State): string =
$state
check eventually sales.agents.len > idx
sales.agents[idx].query(description) == state.some

proc allowRequestToStart {.async.} =
# wait until we're in initialproving state
await sleepAsync(10.millis)
check eventually (await isInState(0, "SaleInitialProving"))
# it won't start proving until the next period
await clock.advanceToNextPeriod(market)

Expand Down Expand Up @@ -291,7 +297,7 @@ asyncchecksuite "Sales":
test "items in queue are readded (and marked seen) once ignored":
await market.requestStorage(request)
let items = SlotQueueItem.init(request)
await sleepAsync(10.millis) # queue starts paused, allow items to be added to the queue
check eventually queue.len > 0 # queue starts paused, allow items to be added to the queue
check eventually queue.paused
# The first processed item will be will have been re-pushed with `seen =
# true`. Then, once this item is processed by the queue, its 'seen' flag
Expand All @@ -311,7 +317,7 @@ asyncchecksuite "Sales":
createAvailability() # enough to fill a single slot
await market.requestStorage(request)
let items = SlotQueueItem.init(request)
await sleepAsync(10.millis) # queue starts paused, allow items to be added to the queue
check eventually queue.len > 0 # queue starts paused, allow items to be added to the queue
check eventually queue.paused
# The first processed item/slot will be filled (eventually). Subsequent
# items will be processed and eventually re-pushed with `seen = true`. Once
Expand Down