testify icon indicating copy to clipboard operation
testify copied to clipboard

Diffs no longer call string()

Open b-gupta opened this issue 4 years ago • 7 comments

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?

b-gupta avatar Aug 02 '21 15:08 b-gupta

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

https://github.com/stretchr/testify/blob/ab6dc3262822ed562480c19876b0257ace761e3e/assert/assertions.go#L1658

that configures the go-spew's behavior.

And here is the PR that changed this: https://github.com/stretchr/testify/pull/895.

tjanez avatar Sep 15 '21 11:09 tjanez

I got bitten by this as well. :-(

mitar avatar May 05 '24 13:05 mitar

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?

brackendawson avatar Oct 05 '24 18:10 brackendawson

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

mitar avatar Oct 05 '24 19:10 mitar

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())

brackendawson avatar Oct 05 '24 19:10 brackendawson

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?

mitar avatar Oct 05 '24 20:10 mitar