Async Method Support Clean-up

[rbdevr]

Right now, Moq doesn't have a proper support for async methods. There are a few methods giving Moq some Async support. Those are spreaded through out Moq fluentApi like ReturnAsync() and ThrowAsync() but it's not developer friendly.
I suggest to implement FluentApi support for async methods starting from SetupAsync() and expose only async functionality.
At the same time, remove all the async methods from the current Setup().
Let me know your thoughts. Should I do a request pull and start working on it?
Regards

Example:
interface IComponent {
   int Process(int value);
   Task<int> ProcessAsync(int value);
}
componentMock.Setup(I => Process(It.Any<int>()).Return(1); // normal method
componentMock.Setup(I => Process(It.Any<int>()).Throw(new Exception()); // normal method
componentMock.Setup(I => Process(It.Any<int>()).Callback(i => capt = i); // normal method
componentMock.SetupAsync(I => ProcessAsync(It.Any<int>()).Return(1); // full async support
componentMock.SetupAsync(I => ProcessAsync(It.Any<int>()).Throw(new Exception()); // full async support
componentMock.SetupAsync(I => ProcessAsync(It.Any<int>()).Callback(i => capt = i); // full async support

[stakx]

At first glance, and without considering any other (possibly more arcane) use cases involving Task / Task<T>, this looks like a great idea! 👍

You are welcome to send a PR.

Let me just mention three points that you're probably already aware of:

• The new, proposed API should be able to cover the same ground as the present "async" API. It may be more complete, but it shouldn't be less complete.

• The present API cannot be removed right away. Superseded methods / interfaces can however be marked as [Obsolete("Replaced by X / Do Y instead.")] and hidden using [EditorBrowsable(EditorBrowsableState.Never)] to guide users towards the new API.

• Please target your PR at the develop branch (and make sure to keep it rebased, if possible).

Thank you!

[stakx]

Some various related things to be tracked along with this issue (I might expand this list later on):

• Async callbacks do not respect async/await #256
• AddRangeAsync xunit .net core error #327 (comment)

[rbdevr]

I've got the basics in place but with two Setup() and SetupAsync() it means double the tests.
I've been thinking about how to handle unit tests.
Duplicate everything?
Use template to generate tests for sync or async?
Share Setup code with SetupAsync as much as possible, test only Setup and take it for granted?
Have only one AsyncSetupFixture with some async scenarions? <-- I've done this way for now.
Any Thoughts?

[rbdevr]

I've finished doing some necessary cleaning up. I've noticed that it is possible to use Setup() to do normal methods and async methods. .Net is capable of identifing one from the other based on the return argument. This makes development a lot more friendlier but there are two things to consider.

1. It's not explicit that the method registered in Setup is Async. This shouldn't matter since it will work both ways. Devs can even change the registered method from normal to async without having to update the tests. In the other hand since it's not explicit it could cause some misunderstanding.
2. There will be some compatibility issues. When updating to this new Moq version. All the current Async methods registered in Setup() will still be working with ReturnAsync and ThrowAsync and will break. unless I implement a compatibility layer.

Let me know your toughts.

So in that case tests will look like:
componentMock.Setup(I => Process(It.Any<int>()).Return(1); // normal method
componentMock.Setup(I => Process(It.Any<int>()).Throw(new Exception()); // normal method
componentMock.Setup(I => Process(It.Any<int>()).Callback(i => capt = i); // normal method
componentMock.Setup(I => ProcessAsync(It.Any<int>()).Return(1); // full async support
componentMock.Setup(I => ProcessAsync(It.Any<int>()).Throw(new Exception()); // full async support
componentMock.Setup(I => ProcessAsync(It.Any<int>()).Callback(i => capt = i); // full async support

[stakx]

Regarding your two points:

1. (I guess you meant "explicit", not implicit?) Not sure how much confusion additional method overloads would cause here. I tend to think having a slimmer API someday (one method to set up instead of 5, one method for callbacks, one for throws, one for returns) would be a net gain. But I'm not certain at this point.

2. Why would ReturnAsync, ThrowAsync et al. break? That shouldn't happen.

@kzu, what's your opinion on (1) above? Would you prefer full integration of all async setup methods into the regular Setup, Callback, Throws, Returns (so there would no longer be a distinction between the sync and async APIs), or would you rather have async setups start with a dedicated SetupAsync "entry point" method?

[rbdevr]

1. Thanks for pointing it out. I've amended it. Between going implicit or explicit, One tough that might happen is: Is there a possibility that a dev might want to mock the Task class in a method? That will not be possible with the implicit approuch.
   Example: Task CreateRunningTask() <returns a mocked task with running state>.
2. The async Setup() works with ISetupAsync. ReturnAsync() and ThrowAsync() are extensions from ISetup. I will have to add them to keep it compatible or force the devs to update their code but I sure this is not going to be your first choice.

Previously I've talked about the unittests. What approuch would you prefer?

[stakx]

Regarding the unit tests, I first need to take a look at what unit tests we are talking about here. I'll get back to you on that!

[rbdevr]

I've pushed the current code with the original requirements (explicit) to:
https://github.com/robydev/moq4/tree/%23384

[rbdevr]

1. If there is a need for returning a Task we could have a specific Returns() available only for async Setup(). I think implicit is a good choice.
   Let me know about the unittests.

[rbdevr]

I've found an issue with the ThrowAsync extensions. They are returning IReturnResults<Type> fluentapi interface. It's my understanding they should return IThrowsResult.
I've chanded this but please confirm if this is the correct approach.

[rbdevr]

I've been struggling to implement the async FluentApi due to the current design. At the same time I've noticed some issues with FluentApi. So I've decided to refactor the methodcall classes and fluentapi functionality. Implementng separation of concerns and giving FluentApi full freedom for future implementations.
I though about having this work done as a separate issue but it is very difficult to implement async fluentapi without it..
It's more work but it will solve restrains caused by current design. It's looking good so far.
Is there a document listing all valid FluentApi selections? That would help.

[rbdevr]

I've noticed IOccurrence was decomission and Verify() was put in place. Doesn't that break the idea of a clear Setup() functionality with FluentApi support previously discussed?

[stakx]

Hi @Robydev, sorry for not getting back to you sooner. I'm going to set aside some time later this week to look at the work you've done so far, and try to answer your questions. Thanks for being patient!

[stakx]

Maybe I can answer the occasional question right now:

I've noticed IOccurrence was decomission and Verify() was put in place. Doesn't that break the idea of a clear Setup() functionality with FluentApi support previously discussed?

Not having the ability to specify a Times constraint during setup doesn't necessarily make setup less "clear" (but perhaps I am misunderstanding what exactly you mean by this). See #373 (comment) for a quick explanation why IOccurrence was obsoleted.

I've been struggling to implement the async FluentApi due to the current design. At the same time I've noticed some issues with FluentApi. So I've decided to refactor the methodcall classes and fluentapi functionality. Implementng separation of concerns and giving FluentApi full freedom for future implementations.
I though about having this work done as a separate issue but it is very difficult to implement async fluentapi without it..
It's more work but it will solve restrains caused by current design.

I haven't yet looked into the work you've done on this, but generally speaking, you could isolate that refactoring out into a separate PR that precedes the work you're doing on async. If you think keeping those two works separate from one another would help understanding and traceability later on, that would probably be the way to go.

[mnivet]

Some news on that work ?
I've just developed some extensions methods that do exactly what is exposed in the first post here and I was starting to think how to give them to the community (PR on moq or distributed though a NuGet package that would just provide the extension methods)

Then I found that post and see that there is no activity here since 11 months...
Is there still something in development here ? Should I take a look at the work started by @rbdevr ?

[rbdevr]

I have checked in the code a long time ago and I didn't hear any feedback.
I have continue working on refactoring fluentapi implementstion but that caused the library to face back compatibility issues.

[mnivet]

Ok without more reactivity from moq team, let chose to publish an extension package:
https://www.nuget.org/packages/Talentsoft.Moq.SetupAsync/1.0.0

Sources and exemples are here:
https://github.com/TalentSoft/Moq.SetupAsync

[stakx]

Hi @rbdevr. Your issue is one of the few that sort of fell by the wayside during the issue burnination sprint I did between June 2017 and February 2018. Your issue was/is one with a comparatively large scope, and judging from your own responses (above), it's one that isn't straightforward and demands some potentially tricky decisions. I was simply too busy with other issues back then to allocate more of my own resources for this, so I posted the brief response above, hoping that might get you a little further. I'd like to apologize to you that I didn't communicate better at the time.

Do feel free to keep working on this. One word of caution, though: At present, it might pay off more to direct any improvements at the upcoming version 5 of Moq (https://github.com/moq/moq). Right now, for the time being, Moq 4's API is frozen so Moq 5 can be finalized.

I can do the occasional review for you guys and give feedback / suggestions if you'd like, however until perhaps the second half of June I won't have a lot of free capacity as I'm currently busy working through the whole stack underneath Moq (Mono/.NET/.NET Core and Castle DynamicProxy) trying to ensure Moq 4 will be able to deal with the new C# 7 language features.

[mnivet]

OK so I let you work on Moq 5 to integrate a clean SetupAsync with a reworked API.
And for Moq 4 we have the package that already provide something nice to use.

And if final implementation in Moq 5 is not to far from the one in the package, we can hope that migration would be transparent or at least scriptable. I've already write some Roslyn code fix to perform such type of migration. So I'll will keep an eye on your progression to provide such code fix if necessary.

[stakx]

It has been rather quiet in here for a while. Last time some work was done on this, things turned out to be a little more difficult than expected because of overload conflicts. So for the protocol, if anyone picks this up in the future, here's something to think about:

It might be a good idea to implement a cleaned-up async API as extension methods in a separate namespace, say Moq.Async; that way you'd only get the async methods when explicitly importing that namespace.

Another option would be to do something similar as .Protected(): you'd have to write mock.Async()... to access the async API.

[mnivet]

Another option would be to do something similar as .Protected(): you'd have to write mock.Async()... to access the async API.

And introducing a new SetupAsync method like I've done in Moq.SetupAsync ?
Why writing mock.Async()... would be preferred to writing mock.SetupAsync()... ? Is there more than the Setup method that need to be specialized for async ?

SetupGet and SetupSet are not applicable to async (properties could not be async), and verify methods do not need to be specialized, since the fact that the method as been called is not linked to the way that the call has been performed.
So maybe it let us SetupSequence method in addition to the Setupmethod to cover.

Is that justified to have an async() scope method to expose just two methods (Setup and SetupSequence) dedicated to async instead of having clear overload from the root (SetupAsync and maybe a SetupSequenceAsync) ?

Furthermore I think that having a scope is less discover-able, than adding a new specialized SetupAsync method, since people are use to type Setup, and that intellisence will then show them the SetupAsync proposition which may guide them to what they need sooner than trying with the classical Setup, and failing, before looking for that async scope...

For Protected()the scope is justified because there is a lot more of methods to expose, and because it's not a basic use case. But working with async methods is a more and more common case, so discover-ability should be take into account in the design of the API in that case.

[stakx]

Why writing mock.Async()... would be preferred to writing mock.SetupAsync()...?

For one thing, because I'd like to avoid new method names. True, in this case it would probably be only SetupAsync and SetupSequenceAsync, but we'd still be setting up a precedent for future extensions.

Ideally (IMHO), the new methods would be called simply Setup (and SetupSequence etc.).

Which brings me to the reason I mentioned above: if we went with just Setup, we'd need to think about minimizing method resolution conflicts. Moq's fluent API has a lot of method overloads, and there are already situations where things get ambiguous. The more we overload methods, the worse the situation will get, and I'd like to prevent us from painting ourselves into a corner as early on as possible.

That's how and why I ended up thinking about .Async().Setup, it fulfills all of the above:

• doesn't introduce new Setup* method names
• doesn't increase the risk of method resolution conflicts of existing methods

We could of course do SetupAsync, SetupSequenceAsync etc. and "hide" them as extension methods in a separate namespace, I simply thought it reasonable to follow the precedent / existing pattern set up by .Protected().

I do agree, however, that discoverability is an important consideration, and I'm not sure .Async() is ideal in that respect.

[stakx]

I'm going to close this issue in favor of #1007. By adding an await-like operator to setup expressions & letting the internal machinery transparently handle the necessary return value transformations, there should no longer be any strict need at all for any SetupAsync / ReturnsAsync / ThrowsAsync etc. methods... and thus no need to clean them up.