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

[Enhancement] #151

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

[Enhancement] #151

navijation opened this issue Jun 13, 2024 · 3 comments · May be fixed by #152
Assignees
@nunnatsa
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.

navijation added a commit to navijation/ginkgolinter that referenced this issue Jun 16, 2024
@navijation
Resolves nunnatsa#151: mark incorrect usages of Succeed()
c721a1c
@navijation navijation linked a pull request Jun 16, 2024 that will close this issue
Open
10 tasks
@nunnatsa
Copy link
Owner

nunnatsa commented Jun 17, 2024 •
edited
Loading

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
Assignees

@nunnatsa nunnatsa

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Resolves #151: mark incorrect usages of Succeed() navijation/ginkgolinter
2 participants
@navijation @nunnatsa