Way to call base implementation when overriding a method on a partial substitute

[logiclrd]

Is your feature request related to a problem? Please describe.
I have a situation with a partial mock where I want to make use of the existing implementation, but intercept the call with my own override to catch its return value. That value is tied into the underlying mocked object's state, and it is called from within the SUT.

If I were creating the partial mock manually, then I would be able to write something like:

public override Context BeginScope()
{
    var scope = base.BeginScope();

    StoreValueForTestAssertions(scope);

    return scope;
}

I haven't been able to find an official way to do this with a mock created by Substitute.ForPartsOf<...>.

Describe the solution you'd like
I'd like an overload of the .Returns method that takes a callback delegate with an object richer than CallInfo that provides the ability to call the base implementation. Obviously, this method would result in an error if used on a full mock where there is no base implementation, but it would enable the above scenario via NSubstitute.

Describe alternatives you've considered
I investigated the possibility of creating this mock manually, but the Context object that the base BeginScope returns needs to be known internally to the object being mocked for subsequent calls.

I then dug into how NSubstitute handles call routing internally and came up with an alternative that is technically all based on public API, but feels really dirty & internally:

        class CapturingCallInfoFactory : ICallInfoFactory
        {
            ICallInfoFactory _realFactory;
            Dictionary<CallInfo, ICall> _callByCallInfo = new Dictionary<CallInfo, ICall>();

            public CapturingCallInfoFactory(ICallInfoFactory realFactory)
            {
                _realFactory = realFactory;
            }

            public CallInfo Create(ICall call)
            {
                var callInfo = _realFactory.Create(call);

                _callByCallInfo[callInfo] = call;

                return callInfo;
            }

            public ICall GetCallByCallInfo(CallInfo callInfo)
            {
                _callByCallInfo.TryGetValue(callInfo, out var call);
                return call;
            }
        }

        [Test]
        void Test()
        {
            // Arrange
            // (..)
            var container = (NSubContainer)NSubstituteDefaultFactory.DefaultContainer;

            var realCallInfoFactory = container.Resolve<ICallInfoFactory>();
            var capturingCallInfoFactory = new CapturingCallInfoFactory(realCallInfoFactory);

            container.Register<ICallInfoFactory>(_ => capturingCallInfoFactory, NSubLifetime.Singleton);

            try
            {
                var mockClient = Substitute.ForPartsOf<Client>();

                Context stashedReturnValue = null;

                mockClient.Configure().BeginScope().Returns(
                    (callInfo) =>
                    {
                        if (stashedReturnValue != null)
                            throw new Exception("This test does not account for multiple calls to this method");

                        var call = capturingCallInfoFactory.GetCallByCallInfo(callInfo);

                        stashedReturnValue = call.TryCallBase().Fold(
                            () => throw new Exception("Could not obtain a value from the base BeginScope implementation"),
                            (value) => (Context)value);

                        Assert.IsNotNull(stashedReturnValue);

                        return stashedReturnValue;
                    });

                var sut = new Service(mockClient);

                // Act
                sut.DoTheThingThatIndirectlyCallsClientBeginScope();

                // Assert
                Assert.IsNotNull(stashedReturnValue);
                AssertThingsWith(stashedReturnValue);
            }
            finally
            {
                // Restore old registration.
                container.RegisterPerScope<ICallInfoFactory, CallInfoFactory>();
            }
        }

The above is pseudocode, so there may be typos, but I have verified that this exact pattern works in actual implementation.

[dtchepak]

Hi @logiclrd , thanks for the excellent issue report. 👍

Could you please provide some more context about the BeginScope() example? If you need to use StoreValueForTestAssertions and assert on that result, is there any reason you can't call StoreValueForTestAssertions(sub.BeginScope()) from the test code, rather than in a Returns callback?

The feature itself is reasonable straightforward to implement, but we like to be careful adding anything to the main API as sometimes these things have unintended consequences. If you can provide more context about the BeginScope example and/or any other examples that can give us good justification for adding.

[logiclrd]

Sorry, yeah, I should have been more explicit.

The subject under test has code like this:

using (var context = client.BeginScope())
{
  context.Configure(..);

  DoOtherThings(client);
}

The client object is ancillary to the test, but is tightly tied to the way the subject is functioning.

The return value of BeginScope is an IDisposable object that participates in its own tracking within client, and in order to prevent callers from interfering with it, it doesn't expose a lot of details -- it doesn't have a public constructor, for instance. It isn't possible to directly mock this type, even as a partial mock.

The objective I have been trying to achieve is to have the test capture the value of scope that the subject is obtaining, so that certain assertions can be done after the operation completes. But, because the scope can't be directly constructed or mocked, I can't just directly mock BeginScope itself, and even if I did, my mock context wouldn't be properly registered in the internal state of client.

So, the path I was trying to follow was a partial mock of client where my mock code would execute when BeginScope was called but my mock implementation could call the real BeginScope and then have access to its return value before passing it along to the calling code inside the subject.

It's not always obvious if I overlook something in an explanation so let me know if I've missed anything :-)

[dtchepak]

Thanks @logiclrd. Is it possible to show an example SUT and test of this scenario? Or is it too involved to reduce to a small case?

@zvirja @alexandrnikitin Do you see any issues with adding something like in dtchepak@b7909cd?

[logiclrd]

This code is untested, in part because I wrote it right in the text input on the GitHub issue page, but also because it makes use of both the CallBase and WhenCalledDoNotCallBase features currently being proposed that don't exist yet.

// This class is in an external library that is unlikely to be changed to satisfy a test.
public class Data
{
  public bool IsCorrectlyUpdated = false;
}

// This class is in an external library that is unlikely to be changed to satisfy a test.
public class Client
{
  List<Context> _scopes = new List<Context>();

  // A nested class for member access reasons.
  public class Context : IDisposable
  {
    Client _owner;

    public Context(Client owner) { _owner = owner; }
    public void Dispose() { _owner.EndScope(this); }
  }

  public virtual Context BeginScope()
  {
    var scope = new Context(this);
    _scopes.Add(scope);
    return scope;
  }

  void EndScope(Context context)
  {
    _scopes.Remove(scope);
  }

  public bool IsActiveScope(Context context) => _scopes.Contains(context);

  public virtual Data GetData()
  {
    // This represents a call to an external service.
    return new Data();
  }

  public virtual void UpdateData(Data data)
  {
    // A real client implementation would do something here, and its behaviour would depend on active scopes.
  }
}

public class TestService
{
  Client _client;

  public TestService(Client client) { _client = client; }

  public void PerformAction()
  {
    using (var scope = _client.BeginScope())
    {
      var data = _client.GetData();

      data.IsCorrectlyUpdated = true;

      _client.UpdateData(data);
    }
  }
}

public class TestServiceTests
{
  [Test]
  public void PerformAction_should_get_data_within_scope()
  {
    // Arrange
    var mockClient = Substitute.ForPartsOf<Client>();

    var scopes = new List<Context>();

    mockClient.Configure().BeginScope().Returns(
      (callInfo) =>
      {
        var scope = callInfo.CallBase<Context>();
        scopes.Add(scope);
        return scope;
      });

    bool haveCorrectlyScopedGetData = false;

    mockClient.Configure().GetData().Returns(
      (callInfo) =>
      {
        haveCorrectlyScopedGetData = scopes.Any(context => mockClient.IsActiveScope(context));
        return callInfo.CallBase<object>();
      });

    Data capturedUpdatedData = null;

    mockClient.When(c => c.UpdateData(Arg.Any<object>()))
      .DoNotCallBase()
      .DoInstead(
        (callInfo) =>
        {
          capturedUpdatedData = callInfo.Arg<Data>();
        });

    var sut = new TestService(mockClient);

    // Act
    sut.PerformAction();

    // Assert
    haveCorrectlyScopedGetData.Should().BeTrue();
    capturedUpdatedData.Should().NotBeNull();
    capturedUpdatedData.IsCorrectlyUpdated.Should().BeTrue();
  }
}

[zvirja]

@zvirja @alexandrnikitin Do you see any issues with adding something like in dtchepak/NSubstitute@b7909cd?

I like the idea of extending CallInfo with the API to have base result. Even though it's a pretty rare case, it doesn't hurt and is very easy to implement (exactly the motivation for guy who introduced null pointer 😄).

I see the following improvements we can make:

• CallInfo is also re-used for When(...), where it makes no sense.
• Returns<T> knows type of the result, so instead of non-typed object we could directly cast result to T under the hood.

I would introduce CallInfoWithBase<T> (maybe with better naming), which contains .CallBase() -> T and give only in Returns<T>() callback. For the When() I would still use CallInfo without ability to invoke base.

