Received/DidNotReceive Both Pass On Non-Virtual Members

[vector-man]

Hi, I'm using ICloneable in my unit tests, and I'm getting false positives. Any help is appreciated. I'm using the latest NSubstitute from NuGet.

    // Both parent and child implement ICloneable, Parent clones child too.
    [Test()]
    [Category("Cloning")]
    public void Parent_WhenCloneCalled_ClonesChild()
    {
        Child child = Substitute.For<Child>();

        Parent parent = new Parent()
        {
            Child = child,
        };
        
        parent.Clone();

        // One of these should fail (in this case DidNotReceive) but both pass.
        child.DidNotReceive().Clone();
        child.Received().Clone();
    }

[alexandrnikitin]

Thank you for reporting that. NSub uses Castle.Core library which can proxy/substitute only virtual members. Received() and DidNotReceive() don't "see" the Clone() call because it's not virtual. That's the reason why they don't fail. Please mark it as virtual. Here's a simple example:

    public class Issue275
    {
        [Test]
        public void Parent_WhenCloneCalled_ClonesChild()
        {
            Child child = Substitute.For<Child>();

            Parent parent = new Parent()
            {
                Child = child,
            };

            parent.Clone();

            child.Received().Clone();
        }
    }

    public class Parent : ICloneable
    {
        public Child Child { get; set; }
        public virtual object Clone()
        {
            Child.Clone();
            return null;
        }
    }

    public class Child : ICloneable
    {
        public virtual object Clone()
        {
            return null;
        }
    }

[vector-man]

Thanks for the quick reply. I guess I should always substitute for interfaces then. I missed that note in the docs!

[vector-man]

Do you think it makes more sense if they both fail in this case? That would let the developer know to check if they are virtual. It would make it easier to catch non-virtual methods.

[alexandrnikitin]

I doubt that we need it. We have the following lines:

1        child.DidNotReceive().Clone();
2        child.Received().Clone();

At first, at line 1 NSub's DidNotReceive() doesn't "see" that the Clone() method was called as it's not virtual. So that we cannot react after this line 1. What we can do is to analyse at line 2 that nothing was called after DidNotReceive() and inform the user. It won't be an exception because we won't go with breaking change. I see it as a special "debug" mode that you can put NSub to analyse unclear behavior like this one. But that's huge piece of work...

[dtchepak]

@alexandrnikitin I think if NSub receives two Received() (or related) calls then we should throw to try to pick up these errors earlier. So these should fail:

child.Received().Received().Blah();
child.Received().DidNotReceive().Blah();
// etc

That might help for this particular case?

[alexandrnikitin]

Yes, that one is the same as the above lines from NSub perspective, it will help in this particular case.

[vector-man]

How about this? child.MustBeVirtual().Received().Clone() // Throws exception, because it's not virtual. child.MustBeVirtual().DidNotReceive().Clone() // Throws exception, because it's not virtual. child.MustBeVirtual().DidNotReceive().AnyNonVirtualMethiod() // Throws exception, because it's not virtual.

Something like that?

[dtchepak]

Hi @vector-man, I don't think this can work. MustBeVirtual() only has a reference to child, not any of the following calls. Later calls won't go through NSubstitute logic as they are non-virtual and therefore won't be routed through NSubstitute. (Happy to be proven wrong! :) )

[alexandrnikitin]

Another option that will work is to use Expressions. There's a PR already #72