fix: enable formatter.require-f-funcs from testifylint

[mmorel-35]

What does this PR do?

Setup formatter.require-f-funcs from testifylint linter and apply it’s recommandations .

Why is it important?

To require f-assertions if format string is used.

Signed-off-by: Matthieu MOREL [email protected]

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	0cbdab1
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/67128a26b373690008ba84f4
😎 Deploy Preview	https://deploy-preview-2836--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mdelapenya]

What is your experience with this rule? I think we are doing good progress in throwing more meaningful error messages, which will be part of the test failure in the case require.Error fails. So I'm not very sure about including this one. Wdyt?

[stevenh]

I don't understand this one, could you clarify why this change is beneficial?

[mmorel-35]

I probably missed something because it's not the intention of the rule to reduce the readability of the test results, on the contrary.
Let's check.

The rule reason is explained here
https://github.com/Antonboom/testifylint?tab=readme-ov-file#formatter

There is also more to read
https://github.com/Antonboom/testifylint?tab=readme-ov-file#historical-reference

[stevenh]

I probably missed something because it's not the intention of the rule to reduce the readability of the test results, on the contrary. Let's check.

The rule reason is explained here https://github.com/Antonboom/testifylint?tab=readme-ov-file#formatter

There is also more to read https://github.com/Antonboom/testifylint?tab=readme-ov-file#historical-reference

Yer that's there to eliminate an embedded fmt.Sprintf as those messages are just raw strings it's not needed.

So its needed for this:

assert.Errorf(t, err, fmt.Sprintf("test %s", test.testName)) // Bad
assert.Errorf(t, err, "test %s", test.testName) // Good

But not needed for this:

assert.Error(t, err, "thing should error") // Not needed as no replaceable variables

[mmorel-35]

Have look over here, Antonboom/testifylint#177 (comment)
I asked the same question to testifylint's author

[stevenh]

Have look over here, Antonboom/testifylint#177 (comment) I asked the same question to testifylint's author

Thanks for that context, really useful.

Having read the history and looked at the code for assert / require, I believe there's a case which was overlooked. If you trace the calls you'll end up in messageFromMsgAndArgs which has a specific case for a single argument as can be found here.

So f versions are actually only required in the case where there is more than one argument.

If that rule doesn't handle that edge case I suggest we leave it disabled as it would cause churn for no real value, I would even suggest that including a reason for things like NoError add little to no value so would be better removed.

[mmorel-35]

OK, I'll close this PR.
@mdelapenya , @stevenh ,
But I would like to suggest having a look at

require.Error(t, results.FirstErr(), kerr.UnknownTopicOrPartition) //nolint:testifylint

In modules/redpanda/redpanda_test.go .

Shall it be turned into

require.ErrorIs(t, results.FirstErr(), kerr.UnknownTopicOrPartition)

or

require.ErrorContains(t, results.FirstErr(), kerr.UnknownTopicOrPartition)

Or am I missing the understanding of the behavior of that test ?

[stevenh]

Yes @mmorel-35 nice catch, that should be an ErrorIs