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

AtLeast(times int) implementation #1648

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chorankates-sfdc
Copy link

@chorankates-sfdc chorankates-sfdc commented Oct 3, 2024

Summary

this PR adds AtLeast(i int) to mocked objects, allowing tests to require that a function is called at least i times.

Changes

all in the mock package:

  • add function AtLeast(i int) to expose the feature
  • add private field minimumCalls int to record the value passed from the original caller
  • modify function checkExpectation to compare the number of calls to the minimum number of expected calls

Motivation

this change is necessary to know if a function has been called at least N times.

this is mostly useful when the count of function execution can't easily be determined (looking at you time.Sleep), so .Twice() or .Times(N) can't be used effectively.

this leads to code that instead uses .Maybe() and then later .AssertCalled() to make sure it was called at least once - but makes the logic a bit disconnected and provides no way to specify a reasonable number of times the function should at least be called.

here's a contrived example on how it can be used:

something.go

package doer

type Doer interface {
	DoSomething()
}

type RealDoer struct{}

func (r *RealDoer) DoSomething() {
	return
}

func targetFuncThatDoesSomething(d Doer, times int) {
	for i := 0; i < times; i++ {
		d.DoSomething()
	}
}

something_test.go

package doer

import (
	"testing"

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

type MockDoer struct {
	mock.Mock
}

func (m *MockDoer) DoSomething() {
	m.Called()
	return
}

// this function calls DoSomething 5 times, and is only expected to call it 3 times
func TestDoSomething_ThatDoesCallEnough(t *testing.T) {
	md := new(MockDoer)
	md.On("DoSomething").Return().AtLeast(3)

	targetFuncThatDoesSomething(md, 5)

	md.AssertExpectations(t)
}

// this function only calls DoSomething 1 time, but is expected to call it at least 3 times
func TestDoSomething_ThatDoesntCallEnough(t *testing.T) {
	md := new(MockDoer)
	md.On("DoSomething").Return().AtLeast(3)

	targetFuncThatDoesSomething(md, 1)

	md.AssertExpectations(t)
}

Problems

currently AssertExpectations specifies that every failure coming from checkExpectation indicates that one more call is necessary - which with this change will not always be true, and can lead to confusion.

i don't have a good solution to this problem, but am very open to feedback.

in the above example, you'd see output similar to:

=== RUN   TestDoSomething_ThatDoesCallEnough
--- PASS: TestDoSomething_ThatDoesCallEnough (0.00s)
=== RUN   TestDoSomething_ThatDoesntCallEnough
    something_test.go:33: FAIL:	DoSomething()
        		at: [/foo/bar/baz/something_test.go:29]
    something_test.go:33: FAIL: 0 out of 1 expectation(s) were met.
        	The code you are testing needs to make 1 more call(s).
        	at: [/foo/bar/baz/something_test.go:33]
--- FAIL: TestDoSomething_ThatDoesntCallEnough (0.00s)

FAIL

but in fact, the code being tested needs to make 2 more calls


additionally, while it doesn't matter in the contrived example or my first actual use case, setting Repeatability to 0 and always returning a value from the function could cause undesired behaviors in others.

@chorankates-sfdc chorankates-sfdc marked this pull request as draft October 3, 2024 23:14
@chorankates-sfdc chorankates-sfdc marked this pull request as ready for review October 4, 2024 01:21
@chorankates-sfdc
Copy link
Author

tests incoming

if this is useful - would AtMost(i int) make sense to be included?

@brackendawson
Copy link
Collaborator

brackendawson commented Oct 4, 2024

Before you get much further with this, the behaviour you are implementing can be described sub-optimally with this:

package kata_test

import (
	"strconv"
	"testing"

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

type myMock struct {
	mock.Mock
}

func (m *myMock) Do() {
	m.Called()
}

func TestIfy(t *testing.T) {
	for i := range 6 {
		t.Run(strconv.Itoa(i), func(t *testing.T) {
			m := &myMock{}
			m.Test(t)
			defer m.AssertExpectations(t)
			m.On("Do").Return().Times(3)
			m.On("Do").Return().Maybe()

			for range i {
				m.Do()
			}
		})
	}
}

The implementation of Mock's Call matching is already too complicated, but we cannot simplify the implementation without a breaking change because some of Mock's fields are exported. So we don't want to add any more code to the call matching if at all possible.

Before accepting this change I'd want to see a real world example of a good testing practice which needs it.

@dolmen dolmen added the pkg-assert Change related to package testify/assert label Oct 4, 2024
@brackendawson brackendawson added pkg-mock Any issues related to Mock and removed pkg-assert Change related to package testify/assert labels Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg-mock Any issues related to Mock
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants