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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shawc71
Copy link

@shawc71 shawc71 commented May 22, 2021

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
Copy link
Author

shawc71 commented Aug 14, 2021

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

@boyan-soubachov
Copy link
Collaborator

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
Copy link

tjanez commented Sep 15, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants