surface panic when calling Finish is implicit (#478)

When Finsh is called from Cleanup, it is not done so in a deffered
context. Because of this, we can't recover from panics. The
testing package will re-surface any panics but only after the
Cleanup functions is executed. Because we were calling t.Fatal we
were hiding panics from users of the library when they were using
Finish implicitly. Fatal calls runtime.Goexit() under the hood so
the panic never gets the chance to be handled by the testing
package.

Also, removed logging as after thinking about it more it seems a
little aggressive to push the feature onto users.

Note, when there is a panic and Finish is called from the Cleanup
function that a few extra lines will be logged from the calls to
ErrorF. This seems like a small price to pay to get the context
of the panic.

Author: codyoss

.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/mock_test.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.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)
+}