Feature: Extend PartsOf to mock non-virtual methods implementing an i… #700

marcoregueira · 2022-07-21T19:09:14Z

Hi

I was in need of a feature allowing PartsOf to mock non-virtual methods implementing an interface, just the same as requested in #625.

I decided to tinker a bit to see If there was any way of doing it. Solution came out faster than expected and here are the results. I thought you might find it useful enough to include it.

In short, Castle already provides this feature and the task was, mostly, opening a way to expose it without breaking anything.

How to use:
~~var substitute = Substitute.ForPartsOf<ISomeInterface,SomeImplementation>(argsList);~~
var substitute = Substitute.ForTypeForwardingTo<ISomeInterface,SomeImplementation>(argsList);

Tests are included, of course, to see it in action.

Obvious limitations:

Overriding virtual methods effectively replaces its implementation both for internal and external calls. With this implementation Nsubstitute will only intercept calls made by client classes using the interface. Calls made from inside the object itself to its own method will hit the actual implementation.

Possible improvements

The next logical improvement is to provide mocking for already created objects conforming to an interface. Something like this:

ISomeInterface  someObject = new SomeClass();
ISomeInterface substitute = Substitute.ForInstanceForwardingTo<ISomeInterface>(someObject);

Please kindly take a look and give your feedback! Thanks!

marcoregueira · 2022-07-21T21:07:15Z

Nice feature to test the examples in the documentation. 😄 There was an use case in the docs that was not in the code, in the file 2010-01-02-creating-a-substitute.markdown.

There was a regression because the new ForPartsOf is based on For<>, and there was a new check to ensure that the class implemented the interface. It did not break any of the existing tests but it did with those on the documentation.

I have solved it and copied the test into the solution.

Now ForPartsOf<ITestInterface, TestClass> will behave exactly as For<> if TestClass doesn't implement the interface. If you feel the check is necessary, I think I will just add it to the implementation of ForPastOf as the first step. It would be more difficult to check this at a later step.

marcoregueira · 2022-07-22T11:42:21Z

I've found that the use case mentioned in my previous comment and taken from the docs was not working ok for all scenarios.

In the case of partial substitutes, the substitute can't extend the base class, just the interface, since we are substituting non-virtual methods. I think this is an acceptable limitation. In my previous implementation, regretfully, this was happening for all other substitutes. While I believe it is an edge case, it would have meant a breaking change.

The new implementation involves passing a parameter around to know when we are creating a partial substitute or not. I find this a bit inelegant, but it causes less friction with the current code.

I've removed the previous commits and replaced them with a squashed version.

Relevant tests are in
NSubstitute.Acceptance.Specs\SubbingForConcreteTypesAndMultipleInterfaces.cs
and
NSubstitute.Acceptance.Specs\PartialSubs.cs

dtchepak

Sorry for the delay in reviewing this. 😓 🙇

This is a really nice PR, thanks! I'm wondering if, to be extra careful, we should leave the existing Substitute.ForPartsOf unchanged, and expose this functionality through Substitute.ForTypeForwardingTo() or similar? That way changes to the types passed will not result in significant changes to how the resulting substitute behaves. WDYT?

src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs

marcoregueira · 2022-10-15T09:26:50Z

Hi! Thank you for taking the time to review the PR. I'm glad you like it. I will make the amendments you suggested.

marcoregueira · 2022-10-29T10:32:59Z

Hi again

I've added the changes you suggested:

Renamed the method to ForTypeForwardingTo and moved all tests, accordingly, to a new class named TypeForwarding. All changes in PartialSubs have been removed.
Created new specific exceptions a shown in the following image.

Please, kindly let me know if you find something else to improve.

johncrim · 2024-04-10T15:07:13Z

I would love to have this integrated in NSubstitute... @marcoregueira - thank you for your work on this!

NidhalWers · 2024-05-06T14:06:52Z

Thanks @marcoregueira for your solution, it's something I look forward to have in NSubstitute.
@dtchepak Is there any blocker to integrate this ?

dtchepak · 2024-05-08T13:13:04Z

Thanks @marcoregueira for your solution, it's something I look forward to have in NSubstitute. @dtchepak Is there any blocker to integrate this ?

Sorry, this completely slipped off my radar. 😞 🙇

Before I merged I wanted to add some tests to demonstrate equivalence between substitutes created using this method vs. ForPartsOf and manually forwarding to the concrete type for the standard NSub features.

