-
-
Notifications
You must be signed in to change notification settings - Fork 819
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? |
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
GetMethod
logic 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)); |
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); |
Copilot uses AI. Check for mistakes.
methods = methods.Where(m => m.GetParameterTypes().CompareTo(argTypes, exact, considerTypeMatchers: false)); | ||
|
||
if (methods.Count() < 2) |
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) |
Copilot uses AI. Check for mistakes.
This PR fixes an issue described in the #1341