CallBase should not be allowed for delegate mocks
#708
Conversation
There was a problem hiding this comment.
Hi @tehmantra - Thank you for submitting this PR, it looks very good already! 👍 See the below review comments for a few minor change requests. Also, could you please add an entry in the changelog, e.g. as follows:
#### Fixed
* `NullReferenceException` when using `SetupSet` on indexers with multiple parameters (@idigra, #694)
+* <short description of your fix, e.g. your PR's title> (@tehmantra, #708)
## 4.10.0 (2018-09-08)Thank you!
src/Moq/MethodCall.cs
Outdated
| @@ -123,6 +123,11 @@ public override void Execute(Invocation invocation) | |||
|
|
|||
| public virtual void SetCallBaseResponse() | |||
| { | |||
| if (typeof(Delegate).IsAssignableFrom(this.Mock.TargetType)) | |||
There was a problem hiding this comment.
Moq defines an extension method Type.IsDelegate(), could you please use it here? → this.Mock.TargetType.IsDelegate()
src/Moq/Mock.Generic.cs
Outdated
| set => this.callBase = value; | ||
| set | ||
| { | ||
| if (typeof(Delegate).IsAssignableFrom(this.MockedType)) |
There was a problem hiding this comment.
(As above:) Moq defines an extension method Type.IsDelegate(), could you please use it here?
src/Moq/Mock.Generic.cs
Outdated
| { | ||
| if (typeof(Delegate).IsAssignableFrom(this.MockedType)) | ||
| { | ||
| throw new NotSupportedException(string.Format(CultureInfo.CurrentCulture, Resources.CallBaseUsedOnDelegateException)); |
There was a problem hiding this comment.
It is important that an exception be thrown only if value == true. This is because when you have a mock with DefaultValue.Mock, and Moq auto-mocks something (e.g. the delegate mock's return value), it'll set that new mock's CallBase to the same value as the "parent" mock (here).
| @@ -87,6 +87,15 @@ internal class Resources { | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Looks up a localized string similar to CallBase cannot be used to mock Delegate types.. | |||
| /// </summary> | |||
There was a problem hiding this comment.
There appears to be something wrong with indentation here, could you please take a look & fix this?
src/Moq/Properties/Resources.resx
Outdated
| <data name="CallBaseUsedOnDelegateException" xml:space="preserve"> | ||
| <value>CallBase cannot be used to mock Delegate types.</value> |
There was a problem hiding this comment.
I'd suggest a slight rephrasing here:
-CallBase cannot be used to mock Delegate types.
+CallBase cannot be used with Delegate mocks.(The reason for rephrasing being that CallBase isn't the party that actively mocks something; the delegate mock exists already by the time CallBase is being set.)
* Update CHANGELOG * Use .IsDelegate() extension * Only throw if CallBase == true * Fix whitespace (spaces to tabs) * Fix CallBase error name and description
|
All comments resolved. Cheers. |
There was a problem hiding this comment.
Thanks for making the changes! One last thing, which I only discovered now (sorry for not noticing it earlier): .CallBase() still works for non-void delegate mocks:
var mock = new Mock<Func<bool>>();
mock.Setup(m => m()).CallBase(); // no exception gets thrownwhich is likely because another this.Mock.TargetType.IsDelegate() check is missing here:
Could you please add the missing check, along with an additional unit test verifying this scenario?
After this, we should be ready to merge. Thanks once again!
|
Done. Missed that this has 2 implementations! Cheers. |
|
Awesome! Thank you for contributing! |
|
@tehmantra - Moq 4.10.1, which includes your bugfix, has just been published on NuGet. |
Fixes: #706
First time looking at this codebase, I've tried to follow the style as best as I could.
I added tests that capture the issue, hopefully I have understood it correctly.
Cheers