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
Merged
stakx
merged 6 commits into
devlooped:main
from
tonyhallett:fix-protectedmock-expression-type-args
Aug 3, 2021
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ae588a4
fix #1188
tonyhallett 1059cd7
Add changelog entry
tonyhallett c145b2e
code style
tonyhallett 3f381ea
Correct LambdaExpression logic and reduce unnecessary Matcher checks
tonyhallett e05680b
remove condition match test and reorder match test order
tonyhallett f6a0837
extend the scope of the fix in the changelog
tonyhallett File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.