NSubstitute icon indicating copy to clipboard operation
NSubstitute copied to clipboard

Recieved does not seem to work for mocked output parameters

Open thehiflyer opened this issue 2 years ago • 8 comments

Question I've been struggling with a test that needs to verify calls to an out param passed to my SUT from a third party API. It reports success for any number passed to Recieved(). Am I doing something wrong here or is this a bug?

public class Sut
{
    private Dependency dep;

    public Sut(Dependency dep)
    {
        this.dep = dep;
    }

    public void DoStuff()
    {
        dep.Method(out var outParam);
        outParam.Dispose();
    }
}

public interface Dependency
{
    void Method(out OutParam outParam);
}

public class OutParam : IDisposable
{
    public void Dispose()
    {
    }
}

[Test]
public void Isolated()
{
    OutParam outParam = null;
    Dependency dep = Substitute.For<Dependency>();
    Sut sut = new Sut(dep);

    dep.When(dependency => dependency.Method(out Arg.Any<OutParam>())).
        Do(info =>
        {
            outParam = Substitute.For<OutParam>();
            info[0] = outParam;
        } );

    sut.DoStuff();

    outParam.Received(42).Dispose();
}

thehiflyer avatar Jul 01 '22 13:07 thehiflyer

You are trying to check received calls for Dispose method of OutParam type - as you did Substitute.For<OutParam>(). As Dispose is not virtual method and OutParam is not interface, NSubstitute is not able to correctly check received calls. As you cant do

outParam = Substitute.For<IDisposable>();

due to out param type constraint, you have to help NSubstitute to understand that you are interested in IDisposable interface checks

outParam = Substitute.For<OutParam, IDisposable>();

tpodolak avatar Jul 01 '22 14:07 tpodolak

Your explanation makes sense so I tried to add the IDisposable type to the Substitute.For but it still doesn't fail the test. Does it work for you?

thehiflyer avatar Jul 04 '22 08:07 thehiflyer

Sorry @thehiflyer, I've accidentally marked Dispose method as virtual thats why it worked for me. I am not sure what is the best way to approach this issue(without marking unnecessary methods as virtual). @dtchepak any thoughts?

tpodolak avatar Jul 04 '22 22:07 tpodolak

I'm not sure on a neat way to approach this. Dispose [should not be virtual])https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose), so NSubstitute should not be intercepting it. I can think of a couple of less-than-ideal options:

  • Make OutParam.Dispose set an Output.IsDisposed field/property and check that with a normal assertion. (e.g. independent of NSubstitute).
  • Depending on how OutParam needs to be used, change the param type to IDisposable: `Dependency.Method(out IDisposable outParam) and mock that.
  • Make OutParam an interface and either substitute the output param or use a real class that sets a flag as per the first option.

dtchepak avatar Jul 10 '22 07:07 dtchepak

Personally I would suggest a mix of the recommendations. I would change the OutParam to follow the Dispose pattern. Then your OutParam has a nice virtual Dispose method.

Then you can add a manually crafted OutParamSubstitute in your test project with the IsDisposed property like suggested before.

GeraldLx avatar Jul 11 '22 16:07 GeraldLx

Another option could be to avoid the problem entirely -- if Dependency creates the OutParam, make it responsible for handling disposing it:

public interface Dependency
{
    T WithOutParam(Func<OutParam, T> f);
}

public class SampleDep : Dependency {
  public T WithOutParam<T>(Func<OutParam, T> f) {
    using (var outParam = new OutParam()) {
        return f(outParam);
    }
  }
}

May not work for you depending on how OutParam needs to be used, and just moves the testing of this dispose call elsewhere, but may make consumers easier to test. :shrug:

dtchepak avatar Jul 12 '22 00:07 dtchepak

I should have been more clear in the naming of my example. The Dependecy is actually ThirdPartyDependency and OutParam is also something that they provide so I can't change that easily. (at the very least a PR to them and convincing them it should be merged)

thehiflyer avatar Jul 12 '22 07:07 thehiflyer

@thehiflyer Do you have NSubstitute.Analyzers added to your test project? That can help identify what is mockable and what is not.

If you can't modify Dependency then it may not be possible to mock this type effectively. (As an aside, here's my thoughts from ages ago on mocking types we don't own).

dtchepak avatar Jul 17 '22 04:07 dtchepak

I assume your question has been answered, if not, please let us know!

304NotModified avatar Apr 29 '24 12:04 304NotModified