Resolves #151: mark incorrect usages of Succeed() #152
Conversation
tests/e2e.sh
Outdated
| @@ -6,34 +6,36 @@ cp ginkgolinter testdata/src/a | |||
| cd testdata/src/a | |||
|
|
|||
| # no suppress | |||
| [[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2604 ]] | |||
| [[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2628 ]] | |||
There was a problem hiding this comment.
Inspection of succeed/succeed.go and succeedconfig/succeed.go shows that 24 new errors should be reported. 8 of these assertions of the form Expect(err).To(Succeed()) vanish when succeed suppression is enabled, leaving 16 extra.
So for each test case here, the number of errors should rise by either 24 or 16.
navijation
changed the title
tests/e2e.sh
Outdated
| # suppress all but spec pollution | ||
| [[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-spec-pollution=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 253 ]] | ||
| [[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-spec-pollution=true --suppress-type-compare-assertion=true --suppress-succeed-assertion=true a/... 2>&1 | wc -l) == 275 ]] |
There was a problem hiding this comment.
This one goes up by 22 errors... close to 24 but not quite. Probably fine though...
tests/e2e.sh
Outdated
| # suppress spec pollution | ||
| [[ $(./ginkgolinter --forbid-spec-pollution=true a/... 2>&1 | wc -l) == 2702 ]] | ||
| [[ $(./ginkgolinter --forbid-spec-pollution=true a/... 2>&1 | wc -l) == 2732 ]] |
There was a problem hiding this comment.
This one goes up by 30 errors. I did not intend to add that many new errors. Maybe it's because my test data changes inadvertently had some spec pollution going on.
There was a problem hiding this comment.
Thanks you so much for this PR @navijation!
I added a few preliminary comments. I didn't reviewed the logic of the new rule, yet.
One general comment: please also update the Readme file.
testdata/src/a/succeed/succeed.go
Outdated
| func safeDivide(num int, denom int) (int, error) { | ||
| if denom == 0 { | ||
| return 0, errors.New("divide by zero") | ||
| } | ||
| return num / denom, nil | ||
| } | ||
|
|
||
| func attemptToCallNetwork(shouldTry bool) error { | ||
| if !shouldTry { | ||
| return nil | ||
| } | ||
| return errors.New("don't know how to connect to network") | ||
| } |
There was a problem hiding this comment.
nit: thinking out load (not sure about it), but since we doing a static analysis here, we really don't care about the logic of these functions and what is actually been returned on runtime.
So what I'm not sure about is if using an actual logic will confuse a future reader. Because we don't know and don't care what the function will return on runtime, but only what it can return.
testdata/src/a/succeed/succeed.go
Outdated
| wrap := struct { | ||
| err error | ||
| }{ | ||
| err: err0, | ||
| } |
There was a problem hiding this comment.
should we care about wrapped errors? Is it a real usecase?
There was a problem hiding this comment.
I just added this to test the case where the actual argument is a neither a function call nor an identifier. I could remove it, although more case coverage wouldn't hurt I'd think.
| value0, err0 := safeDivide(0, 0) | ||
| value1, _ := safeDivide(0, 1) | ||
|
|
||
| Expect(value0, err0).ToNot(Succeed()) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail` |
There was a problem hiding this comment.
There are two issues here: this is also not a function call. Is there any way to catch them both? One of the concepts I'm trying to apply in this linter, is that if the user fixes a warning, they won't get a new warning for the fixed code.
There was a problem hiding this comment.
So skip directly to Expect(err0).ToNot(HaveOccurred())? This seems to imply that I should add a suggested fix for the original type error. It's a bit more challenging to do that in general. e.g. For Expect(os.ReadFile("someFile")).To(Succeed()) and Expect(err1, err2).To(Succeed()), what would the suggested fixes be? I can take a look at doing this but some guidance would be helpful.
IMO, multiple issues are not ideal but not the end of the world. It's like having a Go type error. Generally you need to fix those before fixing other lint errors, which will come after the fix.
There was a problem hiding this comment.
Also note that some of these suggested fixes can either change the code semantics by dropping function calls or introduce compiler errors due to unused variables. It might be better to let the user figure out how they want to satisfy the type constraint.
There was a problem hiding this comment.
The linter only suggests fixes if it can do it without changing the test logic. The suggested fixes is less important than the warnings.
For example, the "expect without assertion" rule does not offer any fix.
There was a problem hiding this comment.
I may have misunderstood. Are you suggesting that that the linter should say “Succeed matcher must be asserted against a function call returning one error, or it will always fail”.
I can do that, but it’s sort of an inaccurate message because it is not an error to use it against a value, just a style issue. Some users might suppress the style issue. If that isn’t a problem I can simplify the message that way, or complexify it like “Succeed matcher must be asserted against a single expression of type error, ideally a function call, or it will always fail”.
There was a problem hiding this comment.
@nunnatsa I'm still not entirely sure what you're proposing here so I held off on making a change.
Option 1
Keep it as is. The user can fix the type error first and then the style error if applicable based on their config.
Option 2
Change the linter to say "Succeed matcher must be asserted against a function call returning exactly one error, or it will always fail" so that the user changes the usage to be that of a function call immediately to avoid the style error and type error.
Option 3
Change the linter to say “Succeed matcher must be asserted against a single expression of type error, ideally a function call, or it will always fail” or something of that sort.
There was a problem hiding this comment.
What I meant is that the reportBuilder supports multiple warnings. I'm not trying to build a special warning message for each case, but gather all the relevant warnings. see here for example:
There was a problem hiding this comment.
I see...
One problem is that the fixes are kind of conflicting. If you switch to using Expect(err).ToNot(HaveOccurred()), it's no longer relevant to fix the signature to match what Succeed expects.
And secondly, it means the linter needs to figure out a suggestion in the special case where exactly one argument is an error, e.g. Expect(5, err).To(Succeed()) can be suggest fixed as Expect(err).ToNot(HaveOccurred()), but Expect(5).To(Succeed()) has no suggested fix. And I'm not sure what the suggested fix should be for Expect(err1, err2).To(Succeed()).
So what I gather is:
- if it is possible and configured to suggest a switch to
Expect(...).NotTo(HaveOccurred()), then do that
a. "possible" meaning exactly a single argument implementserrorand the rest do not
b. although fixing this error as such will likely lead to unused variable errors from the Go compiler that the user will still have to fix - (maybe) Do not post the warning about how
Succeed's expected argument types work if condition 1 holds
This complicates things. I can manage it, but something about this approach feels off to me, especially point 1b.
Pull Request Test Coverage Report for Build 9542355837Details
💛 - Coveralls |
| @@ -7,16 +7,18 @@ import ( | |||
|
|
|||
| var _ = Describe("gomega var", func() { | |||
| It("in a valid Eventually", func() { | |||
| Eventually(func(g Gomega) { | |||
| Eventually(func(g Gomega) error { | |||
There was a problem hiding this comment.
This is actually a valid usecase, documented here: https://onsi.github.io/ginkgo/#testing-external-systems
We must not trigger a warning for code like:
Eventually(func (g Gomega){
g.Expect(...)
...
}).Should(Succeeed())| value0, err0 := safeDivide(0, 0) | ||
| value1, _ := safeDivide(0, 1) | ||
|
|
||
| Expect(value0, err0).ToNot(Succeed()) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail` |
There was a problem hiding this comment.
The linter only suggests fixes if it can do it without changing the test logic. The suggested fixes is less important than the warnings.
For example, the "expect without assertion" rule does not offer any fix.
linter/ginkgo_linter.go
Outdated
|
|
||
| isAsync := false | ||
| return doCheckSucceed( | ||
| pass, assertionExpCopy, matcherCopy, actualArgs, handler, reportBuilder, isAsync, |
There was a problem hiding this comment.
isAsync is always false. so we don't need this variable.
There was a problem hiding this comment.
This is more for readability purposes because boolean arguments otherwise are unreadable. I can remove though
| @@ -396,21 +401,25 @@ func doCheckExpression(pass *analysis.Pass, config types.Config, assertionExp *a | |||
|
|
|||
| } else if checkMatchError(pass, assertionExp, actualArg, handler, reportBuilder, isExprError) { | |||
| return false | |||
| } else if checkSucceedSync(pass, assertionExp, actualArgs, handler, reportBuilder, bool(config.SuppressSucceed)) { | |||
There was a problem hiding this comment.
nit: can we split it to first check if we're using Succeed() here, and then validate the usage of it?
There was a problem hiding this comment.
If I understand what you're saying correctly, i.e. something like if isSucceed(...) { return checkSucceed(...) }, this will prevent the rules below from being checked even if the usage of Succeed is fine.
Also, most of the checks here work the same way of doing both structure check as well as reporting. What's special about Succeed?
linter/ginkgo_linter.go
Outdated
| @@ -1637,6 +1792,23 @@ func isExprError(pass *analysis.Pass, expr ast.Expr) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // Returns whether the expression implements type error or is a tuple of length 1 whose only | |||
| // element implements type error. | |||
| func isExprExactError(pass *analysis.Pass, expr ast.Expr) bool { | |||
There was a problem hiding this comment.
I'm confused by the name of this function. Maybe I'm wrong here, but could it be that the logic is reverse than what its name suggests?
There was a problem hiding this comment.
This is checking to see if the expression evaluates to a single value which is an error, as opposed to multiple values of a tuple where the first is an error. Maybe something like isExprSingleError?
linter/ginkgo_linter.go
Outdated
|
|
||
| // Negate an assertion by negating the assertion function, e.g. turn `Expect(x).To(BeEmpty())` | ||
| // to `Expect(x).ToNot(BeEmpty())` | ||
| func negateAssertion(assertionExpr *ast.CallExpr, handler gomegahandler.Handler) bool { |
There was a problem hiding this comment.
This is done by function in the reverseassertion package. If these functions are not the prefect match here, please move this function to this package
linter/ginkgo_linter.go
Outdated
| assertionExpCopy := astcopy.CallExpr(assertionExp) | ||
| matcherCopy := assertionExpCopy.Args[0].(*ast.CallExpr) | ||
|
|
||
| isAsync := true |
There was a problem hiding this comment.
this variable is not needed. It never changed
| // | ||
| // // function signature is `os.ReadFile(string) ([]byte, error)` | ||
| // Eventually(func() ([]byte, error) { return os.ReadFile("someFile") }).Should(Succeed()) | ||
| func checkSucceedAsync( |
There was a problem hiding this comment.
Be careful here. The following code is valid, but will trigger a warning with the current logic:
Eventually(func(g Gomega){
...
g.Expect(...)...
...
}).Should(Succeed())
Pull Request Test Coverage Report for Build 9639348734Details
💛 - Coveralls |
|
@navijation - would you mind to change the commits, so they will include only changes from main, and not from each other? For example, if a commit added some code that was later modified or removed, please use the final version of the change in the commit. we have now 14 commits in this PR, and I think we can do that with 4-5 commits. For a specific example, the "fix bug" commit should be rebased with the commit that added the code before the fix. |
navijation
force-pushed
the
nav/enforce-correct-succeed-usage
branch
from
9232a75
to
6387fd1
Compare
|
@nunnatsa I've rebased as 4 commits to the best of my ability. Although I would probably recommend squash merging this PR since the boundaries are a little blurry between each commit after merging them. |
navijation
force-pushed
the
nav/enforce-correct-succeed-usage
branch
from
6387fd1
to
76d171c
Compare
Pull Request Test Coverage Report for Build 9739311110Details
💛 - Coveralls |
|
|
||
| // Note: we cannot check for an argument which _implements_ gomega.Gomega without adding a Go | ||
| // module dependency on gomega. This is because the gomega.Gomega interface definition references | ||
| // gomega.AsyncAssertion, whose methods return gomega.AsyncAssertion. Because Go does not have | ||
| // interface covariance or contravariance, any "local copy" of gomega.AsyncAssertion cannot | ||
| // be satisified by any actual `gomega.AsyncAssertion` implementation, as the methods do not return | ||
| // local.AsyncAssertion but rather gomega.AsyncAssertion. | ||
| // | ||
| // Also, Gomega probably doesn't even accept an argument whose type implements the interface, but | ||
| // rather whose type _is_ the interface. So this check should suffice. | ||
| func IsGomega(t gotypes.Type) bool { | ||
| named, isNamed := t.(*gotypes.Named) | ||
| if !isNamed { | ||
| return false | ||
| } | ||
|
|
||
| obj := named.Obj() | ||
|
|
||
| if obj.Name() != "Gomega" { | ||
| return false | ||
| } | ||
|
|
||
| return isPackageSymbol(named.Obj(), "github.com/onsi/gomega/types", "Gomega") | ||
| } | ||
|
|
||
| func isPackageSymbol(obj types.Object, pkgPath, name string) bool { | ||
| if obj.Name() != name { | ||
| return false | ||
| } | ||
|
|
||
| vendorPieces := strings.Split(pkgPath, "/vendor/") | ||
|
|
||
| return pkgPath == vendorPieces[len(vendorPieces)-1] | ||
| } |
There was a problem hiding this comment.
I think, but not sure, that we can use the gomegahandler package for this check. The handler only exists if the gomega package is imported, and holds the knowledge if it's dot import or named import.
So you'll only need to check if the type name is Gomega and the left side, if exists, is expected by the handler. I think it simpler. WDYT?
| resp := struct { | ||
| response any | ||
| err error | ||
| }{ | ||
| response: nil, | ||
| err: err0, | ||
| } |
There was a problem hiding this comment.
Not sure testing an error field adds any value, over testing an error variable.
linter/ginkgo_linter.go
Outdated
| // Returns whether the expression is a function that takes in a single gomega.Gomega argument and | ||
| // returns nothing, e.g. `func(g gomega.Gomega) { g.Expect(myStringMaker()).ToNot(BeEmpty()) }` |
There was a problem hiding this comment.
The Gomega parameter is not necessary a single parameter. The check should be about the first parameter, if exists.
linter/ginkgo_linter.go
Outdated
| if t.Results().Len() != 0 || t.Params().Len() != 1 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Continue: this condition will give the wrong result for valid usages like:
Eventually(func(g Gomega, ctx context.Context){...})...Or
Eventually(func(g Gomega, x, y int){
g.Expect(x).To(Equal(y))
}).WithArguments(1,2).Should(Succeed())
|
|
||
| if isAsync { | ||
| if len(actualArgs) != 1 || | ||
| !(isExprErrFunc(pass, actualArgs[0]) || isExprGomegaFunc(pass, actualArgs[0])) { |
There was a problem hiding this comment.
the function under test is not always the first argument. e.g. this code is valid:
Eventually(ctx, func(...){...}).Should(Succeed())Eventually is with very flexible api...
There was a problem hiding this comment.
This is actually checked here: https://github.com/nunnatsa/ginkgolinter/blob/main/linter/ginkgo_linter.go#L743-L759
Description
Fixes #151. Add a new, partially suppressible error set for calls to
Succeed.Whenever the actual argument is not a single error value for synchronous calls, or not a function returning a single error value for asynchronous calls, this creates a non-suppressible error.
Whenever the actual argument is anything but a function call, e.g.
Expect(err).To(Succeed()), this creates a suppressible error that can be suppressed with command line flags or comment directives, and suggests a replacement toExpect(err).To(HaveOccurred()).Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
testdata/src/a/succeed/succeed.gofor test cases in strict configtestdata/src/a/succeedconfig/succeedconfig.gofor test cases with suppressed configanalyzer_test.gobetween two lint rules
Checklist:
@nunnatsa