Moq should not use a mock's internal implementation of IEnumerable

[Eregrith]

( See #464 )

https://github.com/moq/moq4/blob/61c35056dda76a157dd78cd9578cee86f7a16f02/Source/Extensions.cs#L91-L100

This part calls for the checked value's implementation of GetEnumerator(). If and when that value is a mock, calling GetEnumerator() might do anything, as far as throwing an exception.
Moq should not call for that value's implementation of IEnumerable, or even any method on that value for that matter.

[Eregrith]

@stakx Ouch. Just got to open the project on VS2017. I wanted to see what tests were done on ExpressionStringBuilder so I added a throw in the constructor.

Turns out two thirds of the tests fail then. I guess I'll have a worst time than anticipated XD

[stakx]

@Eregrith: Yes :-D It is an important component. Apart from its use for diagnostic messages, Moq uses string representations of setup expressions to quickly look up setups (IProxyCall aka MethodCall) in a dictionary.

As long as you can make sure that the string representation of a setup expression properly captures a method call's "shape", you're good.

Perhaps the easiest thing would be to change / augment those tests to see if value is IMocked. All mock objects implement that interface (except for delegate mocks, whose method sits in a type that implements IMocked).

As per our previous discussion, I'll assign you to this issue for now. If you don't find a good way to untangle Moq from its IEnumerable addiction, just let me know and I'll take a look, too.

[Eregrith]

@stakx Thanks ! I'll have to do some speleology to see if I can find tests targetting ExpressionStringBuilder specifically, because 500+ tests breaking for each change is not going to work for me. I hoped the unit tests were properly decoupling tested parts :/

[stakx]

Given StringExpressionBuilder's possible origin as a diagnostic support infrastructure, there probably won't be many tests targeting it specifically. Good luck nevertheless! :) Feel free to ask any questions you might have, I'll do my best to offer you some guidance.

(P.S.: You're of course welcome to add proper unit tests for that component, if you don't find any.)

[stakx]

[...] because 500+ tests breaking for each change is not going to work for me. I hoped the unit tests were properly decoupling tested parts :/

@Eregrith - quick ping. Are you still looking into this at all, or would you rather hand this back?

I've just been playing around a bit with this:

https://github.com/moq/moq4/blob/61c35056dda76a157dd78cd9578cee86f7a16f02/Source/Extensions.cs#L91-L95

You could indeed get things (including your originally reported issue) working properly simply by replacing the highlighted second condition with:

&& !(value is IMocked)

[Eregrith]

@stakx Sorry I'm not currently in a schedule allowing me to look into this :/ If you're able to handle it, I'll gladly let you have at it :)

[stakx]

@Eregrith: No worries, I can make the necessary change.