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

Add new overload for throwsasync to allow Func<Exception> #1190

Closed
wants to merge 2 commits into from
Closed

Add new overload for throwsasync to allow Func<Exception> #1190

wants to merge 2 commits into from

Conversation

adam-knights
Copy link
Contributor

@adam-knights adam-knights commented Jul 25, 2021

Related to #1048. This gets us a step towards the enhancement, in this first instance we now allow ThrowsAsync(() => new Exception()). And we refactor to use this in the existing methods.

In a future PR we could add support for input arguments to fully cover #1048 - I thought I'd make this change first to make it smaller, before having to also edit ReturnsExtensions.tt etc.


Note: Git isn't doing the best job of the diff here, probably better viewed side by side. Details...

Three new functions were added underneath the existing ThrowsAync functions.
The original functions were changed to call the new functions using () => exception, in the same way ReturnsAsync does on line 28.


I'm happy to add this to moq5 too, either after or before this is merged... didn't want to setup two PRs until this one had some feedback :).

@stakx
Copy link
Contributor

stakx commented Jul 25, 2021

Hi @adam-knights, and first of all, thank you for participating in Moq development!

I'll need some more time to decide whether to proceed with this... the main reason being that this new method overload is arguably the least needed one:

  • Moq has recently gained the ability to set up task .Results (as mentioned in the changelog), which essentially renders the ReturnsAsync / ThrowsAsync APIs redundant; you can now use the main methods Returns / Throws directly. So ideally, development should go there, as it will benefit both async and synchronous use cases.

  • While Throws[Async](Func<Exception>) would definitely be a start, it provides less additional value over Throws[Async](Exception) (deferred instantiation) than the various Throws[Async](Func<..., Exception>) (making arguments accessible) overloads would. Ideally, we'd include all those, too.

If you're willing to work on setup.Throws(Func<[...], Exception>) overloads, I think that would be the most valuable addition, and I'd happily review such a PR. But I think ThrowsAsync should wait.

@adam-knights
Copy link
Contributor Author

Thanks for such a quick reply. What you've said makes sense, I must admit I did see the new .Result change but in my head I discounted it because over the years I've learnt to never use .Result and .Wait, but clearly here as we are not calling real code, we know there won't be a deadlock. Maybe that's worth strengthening in the Quickstart (or maybe I'm just overly cautious ;) ).

I think I will go ahead and try a new PR for what you've said. There seems to be a few choices as to where to put the implementation - I think it would be NonVoidSetupPhase ? But worth me asking the question before trying again!

I also took a look at moq 5 and your chat in https://github.com/moq/moq/issues/53, if I've looked correctly then moq5 does have ReturnsAsync but does not have ThrowsAsync, so you would have a +1 from me on marking ThrowsAsync as obsolete, with ReturnsAsync tbd.

@stakx
Copy link
Contributor

stakx commented Jul 25, 2021

@adam-knights:

There seems to be a few choices as to where to put the implementation [...]

I'd start in src/Moq/Language/IThrows.cs (or rather in a IThrows.tt file which generates IThrows.Generated.cs, to stay in line with how things are done for the other verbs). The compiler should then give you errors where the implementation is missing – I would suspect in SetupPhrase (the base class for VoidSetupPhrase and NonVoidSetupPhrase) since the Throws verb is available for both void and non-void methods.

you would have a +1 from me on marking ThrowsAsync as obsolete, with ReturnsAsync tbd.

I think I won't mark any of them as obsolete as long as Moq 5 has even one of them. (The way I currently see it, they're a set of methods that really belong together: if Returns is complemented by ReturnsAsync, why shouldn't Throws follow the same pattern?) Given what you said above, I'd argue that Moq 5 should either remove ReturnsAsync, or add ThrowsAsync, for consistency's sake.

@adam-knights
Copy link
Contributor Author

adam-knights commented Jul 26, 2021

Closing in favor of #1191.

@adam-knights adam-knights deleted the throwsasync-exception-func branch July 26, 2021 17:25
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

Successfully merging this pull request may close these issues.

None yet

2 participants