Repository navigation
Enable multi-method setups - #1203
Conversation
The difference between those two classes being that `Expectation` isn't
necessarily bound to a single, specific method.
Then make `Setup` use `Expectation` instead so that the types correspond
as follows:
`Setup` <-> `Expectation`
`MethodSetup` <-> `MethodExpectation`
But don't expose that fact via `ISetup.InnerMock[s]` just yet; see the code comment in `ISetup` for an explanation.
| public virtual bool HasResultExpression(out IAwaitableFactory awaitableFactory) | ||
| { | ||
| awaitableFactory = null; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This method doesn't really fit in very well here, as it doesn't have much to do with the core purpose of Expectation.
A possibly better design might be to create a custom LINQ expression tree node type ResultExpression (which would carry the IAwaitableFactory as data), then add such a node directly to Expression.
That can be done as a separate refactoring another time.
| public virtual void SetupEvaluatedSuccessfully(Invocation invocation) | ||
| { | ||
| } |
There was a problem hiding this comment.
This also doesn't fit in very well here.
I'm not yet sure how to get rid of it. A few possible options off the top off my head:
-
Let
Expectationexpose its argument matchers through a property. When a setup is successfully matched,Setupcould then skip overExpectation.SetupEvaluatedSuccessfullyand call the matchers'SetupEvaluatedSuccessfullymethods directly. -
Add an event
Setup.Matched, to which argument matchers may optionally subscribe theirSetupEvaluatedSuccessfullymethods.
The first option seems like the more plausible in the short term. It doesn't make much sense to add a Matched event just to get rid of this method here, however that solution may start to look more plausible if the event is going to get added to also support other features (such as better support for sequences).
| lock (this.setups) | ||
| { | ||
| this.setups.RemoveAll(x => x.Method.IsPropertyAccessor()); | ||
| this.setups.RemoveAll(s => s is MethodSetup ms && ms.Method.IsPropertyAccessor()); |
There was a problem hiding this comment.
This is a bad change, but acceptable if it means we'll soon be able to remove this whole method.
|
@stakx, thanks for completing this. What version of MoQ will this change be in? |
|
@aysan0, this change was made mainly to pave the way for fixing #1066. For now, it remains strictly an implementation detail, and this will probably not be exposed in any public API anytime soon. (The likely moment for making this public would be when we allow user code to add fully custom |
This refactoring of Moq internals enables setups that handle more than just a single method.
I have two use cases in mind:
StubbedPropertyGetterSetupandStubbedPropertySetterSetupcould be merged into a single setup type. This would be more efficient than what we have now, and also better capture the semantics of a call tomock.SetupProperty().It would allow creation of a new setup type
StubbedPropertiesSetupbackingmock.SetupAllProperties(). This could keep track of any number of properties' values in a dictionary. Again, this would be more efficient than the current solution, allow us to get rid of some convoluted internal bits (such asSetupCollection.RemoveAllPropertyAccessorSetups), and it may help solve Property setups are ignored on mocks instantiated usingMock.Of#1066.