From 59a9c6ee9308a77981d3c3b4b789e1694e48e720 Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 26 Mar 2019 12:37:15 -0700 Subject: [PATCH 1/7] Fix error_test.go The output of Stack() isn't stable across versions of Go and shouldn't be relied on. That makes the tests fail against Go 1.10, 1.11, and 1.12 (possibly earlier too). Instead, the stack frames in Error should be verified against the stack frames supplied by the runtime. Although the stack frames may vary from runtime impl to impl and version to version, we are comparing like for like so the tests shouldn't break. --- errors/error_test.go | 45 ++++++++++++++++++++++++++------------------ errors/stackframe.go | 28 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/errors/error_test.go b/errors/error_test.go index 55cd0653..e71f9feb 100644 --- a/errors/error_test.go +++ b/errors/error_test.go @@ -4,28 +4,39 @@ import ( "bytes" "fmt" "io" - "runtime/debug" + "reflect" + "runtime" "testing" ) -func TestStackFormatMatches(t *testing.T) { +type prettyStack []StackFrame +func (s prettyStack) String() string { + buf := &bytes.Buffer{} + for _, f := range s { + _, _ = fmt.Fprintf(buf, "%#v\n", f) + } + return buf.String() +} + +func callerFrames(skip int) []runtime.Frame { + callers := make([]uintptr, MaxStackDepth) + n := runtime.Callers(2+skip, callers) + return pcsToFrames(callers[:n]) +} + +func TestStackFrameMatches(t *testing.T) { defer func() { err := recover() if err != 'a' { t.Fatal(err) } - bs := [][]byte{Errorf("hi").Stack(), debug.Stack()} - - // Ignore the first line (as it contains the PC of the .Stack() call) - bs[0] = bytes.SplitN(bs[0], []byte("\n"), 2)[1] - bs[1] = bytes.SplitN(bs[1], []byte("\n"), 2)[1] + expected := runtimeToErrorFrames(callerFrames(0))[1:] + got := Errorf("hi").StackFrames()[1:] - if bytes.Compare(bs[0], bs[1]) != 0 { - t.Errorf("Stack didn't match") - t.Errorf("%s", bs[0]) - t.Errorf("%s", bs[1]) + if !reflect.DeepEqual(expected, got) { + t.Errorf("Stacks didn't match\nGot:\n%v\nExpected:\n%v", prettyStack(got), prettyStack(expected)) } }() @@ -33,19 +44,17 @@ func TestStackFormatMatches(t *testing.T) { } func TestSkipWorks(t *testing.T) { - defer func() { err := recover() if err != 'a' { t.Fatal(err) } - bs := [][]byte{New("hi", 2).Stack(), debug.Stack()} + expected := runtimeToErrorFrames(callerFrames(2)) + got := New("hi", 2).StackFrames() - if !bytes.HasSuffix(bs[1], bs[0]) { - t.Errorf("Stack didn't match") - t.Errorf("%s", bs[0]) - t.Errorf("%s", bs[1]) + if !reflect.DeepEqual(expected, got) { + t.Errorf("Stacks didn't match\nGot:\n%v\nExpected:\n%v", prettyStack(got), prettyStack(expected)) } }() @@ -130,7 +139,7 @@ func ExampleError_Stack() { fmt.Printf("Stack is %d bytes", len(e.Stack())) // Output: // Error: Oh noes! - // Stack is 589 bytes + // Stack is 505 bytes } func a() error { diff --git a/errors/stackframe.go b/errors/stackframe.go index 4edadbc5..7ed19223 100644 --- a/errors/stackframe.go +++ b/errors/stackframe.go @@ -95,3 +95,31 @@ func packageAndName(fn *runtime.Func) (string, string) { name = strings.Replace(name, "·", ".", -1) return pkg, name } + +func pcsToFrames(pcs []uintptr) []runtime.Frame { + frames := runtime.CallersFrames(pcs) + s := make([]runtime.Frame, 0, len(pcs)) + for { + frame, more := frames.Next() + s = append(s, frame) + if !more { + break + } + } + return s +} + +func runtimeToErrorFrames(rtFrames []runtime.Frame) []StackFrame { + frames := make([]StackFrame, len(rtFrames)) + for i, f := range rtFrames { + pkg, name := packageAndName(f.Func) + frames[i] = StackFrame{ + File: f.File, + LineNumber: f.Line, + Name: name, + Package: pkg, + ProgramCounter: f.PC, + } + } + return frames +} From ecc7eeb2e9a7e56b2c8edec988b8d65ddd1a788c Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 26 Mar 2019 12:45:16 -0700 Subject: [PATCH 2/7] Fix failing error_test The stack frame information in Error were got by various functions in the runtime pkg. This is explicitly discouraged in the [docs](https://golang.org/pkg/runtime/#Callers) which instead suggest to use runtime.CallersFrames to translate PCs to symbolic info. --- errors/error.go | 7 +------ errors/stackframe.go | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/errors/error.go b/errors/error.go index be7b7c89..2ada8c4a 100644 --- a/errors/error.go +++ b/errors/error.go @@ -105,13 +105,8 @@ func (err *Error) Stack() []byte { // stack. func (err *Error) StackFrames() []StackFrame { if err.frames == nil { - err.frames = make([]StackFrame, len(err.stack)) - - for i, pc := range err.stack { - err.frames[i] = NewStackFrame(pc) - } + err.frames = runtimeToErrorFrames(pcsToFrames(err.stack)) } - return err.frames } diff --git a/errors/stackframe.go b/errors/stackframe.go index 7ed19223..c4b5a86e 100644 --- a/errors/stackframe.go +++ b/errors/stackframe.go @@ -20,7 +20,6 @@ type StackFrame struct { // NewStackFrame popoulates a stack frame object from the program counter. func NewStackFrame(pc uintptr) (frame StackFrame) { - frame = StackFrame{ProgramCounter: pc} if frame.Func() == nil { return @@ -31,7 +30,18 @@ func NewStackFrame(pc uintptr) (frame StackFrame) { // and we want to show the line that corresponds to the function call frame.File, frame.LineNumber = frame.Func().FileLine(pc - 1) return +} +// NewStackFrameFromRuntime populates a stack frame object from a runtime.Frame object. +func NewStackFrameFromRuntime(frame runtime.Frame) StackFrame { + pkg, name := packageAndName(frame.Func) + return StackFrame{ + File: frame.File, + LineNumber: frame.Line, + Name: name, + Package: pkg, + ProgramCounter: frame.PC, + } } // Func returns the function that this stackframe corresponds to @@ -112,14 +122,7 @@ func pcsToFrames(pcs []uintptr) []runtime.Frame { func runtimeToErrorFrames(rtFrames []runtime.Frame) []StackFrame { frames := make([]StackFrame, len(rtFrames)) for i, f := range rtFrames { - pkg, name := packageAndName(f.Func) - frames[i] = StackFrame{ - File: f.File, - LineNumber: f.Line, - Name: name, - Package: pkg, - ProgramCounter: f.PC, - } + frames[i] = NewStackFrameFromRuntime(f) } return frames } From 53d41e9bab7fd4b38d085513e055f789dc9c75fb Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 26 Mar 2019 21:13:29 -0700 Subject: [PATCH 3/7] Determine number of frames to expect at runtime for notifier panic tests --- notifier_test.go | 51 ++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/notifier_test.go b/notifier_test.go index 419e7541..e501a0ea 100644 --- a/notifier_test.go +++ b/notifier_test.go @@ -2,9 +2,11 @@ package bugsnag_test import ( "fmt" + "runtime" "testing" - simplejson "github.com/bitly/go-simplejson" + "github.com/bitly/go-simplejson" + "github.com/bugsnag/bugsnag-go" . "github.com/bugsnag/bugsnag-go/testutil" ) @@ -22,6 +24,26 @@ func crash(s interface{}) int { return s.(int) } +// numCrashFrames returns the number of frames to expect from calling crash(...) +func numCrashFrames(s interface{}) (n int) { + defer func() { + _ = recover() + pcs := make([]uintptr, 50) + // exclude Callers, deferred anon function, & numCrashFrames itself + m := runtime.Callers(3, pcs) + frames := runtime.CallersFrames(pcs[:m]) + for { + n++ + _, more := frames.Next() + if !more { + break + } + } + }() + crash(s) + return +} + func TestStackframesAreSkippedCorrectly(t *testing.T) { ts, reports := Setup() bugsnaggedReports = reports @@ -61,22 +83,13 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { assertStackframeCount(st, 3) }) - // Expect the following frames to be present for *.AutoNotify - /* - { "file": "runtime/panic.go", "method": "gopanic" }, - { "file": "runtime/iface.go", "method": "panicdottypeE" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func2.1" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func3" }, - { "file": "testing/testing.go", "method": "tRunner" }, - { "file": "runtime/asm_amd64.s", "method": "goexit" } - */ t.Run("notifier.AutoNotify", func(st *testing.T) { func() { defer func() { recover() }() defer notifier.AutoNotify() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) t.Run("bugsnag.AutoNotify", func(st *testing.T) { func() { @@ -84,35 +97,27 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { defer bugsnag.AutoNotify() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) - // Expect the following frames to be present for *.Recover - /* - { "file": "runtime/panic.go", "method": "gopanic" }, - { "file": "runtime/iface.go", "method": "panicdottypeE" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func4.1" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func4" }, - { "file": "testing/testing.go", "method": "tRunner" }, - { "file": "runtime/asm_amd64.s", "method": "goexit" } - */ t.Run("notifier.Recover", func(st *testing.T) { func() { defer notifier.Recover() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) t.Run("bugsnag.Recover", func(st *testing.T) { func() { defer bugsnag.Recover() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) } func assertStackframeCount(t *testing.T, expCount int) { + t.Helper() report, _ := simplejson.NewJson(<-bugsnaggedReports) stacktrace := GetIndex(GetIndex(report, "events", 0), "exceptions", 0).Get("stacktrace") if s := stacktrace.MustArray(); len(s) != expCount { From 92fb0e99da895a4ee394c3aef33de5024ef1316d Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 26 Mar 2019 21:14:38 -0700 Subject: [PATCH 4/7] Test that there is a frame corresponding to the handle panic test This should be sufficient and robust against differing number of frames between runtimes --- panicwrap_test.go | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/panicwrap_test.go b/panicwrap_test.go index c156d05f..bb8a8539 100644 --- a/panicwrap_test.go +++ b/panicwrap_test.go @@ -20,6 +20,7 @@ func TestPanicHandlerHandledPanic(t *testing.T) { startPanickingProcess(t, "handled", ts.URL) + // Yeah, we just caught a panic from the init() function below and sent it to the server running above (mindblown) json, err := simplejson.NewJson(<-reports) if err != nil { t.Fatal(err) @@ -42,12 +43,24 @@ func TestPanicHandlerHandledPanic(t *testing.T) { event := getIndex(json, "events", 0) assertValidSession(t, event, true) - // Yeah, we just caught a panic from the init() function below and sent it to the server running above (mindblown) - frame := getIndex(getIndex(event, "exceptions", 0), "stacktrace", 1) - if getBool(frame, "inProject") != true || - getString(frame, "file") != "panicwrap_test.go" || - getInt(frame, "lineNumber") == 0 { - t.Errorf("stack frame seems wrong at index 1: %v", frame) + frames := event.Get("exceptions"). + GetIndex(0). + Get("stacktrace") + + found := false + for i := range frames.MustArray() { + frame := frames.GetIndex(i) + if getString(frame, "file") == "panicwrap_test.go" && + getBool(frame, "inProject") && + getInt(frame, "lineNumber") != 0 { + found = true + break + } + } + if !found { + t.Errorf("stack frames seem wrong: can't find panicwrap_test.go frame") + s, _ := frames.EncodePretty() + t.Log(string(s)) } } From e649941236b1c79d8fa49be990e9cc68320ab51e Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 26 Mar 2019 21:23:11 -0700 Subject: [PATCH 5/7] Remove runtime-dependent example error test --- errors/error_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/errors/error_test.go b/errors/error_test.go index e71f9feb..d530ffb0 100644 --- a/errors/error_test.go +++ b/errors/error_test.go @@ -133,15 +133,6 @@ func ExampleNew_skip() { }() } -func ExampleError_Stack() { - e := New("Oh noes!", 1) - fmt.Printf("Error: %s\n", e.Error()) - fmt.Printf("Stack is %d bytes", len(e.Stack())) - // Output: - // Error: Oh noes! - // Stack is 505 bytes -} - func a() error { b(5) return nil From fb827c683bfc74ca78192ddaa142fadf82810da6 Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 26 Mar 2019 21:34:05 -0700 Subject: [PATCH 6/7] Remove t.Helper() Not introduced until Go 1.9 --- notifier_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/notifier_test.go b/notifier_test.go index e501a0ea..010b9304 100644 --- a/notifier_test.go +++ b/notifier_test.go @@ -117,7 +117,6 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { } func assertStackframeCount(t *testing.T, expCount int) { - t.Helper() report, _ := simplejson.NewJson(<-bugsnaggedReports) stacktrace := GetIndex(GetIndex(report, "events", 0), "exceptions", 0).Get("stacktrace") if s := stacktrace.MustArray(); len(s) != expCount { From ccd64cd20d7231ccbf1cb399991441f1fe7f1c9b Mon Sep 17 00:00:00 2001 From: Adrian Duong Date: Tue, 2 Apr 2019 12:30:44 -0700 Subject: [PATCH 7/7] Use Frame.Function if no Frame.Func --- errors/stackframe.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/errors/stackframe.go b/errors/stackframe.go index c4b5a86e..0c8303da 100644 --- a/errors/stackframe.go +++ b/errors/stackframe.go @@ -24,7 +24,7 @@ func NewStackFrame(pc uintptr) (frame StackFrame) { if frame.Func() == nil { return } - frame.Package, frame.Name = packageAndName(frame.Func()) + frame.Package, frame.Name = packageAndName(frame.Func().Name()) // pc -1 because the program counters we use are usually return addresses, // and we want to show the line that corresponds to the function call @@ -34,7 +34,14 @@ func NewStackFrame(pc uintptr) (frame StackFrame) { // NewStackFrameFromRuntime populates a stack frame object from a runtime.Frame object. func NewStackFrameFromRuntime(frame runtime.Frame) StackFrame { - pkg, name := packageAndName(frame.Func) + var pkg, name string + if frame.Func != nil { + pkg, name = packageAndName(frame.Func.Name()) + } else if frame.Function != "" { + pkg, name = packageAndName(frame.Function) + } else { + return StackFrame{} + } return StackFrame{ File: frame.File, LineNumber: frame.Line, @@ -81,10 +88,9 @@ func (frame *StackFrame) SourceLine() (string, error) { return string(bytes.Trim(lines[frame.LineNumber-1], " \t")), nil } -func packageAndName(fn *runtime.Func) (string, string) { - name := fn.Name() - pkg := "" - +// packageAndName splits a package path-qualified function name into the package path and function name. +func packageAndName(qualifiedName string) (pkg string, name string) { + name = qualifiedName // The name includes the path name to the package, which is unnecessary // since the file name is already included. Plus, it has center dots. // That is, we see @@ -103,7 +109,7 @@ func packageAndName(fn *runtime.Func) (string, string) { } name = strings.Replace(name, "·", ".", -1) - return pkg, name + return } func pcsToFrames(pcs []uintptr) []runtime.Frame {