Skip to content

Commit

Permalink
Fix Call.Unset() panic (issue #1236) (#1250)
Browse files Browse the repository at this point in the history
Unset changes len of a `ExpectedCalls` slice during iteration while using index from original slice, and under some conditions it tries to reslice element beyond of the slice boundaries. That causes panic.

The proposed solution uses independent write index to count elements kept in output slice.

Tests (the simplest and more complicated cases) and comments are included.
  • Loading branch information
lisitsky authored Jan 3, 2023
1 parent 9acc222 commit 2b00d33
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 3 deletions.
12 changes: 9 additions & 3 deletions mock/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
74 changes: 74 additions & 0 deletions mock/mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

1 comment on commit 2b00d33

@lefinal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boyan-soubachov When can we expect this to be released?

Please sign in to comment.