Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

ctrl.Finish() is not called in 1.14+ #447

Closed
sarpdoruk opened this issue Jun 13, 2020 · 7 comments · Fixed by #461
Closed

ctrl.Finish() is not called in 1.14+ #447

sarpdoruk opened this issue Jun 13, 2020 · 7 comments · Fixed by #461

Comments

@sarpdoruk
Copy link

It is stated that if go version is 1.14+ and *t.Testing is passed into the controller it is not needed to call ctrl.Finish().

Actual behavior
Although I'm using 1.14.3, ctrl.Finish() is not called when, considering Times(x), x is greater than the actual number calls and results in a successful test. This is not the case when x is smaller than the number of calls made then it throws the following error as expected;
expected call at ..._test.go:XX has already been called the max number of times

Expected behavior
When Times(x) is used to track number of calls and x is not equal to the number of calls, tests should fail.

To Reproduce

1- Use GO 1.14.3
2- Call a method 3 times in your actual code
3- Do not call ctrl.Finish() in your test
4- Test the following cases;

  • If you use Times(1) in your test, it fails. (As expected)
  • If you use Times(3) in your test, it passes. (As expected)
  • If you use Times(172863) in your test, it passes. (Not expected)

Additional Information

  • gomock mode (reflect or source): source
  • gomock version or git ref: 1.4.3
  • golang version: 1.14.3

Example
main.go

package main

import (
	"fmt"
	"io"
	"time"
)

type Sleeper interface {
	Sleep(d time.Duration)
}

type DefaultSleeper struct{}

func (s *DefaultSleeper) Sleep(d time.Duration) {
	time.Sleep(d)
}

func Countdown(w io.Writer, s Sleeper) {
	for i := 3; i > 0; i-- {
		fmt.Fprintf(w, "%d\n", i)
		s.Sleep(time.Second)
	}

	fmt.Fprint(w, "Go!")
}

func main() {
	Countdown(os.Stdout, &DefaultSleeper{})
}

main_test.go

package main

import (
	"bytes"
	"github.com/golang/mock/gomock"
	"testing"
	"time"
)

func TestCountdown(t *testing.T) {
	buffer := &bytes.Buffer{}
	ctrl := gomock.NewController(t)
	// defer ctrl.Finish()
	s := NewMockSleeper(ctrl)

	s.
		EXPECT().
		Sleep(gomock.Eq(time.Second)).
		Times(172863)

	Countdown(buffer, s)

	got := buffer.String()
	want := `3
2
1
Go!`

	if got != want {
		t.Errorf("got %q, want %q", got, want)
	}
}
@codyoss
Copy link
Member

codyoss commented Jun 15, 2020

Thanks for the report I will take a look.

@AndersonQ
Copy link

I was about to report this bug.

I did a bit of digging and turns out #422 is merged on master, but is not part of any release, therefore getting the latest as the Readme suggests (GO111MODULE=on go get github.com/golang/mock/mockgen@latest) will give you a release without the new feature.

Getting it from master, GO111MODULE=on go get github.com/golang/mock/mockgen@master gives you the feature.

IMHO the readme should match the latest release, which would avoid people being mislead. And then get a release including #422 and update the readme again.

@sarpdoruk
Copy link
Author

I was about to report this bug.

I did a bit of digging and turns out #422 is merged on master, but is not part of any release, therefore getting the latest as the Readme suggests (GO111MODULE=on go get github.com/golang/mock/mockgen@latest) will give you a release without the new feature.

Getting it from master, GO111MODULE=on go get github.com/golang/mock/mockgen@master gives you the feature.

IMHO the readme should match the latest release, which would avoid people being mislead. And then get a release including #422 and update the readme again.

Thank you @AndersonQ for pointing that out, appreciated.

@codyoss
Copy link
Member

codyoss commented Jun 23, 2020

I agree, I will update the readme soon. In the meantime please use GO111MODULE=on go get github.com/golang/mock/mockgen@v1.4.3. Still trying to figure out if there is a good way to support #422.

@AndersonQ
Copy link

@codyoss do you need any help? I'm happy to help :)

I'll take the risk and keep with either master and keep an eye to see if I spot any bug :)

@ash2k
Copy link

ash2k commented Jun 30, 2020

Releasing a new version of the library and documenting that version X or newer is required will probably help to avoid the confusion.

@hasryan
Copy link

hasryan commented Dec 10, 2020

Apologies for commenting on this closed issue. What I am describing is surely categorized as user error, but I suspect it could be impacting others so I wanted to describe my experience.

On a previous project I came to rely on Finish being called automatically, but when I took over another project recently I pulled in the most recent tagged version (v1.4.4 as of today) and mistakenly assumed it was doing the same. Only later did I realize my mistake: the previous project I worked on was pinned to a commit which is more recent than v1.4.4.

Would it be possible to create a new release that includes some of the recent changes? Full disclosure, the projects I'm working on are still using dep instead of native modules so it's certainly possible I am just behind the times and that switching would resolve my issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants