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

[marcoregueira]

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]

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]

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?

[marcoregueira]

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]

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]

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

[NidhalWers]

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

[dtchepak]

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]

@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.