assert: fix TestEventuallyTimeout

[dolmen]

Summary

Fix TestEventuallyIssue805 which was flaky because of the use of a timer that has exactly the same duration as the Eventually timout. But time is not a synchronization primitive.

Changes

• Now we use channels to ensure that the condition is still running and Eventually times out before checking its return value.
• The test is also renamed to TestEventuallyTimeout to more clearly show its purpose.

Motivation

Test was flaky (see #1411). CI was unreliable.

Related issues

Closes #1411.
Related to #805, #869, #865.

[SuperQ]

Nice find

[SuperQ]

I'd also recommend changing the timer values for this to be similar to the other timer tests to be values larger than a single millisecond/microsecond. I've seen cases where CI tests be flaky because of too-close (+-1ms) a timer wait.

[brackendawson]

The durations shouldn't need to be increased now because there are no longer two short timers running at the same time. Increasing them would only slow down the test. I'd approve this as is.

I've found similar cases where a potential allocation by time.After can slow down the creation of one timer after the other has started, this can be solved like so: 8cd9017

[stevenh]

Nice approach to eliminate the timing totally.

I would use the idiomatic chan struct{} for the done channel's, but that's just a nit given it's a test.

Not sure if you intended to leave the logging in or not?

[dolmen]

Following my announcement on Slack, I'm pinging all other co-maintainers.

[MovieStoreGuy]

I will be happy to merge this once @brackendawson has reviewed again after the request for changes.

[brackendawson]

Looks great!