Repository: stretchr/testify
Pull request #1474 (2023-09-26 08:20:11)

suite: refactor test assertions
## Summary

This PR refactors `suite` tests.

## Changes

- reverse actual-expected parameters in asserts;
- use `assert.False(t, val)` instead of `assert.Equal(t, false, val)`;
- use `assert.Len(t, slice, v)` instead of `assert.Equal(t, len(slice), v)`;
- remove duplicated `assert.Equal(suite.T(),...)`.

## Motivation

To simplify test code and provide precise output if a test fails.

## Related issues
<!-- Put `Closes #XXXX` for each issue number this PR fixes/closes -->



Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 0ffd24c..be8b4af 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -7,12 +7,12 @@ jobs:
     strategy:
       matrix:
         go_version:
-          - "1.19"
           - "1.20"
+          - "1.21"
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Setup Go
-        uses: actions/setup-go@v3.2.0
+        uses: actions/setup-go@v4.1.0
         with:
           go-version: ${{ matrix.go_version }}
       - run: ./.ci.gogenerate.sh
@@ -28,10 +28,11 @@ jobs:
           - "1.18"
           - "1.19"
           - "1.20"
+          - "1.21"
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Setup Go
-        uses: actions/setup-go@v3.2.0
+        uses: actions/setup-go@v4.1.0
         with:
           go-version: ${{ matrix.go_version }}
       - run: go test -v -race ./...
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 67fc95b..cd4331c 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -13,7 +13,7 @@ jobs:
       contents: write
     steps:
       - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Create GitHub release from tag
         uses: softprops/action-gh-release@v1
\ No newline at end of file
diff --git a/assert/assertions.go b/assert/assertions.go
index 719164e..6843d42 100644
--- a/assert/assertions.go
+++ b/assert/assertions.go
@@ -59,20 +59,25 @@ func ObjectsAreEqual(expected, actual interface{}) bool {
 	if expected == nil || actual == nil {
 		return expected == actual
 	}
-
-	exp, ok := expected.([]byte)
-	if !ok {
+	switch exp := expected.(type) {
+	case []byte:
+		act, ok := actual.([]byte)
+		if !ok {
+			return false
+		}
+		if exp == nil || act == nil {
+			return exp == nil && act == nil
+		}
+		return bytes.Equal(exp, act)
+	case time.Time:
+		act, ok := actual.(time.Time)
+		if !ok {
+			return false
+		}
+		return exp.Equal(act)
+	default:
 		return reflect.DeepEqual(expected, actual)
 	}
-
-	act, ok := actual.([]byte)
-	if !ok {
-		return false
-	}
-	if exp == nil || act == nil {
-		return exp == nil && act == nil
-	}
-	return bytes.Equal(exp, act)
 }
 
 // copyExportedFields iterates downward through nested data structures and creates a copy
@@ -110,7 +115,12 @@ func copyExportedFields(expected interface{}) interface{} {
 		return result.Interface()
 
 	case reflect.Array, reflect.Slice:
-		result := reflect.MakeSlice(expectedType, expectedValue.Len(), expectedValue.Len())
+		var result reflect.Value
+		if expectedKind == reflect.Array {
+			result = reflect.New(reflect.ArrayOf(expectedValue.Len(), expectedType.Elem())).Elem()
+		} else {
+			result = reflect.MakeSlice(expectedType, expectedValue.Len(), expectedValue.Len())
+		}
 		for i := 0; i < expectedValue.Len(); i++ {
 			index := expectedValue.Index(i)
 			if isNil(index) {
@@ -140,6 +150,8 @@ func copyExportedFields(expected interface{}) interface{} {
 // structures.
 //
 // This function does no assertion of any kind.
+//
+// Deprecated: Use [EqualExportedValues] instead.
 func ObjectsExportedFieldsAreEqual(expected, actual interface{}) bool {
 	expectedCleaned := copyExportedFields(expected)
 	actualCleaned := copyExportedFields(actual)
@@ -1868,23 +1880,18 @@ func (c *CollectT) Errorf(format string, args ...interface{}) {
 }
 
 // FailNow panics.
-func (c *CollectT) FailNow() {
+func (*CollectT) FailNow() {
 	panic("Assertion failed")
 }
 
-// Reset clears the collected errors.
-func (c *CollectT) Reset() {
-	c.errors = nil
+// Deprecated: That was a method for internal usage that should not have been published. Now just panics.
+func (*CollectT) Reset() {
+	panic("Reset() is deprecated")
 }
 
-// Copy copies the collected errors to the supplied t.
-func (c *CollectT) Copy(t TestingT) {
-	if tt, ok := t.(tHelper); ok {
-		tt.Helper()
-	}
-	for _, err := range c.errors {
-		t.Errorf("%v", err)
-	}
+// Deprecated: That was a method for internal usage that should not have been published. Now just panics.
+func (*CollectT) Copy(TestingT) {
+	panic("Copy() is deprecated")
 }
 
 // EventuallyWithT asserts that given condition will be met in waitFor time,
@@ -1910,8 +1917,8 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 		h.Helper()
 	}
 
-	collect := new(CollectT)
-	ch := make(chan bool, 1)
+	var lastFinishedTickErrs []error
+	ch := make(chan []error, 1)
 
 	timer := time.NewTimer(waitFor)
 	defer timer.Stop()
@@ -1922,19 +1929,25 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 	for tick := ticker.C; ; {
 		select {
 		case <-timer.C:
-			collect.Copy(t)
+			for _, err := range lastFinishedTickErrs {
+				t.Errorf("%v", err)
+			}
 			return Fail(t, "Condition never satisfied", msgAndArgs...)
 		case <-tick:
 			tick = nil
-			collect.Reset()
 			go func() {
+				collect := new(CollectT)
+				defer func() {
+					ch <- collect.errors
+				}()
 				condition(collect)
-				ch <- len(collect.errors) == 0
 			}()
-		case v := <-ch:
-			if v {
+		case errs := <-ch:
+			if len(errs) == 0 {
 				return true
 			}
+			// Keep the errors from the last ended condition, so that they can be copied to t if timeout is reached.
+			lastFinishedTickErrs = errs
 			tick = ticker.C
 		}
 	}
diff --git a/assert/http_assertions.go b/assert/http_assertions.go
index 6cfb6a8..8b82ca8 100644
--- a/assert/http_assertions.go
+++ b/assert/http_assertions.go
@@ -12,7 +12,7 @@ import (
 // an error if building a new request fails.
 func httpCode(handler http.HandlerFunc, method, url string, values url.Values) (int, error) {
 	w := httptest.NewRecorder()
-	req, err := http.NewRequest(method, url, nil)
+	req, err := http.NewRequest(method, url, http.NoBody)
 	if err != nil {
 		return -1, err
 	}
@@ -116,7 +116,7 @@ func HTTPBody(handler http.HandlerFunc, method, url string, values url.Values) s
 	if len(values) > 0 {
 		url += "?" + values.Encode()
 	}
-	req, err := http.NewRequest(method, url, nil)
+	req, err := http.NewRequest(method, url, http.NoBody)
 	if err != nil {
 		return ""
 	}


Test patch:
diff --git a/assert/assertions_test.go b/assert/assertions_test.go
index 40e3720..9a44f95 100644
--- a/assert/assertions_test.go
+++ b/assert/assertions_test.go
@@ -148,6 +148,12 @@ func TestObjectsAreEqual(t *testing.T) {
 		t.Fail()
 	}
 
+	tm := time.Now()
+	tz := tm.In(time.Local)
+	if !ObjectsAreEqualValues(tm, tz) {
+		t.Error("ObjectsAreEqualValues should return true for time.Time objects with different time zones")
+	}
+
 }
 
 type Nested struct {
@@ -402,6 +408,11 @@ func TestEqualExportedValues(t *testing.T) {
 	            	+   Exported: (int) 2,
 	            	    notExported: (interface {}) <nil>`,
 		},
+		{
+			value1:        S{[2]int{1, 2}, Nested{2, 3}, 4, Nested{5, 6}},
+			value2:        S{[2]int{1, 2}, Nested{2, nil}, nil, Nested{}},
+			expectedEqual: true,
+		},
 	}
 
 	for _, c := range cases {
@@ -2760,11 +2771,22 @@ func TestEventuallyTrue(t *testing.T) {
 	True(t, Eventually(t, condition, 100*time.Millisecond, 20*time.Millisecond))
 }
 
+// errorsCapturingT is a mock implementation of TestingT that captures errors reported with Errorf.
+type errorsCapturingT struct {
+	errors []error
+}
+
+func (t *errorsCapturingT) Errorf(format string, args ...interface{}) {
+	t.errors = append(t.errors, fmt.Errorf(format, args...))
+}
+
+func (t *errorsCapturingT) Helper() {}
+
 func TestEventuallyWithTFalse(t *testing.T) {
-	mockT := new(CollectT)
+	mockT := new(errorsCapturingT)
 
 	condition := func(collect *CollectT) {
-		True(collect, false)
+		Fail(collect, "condition fixed failure")
 	}
 
 	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
@@ -2772,7 +2794,7 @@ func TestEventuallyWithTFalse(t *testing.T) {
 }
 
 func TestEventuallyWithTTrue(t *testing.T) {
-	mockT := new(CollectT)
+	mockT := new(errorsCapturingT)
 
 	state := 0
 	condition := func(collect *CollectT) {
@@ -2786,6 +2808,41 @@ func TestEventuallyWithTTrue(t *testing.T) {
 	Len(t, mockT.errors, 0)
 }
 
+func TestEventuallyWithT_ConcurrencySafe(t *testing.T) {
+	mockT := new(errorsCapturingT)
+
+	condition := func(collect *CollectT) {
+		Fail(collect, "condition fixed failure")
+	}
+
+	// To trigger race conditions, we run EventuallyWithT with a nanosecond tick.
+	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, time.Nanosecond))
+	Len(t, mockT.errors, 2)
+}
+
+func TestEventuallyWithT_ReturnsTheLatestFinishedConditionErrors(t *testing.T) {
+	// We'll use a channel to control whether a condition should sleep or not.
+	mustSleep := make(chan bool, 2)
+	mustSleep <- false
+	mustSleep <- true
+	close(mustSleep)
+
+	condition := func(collect *CollectT) {
+		if <-mustSleep {
+			// Sleep to ensure that the second condition runs longer than timeout.
+			time.Sleep(time.Second)
+			return
+		}
+
+		// The first condition will fail. We expect to get this error as a result.
+		Fail(collect, "condition fixed failure")
+	}
+
+	mockT := new(errorsCapturingT)
+	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
+	Len(t, mockT.errors, 2)
+}
+
 func TestNeverFalse(t *testing.T) {
 	condition := func() bool {
 		return false
diff --git a/assert/http_assertions_test.go b/assert/http_assertions_test.go
index 413c971..8da117e 100644
--- a/assert/http_assertions_test.go
+++ b/assert/http_assertions_test.go
@@ -2,6 +2,7 @@ package assert
 
 import (
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"testing"
@@ -11,6 +12,12 @@ func httpOK(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusOK)
 }
 
+func httpReadBody(w http.ResponseWriter, r *http.Request) {
+	_, _ = io.Copy(io.Discard, r.Body)
+	w.WriteHeader(http.StatusOK)
+	_, _ = w.Write([]byte("hello"))
+}
+
 func httpRedirect(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusTemporaryRedirect)
 }
@@ -41,6 +48,10 @@ func TestHTTPSuccess(t *testing.T) {
 	mockT4 := new(testing.T)
 	assert.Equal(HTTPSuccess(mockT4, httpStatusCode, "GET", "/", nil), false)
 	assert.True(mockT4.Failed())
+
+	mockT5 := new(testing.T)
+	assert.Equal(HTTPSuccess(mockT5, httpReadBody, "POST", "/", nil), true)
+	assert.False(mockT5.Failed())
 }
 
 func TestHTTPRedirect(t *testing.T) {
@@ -122,7 +133,7 @@ func TestHTTPStatusesWrapper(t *testing.T) {
 
 func httpHelloName(w http.ResponseWriter, r *http.Request) {
 	name := r.FormValue("name")
-	_, _ = w.Write([]byte(fmt.Sprintf("Hello, %s!", name)))
+	_, _ = fmt.Fprintf(w, "Hello, %s!", name)
 }
 
 func TestHTTPRequestWithNoParams(t *testing.T) {
@@ -165,6 +176,8 @@ func TestHttpBody(t *testing.T) {
 	assert.False(HTTPBodyNotContains(mockT, httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, "Hello, World!"))
 	assert.False(HTTPBodyNotContains(mockT, httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, "World"))
 	assert.True(HTTPBodyNotContains(mockT, httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, "world"))
+
+	assert.True(HTTPBodyContains(mockT, httpReadBody, "GET", "/", nil, "hello"))
 }
 
 func TestHttpBodyWrappers(t *testing.T) {
@@ -178,5 +191,4 @@ func TestHttpBodyWrappers(t *testing.T) {
 	assert.False(mockAssert.HTTPBodyNotContains(httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, "Hello, World!"))
 	assert.False(mockAssert.HTTPBodyNotContains(httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, "World"))
 	assert.True(mockAssert.HTTPBodyNotContains(httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, "world"))
-
 }
diff --git a/suite/suite_test.go b/suite/suite_test.go
index d684f52..e21fb71 100644
--- a/suite/suite_test.go
+++ b/suite/suite_test.go
@@ -34,7 +34,7 @@ func TestSuiteRequireTwice(t *testing.T) {
 			},
 		}},
 	)
-	assert.Equal(t, false, ok)
+	assert.False(t, ok)
 }
 
 func (s *SuiteRequireTwice) TestRequireOne() {
@@ -301,13 +301,13 @@ func TestRunSuite(t *testing.T) {
 
 	// The suite was only run once, so the SetupSuite and TearDownSuite
 	// methods should have each been run only once.
-	assert.Equal(t, suiteTester.SetupSuiteRunCount, 1)
-	assert.Equal(t, suiteTester.TearDownSuiteRunCount, 1)
+	assert.Equal(t, 1, suiteTester.SetupSuiteRunCount)
+	assert.Equal(t, 1, suiteTester.TearDownSuiteRunCount)
 
-	assert.Equal(t, len(suiteTester.SuiteNameAfter), 4)
-	assert.Equal(t, len(suiteTester.SuiteNameBefore), 4)
-	assert.Equal(t, len(suiteTester.TestNameAfter), 4)
-	assert.Equal(t, len(suiteTester.TestNameBefore), 4)
+	assert.Len(t, suiteTester.SuiteNameAfter, 4)
+	assert.Len(t, suiteTester.SuiteNameBefore, 4)
+	assert.Len(t, suiteTester.TestNameAfter, 4)
+	assert.Len(t, suiteTester.TestNameBefore, 4)
 
 	assert.Contains(t, suiteTester.TestNameAfter, "TestOne")
 	assert.Contains(t, suiteTester.TestNameAfter, "TestTwo")
@@ -338,20 +338,20 @@ func TestRunSuite(t *testing.T) {
 	// There are four test methods (TestOne, TestTwo, TestSkip, and TestSubtest), so
 	// the SetupTest and TearDownTest methods (which should be run once for
 	// each test) should have been run four times.
-	assert.Equal(t, suiteTester.SetupTestRunCount, 4)
-	assert.Equal(t, suiteTester.TearDownTestRunCount, 4)
+	assert.Equal(t, 4, suiteTester.SetupTestRunCount)
+	assert.Equal(t, 4, suiteTester.TearDownTestRunCount)
 
 	// Each test should have been run once.
-	assert.Equal(t, suiteTester.TestOneRunCount, 1)
-	assert.Equal(t, suiteTester.TestTwoRunCount, 1)
-	assert.Equal(t, suiteTester.TestSubtestRunCount, 1)
+	assert.Equal(t, 1, suiteTester.TestOneRunCount)
+	assert.Equal(t, 1, suiteTester.TestTwoRunCount)
+	assert.Equal(t, 1, suiteTester.TestSubtestRunCount)
 
-	assert.Equal(t, suiteTester.TearDownSubTestRunCount, 2)
-	assert.Equal(t, suiteTester.SetupSubTestRunCount, 2)
+	assert.Equal(t, 2, suiteTester.TearDownSubTestRunCount)
+	assert.Equal(t, 2, suiteTester.SetupSubTestRunCount)
 
 	// Methods that don't match the test method identifier shouldn't
 	// have been run at all.
-	assert.Equal(t, suiteTester.NonTestMethodRunCount, 0)
+	assert.Equal(t, 0, suiteTester.NonTestMethodRunCount)
 
 	suiteSkipTester := new(SuiteSkipTester)
 	Run(t, suiteSkipTester)
@@ -359,8 +359,8 @@ func TestRunSuite(t *testing.T) {
 	// The suite was only run once, so the SetupSuite and TearDownSuite
 	// methods should have each been run only once, even though SetupSuite
 	// called Skip()
-	assert.Equal(t, suiteSkipTester.SetupSuiteRunCount, 1)
-	assert.Equal(t, suiteSkipTester.TearDownSuiteRunCount, 1)
+	assert.Equal(t, 1, suiteSkipTester.SetupSuiteRunCount)
+	assert.Equal(t, 1, suiteSkipTester.TearDownSuiteRunCount)
 
 }
 
@@ -571,7 +571,7 @@ func TestFailfastSuite(t *testing.T) {
 			},
 		}},
 	)
-	assert.Equal(t, false, ok)
+	assert.False(t, ok)
 	if failFast {
 		// Test A Fails and because we are running with failfast Test B never runs and we proceed straight to TearDownSuite
 		assert.Equal(t, "SetupSuite;SetupTest;Test A Fails;TearDownTest;TearDownSuite", strings.Join(s.callOrder, ";"))