Repository navigation
8000
Support Linq to Mock for MockBehaviour.Strict #740
Description
This came up in dotnet/winforms#285
Winforms guidelines require strict mocks and I’d like to use linq to mocks but we can’t make strict mocks using Mock.Of
/cc @sharwell
Activity
sharwell commentedon Jan 4, 2019
Technically a duplicate of #154, but that issue needs to be reopened.
sharwell commentedon Jan 4, 2019
📝 Note that the implementation would go here:
https://github.com/moq/moq4/blob/master/src/Moq/Linq/Mock.cs
stakx commentedon Jan 5, 2019
@hughbe, @sharwell - Yes, this is a duplicate of #154, which was left unresolved. @JohanLarsson submitted two PRs: #155, which got closed for being "unmergable" (whitespace issues, merge conflicts, etc.); and #390, which I opposed because it included only
Mock.Of<T>(MockBehavior)instead of the much more usefulMock.Of<T>(MockBehavior, Expression<Func<T, bool>>). We never got to a PR for the latter method overload.Is anyone here interested in implementing the latter method,
Mock.Of<T>(MockBehavior, Expression<Func<T, bool>>)? If so, please raise your hand. :-) Here's some additional cues about issues that might come up:Just adding this latter
Mock.Ofoverload (the one taking a setup expression) to Moq's API should be fairly trivial ifMock.Behaviorwere made internally settable (it's currently a read-only property, and should probably stay so in the public API). You could then mostly delegate to the pre-existingMock.Ofand end by simply settingBehaviorto a new value.One last point that I didn't mention at the time of #154:
MockBehavior.Strictmay actually be of limited use in conjunction withMock.OfbecauseMock.Ofas it is today always callsSetupAllProperties. Because of that call toSetupAllProperties, all property accessors will have a setup, even those properties that don't show up in the setup expression. This might lead to surprises withMockBehavior.Strict, because the resulting mock won't be as strict as you might wish. Say you have this:You might think the only accessible properties will be
WidthandHeight... but not so. Despite this nominally being a "strict" mock, you'll be able to query or set any property without Moq throwing aMockExceptionin your face. The only thing you won't be allowed is to invoke regular methods. Thus a "semi-strict" mock... not very intuitive IMO.An argument could be made that
Mock.Ofsimply shouldn't callSetupAllPropertieswhen used in conjunction withMockBehavior.Strict. This seems reasonable, but without some more thought I'm not certain whether this won't cause unintended side effects or lead to API inconsistencies. A closer look would be required here, as well.stakx commentedon Jan 5, 2019
@hughbe - Do you know about
mock.VerifyNoOtherCalls?stakx commentedon Mar 6, 2019
Oh, it turns out you can already do
Mock.Of+MockBehavior.Strict:True, a more straightforward API might be nice... but at least it's possible.
ishymko commentedon May 7, 2019
@stakx looks like in current implementation of LINQ to Mocks
SetupAllPropertiesis mandatory, since during building setups some invocations on a mock are performed. I've found it in a TODO inLinq\Mock.cs:TODO: Make LINQ to Mocks set up mocks without causing invocations of its own, then remove this hack.. The TODO is related to a code nearby, which performs cleanup of invocations before returning created mock.Moreover, using
SetupAllPropertiesleads to the following: even in case of using LINQ to Mocks from a repository with strict behaviour, you'll get all properties being set up, which is strictly speaking not quite correct.Given that
SetupAllPropertiesis what we have, I suppose another hacking can be applied in order to make it consistent: we can mark with a flag setups made bySetupAllPropertiesto remove them after LINQ to Mocks job is done. It feels really hacky though...Or maybe alternative mechanism mentioned in the TODO is in progress for the next version?
stakx commentedon May 7, 2019
@vanashimko, your analysis is correct, I think you understand now why nothing has happened yet. The combination of
SetupAllProperties+ strict mock doesn't make any sense,Mock.Offor a strict mock would have to be reimplemented in a way that does not rely onSetupAllPropertiesat all. It's possible, but probably non-trivial.While the idea to "flag" property setups for later easy removal probably wouldn't be easy to implement in practice, it goes in the same direction that I am seeing for
SetupAllPropertiesin general. This method is a performance bottleneck. What I'd like to do is rewrite it such that it doesn't actually do anything other than setting a switch on the mock. That switch would effect that accesses to properties without a setup will cause a setup to be added on-demand, i.e. at the time of the first access instead of upfront.That change is also not trivial BUT if it were applied, it should be fairly easy to let
Mock.Ofcontinue working as it always has, except for strict mocks theSetupAllPropertiesswitch would be disabled again when the mock has been created. That just might do the trick.I'd love to make that change in the next minor iteration, but only if it's feasible as a non-breaking change.
ishymko commentedon May 7, 2019
@stakx thanks for clarification, I see your point.
It seems to be that the problem of
SetupAllPropertiesbeing called for Strict repositories too causing side effect of "non-strictness" is a different issue. So it can be fixed aside from the change described in this topic. I can open a new issue based on the information you provided here, if you OK with it. Also it appears that fix of the issue lies in a bucket 3 according to this clarification of breaking changes: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-changes.md. It's easy to believe that there is a code implicitly depends on the fact, that strict repository produces mocks withSetupAllProperties(probably I'm the one who wrote code like this too, don't remember for sure 😄).As for the changes discussed earlier in this topic, do you think it makes sense to extend the API without fixing the stuff related to strictness and
SetupAllProperties? You mentioned that it can be easily done by addinginternal settoBehaviorproperty, if so, I think I can pick this up.stakx commentedon May 7, 2019
We already have this issue. Do we really need another for discussing the same thing? (Or did you mean a new issue re:
SetupAllPropertiesbeing a performance bottleneck?)I don't think implementing a half-way solution would be a good idea. Once we add the new API with
SetupAllPropertiesstill in place, even for strict mocks, we will no longer be able to later implement the API more reasonably, as that would then be a breaking change. If you really need a strict mock with lots of setups you didn't ask for, you can use theMockRepositoryworkaround. But that's not a useful-enough end result to warrant a new API; let's reserve that one for a more reasonable behavior (i.e. one that doesn't set up everything needlessly).ishymko commentedon May 7, 2019
@stakx yep, I suppose implementing a mechanism of on-demand setups you described can go independent from the issue discussing here. After that it can be possible, I think, to add this new API and make it consistent with MockRepository (which means removing SetupAllProperties for strict mocks in repository). Maybe it also means that this issue should be marked somehow, that it depends on another one and can not be easily implemented for now.