assert: early return recovering on panic in EventuallyWithT

[pducolin]

Summary

Catch and recover from panic on assert.EventuallyWithT

Changes

• Add failed channel to handle early return failures
• Trigger failed when recovering from panic in condition routine
• Add test to assert early fail on calls to collect.FailNow, preserving errors trace

Motivation

While using EventuallyWithT we might want to early return on a never expected condition: in this case, we would call require within the condition and expect to early return. Currently, calling require from a condition function panics losing the requirement error trace. Catching and recovering from this panic allows for early failing the EventuallyWithT assertion and keeping the failed require error trace.

An example of such a use case is an assertion on a network request, where we want to early failed on http error, but we want to eventually assert on the body's properties

import (
	"errors"
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

// change shoulMockNetworkError to true to reproduce panic in eventually
var state = 0
const shouldMockNetworkError = false

func mockHttpGet() (ok bool, err error) {
	defer func() {
		state += 1
	}()
	if state > 1 {
		return true, nil
	}
	if shouldMockNetworkError {
		return false, errors.New("network error")
	}
	return false, nil
}

func TestEventuallyHttp(t *testing.T) {
	ret := assert.EventuallyWithT(t, func(c *assert.CollectT) {
		isOK, err := mockHttpGet()
		require.NoError(c, err)
		assert.True(c, isOK)
	}, 5*time.Second, 200*time.Millisecond)
	require.True(t, ret)
}

Try it in Golang Playground

Related issues

Closes #1396
Closes #1457

[pducolin]

I see this PR conflicts with https://github.com/stretchr/testify/pull/1395/files, I can come with an alternative solution, passing collected errors to the failed channel

func (c *CollectT) FailNow() {
  panic(c.errors)
}

// then in EventuallyWithT select
case <-tick:
  go func() {
    defer func() {
	    if r := recover(); r != nil {
		    failed <- r
	    }
    }()
    condition(collect)
    ch <- collect.errors
  }()
case errs := <-failed:
  for _, err := range errs {
    t.Errorf("%v", err)
  }
  return Fail(t, "Require assertion failed", msgAndArgs...)
}

[marshall-lee]

Hi @pducolin!

Unfortunately it doesn't fix the compatibility with require package. The return is too early so I cannot for example write require.Equal(c, expected, actual) that fails several times but eventually is true.

I proposed an alternative solution #1481

[pducolin]

Interesting, I interpreted require as assertions that should never happen, like network errors. So I expect require to panic and stop the loop.

[marshall-lee]

I see assert and require as two different styles of writing tests with testify. I've seen projects that forbid a use of assert API and require is all they use.

As of network errors they also may be temporary. Suppose we dial a 127.0.0.1:30000 but the service is starting in background. So instead of:

	require.EventuallyWithT(t, func(c *assert.CollectT) {
		conn, err := net.Dial("tcp", "127.0.0.1:30000")
		if assert.NoError(err) {
			return
		}
		n, err := conn.Read(buf)
		if assert.NoError(err) {
			return
		}
	}, 100*time.Millisecond, 10*time.Millisecond)

I would want just write:

	require.EventuallyWithT(t, func(c *assert.CollectT) {
		conn, err := net.Dial("tcp", address)
		require.NoError(err)
		n, err := conn.Read(buf)
		require.NoError(err)
	}, 100*time.Millisecond, 10*time.Millisecond)

[pducolin]

I tend to use require and assert within the same context. I use require when I want the unexpected condition to stop the test, so here I would expect it to panic as it does normally. If I want a test to fail and retry, I would probably use the longer

assert.EventuallyWithT(t, func(c *assert.CollectT) {
  conn, err := net.Dial("tcp", "127.0.0.1:30000")
  if assert.NoError(err) {
	  return
  }
  n, err := conn.Read(buf)
  if assert.NoError(err) {
	  return
  }
}, 100*time.Millisecond, 10*time.Millisecond)

[pducolin]

From testify docs require calls t.FailNow on test failure.
From testing docs t.FailNow

marks the function as having failed and stops its execution by calling runtime.Goexit (which then runs all deferred calls in the current goroutine). Execution will continue at the next test or benchmark.

so your fix is indeed the expected behaviour.

[pducolin]

Closing this PR, thank you for the review !