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.

Sign up for GitHub

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

Jump to bottom

Bad suggested fix for negative-positive #210

Open
denik opened this issue Jan 2, 2025 · 2 comments · May be fixed by #212
Open

Bad suggested fix for negative-positive #210

denik opened this issue Jan 2, 2025 · 2 comments · May be fixed by #212
Labels
bug Something isn't working

Comments

@denik
Copy link

denik commented Jan 2, 2025 •
edited
Loading 

% cat my_test.go
package main

import (
        "testing"

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

func TestCommandsDontUseUnderscoreInName(t *testing.T) {
        x := []string{}
        assert.True(t, len(x) > 0)
}

% testifylint --fix my_test.go
/Users/denis.bilenko/work/cli/my_test.go:11:2: negative-positive: use assert.Positive

% diff -u my_test.go.bak my_test.go
--- my_test.go.bak      2025-01-02 11:47:19
+++ my_test.go  2025-01-02 11:49:03
@@ -8,5 +8,5 @@

 func TestCommandsDontUseUnderscoreInName(t *testing.T) {
        x := []string{}
-       assert.True(t, len(x) > 0)
+       assert.Positive(t, x)
 }

Expected assert.NotEmpty(t, x) or assert.Positive(t, len(x)) but not the above.
@denik denik mentioned this issue Jan 2, 2025
Merged
denik added a commit to databricks/cli that referenced this issue Jan 2, 2025
@denik
Enable testifylint and fix the issues (#2065)
0b80784 
## Changes
- Enable new linter: testifylint.
- Apply fixes with --fix.
- Fix remaining issues (mostly with aider).

There were 2 cases we --fix did the wrong thing - this seems to a be a
bug in linter: Antonboom/testifylint#210

Nonetheless, I kept that check enabled, it seems useful, just need to be
fixed manually after autofix.

## Tests
Existing tests
@ccoVeille
Copy link
Contributor

I think I'm the one who added the negative-positive linter.

I will take a look in the next days

Thanks for reporting the issue

@ccoVeille
Copy link
Contributor

ccoVeille commented Jan 3, 2025 •
edited
Loading

The problem is right here and might be more general than reported by this

testifylint/internal/checkers/negative_positive.go

Line 155 in b5976b4

return newUsePositiveDiagnostic(expr.Pos(), expr.End(), untype(survivingArg)) 

// untype returns v from type(v) expression or v itself if there is no type conversion.
func untype(e ast.Expr) ast.Expr {
	ce, ok := e.(*ast.CallExpr)
	if !ok || len(ce.Args) != 1 {
		return e
	}
	return ce.Args[0]
}

The call to untype transforms not only the int(8) to 8 (this one is legitimate), but also the len(whatever) to whatever (here is the bug)

untype was added by myself, and nothing else use it. I will check for a better test, or simply existing another helper available in testifylint

@ccoVeille ccoVeille linked a pull request Jan 4, 2025 that will close this issue
Open
@Antonboom Antonboom added the bug Something isn't working label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: negative positive with type conversion ccoveille-forks/Antonboom-testifylint
3 participants
@denik @ccoVeille @Antonboom