nsubstitute / NSubstitute

A friendly substitute for .NET mocking libraries.
https://nsubstitute.github.io
Other
2.67k stars 263 forks source link

NSubstitute makes it impossible to unit test memory leaks #771

Open albyrock87 opened 9 months ago

albyrock87 commented 9 months ago

Describe the bug I'm trying to test memory leaks on my library, but NSubstitute retains reference to call arguments (even after ClearReceivedCalls).

This makes it impossible to test memory leaks which in some part require mocked interfaces.

To Reproduce

The following unit test fails.

public class NSubstituteLeakTests
{
    public class Foo;

    public interface IInterface
    {
        public void Method(Foo foo);
    }

    private readonly IInterface _interfaceMock = Substitute.For<IInterface>();

    [Fact(DisplayName = "NSubstitute does not leak")]
    public async Task NSubstituteDoesNotLeak()
    {
        WeakReference<Foo> weakFoo;
        {
            var foo = new Foo();
            weakFoo = new WeakReference<Foo>(foo);
            _interfaceMock.Method(foo);
            _interfaceMock.ClearReceivedCalls();
        }

        await Task.Yield();
        GC.Collect();
        GC.WaitForPendingFinalizers();

        weakFoo.TryGetTarget(out _).Should().BeFalse();
    }
}

This is what retains the reference to the object. image

Expected behaviour Unit test above passes.

Environment:

alexandrnikitin commented 9 months ago

@albyrock87 Thank you for the issue and great repro! I tried to remove NSubstitute related code from your test but it still fails. I don't think it's NSubstitute related.

public class NSubstituteLeakTests
{
    public class Foo;

    [Fact(DisplayName = "NSubstitute does not leak")]
    public async Task NSubstituteDoesNotLeak()
    {
        WeakReference<Foo> weakFoo;
        {
            var foo = new Foo();
            weakFoo = new WeakReference<Foo>(foo);
        }

        await Task.Yield();
        GC.Collect();
        GC.WaitForPendingFinalizers();

        weakFoo.TryGetTarget(out _).Should().BeFalse();
    }
}
albyrock87 commented 9 months ago

@alexandrnikitin I was pretty sure about the issue so I double checked again, and as you can see from this screenshot, the test without NSubstitute is working (see the green mark on the left side)

image

This makes me think we're running the unit test on different frameworks.

My XUnit test project is using net8.0 as TargetFramework and I have .NET 8.0.101 installed.

    <PackageReference Include="xunit" Version="2.4.2" />
    <PackageReference Include="xunit.runner.visualstudio" PrivateAssets="all" Version="2.4.5">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
alexandrnikitin commented 9 months ago

Here's a test that fails for me on Windows and .NET 8 https://github.com/nsubstitute/NSubstitute/pull/775 Not sure what is the difference in our setup but I don't think WeakReference is deterministic enough for memory leak tests.

albyrock87 commented 9 months ago

@alexandrnikitin this is what Microsoft does to test leaks

Anyway let's try with this:

public class NSubstituteLeakTests
{
    public class Foo;

    public interface IInterface
    {
        public void Method(Foo foo);
    }

    // private readonly IInterface _interfaceMock = Substitute.For<IInterface>();
    [Fact(DisplayName = "NSubstitute does not leak")]
    public async Task NSubstituteDoesNotLeak()
    {
        WeakReference<Foo> weakFoo;
        {
            var foo = new Foo();
            weakFoo = new WeakReference<Foo>(foo);

            // _interfaceMock.Method(foo);
            // _interfaceMock.ClearReceivedCalls();
        }

        await GcCollect();

        weakFoo.TryGetTarget(out _).Should().BeFalse();
    }

    private static async Task GcCollect()
    {
        for (var i = 0; i < 3; i++)
        {
            await Task.Yield();
            GC.Collect();
            GC.WaitForPendingFinalizers();
        }
    }
}