Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show stack traces on assertion errors #4708

Closed
4 tasks
ethanfrey opened this issue Jul 10, 2019 · 3 comments
Closed
4 tasks

Show stack traces on assertion errors #4708

ethanfrey opened this issue Jul 10, 2019 · 3 comments
Labels
S:proposal accepted T: Dev UX UX for SDK developers (i.e. how to call our code)
Milestone

Comments

@ethanfrey
Copy link
Contributor

ethanfrey commented Jul 10, 2019

Summary

assert.NoError(t, err) typically dumps out a stack trace of the error, which is a wonderful help for debugging. This is based on pkg/errors work and is pretty much the standard in golang error handling since 2016 or 2017. cosmos-sdk shows no such stack traces

Problem Definition

This feature was present in older versions of the sdk. And all the groundwork is already done

Proposal

stretchr/testify the popular testing library used by cosmos-sdk, has nice support for showing stacktraces in NoError: return Fail(t, fmt.Sprintf("Received unexpected error:\n%+v", err), msgAndArgs...)

Note the use of %+v, which is a special rune used by pkg/errors

After digging into sdk code, I found that there is support for stacktraces in cmnError which seems to be the root of the inheritance jungle. It even has Format implemented. The only problem is that it chooses to display the stacktrace upon %#s, %#v, %#q, etc... https://github.com/tendermint/tendermint/blob/master/libs/common/errors.go#L127-L133

I don't know if changing tendermint implementation is an option (and out of scope here), but you could override the Format method in sdk.Error to give a behavior more in line with standard packages.

Or for a deeper changes, you could look at small, well-designed error handling package that captures sdk errors with codes and even handles joining multiple validation errors into one response.

I'm happy to discuss this more, but presence of stack traces in failing tests (without requiring a panic) is a huge productivity enhancement.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@jackzampolin
Copy link
Member

I think @fedekunze might have the best context here.

@tac0turtle tac0turtle added T: Dev UX UX for SDK developers (i.e. how to call our code) S:proposed labels Jul 11, 2019
@alexanderbez
Copy link
Contributor

This essentially ties into removing cmn.Error correct? I think @marbar3778 is spearheading this.

@fedekunze fedekunze added this to the v0.37.0 milestone Jul 19, 2019
@fedekunze fedekunze removed their assignment Jul 23, 2019
@fedekunze
Copy link
Collaborator

Closed via #4821

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:proposal accepted T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
None yet
Development

No branches or pull requests

5 participants