If anyone has time to look at that I would greatly appreciate it, otherwise thanks a lot for the reminder and I'll try to look into this again shortly.

marcoregueira · 2024-05-13T08:47:53Z

@dtchepak,

Thank you for your review.

I've upgraded the code to the latest version and made the necessary updates to the PR. To verify the new feature, please refer to the tests in TypeForwarding.cs.

dtchepak

Looks great. Couple of very minor nitpicks to resolve then I'll merge 👍

dtchepak · 2024-05-21T12:08:43Z

src/NSubstitute/Substitute.cs

@@ -89,4 +90,19 @@ public static T ForPartsOf<T>(params object[] constructorArguments)
        var substituteFactory = SubstitutionContext.Current.SubstituteFactory;
        return (T)substituteFactory.CreatePartial([typeof(T)], constructorArguments);
    }
+
+        public static T ForTypeForwardingTo<T,TClass>(params object[] constructorArguments)


chore: could you please add doc comments here (see ForPartsOf and other public methods for examples).

dtchepak · 2024-05-21T12:09:03Z

src/NSubstitute/Substitute.cs

+        //public static T ForTypeForwardingTo<T, T2, T3>(params object[] constructorArguments)
+        //    where T : class
+        //{
+        //    var substituteFactory = SubstitutionContext.Current.SubstituteFactory;
+        //    return (T)substituteFactory.CreatePartial(new[] { typeof(T), typeof(TClass) }, constructorArguments);
+        //}


chore (nitpick): please remove commented out code.

dtchepak · 2024-05-21T12:09:35Z

src/NSubstitute/Substitute.cs

        var substituteFactory = SubstitutionContext.Current.SubstituteFactory;
        return (T)substituteFactory.CreatePartial([typeof(T)], constructorArguments);
    }
+
+        public static T ForTypeForwardingTo<T,TClass>(params object[] constructorArguments)
+            where T : class
+        {
+            var substituteFactory = SubstitutionContext.Current.SubstituteFactory;
+            return (T)substituteFactory.CreatePartial(new[] { typeof(T), typeof(TClass) }, constructorArguments);
+        }
+


question: not sure if it is just the diff view but the code indenting looks non-standard here?

dtchepak · 2024-05-21T12:10:18Z

src/NSubstitute/Proxies/ProxyFactory.cs

+
+

nitpick: additional blank lines not required

dtchepak · 2024-05-21T12:11:29Z

src/NSubstitute/Core/SubstituteFactory.cs

@@ -1,4 +1,6 @@
+


question (nitpick): several of these changed files have a blank line to added to start. Is this an editorconfig thing or accidental?

…ods or sealed classes implementing an interface. How to use: var substitute = Substitute.ForTypeForwardingTo <ISomeInterface,SomeImplementation>(argsList); In this case, it doesn't matter if methods are virtual or not; it will intercept all calls since we will be working with an interface all the time. For Limitations: Overriding virtual methods effectively replaces its implementation both for internal and external calls. With this implementation Nsubstitute will only intercept calls made by client classes using the interface. Calls made from inside the object itself to it's own method, will hit the actual implementation.

marcoregueira · 2024-08-07T14:51:16Z

Hi @dtchepak

I found some time to address the issues you raised. Thank you for reviewing them.

I've included the documentation section you requested. Please let me know if the wording needs further adjustment.

Feel free to let me know if you need any more changes!

Changes:

Updated namespaces and adjusted indentation to align with the new file structure.
Fixed changes in the includes section.
Simplified collection initializations using collection expressions.
Addressed primary constructor warnings.
Removed unnecessary comments.
Added documentation coments.

dtchepak · 2024-08-11T04:09:24Z

src/NSubstitute/Exceptions/TypeForwardingException.cs

