From b074924938f86d417f1c9a845c7e8b0784d7f937 Mon Sep 17 00:00:00 2001 From: Vladimir Kochnev Date: Thu, 13 Jun 2024 23:09:51 +0300 Subject: [PATCH] assert: collect.FailNow() should not panic (#1481) ## 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 https://github.com/stretchr/testify/issues/1396 Fixes https://github.com/stretchr/testify/issues/1457 --- assert/assertions.go | 30 ++++++++++++++++++++++-------- assert/assertions_test.go | 20 +++++++++++++++----- require/requirements_test.go | 29 +++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 13 deletions(-) diff --git a/assert/assertions.go b/assert/assertions.go index 45777f0..104a0c9 100644 --- a/assert/assertions.go +++ b/assert/assertions.go @@ -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 } @@ -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. @@ -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 @@ -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() @@ -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 } } diff --git a/assert/assertions_test.go b/assert/assertions_test.go index 8b87727..064b92f 100644 --- a/assert/assertions_test.go +++ b/assert/assertions_test.go @@ -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) { @@ -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 diff --git a/require/requirements_test.go b/require/requirements_test.go index febf0c1..824b159 100644 --- a/require/requirements_test.go +++ b/require/requirements_test.go @@ -5,6 +5,8 @@ import ( "errors" "testing" "time" + + "github.com/stretchr/testify/assert" ) // AssertionTesterInterface defines an interface to be used for testing assertion methods @@ -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") +}