diff --git a/.travis.yml b/.travis.yml index 83c7da98..512e5cf6 100644 --- a/.travis.yml +++ b/.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 ./... diff --git a/ci/check_panic_handling.sh b/ci/check_panic_handling.sh new file mode 100755 index 00000000..b910f873 --- /dev/null +++ b/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 diff --git a/gomock/controller.go b/gomock/controller.go index ee175cdd..3b656909 100644 --- a/gomock/controller.go +++ b/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)") } } diff --git a/gomock/controller_114_test.go b/gomock/controller_114_test.go index b6c325b1..dcefa096 100644 --- a/gomock/controller_114_test.go +++ b/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() diff --git a/mockgen/internal/tests/panicing_test/mock_test.go b/mockgen/internal/tests/panicing_test/mock_test.go new file mode 100644 index 00000000..5c7ef668 --- /dev/null +++ b/mockgen/internal/tests/panicing_test/mock_test.go @@ -0,0 +1,62 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: panic.go + +// Package paniccode is a generated GoMock package. +package paniccode + +import ( + reflect "reflect" + + gomock "github.com/golang/mock/gomock" +) + +// MockFoo is a mock of Foo interface. +type MockFoo struct { + ctrl *gomock.Controller + recorder *MockFooMockRecorder +} + +// MockFooMockRecorder is the mock recorder for MockFoo. +type MockFooMockRecorder struct { + mock *MockFoo +} + +// NewMockFoo creates a new mock instance. +func NewMockFoo(ctrl *gomock.Controller) *MockFoo { + mock := &MockFoo{ctrl: ctrl} + mock.recorder = &MockFooMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFoo) EXPECT() *MockFooMockRecorder { + return m.recorder +} + +// Bar mocks base method. +func (m *MockFoo) Bar() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Bar") + ret0, _ := ret[0].(string) + return ret0 +} + +// Bar indicates an expected call of Bar. +func (mr *MockFooMockRecorder) Bar() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Bar", reflect.TypeOf((*MockFoo)(nil).Bar)) +} + +// Baz mocks base method. +func (m *MockFoo) Baz() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Baz") + ret0, _ := ret[0].(string) + return ret0 +} + +// Baz indicates an expected call of Baz. +func (mr *MockFooMockRecorder) Baz() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Baz", reflect.TypeOf((*MockFoo)(nil).Baz)) +} diff --git a/mockgen/internal/tests/panicing_test/panic.go b/mockgen/internal/tests/panicing_test/panic.go new file mode 100644 index 00000000..78348b85 --- /dev/null +++ b/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!") + } +} diff --git a/mockgen/internal/tests/panicing_test/panic_test.go b/mockgen/internal/tests/panicing_test/panic_test.go new file mode 100644 index 00000000..238dece1 --- /dev/null +++ b/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) +}