From 2b8ec120432fc5161f7e5894570274be27d4b2e2 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Tue, 1 Sep 2020 13:53:38 -0600 Subject: [PATCH 1/3] surface panic when Finish is implicit 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 --- .travis.yml | 1 + ci/check_panic_handling.sh | 23 +++++++ gomock/controller.go | 27 ++++---- gomock/controller_114_test.go | 2 +- .../internal/tests/panicing_test/mock_test.go | 62 +++++++++++++++++++ mockgen/internal/tests/panicing_test/panic.go | 28 +++++++++ .../tests/panicing_test/panic_test.go | 40 ++++++++++++ 7 files changed, 171 insertions(+), 12 deletions(-) create mode 100755 ci/check_panic_handling.sh create mode 100644 mockgen/internal/tests/panicing_test/mock_test.go create mode 100644 mockgen/internal/tests/panicing_test/panic.go create mode 100644 mockgen/internal/tests/panicing_test/panic_test.go 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..f02b27bd --- /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 TestSomething_Panics_Explicit | grep "Danger, Will Robinson!" +go test -v -tags=panictest -run TestSomething_Panics_Implicit | grep "Danger, Will Robinson!" +popd diff --git a/gomock/controller.go b/gomock/controller.go index ee175cdd..0b225acd 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() + defer 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..5c2e31e2 --- /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 TestSomething_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 TestSomething_Panics_Implicit(t *testing.T) { + ctrl := gomock.NewController(t) + mock := NewMockFoo(ctrl) + mock.EXPECT().Bar().Return("Bar") + mock.EXPECT().Bar().Return("Baz") + Danger(mock) +} From d0178670e1acc5765f153e9c8335ef881b9662f8 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Tue, 1 Sep 2020 16:46:26 -0600 Subject: [PATCH 2/3] fix test name --- ci/check_panic_handling.sh | 4 ++-- mockgen/internal/tests/panicing_test/panic_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/check_panic_handling.sh b/ci/check_panic_handling.sh index f02b27bd..b910f873 100755 --- a/ci/check_panic_handling.sh +++ b/ci/check_panic_handling.sh @@ -18,6 +18,6 @@ set -eux pushd mockgen/internal/tests/panicing_test -go test -v -tags=panictest -run TestSomething_Panics_Explicit | grep "Danger, Will Robinson!" -go test -v -tags=panictest -run TestSomething_Panics_Implicit | grep "Danger, Will Robinson!" +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/mockgen/internal/tests/panicing_test/panic_test.go b/mockgen/internal/tests/panicing_test/panic_test.go index 5c2e31e2..238dece1 100644 --- a/mockgen/internal/tests/panicing_test/panic_test.go +++ b/mockgen/internal/tests/panicing_test/panic_test.go @@ -22,7 +22,7 @@ import ( "github.com/golang/mock/gomock" ) -func TestSomething_Panics_Explicit(t *testing.T) { +func TestDanger_Panics_Explicit(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mock := NewMockFoo(ctrl) @@ -31,7 +31,7 @@ func TestSomething_Panics_Explicit(t *testing.T) { Danger(mock) } -func TestSomething_Panics_Implicit(t *testing.T) { +func TestDanger_Panics_Implicit(t *testing.T) { ctrl := gomock.NewController(t) mock := NewMockFoo(ctrl) mock.EXPECT().Bar().Return("Bar") From 837ac56966f31a84226eda0f4cc644a9634c5610 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 3 Sep 2020 12:07:02 -0600 Subject: [PATCH 3/3] remove unneeded defer --- gomock/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gomock/controller.go b/gomock/controller.go index 0b225acd..3b656909 100644 --- a/gomock/controller.go +++ b/gomock/controller.go @@ -263,7 +263,7 @@ 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() - defer ctrl.finish(false, err) + ctrl.finish(false, err) } func (ctrl *Controller) finish(cleanup bool, panicErr interface{}) {