Inconsistent nature of `messageFromMsgAndArgs`

[Antonboom]

Description

Hello, team!

I was surprised, but testify allows to set non-string in msgAndArgs... if this is the single argument:

testify/assert/assertions.go

Lines 290 to 296 in 89cbdd9

	if len(msgAndArgs) == 1 {
	msg := msgAndArgs[0]
	if msgAsStr, ok := msg.(string); ok {
	return msgAsStr
	}
	return fmt.Sprintf("%+v", msg)
	}

So I can do

assert.Equal(a, b, new(time.Time))

But cannot

assert.Equal(a, b, new(time.Time), 1, 2, 3) // Panics.

Proposed solution

Remove support of non-string single arg (as rare and strange? case) and allows only msg string + args ...any.

Use case

f-assertions future (#1089 (comment)).

[Antonboom]

Some real examples of using non-string arg: Antonboom/testifylint#169 (comment)

And this explores the another solution – to implement args ...any printing like

package fmt
func Print(a ...any) (n int, err error)
func Printf(format string, a ...any) (n int, err error)

Thus, support both options (formatted print and not formatted print of N args). But I am not happy with it :(

[brackendawson]

My word! I didn't know that was supported. I'd prefer to entirely remove the msgAndArgs argument from the "non-f" assertions. But that and removing this is a breaking change, no?

[Antonboom]

But that and removing this is a breaking change, no?

Technically no (because no signature changes), but logically yes (panicking of existing code) 🤔
(I am about stop supporting the single not-string arg, not about removing. Removing, of course, is breaking change).

But I need to know "party vector", if 💯 you are moving to "entirely remove the msgAndArgs argument from the "non-f" assertions", then I can enable warning on this single-not-string arg case in testifylint.

Currently I propose use f-assertions only if format string is used, but this special case is ignored, that leads to checker inconsistency 😞

[brackendawson]

I would go with always direct people to the f functions. The variadic just shouldn't be used, even for this sprint like behavior.

[Antonboom]

Got it. Thanks 🫡

[Antonboom]

+1 more interesting bomb from k8s code:

as := assert.New(t)
as.Fail("test case [%d] failed.  Expected: %+v, Got: %+v", idx, tc.expected, actual)

👆 panics, because failureMessage is not a format string, but the developer assumed it was.

[brackendawson]

+1 more interesting bomb from k8s code:

as := assert.New(t)
as.Fail("test case [%d] failed. Expected: %+v, Got: %+v", idx, tc.expected, actual)

👆 panics, because failureMessage is not a format string, but the developer assumed it was.

I think this might be a different issue, perhaps #474. I suspect Fail was accidentally exported.