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

Modify tests to distinguish between emit- and interpreted-based invoke #70114

Merged
merged 5 commits into from
Jun 9, 2022

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Jun 1, 2022

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.

@ghost
Copy link

ghost commented Jun 1, 2022

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

Issue Details

Verifying before review

Author: steveharter
Assignees: steveharter
Labels:

area-System.Reflection

Milestone: -

@steveharter steveharter marked this pull request as ready for review June 2, 2022 15:52
@steveharter steveharter added this to the 7.0.0 milestone Jun 2, 2022
public static bool ForceEmitInvoke
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue("Switches.System.Reflection.ForceEmitInvoke", ref s_forceEmitInvoke);
Copy link
Member

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?

Copy link
Member Author

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.

<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<TestRuntime>true</TestRuntime>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix</TargetFrameworks>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@jkotas jkotas Jun 8, 2022

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

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'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.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@steveharter steveharter merged commit 61ddecc into dotnet:main Jun 9, 2022
@steveharter steveharter deleted the InvokeTest2 branch June 9, 2022 20:45
@ghost ghost locked as resolved and limited conversation to collaborators Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants