Added `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results #594

snrnats · 2018-02-26T09:56:58Z

Added new method overloads:

public interface ISetupSequentialResult<TResult>
{
    ...
    ISetupSequentialResult<TResult> Returns(Delegate valueFunction);

    ISetupSequentialResult<TResult> Returns(Func<TResult> valueExpression);
    ...
}

They are needed to resolve #592

stakx

Thanks for the PR. I think there's one main issue, and that's the superfluous Returns overload taking a Delegate. See review comments for details.

stakx · 2018-02-26T11:11:12Z

Source/Language/Flow/SetupSequencePhrase.cs

@@ -93,6 +95,38 @@ public ISetupSequentialResult<TResult> Returns(TResult value)
 			this.setup.AddReturns(value);
 			return this;
 		}
+		public ISetupSequentialResult<TResult> Returns(Delegate valueFunction)


Please insert a blank line before this one.

stakx · 2018-02-26T11:11:18Z

Source/Language/Flow/SetupSequencePhrase.cs

@@ -40,6 +40,8 @@

 using System;
 using System.ComponentModel;
+using System.Reflection;
+using Moq.Properties;


Please insert a blank line before this one (such that namespace imports are grouped by main namespace).

stakx · 2018-02-26T11:17:45Z

Source/Language/Flow/SetupSequencePhrase.cs

@@ -93,6 +95,38 @@ public ISetupSequentialResult<TResult> Returns(TResult value)
 			this.setup.AddReturns(value);
 			return this;
 		}
+		public ISetupSequentialResult<TResult> Returns(Delegate valueFunction)


The parameter type Delegate doesn't seem right. It wouldn't make sense to pass any kind of delegate to Returns; how would Moq know what arguments to pass, and in case anything other than a TResult comes back, how to return it? See your implementation in SequenceMethodCall: you're always calling DynamicInvoke without any parameters.

I'd say get rid of this particular new overload.

stakx · 2018-02-26T11:31:38Z

CHANGELOG.md

+#### Added
+
+* Add `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results (@snrnats, #594)
+
 ## 4.8.2 (2018-02-23)


(Detail, but if it's not too much hassle, please insert a second blank line before this one.)

stakx

Sorry, I overlooked a couple of things.

stakx · 2018-02-26T12:32:10Z

Source/Language/Flow/SetupSequencePhrase.cs

+		}
+
+		public ISetupSequentialResult<TResult> Returns(Func<TResult> valueExpression)
+		{


Perhaps add a Guard.NotNull (argument validation) here.

stakx · 2018-02-26T12:34:06Z

Source/Language/Flow/SetupSequencePhrase.cs

+		}
+
+		public ISetupSequentialResult<TResult> Returns(Func<TResult> valueExpression)
+		{


Like I mentioned in the issue, I'm not sure if it's 100 % safe to add a new overload to that method. What if there's user code out there that passes a Func to the previous only method with the intent that the Func itself is the result? If we now add a method overload that takes a Func, the compiler might suddenly pick that new overload instead of the original function, causing the sequence setup to produce a different value (the result of the function instead of the function itself). Perhaps try to write a test that provokes this situation.

stakx · 2018-02-26T12:34:33Z

Source/Language/ISetupSequentialResult.cs

+		/// Uses delegate to get return value
+		/// </summary>
+		/// <returns></returns>
+		ISetupSequentialResult<TResult> Returns(Delegate valueFunction);


As above, I don't think this overload is necessary. I'd say remove it.

stakx

Thank you for making the requested changes! 👍 Regarding the special case check for Func<>, I think we're still not quite there yet. See review comments for details.

stakx · 2018-02-26T15:22:23Z

Source/Language/Flow/SetupSequencePhrase.cs

+		{
+			Guard.NotNull(valueExpression, nameof(valueExpression));
+
+			if (valueExpression.GetMethodInfo().ReturnType == typeof(void))


This if is redundant, the .NET compilers already ensure that typeof(TResult) != typeof(void). (The signature of a void method is not compatible with any of the Func<> delegate types.) Please remove.

stakx · 2018-02-26T15:28:13Z

Source/Language/ISetupSequentialResult.cs

+		/// Uses delegate to get return value
+		/// </summary>
+		/// <returns></returns>
+		ISetupSequentialResult<TResult> Returns(Func<TResult> valueExpression);


A detail that I'd missed before (sorry!), but could you please rename the parameter to valueFunction to be consistent with these:

https://github.com/moq/moq4/blob/b7b662774f27c46f32e715a75ddf180186cf8502/Source/Language/IReturns.cs#L103

https://github.com/moq/moq4/blob/b7b662774f27c46f32e715a75ddf180186cf8502/Source/Language/IReturnsGetter.cs#L84

(It'd be great to achieve as much consistency as possible now, because once this becomes part of the public API, we won't be able to easily rename it without the risk of breaking user code.)

Additionally, if you could include an XML documentation comment for valueFunction that'd be great. Example:

https://github.com/moq/moq4/blob/b7b662774f27c46f32e715a75ddf180186cf8502/Source/Language/IReturnsGetter.cs#L72

stakx · 2018-02-26T15:41:13Z

Source/Language/Flow/SetupSequencePhrase.cs

+			// as a return value. We don't want to invoke the passed Func to get a return value; the 
+			// passed func already is the return value. To prevent it we need to wrap up this func because 
+			// `SequenceMethodCall.Execute` invokes every func it encounters
+			if (typeof(TResult).IsConstructedGenericType && typeof(TResult).GetGenericTypeDefinition() == typeof(Func<>))


Hmm, I still have the nagging feeling that this isn't quite right yet:

Imagine that we're setting up a method object GetSomeObject():

mock.SetupSequence(m => m.GetSomeObject()).Returns(() => "Foo"); // TResult := object

Before your new method overload is added, the compiler will encode a call to the Returns(object) method, meaning that calling mock.Object.GetSomeObject() will return () => "Foo", not "Foo".

After your new method overload is added, if you recompile the above, the compiler will encode a call to the Returns(Func<object>) overload (which is the better match), meaning that calling mock.Object.GetSomeObject() will now return "Foo" and no longer () => "Foo".

Below you'll find a short demo of this problem. Run this program twice: Once as is (representing the status quo), and once with the last method uncommented (to see what happens when we add the method overload). Then observe how the output changes:

using System; class Program { static void Main() { Func<object> fn = () => "Foo"; Do(fn); } static void Do(object x) { Console.WriteLine(x); } //static void Do(Func<object> x) //{ // Console.WriteLine(x.Invoke()); //} }

This is the breaking change that I believe we should avoid. I'm not completely sure at this point whether there might be any others, but I don't think so.

Here's what I think we need to do:

Move the check for the special case over to the new Returns(Func<TResult>) method.

It might not even be necessary to check for the generic type definition being Func<>. Instead, it might be sufficient to check whether typeof(TResult) == typeof(object), and if so, make sure that the passed delegate (valueFunction) is not invoked, but treated as the final result value.

Please add a unit test that covers this very scenario.

Can you confirm this?

In the latest changes I tried to cover slightly different case of breaking changes:

After my changes SequenceMethodCall.Execute() treats all Delegate as a function that need to be evaluated. Because of these an old code that mocked method like Func<AnyType> GetFunc() would be broken. So it's why I have a check inside Returns(TResult). Although my checks didn't cover all cases.

Now I am considering that it's better to have a new enum value SequenceMethodCall.ResponseKind.InvokeFunc for all funcs that need to be invoked to get the return value. It will allow to remove checks that I was trying to accomplish.

Anyway I am still missing the scenario that you described. If I understand it right we also need to handle some other cases:

public interface IFoo { object GetObj(); Delegate GetDel(); MulticastDelegate GetMulticastDel() }

stakx · 2018-02-26T16:32:18Z

Moq.Tests/SequenceExtensionsFixture.cs

 		public interface IFoo
 		{
 			string Value { get; set; }

 			int Do();

 			Task<int> DoAsync();
+
+			Func<int> GetFunc();


Suggestion: Add another test that uses the new Returns overload for a method with return type object. This might be relevant because when TResult := object, both overloads Returns(object) and Returns(Func<object>) become relevant. (See also long review comment.)

stakx

All good to go now! Thank you for bearing with me. 😄

stakx · 2018-02-26T21:50:40Z

Moq.Tests/SequenceExtensionsFixture.cs

 		public interface IFoo
 		{
 			string Value { get; set; }

 			int Do();

 			Task<int> DoAsync();
+
+			Func<int> GetFunc();


snrnats added 2 commits February 26, 2018 12:40

Implemented deferred method call in SequenceMethodCall

1960e34

Updated changelog

bddce57

stakx suggested changes Feb 26, 2018

View reviewed changes

snrnats added 3 commits February 26, 2018 17:39

Removed ISetupSequentialResult<TResult>.Returns(Delegate)

870ddeb

Added unit tests

d5eb561

cleanup

fbea781

stakx suggested changes Feb 26, 2018

View reviewed changes

Added support of SequenceMethodCall.ResponseKind.InvokeFunc

ee112d2

stakx approved these changes Feb 26, 2018

View reviewed changes

stakx merged commit 9c61812 into devlooped:master Feb 26, 2018

stakx mentioned this pull request Feb 26, 2018

Usage of ReturnsExtensions.ThrowsAsync() can cause UnobservedTaskException #592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results #594

Added `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results #594

snrnats commented Feb 26, 2018

stakx left a comment •

edited

stakx Feb 26, 2018

stakx Feb 26, 2018

stakx Feb 26, 2018 •

edited

stakx Feb 26, 2018

stakx left a comment

stakx Feb 26, 2018

stakx Feb 26, 2018

stakx Feb 26, 2018

stakx left a comment

stakx Feb 26, 2018

stakx Feb 26, 2018

stakx Feb 26, 2018 •

edited

snrnats Feb 26, 2018

stakx Feb 26, 2018 •

edited

stakx Feb 26, 2018

stakx left a comment

stakx Feb 26, 2018

Added ISetupSequentialResult<TResult>.Returns method overload that support delegate for deferred results #594

Added ISetupSequentialResult<TResult>.Returns method overload that support delegate for deferred results #594

Conversation

snrnats commented Feb 26, 2018

stakx left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Feb 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Feb 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Feb 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Added `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results #594

Added `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results #594

stakx left a comment •

edited

stakx Feb 26, 2018 •

edited

stakx Feb 26, 2018 •

edited

stakx Feb 26, 2018 •

edited