-
-
Notifications
You must be signed in to change notification settings - Fork 825
mock.Protected().Setup<int>("Foo") fails base implementation of Foo is hidden in the derived class #1342
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
base: main
Are you sure you want to change the base?
Conversation
|
|
…s hidden in the derived class (devlooped#1341)
|
Thanks for the PR! Could you rebase on top of main, since files moved around? |
stakx
left a comment
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.
I haven't reviewed & tested this in detail TBH, but it generally looks good to me.
The changelog should be first updated with the latest few missing releases though, otherwise the changelog entry will end up with an incorrect (already published) 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.
Pull Request Overview
This PR addresses issue #1341 by improving how ProtectedMock selects hidden virtual methods and adds tests to cover this scenario.
- Adjusted
GetMethodlogic to prefer methods declared in derived classes when multiple matches exist. - Added a new test (
SetupResultAllowsHiddenVirtualMethods) to verify setup of hidden protected methods. - Updated changelog to document the fix.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Moq.Tests/ProtectedMockFixture.cs | Added a test verifying setup of hidden protected methods. |
| src/Moq/Protected/ProtectedMock.cs | Enhanced method resolution logic for hidden virtual methods. |
| CHANGELOG.md | Documented the fix in the Unreleased fixed section. |
|
|
||
| for (Type type = typeof(T); type != typeof(object); type = type.BaseType) | ||
| { | ||
| var method = methods.SingleOrDefault(m => m.DeclaringType == typeof(T)); |
Copilot
AI
May 26, 2025
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 loop variable type is never used: the predicate always checks DeclaringType == typeof(T). It should use the loop variable type (m.DeclaringType == type) to correctly select the most derived declaring type.
| var method = methods.SingleOrDefault(m => m.DeclaringType == typeof(T)); | |
| var method = methods.SingleOrDefault(m => m.DeclaringType == type); |
| methods = methods.Where(m => m.GetParameterTypes().CompareTo(argTypes, exact, considerTypeMatchers: false)); | ||
|
|
||
| if (methods.Count() < 2) |
Copilot
AI
May 26, 2025
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.
Calling Count() on an IEnumerable will enumerate it; since it's used again below, consider materializing methods into a collection (e.g., var list = methods.ToList()) to avoid multiple enumerations.
| methods = methods.Where(m => m.GetParameterTypes().CompareTo(argTypes, exact, considerTypeMatchers: false)); | |
| if (methods.Count() < 2) | |
| methods = methods.Where(m => m.GetParameterTypes().CompareTo(argTypes, exact, considerTypeMatchers: false)).ToList(); | |
| if (methods.Count < 2) |
This PR fixes an issue described in the #1341