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

Diffs no longer call string() #1108

Open
b-gupta opened this issue Aug 2, 2021 · 6 comments
Open

Diffs no longer call string() #1108

b-gupta opened this issue Aug 2, 2021 · 6 comments
Labels
pkg-assert Change related to package testify/assert

Comments

@b-gupta
Copy link

b-gupta commented Aug 2, 2021

https://github.com/stretchr/testify/blob/master/assert/assertions.go#L1654

	// DisableMethods specifies whether or not error and Stringer interfaces are
	// invoked for types that implement them.

Not sure why this was added by default. Can we make this configurable or remove it?

@tjanez
Copy link

tjanez commented Sep 15, 2021

To save others some time, here is the relevant line:

DisableMethods: true,

that configures the go-spew's behavior.

And here is the PR that changed this: #895.

@mitar
Copy link

mitar commented May 5, 2024

I got bitten by this as well. :-(

@dolmen dolmen added the pkg-assert Change related to package testify/assert label May 6, 2024
@brackendawson
Copy link
Collaborator

Given that the assertions never call such methods I think we should not be showing their output in diffs. There is a risk of saying "these two things are different and here is a diff showing that they are identical".

@mitar can you give a more detailed experience report explaining how this bit you?

@mitar
Copy link

mitar commented Oct 5, 2024

It is simple, if errors contain private fields, diffing them does not return anything useful. While Error() might return useful data.

@brackendawson
Copy link
Collaborator

Yea ok, that's a common theme. Private struct fields are used in comparisons with == and reflect.DeepEqual but are otherwise completely inaccessible.

For most custom errror types I've seen in the wild their Error() method does not yeild all of their fields. In fact I've seen many not yield public fields. So I definately hold a position against reversing the change.

While we decide whether to expose the configuration, you can put the method output into msgAndArgs to add additional information to the error output:

assert.Equalf(t, expected, actual, "\nexpected: %q\nactual:  %q", expected.Error(), actual.Error())

@mitar
Copy link

mitar commented Oct 5, 2024

For most custom errror types I've seen in the wild their Error() method does not yeild all of their fields.

But the difference is between having {} struct without even an error message shown (because it is stored in a private field) and having at least an error message string.

While we decide whether to expose the configuration, you can put the method output into msgAndArgs to add additional information to the error output:

Yes, but this is really tedious and it works only at top-level, but not if error is nested somewhere in a struct (not common, but still).

Maybe a solution could be to check for a custom interface method defined by this package and used only when diffing which expects to dump the whole struct into string?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg-assert Change related to package testify/assert
Projects
None yet
Development

No branches or pull requests

5 participants