Skip to content

Commit

Permalink
new checker 'encoded-compare' (#179)
Browse files Browse the repository at this point in the history
* merge master

* return README formatting

* self-review fixes

* self-review fixes

* self-review fixes

* encoded-compare: more cases from real world

* self-review fixes
  • Loading branch information
Antonboom authored Oct 3, 2024
1 parent 3c9db93 commit 8b28677
Show file tree
Hide file tree
Showing 18 changed files with 1,140 additions and 54 deletions.
36 changes: 35 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ https://golangci-lint.run/usage/linters/#testifylint
| [compares](#compares) |||
| [contains](#contains) |||
| [empty](#empty) |||
| [encoded-compare](#encoded-compare) |||
| [error-is-as](#error-is-as) || 🤏 |
| [error-nil](#error-nil) |||
| [expected-actual](#expected-actual) |||
Expand Down Expand Up @@ -274,6 +275,39 @@ assert.NotEmpty(t, err)

---

### encoded-compare

```go
assert.Equal(t, `{"foo": "bar"}`, body)
assert.EqualValues(t, `{"foo": "bar"}`, body)
assert.Exactly(t, `{"foo": "bar"}`, body)
assert.Equal(t, expectedJSON, resultJSON)
assert.Equal(t, expBodyConst, w.Body.String())
assert.Equal(t, fmt.Sprintf(`{"value":"%s"}`, hexString), result)
assert.Equal(t, "{}", json.RawMessage(resp))
assert.Equal(t, expJSON, strings.Trim(string(resultJSONBytes), "\n")) // + Replace, ReplaceAll, TrimSpace

assert.Equal(t, expectedYML, conf)

assert.JSONEq(t, `{"foo": "bar"}`, body)
assert.YAMLEq(t, expectedYML, conf)
```

**Autofix**: true. <br>
**Enabled by default**: true. <br>
**Reason**: Protection from bugs and more appropriate `testify` API with clearer failure message.

`encoded-compare` detects JSON-style string constants (usable in `fmt.Sprintf` also) and JSON-style/YAML-style named
variables. If variable is converted to `json.RawMessage`, then it is considered JSON unconditionally.

When fixing, `encoded-compare` removes unnecessary conversions to `[]byte`, `string`, `json.RawMessage` and calls of
`strings.Replace`, `strings.ReplaceAll`, `strings.Trim`, `strings.TrimSpace`, and adds a conversion to `string` when
needed.

---

### error-is-as

```go
Expand Down Expand Up @@ -650,7 +684,7 @@ assert.Equal(t, (chan Event)(nil), eventsChan)
assert.NotEqual(t, (chan Event)(nil), eventsChan)
```

But in the case of `Equal`, `NotEqual` and `Exactly` type casting approach still doesn't work for the function type.
But in the case of `Equal`, `NotEqual` and `Exactly` type conversion approach still doesn't work for the function type.

The best option here is to just use `Nil` / `NotNil` (see [details](https://github.com/stretchr/testify/issues/1524)).

Expand Down
46 changes: 23 additions & 23 deletions analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,29 @@ import (
func TestTestifyLint(t *testing.T) {
t.Parallel()

for _, checker := range checkers.All() {
checker := checker

t.Run(checker, func(t *testing.T) {
t.Parallel()

anlzr := analyzer.New()
if err := anlzr.Flags.Set("disable-all", "true"); err != nil {
t.Fatal(err)
}
if err := anlzr.Flags.Set("enable", checker); err != nil {
t.Fatal(err)
}

pkg := filepath.Join("checkers-default", checker)
analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), anlzr, pkg)
})
}
}

func TestTestifyLint_NotDefaultCases(t *testing.T) {
t.Parallel()

cases := []struct {
dir string
flags map[string]string
Expand Down Expand Up @@ -143,26 +166,3 @@ func TestTestifyLint(t *testing.T) {
})
}
}

func TestTestifyLint_CheckersDefault(t *testing.T) {
t.Parallel()

for _, checker := range checkers.All() {
checker := checker

t.Run(checker, func(t *testing.T) {
t.Parallel()

anlzr := analyzer.New()
if err := anlzr.Flags.Set("disable-all", "true"); err != nil {
t.Fatal(err)
}
if err := anlzr.Flags.Set("enable", checker); err != nil {
t.Fatal(err)
}

pkg := filepath.Join("checkers-default", checker)
analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), anlzr, pkg)
})
}
}
2 changes: 2 additions & 0 deletions analyzer/checkers_factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ func Test_newCheckers(t *testing.T) {
checkers.NewErrorNil(),
checkers.NewNilCompare(),
checkers.NewErrorIsAs(),
checkers.NewEncodedCompare(),
checkers.NewExpectedActual(),
checkers.NewRegexp(),
checkers.NewSuiteExtraAssertCall(),
Expand All @@ -43,6 +44,7 @@ func Test_newCheckers(t *testing.T) {
checkers.NewErrorNil(),
checkers.NewNilCompare(),
checkers.NewErrorIsAs(),
checkers.NewEncodedCompare(),
checkers.NewExpectedActual(),
checkers.NewRegexp(),
checkers.NewSuiteExtraAssertCall(),
Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

46 changes: 46 additions & 0 deletions internal/analysisutil/encoded.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package analysisutil

import "strings"

var whitespaceRemover = strings.NewReplacer("\n", "", "\\n", "", "\t", "", "\\t", "", " ", "")

// IsJSONLike returns true if the string has JSON format features.
// A positive result can be returned for invalid JSON as well.
func IsJSONLike(s string) bool {
s = whitespaceRemover.Replace(unescape(s))

var startMatch bool
for _, prefix := range []string{
`{{`, `{[`, `{"`,
`[{{`, `[{[`, `[{"`,
} {
if strings.HasPrefix(s, prefix) {
startMatch = true
break
}
}
if !startMatch {
return false
}

for _, keyValue := range []string{`":{`, `":[`, `":"`} {
if strings.Contains(s, keyValue) {
return true
}
}
return false

// NOTE(a.telyshev): We do not check the end of the string, because this is usually a field for typos.
// And one of the reasons for using JSON-specific assertions is to catch typos like this.
}

func unescape(s string) string {
s = strings.ReplaceAll(s, `\"`, `"`)
s = unquote(s, `"`)
s = unquote(s, "`")
return s
}

func unquote(s string, q string) string {
return strings.TrimLeft(strings.TrimRight(s, q), q)
}
64 changes: 64 additions & 0 deletions internal/analysisutil/encoded_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package analysisutil_test

import (
"testing"

"github.com/Antonboom/testifylint/internal/analysisutil"
)

func TestIsJSONLike(t *testing.T) {
cases := []struct {
in string
expected bool
}{
{
in: `[{"name": "values-files", "array": ["values-dev.yaml"]}, {"name": "helm-parameters", "map": {"image.tag": "v1.2.3"}}]`,
expected: true,
},
{
in: `{"labels":{"aaa":"111"},"annotations":{"ccc":"333"}}`,
expected: true,
},
{
in: "{\"message\":\"No user was found in the LDAP server(s) with that username\"}",
expected: true,
},
{
in: `"{\n \"first\": \"Tobi\",\n \"last\": \"Ferret\"\n}"`,
expected: true,
},
{
in: `"{\"message\":\"No user was found in the LDAP server(s) with that username\"}"`,
expected: true,
},
{
in: `{"uuid": "b65b1a22-db6d-4f5a-9b3d-7302368a82e6"}`,
expected: true,
},
{
in: `apiVersion: 3`,
expected: false,
},
{
in: `[{}]`,
expected: false,
},
{
in: `{{ .TemplateVar }}`,
expected: false,
},
{
in: `{{-.TemplateVar}}`,
expected: false,
},
}

for _, tt := range cases {
t.Run("", func(t *testing.T) {
isJSON := analysisutil.IsJSONLike(tt.in)
if isJSON != tt.expected {
t.FailNow()
}
})
}
}
4 changes: 2 additions & 2 deletions internal/checkers/bool_compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,15 @@ func (checker BoolCompare) Check(pass *analysis.Pass, call *CallMeta) *analysis.
case xor(t1, t2):
survivingArg, _ := anyVal([]bool{t1, t2}, arg2, arg1)
if call.Fn.NameFTrimmed == "Exactly" && !isBuiltinBool(pass, survivingArg) {
// NOTE(a.telyshev): `Exactly` assumes no type casting.
// NOTE(a.telyshev): `Exactly` assumes no type conversion.
return nil
}
return newUseTrueDiagnostic(survivingArg, arg1.Pos(), arg2.End())

case xor(f1, f2):
survivingArg, _ := anyVal([]bool{f1, f2}, arg2, arg1)
if call.Fn.NameFTrimmed == "Exactly" && !isBuiltinBool(pass, survivingArg) {
// NOTE(a.telyshev): `Exactly` assumes no type casting.
// NOTE(a.telyshev): `Exactly` assumes no type conversion.
return nil
}
return newUseFalseDiagnostic(survivingArg, arg1.Pos(), arg2.End())
Expand Down
1 change: 1 addition & 0 deletions internal/checkers/checkers_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ var registry = checkersRegistry{
{factory: asCheckerFactory(NewErrorNil), enabledByDefault: true},
{factory: asCheckerFactory(NewNilCompare), enabledByDefault: true},
{factory: asCheckerFactory(NewErrorIsAs), enabledByDefault: true},
{factory: asCheckerFactory(NewEncodedCompare), enabledByDefault: true},
{factory: asCheckerFactory(NewExpectedActual), enabledByDefault: true},
{factory: asCheckerFactory(NewRegexp), enabledByDefault: true},
{factory: asCheckerFactory(NewSuiteExtraAssertCall), enabledByDefault: true},
Expand Down
2 changes: 2 additions & 0 deletions internal/checkers/checkers_registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func TestAll(t *testing.T) {
"error-nil",
"nil-compare",
"error-is-as",
"encoded-compare",
"expected-actual",
"regexp",
"suite-extra-assert-call",
Expand Down Expand Up @@ -81,6 +82,7 @@ func TestEnabledByDefault(t *testing.T) {
"error-nil",
"nil-compare",
"error-is-as",
"encoded-compare",
"expected-actual",
"regexp",
"suite-extra-assert-call",
Expand Down
99 changes: 99 additions & 0 deletions internal/checkers/encoded_compare.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package checkers

import (
"go/ast"

"golang.org/x/tools/go/analysis"
)

// EncodedCompare detects situations like
//
// assert.Equal(t, `{"foo": "bar"}`, body)
// assert.EqualValues(t, `{"foo": "bar"}`, body)
// assert.Exactly(t, `{"foo": "bar"}`, body)
// assert.Equal(t, expectedJSON, resultJSON)
// assert.Equal(t, expBodyConst, w.Body.String())
// assert.Equal(t, fmt.Sprintf(`{"value":"%s"}`, hexString), result)
// assert.Equal(t, "{}", json.RawMessage(resp))
// assert.Equal(t, expJSON, strings.Trim(string(resultJSONBytes), "\n")) // + Replace, ReplaceAll, TrimSpace
//
// assert.Equal(t, expectedYML, conf)
//
// and requires
//
// assert.JSONEq(t, `{"foo": "bar"}`, body)
// assert.YAMLEq(t, expectedYML, conf)
type EncodedCompare struct{}

// NewEncodedCompare constructs EncodedCompare checker.
func NewEncodedCompare() EncodedCompare { return EncodedCompare{} }
func (EncodedCompare) Name() string { return "encoded-compare" }

func (checker EncodedCompare) Check(pass *analysis.Pass, call *CallMeta) *analysis.Diagnostic {
switch call.Fn.NameFTrimmed {
case "Equal", "EqualValues", "Exactly":
default:
return nil
}

if len(call.Args) < 2 {
return nil
}
lhs, rhs := call.Args[0], call.Args[1]

a, aIsExplicitJSON := checker.unwrap(pass, call.Args[0])
b, bIsExplicitJSON := checker.unwrap(pass, call.Args[1])

var proposed string
switch {
case aIsExplicitJSON, bIsExplicitJSON, isJSONStyleExpr(pass, a), isJSONStyleExpr(pass, b):
proposed = "JSONEq"
case isYAMLStyleExpr(a), isYAMLStyleExpr(b):
proposed = "YAMLEq"
}

if proposed != "" {
return newUseFunctionDiagnostic(checker.Name(), call, proposed,
newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
Pos: lhs.Pos(),
End: lhs.End(),
NewText: formatWithStringCastForBytes(pass, a),
}, analysis.TextEdit{
Pos: rhs.Pos(),
End: rhs.End(),
NewText: formatWithStringCastForBytes(pass, b),
}))
}
return nil
}

// unwrap unwraps expression from string, []byte, strings.Replace(All), strings.Trim(Space) and json.RawMessage conversions.
// Returns true in the second argument, if json.RawMessage was in the chain.
func (checker EncodedCompare) unwrap(pass *analysis.Pass, e ast.Expr) (ast.Expr, bool) {
ce, ok := e.(*ast.CallExpr)
if !ok {
return e, false
}
if len(ce.Args) == 0 {
return e, false
}

if isJSONRawMessageCast(pass, ce) {
if isNil(ce.Args[0]) { // NOTE(a.telyshev): Ignore json.RawMessage(nil) case.
return checker.unwrap(pass, ce.Args[0])
}

v, _ := checker.unwrap(pass, ce.Args[0])
return v, true
}

if isIdentWithName("string", ce.Fun) ||
isByteArray(ce.Fun) ||
isStringsReplaceCall(pass, ce) ||
isStringsReplaceAllCall(pass, ce) ||
isStringsTrimCall(pass, ce) ||
isStringsTrimSpaceCall(pass, ce) {
return checker.unwrap(pass, ce.Args[0])
}
return e, false
}
Loading

0 comments on commit 8b28677

Please sign in to comment.