-
-
Notifications
You must be signed in to change notification settings - Fork 802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Give mocks names #76
Give mocks names #76
Conversation
var mock = new Mock<ToStringOverrider>(); | ||
|
||
Assert.NotNull(mock.ToString()); | ||
Assert.Null(mock.Object.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda weird. I would have hoped the return value would be "correct value". Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting, yes. This is mostly just that I forgot to change the string; it is supposed to return null, because it's a virtual method, and by default Moq should stub out all stubbable methods with no-op implementations that return the relevant default value (false/null/0), and this is one of those.
I'll change it from 'correct value' to make that clearer though, that's definitely a mistake.
Hmm, I have just discovered that this actually only changes ToString() for mocked interfaces, not mocked classes, because that's how the BaseTypeForInterfaceProxy option works, and I can't see an easy way around it. That's not a disaster I suppose, it's still useful in that case, but it's not as good as I'd like it to be... Any ideas? |
If Name cannot be changed for class-based mocks, isn't it a bit less useful I think it's useful enough if the name is used when rendering the Not sure I like that mocks are automatically named. I'd be more comfortable /kzu Daniel Cazzulino On Thu, Dec 19, 2013 at 4:07 PM, Tim Perry [email protected] wrote:
|
Mock naming on it's own isn't really useful though; afaik you're very rarely looking at error messages that involve only the mocks themselves, it's almost always the resulting mock object that your code is asserting on. Mock naming is really only good for relating the mock to the actual mock object instance anyway, which only works when it has a related name too. It's worth pointing out that regardless of whether this functionality is added, you're changing the default ToString value anyway; if you mock an object then the castle proxying messes around with it so you end with ToString returning It is definitely inconsistent though as it stands, you're not wrong, and I don't like that either! I'm going to spend a little longer playing with it, and see if I can find a way to get the whole thing working consistently for interfaces and classes. There's then a separate question of whether we include default names at all, or whether it's only if |
Very good points indeed. Your proposal sounds very sensible indeed :) Thanks for taking the time to think through these use cases to make this /kzu |
Right, sorry that took a while with Christmas and all, but I've now got it finished and working! With these changes, this currently gives default names to all mock objects, which can be overridden, and now has working implementations that set |
Any chance you can Fetch+Rebase these commits so they can be automatically merged? Thanks! |
Ah, sure, done. |
/// | ||
/// This is required to allow Moq to mock ToString on proxy *interface* implementations. | ||
/// </summary> | ||
internal abstract class ProxyBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should call this InterfaceProxy instead? It's a bit more explicit. Not a fan of the "Base" suffix ;)
All sounds good to me; updated. |
I believe this PR broke the Silverlight version, since CSharp is not available as a namespace? Build FAILED. |
Did you have a chance to look at fixing the Silverlight project? /kzu Daniel Cazzulino On Tue, Jan 7, 2014 at 5:07 AM, Tim Perry [email protected] wrote:
|
Not right now, although I should do this weekend if you're happy to wait until then. If not, if you want to fix it yourself I'd suggest just need to wrapping the complaining code in |
yup, agreed. I can wait :) thanks in advance! /kzu Daniel Cazzulino On Thu, Jan 9, 2014 at 1:47 PM, Tim Perry [email protected] wrote:
|
I'm afraid @pimterry this refactoring is causing issues in the latest release. Install it with: Install-Package Moq -Version 4.2.1401.2701 The dynamic proxy cannot access InterfaceProxy, the new base class for interface-based proxies we have. I don't know why, since Moq itself has InternalsVisibleTo DP, but as far as I can see, that's the change that's breaking the latest release. Plz update issue #98 if you find more about it. Thanks! |
Fixes #74. This gives ever mock a unique name initially, of the form
Mock<TypeBeingMocked:1234>
, where1234
is a random 4 numbers, to make mocks more easily distinguishable.This name can be reconfigured, via the new
mock.Name
property, and the name is passed on to the actual mock object; if there's no explicit implementation of ToString in the hierarchy (so it's falling back to the default object implementation) then it instead takes the name of the mock plus '.Object', e.g.Mock<TypeBeingMocked:1234>.Object
. If there is a virtual implementation of .ToString already present in a superclass of the mock, then we proxy that as normal to preserve previous behaviour, returningnull
as a default value.