* Use custom exception on protected method mock errors and additional…

… unit tests to verify error conditions * Rename unit tests with better description * Fix unit test failure due to orphaned argument spec when previous setup throws
nsubstitute · Jan 2, 2025 · 18191e2 · 18191e2
1 parent 88998b2
commit 18191e2
Show file tree

Hide file tree

Showing 6 changed files with 151 additions and 11 deletions.
diff --git a/src/NSubstitute/Core/ThreadLocalContext.cs b/src/NSubstitute/Core/ThreadLocalContext.cs
@@ -112,9 +112,8 @@ public IList<IArgumentSpecification> DequeueAllArgumentSpecifications()
     public IList<IArgumentSpecification> PeekAllArgumentSpecifications()
     {
         var queue = _argumentSpecifications.Value;
-        if (queue == null) { throw new SubstituteInternalException("Argument specification queue is null."); }
 
-        if (queue.Count > 0)
+        if (queue?.Count > 0)
         {
             var items = new IArgumentSpecification[queue.Count];
 

diff --git a/src/NSubstitute/Exceptions/ProtectedMethodNotFoundException.cs b/src/NSubstitute/Exceptions/ProtectedMethodNotFoundException.cs
@@ -0,0 +1,10 @@
+namespace NSubstitute.Exceptions;
+
+public class ProtectedMethodNotFoundException(string message, Exception? innerException) : SubstituteException(message, innerException)
+{
+    public ProtectedMethodNotFoundException() : this("", null)
+    { }
+
+    public ProtectedMethodNotFoundException(string message) : this(message, null)
+    { }
+}
diff --git a/src/NSubstitute/Exceptions/ProtectedMethodNotVirtualException.cs b/src/NSubstitute/Exceptions/ProtectedMethodNotVirtualException.cs
@@ -0,0 +1,10 @@
+namespace NSubstitute.Exceptions;
+
+public class ProtectedMethodNotVirtualException(string message, Exception? innerException) : SubstituteException(message, innerException)
+{
+    public ProtectedMethodNotVirtualException() : this("", null)
+    { }
+
+    public ProtectedMethodNotVirtualException(string message) : this(message, null)
+    { }
+}
diff --git a/src/NSubstitute/Extensions/ProtectedExtensions.cs b/src/NSubstitute/Extensions/ProtectedExtensions.cs
@@ -19,6 +19,7 @@ public static class ProtectedExtensions
     /// <param name="args">The method arguments.</param>
     /// <returns>Result object from the method invocation.</returns>
     /// <exception cref="NSubstitute.Exceptions.NullSubstituteReferenceException">Substitute - Cannot mock null object</exception>
+    /// <exception cref="NSubstitute.Exceptions.ProtectedMethodNotFoundException">Error mocking method.  Method must be protected virtual and with correct matching arguments and type</exception>
     /// <exception cref="System.ArgumentException">Must provide valid protected method name to mock - methodName</exception>
     public static object Protected<T>(this T obj, string methodName, params object[] args) where T : class
     {
@@ -28,8 +29,17 @@ public static object Protected<T>(this T obj, string methodName, params object[]
         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"); }
+        if (mthdInfo == null)
+        {
+            _ = SubstitutionContext.Current.ThreadContext.DequeueAllArgumentSpecifications();
+            throw new ProtectedMethodNotFoundException($"No protected virtual method found with signature {methodName}({string.Join(", ", argTypes.Select(x => x.ForType))}) in {obj.GetType().BaseType!.Name}. " +
+                                                    "Check that the method name and arguments are correct.  Public virtual methods must use standard NSubstitute mocking.  See the documentation for additional info.");
+        }
+        if (!mthdInfo.IsVirtual)
+        {
+            _ = SubstitutionContext.Current.ThreadContext.DequeueAllArgumentSpecifications();
+            throw new ProtectedMethodNotVirtualException($"{mthdInfo} is not virtual.  NSubstitute can only work with virtual members of the class that are overridable in the test assembly");
+        }
 
         return mthdInfo.Invoke(obj, args);
     }
@@ -43,6 +53,7 @@ public static object Protected<T>(this T obj, string methodName, params object[]
     /// <param name="args">The method arguments.</param>
     /// <returns>WhenCalled&lt;T&gt;.</returns>
     /// <exception cref="NSubstitute.Exceptions.NullSubstituteReferenceException">Substitute - Cannot mock null object</exception>
+    /// <exception cref="NSubstitute.Exceptions.ProtectedMethodNotFoundException">Error mocking method.  Method must be protected virtual and with correct matching arguments and type</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
     {
@@ -52,8 +63,17 @@ public static WhenCalled<T> When<T>(this T obj, string methodName, params object
         IList<IArgumentSpecification> argTypes = SubstitutionContext.Current.ThreadContext.PeekAllArgumentSpecifications();
         MethodInfo mthdInfo = obj.GetType().GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Instance, Type.DefaultBinder, argTypes.Select(y => y.ForType).ToArray(), null);
 
-        if (mthdInfo == null) { throw new Exception($"Method {methodName} not found"); }
-        if (!mthdInfo.IsVirtual) { throw new Exception($"Method {methodName} is not virtual"); }
+        if (mthdInfo == null)
+        {
+            _ = SubstitutionContext.Current.ThreadContext.DequeueAllArgumentSpecifications();
+            throw new ProtectedMethodNotFoundException($"No protected virtual method found with signature {methodName}({string.Join(", ", argTypes.Select(x => x.ForType))}) in {obj.GetType().BaseType!.Name}. " +
+                                                    "Check that the method name and arguments are correct.  Public virtual methods must use standard NSubstitute mocking.  See the documentation for additional info.");
+        }
+        if (!mthdInfo.IsVirtual)
+        {
+            _ = SubstitutionContext.Current.ThreadContext.DequeueAllArgumentSpecifications();
+            throw new ProtectedMethodNotVirtualException($"{mthdInfo} is not virtual.  NSubstitute can only work with virtual members of the class that are overridable in the test assembly");
+        }
 
         return new WhenCalled<T>(SubstitutionContext.Current, obj, x => mthdInfo.Invoke(x, args), MatchArgs.AsSpecifiedInCall);
     }

diff --git a/tests/NSubstitute.Acceptance.Specs/Infrastructure/AnotherClass.cs b/tests/NSubstitute.Acceptance.Specs/Infrastructure/AnotherClass.cs
@@ -14,6 +14,11 @@ public abstract class AnotherClass
 
     protected abstract void ProtectedMethodWithNoReturn(string msg, int i, char j);
 
+    public abstract void PublicVirtualMethod();
+
+    protected void ProtectedNonVirtualMethod()
+    { }
+
     public string DoWork()
     {
         return ProtectedMethod();

diff --git a/tests/NSubstitute.Acceptance.Specs/ProtectedExtensionsTests.cs b/tests/NSubstitute.Acceptance.Specs/ProtectedExtensionsTests.cs
@@ -1,4 +1,5 @@
 using NSubstitute.Acceptance.Specs.Infrastructure;
+using NSubstitute.Exceptions;
 using NSubstitute.Extensions;
 using NUnit.Framework;
 
@@ -29,8 +30,7 @@ public void Should_mock_and_verify_protected_method_with_arg()
         sub.Protected("ProtectedMethod", Arg.Any<int>()).Returns(expectedMsg);
 
         Assert.That(worker.DoMoreWork(sub, 5), Is.EqualTo(expectedMsg));
-        var a = sub.Received(1);
-        a.Protected("ProtectedMethod", Arg.Any<int>());
+        sub.Received(1).Protected("ProtectedMethod", Arg.Any<int>());
     }
 
     [Test]
@@ -47,7 +47,55 @@ public void Should_mock_and_verify_protected_method_with_multiple_args()
     }
 
     [Test]
-    public void Should_mock_and_verify_method_with_no_return_and_no_args()
+    public void Should_throw_on_mock_null_substitute()
+    {
+        Assert.Throws<NullSubstituteReferenceException>(() => (null as AnotherClass).Protected("ProtectedMethod"));
+    }
+
+    [TestCase("")]
+    [TestCase("   ")]
+    [TestCase(null)]
+    public void Should_throw_on_mock_invalid_method_name(string methodName)
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ArgumentException>(() => sub.Protected(methodName));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_method_not_found()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotFoundException>(() => sub.Protected("MethodDoesNotExist"));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_method_arg_mismatch()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotFoundException>(() => sub.Protected("ProtectedMethod", Arg.Any<IEnumerable<char>>()));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_public_virtual_method()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotFoundException>(() => sub.Protected("PublicVirtualMethod"));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_non_virtual()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotVirtualException>(() => sub.Protected("ProtectedNonVirtualMethod"));
+    }
+
+    [Test]
+    public void Should_mock_and_verify_void_method_and_no_args()
     {
         var count = 0;
         var sub = Substitute.For<AnotherClass>();
@@ -61,7 +109,7 @@ public void Should_mock_and_verify_method_with_no_return_and_no_args()
     }
 
     [Test]
-    public void Should_mock_and_verify_method_with_no_return_with_arg()
+    public void Should_mock_and_verify_void_method_with_arg()
     {
         var count = 0;
         var sub = Substitute.For<AnotherClass>();
@@ -75,7 +123,7 @@ public void Should_mock_and_verify_method_with_no_return_with_arg()
     }
 
     [Test]
-    public void Should_mock_and_verify_method_with_no_return_with_multiple_args()
+    public void Should_mock_and_verify_void_method_with_multiple_args()
     {
         var count = 0;
         var sub = Substitute.For<AnotherClass>();
@@ -88,6 +136,54 @@ public void Should_mock_and_verify_method_with_no_return_with_multiple_args()
         sub.Received(1).Protected("ProtectedMethodWithNoReturn", Arg.Any<string>(), Arg.Any<int>(), Arg.Any<char>());
     }
 
+    [Test]
+    public void Should_throw_on_void_method_null_substitute()
+    {
+        Assert.Throws<NullSubstituteReferenceException>(() => (null as AnotherClass).When("ProtectedMethod"));
+    }
+
+    [TestCase("")]
+    [TestCase("   ")]
+    [TestCase(null)]
+    public void Should_throw_on_mock_void_method_invalid_method_name(string methodName)
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ArgumentException>(() => sub.When(methodName));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_void_method_not_found()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotFoundException>(() => sub.When("MethodDoesNotExist"));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_void_method_arg_mismatch()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotFoundException>(() => sub.When("ProtectedMethod", Arg.Any<IEnumerable<char>>()));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_public_virtual_void_method()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotFoundException>(() => sub.When("PublicVirtualMethod"));
+    }
+
+    [Test]
+    public void Should_throw_on_mock_non_virtual_void_method()
+    {
+        var sub = Substitute.For<AnotherClass>();
+
+        Assert.Throws<ProtectedMethodNotVirtualException>(() => sub.When("ProtectedNonVirtualMethod"));
+    }
+
     private class Worker
     {
         internal string DoWork(AnotherClass worker)