-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
Introduce .Protected().As<TDuck>()
to perform duck-typed setup and verification of protected members
#495
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from missing setup and verification methods, and missing unit tests, there are some smallish details that need to be addressed.
/// Any type with members whose signatures are identical to the mock's protected members (except for their accessibility level). | ||
/// </typeparam> | ||
IProtectedAsMock<TMock, TDuck> As<TDuck>() | ||
where TDuck : class; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it wise to overload the As
verb with yet another meaning, or should this be named differently (perhaps Like<TDuck>
)?
} | ||
|
||
public ISetup<T> Setup(Expression<Action<TDuck>> expression) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of argument validation needs to be performed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: expression
must not be null, and it must not be about a member that doesn't exist in T
.
Source/Protected/ProtectedAsMock.cs
Outdated
} | ||
|
||
public ISetup<T, TResult> Setup<TResult>(Expression<Func<TDuck, TResult>> expression) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of argument validation needs to be performed here?
Source/Protected/ProtectedAsMock.cs
Outdated
|
||
private static LambdaExpression ReplaceDuck(LambdaExpression expression) | ||
{ | ||
var targetParameter = Expression.Parameter(typeof(T), expression.Parameters[0].Name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a Debug.Assert
verifying that the passed-in LambdaExpression
has one parameter.
/// <summary> | ||
/// <see cref="ExpressionVisitor"/> used to replace occurrences of `TDuck.Member` sub-expressions with `T.Member`. | ||
/// </summary> | ||
private sealed class DuckReplacer : ExpressionVisitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does nesting this non-generic type inside a generic type mean that the JIT / runtime will potentially generate it more than once? It would perhaps be safer to un-nest it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the answers to the Stack Overflow question, "Should I avoid nested types in generic types?", I'd say its fairly safe to keep this class nested, since the outer class' type parameters are all constrained to be reference types.
Source/Protected/ProtectedAsMock.cs
Outdated
{ | ||
if (node.Expression is ParameterExpression left && left.Type == this.duckType) | ||
{ | ||
var targetParameter = Expression.Parameter(typeof(T), left.Name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace typeof(T)
with this.targetType
.
Source/Protected/ProtectedAsMock.cs
Outdated
.GetMethods(BindingFlags.NonPublic | BindingFlags.Instance) | ||
.Where(ctm => IsCorrespondingMethod(duckMethod, ctm)); | ||
|
||
var targetMethod = candidateTargetMethods.Single(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a check whether exactly one target method was identified (and if not, throw an exception with a meaningful Message
)?
Source/Protected/ProtectedAsMock.cs
Outdated
.GetProperties(BindingFlags.NonPublic | BindingFlags.Instance) | ||
.Where(ctp => IsCorrespondingProperty(duckProperty, ctp)); | ||
|
||
return candidateTargetProperties.Single(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a check whether exactly one target property was identified (and if not, throw an exception with a meaningful Message
)?
Source/Protected/ProtectedAsMock.cs
Outdated
return candidateTargetProperties.Single(); | ||
} | ||
|
||
private static bool IsCorrespondingMethod(MethodInfo duckMethod, MethodInfo candidateTargetMethod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if we are comparing method signatures for a precise match anywhere else in Moq's code base, and either reuse existing code, or make this implementation here reusable. Also check if the framework already offers something to compare method signatures—no need to reinvent the wheel.
Source/Protected/IProtectedAsMock.cs
Outdated
/// <param name="expression">Lambda expression that specifies the expected method invocation.</param> | ||
/// <seealso cref="Mock{T}.Setup{TResult}(Expression{Func{T, TResult}})"/> | ||
ISetup<T, TResult> Setup<TResult>(Expression<Func<TDuck, TResult>> expression); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing: SetupGet
, SetupSet
, SetupSequence
, and all Verify
methods.
4dbb452
to
7dd3cb4
Compare
Since `SetupSet`'s parameter cannot be an expression (they may not contain any assignments), there's nothing to rewrite like with the other methods; if we stayed true to how Moq usually handles this kind of expression tree limitation, we'd declare an `Action<TDuck>` param- eter and then execute it inside a `FluentMockContext`. This doesn't work either, since we have no instance of `TDuck` to hand to the delegate.
This proposes a new feature that aims to offer more complete support for setting up and verifying protected members (including the ability to set up protected generic methods, and protected methods with by-ref parameters).
It closes #223 and #249 if merged.
Problem:
The current
Setup
andVerify
methods accessible viamock.Protected()
admittedly offer a handy way to get at protected members, but with the following downsides:Proposed solution:
The new feature proposed here adds a new method,
mock.Protected().As<TDuck>()
, which offersSetup
andVerify
methods by which protected members can be accessed in a strongly-typed manner through a totally unrelated typeTDuck
. This type's members are expected to correspond to the mocked type's members (by having identical signatures).When does this feature offer true added value?
This feature might seem pointless since it forces the user to declare a type just for testing; one could argue that it would be just as easy, and more meaningful, to simply let
Foo
implementFooish
and then mockFooish
directly. This is a fair assessment, but there might be cases where the type to be mocked is not owned by the user and cannot be modified. In these cases, the new feature offers a way to get at all protected members of that type.