Skip to content

Commit

Permalink
assert: collect.FailNow() should not panic (stretchr#1481)
Browse files Browse the repository at this point in the history
## Summary
`collect.FailNow()` should exit goroutine without a panic to be usable
with `require` package.

## Changes
`collect.FailNow()` just does `runtime.Goexit()` instead of `panic()`.
For example `FailNow()` from `testing` package [behaves
similarly](https://cs.opensource.google/go/go/+/refs/tags/go1.21.2:src/testing/testing.go;l=973).

## Motivation

I just want `require` package to be usable with `EventuallyWithT` e.g. I
want this example to pass but it panics:

```go
package main

import (
	"testing"
	"time"

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

func TestRequireEventuallyWithT(t *testing.T) {
	state := 0
	require.EventuallyWithT(t, func(c *assert.CollectT) {
		defer func() { state += 1 }()
		require.True(c, state == 2)
	}, 100*time.Millisecond, 10*time.Millisecond)
}
```

See https://go.dev/play/p/Oqd95IT7qxQ

## Related issues
Fixes stretchr#1396
Fixes stretchr#1457
  • Loading branch information
marshall-lee authored Jun 13, 2024
1 parent 84619f5 commit b074924
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 13 deletions.
30 changes: 22 additions & 8 deletions assert/assertions.go
Original file line number Diff line number Diff line change
Expand Up @@ -1956,6 +1956,9 @@ func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick t

// CollectT implements the TestingT interface and collects all errors.
type CollectT struct {
// A slice of errors. Non-nil slice denotes a failure.
// If it's non-nil but len(c.errors) == 0, this is also a failure
// obtained by direct c.FailNow() call.
errors []error
}

Expand All @@ -1964,9 +1967,10 @@ func (c *CollectT) Errorf(format string, args ...interface{}) {
c.errors = append(c.errors, fmt.Errorf(format, args...))
}

// FailNow panics.
func (*CollectT) FailNow() {
panic("Assertion failed")
// FailNow stops execution by calling runtime.Goexit.
func (c *CollectT) FailNow() {
c.fail()
runtime.Goexit()
}

// Deprecated: That was a method for internal usage that should not have been published. Now just panics.
Expand All @@ -1979,6 +1983,16 @@ func (*CollectT) Copy(TestingT) {
panic("Copy() is deprecated")
}

func (c *CollectT) fail() {
if !c.failed() {
c.errors = []error{} // Make it non-nil to mark a failure.
}
}

func (c *CollectT) failed() bool {
return c.errors != nil
}

// EventuallyWithT asserts that given condition will be met in waitFor time,
// periodically checking target function each tick. In contrast to Eventually,
// it supplies a CollectT to the condition function, so that the condition
Expand All @@ -2003,7 +2017,7 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
}

var lastFinishedTickErrs []error
ch := make(chan []error, 1)
ch := make(chan *CollectT, 1)

timer := time.NewTimer(waitFor)
defer timer.Stop()
Expand All @@ -2023,16 +2037,16 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
go func() {
collect := new(CollectT)
defer func() {
ch <- collect.errors
ch <- collect
}()
condition(collect)
}()
case errs := <-ch:
if len(errs) == 0 {
case collect := <-ch:
if !collect.failed() {
return true
}
// Keep the errors from the last ended condition, so that they can be copied to t if timeout is reached.
lastFinishedTickErrs = errs
lastFinishedTickErrs = collect.errors
tick = ticker.C
}
}
Expand Down
20 changes: 15 additions & 5 deletions assert/assertions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2923,16 +2923,15 @@ func TestEventuallyWithTFalse(t *testing.T) {
func TestEventuallyWithTTrue(t *testing.T) {
mockT := new(errorsCapturingT)

state := 0
counter := 0
condition := func(collect *CollectT) {
defer func() {
state += 1
}()
True(collect, state == 2)
counter += 1
True(collect, counter == 2)
}

True(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
Len(t, mockT.errors, 0)
Equal(t, 2, counter, "Condition is expected to be called 2 times")
}

func TestEventuallyWithT_ConcurrencySafe(t *testing.T) {
Expand Down Expand Up @@ -2970,6 +2969,17 @@ func TestEventuallyWithT_ReturnsTheLatestFinishedConditionErrors(t *testing.T) {
Len(t, mockT.errors, 2)
}

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

condition := func(collect *CollectT) {
collect.FailNow()
}

False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
Len(t, mockT.errors, 1)
}

func TestNeverFalse(t *testing.T) {
condition := func() bool {
return false
Expand Down
29 changes: 29 additions & 0 deletions require/requirements_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"errors"
"testing"
"time"

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

// AssertionTesterInterface defines an interface to be used for testing assertion methods
Expand Down Expand Up @@ -681,3 +683,30 @@ func TestErrorAssertionFunc(t *testing.T) {
})
}
}

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

condition := func(collect *assert.CollectT) {
True(collect, false)
}

EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond)
True(t, mockT.Failed, "Check should fail")
}

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

counter := 0
condition := func(collect *assert.CollectT) {
defer func() {
counter += 1
}()
True(collect, counter == 1)
}

EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond)
False(t, mockT.Failed, "Check should pass")
Equal(t, 2, counter, "Condition is expected to be called 2 times")
}

0 comments on commit b074924

Please sign in to comment.