Add ability to set up the .Result of (value) tasks
#1126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stakx
commented
Jan 1, 2021
Comment on lines
+300
to
+304
| Split(memberAccessExpression.Expression, out r, out p); | ||
| p.AddResultExpression( | ||
| awaitable => Expression.MakeMemberAccess(awaitable, memberAccessExpression.Member), | ||
| awaitableFactory); |
There was a problem hiding this comment.
Once we add support for custom awaitable types, it will be better to preserve the original expression, instead of replacing the .Result access with a new result expression.
Comment on lines
+96
to
+99
| if (this.result is ExceptionResult r) | ||
| { | ||
| this.result = awaitableFactory.CreateFaulted(r.Exception); | ||
| } |
There was a problem hiding this comment.
There may be circumstances where we may have to call the other CreateFaulted(IEnumerable<Exception> exceptions) overload instead (which, I'm assuming, is meant to cover AggregateException). I haven't yet looked into whether this is actually necessary. Perhaps it would be better to remove that other overload until that time?
Comment on lines
+74
to
+77
| catch (Exception exception) | ||
| { | ||
| invocation.Exception = exception; | ||
| } |
There was a problem hiding this comment.
We may have to change this some day in order to account for several / aggregate exceptions.
stakx
force-pushed
the
setup-task-result
branch
from
97a506f
to
6f6a89d
Compare
mburumaxwell
pushed a commit
to faluapp/falu-dotnet
that referenced
this pull request
Jun 12, 2021
Bumps [Moq](https://github.com/moq/moq4) from 4.15.2 to 4.16.0. #Changelog *Sourced from [Moq's changelog](https://github.com/moq/moq4/blob/main/CHANGELOG.md).* > ## 4.16.0 (2021-01-16) > > #### Added > > * Ability to directly set up the `.Result` of tasks and value tasks, which makes setup expressions more uniform by rendering dedicated async verbs like `.ReturnsAsync`, `.ThrowsAsync`, etc. unnecessary: > > ```diff > -mock.Setup(x => x.GetFooAsync()).ReturnsAsync(foo) > +mock.Setup(x => x.GetFooAsync().Result).Returns(foo) > ``` > > This is useful in places where there currently aren't any such async verbs at all: > > ```diff > -Mock.Of<X>(x => x.GetFooAsync() == Task.FromResult(foo)) > +Mock.Of<X>(x => x.GetFooAsync().Result == foo) > ``` > > This also allows recursive setups / method chaining across async calls inside a single setup expression: > > ```diff > -mock.Setup(x => x.GetFooAsync()).ReturnsAsync(Mock.Of<IFoo>(f => f.Bar == bar)) > +mock.Setup(x => x.GetFooAsync().Result.Bar).Returns(bar) > ``` > > or, with only `Mock.Of`: > > ```diff > -Mock.Of<X>(x => x.GetFooAsync() == Task.FromResult(Mock.Of<IFoo>(f => f.Bar == bar))) > +Mock.Of<X>(x => x.GetFooAsync().Result.Bar == bar) > ``` > > This should work in all principal setup methods (`Mock.Of`, `mock.Setup…`, `mock.Verify…`). Support in `mock.Protected()` and for custom awaitable types may be added in the future. (@stakx, [#1126](devlooped/moq#1126)) > > #### Changed > > * Attempts to mark conditionals setup as verifiable are once again allowed; it turns out that forbidding it (as was done in [#997](devlooped/moq#997) for version 4.14.0) is in fact a regression. (@stakx, [#1121](devlooped/moq#1121)) > > #### Fixed > > * Performance regression: Adding setups to a mock becomes slower with each setup (@CeesKaas, [#1110](devlooped/moq#1110)) > > * Regression: `mock.Verify[All]` no longer marks invocations as verified if they were matched by conditional setups. (@Lyra2108, [#1114](devlooped/moq#1114)) #Commits - [`74d5863`](devlooped/moq@74d5863) Update version to 4.16.0 - [`424fe31`](devlooped/moq@424fe31) Fix typo in changelog - [`f48c0f4`](devlooped/moq@f48c0f4) Merge pull request [#1126](devlooped/moq#1126) from stakx/setup-task-result - [`6f6a89d`](devlooped/moq@6f6a89d) Update the changelog - [`66bcb21`](devlooped/moq@66bcb21) Enable `task.Result` in delegate-based setup methods - [`42521c4`](devlooped/moq@42521c4) Add ability in `IAwaitableFactory` to create result...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This much more focused version of #1123 only covers setting up the
.Resultof (value) task-based async methods.Benefits
The dedicated async setup methods
.ReturnsAsync,.ThrowsAsyncetc. are now largely superfluous:From a user perspective, setups become more uniform in shape. From a maintainer's perspective, we have less work. More specifically, we no longer need to ensure that those async setup verbs are at feature parity with their non-async counterparts.
It adds async support where such async verbs aren't currently available:
but also in other places like in
Verify…(callExpression, …).It enables recursive setups across async calls in a single setup expression. For example, using just
Mock.Of:Should
.ReturnsAsync,.ThrowsAsyncetc. be marked as[Obsolete]?I am not marking those methods as
[Obsolete]just yet, mainly for two reasons:There are a few specialized overloads (e.g. for delayed task completion) that would need to be rewritable using just the regular setup verbs. I'd like to look at those separately.
.PassAsyncfor sequence setups doesn't have an easy replacement, because non-generic tasks do not have a.Resultproperty that one could set up instead. Replacing.PassAsyncwould require anawait-like operator substitute (as proposed in Easier async setups through a newAwait(...)operator #1007 and implemented inAwaitanything & custom awaitable types #1123), which would likely be added together with support for custom awaitable types. Until that happens, it doesn't make sense to deprecate only some (but not all) async setup verbs.What is still missing?
As noted in the new changelog entry, support in
mock.Protected()is still missing, or at least spotty. Also, unlike #1123, there is no support for custom awaitable types just yet; however, most of the necessary machinery is now in place, so this should be relatively easy to add in the future.Resolves #384, closes #1007, closes #1123.