surface panic when calling Finish is implicit

.travis.yml

@@ -17,4 +17,5 @@ script:
   - ./ci/check_go_lint.sh
   - ./ci/check_go_generate.sh
   - ./ci/check_go_mod.sh
+  - ./ci/check_panic_handling.sh
   - go test -v ./...

ci/check_panic_handling.sh

@@ -0,0 +1,23 @@
+#!/bin/bash
+# Copyright 2010 Google LLC.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# This script is used to ensure that panics are properly reported in tests.
+
+set -eux
+
+pushd mockgen/internal/tests/panicing_test
+go test -v -tags=panictest -run TestDanger_Panics_Explicit | grep "Danger, Will Robinson!"
+go test -v -tags=panictest -run TestDanger_Panics_Implicit | grep "Danger, Will Robinson!"
+popd

gomock/controller.go

@@ -136,7 +136,7 @@ func NewController(t TestReporter) *Controller {
 	if c, ok := isCleanuper(ctrl.T); ok {
 		c.Cleanup(func() {
 			ctrl.T.Helper()
-			ctrl.Finish()
+			ctrl.finish(true, nil)
 		})
 	}
 
@@ -260,6 +260,13 @@ func (ctrl *Controller) Call(receiver interface{}, method string, args ...interf
 // were called. It should be invoked for each Controller. It is not idempotent
 // and therefore can only be invoked once.
 func (ctrl *Controller) Finish() {
+	// If we're currently panicking, probably because this is a deferred call.
+	// This must be recovered in the deferred function.
+	err := recover()
+	ctrl.finish(false, err)
+}
+
+func (ctrl *Controller) finish(cleanup bool, panicErr interface{}) {
 	ctrl.T.Helper()
 
 	ctrl.mu.Lock()
@@ -269,19 +276,13 @@ func (ctrl *Controller) Finish() {
 		if _, ok := isCleanuper(ctrl.T); !ok {
 			ctrl.T.Fatalf("Controller.Finish was called more than once. It has to be called exactly once.")
 		}
-		// provide a log message to guide users to remove `defer ctrl.Finish()` in Go 1.14+
-		tr := unwrapTestReporter(ctrl.T)
-		if l, ok := tr.(interface{ Log(args ...interface{}) }); ok {
-			l.Log("In Go 1.14+ you no longer need to `ctrl.Finish()` if a *testing.T is passed to `NewController(...)`")
-		}
 		return
 	}
 	ctrl.finished = true
 
-	// If we're currently panicking, probably because this is a deferred call,
-	// pass through the panic.
-	if err := recover(); err != nil {
-		panic(err)
+	// Short-circuit, pass through the panic.
+	if panicErr != nil {
+		panic(panicErr)
 	}
 
 	// Check that all remaining expected calls are satisfied.
@@ -290,7 +291,11 @@ func (ctrl *Controller) Finish() {
 		ctrl.T.Errorf("missing call(s) to %v", call)
 	}
 	if len(failures) != 0 {
-		ctrl.T.Fatalf("aborting test due to missing call(s)")
+		if !cleanup {
+			ctrl.T.Fatalf("aborting test due to missing call(s)")
+			return
+		}
+		ctrl.T.Errorf("aborting test due to missing call(s)")
 	}
 }
 

gomock/controller_114_test.go

@@ -30,7 +30,7 @@ func (e *ErrorReporter) Cleanup(f func()) {
 func TestMultipleDefers(t *testing.T) {
 	reporter := NewErrorReporter(t)
 	reporter.Cleanup(func() {
-		reporter.assertLogf("In Go 1.14+ you no longer need to `ctrl.Finish()` if a *testing.T is passed to `NewController(...)`")
+		reporter.assertPass("No errors for multiple calls to Finish")
 	})
 	ctrl := gomock.NewController(reporter)
 	ctrl.Finish()

mockgen/internal/tests/panicing_test/panic.go

@@ -0,0 +1,28 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package paniccode
+
+//go:generate mockgen --source=panic.go --destination=mock_test.go --package=paniccode
+
+type Foo interface {
+	Bar() string
+	Baz() string
+}
+
+func Danger(f Foo) {
+	if f.Bar() == "Bar" {
+		panic("Danger, Will Robinson!")
+	}
+}

mockgen/internal/tests/panicing_test/panic_test.go

@@ -0,0 +1,40 @@
+// +build panictest
+
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package paniccode
+
+import (
+	"testing"
+
+	"github.com/golang/mock/gomock"
+)
+
+func TestDanger_Panics_Explicit(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	mock := NewMockFoo(ctrl)
+	mock.EXPECT().Bar().Return("Bar")
+	mock.EXPECT().Bar().Return("Baz")
+	Danger(mock)
+}
+
+func TestDanger_Panics_Implicit(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	mock := NewMockFoo(ctrl)
+	mock.EXPECT().Bar().Return("Bar")
+	mock.EXPECT().Bar().Return("Baz")
+	Danger(mock)
+}