-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Modify tests to distinguish between emit- and interpreted-based invoke #70114
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection Issue DetailsVerifying before review
|
public static bool ForceEmitInvoke | ||
{ | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
get => GetCachedSwitchValue("Switches.System.Reflection.ForceEmitInvoke", ref s_forceEmitInvoke); |
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.
Other names in this file start with Switch.
. Is Switches.
here intentional or a mistake?
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.
Yikes - I'll change that thanks.
src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs
Show resolved
Hide resolved
src/libraries/System.Reflection/tests/InvokeEmit/System.Reflection.InvokeEmit.Tests.csproj
Outdated
Show resolved
Hide resolved
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
<TestRuntime>true</TestRuntime> | ||
<IncludeRemoteExecutor>true</IncludeRemoteExecutor> | ||
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix</TargetFrameworks> |
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.
Why do we need separate builds for Windows and Linux?
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.
Not sure; it's matching the original project except I didn't add -Browser.
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.
System.Runtime.Tests.csproj has conditional compilation for Windows vs. Unix, so it needs separate TargetFrameworks.
This project does not have any conditional compilation, so it should not need separate TargetFrameworks
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'll let this CI run finish, and then change it to just NetCoreAppCurrent
. I suspect that other tests were failing in the original project so we should be OK to change it here for just the subset of the reflection tests.
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.
LGTM, thank you
As a follow-up to #67917, explicitly support testing of both emit-based and interpreter-based invoke.
This is done using app context switches to enable the emit- or interpreter-based invoke and then adding new test projects to both the Runtime and Reflection solutions that set the appropriate context switch. The existing tests that use Invoke were added to these new test projects.
This is a replacement for PR #69081 which did not use app context switches.