Always construct mocks as strict#43844
Conversation
eng/config/test/BannedSymbols.txt
Outdated
| @@ -0,0 +1,2 @@ | |||
| M:Moq.Mock`1.#ctor; Pass MockBehavior.Strict explicitly to the constructor for Mock<T> | |||
| F:Moq.MockBehavior.Loose; Use MockBehavior.Strict for mocks | |||
There was a problem hiding this comment.
I get the first one, but we don't want people to be opting in, even explicitly?
There was a problem hiding this comment.
No. Mocks are challenging for maintainability because they do not show up as implementations of classes and interfaces, so they are prone to unintended deviations from adherence to API post-conditions. Loose mocks are the worst case scenario because they also don't show up as references to the API which changed.
Stubs are preferred to mocks because they are true implementations of the API, but one step at a time...
jasonmalinowski
left a comment
There was a problem hiding this comment.
I'm 100% for banning the constructor that doesn't specify the MockBehavior.Strict; my concern was also banning MockBehavior.Loose. @sharwell privately made a separate argument that there wasn't much reason to keep it around since for the most part the PR here was trivial and switching things to strict mocks worked just fine, and that's a much better argument to me than the other argument presented -- I'm a firm believer that all tools have a time and place.
Looking at the actual textual diffs though I'm kinda swaying back: there's two places where the loose mock seems fine, and I wish we'd just keep that.
| New Mock(Of IFindPeekResultsCallback)(MockBehavior.Loose).Object) | ||
| callbackMock.Object) |
There was a problem hiding this comment.
Kinda wish we were keeping the loose mock here -- behavior changes really shouldn't require test changes here I don't think...
| Dim mockSVsServiceProvider = New Mock(Of SVsServiceProvider)(MockBehavior.Strict) | ||
| mockSVsServiceProvider.Setup(Function(s) s.GetService(GetType(SVsTextManager))).Returns(Nothing) |
There was a problem hiding this comment.
Wish we also had the loose mock here as well. A random implementation change elsewhere doesn't really feel like it should trigger a requirement to add an explicit "Returns(Nothing)" if the test would have already passed.
There was a problem hiding this comment.
A random implementation change elsewhere doesn't really feel like it should trigger a requirement to add an explicit "Returns(Nothing)" if the test would have already passed.
This is why we don't use Moq.
Banned API analyzer is updated to report construction of loose mocks.