add stacktrace to chain and formatted data

[diPhantxm]

#160

• add EnableStacktrace
• add Stacktrace on fail in chain
• add Stacktrace to FormatData

[diPhantxm]

Hey @gavv. I decided to add Stacktrace to chain and fill it when fail() called. chain_test started failing because of comparing assertionFailure in tests. Should I remake tests to make it not compare Stacktrace or was it a bad idea to add to chain?

[gavv]

Hey @gavv. I decided to add Stacktrace to chain and fill it when fail() called. chain_test started failing because of comparing assertionFailure in tests. Should I remake tests to make it not compare Stacktrace or was it a bad idea to add to chain?

This is a good idea indeed, this way trace would be much more convenient.

[coveralls]

Coverage: 95.65% (-0.004%) from 95.655% when pulling 71795fe on diPhantxm:feature/enable-stacktrace-flag into 2d6c5d7 on gavv:master.

[diPhantxm]

I think it's done. Please check defaultFailureTemplate for formatting stacktrace.

Maybe there is a point to think about shortening stacktrace if it's too long.

[gavv]

Thanks for PR! Here are some comments.

[gavv]

Can we do some simple sanity check here?

E.g. that Stacktrace contains "TestChain_Stacktrace" and "chain.fail".

[gavv]

nit: let's move it upper, right after other flags (..., DisableRequests, DisableResponses)

[gavv]

nit: let's move it after Request and Response, and add HaveStacktrace, for consistency with other fields in FormatData. HaveXxx is not strictly necessary, but is descriptive when used in template.

[gavv]

Let's guard it with HaveStacktrace check, for consistency with other stuff. We should ensure that if HaveStacktrace, no additional empty lines are inserted.

[gavv]

Also let's move stacktrace between "test name" and "assertion" and add "trace" header. It will look like this:

        <error message>
        
        test name: TestBlah

        trace:
          <stacktrace here>
        
        assertion:
          Request("GET").Expect().JSON().Object()
        
        actual value:
          <blah>
        
        reference value:
         <blah>

This way, error message and test name, which are most important, go first; trace and assertion are grouped together, which makes sense because both of them are kind of traces; assertion and actual/expected/reference are also grouped which is also logical because all them describe assertion made.

[gavv]

Let's make fillStacktrace guarantee that data.Stacktrace is always a non-nil slice, like we do for Errors in fillErrors.

TBH I don't remember why, but for some reason it was important or useful for Errors slice, in the context of templates :)

[gavv]

I've played a bit with this patch and I think it may be convenient to resemble behavior of testify to produce a more compact trace. It is implemented here: https://github.com/stretchr/testify/blob/c5fc9d6b6b21ea89be8480c0dc35e2977ab988f6/assert/assertions.go#L147

If we'll use assert.CallerInfo() instead of debug.Stack(), the stack will look like this:

        <error message>
        
        test name: TestBlah

        trace:
          chain.go:442
          array.go:719
          report_test.go:380

        assertion:
          Request("GET").Expect().JSON().Array().IsEmpty()
        
        actual value:
          <blah>
        
        reference value:
         <blah>

which is very compact, fits the style of httpexpect output, and looks usual to testify users.

However, it loses some information compared to original trace, and some users may prefer to see the full trace. So I'd make it configurable - replace EnableStacktrace boolean with enum of three values: disabled, full, and compact.

For compact mode, if feasible, I'd even reimplement assert.CallerInfo() and use the following format instead:

module/file:line struct.method()

so instead of:

array.go:719

it would be:

httpexpect/v2/array.go:719 Array.IsEmpty()

IMHO this is the best compromise between providing enough information and keeping it compact.

---

What do you think? Do you find these ideas useful? Would you like to implement some of them in this PR, or should I create a separate issue?

[diPhantxm]

I've played a bit with this patch and I think it may be convenient to resemble behavior of testify to produce a more compact trace. It is implemented here: https://github.com/stretchr/testify/blob/c5fc9d6b6b21ea89be8480c0dc35e2977ab988f6/assert/assertions.go#L147

If we'll use assert.CallerInfo() instead of debug.Stack(), the stack will look like this:

        <error message>
        
        test name: TestBlah

        trace:
          chain.go:442
          array.go:719
          report_test.go:380

        assertion:
          Request("GET").Expect().JSON().Array().IsEmpty()
        
        actual value:
          <blah>
        
        reference value:
         <blah>

which is very compact, fits the style of httpexpect output, and looks usual to testify users.

However, it loses some information compared to original trace, and some users may prefer to see the full trace. So I'd make it configurable - replace EnableStacktrace boolean with enum of three values: disabled, full, and compact.

For compact mode, if feasible, I'd even reimplement assert.CallerInfo() and use the following format instead:

module/file:line struct.method()

so instead of:

array.go:719

it would be:

httpexpect/v2/array.go:719 Array.IsEmpty()

IMHO this is the best compromise between providing enough information and keeping it compact.

What do you think? Do you find these ideas useful? Would you like to implement some of them in this PR, or should I create a separate issue?

TBH I don't see much difference between compact and full modes. file:line format is not so useful, because you may have multiple files with the same name but in different packages, and I don't think we should make people worry about switching modes. I agree debug.Stack() provides too much info and it doesn't look nice, so I would stay only with full format you provided upper httpexpect/v2/array.go:719 Array.IsEmpty()
I can implement this feature in this pr.
Let me know what we're doing here

[gavv]

file:line format is not so useful, because you may have multiple files with the same name but in different packages

Agree

I can implement this feature in this pr.
Let me know what we're doing here

Awesome, thanks!

I don't think we should make people worry about switching modes.

I feel that supporting stanard format makes sense just because it's common and complete. Someone may be just used to it, someone may want absolute paths, someone may want to use tools like panicparse or maybe their editor understands this trace format, etc. Also having unprocessed trace may be useful to debug httpexpect itself.

We could add it later. Could you use enum instead of boolean? This will allow us to add formats in future, doesn't matter if this pr will implement 1 or 2.

[gavv]

Another little thought. Since we'll collect caller info and then format it, we can store caller info instead of formatted string in AssertionFailure, and do formatting in DefaultFormatter.

First, it's just logical, and second, it will give us additional flexibility for free: user will be able to implement their own formatting when using custom formatter.

[diPhantxm]

I got your idea. I'll do it and ping you when it's ready for review

[diPhantxm]

@gavv I think it's ready for review.
I made a struct CallerInfo that stores function name, file and line, and then slice of these objects is formatted in formatter.
I made such format at [function]([file]:[line]). Maybe that was a bad decision, I found such format on google. Maybe I should have kept default Golang format.

[gavv]

Thanks, will look in upcoming days! BTW I don't remember if I mentioned - welcome to discord chat if you like that format.

[gavv]

Thanks for update!

Follow-up commit: ca5a5dd

renames, two stacktrace modes, changes in stacktrace format, and improvements in tests