Eventually/EventuallyWithT should only use one goroutine

[cszczepaniak]

Currently, Eventually and EventuallyWithT spin up a goroutine per condition check. It also sets the ticker channel to nil while the condition is being checked, which makes it hard to understand to readers that the select will never hit case <-tick while the condition is running.

A better implementation may be to spin up a single goroutine which polls the condition repeatedly while the function waits on the result or a timeout.

This would also solve #1424 if implemented correctly.

[cszczepaniak]

Here's the implementation I have in mind. We could similarly implement EventuallyWithT. I will open a PR if this is desirable:

func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick time.Duration, msgAndArgs ...interface{}) bool {
	if h, ok := t.(tHelper); ok {
		h.Helper()
	}

	ctx, done := context.WithTimeout(context.Background(), waitFor)
	defer done()

	ch := make(chan bool, 1)
	defer close(ch)

	go func() {
		ticker := time.NewTicker(tick)
		defer ticker.Stop()

		for {
			if condition() {
				ch <- true
				return
			}

			select {
			case <-ticker.C:
			case <-ctx.Done():
				return
			}
		}
	}()

	select {
	case <-ch:
		return true
	case <-ctx.Done():
		return Fail(t, "Condition never satisfied", msgAndArgs...)
	}
}

[dolmen]

I like the idea of not overlapping the executions of the condition. However this will change the behavior.

We need feedback from the community.

[cszczepaniak]

The behavior will change as described in #1424 (namely, that the condition will be checked before waiting for one tick) -- are there more ways in which this will change the behavior that should be enumerated here for completeness?

[cszczepaniak]

This comment suggests that Eventually is guaranteed to check the condition every tick, which may be an additional change to behavior. This is not true however, because the implementation sets the tick channel to nil in between executions. Here's a simple test that demonstrates the condition being called strictly fewer times than there are ticks in the total timeout duration:

func TestEventuallyCallsEveryTick(t *testing.T) {
	mockT := new(testing.T)

	n := 0
	condition := func() bool {
		n++
		time.Sleep(250 * time.Millisecond)
		return false
	}

	False(t, Eventually(mockT, condition, 500*time.Millisecond, 100*time.Millisecond))
	// If called every tick, should be 5 because 500ms/100ms = 5
	Equal(t, 5, n)
}

With result:

--- FAIL: TestEventuallyCallsEveryTick (0.50s)
    testify/assert/assertions_test.go:2723: 
        	Error Trace:	
        	Error:      	Not equal: 
        	            	expected: 5
        	            	actual  : 2
        	Test:       	TestEventuallyCallsEveryTick
FAIL
FAIL	github.com/stretchr/testify/assert	0.766s
FAIL

[dolmen]

@cszczepaniak Thanks for this analysis.

Here is a more verbose test running on go.dev/play: https://go.dev/play/p/-Dyc4bZRCTk

func TestEventuallyCallsEveryTick(t *testing.T) {
	n := 0
	condition := func() bool {
		m := n
		t.Logf("cond%d %v", m, time.Now())
		n++
		time.Sleep(250 * time.Millisecond)
		t.Logf("cond%d %v", m, time.Now())
		return false
	}

	assert.False(t, assert.Eventually(t, condition, 500*time.Millisecond, 100*time.Millisecond))
	// If called every tick, should be 5 because 500ms/100ms = 5
	assert.Equal(t, 5, n)
}

=== RUN   TestEventuallyCallsEveryTick
    prog_test.go:14: cond0 2009-11-10 23:00:00.1 +0000 UTC m=+0.100000001
    prog_test.go:17: cond0 2009-11-10 23:00:00.35 +0000 UTC m=+0.350000001
    prog_test.go:14: cond1 2009-11-10 23:00:00.35 +0000 UTC m=+0.350000001
    prog_test.go:21: 
        	Error Trace:	/tmp/sandbox109644822/prog_test.go:21
        	Error:      	Condition never satisfied
        	Test:       	TestEventuallyCallsEveryTick
    prog_test.go:23: 
        	Error Trace:	/tmp/sandbox109644822/prog_test.go:23
        	Error:      	Not equal: 
        	            	expected: 5
        	            	actual  : 2
        	Test:       	TestEventuallyCallsEveryTick
--- FAIL: TestEventuallyCallsEveryTick (0.50s)
FAIL

[cszczepaniak]

Thanks @dolmen, today I learned that the Go playground can run tests!

[brackendawson]

I hadn't noticed the nil ticker. Given the current actual behaviour does not overlap ticks I'm quite happy with such a change. We should also bring the documentation into line with the real behaviour.