Deprecate the function ctrl.Finish() (#50)

This PR marks the function `ctrl.Finish` as `Deprecated`. It's no need to call this function starting from Go 1.14 when [T.Cleanup](https://pkg.go.dev/testing#T.Cleanup) was added.
uber-go · Sep 5, 2023 · 974b477 · 974b477
1 parent 665220d
commit 974b477
Show file tree

Hide file tree

Showing 17 changed files with 14 additions and 97 deletions.
diff --git a/README.md b/README.md
@@ -139,9 +139,6 @@ func SUT(f Foo) {
 func TestFoo(t *testing.T) {
   ctrl := gomock.NewController(t)
 
-  // Assert that Bar() is invoked.
-  defer ctrl.Finish()
-
   m := NewMockFoo(ctrl)
 
   // Asserts that the first and only call to Bar() is passed 99.
@@ -155,11 +152,6 @@ func TestFoo(t *testing.T) {
 }
 ```
 
-If you are using a Go version of 1.14+, a mockgen version of 1.5.0+, and are
-passing a *testing.T into `gomock.NewController(t)` you no longer need to call
-`ctrl.Finish()` explicitly. It will be called for you automatically from a self
-registered [Cleanup](https://pkg.go.dev/testing?tab=doc#T.Cleanup) function.
-
 ## Building Stubs
 
 ```go
@@ -176,7 +168,6 @@ func SUT(f Foo) {
 ```go
 func TestFoo(t *testing.T) {
   ctrl := gomock.NewController(t)
-  defer ctrl.Finish()
 
   m := NewMockFoo(ctrl)
 

diff --git a/gomock/controller.go b/gomock/controller.go
@@ -53,19 +53,16 @@ type cleanuper interface {
 //
 //	func TestFoo(t *testing.T) {
 //	  ctrl := gomock.NewController(t)
-//	  defer ctrl.Finish()
 //	  // ..
 //	}
 //
 //	func TestBar(t *testing.T) {
 //	  t.Run("Sub-Test-1", st) {
 //	    ctrl := gomock.NewController(st)
-//	    defer ctrl.Finish()
 //	    // ..
 //	  })
 //	  t.Run("Sub-Test-2", st) {
 //	    ctrl := gomock.NewController(st)
-//	    defer ctrl.Finish()
 //	    // ..
 //	  })
 //	})
@@ -81,11 +78,10 @@ type Controller struct {
 	finished      bool
 }
 
-// NewController returns a new Controller. It is the preferred way to create a
-// Controller.
+// NewController returns a new Controller. It is the preferred way to create a Controller.
 //
-// New in go1.14+, if you are passing a *testing.T into this function you no
-// longer need to call ctrl.Finish() in your test methods.
+// Passing [*testing.T] registers cleanup function to automatically call [Controller.Finish]
+// when the test and all its subtests complete.
 func NewController(t TestReporter, opts ...ControllerOption) *Controller {
 	h, ok := t.(TestHelper)
 	if !ok {
@@ -238,12 +234,11 @@ func (ctrl *Controller) Call(receiver any, method string, args ...any) []any {
 	return rets
 }
 
-// Finish checks to see if all the methods that were expected to be called
-// were called. It should be invoked for each Controller. It is not idempotent
-// and therefore can only be invoked once.
+// Finish checks to see if all the methods that were expected to be called were called.
+// It is not idempotent and therefore can only be invoked once.
 //
-// New in go1.14+, if you are passing a *testing.T into NewController function you no
-// longer need to call ctrl.Finish() in your test methods.
+// Deprecated: Calling this function in test methods is not required starting from Go 1.14.
+// It will be called automatically from a self registered [testing.T.Cleanup] function.
 func (ctrl *Controller) Finish() {
 	// If we're currently panicking, probably because this is a deferred call.
 	// This must be recovered in the deferred function.

diff --git a/gomock/controller_test.go b/gomock/controller_test.go
@@ -17,9 +17,8 @@ package gomock_test
 import (
 	"fmt"
 	"reflect"
-	"testing"
-
 	"strings"
+	"testing"
 
 	"go.uber.org/mock/gomock"
 )
@@ -177,8 +176,7 @@ func createFixtures(t *testing.T) (reporter *ErrorReporter, ctrl *gomock.Control
 }
 
 func TestNoCalls(t *testing.T) {
-	reporter, ctrl := createFixtures(t)
-	ctrl.Finish()
+	reporter, _ := createFixtures(t)
 	reporter.assertPass("No calls expected or made.")
 }
 
@@ -189,7 +187,6 @@ func TestNoRecordedCallsForAReceiver(t *testing.T) {
 	reporter.assertFatal(func() {
 		ctrl.Call(subject, "NotRecordedMethod", "argument")
 	}, "Unexpected call to", "there are no expected calls of the method \"NotRecordedMethod\" for that receiver")
-	ctrl.Finish()
 }
 
 func TestNoRecordedMatchingMethodNameForAReceiver(t *testing.T) {
@@ -213,7 +210,6 @@ func TestExpectedMethodCall(t *testing.T) {
 
 	ctrl.RecordCall(subject, "FooMethod", "argument")
 	ctrl.Call(subject, "FooMethod", "argument")
-	ctrl.Finish()
 
 	reporter.assertPass("Expected method call made.")
 }
@@ -225,8 +221,6 @@ func TestUnexpectedMethodCall(t *testing.T) {
 	reporter.assertFatal(func() {
 		ctrl.Call(subject, "FooMethod", "argument")
 	})
-
-	ctrl.Finish()
 }
 
 func TestRepeatedCall(t *testing.T) {
@@ -241,7 +235,6 @@ func TestRepeatedCall(t *testing.T) {
 	reporter.assertFatal(func() {
 		ctrl.Call(subject, "FooMethod", "argument")
 	})
-	ctrl.Finish()
 	reporter.assertFail("After calling one too many times.")
 }
 
@@ -388,7 +381,6 @@ func TestAnyTimes(t *testing.T) {
 		ctrl.Call(subject, "FooMethod", "argument")
 	}
 	reporter.assertPass("After 100 method calls.")
-	ctrl.Finish()
 }
 
 func TestMinTimes1(t *testing.T) {
@@ -514,8 +506,6 @@ func TestDo(t *testing.T) {
 	if wantArg != argument {
 		t.Error("Do callback received wrong argument.")
 	}
-
-	ctrl.Finish()
 }
 
 func TestDoAndReturn(t *testing.T) {
@@ -551,8 +541,6 @@ func TestDoAndReturn(t *testing.T) {
 	} else if ret != 5 {
 		t.Errorf("DoAndReturn return value: got %d, want 5", ret)
 	}
-
-	ctrl.Finish()
 }
 
 func TestSetArgSlice(t *testing.T) {
@@ -574,8 +562,6 @@ func TestSetArgSlice(t *testing.T) {
 	if !reflect.DeepEqual(in, set) {
 		t.Error("Expected SetArg() to modify input slice argument as any")
 	}
-
-	ctrl.Finish()
 }
 
 func TestSetArgMap(t *testing.T) {
@@ -597,8 +583,6 @@ func TestSetArgMap(t *testing.T) {
 	if !reflect.DeepEqual(in, set) {
 		t.Error("Expected SetArg() to modify input map argument as any")
 	}
-
-	ctrl.Finish()
 }
 
 func TestSetArgPtr(t *testing.T) {
@@ -620,7 +604,6 @@ func TestSetArgPtr(t *testing.T) {
 	if in != set {
 		t.Error("Expected SetArg() to modify value pointed to by argument as any")
 	}
-	ctrl.Finish()
 }
 
 func TestReturn(t *testing.T) {
@@ -640,7 +623,6 @@ func TestReturn(t *testing.T) {
 		t,
 		[]any{5},
 		ctrl.Call(subject, "FooMethod", "five"))
-	ctrl.Finish()
 }
 
 func TestUnorderedCalls(t *testing.T) {
@@ -707,7 +689,6 @@ func TestPanicOverridesExpectationChecks(t *testing.T) {
 
 func TestSetArgWithBadType(t *testing.T) {
 	rep, ctrl := createFixtures(t)
-	defer ctrl.Finish()
 
 	s := new(Subject)
 	// This should catch a type error:
@@ -719,7 +700,6 @@ func TestSetArgWithBadType(t *testing.T) {
 
 func TestTimes0(t *testing.T) {
 	rep, ctrl := createFixtures(t)
-	defer ctrl.Finish()
 
 	s := new(Subject)
 	ctrl.RecordCall(s, "FooMethod", "arg").Times(0)
@@ -735,7 +715,6 @@ func TestVariadicMatching(t *testing.T) {
 	s := new(Subject)
 	ctrl.RecordCall(s, "VariadicMethod", 0, "1", "2")
 	ctrl.Call(s, "VariadicMethod", 0, "1", "2")
-	ctrl.Finish()
 	rep.assertPass("variadic matching works")
 }
 
@@ -750,7 +729,6 @@ func TestVariadicNoMatch(t *testing.T) {
 	}, "expected call at", "doesn't match the argument at index 0",
 		"Got: 1 (int)\nWant: is equal to 0 (int)")
 	ctrl.Call(s, "VariadicMethod", 0)
-	ctrl.Finish()
 }
 
 func TestVariadicMatchingWithSlice(t *testing.T) {
@@ -771,7 +749,6 @@ func TestVariadicMatchingWithSlice(t *testing.T) {
 				args[i+1] = arg
 			}
 			ctrl.Call(s, "VariadicMethod", args...)
-			ctrl.Finish()
 			rep.assertPass("slices can be used as matchers for variadic arguments")
 		})
 	}
@@ -798,7 +775,6 @@ func TestVariadicArgumentsGotFormatter(t *testing.T) {
 	}, "expected call to", "doesn't match the argument at index 0",
 		"Got: test{1}\nWant: is equal to 0")
 	ctrl.Call(s, "VariadicMethod", 0)
-	ctrl.Finish()
 }
 
 func TestVariadicArgumentsGotFormatterTooManyArgsFailure(t *testing.T) {
@@ -823,7 +799,6 @@ func TestVariadicArgumentsGotFormatterTooManyArgsFailure(t *testing.T) {
 	}, "expected call to", "doesn't match the argument at index 1",
 		"Got: test{[2 3]}\nWant: is equal to 1")
 	ctrl.Call(s, "VariadicMethod", 0, "1")
-	ctrl.Finish()
 }
 
 func TestNoHelper(t *testing.T) {
@@ -854,22 +829,7 @@ func TestMultipleDefers(t *testing.T) {
 	reporter.Cleanup(func() {
 		reporter.assertPass("No errors for multiple calls to Finish")
 	})
-	ctrl := gomock.NewController(reporter)
-	ctrl.Finish()
-}
-
-// Equivalent to the TestNoRecordedCallsForAReceiver, but without explicitly
-// calling Finish.
-func TestDeferNotNeededFail(t *testing.T) {
-	reporter := NewErrorReporter(t)
-	subject := new(Subject)
-	var ctrl *gomock.Controller
-	reporter.Cleanup(func() {
-		reporter.assertFatal(func() {
-			ctrl.Call(subject, "NotRecordedMethod", "argument")
-		}, "Unexpected call to", "there are no expected calls of the method \"NotRecordedMethod\" for that receiver")
-	})
-	ctrl = gomock.NewController(reporter)
+	_ = gomock.NewController(reporter)
 }
 
 func TestDeferNotNeededPass(t *testing.T) {

diff --git a/gomock/doc.go b/gomock/doc.go
@@ -23,7 +23,7 @@
 //	(2) Use mockgen to generate a mock from the interface.
 //	(3) Use the mock in a test:
 //	      func TestMyThing(t *testing.T) {
-//	        mockCtrl := gomock.NewController(t)//
+//	        mockCtrl := gomock.NewController(t)
 //	        mockObj := something.NewMockMyInterface(mockCtrl)
 //	        mockObj.EXPECT().SomeMethod(4, "blah")
 //	        // pass mockObj to a real object and play with it.

diff --git a/gomock/matchers_test.go b/gomock/matchers_test.go
@@ -74,7 +74,6 @@ func TestMatchers(t *testing.T) {
 // A more thorough test of notMatcher
 func TestNotMatcher(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	mockMatcher := mock_gomock.NewMockMatcher(ctrl)
 	notMatcher := gomock.Not(mockMatcher)
@@ -98,9 +97,6 @@ type ctxKey struct{}
 
 // A thorough test of assignableToTypeOfMatcher
 func TestAssignableToTypeOfMatcher(t *testing.T) {
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
 	aStr := "def"
 	anotherStr := "ghi"
 

diff --git a/mockgen/internal/tests/aux_imports_embedded_interface/bugreport_test.go b/mockgen/internal/tests/aux_imports_embedded_interface/bugreport_test.go
@@ -9,7 +9,6 @@ import (
 // TestValidInterface assesses whether or not the generated mock is valid
 func TestValidInterface(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	s := NewMockSource(ctrl)
 	s.EXPECT().Method().Return("")

diff --git a/mockgen/internal/tests/custom_package_name/greeter/greeter_test.go b/mockgen/internal/tests/custom_package_name/greeter/greeter_test.go
@@ -9,7 +9,6 @@ import (
 
 func TestGreeter_Greet(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	input := client.GreetInput{
 		Name: "Foo",

diff --git a/mockgen/internal/tests/generated_identifier_conflict/bugreport_test.go b/mockgen/internal/tests/generated_identifier_conflict/bugreport_test.go
@@ -1,8 +1,9 @@
 package bugreport
 
 import (
-	"go.uber.org/mock/gomock"
 	"testing"
+
+	"go.uber.org/mock/gomock"
 )
 
 func TestExample_Method(t *testing.T) {
@@ -11,8 +12,6 @@ func TestExample_Method(t *testing.T) {
 	m.EXPECT().Method(1, 2, 3, 4)
 
 	m.Method(1, 2, 3, 4)
-
-	ctrl.Finish()
 }
 
 func TestExample_VarargMethod(t *testing.T) {
@@ -21,6 +20,4 @@ func TestExample_VarargMethod(t *testing.T) {
 	m.EXPECT().VarargMethod(1, 2, 3, 4, 6, 7)
 
 	m.VarargMethod(1, 2, 3, 4, 6, 7)
-
-	ctrl.Finish()
 }
diff --git a/mockgen/internal/tests/generics/source/mock_external_test.go b/mockgen/internal/tests/generics/source/mock_external_test.go
@@ -10,7 +10,6 @@ import (
 
 func TestMockEmbeddingIface_One(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	m := NewMockEmbeddingIface[int, float64](ctrl)
 	m.EXPECT().One("foo").Return("bar")
@@ -21,7 +20,6 @@ func TestMockEmbeddingIface_One(t *testing.T) {
 
 func TestMockUniverse_Water(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	m := NewMockUniverse[int](ctrl)
 	m.EXPECT().Water(1024)
@@ -30,7 +28,6 @@ func TestMockUniverse_Water(t *testing.T) {
 
 func TestNewMockGroup_Join(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	m := NewMockGroup[generics.Generator[any]](ctrl)
 	ctx := context.TODO()

diff --git a/mockgen/internal/tests/import_embedded_interface/bugreport_test.go b/mockgen/internal/tests/import_embedded_interface/bugreport_test.go
@@ -23,7 +23,6 @@ import (
 // TestValidInterface assesses whether or not the generated mock is valid
 func TestValidInterface(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	s := NewMockSource(ctrl)
 	s.EXPECT().Ersatz().Return("")

diff --git a/mockgen/internal/tests/import_embedded_interface/net_test.go b/mockgen/internal/tests/import_embedded_interface/net_test.go
@@ -23,7 +23,6 @@ import (
 // TestValidInterface assesses whether or not the generated mock is valid
 func TestValidNetInterface(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	s := NewMockNet(ctrl)
 	s.EXPECT().WriteHeader(10)

diff --git a/mockgen/internal/tests/overlapping_methods/overlap_test.go b/mockgen/internal/tests/overlapping_methods/overlap_test.go
@@ -10,7 +10,6 @@ import (
 // TestValidInterface assesses whether or not the generated mock is valid
 func TestValidInterface(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
 	s := NewMockReadWriteCloser(ctrl)
 	s.EXPECT().Close().Return(errors.New("test"))