formatter: support non-string-message checks (#221)

* formatter: warn on non-string single arg

* formatter: warn on args after non-string single arg

* formatter: support --formatter.allow-non-string-msg

* formatter: checker refactoring

* formatter: ignore calls with ellipsis

* formatter: replace allow-non-string-msg with require-string-msg

* formatter: introduce Fail/FailNow failure message check

* formatter: describe Fail/FailNow failure message check in README

* self-review fixes

Author: Antonboom

CONTRIBUTING.md

@@ -174,6 +174,7 @@ Describe a new checker in [checkers section](./README.md#checkers).
      assert.Equal(t, err.Error(), "user not found")
      assert.Equal(t, err, errSentinel) // Through `reflect.DeepEqual` causes error strings to be compared.
      assert.NotEqual(t, err, errSentinel)
+     require.Error(t, fmt.Errorf("you need to specify either logGroupName or logGroupArn"), err) // grafana case
      // etc.
 
 ✅   assert.ErrorIs(t, err, ErrUserNotFound)

README.md

@@ -73,7 +73,7 @@ $ testifylint --enable-all --disable=empty,error-is-as ./...
 # Checkers configuration.
 $ testifylint --bool-compare.ignore-custom-types ./...
 $ testifylint --expected-actual.pattern=^wanted$ ./...
-$ testifylint --formatter.check-format-string --formatter.require-f-funcs ./...
+$ testifylint --formatter.check-format-string --formatter.require-f-funcs --formatter.require-string-msg ./...
 $ testifylint --go-require.ignore-http-handlers ./...
 $ testifylint --require-error.fn-pattern="^(Errorf?|NoErrorf?)$" ./...
 $ testifylint --suite-extra-assert-call.mode=require ./...
@@ -471,11 +471,12 @@ disable this feature, use `--formatter.check-format-string=false` flag.
 
 #### 3)
 
-Requirement of the f-assertions (e.g. assert.Equal**f**) if format string is used. Disabled by default, use 
+Requirement of the f-assertions (e.g. assert.Equal**f**) if format string is used. Disabled by default, use
 `--formatter.require-f-funcs` flag to enable. <br>
 
 This helps follow Go's implicit convention _"Printf-like functions must end with `f`"_ and sets the stage for moving to
-`v2` of `testify`. In this way the checker resembles the [goprintffuncname](https://github.com/jirfag/go-printf-func-name) 
+`v2` of `testify`. In this way the checker resembles
+the [goprintffuncname](https://github.com/jirfag/go-printf-func-name)
 linter (included in [golangci-lint](https://golangci-lint.run/usage/linters/)). <br>
 
 Also, verbs in the format string of f-assertions are highlighted by an IDE, e.g. GoLand:
@@ -485,7 +486,7 @@ Also, verbs in the format string of f-assertions are highlighted by an IDE, e.g.
 <br>
 
 > [!CAUTION]
-> `--formatter.require-f-funcs` requires f-assertions, **even if there are no variable-length variables**, i.e. it 
+> `--formatter.require-f-funcs` requires f-assertions, **even if there are no variable-length variables**, i.e. it
 > requires `require.NoErrorf` for both these cases:
 > ```
 > require.NoErrorf(t, err, "unexpected error")
@@ -566,6 +567,44 @@ in `v2` of `testify`.
 
 </details>
 
+#### 4)
+
+Validating the first argument of `msgAndArgs` has `string` type (based on `testify`'s
+maintainer's [feedback](https://github.com/stretchr/testify/issues/1679#issuecomment-2480629257)):
+
+```go
+❌
+assert.Equal(t, 1, strings.Count(b.String(), "hello"), tc)
+
+✅
+assert.Equal(t, 1, strings.Count(b.String(), "hello"), "%+v", tc)
+```
+
+You can disable this behaviour with the `--formatter.require-string-msg=false` flag.
+
+#### 5)
+
+Validating there are no arguments in `msgAndArgs` if message is not a string:
+
+```go
+❌
+assert.True(t, cco.IsCardNumber(valid), i, valid) // Causes panic.
+```
+
+See [testify's issue](https://github.com/stretchr/testify/issues/1679) for details.
+
+#### 6) 
+
+Finally, it checks that failure message in `Fail` and `FailNow` is not used as a format string (which won't work):
+
+```go
+❌
+assert.Fail(t, "test case [%d] failed. %+v != %+v", idx, tc.expected, actual) // Causes panic.
+
+✅
+assert.Fail(t, "good luck!", "test case [%d] failed. %+v != %+v", idx, tc.expected, actual)
+```
+
 ---
 
 ### go-require
@@ -981,6 +1020,7 @@ assert.False(t, num != num)
 ```
 
 And against these meaningless assertions:
+
 ```go
 assert.Empty(t, "")
 assert.False(t, false)

Taskfile.yml

@@ -50,7 +50,7 @@ tasks:
   test:checker:
     deps: [ test:gen ]
     cmds:
-      - go test -count 1 -run TestTestifyLint_CheckersDefault/{{.CLI_ARGS}} ./analyzer
+      - go test -count 1 -run TestTestifyLint/{{.CLI_ARGS}} ./analyzer
 
   test:coverage:
     env:

analyzer/analyzer_test.go

@@ -105,6 +105,15 @@ func TestTestifyLint_NotDefaultCases(t *testing.T) {
 				"enable":                        checkers.NewFormatter().Name(),
 				"formatter.check-format-string": "false",
 				"formatter.require-f-funcs":     "true",
+				"formatter.require-string-msg":  "false",
+			},
+		},
+		{
+			dir: "formatter-require-f-for-non-string-arg",
+			flags: map[string]string{
+				"disable-all":               "true",
+				"enable":                    checkers.NewFormatter().Name(),
+				"formatter.require-f-funcs": "true",
 			},
 		},
 		{

analyzer/checkers_factory.go

@@ -58,6 +58,7 @@ func newCheckers(cfg config.Config) ([]checkers.RegularChecker, []checkers.Advan
 		case *checkers.Formatter:
 			c.SetCheckFormatString(cfg.Formatter.CheckFormatString)
 			c.SetRequireFFuncs(cfg.Formatter.RequireFFuncs)
+			c.SetRequireStringMsg(cfg.Formatter.RequireStringMsg)
 
 		case *checkers.GoRequire:
 			c.SetIgnoreHTTPHandlers(cfg.GoRequire.IgnoreHTTPHandlers)

analyzer/checkers_factory_test.go

@@ -69,9 +69,10 @@ func Test_newCheckers(t *testing.T) {
 		checkers.NewSuiteTHelper(),
 	}
 
-	zeroedFormatter := checkers.RegularChecker(checkers.NewFormatter().
+	formatterWithoutEnabledOptions := checkers.RegularChecker(checkers.NewFormatter().
 		SetCheckFormatString(false).
-		SetRequireFFuncs(false))
+		SetRequireFFuncs(false).
+		SetRequireStringMsg(false))
 
 	cases := []struct {
 		name        string
@@ -82,7 +83,7 @@ func Test_newCheckers(t *testing.T) {
 		{
 			name:        "no config",
 			cfg:         config.Config{},
-			expRegular:  replace(enabledByDefaultRegularCheckers, zeroedFormatter),
+			expRegular:  replace(enabledByDefaultRegularCheckers, formatterWithoutEnabledOptions),
 			expAdvanced: enabledByDefaultAdvancedCheckers,
 		},
 		{
@@ -116,7 +117,7 @@ func Test_newCheckers(t *testing.T) {
 					checkers.NewSuiteTHelper().Name(),
 				},
 			},
-			expRegular: filter(replace(allRegularCheckers, zeroedFormatter), config.KnownCheckersValue{
+			expRegular: filter(replace(allRegularCheckers, formatterWithoutEnabledOptions), config.KnownCheckersValue{
 				checkers.NewRequireError().Name(),
 				checkers.NewSuiteTHelper().Name(),
 			}),
@@ -132,7 +133,7 @@ func Test_newCheckers(t *testing.T) {
 					checkers.NewSuiteTHelper().Name(),
 				},
 			},
-			expRegular:  replace(enabledByDefaultRegularCheckers, zeroedFormatter),
+			expRegular:  replace(enabledByDefaultRegularCheckers, formatterWithoutEnabledOptions),
 			expAdvanced: allAdvancedCheckers,
 		},
 		{
@@ -144,7 +145,7 @@ func Test_newCheckers(t *testing.T) {
 					checkers.NewRequireError().Name(),
 				},
 			},
-			expRegular: filter(replace(enabledByDefaultRegularCheckers, zeroedFormatter), config.KnownCheckersValue{
+			expRegular: filter(replace(enabledByDefaultRegularCheckers, formatterWithoutEnabledOptions), config.KnownCheckersValue{
 				checkers.NewNilCompare().Name(),
 				checkers.NewErrorNil().Name(),
 				checkers.NewRequireError().Name(),