Make spew config selection configurable to allow for helpful diffs on time.Time objects nested in structs.

[shawc71]

Summary

Make time.Time objects nested inside structs produce reasonable diffs by making spewConfig configurable

Changes

#895 had some unintended side-effects on what diffs of time.Time look like: see #1078.

This attempts to solve the problem by allowing the user to select if they want to use the SpewConfig that has Stringer methods enabled. The default behavior remains the same. But if the user runs their tests after setting the environment variable TESTIFY_SPEW_STRINGER_ENABLE to TRUE, testify will pick the stringer config that does not disable the Stringer methods and thus produces nice diffs for time.Time nested inside structs.

This contains a minimal repro of this issue, right now those tests produce an almost 1500 line diff on test failure. After this change and when TESTIFY_SPEW_STRINGER_ENABLE env var is set to TRUE it will produce a user friendly diff like:

--- FAIL: TestDemo (0.00s)
    --- FAIL: TestDemo/same_tzs (0.00s)
        example_test.go:29: 
                Error Trace:    example_test.go:29
                Error:          Not equal: 
                                expected: &testify_issue_demo.structWithTime{someTime:(*time.Time)(0xc00000e460), someString:"some val"}
                                actual  : &testify_issue_demo.structWithTime{someTime:(*time.Time)(0xc00000e4a0), someString:"some val"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,3 +1,3 @@
                                 (*testify_issue_demo.structWithTime)({
                                - someTime: (*time.Time)(2020-05-22 00:00:00 +0000 UTC),
                                + someTime: (*time.Time)(2020-05-21 00:00:00 +0000 UTC),
                                  someString: (string) (len=8) "some val"
                Test:           TestDemo/same_tzs
    --- FAIL: TestDemo/different_tzs (0.00s)
        example_test.go:47: 
                Error Trace:    example_test.go:47
                Error:          Not equal: 
                                expected: &testify_issue_demo.structWithTime{someTime:(*time.Time)(0xc00000e740), someString:"some val"}
                                actual  : &testify_issue_demo.structWithTime{someTime:(*time.Time)(0xc00000e780), someString:"some val"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,3 +1,3 @@
                                 (*testify_issue_demo.structWithTime)({
                                - someTime: (*time.Time)(2020-05-22 00:00:00 +0000 UTC),
                                + someTime: (*time.Time)(2020-05-21 00:00:00 -0400 EDT),
                                  someString: (string) (len=8) "some val"
                Test:           TestDemo/different_tzs
FAIL
FAIL    testify_issue_demo      0.006s

Motivation

Please see #1078 which describes the issue in detail

export TESTIFY_SPEW_STRINGER_ENABLE=TRUE && go test ./...

Related issues

#1078

[shawc71]

@boyan-soubachov would it be possible to take a look at this. I think this is a nice usability improvement.

[boyan-soubachov]

Hmm. It seems like most of the feedback after #895 seems to be that time.Time is not very human-readable. I think it would make sense to revert back to the old behaviour and have some kind of option to enable what is the current behaviour.

In other words:

1. The logic in this PR should become default (reverting to the previous behaviour)
2. A control should be added to enable the (what is now) current printing mode.

Would it make more sense if the control is something that's injected in the code rather than an env var. I think it would make more sense if it's something explicit and isn't coupled to environment configuration since this is a unit testing framework, after all :)

[tjanez]

In other words:

1. The logic in this PR should become default (reverting to the previous behaviour)

2. A control should be added to enable the (what is now) current printing mode.

Agreed.

Would it make more sense if the control is something that's injected in the code rather than an env var.

Agreed. I don't think this is something one would want to change depending on a particular environment, but rather an option a project could use to control how diffs are printed in a static manner.

[SchumacherFM]

Can this be please merged as it is? Wouldn't change the logic as it breaks backwards compatibility.

[brackendawson]

I'm open to allowing users to configure the diff to use stringers so long as they are aware that it can call methods on the potentially faulty code under test.

I'm not convinced an environment variable is the way to do this, one would need to see the horrible diff with all the time zones listed, know this feature exists and then re-run the tests with an environment variable set. Setting environment variables for Go tests in some contexts (like VSCode) is overly burdensome.

I actually think a package global variable in the assert package would be better. This can be set by the test code calling testify in an init function for all tests in a package, or in a narrower scope such as one synchronous test or even a few lines of one test. This would only affect the package being tested as other packages are tested with newly initialised imports.

[brackendawson]

Don't expose the library name in the environment variable. Something like "TESTIFY_DIFF_STRINGER_ENABLE" would be better, if we use an environment variable at all.

[ccoVeille]

I have opened another PR to address this issue, I would like to get feedback from people who were involved in this current PR ☝️

• fix: time.Time rendering in the diff returned by error message #1829