Skip to content
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

Merged
merged 3 commits into from
Mar 4, 2025

Conversation

steveharter
Copy link
Member

Contributes to #112994

Hardens two tests:

  • Avoids checking the name of internal call stack method names between interpreted and IL-emit generated reflection thunks. We are planning on adding a new type of hard-coded thunk\wrapper for common signatures and may remove the interpreted path which will cause these tests to fail.
  • Adds a dummy parameter to a stack frame test method so that it won't attempt to use a hard-coded common signature which will have a different stack frame.

@steveharter steveharter added area-System.Reflection test-enhancement Improvements of test source code labels Feb 27, 2025
@steveharter steveharter self-assigned this Feb 27, 2025
@Copilot Copilot bot review requested due to automatic review settings February 27, 2025 20:34
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

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.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@steveharter steveharter merged commit b4f8d4f into dotnet:main Mar 4, 2025
80 of 83 checks passed
@steveharter steveharter deleted the invoke-unloadable branch March 4, 2025 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants