Add ReturnsTaskResult for async methods

[mariusGundersen]

Async methods returns a generic Task, so when using the Returns() extension method I first need to wrap the (mock) result in Task.FromResult, which gets tedious. So I propose we add the following extension methods, which wrap the provided results as Tasks:

public static ConfiguredCall ReturnsAsyncResult<T>(this Task<T> value, T returnThis, params T[] returnThese)
{
    return value.Returns(Task.FromResult(returnThis), returnThese.Select(Task.FromResult).ToArray());
}

public static ConfiguredCall ReturnsAsyncResult<T>(this Task<T> value, Func<CallInfo, T> returnThis, params Func<CallInfo, T>[] returnThese)
{
    var returnTheseAsyncResults = returnThese.Select<Func<CallInfo, T>, Func<CallInfo, Task<T>>>(
        x => k => Task.FromResult(x(k))).ToArray();
    return value.Returns(
        x => Task.FromResult(returnThis(x)),
        returnTheseAsyncResults);
}

It can now be used like so:

var mocked = Substitute.For<IAmAnAsyncService>();
mocked.AsyncMethod(Arg.Any<string>())
       .ReturnsAsyncResult("Some result that will be wrapped in a Task");

[dtchepak]

I think this is a good idea, but I wonder if it would be better to use a non-NSub extension method that is more widely applicable? Something like:

public static Task<T> ToTask<T>(this T value) {
    return Task.FromResult(value);
}

While ReturnsAsyncResult(...) would be added to all Task<T> values (even when that wouldn't make sense), the ToTask(...) method could be relevant anywhere, including during stubbing:

var mocked = Substitute.For<IAmAnAsyncService>();
mocked.AsyncMethod(Arg.Any<string>())
       .Returns("Some result that will be wrapped in a Task".ToTask());

Not as neat as the proposed extension, but I already feel guilty for dumping extension methods on everything and making non-sensical things like 2.Returns(3) legal, so I'm trying to be extra careful with these things now. :)

What do you think?

[mariusGundersen]

ToTask() doesn't feel like something that should be part of NSubstitute, as it is useful in many other situations too. It wasn't difficult to add the proposed methods to a static class in my test project, so I have the functionality I want in my tests, I just thought it would be useful for others too.

As for dumping extension methods everywhere; lots of built in methods (like Task.FromResult or string.IsNullOrWhiteSpace) would have been nice as extension methods as well, so I don't think you should feel too guilty about it. And since the proposed methods work pretty much like existing methods they don't feel like new extension methods, more like overloads of existing methods.

[dtchepak]

Yes, I agreed they'd probably be useful to others (I don't use async so I'm not in a good position to judge here :) ).

Maybe could add to ReturnsExtensions? Might be fine in standard SubstituteExtensions too, as it only adds methods to Task<T>.

Would also require ...ForAnyArgs variants, and conditional compilation so NET35 still works.

[iamkoch]

Would something like this work in SubstituteExtensions?

    public static ConfiguredCall Returns<T>(this System.Threading.Tasks.Task<T> value, T returnThis)
    {
        return Returns(MatchArgs.AsSpecifiedInCall, Task.FromResult(returnThis));
    }

Effectively target the task based methods and allow an unwrapped value to be passed into the returnThis parameter.

Having troubles getting this to run the tests on my machine so can't validate if this will work.

[iamkoch]

Just managed to get the tests running and it works.

Will look into productionising and creating a PR if you guys think it's a good approach?

[iamkoch]

Here's a diff by way of a proof of concept.

https://github.com/nsubstitute/NSubstitute/compare/master...iamkoch:master

Thoughts?

[dtchepak]

@iamkoch Happy to get a PR. It would be really helpful if the PR:

• covers all the overloads of Returns. (ReturnsForAnyArgs, with multiple results etc. ]Marius's comment](https://github.com/nsubstitute/NSubstitute/issues/189#issue-92341321) has some extra examples)
• builds with build-fake.bat targets=ALL to make sure it works for NET35, NET40, and NET45 (or build-fake.sh targets=ALL if running from Git Bash on Windows. We don't have a Mono build script unfortunately).

@mariusGundersen, @alexandrnikitin: do you have any opinions on having:

ConfiguredCall Returns(this Task<T> value, T returnsThis)
// vs..
ConfiguredCall ReturnsAsync(this Task<T> value, T returnsThis)

The former is low ceremony but deviates from the usual Returns(this T value, T returnsThis) pattern. Latter is more explicit at the cost of introducing a new concept (Returns, ReturnsForAnyArgs, then ReturnsAsync).

[iamkoch]

I think ReturnsAsync isn't strictly correct as it doesn't return an awaitable task.

Having used NSubstitute, Moq and other frameworks a lot, I think the nice obfuscation of the .Net async/await pattern has led to me tut, and then wrap a returned object in Task.FromResult enough times for .Returns() to be an OK solution that will seamlessly fit into people's dev flow.

[alexandrnikitin]

I agree with @iamkoch, ReturnsAsync lies as it doesn't return a task. Returns is ok for me. I would prefer to have it in SubstituteExtensions namespace because original Returns is extendable enough to deal with Tasks without any additional extension methods.

[iamkoch]

OK cool. I'll work on it and create a PR for review.

[mariusGundersen]

I was under the assumption that the compiler would be confused if you called the Returns method with a Task result, as in:

_fakeLookup.GetSomething.Returns(Task.FromResult(new Something()));

But after testing with the extension method named Returns it turns out to work perfectly fine, so yeah, using Returns will be backwards compatible and will just magically work for users. It might be slightly confusing that it works, but since I already use NSubstitute without the faintest idea of how it works I can live with that.

[iamkoch]

I thought that at first, but it honours the Returns(this T, T returnThis) over Returns(this Task ...) so it's not an issue.

[dtchepak]

Thanks for the comments everyone. Great points. Let me know if you need any assistance with a PR.

[iamkoch]

Just raised a PR. Let me know if anything needs changing or updating.

[mariusGundersen]

I'm going to close this method as it seems the pull request has been merged. Any timeframe on the 1.8.3 release @dtchepak?

[dtchepak]

Hi @mariusGundersen, next release is just waiting on me finding time to update the docs (#199).

[dtchepak]

@mariusGundersen ICYMI, this is in the NSubstitute 1.9.1 release. @iamkoch: Some feedback on this feature: https://twitter.com/roryprimrose/status/653070788367486976 https://twitter.com/roryprimrose/status/653081303638642689 :)

[iamkoch]

Thanks for pointing out those! I'll retweet them and shower in the glory.

[govorovvs]

Not working in net6.0

[dtchepak]

Hi @govorovvs , can you please share a failing test? I'll add it to the test suite and run against net6.0

[304NotModified]

What are the docs for this?