-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Harden some reflection invoke tests #113000
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection |
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.
PR Overview
This PR hardens tests related to reflection invocation and stack frame analysis in anticipation of upcoming changes in reflection thunk implementations.
- Adds VerifyInnerException tests for constructor and method invocations.
- Adjusts stack frame tests by adding a dummy parameter to avoid reflection optimizations.
- Removes obsolete InvokeInterpreted and InvokeEmit tests to align with planned changes.
Reviewed Changes
File | Description |
---|---|
src/libraries/System.Runtime/tests/System.Reflection.Tests/ConstructorCommonTests.cs | Added a test to check the inner exception for constructor invocations. |
src/libraries/System.Runtime/tests/System.Reflection.Tests/MethodCommonTests.cs | Added a test to check the inner exception for method invocations. |
src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs | Updated inline data and added a dummy parameter to avoid reflection optimizations. |
src/libraries/Common/tests/System/Reflection/InvokeInterpretedTests.cs | Removed obsolete tests. |
src/libraries/Common/tests/System/Reflection/InvokeEmitTests.cs | Removed obsolete tests. |
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
[InlineData(0, false)] | ||
[InlineData(1, false)] | ||
// This is highly dependent on reflection implementation and is not consistent across runtimes. | ||
// The extra 'bool _' parameter avoids a potential reflection optimization for common signatures which may interfere with stack frame count. |
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.
This will break again if this signature gets added to the list of optimized signatures. Should this be test be deleted, or ignore the potential extra frames instead?
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.
Correct - I would like to just remove this test but I was not sure on the long background\history here.
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 did remove that test. In general, the rest of the stack frame tests still verify the stack frame APIs work; this test verified constructors.
Also I will close the Mono mono/mono#15183 issue once this PR is in.
Contributes to #112994
Hardens two tests: