Repository: stretchr/testify (go)

Issue:
[ISSUE] mockCall.Unset cause panic
Hi team, I'm using testify to testing my method but encounter this issue (It only occur if using matchedBy arg)
When I using mock following by example but got error when using mockCall.Unset().
playground: https://go.dev/play/p/oQ2sGSRCtbR


Pull request description:
Fix Call.Unset() panic (issue #1236)
## Summary
`Unset()` doesn't work as expected in documentation. Instead of unsetting call specification it causes panic.

Reason: During call specification filtering `Unset()` changes len of a `ExpectedCalls` slice while using index from original slice. Under some circumstances it tries to reslice elements beyond of the slice boundaries. That causes panic.

## Changes
The proposed solution uses independent write index to count elements kept in result slice.
Tests (the simplest and  a more complicated cases) and comments are included.


<!-- ... -->

## Motivation
Because core functionality required by users to write tests doesn't work as expected.

<!-- ## Example usage (if applicable) -->

```golang
// Unset removes a mock handler from being called.
test.On("func", mock.Anything).Return(nil)  // OK
test.On("func", mock.Anything).Unset()      // panic
```

## Related issues
Closes #1236 


Patch:
diff --git a/assert/assertions.go b/assert/assertions.go
index fa1245b..361d0bd 100644
--- a/assert/assertions.go
+++ b/assert/assertions.go
@@ -530,7 +530,7 @@ func isNil(object interface{}) bool {
 		[]reflect.Kind{
 			reflect.Chan, reflect.Func,
 			reflect.Interface, reflect.Map,
-			reflect.Ptr, reflect.Slice},
+			reflect.Ptr, reflect.Slice, reflect.UnsafePointer},
 		kind)
 
 	if isNilableKind && value.IsNil() {
diff --git a/go.mod b/go.mod
index c5d3e88..3fe9bab 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,6 @@ go 1.13
 require (
 	github.com/davecgh/go-spew v1.1.1
 	github.com/pmezard/go-difflib v1.0.0
-	github.com/stretchr/objx v0.4.0
+	github.com/stretchr/objx v0.5.0
 	gopkg.in/yaml.v3 v3.0.1
 )
diff --git a/go.sum b/go.sum
index 3be384b..4f3ced6 100644
--- a/go.sum
+++ b/go.sum
@@ -4,9 +4,11 @@ github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
-github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
+github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
diff --git a/mock/mock.go b/mock/mock.go
index f0af824..e6ff8df 100644
--- a/mock/mock.go
+++ b/mock/mock.go
@@ -218,16 +218,22 @@ func (c *Call) Unset() *Call {
 
 	foundMatchingCall := false
 
-	for i, call := range c.Parent.ExpectedCalls {
+	// in-place filter slice for calls to be removed - iterate from 0'th to last skipping unnecessary ones
+	var index int // write index
+	for _, call := range c.Parent.ExpectedCalls {
 		if call.Method == c.Method {
 			_, diffCount := call.Arguments.Diff(c.Arguments)
 			if diffCount == 0 {
 				foundMatchingCall = true
-				// Remove from ExpectedCalls
-				c.Parent.ExpectedCalls = append(c.Parent.ExpectedCalls[:i], c.Parent.ExpectedCalls[i+1:]...)
+				// Remove from ExpectedCalls - just skip it
+				continue
 			}
 		}
+		c.Parent.ExpectedCalls[index] = call
+		index++
 	}
+	// trim slice up to last copied index
+	c.Parent.ExpectedCalls = c.Parent.ExpectedCalls[:index]
 
 	if !foundMatchingCall {
 		unlockOnce.Do(c.unlock)
diff --git a/suite/interfaces.go b/suite/interfaces.go
index 8b98a8a..fed037d 100644
--- a/suite/interfaces.go
+++ b/suite/interfaces.go
@@ -7,6 +7,7 @@ import "testing"
 type TestingSuite interface {
 	T() *testing.T
 	SetT(*testing.T)
+	SetS(suite TestingSuite)
 }
 
 // SetupAllSuite has a SetupSuite method, which will run before the
@@ -51,3 +52,15 @@ type AfterTest interface {
 type WithStats interface {
 	HandleStats(suiteName string, stats *SuiteInformation)
 }
+
+// SetupSubTest has a SetupSubTest method, which will run before each
+// subtest in the suite.
+type SetupSubTest interface {
+	SetupSubTest()
+}
+
+// TearDownSubTest has a TearDownSubTest method, which will run after
+// each subtest in the suite have been run.
+type TearDownSubTest interface {
+	TearDownSubTest()
+}
diff --git a/suite/suite.go b/suite/suite.go
index 8955918..8b4202d 100644
--- a/suite/suite.go
+++ b/suite/suite.go
@@ -22,9 +22,13 @@ var matchMethod = flag.String("testify.m", "", "regular expression to select tes
 // retrieving the current *testing.T context.
 type Suite struct {
 	*assert.Assertions
+
 	mu      sync.RWMutex
 	require *require.Assertions
 	t       *testing.T
+
+	// Parent suite to have access to the implemented methods of parent struct
+	s TestingSuite
 }
 
 // T retrieves the current *testing.T context.
@@ -43,6 +47,12 @@ func (suite *Suite) SetT(t *testing.T) {
 	suite.require = require.New(t)
 }
 
+// SetS needs to set the current test suite as parent
+// to get access to the parent methods
+func (suite *Suite) SetS(s TestingSuite) {
+	suite.s = s
+}
+
 // Require returns a require context for suite.
 func (suite *Suite) Require() *require.Assertions {
 	suite.mu.Lock()
@@ -85,7 +95,18 @@ func failOnPanic(t *testing.T, r interface{}) {
 // Provides compatibility with go test pkg -run TestSuite/TestName/SubTestName.
 func (suite *Suite) Run(name string, subtest func()) bool {
 	oldT := suite.T()
-	defer suite.SetT(oldT)
+
+	if setupSubTest, ok := suite.s.(SetupSubTest); ok {
+		setupSubTest.SetupSubTest()
+	}
+
+	defer func() {
+		suite.SetT(oldT)
+		if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok {
+			tearDownSubTest.TearDownSubTest()
+		}
+	}()
+
 	return oldT.Run(name, func(t *testing.T) {
 		suite.SetT(t)
 		subtest()
@@ -98,6 +119,7 @@ func Run(t *testing.T, suite TestingSuite) {
 	defer recoverAndFailOnPanic(t)
 
 	suite.SetT(t)
+	suite.SetS(suite)
 
 	var suiteSetupDone bool
 


Test patch:
diff --git a/assert/assertions_test.go b/assert/assertions_test.go
index bdab184..b5193a5 100644
--- a/assert/assertions_test.go
+++ b/assert/assertions_test.go
@@ -15,6 +15,7 @@ import (
 	"strings"
 	"testing"
 	"time"
+	"unsafe"
 )
 
 var (
@@ -2558,3 +2559,10 @@ func TestErrorAs(t *testing.T) {
 		})
 	}
 }
+
+func TestIsNil(t *testing.T) {
+	var n unsafe.Pointer = nil
+	if !isNil(n) {
+		t.Fatal("fail")
+	}
+}
diff --git a/mock/mock_test.go b/mock/mock_test.go
index f77dfe4..260bb9c 100644
--- a/mock/mock_test.go
+++ b/mock/mock_test.go
@@ -569,6 +569,80 @@ func Test_Mock_UnsetIfAlreadyUnsetFails(t *testing.T) {
 	assert.Equal(t, 0, len(mockedService.ExpectedCalls))
 }
 
+func Test_Mock_UnsetByOnMethodSpec(t *testing.T) {
+	// make a test impl object
+	var mockedService = new(TestExampleImplementation)
+
+	mock1 := mockedService.
+		On("TheExampleMethod", 1, 2, 3).
+		Return(0, nil)
+
+	assert.Equal(t, 1, len(mockedService.ExpectedCalls))
+	mock1.On("TheExampleMethod", 1, 2, 3).
+		Return(0, nil).Unset()
+
+	assert.Equal(t, 0, len(mockedService.ExpectedCalls))
+
+	assert.Panics(t, func() {
+		mock1.Unset()
+	})
+
+	assert.Equal(t, 0, len(mockedService.ExpectedCalls))
+}
+
+func Test_Mock_UnsetByOnMethodSpecAmongOthers(t *testing.T) {
+	// make a test impl object
+	var mockedService = new(TestExampleImplementation)
+
+	_, filename, line, _ := runtime.Caller(0)
+	mock1 := mockedService.
+		On("TheExampleMethod", 1, 2, 3).
+		Return(0, nil).
+		On("TheExampleMethodVariadic", 1, 2, 3, 4, 5).Once().
+		Return(nil)
+	mock1.
+		On("TheExampleMethodFuncType", Anything).
+		Return(nil)
+
+	assert.Equal(t, 3, len(mockedService.ExpectedCalls))
+	mock1.On("TheExampleMethod", 1, 2, 3).
+		Return(0, nil).Unset()
+
+	assert.Equal(t, 2, len(mockedService.ExpectedCalls))
+
+	expectedCalls := []*Call{
+		{
+			Parent:          &mockedService.Mock,
+			Method:          "TheExampleMethodVariadic",
+			Repeatability:   1,
+			Arguments:       []interface{}{1, 2, 3, 4, 5},
+			ReturnArguments: []interface{}{nil},
+			callerInfo:      []string{fmt.Sprintf("%s:%d", filename, line+4)},
+		},
+		{
+			Parent:          &mockedService.Mock,
+			Method:          "TheExampleMethodFuncType",
+			Arguments:       []interface{}{Anything},
+			ReturnArguments: []interface{}{nil},
+			callerInfo:      []string{fmt.Sprintf("%s:%d", filename, line+7)},
+		},
+	}
+
+	assert.Equal(t, 2, len(mockedService.ExpectedCalls))
+	assert.Equal(t, expectedCalls, mockedService.ExpectedCalls)
+}
+
+func Test_Mock_Unset_WithFuncPanics(t *testing.T) {
+	// make a test impl object
+	var mockedService = new(TestExampleImplementation)
+	mock1 := mockedService.On("TheExampleMethod", 1)
+	mock1.Arguments = append(mock1.Arguments, func(string) error { return nil })
+
+	assert.Panics(t, func() {
+		mock1.Unset()
+	})
+}
+
 func Test_Mock_Return(t *testing.T) {
 
 	// make a test impl object
diff --git a/suite/suite_test.go b/suite/suite_test.go
index 446029a..d684f52 100644
--- a/suite/suite_test.go
+++ b/suite/suite_test.go
@@ -151,14 +151,16 @@ type SuiteTester struct {
 	Suite
 
 	// Keep counts of how many times each method is run.
-	SetupSuiteRunCount    int
-	TearDownSuiteRunCount int
-	SetupTestRunCount     int
-	TearDownTestRunCount  int
-	TestOneRunCount       int
-	TestTwoRunCount       int
-	TestSubtestRunCount   int
-	NonTestMethodRunCount int
+	SetupSuiteRunCount      int
+	TearDownSuiteRunCount   int
+	SetupTestRunCount       int
+	TearDownTestRunCount    int
+	TestOneRunCount         int
+	TestTwoRunCount         int
+	TestSubtestRunCount     int
+	NonTestMethodRunCount   int
+	SetupSubTestRunCount    int
+	TearDownSubTestRunCount int
 
 	SuiteNameBefore []string
 	TestNameBefore  []string
@@ -255,6 +257,14 @@ func (suite *SuiteTester) TestSubtest() {
 	}
 }
 
+func (suite *SuiteTester) TearDownSubTest() {
+	suite.TearDownSubTestRunCount++
+}
+
+func (suite *SuiteTester) SetupSubTest() {
+	suite.SetupSubTestRunCount++
+}
+
 type SuiteSkipTester struct {
 	// Include our basic suite logic.
 	Suite
@@ -336,6 +346,9 @@ func TestRunSuite(t *testing.T) {
 	assert.Equal(t, suiteTester.TestTwoRunCount, 1)
 	assert.Equal(t, suiteTester.TestSubtestRunCount, 1)
 
+	assert.Equal(t, suiteTester.TearDownSubTestRunCount, 2)
+	assert.Equal(t, suiteTester.SetupSubTestRunCount, 2)
+
 	// Methods that don't match the test method identifier shouldn't
 	// have been run at all.
 	assert.Equal(t, suiteTester.NonTestMethodRunCount, 0)