search

nunnatsa / ginkgolinter

golang linter for ginkgo and gomega
MIT License
24 stars 6 forks source link

[Enhancement] #151

Open navijation opened 3 weeks ago

navijation commented 3 weeks ago

Is your feature request related to a problem? Please describe.

To(Succeed()) in Ginkgo has the rather peculiar behavior of failing when the Expect takes in multiple arguments. It is considered a misuse to use Succeed in this context according to Gomega docs.

Describe the solution you'd like Report an error if Expect(someFunction()).To(Succeed()) is called and the function returns multiple values, or if the number of arguments to Expect here is greater than 1.

Describe alternatives you've considered N/A

Additional context N/A

nunnatsa commented 3 weeks ago

Thanks @navijatio. I love this idea. I would make it even strict: to force only the checked value to be only a function call, that only returns one value of error type.

Do you want to implement this?

navijation commented 2 weeks ago

@nunnatsa yeah that makes sense, Expect(err).To(Not(Suceed()) doesn't feel very readable. I'll see if I or anyone I know can take this on this week.

nunnatsa commented 2 weeks ago

Thinking about it again, this is a good linter rule, but I think the real fix will be to fix the Succeed matcher itself, to always check the last return value, if it's a nil error, ignoring the rest of the return values, and won't limit itself to a single return value.

For example, this is a very common patterns when using gomega:

_, err := funcReturnsValueAndError()
Expect(err).ToNot(HaveOccurred())
...
Expect(funcReturnsOnlyError()).To(Succeed())

I think it makes more sense that the this code will work:

Expect(funcReturnsValueAndError()).To(Succeed())
...
Expect(funcReturnsOnlyError()).To(Succeed())

@onsi - what do you think?