Fixes race in testsales #995
Conversation
tests/codex/sales/testsales.nim
Outdated
| @@ -190,7 +190,7 @@ asyncchecksuite "Sales": | |||
|
|
|||
| proc allowRequestToStart {.async.} = | |||
| # wait until we're in initialproving state | |||
| await sleepAsync(10.millis) | |||
| check eventually clock.isWaiting | |||
There was a problem hiding this comment.
If the underlying function changes (adding a waitUntil in a certain sales state), then the test will fail and it may be a bit difficult to test. Instead, could we perhaps check the state? I think this should do it:
proc isInState(idx: int, state: string): bool =
proc description(state: State): string =
$state
sales.agents[idx].query(description) == state.some
check eventually isInState(0, "SaleInitialProving")There was a problem hiding this comment.
I like this, it seems a lot more reusable. I think isWaiting for a mockClock makes perfect sense. I'm giving this a try.
There was a problem hiding this comment.
Be aware though that allowRequestToStart may not actually have been checking that the state was in the initial proving state, so if you update it, some tests may break
There was a problem hiding this comment.
I have applied your suggestion and I like it. The tests are all happy.
I know every keystroke I make reduces the likelihood that I can get this merged, BUT I did replace two more sleeps with check-eventually. Plz check if you agree with them.
| @@ -188,9 +189,14 @@ asyncchecksuite "Sales": | |||
| await repoTmp.destroyDb() | |||
| await metaTmp.destroyDb() | |||
|
|
|||
| proc isInState(idx: int, state: string): Future[bool] {.async.} = | |||
There was a problem hiding this comment.
Does this need to be async?
There was a problem hiding this comment.
Yes: check-eventually is needed to give sales time to load the agent.
There was a problem hiding this comment.
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....
Related to (but probably doesn't fix) #992