Ability to mock protected methods with and without return value #845
Conversation
Jason31569
changed the title
| public static object Protected<T>(this T obj, string methodName, params object[] args) where T : class | ||
| { | ||
| if (obj == null) { throw new ArgumentNullException(nameof(obj), "Cannot mock null object"); } | ||
| if (string.IsNullOrWhiteSpace(methodName)) { throw new ArgumentException("Must provide valid protected method name to mock", nameof(methodName)); } | ||
|
|
||
| IList<IArgumentSpecification> argTypes = SubstitutionContext.Current.ThreadContext.PeekAllArgumentSpecifications(); | ||
| MethodInfo mthdInfo = obj.GetType().GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Instance, Type.DefaultBinder, argTypes.Select(x => x.ForType).ToArray(), null); | ||
|
|
||
| if (mthdInfo == null) { throw new Exception($"Method {methodName} not found"); } | ||
| if (!mthdInfo.IsVirtual) { throw new Exception($"Method {methodName} is not virtual"); } |
There was a problem hiding this comment.
suggestion (nitpick): ideally should use a custom exception(subclassing SubstituteException) with details on what they should do to fix the issue. (see NSubstitute.Exceptions for examples.)
For obj == null, we have NullSubstituteReferenceException. Should also ensure receiver is a substitute otherwise throw NotASubstituteException.
For mthdInfo == null, maybe include arg types checked, something like "No method found with signature Foo(Int, String) on IMySubstitute. Check the method name and arguments are correct."
For non-virtual, can probably use a summary of the warning given in the documentation.
| public IList<IArgumentSpecification> PeekAllArgumentSpecifications() | ||
| { | ||
| var queue = _argumentSpecifications.Value; | ||
| if (queue == null) { throw new SubstituteInternalException("Argument specification queue is null."); } |
There was a problem hiding this comment.
question: do you know under what circumstances this occurs? If it is expected can we just return EmptySpecifications in that case?
There was a problem hiding this comment.
tbh it doesn't look like it could ever be null. I decided to have/keep this in line with enqueue and dequeue methods in case it is something I failed to see
Should we change all 3 (along with enqueue, dequeue) methods to provide a consistent behavior?
| /// <returns>WhenCalled<T>.</returns> | ||
| /// <exception cref="System.ArgumentNullException">Substitute - Cannot mock null object</exception> | ||
| /// <exception cref="System.ArgumentException">Must provide valid protected method name to mock - methodName</exception> | ||
| public static WhenCalled<T> When<T>(this T obj, string methodName, params object[] args) where T : class |
There was a problem hiding this comment.
thought: maybe this should be WhenProtected or something to more easily distinguish from When? What do you think? (I'm genuinely not sure, just want something natural for people when both are in scope)
There was a problem hiding this comment.
My ocd prefers seeing a bunch of When methods for configuring methods with no return value, but I agree it may be more intuitive to some people when the method screams out "Protected", maybe we need a tie breaker if we're both on the fence?
Closes #800