Remove arbitrary sleeps from tests #87
Conversation
| } | ||
| } | ||
|
|
||
| func waitUntil(ctx context.Context, condition func(context.Context) (bool, error), interval time.Duration) error { |
There was a problem hiding this comment.
Why not use require.Eventually?
There was a problem hiding this comment.
It's so simple and idiomatic that it hardly seems worth another dependency
There was a problem hiding this comment.
Plus the context already controls the duration of the test so we don't have to guess a max duration to wait for.
There was a problem hiding this comment.
It's so simple and idiomatic that it hardly seems worth another dependency
We use require in almost every other libp2p package, so I'd much prefer to use it here as well.
Plus the context already controls the duration of the test so we don't have to guess a max duration to wait for.
As far as I can see, the context here doesn't actually do anything, as it's only canceled in defer. Or am I missing something?
There was a problem hiding this comment.
It's cancelled by the test runner, controlled by go test -timeout so it can be adjusted externally.
There was a problem hiding this comment.
It's cancelled by the test runner, controlled by go test -timeout so it can be adjusted externally.
Is it? I've never heard of that feature. I wrote a test case:
func TestCancelation(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
<-ctx.Done()
fmt.Println("test canceled", ctx.Err())
}and ran with go test -timeout 1s. The test is just blowing up, and the context is not canceled.
There was a problem hiding this comment.
Yeah I misremembered, you need to use t.Deadline() and pass it to context.WithDeadline
|
Updated to use require.Eventually. We lose the ability to abort the test early on a non-expected error and there is an arbitrary 30 second cutoff but it makes the test more deterministic than the arbitrary sleeps. |
|
Hmm. Seems my guess of 30s timeout is too low. Would much prefer not to have to make these kinds of guesses. |
|
Increased to 5 minutes |
|
Still failing. Using require.Eventually doesn't help fix these flakes. |
|
Still seems flaky compared with the original change. |
|
Let's use the original change then? Sorry for the confusion. |
|
Rebased back to the original. It's not clear to me why require.Eventually performs so poorly when with this version all tests pass within 2 minutes. |
Also speeds up tests locally since most propagation happens in much less than 1 second