Repository navigation
Allow DefaultValue.Mock to mock Task<TMockable> and ValueTask<TMockable> - #502
Conversation
There was a problem hiding this comment.
Could still use some work to make code easier to understand (esp. MockDefaultValueProvider and MockWithOptionalWrapper.Wrapper).
Also, this PR probably breaks some of MockDefaultValueProvider's current behavior. Ensure this is what it does (as mentioned in #171 (comment)):
- Let the empty default value provider produce a completed Task containing default(T).
- If that contained default value is not null, return the produced Task.
- Otherwise, check if T is mockable. If so, produce another completed Task containing a new Mock() and return it.
- Otherwise (if T is not mockable), return the empty completed Task produced in (1).
| { | ||
| // Task<T> inherits from Task, so just return Task<bool> | ||
| return GetCompletedTaskForType(typeof(bool)); | ||
| return Task.FromResult(false); |
There was a problem hiding this comment.
(This optimization is not directly related to this PR.)
| return Task.FromResult(false); | ||
| } | ||
| else if (valueType.GetTypeInfo().IsGenericType && valueType.GetGenericTypeDefinition() == typeof(IEnumerable<>)) | ||
| else if (valueType.GetTypeInfo().IsGenericType) |
There was a problem hiding this comment.
(This optimization is not directly related to this PR, but since one commit targets code de-duplication in MockDefaultValueProvider, it seems only logical to make the same effort in EmptyDefaultValueProvider, too.)
| } | ||
|
|
||
| object value = null; | ||
| if (taskResultType == null) |
There was a problem hiding this comment.
This is still pretty ugly. What these lines of code basically amount to is, "If I have some special handling capabilities for some special type (Task<T>), don't let the fallback value provider generate a value for it; I'll deal with it myself right away." Find a way to express this more clearly in code.
| } | ||
|
|
||
| return mock != null ? mock.Object : value; | ||
| return mock.Mock != null ? mock.Wrapper ?? mock.Mock.Object : value; |
There was a problem hiding this comment.
Doesn't read very clearly. Probably the same as mock.Wrapper ?? mock.Mock?.Object ?? value, but it would be better to rewrite the whole method so that no convoluted expression is required at its end.
|
|
||
| namespace Moq | ||
| { | ||
| internal struct MockWithOptionalWrapper |
There was a problem hiding this comment.
This type is useful because it helps us avoid wrapping and unwrapping values in a completed Task<T>, but its semantics are still somewhat confusing, esp. the fact that the Wrapper does not wrap Mock, but Mock.Object. Perhaps rename that property.
This adds some unit tests that demonstrates missing support of the MockDefaultValueProvider for both `Task<>` and `ValueTask<>`.
It isn't enough for `MockDefaultValueProvider` to correctly wrap a mocked object in `Task<T>` or `ValueTask<T>`. Produced mocks also have to be added to the owning / outer mock's `InnerMocks` dictionary in order to be included in `mock.Verify[All]`. The tests added in this commit show that this doesn't work yet.
The main difficulty here lies in how to avoid repeated wrapping and unwrapping of task objects, because the mocked objects are required both wrapped in tasks (for method return values) and unwrapped as the plain mock object (for `InnerMocks` verification). (This is why the helper type `MockWithWrappedMockObject` gets introduced.)
328faf7 to
0c4adf2
Compare
DefaultValue.Mock to mock Task<TMockable>DefaultValue.Mock to mock Task<TMockable> and ValueTask<TMockable>
0c4adf2 to
bc11f5c
Compare
| } | ||
|
|
||
| var result = (useAlternateProvider ?? this.DefaultValueProvider).GetDefaultReturnValue(method, this); | ||
| var unwrappedResult = TryUnwrapResultFromCompletedTaskRecursively(result); |
There was a problem hiding this comment.
For the standard default value providers (Empty and Mock), it would be great if the wrapping (inside the provider) and unwrapping (here) could be avoided. It should be possible to add a fast path as follows:
- Check whether
this.DefaultValue != DefaultValue.Custom. If false, stop. - Check whether
methodreturns a task. If so, ask the provider not for the actual task return type viaGetDefaultReturnValue, but for the type of the wrapped value viaGetDefaultValue. - Perform the wrapping into completed tasks here, given the value returned from the provider.
This would save us one unnecessary recursive task unwrapping operation.
bc11f5c to
a3e05bd
Compare
This PR enables the following scenario (shown for
Task<T>, but same goes forValueTask<T>):This closes #171.