SetupAllProperties causes mocks to become race-prone

[estrizhok]

Moq version: 4.16.1
Target Framework: Probably any, tested on net48 and net6.0

A test to reproduce the problem:

using System.Threading;
using System.Threading.Tasks;
using Moq;
using NUnit.Framework;

namespace NUnitTestProject2
{
  public interface IEntity
  {
    int Property { get; }
  }

  public class Tests
  {
    [Test]
    [Repeat(10000)]
    public void Using_SetupAllProperties_should_not_break_thread_safety()
    {
      TestContext.Progress.WriteLine("========== Test Started ===========");

      var barrier = new AutoResetEvent(false);
      var gate = new AutoResetEvent(false);
      var mock = new Mock<IEntity>();

      // Commenting the following line makes the test pass
      mock.SetupAllProperties();

      var task = Task.Factory.StartNew(() =>
      {
        barrier.Set();

        while (!gate.WaitOne(0))
        {
          TestContext.Progress.WriteLine($"Property = {mock.Object.Property}");
        }
      });

      barrier.WaitOne();
      gate.Set();

      TestContext.Progress.WriteLine("Changing Property to 5");
      mock.Setup(x => x.Property).Returns(5);
      TestContext.Progress.WriteLine("Changed Property to 5");

      Assert.That(mock.Object.Property, Is.EqualTo(5), () => "Property is not equal to 5");

      task.Wait();

      TestContext.Progress.WriteLine("========== Test Finished ===========");
    }
  }
}

[stakx]

Thanks @estrizhok for reporting this problem.

I believe the PR linked above will take care of this.

I would like to add your repro code to the test suite, but unfortunately the [Repeat(10000)] doesn't translate well to xUnit & Visual Studio's Test Explorer. The latter will display 10,000 separate tests in the test list, and test execution gets a lot slower. So I haven't added this to the test suite for now.

That being said, I have run the above code. It fails for the current main branch (where the PR isn't merged yet), but succeeds when the changes in the PR are applied.

[estrizhok]

Thanks @stakx for reacting quickly on it.

I don't see much problem in replacing [Repeat(10000)] with a loop inside the test to avoid creation of excessive tests. Also, this number could probably be refined to a much smaller value or even to 1 for the purposes of CI.

Thanks again!