Moq4: Property setups are ignored on mocks instantiated using `Mock.Of`

[stakx]

This report goes back to a Gitter chat post by @bluechrism on Sep 3 2020:

[...] can someone explain this difference between new Mock<T> and using Mock.Of<T> with Mock.Get to set up a property: Again, both the Tests would pass with Mock 4.7.8 but the second now fails with 4.14.5.

Moq 4.12.0 introduced a behavioral change that leads to property setups sometimes being ignored on mocks that were created with Mock.Of<> (or on mocks on whom SetupAllProperties was called):

Repro code:

public interface IX
{
    int Property { get; set; }
}

[Fact]
public void Stubbed_property_set_before_SetupGet()
{
    var mock = Mock.Get(Mock.Of<IX>());
    mock.Object.Value = 4;
    mock.SetupGet(m => m.Property).Returns(3);
    Assert.Equal(3, mock.Object.Value);
}

[Fact]
public void Stubbed_property_set_after_SetupGet()
{
    var mock = Mock.Get(Mock.Of<IX>());
    mock.SetupGet(m => m.Property).Returns(3);
    mock.Object.Value = 4;
    Assert.Equal(3, mock.Object.Value);
}

Expected behavior:

Both tests should pass, since one would expect that the SetupGet setup will be invoked.

Actual behavior:

The second test fails.

Likely cause:

826 made stubbed properties fully lazy. (Mock.Of<T> traditionally calls SetupAllProperties, which stubs all properties.) Setups for stubbed properties are now only added once that property is first queried (or being assigned to). That's the reason why the SetupGet setup in the second test gets overridden/shadowed by the stubbed property getter.

Possible resolution:

If we classify the new behavior (where the second test fails since 4.12.0) as a regression or bug, we could only register stubbed properties' lazy accessor setups if there isn't already a pre-existing setup for that accessor.

/cc @bluechrism @lepijohnny

[lepijohnny]

Hi @stakx,

the proposal makes sense. However, I have 2 questions:

1. Would the change be as simple as adding, and respective setter?
   if (ProxyFactory.Instance.IsMethodVisible(getter, out _) && !mock.MutableSetups.Any(setup => setup.Method.Equals(getter))) at https://github.com/moq/moq4/blob/master/src/Moq/Interception/InterceptionAspects.cs#L227

2. The other option, although might be a bit hacky, is to add method AddStub to the SetupCollection and add stubs always to the end so they have less priority than normal setups... There is one unit test failing but it is questionable if it is a proper test, I think it is low risk to alter that one. (failing test)

[stakx]

@lepijohnny:

(2) is a no-go because we should be moving away from different setup types and towards a single unified setup type. (That's what my draft PR for Behavior-based setups is about.)

(1) looks about right, although it's possible that there are corner cases that might complicate things. I cannot remember from memory right now but we'd possibly have to consider at least (a) method overrides, (b) indexers, and (c) setups for set accessors with a fixed target value.

Re: (a) the possible issue is that .Method.Equals(...) might be too strict. Haven't thought about that in-depth yet.

Re: (b) and (c), the trouble might be us adding a very non-specific setup with It.IsAny<> for the parameters (target value or indexes), while more specific setups might already have been made; our less specific setup would then override the pre-existing, more specific one, which would be bad. The good news is that IIRC indexers likely aren't stubbed, so we might only have to deal with (c).

[lepijohnny]

@stakx

(a) (b) the whole block is already wrapped by IsPropertyAccessor so don't expect the method or indexer to end up there at all.
(c) This should be covered by this test case? (currently failing)

[Fact]
public void Stubbed_property_set_with_fixed_target_value()
{
    var mock = Mock.Get(Mock.Of<IFoo>());
    mock.SetupSet(m => m.ValueProperty = 6);
    mock.Object.ValueProperty = 4;
    Assert.Equal(4, mock.Object.ValueProperty);
}

[stakx]

@lepijohnny – thanks for taking a closer look. Looks about right (except for the potential method override issue, we should perhaps have a test for that, too). Feel free to work on this, if you'd like.

[lepijohnny]

@stakx,

Let me take a closer look to the ongoing PR about the behavioral setups(I like the idea btw).

I will come back with some proposals or dilemmas soon.