ovechkin-dm / mockio

Mockito for golang
MIT License
73 stars 2 forks source link

Argument Matchers are being reused across mocks #36

Closed Zurvarian closed 9 months ago

Zurvarian commented 9 months ago

The issue is a bit difficult to spot, but the problem is that Argument Matchers are being re-used across mocks, so if you define two mocks of two different interfaces in the same test with different matchers (e.g.: first AnyString() and second AnyInt) the AnyInt matcher will be used to verify the calls of the first Mock instance.

func TestGetAllFromServiceAUsingMockio(t *testing.T) {
    mockio.SetUp(t)
    mockServiceA := mockio.Mock[test.APIServiceA]()

    mockio.When(mockServiceA.Add(mockio.AnyString()))

    mockServiceX := mockio.Mock[test.APIServiceX]()
    mockServiceX.SendMessage("TEST2")

    s := OtherService{
        serviceA: mockServiceA,
        serviceX: mockServiceX,
    }

    err := s.AddAllFromServiceA([]string{"TEST", "OTHER"})

    require.NoError(t, err)

    mockio.Verify(mockServiceA, mockio.Once()).Add("TEST") // This line here fails to verify.
    mockio.Verify(mockServiceA, mockio.Once()).Add("OTHER")
    mockio.Verify(mockServiceX, mockio.Once()).IncrementCounterBy(1)
    mockio.VerifyNoMoreInteractions(mockServiceA)
    mockio.VerifyNoMoreInteractions(mockServiceX)
}

I've debugged the code and found that the reason behind this issue is that the matchers are assigned to the Thread State when the When methods are invoked BUT they are not bound to the mock being "mocked" but rather to the thread state.

So when the invocation of mockio.AnyInt() happens it is overriding the matcher defined just right above.

I think that the solution should be to bind the matchers to the Handler itself when the mockio.AnyString() is invoked, rather than saving it into a thread scope.

ovechkin-dm commented 9 months ago

It is an interesting scenario, and should not be hard to fix. I will look into it after the weekend.

ovechkin-dm commented 9 months ago

So I wasn't able to reproduce this behavior. In the issue you mentioning AnyInt(), but I do not see the usage of AnyInt() in the provided example. You can review the MR that I created. Also can you provide a full failing example with all the interfaces and structs declarations?

Zurvarian commented 9 months ago

I see that I copied the wrong sample, sorry about that, here it is the one that failed:

func TestGetAllFromServiceAUsingMockio(t *testing.T) {
    mockio.SetUp(t)
    mockServiceA := mockio.Mock[APIServiceA]()

    mockio.When(mockServiceA.Add(mockio.AnyString()))

    mockServiceX := mockio.Mock[APIServiceX]()
    mockServiceX.IncrementCounterBy(mockio.AnyInt())

    s := OtherService{
        serviceA: mockServiceA,
        serviceX: mockServiceX,
    }

    err := s.AddAllFromServiceA([]string{"TEST", "OTHER"})

    require.NoError(t, err)

    mockio.Verify(mockServiceA, mockio.Once()).Add("TEST")
    mockio.Verify(mockServiceA, mockio.Once()).Add("OTHER")
    mockio.Verify(mockServiceX, mockio.Once()).IncrementCounterBy(1)
    mockio.VerifyNoMoreInteractions(mockServiceA)
}

Interfaces

type APIServiceA interface {
    GetAll() ([]string, error)
    Delete(ID string) error
    Add(ID string) error
}

type APIServiceX interface {
    SendMessage(message string) error
    IncrementCounterBy(incr int) error
}

These are totally made up interfaces for the sake of testing your library, so that's why the names are like A and X.

ovechkin-dm commented 9 months ago

Thanks, can you also provide an implementation of OtherService ?

Zurvarian commented 9 months ago

Yes, good point.

type OtherService struct {
    serviceA test.APIServiceA
    serviceX test.APIServiceX
}

func (s *OtherService) AddAllFromServiceA(IDs []string) error {
    var err error

    for _, ID := range IDs {
        err = s.serviceA.Add(ID)
        if err != nil {
            return err
        }
    }

    s.serviceX.IncrementCounterBy(len(IDs))

    return nil
}

This is simulating the case when you upsert some amount of data and then increment a counter for audit (which is a fairly common case)

ovechkin-dm commented 9 months ago

It is reproducible now, thanks. The expected behavior there should be the unexpected use of matchers error. Because Matchers in this library (and in mockito) should only be used inside When call.

Zurvarian commented 9 months ago

I see, my mistake when identifying the issue (I read this tons of times and still I didn't realize of the missing mockio.When())

Thanks for the reply :)

ovechkin-dm commented 9 months ago

You made a good point though, I will create a fix that will fail the test with correct error in such cases