+public sealed class CanNotForwardCallsToClassNotImplementingInterfaceException(Type type) : TypeForwardingException(DescribeProblem(type))
+{
+    private static string DescribeProblem(Type type)
+    {
+        return string.Format("The provided class '{0}' doesn't implement all requested interfaces. ", type.Name);
+    }


suggestion (non-blocking): for a future change, I think it would be helpful for this to explain which of the requested interfaces has not been implemented. If we see "Class 'A' does not implement all requested interfaces. To support call forwarding 'A' must implement 'B', 'C', and 'D'." then it gives us a good hint on how to fix the exception.

dtchepak · 2024-08-11T04:10:04Z

src/NSubstitute/Exceptions/TypeForwardingException.cs

+{
+    private static string DescribeProblem(Type type)
+    {
+        return string.Format("The provided class '{0}' is abstract. ", type.Name);


nitpick (non-blocking): trailing space: "...abstract . " vs "...abstract ."..

dtchepak · 2024-08-11T04:12:18Z

src/NSubstitute/Proxies/CastleDynamicProxy/CastleInvocationMapper.cs

-            !castleInvocation.MethodInvocationTarget.IsFinal)
+            !castleInvocation.MethodInvocationTarget.IsAbstract)


question: dropping "not final" constraint here is required to support call forwarding?

dtchepak · 2024-08-11T04:13:19Z

src/NSubstitute/Substitute.cs

+    /// <summary>
+    /// Creates a proxy for a class that implements an interface, forwarding methods and properties to an instance of the class, effectively mimicking a real instance.
+    /// Both the interface and the class must be provided as parameters.
+    /// The proxy will log calls made to the interface members and delegate them to an instance of the class. Specific members can be substituted 


nitpick (non-blocking): "will log calls" -> "will intercept calls"

src/NSubstitute/Substitute.cs

dtchepak · 2024-08-11T04:34:03Z

Thanks @marcoregueira !

marcoregueira mentioned this pull request Jul 21, 2022

Improve ForPartsOf to use interface and implementation class instead of class only #625

Closed

marcoregueira force-pushed the partsof_extension branch from 302440e to a04dc32 Compare July 22, 2022 11:13

dtchepak reviewed Oct 15, 2022

View reviewed changes

src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs Outdated Show resolved Hide resolved

src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs Outdated Show resolved Hide resolved

dtchepak mentioned this pull request Jan 25, 2023

Update target frameworks for .NET 6, .NET Framework 4.6.2, .NET Standard 2.0 #710

Merged

marcoregueira force-pushed the partsof_extension branch from e88249f to 05cb82c Compare May 13, 2024 08:40

dtchepak reviewed May 21, 2024

View reviewed changes

marcoregueira force-pushed the partsof_extension branch from 266ace9 to eed5290 Compare August 7, 2024 14:30

dtchepak approved these changes Aug 11, 2024

View reviewed changes

dtchepak merged commit 4d258a2 into nsubstitute:main Aug 11, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Extend PartsOf to mock non-virtual methods implementing an i… #700

Feature: Extend PartsOf to mock non-virtual methods implementing an i… #700

marcoregueira commented Jul 21, 2022 •

edited

Loading

marcoregueira commented Jul 21, 2022

marcoregueira commented Jul 22, 2022

dtchepak left a comment

marcoregueira commented Oct 15, 2022 via email

marcoregueira commented Oct 29, 2022

johncrim commented Apr 10, 2024

NidhalWers commented May 6, 2024

dtchepak commented May 8, 2024

marcoregueira commented May 13, 2024

dtchepak left a comment

dtchepak May 21, 2024

dtchepak May 21, 2024

dtchepak May 21, 2024

dtchepak May 21, 2024

dtchepak May 21, 2024

marcoregueira commented Aug 7, 2024

dtchepak Aug 11, 2024

dtchepak Aug 11, 2024

dtchepak Aug 11, 2024

dtchepak Aug 11, 2024

dtchepak commented Aug 11, 2024

		!castleInvocation.MethodInvocationTarget.IsFinal)
		!castleInvocation.MethodInvocationTarget.IsAbstract)

Feature: Extend PartsOf to mock non-virtual methods implementing an i… #700

Feature: Extend PartsOf to mock non-virtual methods implementing an i… #700

Conversation

marcoregueira commented Jul 21, 2022 • edited Loading

Obvious limitations:

Possible improvements

marcoregueira commented Jul 21, 2022

marcoregueira commented Jul 22, 2022

dtchepak left a comment

Choose a reason for hiding this comment

marcoregueira commented Oct 15, 2022 via email

marcoregueira commented Oct 29, 2022

johncrim commented Apr 10, 2024

NidhalWers commented May 6, 2024

dtchepak commented May 8, 2024

marcoregueira commented May 13, 2024

dtchepak 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoregueira commented Aug 7, 2024

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

dtchepak commented Aug 11, 2024

marcoregueira commented Jul 21, 2022 •

edited

Loading