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

Feature request: CallbackAsync #737

Closed
jamietwells opened this issue Dec 21, 2018 · 6 comments
Closed

Feature request: CallbackAsync #737

jamietwells opened this issue Dec 21, 2018 · 6 comments

Comments

@jamietwells
Copy link

jamietwells commented Dec 21, 2018

If you're mocking an implementation and a class has method you want to test it might have a parameter that is a Task<something> if this is the case, you might want to ensure the something is correct so you might do a callback:

mock
    .Setup(...)
    .Callback(task async => (await task).Should().Be(expectedSomething));

mock.TheMethod(null);

Unfortunately, the Callback signature is Action<Task> not Func<Task, Task> so the callback is not awaited and the exception is lost (in this case it is of course the NRE from awaiting null).

(This came about because I was trying to verify the Content of an HttpRequestMessage was the expected content, but of course you must do request.Content.ReadAsStringAsync() to get the passed string, which I couldn't do!)

I know just thinking about it it seems impossible to implement because where would the await go? Inside wherever the callback is called but then who would await that task? At the end of the day the test is the starting point and there's no guarantee the test is async.

Anyway, I just thought I'd ask and get an opinion on if this feature seems possible, and can I also say how much I love this package! :)

@stakx
Copy link
Contributor

stakx commented Dec 21, 2018

This reminds me of two issues over at the repo for DynamicProxy (Castle.Core) about async interception. I suspect that even if we tried to implement what you ask for, we'd run into a roadblock with DynamicProxy and thread safety issues. But perhaps we don't even have to get that far... I suspect there might be a problem with the feature request per se:

If you are setting up a method that returns a Task<T>, then you can move your code to a .Return callback. (This makes sense when you think about how a void method (think .Callback) suddenly gets a return value (think .Return) when you convert it to an async method in the proper fashion.) That's what I'd recommend here.

If you are set on setting up a method that does not return a Task, then I wonder how you would do what you're trying to do if you wrote your mock by hand, i.e. without Moq. You cannot await inside a method that isn't async. (Well, you could if the return type were void, but that's considered a bad idea due to the possibility of swallowed exceptions.) If you wanted to call an async method from a non-async method, you'd either have to block synchronously using e.g. asyncMethod().GetAwaiter().GetResult(), or start a new task on another thread but then not block & wait for it. You have exactly the same choices inside a Moq .Callback() as far as I can tell right now. And that's not due to a limitation of Moq but simply due to how async works.

@jamietwells
Copy link
Author

jamietwells commented Dec 21, 2018

Yeah, I was thinking this when I was writing the request. It sounds impossible. I asked on stack overflow and someone had the great idea that I should just assign the task parameter to a variable in an outside scope and await it there. I'm just so used to writing the callback like: param => param.thing.should().Be(correct value) that it just didn't occur to me to assign it to a variable outside the lambda.

Feel free to close the request if it's impractical or impossible as I have a perfectly good workaround for now.

At least if anyone else is being as dense as me (unlikely I know) and they're looking for a similar feature hopefully they'll come across this and can make use of this technique.

@stakx
Copy link
Contributor

stakx commented Dec 21, 2018

Expanded my answer above a little. Let's leave your issue open for tiny little while longer, in case someone has any another idea or suggestion.

@jamietwells
Copy link
Author

jamietwells commented Dec 21, 2018

Actually, I've just thought about it a teensy bit longer. The only way I see it working is if you did something like:

mock
    .Setup(m => m.InvokeMethod(It.IsAny<Whatever>()))
    .CallbackAsync(p async => (await p.GetValue()).Should().Be(correct));

mock.Object.InvokeMethod(testParameter);

await mock.AwaitAsyncCallbacks();

@stakx
Copy link
Contributor

stakx commented Dec 22, 2018

@jamietwells - Agreed that something like it is generally feasible. It should be possible to implement such a CallbackAsync and AwaitAsyncCallbacks as extension methods on top of the existing API.

I'm however cautious about having something like this added to Moq. It may not be a frequent-enough scenario to warrant addition to the core library, and it may actually hinder an understanding of how async works; i.e. how the task inside the callback gets captured and how it relates to the actual method being called. Some people may end up thinking that a method called CallbackAsync implies that the method being set up is an async one (which is not the case). In my personal opinion, it might be less confusing to just openly capture, inside the callback, a task in a variable and then await it after the actuall mock method call (as someone already appears to have suggested to you).

@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

Closing for the reasons already given above. Better IMO to let users capture the task inside the callback (e.g. using a capturing matcher, i.e. Capture.In) and then await those tasks outside the setup.

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

No branches or pull requests

2 participants