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

[Enhancement] #151

Closed
navijation opened this issue Jun 13, 2024 · 3 comments · Fixed by #164 · May be fixed by #152
Closed

[Enhancement] #151

navijation opened this issue Jun 13, 2024 · 3 comments · Fixed by #164 · May be fixed by #152
Assignees
Labels
enhancement New feature or request

Comments

@navijation
Copy link

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

@navijation navijation added the enhancement New feature or request label Jun 13, 2024
@nunnatsa
Copy link
Owner

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
Copy link
Author

@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
Copy link
Owner

nunnatsa commented Jun 17, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants