Introduce `.Protected().As<TDuck>()` to perform duck-typed setup and verification of protected members by stakx · Pull Request #495 · devlooped/moq

stakx · 2017-10-22T09:55:22Z

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 and Verify methods accessible via mock.Protected() admittedly offer a handy way to get at protected members, but with the following downsides:

They are by necessity completely "untyped". As such, they do not fit well into Moq's generally strongly-typed API.
They are not easy to extend with support for generic methods, or methods with by-ref parameters.

Proposed solution:

The new feature proposed here adds a new method, mock.Protected().As<TDuck>(), which offers Setup and Verify methods by which protected members can be accessed in a strongly-typed manner through a totally unrelated type TDuck. This type's members are expected to correspond to the mocked type's members (by having identical signatures).

abstract class Foo
{
    protected abstract void Protected(); 
}

interface Fooish
{
    void Protected();
}

var mock = new Mock<Foo>();
mock.Protected().As<Fooish>()
    .Setup(m => m.Protected())....; // `Foo.Protected` gets set up instead of `Fooish.Protected`

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 implement Fooish and then mock Fooish 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.

stakx

Apart from missing setup and verification methods, and missing unit tests, there are some smallish details that need to be addressed.

stakx · 2017-10-22T10:13:20Z

+		/// 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;


Is it wise to overload the As verb with yet another meaning, or should this be named differently (perhaps Like<TDuck>)?

stakx · 2017-10-22T10:14:22Z

+		}
+
+		public ISetup<T> Setup(Expression<Action<TDuck>> expression)
+		{


What kind of argument validation needs to be performed here?

Done: expression must not be null, and it must not be about a member that doesn't exist in T.

stakx · 2017-10-22T10:14:34Z

+		}
+
+		public ISetup<T, TResult> Setup<TResult>(Expression<Func<TDuck, TResult>> expression)
+		{


What kind of argument validation needs to be performed here?

stakx · 2017-10-22T10:15:21Z

+
+		private static LambdaExpression ReplaceDuck(LambdaExpression expression)
+		{
+			var targetParameter = Expression.Parameter(typeof(T), expression.Parameters[0].Name);


Add a Debug.Assert verifying that the passed-in LambdaExpression has one parameter.

stakx · 2017-10-22T10:17:15Z

+		/// <summary>
+		/// <see cref="ExpressionVisitor"/> used to replace occurrences of `TDuck.Member` sub-expressions with `T.Member`.
+		/// </summary>
+		private sealed class DuckReplacer : ExpressionVisitor


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.

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.

stakx · 2017-10-22T10:18:06Z

+			{
+				if (node.Expression is ParameterExpression left && left.Type == this.duckType)
+				{
+					var targetParameter = Expression.Parameter(typeof(T), left.Name);


Replace typeof(T) with this.targetType.

stakx · 2017-10-22T10:19:44Z

+				    .GetMethods(BindingFlags.NonPublic | BindingFlags.Instance)
+				    .Where(ctm => IsCorrespondingMethod(duckMethod, ctm));
+
+				var targetMethod = candidateTargetMethods.Single();


Should there be a check whether exactly one target method was identified (and if not, throw an exception with a meaningful Message)?

stakx · 2017-10-22T10:20:12Z

+					.GetProperties(BindingFlags.NonPublic | BindingFlags.Instance)
+					.Where(ctp => IsCorrespondingProperty(duckProperty, ctp));
+
+				return candidateTargetProperties.Single();


Should there be a check whether exactly one target property was identified (and if not, throw an exception with a meaningful Message)?

stakx · 2017-10-22T10:23:49Z

+				return candidateTargetProperties.Single();
+			}
+
+			private static bool IsCorrespondingMethod(MethodInfo duckMethod, MethodInfo candidateTargetMethod)


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.

stakx · 2017-10-22T10:25:43Z

+		/// <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);
+	}


Missing: SetupGet, SetupSet, SetupSequence, and all Verify methods.

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.

stakx added the draft label Oct 22, 2017

stakx commented Oct 22, 2017

View reviewed changes

stakx force-pushed the protected-as branch 2 times, most recently from 4dbb452 to 7dd3cb4 Compare October 22, 2017 18:25

stakx added 2 commits October 22, 2017 21:07

Prototype new method group .Protected().As<TDuck>().Setup

66dc1fd

Improve validation & add unit tests for Setup

bf97a95

stakx force-pushed the protected-as branch from 7dd3cb4 to bf97a95 Compare October 22, 2017 19:07

stakx added 3 commits October 23, 2017 20:23

Add Verify methods (except VerifySet)

f9e98fe

Update the changelog

4ac0c2e

stakx removed the draft label Oct 23, 2017

stakx merged commit 9db16a0 into devlooped:develop Oct 23, 2017

stakx deleted the protected-as branch October 23, 2017 19:08

devlooped locked and limited conversation to collaborators Sep 7, 2024

Uh oh!

Conversation

stakx commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Proposed solution:

When does this feature offer true added value?

Uh oh!

stakx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

stakx commented Oct 22, 2017 •

edited

Loading