Incorrect formatter fix with single non-string msgAndArgs argument

[pgimalac]

Testify assertions can be called with a non-string value for msgAndArgs argument if there is a single such argument, as can be seen in the code it will be rendered using fmt.Sprintf("%+v").

This means that using assert.Fail(t, "failed", true) is functionally correct (this example is odd, but some cases can make sense, like using an error or some struct).

But when using -formatter.require-f-funcs, the formatter rule of testifylint will replace this with assert.Failf(t, "failed", true), which doesn't compile as the first message argument of f-variant assertions must have type string.

Minimal reproducible example:

package main

import (
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestA(t *testing.T) {
	assert.Fail(t, "failed", true)
}

[ccoVeille]

Thanks for reporting this.

It's indeed a real bug that has consequences

We will take a look.

[ccoVeille]

I took a look. I found the piece of code involved.

I started adding the unit test.

I will have to find a way to detect such "uncommon" usage.

I'm very polite here, because the example you gave looks improper usage of testify.

But then, I'm unsure how to ignore/report it

Reporting it as invalid could be counter productive on large code base.

So for now, I think the better would be to simply ignore it

[ccoVeille]

Hey @pgimalac do you have any real life example available on GitHub that you could share about this ?

Thanks

[pgimalac]

I ran the linter on the https://github.com/DataDog/datadog-agent repository (over 1.1M LoC in Go, including 400k lines of test) and found dozens of occurrences (at least 30).

Most of them look like invalid uses of testify (eg. missing failureMessage arg of Fail, or using Error instead of ErrorIs), which I'll try to fix, but a few are still interesting:

• this uses a loop index as message
• this uses a whole testcase struct as message
• the best case I could find is this one: msgAndArgs is passed as a slice, so it works fine with Fail but doesn't compile with Failf which expects a string then a slice

Maybe a disabled-by-default rule to warn when the first msgAndArgs argument doesn't have type string could be interesting ? (or enabled by default, depends if you consider this is odd or just invalid)

[ccoVeille]

Thanks for your valuable examples.

I think for now, what's is important is to avoid suggesting something invalid.

then I have doubts about what else we could/should do :

• I'm unsure if formatter is the right checker to report this
• should it report even use use-f-func is false ? because we could consider it's somehow invalid, even it "works" with non-f version, but cannot with the f version
• a few things like that

[pgimalac]

Linking the testify PR which allowed using non-string if there's a single msg argument stretchr/testify#699, for reference.

It mentions it's to help with debugging so arguably that doesn't make it a correct usage, but now that it works it's not great if the suggested fix doesn't compile...

On the other hand even without considering non-string argument, the fix is invalid when passing msgAndArgs using a slice, so it definitely needs fixing:

the best case I could find is this one: msgAndArgs is passed as a slice, so it works fine with Fail but doesn't compile with Failf which expects a string then a slice

[ccoVeille]

Thanks for pointing out the PR and context.

Please expect delays as most maintainers are in holidays (including me)

[pgimalac]

No worries I'm really not in a hurry, I can just disable the rule anyway !

[Antonboom]

@pgimalac Hi! Thank you for issue

1. Non-string argument is ignored in formatter: ignore non-string single arg #200.
2. Discussion is continued in Inconsistent nature of messageFromMsgAndArgs stretchr/testify#1679

Thanks 🫡