Should we also rename CallBase() to BaseResult() or similar? To name API more from the result perspective, rather than from side-effect 🤔 It will also align it with Arg<T>() we already have (as we don't name it GetArg()).

[logiclrd]

@zvirja I have a couple of responses from my position here on the outside :-)

1. Why does CallInfo not make sense for .When(..).Do(..)? That's the only way to attach to void methods, which still have parameters and side-effects. There is another open issue right now on this exact topic, suggesting an enhancement to .When(..).Do(..) to make partial mocking easier, avoiding the implicit call to the base implementation when you want to make it yourself implicitly or elide it altogether. (Registering actions does not suppress base method call on partial mocks #623)

2. I don't think the rename makes sense, because methods do more than produce values, and if you're using a partial mock, it's because you want the methods' side-effects. .BaseResult() implies that you don't know when the base method is actually going to execute, perhaps even suggesting that it might be cached from one invocation to the next. I personally think the method being an action that expresses that it is going to do something at the time it is called makes more sense in this case.

[alexandrnikitin]

@logiclrd Thank you for the great explanation. That's a test scenario I see time to time. I'll try to explain my view on it.
Let's abstract it. There are two different ways to verify code: state based and beharior based testing. In state bassed testing, we test output returned from SUT for given input. Ideally we treat a SUT as a black box and see only its public APIs (ctr, method, args). In behavior based testing we verify interactions with dependencies that there are outgoing calls from SUT/blackbox. Although, In behavior based testing it's harder to treat a SUT as a black box, we still can abstract and imagine that we don't know what's inside.
The main idea of the above is to keep things simple and easy to reason about. I personally would be always for a simpler way.

Now to your example, the "scope" notion is internal to the SUT, it is not exposed from SUT in anyway. We don't see it if we treat SUT as a black box, it's on different level of abstraction. The test code you provided looks over complicated to me.
I see few options that may help you:

• Do not test/ assert scope. In many cases it's fine to keep some parts untested especially internal ones.
• Test scope on its level i.e. Client level. It may be enough and its consumers don't need to verify that logic again.
• Debug.Assert in your code can be helpful if it's 3rd party dependency you don't control.
• Custom fake facade that wraps real client and its responses. Basically this is what you want to achieve with NSub. Yes, it needs some code because it's a complex case. It's fine to have it if there are no other options.
• Redesign the SUT class (or introduce another intermediate one) and "lift" the scope notion. Yes, it is usually risky, requires effort and undesired in general to change code design because of test. But if verification is important then it's a viable option.

To Nsub. In general, I see NSubstitute as a helper (syntax sugar) that helps to reduce boiler plate code, helps to keep things simpler and readable. I don't think it should be "jack of all trades" tool that can do some complex logic inside subs. It may provide some extensibility points for its clients though. Regarding callInfo.CallBase I don't have strong opinion. I think the root of the issue is in the code not mocking library.

[logiclrd]

Some further context on this -- we have a service that, among other things, includes two methods that drive a load-modify-store model for working with the data. But, the "load" part of this is also a general-purpose data retrieval method; there's no automatic assumption that if you use the "load" method then you definitely will be making changes and passing them back up. Because of this, the "load" method by default reads data from a read-only secondary instance of the database (Application Intent=ReadOnly). The problem that arises, then, is that if you are planning to do the modify-store part of the process as well, you have just loaded the data on which you are basing your changes from a different instance of the database, and being a secondary instance, it can be out of date (typically by just a few seconds) with respect to the primary.

In many cases, this delay on the secondary isn't an issue, because in practice nothing has modified the data and it isn't actually out-of-date -- or if it is, it's out-of-date with irrelevant changes. However, we have a persistent service that responds to notification messages on a service bus regarding data changes, and we found that it would regularly read out-of-date data because the notification, sent after a change was made to the primary database instance, would arrive and get processed before the replication from the primary instance to the secondary instance had completed. For this reason, we introduced the concept of a "service client context" scoping system into the client. This scope allows headers to be assigned as ambient state that then apply to all requests made during that scope, and one of those headers can tell the service, "You need to do this read from the read/write primary, even though it is on its own a read-only operation."

