Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formatter: support non-string-message checks #221

Merged
merged 9 commits into from
Feb 15, 2025
1 change: 1 addition & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
48 changes: 44 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 ./...
Expand Down Expand Up @@ -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:
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -981,6 +1020,7 @@ assert.False(t, num != num)
```

And against these meaningless assertions:

```go
assert.Empty(t, "")
assert.False(t, false)
Expand Down
2 changes: 1 addition & 1 deletion Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
9 changes: 9 additions & 0 deletions analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
{
Expand Down
1 change: 1 addition & 0 deletions analyzer/checkers_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 7 additions & 6 deletions analyzer/checkers_factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
},
{
Expand Down Expand Up @@ -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(),
}),
Expand All @@ -132,7 +133,7 @@ func Test_newCheckers(t *testing.T) {
checkers.NewSuiteTHelper().Name(),
},
},
expRegular: replace(enabledByDefaultRegularCheckers, zeroedFormatter),
expRegular: replace(enabledByDefaultRegularCheckers, formatterWithoutEnabledOptions),
expAdvanced: allAdvancedCheckers,
},
{
Expand All @@ -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(),
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading