Data race when mock is called with refernce to same object as expectation

[brackendawson]

In #423 @fterrag pointed out a different cause of a data race, I'm raising that here.

Description

If mock.Mock.Called is passed a pointer to the same object used in the call to mock.Mock.On from a differnt goroutine then there is a race. This seems like a really niche situation but it's the only option if you are testing asynchronous code which:

1. Receives a reference to an object.
2. Unpredictably mutates that object.
3. Sends that object to a mocked interface.

I think we can prevent the race where the test otherwise passes.

Step To Reproduce

The originl reporter's code:

package main

import (
    "sync"
    "testing"

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

type MyMockedObject struct {
    mock.Mock
}

type data struct {
    foo string
}

func (m *MyMockedObject) DoSomething(d *data) (bool, error) {
    args := m.Called(d)
    return args.Bool(0), args.Error(1)
}

func TestSomething(t *testing.T) {
    testObj := &MyMockedObject{}

    var wg sync.WaitGroup

    d := &data{}
    wg.Add(1)
    testObj.On("DoSomething", d).Run(func(args mock.Arguments) {
        wg.Done()
    }).Return(true, nil).Once()

    d2 := &data{}
    wg.Add(1)
    testObj.On("DoSomething", d2).Run(func(args mock.Arguments) {
        wg.Done()
    }).Return(true, nil).Once()

    go func() {
        d.foo = "bar"
        testObj.DoSomething(d)
    }()

    go func() {
        d2.foo = "baz"
        testObj.DoSomething(d2)
    }()

    wg.Wait()

    testObj.AssertExpectations(t)
}

Expected behaviour

Test passes

Actual behaviour

Data race.

[brackendawson]

I suggested a workaround:

package kata_test

import (
    "math/rand"
    "strconv"
    "sync"
    "testing"

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

type SomethingDoer interface {
    DoSomething(*data) (bool, error)
}

type MyMockedObject struct {
    mock.Mock
}

type data struct {
    foo string
}

func (m *MyMockedObject) DoSomething(d *data) (bool, error) {
    args := m.Called(d)
    return args.Bool(0), args.Error(1)
}

func TestSomething(t *testing.T) {
    testObj := &MyMockedObject{}

    var wg sync.WaitGroup

    d := &data{}
    wg.Add(1)
    testObj.On("DoSomething", mock.MatchedBy(func(arg *data) bool {
        return d == arg // just compare the pointer value
    })).Run(func(args mock.Arguments) {
        wg.Done()
    }).Return(true, nil).Once()

    d2 := &data{}
    wg.Add(1)
    testObj.On("DoSomething", mock.MatchedBy(func(arg *data) bool {
        return d2 == arg
    })).Run(func(args mock.Arguments) {
        wg.Done()
    }).Return(true, nil).Once()

    CodeUnderTest(d, d2, testObj)

    wg.Wait()

    testObj.AssertExpectations(t)
}

func CodeUnderTest(d, d2 *data, s SomethingDoer) {
    go func() {
        d.foo = strconv.FormatInt(rand.Int63(), 10) // can't predict this mutation
        s.DoSomething(d)
    }()

    go func() {
        d2.foo = strconv.FormatInt(rand.Int63(), 10)
        s.DoSomething(d2)
    }()
}

I thought the principle of this workaround could be applied in testify, but no; in the case when expected and actual don't match, then any generic code (incliding DeepEqual) will walk into the racy struct. Here was my attempt: https://github.com/brackendawson/testify/commit/a2de985f9d465ad5896ee8cdcc1851888ca22d06

The only way to work around this is code written for the type being compared which knows you're looking for the same instance of the object, and knows when to stop comparing values under pointers. I think this has to be a limitation.