There is another service which has a method that updates data, and this update can affect links with data in this service with the load-modify-store model. During a major refactor, we had to temporarily comment out that second level update because we were migrating to a new platform that didn't support the communication technology the other service used. This meant potentially some updates would get missed, an unavoidable temporary situation until the downstream service also got migrated. It has since also been migrated, and so now we're reintroducing that second level update, converting the calling code to use the new client library. (The new client library exposes the same contract interface but handles the scoping completely differently; the old implementation did not wrap the scoping with any abstraction.) It is this change that I am unit testing.

A key part of this unit test, then, is ensuring that the SUT, which is doing a load-modify-store against the other service, is in fact specifying the correct "intent" in the "load" phase via its scope, to ensure that the data upon which it is basing its changes comes from the read/write primary instance and not the potentially-out-of-date secondary. This is why the test has to care that the service client object, downstream of the SUT, sees an appropriate BeginScope.

Ultimately I may be able to achieve this test by getting my fingers into the client object (which isn't part of the same project) and making its concept of scoping fully abstracted. This, then, would allow the use of full substitutes for the entire client layer downstream of the SUT. But, the current implementation exposes the scope as a "private" object that a caller cannot instantiate, because its lifetime, controlled by IDisposable, ties back into internals of the client, to ensure that the scope gets torn down properly when the context is disposed. This design decision means that it is impossible to completely mock out BeginScope; even if it were possible to instantiate the scope from the test, it wouldn't be tied into the internals of the client properly.

[logiclrd]

(I call it a "private" object in the sense of it having no public constructors; the type itself is not actually marked private :-P)

[alexandrnikitin]

@logiclrd Thank you for the great explanation. It is more clear now.
Another idea I can suggest (and often see in systems that have important branching based on internal flag) is to "lift" that knowledge to a type. Say instead of one Client with a flag inside you have ReadOnlyClient and ReadWriteClient (or Client and ReadWriteClient) Then declare that it's used on SUT level (e.g. in ctr) In that case you test ReadWriteClient on its level and that's it. It doesn't need to be inheritance nor to be in the clients package, there are options how that can be implemented. You can read more about the idea here Type Driven Development by Mark Seemann

[logiclrd]

Thanks for the suggestion :-) So, the consumer using the SUT would supply it with the client appropriate to what it wanted to do, and if you tried to invoke the load-modify-store operation and you had supplied it with a ReadOnlyClient, that would result in a runtime error? This sounds like an interesting concept, though perhaps for new development -- I'm not convinced it would be worth the effort to refactor this existing code, which has a long history and many consumers, but which wasn't written following the best of practices and is only now getting important refactoring and automated testing. :-)

[alexandrnikitin]

So, the consumer using the SUT would supply it with the client appropriate to what it wanted to do

Yes, correct, and the best part of Type DD is that it's compile time, meaning that there won't be runtime errors.

[logiclrd]

Oh I see, you're imagining a SUT that can only work with a ReadWriteClient. So, if the SUT wants to do some operations read-only and some with a load-modify-store pattern, then it has to be supplied both types of client? Off the top of my head, I don't like that because there's nothing guaranteeing you're supplying two instances that are configured to point at the same endpoint. On the other hand, if ReadWriteClient were a subclass of ReadOnlyClient, then one implementation could supply both but you'd need different methods for a normal "Load" as opposed to a "Load with intent to Store". Hmm...

[dtchepak]

I like the idea of extending CallInfo with the API to have base result. Even though it's a pretty rare case, it doesn't hurt and is very easy to implement (exactly the motivation for guy who introduced null pointer 😄).

I see the following improvements we can make:

• CallInfo is also re-used for When(...), where it makes no sense.
• Returns<T> knows type of the result, so instead of non-typed object we could directly cast result to T under the hood.

I would introduce CallInfoWithBase<T> (maybe with better naming), which contains .CallBase() -> T and give only in Returns<T>() callback. For the When() I would still use CallInfo without ability to invoke base.

Thanks for these suggestions @zvirja . I tried pushing the type through Returns<T> but it ends up being a huge change. I think the simpler version is better for now. It should still work to have BaseResult() in When..Do; people can store the base result if they want from the Do block (CallInfo isn't available from the When, just from the Do part).

[logiclrd]

Just to be clear, there is no guarantee that a base result even exists in a .When().Do() because the method probably has a void return type. What's really wanted is the action of calling the base implementation at a specific time and with specific parameters (perhaps different from the original call into the mock). In some cases, people might want to call it more than once. I really think it's better modeled as an action than a value.