testify icon indicating copy to clipboard operation
testify copied to clipboard

Proposal: Pass custom formatters that are used in the failureMessage passed to assert/require.Error

Open jkopczyn opened this issue 2 years ago • 6 comments

The naive string representation of an object is not always a helpful descriptive label for it when it appears in a failed assertion or requirement, and trailing messages (usually msgAndArgs) are harder to parse visually than the main string block of an error message. Therefore, add a class of assertion methods which allow a wrapper to be applied.

Concrete example: I frequently work with protocol buffers which include status enums, and want to assert the expected status, e.g.

assert.Equal(t, pb.Status_VALID, resp.GetStatus())

When this fails, it gives this message:

Error:      	Not equal: 
            	expected: 2
            	actual  : 1
Test:       	TestCreationFooBar/create_asset

Protobuf enums have a .String() method to retrieve their names, so obviously, I can change the assert to

assert.Equal(t, pb.Status_VALID.String(), resp.GetStatus().String())

and that will make the message

Error:      	Not equal: 
            	expected: "VALID"
            	actual  : "IN_PROGRESS"
Test:       	TestCreationFooBar/create_asset

This is much easier to read, and to distinguish from a slice with the wrong length or whatever, but it's a more brittle comparison - the integer comparison is the canonical one, it's just not human-legible.

The obvious way to get around this problem is to have variant types assert.EqualWithWrapper, assert.ElementsMatchWithWrapper, etc. which accept a func(x interface{})string which is applied to the elements being compared before being combined into the error message. The one I'd use here, which would probably be fairly typical, might be func(x Stringable) { return fmt.Sprintf("%v [%s]", x, x.String()) }.

I'm not sure how many methods of the API would benefit from having this equivalent. Equal, EqualValues, Exactly, and NotEqual seem obvious; ElementsMatch, Contains/NotContains, and Subset/NotSubset also seem like they'd benefit. Many others might but it'd be niche, like WithinRange; others like Implements would require a more-complicated type signature (and would still probably be niche). There's clearly a steep tradeoff of API verbosity for this functionality, which isn't ideal, but I think it's worth doing at least in some cases.

jkopczyn avatar Nov 26 '22 02:11 jkopczyn

I don't think adding more functionality for more edge cases like this is useful, as it makes the library bloated and harder to use and maintain with each added feature.

dropdevrahul avatar May 11 '23 06:05 dropdevrahul

I don't think it's reasonable to call this an edge case.

jkopczyn avatar May 11 '23 20:05 jkopczyn

What about using a custom message with assert.Equalf?

dolmen avatar Jul 25 '23 03:07 dolmen

Same problem I noted originally - much harder for humans to parse visually. If the formatting used for assembling the visual block used for the default assertion failure was publicly exposed so that it could be plugged into assert.Equalf, that would be a reasonable solution, but IIRC that would be substantially harder than just adding the wrapper options.

Though I've moved on from this project and no longer remember details.

jkopczyn avatar Aug 08 '23 16:08 jkopczyn

I don't think it's reasonable to call this an edge case.

To elaborate on this: protocol buffers are IME sufficiently ubiquitous that even if there was literally no other use for this (obviously false), it would not be an edge case. The fraction of substantial projects using assert/require that will have this style of problem is certainly at least 5% and extremely likely to be double digits. It may not be the majority but it's far from an edge case.

jkopczyn avatar Aug 08 '23 16:08 jkopczyn

Such customizability is discussed some times (e.g. https://github.com/stretchr/testify/issues/1204) and there's no progress. So I've implemented customizable equality check https://github.com/seiyab/teq. I'll be grad if you try it out.

seiyab avatar May 07 '24 13:05 seiyab