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

Disable System.Reflection.Tests.TypeDelegatorTests.FunctionPointers #82321

Merged

Conversation

BruceForstall
Copy link
Member

Disable this failing test on CoreCLR.

Tracking: #82252

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr libraries-jitstress

@ghost
Copy link

ghost commented Feb 17, 2023

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

Issue Details

Disable this failing test on CoreCLR.

Tracking: #82252

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-System.Reflection

Milestone: -

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall
Copy link
Member Author

@dotnet/jit-contrib PTAL -- want to get a clean libraries stress pipeline

@steveharter
Copy link
Member

It looks like the "jitstress" tests are successful but there's still other failures -- so was disabling that test successful here?

@jkotas
Copy link
Member

jkotas commented Feb 17, 2023

#82325 has the actual fix for the problem.

If this is the only failing test, I am ok with disabling the test before the actual fix is merged.

Note that the failure only repros with tiered compilation disabled, or when the affected code is promoted to Tier 1.

@BruceForstall
Copy link
Member Author

If this is the only failing test, I am ok with disabling the test before the actual fix is merged.

I'd prefer to disable the test when this testing completes, then you can re-enable either with your fix or afterwards, after also running a clean libraries-jitstress pipeline.

fwiw, we should probably add a TieredCompilation=0 run to all libraries changes.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

After the real fix goes in, I'll revert this. Thanks

@jkotas
Copy link
Member

jkotas commented Feb 17, 2023

fwiw, we should probably add a TieredCompilation=0 run to all libraries changes.

I agree that it would be nice, but it is not where we are going with PR CI. We do not have enough budget to test all variants in PR CI. We are on a path to do less testing in PR CI and allow more issues to slip through that are only going to be found later in rolling runs.

@BruceForstall
Copy link
Member Author

BruceForstall commented Feb 18, 2023

I've wondered whether the libraries tests PR testing should be TieredCompilation=0 and the outerloop rolling should be the default TieredCompilation=1. I wonder how many of the functions executed during libraries tests are compiled in Tier-0 versus Tier-1. It probably doesn't matter to the libraries team, but for the JIT team it does matter.

@BruceForstall BruceForstall merged commit 473e278 into dotnet:main Feb 18, 2023
@BruceForstall BruceForstall deleted the DisableFunctionPointersTest branch February 18, 2023 00:17
jkotas added a commit to jkotas/runtime that referenced this pull request Feb 18, 2023
jkotas added a commit that referenced this pull request Feb 18, 2023
* Fix FnPtrTypeDesc::GetManagedClassObject

The code was missing support for allocating RuntimeType objects on frozen heap

Fixes #82252

* Revert "Disable System.Reflection.Tests.TypeDelegatorTests.FunctionPointers (#82321)"

This reverts commit 473e278.
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2023
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.

4 participants