More info when do func has wrong signature #173

msabramo · 2018-04-11T19:12:25Z

This builds on the work in #171 but it's a bit of a different feature, so I submitted a separate PR for it.

If a panic occurs when calling the user-supplied function to Do, catch the panic and show a terser call stack and the file and line number of the function that has the wrong arguments.

One problem with this that I just thought of is this may have a detrimental effect if the user is trying to call a function that intentionally panics. Probably not a common case. Not sure how to deal with that yet. see 0f4f173

Before:

...
    Test Panicked
    reflect: Call using *[]*models.ImageDefinition as type int
    /usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505

    Full Stack Trace
        /usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505 +0x229
    reflect.Value.call(0x4889200, 0x49e60c0, 0x13, 0x49b4159, 0x4, 0xc4203c1c20, 0x3, 0x3, 0x49af000, 0x485f4c0, ...)
        /usr/local/Cellar/go/1.10/libexec/src/reflect/value.go:377 +0x1202
    reflect.Value.Call(0x4889200, 0x49e60c0, 0x13, 0xc4203c1c20, 0x3, 0x3, 0x94, 0x0, 0x0)
        /usr/local/Cellar/go/1.10/libexec/src/reflect/value.go:308 +0xa4
    git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock.(*Call).Do.func1(0xc4202126c0, 0x3, 0x4, 0xc4201b7e40, 0x2, 0x2)
        /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock/call.go:154 +0x23f
    git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock.(*Controller).Call(0xc420177230, 0x4913f80, 0xc42007f7a0, 0x49b5a9c, 0x6, 0xc4202126c0, 0x3, 0x4, 0x0
        /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock/controller.go:170 +0x167
    git.corp.adobe.com/adobe-platform/flight-director/mocks/shared/db.(*MockWrapper).Select(0xc42007f7a0, 0x4829a00, 0xc420212620, 0xc4201de180, 0x34, 0xc420478520, 0x1, 0x1, 0x40133b8, 0x120,
        /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/mocks/shared/db/wrapper.go:82 +0x218
    git.corp.adobe.com/adobe-platform/flight-director/shared/db.(*QueryFilter).SelectIntoEntities(0xc420334988, 0x4a70c00, 0xc42007f7a0, 0x4829a00, 0xc420212620, 0x1, 0x0)
        /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/shared/db/query_filter.go:74 +0x202
    ...

After:

...
    reflect: Call using *[]*models.ImageDefinition as type int (incorrect func args at /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/server/routes/deploys/handlers_t

    git.corp.adobe.com/adobe-platform/flight-director/shared/db.(*QueryFilter).SelectIntoEntities
        /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/shared/db/query_filter.go:74
    ...

Cc: @seanisom, @mayanand, @balshetzer

msabramo · 2018-08-08T15:32:03Z

.

msabramo · 2019-08-15T08:28:48Z

Rebased and fixed merge conflict.

Catch panic and show a terser call stack and the file and line number of the function that has the wrong arguments Before: ``` ... Test Panicked reflect: Call using *[]*models.ImageDefinition as type int /usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505 Full Stack Trace /usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505 +0x229 reflect.Value.call(0x4889200, 0x49e60c0, 0x13, 0x49b4159, 0x4, 0xc4203c1c20, 0x3, 0x3, 0x49af000, 0x485f4c0, ...) /usr/local/Cellar/go/1.10/libexec/src/reflect/value.go:377 +0x1202 reflect.Value.Call(0x4889200, 0x49e60c0, 0x13, 0xc4203c1c20, 0x3, 0x3, 0x94, 0x0, 0x0) /usr/local/Cellar/go/1.10/libexec/src/reflect/value.go:308 +0xa4 git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock.(*Call).Do.func1(0xc4202126c0, 0x3, 0x4, 0xc4201b7e40, 0x2, 0x2) /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock/call.go:154 +0x23f git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock.(*Controller).Call(0xc420177230, 0x4913f80, 0xc42007f7a0, 0x49b5a9c, 0x6, 0xc4202126c0, 0x3, 0x4, 0x0, 0x0, ...) /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/vendor/github.com/golang/mock/gomock/controller.go:170 +0x167 git.corp.adobe.com/adobe-platform/flight-director/mocks/shared/db.(*MockWrapper).Select(0xc42007f7a0, 0x4829a00, 0xc420212620, 0xc4201de180, 0x34, 0xc420478520, 0x1, 0x1, 0x40133b8, 0x120, ...) /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/mocks/shared/db/wrapper.go:82 +0x218 git.corp.adobe.com/adobe-platform/flight-director/shared/db.(*QueryFilter).SelectIntoEntities(0xc420334988, 0x4a70c00, 0xc42007f7a0, 0x4829a00, 0xc420212620, 0x1, 0x0) /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/shared/db/query_filter.go:74 +0x202 ... ``` After: ``` ... reflect: Call using *[]*models.ImageDefinition as type int (incorrect func args at /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/server/routes/deploys/handlers_test.go:71?) git.corp.adobe.com/adobe-platform/flight-director/shared/db.(*QueryFilter).SelectIntoEntities /Users/abramowi/go/src/git.corp.adobe.com/adobe-platform/flight-director/shared/db/query_filter.go:74 ... ```

poy

Thanks for doing this and sorry for the huge delay!

This is really great! I have a few nits. Also, would it be tricky to add a test for this? There is a decent amount of logic in there.

poy · 2019-09-27T05:15:46Z

gomock/call.go

+			if r := recover(); r != nil {
+				errMsg, ok := r.(string)
+
+				// We only handle a very specific panic


Do you see a use case where a user might want to panic instead of failing the test?

If not, WDYTA letting fmt.Sprint do some work here (https://play.golang.org/p/SJh50xjMu_8) and allow more than just strings?

poy · 2019-09-27T05:18:43Z

gomock/stack.go

+	"github.com/pkg/errors"
+)
+
+const skipFrames = 2


Can we add some docs around this magic number?

poy · 2019-09-27T05:20:01Z

gomock/call.go

+				if r := recover(); r != nil {
+					errMsg, ok := r.(string)
+
+					// We only handle a very specific panic


Same question as above.

poy · 2019-09-27T05:22:24Z

gomock/stack.go

+}
+
+func currentStackTrace(skipFrames int) string {
+	err := errors.New("fake error just to get stack")


Can we use the runtime package instead and avoid the extra dependency?

poy · 2019-09-27T05:24:03Z

gomock/stack.go

+}
+
+func stackTraceString(stackTrace errors.StackTrace, skipFrames int) string {
+	buffer := bytes.NewBufferString("")


nit: It doesn't look like we need to use the function bytes.NewBufferString("") as we're taking in an empty string. Maybe just bytes.Buffer{}?

poy · 2019-09-27T05:27:36Z

gomock/call.go

+						!strings.Contains(errMsg, "reflect.Set: value of") {
+						panic(r)
+					}
+					skipFrames := 8


Can we add some docs where 8 came from?

codyoss · 2019-10-31T04:16:23Z

@msabramo are you still interested in this PR?

codyoss · 2019-12-27T19:58:54Z

I am going to close this for now due to lack of inactivity. Please reopen if you are still interested in working on this. Thanks.

msabramo force-pushed the more-info-when-Do-func-has-wrong-signature branch from 23c0a3c to 7251cb7 Compare August 15, 2019 08:28

msabramo force-pushed the more-info-when-Do-func-has-wrong-signature branch from 7251cb7 to 31092bf Compare August 16, 2019 00:36

msabramo force-pushed the more-info-when-Do-func-has-wrong-signature branch from 31092bf to 2fe26fb Compare August 16, 2019 01:07

poy suggested changes Sep 27, 2019

View reviewed changes

poy self-assigned this Sep 27, 2019

codyoss added the status: needs more info This issue need more information from the author. label Oct 31, 2019

codyoss closed this Dec 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

More info when do func has wrong signature #173

More info when do func has wrong signature #173

Uh oh!

msabramo commented Apr 11, 2018 •

edited

Loading

Uh oh!

msabramo commented Aug 8, 2018

Uh oh!

msabramo commented Aug 15, 2019

Uh oh!

poy left a comment

Uh oh!

poy Sep 27, 2019

Uh oh!

poy Sep 27, 2019

Uh oh!

poy Sep 27, 2019

Uh oh!

poy Sep 27, 2019

Uh oh!

poy Sep 27, 2019

Uh oh!

poy Sep 27, 2019

Uh oh!

codyoss commented Oct 31, 2019

Uh oh!

codyoss commented Dec 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

More info when do func has wrong signature #173

More info when do func has wrong signature #173

Uh oh!

Conversation

msabramo commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msabramo commented Aug 8, 2018

Uh oh!

msabramo commented Aug 15, 2019

Uh oh!

poy left a comment

Choose a reason for hiding this comment

Uh oh!

poy Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

poy Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

poy Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

poy Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

poy Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

poy Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

codyoss commented Oct 31, 2019

Uh oh!

codyoss commented Dec 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msabramo commented Apr 11, 2018 •

edited

Loading