Moq4: mock.Setup(m => m.A.B.X) should setup only what's minimally required. It shouldn't setup all properties, nor override preexisting setups in A or B.

[stakx]

A setup such as this:

mock.Setup(m => m.A.B.X);  // A, B, X are method invocations or properties

currently doesn't just set up what is minimally required to set up X. That is, it is not equivalent to the following:

var b = new Mock<B>();  // or, if m.A.B is already set up, retrieve the existing m.A.B instance
b.Setup(_ => _.X);

var a = new Mock<A>();  // or, if m.A is already set up, retrieve the existing m.A instance
a.Setup(_ => _.B).Returns(() => b.Object);

m.Setup(_ => _.A).Returns(() => a.Object);

That's what one might assume would happen, but what actually happens is that Moq also calls SetupAllProperties for the traversed objects A and B. Also, if A, B, or X have previously been set up, it just throws away those and replaces them with the new setups. (This may be simplifying things a bit.) In my opinion, this violates the principle of least astonishment.

Also, the current behavior causes problems like #110 and #191.

I'd like to make a case for changing Moq's current behavior, so that a "recursive" setup expression such as the above will only set up what is minimally required to get at X and set it up, and it should only set up what is visible in the setup expression. For example, the short setup and the multi-line setup above should be functionally equivalent.

I'd welcome any opinions. If you don't have a lot of time to spare, feel free to 👍 (agree) or 👎 (diagree).

[Arithmomaniac]

+1. Issue #191 is preventing me from using VerifyAll for my MockRepositorys.

[Arithmomaniac]

If it ends up being too risky to break backwards compatibility, it would at least be nice to have a property on a MockRepository (and/or constructor overload creating mocks) that could toggle this. MockBehavior could be turned into a [Flags] enum so it could be set that way.

[stakx]

I agree, I'm almost sure this change would have to be introduced with a backwards compatibility switch. I'm just not sure what form that switch should take.

Newer versions of .NET offer a standardized switch facility via AppContext, but I'm not convinced that ambient switches are a terribly good idea—everytime someone asks a question about Moq on Stack Overflow, they'll have to declare their switch settings so that one knows how Moq will behave for their code example. For the same reason, I feel there should be as few of those switches as possible, one shouldn't introduce one without giving it some thought. Perhaps sometimes a clean break(ing change) is just as valid a solution.

I guess having explicit switches would be better. Your idea of turning MockBehavior into a flags enum seems interesting. Can you think of any downsides to doing this?

[Arithmomaniac]

The only downside I can think of is that enum flags assume that the default case has a value of 0, so the current values would have to be reordered, which would break MockRepository serialization. I'm not sure why anyone would ever want to do that, though...

Something like this:

[Flags]
public enum MockBehavior
{
    Default = 0, //currently 1
    [Obsolete("This is the default behavior, and thus should be omitted")]
    Loose = 0, //currently 1
    Strict = 1, //currently 0
    MinimalSetup = 2
}

[stakx]

@Arithmomaniac - I just released a hotfix update of Moq (because of #500). After giving this some more thought, I opted not to reuse MockBehavior to host additional feature switches, as this might break client code that does comparisons such as mock.Behavior == MockBehavior.Strict. Such equality comparisons would all have to be rewritten if the enum became a flags enum. Instead, I've introduced a separate property Switches on both Mock and MockBehavior. (Not super-excited about it, but probably the safer choice in the long run.) So additional feature and compatibility switches (such as the one above) will likely go in the new Switches flags enum. We can use the Switches.Default enum value to specify whether a feature is enabled or disabled by default.

[stakx]

Closing this, this problem will be tracked in #643 (together with a couple other related issues).