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

Disable using Stringer/error interfaces for diffing #895

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

luan
Copy link
Contributor

@luan luan commented Feb 21, 2020

NOTE: this changes something that is the default behavior. I intentionally am doing this as a proposal to change that behavior because I think the new one is better. If this is a problem, we could also achieve the goal of this PR by making spewConfig somewhat accessible so that the user of the library can choose to DisableMethods themselves.

spew's default formatter will call String() or Error() in structs
that implement the fmt.Stringer or error interfaces. Depending on
the implementation of those, the diff can become quite useless to read
(see the example struct I used for the test case in this commit).

This changes spew's configuration to DisableMethods so that it will
always use it's own pretty printer. This makes testing structs less
surprising and generally more useful, without tying the tests to the
implementation of String() (the user here can always choose to
require.Equal(a.String(), b.String()) if testing those is important to
them.

@boyan-soubachov
Copy link
Collaborator

I think this change makes sense but am I correct in thinking that we're now delegating our own test output format to spew's output format? Would this be wise?

Given that the result of our test assertions are definitely a key feature/purpose of this framework, would it make more sense to rather update our formatting output to match `spew's or, at the least, version-lock our spew dependency?

@luan
Copy link
Contributor Author

luan commented Feb 26, 2020

@boyan-soubachov we were always delegating it to spew to be fair. This change merely makes us always delegate as opposed to only when the object being tested doesn't implement fmt.Stringer.

I think maintaining a formatter as part of testify would work. But given that we've been using spew for this for a while it feels like a much larger step.

I do agree with you that the failure messages that testify generates are a key part of the framework and we should find a way to make them as high quality and consistent as we can. So I agree with version locking. But isn't that already done in go.mod here?

@boyan-soubachov boyan-soubachov added this to the v1.6.0 milestone Feb 26, 2020
assert/assertions_test.go Outdated Show resolved Hide resolved
`spew`'s default formatter will call `String()` or `Error()` in structs
that implement the `fmt.Stringer` or `error` interfaces. Depending on
the implementation of those, the diff can become quite useless to read
(see the example struct I used for the test case in this commit).

This changes `spew`'s configuration to `DisableMethods` so that it will
always use it's own pretty printer. This makes testing structs less
surprising and generally more useful, without tying the tests to the
implementation of `String()` (the user here can always chose to
`require.Equal(a.String(), b.String())` if testing those is important to
them.
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! 👍 Thank you for your contribution.

@glesica , would you like to give this PR a once-over?

Copy link
Collaborator

@glesica glesica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very reasonable to me. Thanks for the contribution!

@glesica glesica merged commit afd4130 into stretchr:master Feb 26, 2020
benjaminp pushed a commit to dropbox/dbx_build_tools that referenced this pull request Mar 4, 2020
Summary:
 - github.com/stretchr/testify:   @commit afd4130c14595c7d37c067f8c883f2b5ec2227e2 (from master)
 - github.com/stretchr/objx:      @commit 477a77ecc69700c7cdeb1fa9e129548e1c1c393c (from v0.1.1)
 - gopkg.in/yaml.v2:              not updated
 - github.com/pmezard/go-difflib: not updated

This bump includes a lot of changes that were between the previous commit of `testify` we vendored (be8372ae8ec5c6daaed3cc28ebf73c54b737c240).

It also includes a new change from this [pull-request](stretchr/testify#895) to make
failure messages more consistent (especially useful for`proto.Message`s).

Test Plan: Changes -> we should make sure we're running all go tests that use testify.

GitOrigin-RevId: e88b2e1e997f7ec25df772c27524f17e10b185bc
@AlekSi
Copy link
Contributor

AlekSi commented Aug 1, 2020

See #989

@HaraldNordgren
Copy link
Contributor

Fix: #1072

@marcel
Copy link

marcel commented Jul 17, 2022

This should have exported the spew config to make this configurable, even if it changed the default. Proposing s.Equal(expected.String(), actual.String()) makes sense in the trivial case where you are comparing values directly but is very inconvenient when comparing slices, maps or any other composite data structure that contain types such as those based off of uints or bytes that are not human readable without a lot of cognitive work.

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

Successfully merging this pull request may close these issues.

6 participants