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

AsyncAssertion can't handle Eventually(functionReturnsNilError, <interval>, <interval>).Should(BeNil()) #555

Closed
kaovilai opened this issue May 25, 2022 · 5 comments

Comments

@kaovilai
Copy link

kaovilai commented May 25, 2022

It will panic when actualType.Kind() gets called

case actualType.Kind() != reflect.Func:

Failing sample parameter to function.
actualType is AsyncAssertionType(0)
actualInput is nil <interface {}>

example function

func returnsNilError() error {
    return nil
}
@thediveo
Copy link
Collaborator

Can you please show the code that you are using with Eventually(???) and especially what you are passing into Eventually, as well as the definition of your function you're passing? Could it be that you are passing the result of calling your function instead of passing the function? ...because you say that your test panics in async_assertion.go, line 43 ... and this is when creating the async assertion and thus Eventually(nil, ...).

PS: it would be helpful if you could report the original panic trace in such situations, as it gives us a much better picture, especially with respect to the parameters (including the offending actualInput arg, respectively actual) a few levels deeper down the stack.

@kaovilai
Copy link
Author

You maybe right about the passing va LL he instead of function. If I get the time to try again I'll report here.

Still panicking should be avoided. You should be able to use defer recover pattern to give a more helpful error.

@thediveo
Copy link
Collaborator

@onsi This actually raises the question of how we should handle a "constant" nil value. Is passing in a constant nil actually a valid use case or should we consider it to be a structural test fault where we should panic with a clear description as to why a permanent nil is considered to be a sign of the test programming making a mistake?

@thediveo
Copy link
Collaborator

This should be fixed now and passing in a nil value won't cause a panic but also not warn you about probably doing the wrong thing; this is in line with passing in other non-func values. If we should still please miss something, please feel free to reopen and describe in detail what's missing. Thank you very much for reporting this bug!

@kaovilai
Copy link
Author

kaovilai commented Aug 5, 2022

Thanks @thediveo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants