From 7a85743db0a650187b8eaeb75bed6221d2a501fa Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 10 Nov 2017 17:54:06 +0100 Subject: [PATCH 1/5] Add tests documenting current behavior of `Returns` --- Moq.Tests/ReturnsValidationFixture.cs | 123 ++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 Moq.Tests/ReturnsValidationFixture.cs diff --git a/Moq.Tests/ReturnsValidationFixture.cs b/Moq.Tests/ReturnsValidationFixture.cs new file mode 100644 index 000000000..014edae1f --- /dev/null +++ b/Moq.Tests/ReturnsValidationFixture.cs @@ -0,0 +1,123 @@ +using System; +using System.Reflection; + +using Moq.Language.Flow; + +using Xunit; + +namespace Moq.Tests +{ + public class ReturnsValidationFixture + { + private Mock mock; + private ISetup setup; + + public ReturnsValidationFixture() + { + this.mock = new Mock(); + this.setup = this.mock.Setup(m => m.Method(It.IsAny())); + } + + [Fact] + public void Returns_does_not_accept_delegate_without_return_value() + { + Action delegateWithoutReturnValue = (arg) => { }; + + var ex = Record.Exception(() => + { + this.setup.Returns(delegateWithoutReturnValue); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_accepts_delegate_with_wrong_return_type_but_setup_invocation_will_fail() + { + Func delegateWithWrongReturnType = () => "42"; + this.setup.Returns(delegateWithWrongReturnType); + + var ex = Record.Exception(() => + { + this.mock.Object.Method(42); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_accepts_delegate_with_wrong_return_type_and_setup_invocation_will_succeed_if_retval_convertible() + { + Func delegateWithWrongReturnType = () => null; + this.setup.Returns(delegateWithWrongReturnType); + + var ex = Record.Exception(() => + { + this.mock.Object.Method(42); + }); + + Assert.Null(ex); + } + + [Fact] + public void Returns_accepts_parameterless_delegate_even_for_method_having_parameters() + { + Func delegateWithoutParameters = () => default(IType); + this.setup.Returns(delegateWithoutParameters); + + var ex = Record.Exception(() => + { + this.mock.Object.Method(42); + }); + + Assert.Null(ex); + } + + [Fact] + public void Returns_accepts_delegate_with_wrong_parameter_count_but_setup_invocation_will_fail() + { + Func delegateWithWrongParameterCount = (arg1, arg2) => default(IType); + this.setup.Returns(delegateWithWrongParameterCount); + + var ex = Record.Exception(() => + { + this.mock.Object.Method(42); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_accepts_delegate_with_wrong_parameter_types_but_setup_invocation_will_fail() + { + Func delegateWithWrongParameterType = (arg) => default(IType); + this.setup.Returns(delegateWithWrongParameterType); + + var ex = Record.Exception(() => + { + mock.Object.Method(42); + }); + + Assert.IsType(ex); + } + + [Fact] + public void Returns_accepts_delegate_with_wrong_parameter_types_and_setup_invocation_will_succeed_if_args_convertible() + { + Func delegateWithWrongParameterType = (arg) => default(IType); + this.setup.Returns(delegateWithWrongParameterType); + + var ex = Record.Exception(() => + { + mock.Object.Method(null); + }); + + Assert.Null(ex); + } + + public interface IType + { + IType Method(object arg); + } + } +} From 928d34eaf0add1d86ef38ab3bf026d8b3ac6d390 Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 10 Nov 2017 18:07:42 +0100 Subject: [PATCH 2/5] Validate `Returns` delegate return type This introduces one minor breaking change: `Returns` delegates with non-matching return types could still succeed during an actual invoc- ation if the return value was convertible from the "wrong" type to the expected type (e.g. with `null` for any two reference types). --- Moq.Tests/ReturnsValidationFixture.cs | 21 +++------------------ Source/MethodCall.cs | 4 ++-- Source/MethodCallReturn.cs | 24 ++++++++++++++++++++++++ Source/Properties/Resources.Designer.cs | 9 +++++++++ Source/Properties/Resources.resx | 3 +++ 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/Moq.Tests/ReturnsValidationFixture.cs b/Moq.Tests/ReturnsValidationFixture.cs index 014edae1f..9359c0c1a 100644 --- a/Moq.Tests/ReturnsValidationFixture.cs +++ b/Moq.Tests/ReturnsValidationFixture.cs @@ -32,31 +32,16 @@ public void Returns_does_not_accept_delegate_without_return_value() } [Fact] - public void Returns_accepts_delegate_with_wrong_return_type_but_setup_invocation_will_fail() + public void Returns_does_not_accept_delegate_with_wrong_return_type() { Func delegateWithWrongReturnType = () => "42"; - this.setup.Returns(delegateWithWrongReturnType); var ex = Record.Exception(() => { - this.mock.Object.Method(42); + this.setup.Returns(delegateWithWrongReturnType); }); - Assert.IsType(ex); - } - - [Fact] - public void Returns_accepts_delegate_with_wrong_return_type_and_setup_invocation_will_succeed_if_retval_convertible() - { - Func delegateWithWrongReturnType = () => null; - this.setup.Returns(delegateWithWrongReturnType); - - var ex = Record.Exception(() => - { - this.mock.Object.Method(42); - }); - - Assert.Null(ex); + Assert.IsType(ex); } [Fact] diff --git a/Source/MethodCall.cs b/Source/MethodCall.cs index 31d55fd71..ab07540c1 100644 --- a/Source/MethodCall.cs +++ b/Source/MethodCall.cs @@ -157,14 +157,14 @@ public string FailMessage set => this.failMessage = value; } + public MethodInfo Method => this.method; + bool IProxyCall.IsConditional => this.condition != null; bool IProxyCall.IsVerifiable => this.verifiable; bool IProxyCall.Invoked => this.callCount > 0; - MethodInfo IProxyCall.Method => this.method; - Expression IProxyCall.SetupExpression => this.originalExpression; [Conditional("DESKTOP")] diff --git a/Source/MethodCallReturn.cs b/Source/MethodCallReturn.cs index c0ac1ea1c..a120018be 100644 --- a/Source/MethodCallReturn.cs +++ b/Source/MethodCallReturn.cs @@ -43,6 +43,7 @@ using Moq.Properties; using Moq.Proxy; using System; +using System.Globalization; using System.Linq.Expressions; using System.Reflection; @@ -157,10 +158,33 @@ IReturnsThrowsGetter ICallbackGetter.Callback(Ac private void SetReturnDelegate(Delegate value) { + if (value != null) + { + ValidateReturnDelegate(value); + } + this.valueDel = value; this.returnValueKind = ReturnValueKind.Explicit; } + private void ValidateReturnDelegate(Delegate callback) + { + var callbackMethod = callback.GetMethodInfo(); + + var expectedReturnType = this.Method.ReturnType; + var actualReturnType = callbackMethod.ReturnType; + + if (!expectedReturnType.IsAssignableFrom(actualReturnType)) + { + throw new ArgumentException( + string.Format( + CultureInfo.CurrentCulture, + Resources.InvalidCallbackReturnTypeMismatch, + expectedReturnType, + actualReturnType)); + } + } + protected override void SetCallbackWithoutArguments(Action callback) { if (this.ProvidesReturnValue()) diff --git a/Source/Properties/Resources.Designer.cs b/Source/Properties/Resources.Designer.cs index 42397a2c2..865c23002 100644 --- a/Source/Properties/Resources.Designer.cs +++ b/Source/Properties/Resources.Designer.cs @@ -186,6 +186,15 @@ internal static string InvalidCallbackParameterMismatch { } } + /// + /// Looks up a localized string similar to Invalid callback. Setup on method with return type ({0}) cannot invoke callback with return type ({1}).. + /// + internal static string InvalidCallbackReturnTypeMismatch { + get { + return ResourceManager.GetString("InvalidCallbackReturnTypeMismatch", resourceCulture); + } + } + /// /// Looks up a localized string similar to Type to mock must be an interface or an abstract or non-sealed class. . /// diff --git a/Source/Properties/Resources.resx b/Source/Properties/Resources.resx index d80d1b94f..7ab3e7585 100644 --- a/Source/Properties/Resources.resx +++ b/Source/Properties/Resources.resx @@ -365,4 +365,7 @@ Expected invocation on the mock once, but was {4} times: {1} Type {0} does not have matching protected member: {1} + + Invalid callback. Setup on method with return type ({0}) cannot invoke callback with return type ({1}). + \ No newline at end of file From 4f85ab0cee9d725e16a917ad9b912f504422ce6c Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 10 Nov 2017 18:19:26 +0100 Subject: [PATCH 3/5] Validate `Returns` delegate parameter count --- Moq.Tests/ReturnsValidationFixture.cs | 7 +++---- Source/MethodCallReturn.cs | 15 +++++++++++++++ Source/Properties/Resources.Designer.cs | 9 +++++++++ Source/Properties/Resources.resx | 3 +++ 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/Moq.Tests/ReturnsValidationFixture.cs b/Moq.Tests/ReturnsValidationFixture.cs index 9359c0c1a..c26ec44b5 100644 --- a/Moq.Tests/ReturnsValidationFixture.cs +++ b/Moq.Tests/ReturnsValidationFixture.cs @@ -59,17 +59,16 @@ public void Returns_accepts_parameterless_delegate_even_for_method_having_parame } [Fact] - public void Returns_accepts_delegate_with_wrong_parameter_count_but_setup_invocation_will_fail() + public void Returns_does_not_accept_delegate_with_wrong_parameter_count() { Func delegateWithWrongParameterCount = (arg1, arg2) => default(IType); - this.setup.Returns(delegateWithWrongParameterCount); var ex = Record.Exception(() => { - this.mock.Object.Method(42); + this.setup.Returns(delegateWithWrongParameterCount); }); - Assert.IsType(ex); + Assert.IsType(ex); } [Fact] diff --git a/Source/MethodCallReturn.cs b/Source/MethodCallReturn.cs index a120018be..9ea5229b2 100644 --- a/Source/MethodCallReturn.cs +++ b/Source/MethodCallReturn.cs @@ -171,6 +171,21 @@ private void ValidateReturnDelegate(Delegate callback) { var callbackMethod = callback.GetMethodInfo(); + var actualParameters = callbackMethod.GetParameters(); + if (actualParameters.Length > 0) + { + var expectedParameters = this.Method.GetParameters(); + if (actualParameters.Length != expectedParameters.Length) + { + throw new ArgumentException( + string.Format( + CultureInfo.CurrentCulture, + Resources.InvalidCallbackParameterCountMismatch, + expectedParameters.Length, + actualParameters.Length)); + } + } + var expectedReturnType = this.Method.ReturnType; var actualReturnType = callbackMethod.ReturnType; diff --git a/Source/Properties/Resources.Designer.cs b/Source/Properties/Resources.Designer.cs index 865c23002..f7e874ddb 100644 --- a/Source/Properties/Resources.Designer.cs +++ b/Source/Properties/Resources.Designer.cs @@ -177,6 +177,15 @@ internal static string InvalidCallbackNotADelegateWithReturnTypeVoid { } } + /// + /// Looks up a localized string similar to Invalid callback. Setup on method with {0} parameter(s) cannot invoke callback with different number of parameters ({1}).. + /// + internal static string InvalidCallbackParameterCountMismatch { + get { + return ResourceManager.GetString("InvalidCallbackParameterCountMismatch", resourceCulture); + } + } + /// /// Looks up a localized string similar to Invalid callback. Setup on method with parameters ({0}) cannot invoke callback with parameters ({1}).. /// diff --git a/Source/Properties/Resources.resx b/Source/Properties/Resources.resx index 7ab3e7585..81831732b 100644 --- a/Source/Properties/Resources.resx +++ b/Source/Properties/Resources.resx @@ -368,4 +368,7 @@ Expected invocation on the mock once, but was {4} times: {1} Invalid callback. Setup on method with return type ({0}) cannot invoke callback with return type ({1}). + + Invalid callback. Setup on method with {0} parameter(s) cannot invoke callback with different number of parameters ({1}). + \ No newline at end of file From 79480e2364d4e0c91c5a904edf3e3cdab453f418 Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 10 Nov 2017 19:15:55 +0100 Subject: [PATCH 4/5] Do NOT validate `Returns` delegate parameter types --- Moq.Tests/ReturnsValidationFixture.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Moq.Tests/ReturnsValidationFixture.cs b/Moq.Tests/ReturnsValidationFixture.cs index c26ec44b5..5a0d5c217 100644 --- a/Moq.Tests/ReturnsValidationFixture.cs +++ b/Moq.Tests/ReturnsValidationFixture.cs @@ -83,6 +83,20 @@ public void Returns_accepts_delegate_with_wrong_parameter_types_but_setup_invoca }); Assert.IsType(ex); + + // In case you're wondering why this use case isn't "fixed" by properly validating delegates + // passed to `Returns`... it's entirely possible that some people might do this: + // + // mock.Setup(m => m.Method(It.IsAny()).Returns(obj => ...); + // mock.Setup(m => m.Method(It.IsAny()).Returns(obj => ...); + // + // where `Method` has a parameter of type `object`. That is, people might rely on a matcher + // to ensure that the return callback delegate invocation (and the cast to `object` that has to + // happen) will always succeed. See also the next test, as well as old Google Code issue 267 + // in `IssueReportsFixture.cs`. + // + // While not the cleanest of techniques, it might be useful to some people and probably + // shouldn't be broken by eagerly validating everything. } [Fact] From 46b2f06831bd163f06fb15de69f4451a89d1fd7a Mon Sep 17 00:00:00 2001 From: stakx Date: Fri, 10 Nov 2017 19:18:46 +0100 Subject: [PATCH 5/5] Update the changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d27dbd568..8f7688213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 #### Changed * **Breaking change:** `SetupSequence` now overrides pre-existing setups like all other `Setup` methods do. This means that exhausted sequences no longer fall back to previous setups to produce a "default" action or return value. (@stakx, #476) +* Delegates passed to `Returns` are validated a little more strictly than before (return type and parameter count must match with method being set up) (@stakx, #520) #### Fixed