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

Mark GCStressIncompatible and similar tests as out-of-proc #67876

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Apr 11, 2022

According to current architecture we must mark conditionally
enabled tests as out of process because the special conditions
are tested in their individual execution scripts that are skipped
for ordinary merged tests.

Thanks

Tomas

According to current architecture we must mark conditionally
enabled tests as out of process because the special conditions
are tested in their individual execution scripts that are skipped
for ordinary merged tests.

Thanks

Tomas
@ghost
Copy link

ghost commented Apr 11, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

According to current architecture we must mark conditionally
enabled tests as out of process because the special conditions
are tested in their individual execution scripts that are skipped
for ordinary merged tests.

Thanks

Tomas

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

@ghost ghost assigned trylek Apr 11, 2022
@trylek
Copy link
Member Author

trylek commented Apr 11, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

This seems fine, although it seems like a better solution might be to have GCStressIncompatible automatically imply RequiresProcessIsolation. Otherwise, if we mark or unmark GCStressIncompatible we'll have to remember to do the correct thing for RequiresProcessIsolation. E.g., it's not necessarily true that removing GCStressIncompatible to fix a bug where a test was temporarily disabled on GCStress means you can also remove RequiresProcessIsolation.

@trylek
Copy link
Member Author

trylek commented Apr 12, 2022

Thanks Bruce, I'll make sure to address the various cleanup feedback we continue receiving as part of these changes and our various discoveries. It's actually not just about GCStressIncompatible, the same holds for UnloadabilityCompatible, JitOptimizationSensitive and other special conditions with targeted support in the generated execution scripts. Ultimately I'd love to minimize the cases forcing out-of-process execution because it incurs extra perf cost and is currently incompatible with Mono, we just need to identify a reasonable middle ground I guess.

@jkoritzinsky
Copy link
Member

After looking over things again, I remembered that we can actually use the SkipOnCoreCLR attribute as there's a setting to detect GCStress and skip a test for GCStressIncompatible (same goes for JitOptimizationSensitive).

UnloadabilityIncompatible would have to be handled differently, but it's not impossible to handle.

@trylek
Copy link
Member Author

trylek commented Apr 12, 2022

I'm merging this in for now to expedite the switchover of JIT/Methodical tests as soon as the GC bug I believe to be causing hangs and stack overflows on Linux has been fixed. For the next phase of test merging we should collect all this feedback and prepare a robust design plan ending up with something cleaner than we have now. We can discuss that on the primary issue #54512.

@trylek trylek merged commit 3635e0f into dotnet:main Apr 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2022
@trylek trylek deleted the GCStressIncompatible branch May 10, 2023 18:58
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