-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
fix ProtectedMock fails when arguments are of type Expression #1188 #1189
fix ProtectedMock fails when arguments are of type Expression #1188 #1189
Conversation
Could you please give this PR a somewhat more meaningful title? If this gets merged, the PR title will end up in the commit history, where some random GitHub issue numbers won't be very helpful. |
This needs further thought due to |
37636b7
to
ae588a4
Compare
@stakx Your thoughts please when you have the time. |
@tonyhallett, if I understand the problem correctly, a solution would perhaps need to check whether the provided I'm on the move right now and cannot verify whether this idea works, perhaps you can give it a try. |
We need to consider |
This ( |
I am not sure what you mean by
I have not introduced anything new. On the rare occasion that an argument ( method, property setter, indexer ) is an Expression then we check if it is a matcher expression using the built in method of doing so. |
I know that you haven't. I was assuming that you might need to, but I see now that this assumption doesn't necessarily hold. My comment was in response to your following statement:
I think you're right, and by "the built in method" I suppose you're referring to those extension methods in Yes, that sounds feasible! |
Great ! |
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.
Thanks for the PR, @tonyhallett. Looks good 👍, just a few minor details.
CHANGELOG.md
Outdated
@@ -13,6 +13,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 | |||
|
|||
#### Fixed | |||
|
|||
* mock.Protected().SetupSet fails when argument is of type Expression (@tonyhallett, #1189) |
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.
Please remove. (We don't need a changelog entry because from a user's perspective, this issue never actually existed: It only surfaced after #1186, but wasn't yet observable before that, i.e. in the latest released version.)
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.
No it has always been an issue. You changed the issue title from "ProtectedMock fails when arguments are of type Expression" to mock.Protected().SetupSet fails when argument is of type Expression.
I have just put the issue title in to the changelog.
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.
@tonyhallett, yes, the issue may always been present, but it never surfaced for Moq users because value
was ignored up until recently. (Or am I missing something here?) So users don't need to know about this issue. The PR title is only relevant because it ends up in the commit history. The commit history is important to Moq developers, the changelog OTOH is important to Moq users. Please remove the changelog entry.
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.
The issue could have surfaced at any time over the last 12? years as ToExpressionArg was being called by ToExpressionArgs, called by the GetMethodCall methods ( args
were not ignored ). GetMethodCall methods are called by numerous paths.
For instance
https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L47
Link is from before #1186
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.
As I said, perhaps your change to the issue title made you think it only applied to SetupSet which was ignoring the value
.
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.
@tonyhallet, I see. Thank you for the explanation. The changelog entry certainly made me think so, since it does mention SetupSet
(which so far wouldn't have been affected, right? 😉). Let's keep the changelog entry then. Perhaps you can adjust it to include method(s) where this would have been observable even before now? And do feel free to give this PR a more accurate / correct title if you feel that the current title is inaccurate.
@tonyhallett, this should be ready to merge, right? Or was there anything else you need to finalize here before merging? |
Only thing was do you want to keep the I think I identified a way of adding ref / out to protected mock ( details above ) so I think that we should keep it. I will provide an issue / pull request in a little while. |
I'd say let's look at that in a separate PR. |
Ok so you want me to remove the check for |
Why not keep things as they are now. I don't mind keeping |
Great. Please merge. |
fix #1188