From f1dc3d12006c11162773617ac693a9583ef3fab3 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 24 Aug 2019 23:09:54 +0200 Subject: [PATCH 1/5] Add tests from GitHub #657, #658, and #903 --- tests/Moq.Tests/AsInterfaceFixture.cs | 27 ++++ .../Regressions/IssueReportsFixture.cs | 128 ++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/tests/Moq.Tests/AsInterfaceFixture.cs b/tests/Moq.Tests/AsInterfaceFixture.cs index cd7857f80..29be1a9e9 100644 --- a/tests/Moq.Tests/AsInterfaceFixture.cs +++ b/tests/Moq.Tests/AsInterfaceFixture.cs @@ -171,6 +171,33 @@ public void ShouldNotThrowIfCallExplicitlyImplementedInterfacesMethodWhenCallBas bag.Get("test"); } + [Fact] + public void Setup_targets_method_implementing_interface_not_other_method_with_same_signature() + { + var mock = new Mock() { CallBase = true }; + mock.As().Setup(m => m.GetValue()).Returns(3); + + // The public method should have been left alone since it's not the one implementing `IService`: + var valueOfOtherMethod = mock.Object.GetValue(); + Assert.Equal(1, valueOfOtherMethod); + + // The method implementing the interface method should have be mocked: + var valueOfSetupMethod = ((IService)mock.Object).GetValue(); + Assert.Equal(3, valueOfSetupMethod); + } + + public class Service : IService + { + public virtual int GetValue() => 1; + + int IService.GetValue() => 2; + } + + public interface IService + { + int GetValue(); + } + // see also test fixture `Issue458` in `IssueReportsFixture` public interface IFoo diff --git a/tests/Moq.Tests/Regressions/IssueReportsFixture.cs b/tests/Moq.Tests/Regressions/IssueReportsFixture.cs index ba13d43ce..52fc51c66 100644 --- a/tests/Moq.Tests/Regressions/IssueReportsFixture.cs +++ b/tests/Moq.Tests/Regressions/IssueReportsFixture.cs @@ -2283,6 +2283,46 @@ public interface IMyInterface #endregion + #region 657 + + public class Issue657 + { + [Fact] + public void Test() + { + var mock = new Moq.Mock() { CallBase = true }; + mock.As().Setup(m => m.DoWork()).Returns(3); + Assert.Equal(1, mock.Object.PublicWork()); + } + + public class Contract : IContractOveride + { + public int PublicWork() + { + return this.DoWork(); + } + + protected virtual int DoWork() + { + ((IContractOveride)this).DoWork(); + return 1; + } + + int IContractOveride.DoWork() + { + Console.WriteLine("IFace"); + return 2; + } + } + + public interface IContractOveride + { + int DoWork(); + } + } + + #endregion + #region 696 public class Issue696 @@ -2844,6 +2884,94 @@ public interface IMeteringDataServiceAgent #endregion + #region 903 + + public class Issue903 + { + public interface IX + { + void Method(bool arg); + void Method(int arg); + } + + private readonly Mock mock; + + public Issue903() + { + this.mock = new Mock(); + } + + [Fact] + public void Bool_method_was_setup_first__when_bool_method_invoked__bool_method_setup_should_be_matched() + { + var boolMethodInvoked = false; + this.SetupBoolMethod(() => boolMethodInvoked = true); + this.SetupIntMethod(() => throw new Exception("Wrong method called.")); + + this.InvokeBoolMethod(); + + Assert.True(boolMethodInvoked); + } + + [Fact] + public void Bool_method_was_setup_last__when_bool_method_invoked__bool_method_setup_should_be_matched() + { + var boolMethodInvoked = false; + this.SetupIntMethod(() => throw new Exception("Wrong method called.")); + this.SetupBoolMethod(() => boolMethodInvoked = true); + + this.InvokeBoolMethod(); + + Assert.True(boolMethodInvoked); + } + + [Fact] + public void Int_method_was_setup_last__when_int_method_invoked__int_method_setup_should_be_matched() + { + bool intMethodInvoked = false; + this.SetupBoolMethod(() => throw new Exception("Wrong method called.")); + this.SetupIntMethod(() => intMethodInvoked = true); + + this.InvokeIntMethod(); + + Assert.True(intMethodInvoked); + } + + [Fact] + public void Int_method_was_setup_first__when_int_method_invoked__int_method_setup_should_be_matched() + { + bool intMethodInvoked = false; + this.SetupIntMethod(() => intMethodInvoked = true); + this.SetupBoolMethod(() => throw new Exception("Wrong method called.")); + + this.InvokeIntMethod(); + + Assert.True(intMethodInvoked); + } + + private void InvokeBoolMethod() + { + this.mock.Object.Method(default(bool)); + } + + private void InvokeIntMethod() + { + this.mock.Object.Method(default(int)); + } + + private void SetupBoolMethod(Action callback) + { + mock.Setup(m => m.Method((bool)It.IsAny())).Callback(callback); + } + + private void SetupIntMethod(Action callback) + { + this.mock.Setup(m => m.Method((int)It.IsAny())).Callback(callback); + } + } + + #endregion + // Old @ Google Code #region #47 From 0622c1cc2eb5bf3cfe3cfbe7eb4d63ead6511183 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 24 Aug 2019 20:43:19 +0200 Subject: [PATCH 2/5] `method.GetImplementingMethod()` extension method --- src/Moq/Extensions.cs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/Moq/Extensions.cs b/src/Moq/Extensions.cs index 96d15ed78..adcedb010 100644 --- a/src/Moq/Extensions.cs +++ b/src/Moq/Extensions.cs @@ -26,6 +26,44 @@ public static object GetDefaultValue(this Type type) return type.IsValueType ? Activator.CreateInstance(type) : null; } + /// + /// Gets the least-derived in the given type that provides + /// the implementation for the given . + /// + public static MethodInfo GetImplementingMethod(this MethodInfo method, Type proxyType) + { + Debug.Assert(method != null); + Debug.Assert(proxyType != null); + Debug.Assert(proxyType.IsClass); + + if (method.IsGenericMethod) + { + method = method.GetGenericMethodDefinition(); + } + + var declaringType = method.DeclaringType; + + if (declaringType.IsInterface) + { + Debug.Assert(declaringType.IsAssignableFrom(proxyType)); + + var map = proxyType.GetInterfaceMap(method.DeclaringType); + var index = Array.IndexOf(map.InterfaceMethods, method); + Debug.Assert(index >= 0); + return map.TargetMethods[index].GetBaseDefinition(); + } + else if (declaringType.IsDelegateType()) + { + return proxyType.GetMethod("Invoke"); + } + else + { + Debug.Assert(declaringType.IsAssignableFrom(proxyType)); + + return method.GetBaseDefinition(); + } + } + public static object InvokePreserveStack(this Delegate del, params object[] args) { try From 1c1a934745506fd42146328e5d31e772afdac199 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 24 Aug 2019 19:04:09 +0200 Subject: [PATCH 3/5] Use impl mapping to match invoked & expected methods --- src/Moq/Invocation.cs | 9 +++++- src/Moq/InvocationShape.cs | 34 ++++++-------------- src/Moq/ProxyFactories/CastleProxyFactory.cs | 2 +- src/Moq/ProxyFactories/InterfaceProxy.cs | 24 ++++++++------ 4 files changed, 33 insertions(+), 36 deletions(-) diff --git a/src/Moq/Invocation.cs b/src/Moq/Invocation.cs index 406888d85..a3ec9435a 100644 --- a/src/Moq/Invocation.cs +++ b/src/Moq/Invocation.cs @@ -1,6 +1,7 @@ // Copyright (c) 2007, Clarius Consulting, Manas Technology Solutions, InSTEDD. // All rights reserved. Licensed under the BSD 3-Clause License; see License.txt. +using System; using System.Collections.Generic; using System.Diagnostics; using System.Reflection; @@ -10,6 +11,7 @@ namespace Moq { internal abstract class Invocation : IInvocation { + private readonly Type proxyType; private object[] arguments; private MethodInfo method; private object returnValue; @@ -18,13 +20,16 @@ internal abstract class Invocation : IInvocation /// /// Initializes a new instance of the class. /// + /// The of the concrete proxy object on which a method is being invoked. /// The method being invoked. /// The arguments with which the specified is being invoked. - protected Invocation(MethodInfo method, params object[] arguments) + protected Invocation(Type proxyType, MethodInfo method, params object[] arguments) { + Debug.Assert(proxyType != null); Debug.Assert(arguments != null); Debug.Assert(method != null); + this.proxyType = proxyType; this.arguments = arguments; this.method = method; } @@ -45,6 +50,8 @@ protected Invocation(MethodInfo method, params object[] arguments) IReadOnlyList IInvocation.Arguments => this.arguments; + public Type ProxyType => this.proxyType; + public object ReturnValue => this.returnValue; internal bool Verified => this.verificationState == VerificationState.Verified; diff --git a/src/Moq/InvocationShape.cs b/src/Moq/InvocationShape.cs index 64ab7cdbf..a258bb316 100644 --- a/src/Moq/InvocationShape.cs +++ b/src/Moq/InvocationShape.cs @@ -60,17 +60,12 @@ public void Deconstruct(out LambdaExpression expression, out MethodInfo method, public bool IsMatch(Invocation invocation) { - var arguments = invocation.Arguments; - if (this.argumentMatchers.Length != arguments.Length) - { - return false; - } - - if (invocation.Method != this.Method && !this.IsOverride(invocation.Method)) + if (invocation.Method != this.Method && !this.IsOverride(invocation)) { return false; } + var arguments = invocation.Arguments; for (int i = 0, n = this.argumentMatchers.Length; i < n; ++i) { if (this.argumentMatchers[i].Matches(arguments[i]) == false) @@ -91,21 +86,19 @@ public void SetupEvaluatedSuccessfully(Invocation invocation) } } - private bool IsOverride(MethodInfo invocationMethod) + private bool IsOverride(Invocation invocation) { + Debug.Assert(invocation.Method != this.Method); + var method = this.Method; + var invocationMethod = invocation.Method; - if (!method.DeclaringType.IsAssignableFrom(invocationMethod.DeclaringType)) - { - return false; - } + var proxyType = invocation.ProxyType; - if (!method.Name.Equals(invocationMethod.Name, StringComparison.Ordinal)) - { - return false; - } + var invocationMethodImplementation = invocationMethod.GetImplementingMethod(proxyType); + var methodImplementation = method.GetImplementingMethod(proxyType); - if (method.ReturnType != invocationMethod.ReturnType) + if (invocationMethodImplementation != methodImplementation) { return false; } @@ -117,13 +110,6 @@ private bool IsOverride(MethodInfo invocationMethod) return false; } } - else - { - if (!invocationMethod.GetParameterTypes().CompareTo(method.GetParameterTypes(), exact: true)) - { - return false; - } - } return true; } diff --git a/src/Moq/ProxyFactories/CastleProxyFactory.cs b/src/Moq/ProxyFactories/CastleProxyFactory.cs index 928b47b32..ef6564e6e 100644 --- a/src/Moq/ProxyFactories/CastleProxyFactory.cs +++ b/src/Moq/ProxyFactories/CastleProxyFactory.cs @@ -129,7 +129,7 @@ private sealed class Invocation : Moq.Invocation { private Castle.DynamicProxy.IInvocation underlying; - internal Invocation(Castle.DynamicProxy.IInvocation underlying) : base(underlying.Method, underlying.Arguments) + internal Invocation(Castle.DynamicProxy.IInvocation underlying) : base(underlying.Proxy.GetType(), underlying.Method, underlying.Arguments) { this.underlying = underlying; } diff --git a/src/Moq/ProxyFactories/InterfaceProxy.cs b/src/Moq/ProxyFactories/InterfaceProxy.cs index 8547b6988..c1c4c278f 100644 --- a/src/Moq/ProxyFactories/InterfaceProxy.cs +++ b/src/Moq/ProxyFactories/InterfaceProxy.cs @@ -1,6 +1,7 @@ // Copyright (c) 2007, Clarius Consulting, Manas Technology Solutions, InSTEDD. // All rights reserved. Licensed under the BSD 3-Clause License; see License.txt. +using System; using System.ComponentModel; using System.Diagnostics; using System.Reflection; @@ -25,8 +26,9 @@ public abstract class InterfaceProxy public sealed override bool Equals(object obj) { // Forward this call to the interceptor, so that `object.Equals` can be set up. - var invocation = new Invocation(equalsMethod, obj); - ((IInterceptor)((IProxy)this).Interceptor).Intercept(invocation); + var interceptor = (IInterceptor)((IProxy)this).Interceptor; + var invocation = new Invocation(interceptor.GetType(), equalsMethod, obj); + interceptor.Intercept(invocation); return (bool)invocation.ReturnValue; } @@ -35,8 +37,9 @@ public sealed override bool Equals(object obj) public sealed override int GetHashCode() { // Forward this call to the interceptor, so that `object.GetHashCode` can be set up. - var invocation = new Invocation(getHashCodeMethod); - ((IInterceptor)((IProxy)this).Interceptor).Intercept(invocation); + var interceptor = (IInterceptor)((IProxy)this).Interceptor; + var invocation = new Invocation(interceptor.GetType(), getHashCodeMethod); + interceptor.Intercept(invocation); return (int)invocation.ReturnValue; } @@ -45,8 +48,9 @@ public sealed override int GetHashCode() public sealed override string ToString() { // Forward this call to the interceptor, so that `object.ToString` can be set up. - var invocation = new Invocation(toStringMethod); - ((IInterceptor)((IProxy)this).Interceptor).Intercept(invocation); + var interceptor = (IInterceptor)((IProxy)this).Interceptor; + var invocation = new Invocation(interceptor.GetType(), toStringMethod); + interceptor.Intercept(invocation); return (string)invocation.ReturnValue; } @@ -54,13 +58,13 @@ private sealed class Invocation : Moq.Invocation { private static object[] noArguments = new object[0]; - public Invocation(MethodInfo method, params object[] arguments) - : base(method, arguments) + public Invocation(Type proxyType, MethodInfo method, params object[] arguments) + : base(proxyType, method, arguments) { } - public Invocation(MethodInfo method) - : base(method, noArguments) + public Invocation(Type proxyType, MethodInfo method) + : base(proxyType, method, noArguments) { } From 1ffe0ed0ea56a6df9580e196b89200da4d8cf622 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 24 Aug 2019 22:27:04 +0200 Subject: [PATCH 4/5] Cache the method impl mappings --- src/Moq/Invocation.cs | 18 ++++++++++++++++-- src/Moq/InvocationShape.cs | 28 +++++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/Moq/Invocation.cs b/src/Moq/Invocation.cs index a3ec9435a..6d7c6b5fa 100644 --- a/src/Moq/Invocation.cs +++ b/src/Moq/Invocation.cs @@ -11,9 +11,10 @@ namespace Moq { internal abstract class Invocation : IInvocation { - private readonly Type proxyType; private object[] arguments; private MethodInfo method; + private MethodInfo methodImplementation; + private readonly Type proxyType; private object returnValue; private VerificationState verificationState; @@ -29,9 +30,9 @@ protected Invocation(Type proxyType, MethodInfo method, params object[] argument Debug.Assert(arguments != null); Debug.Assert(method != null); - this.proxyType = proxyType; this.arguments = arguments; this.method = method; + this.proxyType = proxyType; } /// @@ -39,6 +40,19 @@ protected Invocation(Type proxyType, MethodInfo method, params object[] argument /// public MethodInfo Method => this.method; + public MethodInfo MethodImplementation + { + get + { + if (this.methodImplementation == null) + { + this.methodImplementation = this.method.GetImplementingMethod(this.proxyType); + } + + return this.methodImplementation; + } + } + /// /// Gets the arguments of the invocation. /// diff --git a/src/Moq/InvocationShape.cs b/src/Moq/InvocationShape.cs index a258bb316..27db5d9fb 100644 --- a/src/Moq/InvocationShape.cs +++ b/src/Moq/InvocationShape.cs @@ -28,7 +28,11 @@ internal sealed class InvocationShape : IEquatable public readonly IReadOnlyList Arguments; private readonly IMatcher[] argumentMatchers; + private MethodInfo methodImplementation; private Expression[] partiallyEvaluatedArguments; +#if DEBUG + private Type proxyType; +#endif public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnlyList arguments = null) { @@ -94,11 +98,29 @@ private bool IsOverride(Invocation invocation) var invocationMethod = invocation.Method; var proxyType = invocation.ProxyType; +#if DEBUG + // The following `if` block is a sanity check to ensure this `InvocationShape` always + // runs against the same proxy type. This is important because we're caching the result + // of mapping methods into that particular proxy type. We have no cache invalidation + // logic in place; instead, we simply assume that the cached results will stay valid. + // If the below assertion fails, that assumption was wrong. + if (this.proxyType == null) + { + this.proxyType = proxyType; + } + else + { + Debug.Assert(this.proxyType == proxyType); + } +#endif - var invocationMethodImplementation = invocationMethod.GetImplementingMethod(proxyType); - var methodImplementation = method.GetImplementingMethod(proxyType); + // If not already in the cache, map this `InvocationShape`'s method into the proxy type: + if (this.methodImplementation == null) + { + this.methodImplementation = method.GetImplementingMethod(proxyType); + } - if (invocationMethodImplementation != methodImplementation) + if (invocation.MethodImplementation != this.methodImplementation) { return false; } From 8b9f2e25e0559e53eb121f2c009d3ca41692eba8 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 24 Aug 2019 23:44:40 +0200 Subject: [PATCH 5/5] Update the changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c6b3a700..f7463b6bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,12 +19,16 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 * Moq will throw when it detects that an argument matcher will never match anything due to the presence of an implicit conversion operator. (@michelcedric, #897, #898) +* New algorithm for matching invoked methods against methods specified in setup/verification expressions. (@stakx, #904) + #### Added * Added support for setup and verification of the event handlers through `Setup[Add|Remove]` and `Verify[Add|Remove|All]` (@lepijohnny, #825) #### Fixed +* Moq does not mock explicit interface implementation and `protected virtual` correctly. (@oddbear, #657) + * `Invocations.Clear()` does not cause `Verify` to fail (@jchessir, #733) * Regression: `SetupAllProperties` can no longer set up properties whose names start with `Item`. (@mattzink, #870; @kaan-kaya, #869) @@ -35,6 +39,8 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 * Regression in 4.12.0: `SetupAllProperties` removes indexer setups. (@stakx, #901) +* Parameter types are ignored when matching an invoked generic method against setups. (@stakx, #903) + ## 4.12.0 (2019-06-20)