Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mock can be deadlocked by a panic #1157

Merged

Conversation

brackendawson
Copy link
Contributor

@brackendawson brackendawson commented Feb 10, 2022

Summary

If an argumentMatcher function panics and AssertExpectations is deferred then the test would deadlock.

Changes

  • Capture panics from argumentMatcher functions and show them to the user as a difference

Motivation

It was reported that when AssertExpectations() is deferred and an argumentMatcher function panics, that the test will deadlock.

Example from linked issue:

import (
	"testing"

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

type Mock struct {
	mock.Mock
}

func (_m *Mock) F() {
	_m.Called()
}

func TestMatcherPanic(t *testing.T) {
  m := &Mock{}
  defer m.AssertExpectations(t)

  matcher := mock.MatchedBy(func(_ interface{}) bool {
    panic("my hovercraft is full of eels")
  })
  m.On("F", matcher)

  m.F()
}

After this change the output will now look like:

--- FAIL: TestMatcherPanic (0.00s)
    /Users/alandaws/src/github.com/brackendawson/kata/mock.go:264: 
        
        mock: Unexpected Method Call
        -----------------------------
        
        F()
        		
        
        The closest call I have is: 
        
        F(mock.argumentMatcher)
        		0: mock.argumentMatcher{fn:reflect.Value{typ:(*reflect.rtype)(0x126a520), ptr:(unsafe.Pointer)(0x12b5f88), flag:0x13}}
        
        Provided 1 arguments, mocked for 0 arguments
        Diff: 0: FAIL:  panic in argument matcher: my hovercraft is full of eels not matched by func(interface {}) bool
    /Users/alandaws/src/github.com/brackendawson/kata/panic.go:661: FAIL:	F(mock.argumentMatcher)
        		at: [kata_test.go:25]
    /Users/alandaws/src/github.com/brackendawson/kata/panic.go:661: FAIL: 0 out of 1 expectation(s) were met.
        	The code you are testing needs to make 1 more call(s).
        	at: [panic.go:661 testing.go:756 kata_test.go:14 kata_test.go:27]

I wonder if that is obvious enough?

Related issues

fixes #1155

If an argumentMatcher function panics and AssertExpectations is deferred then the test would deadlock.
@lzambarda
Copy link

@lzambarda lzambarda commented Jun 7, 2022

It would be really good to have this merged as it's really hard to write tests when you need to guess why it is hanging!

if matcher.Matches(actual) {
var matches bool
func() {
defer func() {
Copy link
Collaborator

@boyan-soubachov boyan-soubachov Jun 20, 2022

Choose a reason for hiding this comment

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

Could you remind me why we have to wrap the deferred function call in another function? Is it because of line 822?

Copy link
Contributor Author

@brackendawson brackendawson Jun 21, 2022

Choose a reason for hiding this comment

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

I chose to do that so that only a panic from the argument matcher function will be caught by this recover. By not having the outer func() any other panics in the testify codebase would be a false positive for a panic in the argument matcher.

Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Thank you for your contribution!

@boyan-soubachov boyan-soubachov merged commit c206b2e into stretchr:master Jun 23, 2022
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants