Skip to content

Migrate JitOptimizationSensitive and GCStressIncompatible to SkipOnCoreClr attributes#126108

Open
Copilot wants to merge 26 commits intomainfrom
copilot/update-requirements-for-requiresprocessisolation
Open

Migrate JitOptimizationSensitive and GCStressIncompatible to SkipOnCoreClr attributes#126108
Copilot wants to merge 26 commits intomainfrom
copilot/update-requirements-for-requiresprocessisolation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 25, 2026

Replaces eligible MSBuild <JitOptimizationSensitive> and <GCStressIncompatible> properties with [SkipOnCoreClr(...)] applied directly to test entry points, moving skip logic from build-time property injection into the XUnit test runner where the test can safely run without process isolation.

Description

Why

MSBuild properties like <GCStressIncompatible> and <JitOptimizationSensitive> require <RequiresProcessIsolation>true</RequiresProcessIsolation> to take effect — process isolation is expensive. Moving eligible skips to XUnit attributes allows tests to be skipped in-process without spawning a new process per test. Tests that still require process isolation for other reasons retain <RequiresProcessIsolation>true</RequiresProcessIsolation>.

Changes

JitOptimizationSensitive — ilproj tests (4 .il files, 4 .ilproj files)

  • Added SkipOnCoreClrAttribute with AnyJitOptimizationStress (0x11E) to each IL entry point
  • Added .assembly extern Microsoft.DotNet.XUnitExtensions where missing
  • Removed <JitOptimizationSensitive> and sole-cause <RequiresProcessIsolation> from project files
  • Added <ProjectReference Include="$(TestLibraryProjectPath)" /> to project files that were missing it

GCStressIncompatible — all project types (~310 files total)

C# (150 .cs + 150 .csproj):

  • Added [SkipOnCoreClr("This test is not compatible with GC stress.", RuntimeTestModes.AnyGCStress)] before each [Fact]/[Theory]/[ConditionalFact]/[ConditionalTheory]
  • Removed <GCStressIncompatible> and sole-cause <RequiresProcessIsolation> from project files
  • Added <ProjectReference Include="$(TestLibraryProjectPath)" /> to 65 csproj files that were missing it
  • Removed dangling <GCStressIncompatible> from project files where the compiled source already had a corresponding [SkipOnCoreClr(..., RuntimeTestModes.AnyGCStress)] attribute

IL (6 .ilproj, 3 .il):

  • 3 GenericContext tests: IL was pre-migrated; removed MSBuild properties from project files
  • 3 JIT regression tests: Added SkipOnCoreClrAttribute (0xC0 = AnyGCStress) to IL entry points; updated project files
  • Added <ProjectReference Include="$(TestLibraryProjectPath)" /> to 6 ilproj files that were missing it

F#:

  • Runtime_72845 was left on the standalone [<EntryPoint>]/exit-code pattern with <GCStressIncompatible>true</GCStressIncompatible> and <RequiresProcessIsolation>true</RequiresProcessIsolation> retained, because the attempted XUnit conversion did not compile in targeted test builds

HeapVerifyIncompatible — migrated tests

  • Added <ProjectReference Include="$(TestLibraryProjectPath)" /> to project files that were missing it
  • For tests where <HeapVerifyIncompatible> was conditional on architecture, replaced the unconditional [SkipOnCoreClr(..., RuntimeTestModes.HeapVerify)] with [ConditionalFact] backed by a static bool property that combines the architecture check with TestLibrary.CoreClrConfigurationDetection.IsHeapVerify, preserving the original per-architecture skip semantics

Comment cleanup

  • Audited edited project files where <GCStressIncompatible> was removed and updated or removed stale RequiresProcessIsolation comments
  • Audited orphaned project comments that provided GC stress skip context, removed the orphaned comments, and moved important issue/context details into the corresponding [SkipOnCoreClr(...)] attribute messages

Infrastructure and documentation

  • Doubled the browser-wasm (CoreCLR) runtime test work item timeout in helixpublishwitharcade.proj (123 → 246 minutes) to accommodate the larger merged test collections
  • Updated requiresprocessisolation.md to document two additional triggers for <RequiresProcessIsolation>: <IsLongRunningGCTest> and <CLRTestExecutionArguments>

CI failure fixes and process-isolation audit

  • Restored <RequiresProcessIsolation>true</RequiresProcessIsolation> to tests that still require isolation for GC measurement accuracy, long-running GC pre-commands, custom CLR test execution arguments, GC.WaitForPendingFinalizers, unsupported target handling, unloadability, NativeAOT incompatibility, output-copying project references, or Process.Start
  • Added targeted [ActiveIssue] suppressions for CI failures on browser-wasm CoreCLR and Mono interpreter browser-wasm where appropriate
  • Resolved merge conflict in GetTotalAllocatedBytes.cs while preserving GC stress and HeapVerify skip behavior

Not migrated (MSBuild property retained)

  • Tests with ReferenceXUnitWrapperGenerator=false — no XUnit runner
  • Profiler tests — custom host runner with no XUnit
  • HW intrinsics wrapper projects — architecture-conditional property, no owned source
  • OutputType=Library projects — no entry point
  • Tests using top-level statements with no [Fact] method
  • Runtime_72845.fsproj — F# XUnit conversion did not compile in targeted test builds, so this test retains <GCStressIncompatible>true</GCStressIncompatible> and standalone process isolation
  • ReadyToRun tests with crossgen2 shell pre-commands (multifolder, determinism/crossgen2determinism) — [SkipOnCoreClr] only skips the XUnit test body, not pre-commands that run before the test executable
  • Tests that still match requiresprocessisolation.md rules retain <RequiresProcessIsolation>, including projects with CLRTestExecutionArguments, IsLongRunningGCTest, GC.WaitForPendingFinalizers, CLRTestTargetUnsupported, UnloadabilityIncompatible, NativeAotIncompatible, ProjectReference/content copying requirements, or Process.Start

Copilot AI and others added 7 commits March 24, 2026 21:57
…odes.HeapVerify)

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b9ff7f11-db85-474b-bed1-1e4bd364f234
…ource files

Replace MSBuild property JitOptimizationSensitive with
[SkipOnCoreClr("This test is sensitive to JIT optimizations.", RuntimeTestModes.AnyJitOptimizationStress)]
attribute directly on [Fact]/[Theory] methods across 48 test source files.

Corresponding .csproj files have the <JitOptimizationSensitive> property removed.
RequiresProcessIsolation is also removed from project files where
JitOptimizationSensitive was the sole reason for it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e migration

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/291e5882-a072-4964-8394-d20db497e472
…IL source files

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e0266c53-b95c-4538-80c9-b7d881f45840
… ilproj, fsproj

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/66a86949-b520-4439-8d71-659f2b4391fc
…projects

- Remove GCStressIncompatible from 7 OutputType=Library projects where the
  property is a no-op.
- Remove redundant conditional GCStressIncompatible from 10 X86 HWIntrinsics
  projects (both arm64+osx and arm conditions already covered by
  CLRTestTargetUnsupported).
- Remove redundant arm-only GCStressIncompatible from 2 Arm HWIntrinsics
  projects (arm condition already covered by CLRTestTargetUnsupported).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add runtime HeapVerify check to ServerModel test using
  CoreClrConfigurationDetection.IsHeapVerify (the last project using it).
- Remove HeapVerify skip logic from Batch and Bash execution script targets.
- Remove auto-setting of HeapVerifyIncompatible for disasm-check tests in
  CLRTest.Jit.targets.
- Remove all documentation references to HeapVerifyIncompatible.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@MichalStrehovsky
Copy link
Copy Markdown
Member

Before we make more changes to the test tree, it would be nice to make the test tree buildable again. https://github.com/dotnet/runtime/runs/68503855882

I assume this is from #125460, I've seen similar failures when I worked on the Loader tree. It would be nice to re-validate we're still running the same number of tests after #125460, the validation at #125460 (comment) must have missed this.

@jkoritzinsky
Copy link
Copy Markdown
Member

I must have fat fingered the build command and missed the pri1 tests somehow. I'll fix that up before coming back to this.

@jkoritzinsky
Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Copy Markdown
Member

@copilot some of the projects you've edited still mention that they have RequiresProcessIsolation due to GCStressIncompatible. Please audit the projects you've edited to make sure that the reasons for a project being marked RPI are accurate.

… comments

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c882c5b1-c293-4d09-8e31-17947a0bdd1e

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 2, 2026

@copilot some of the projects you've edited still mention that they have RequiresProcessIsolation due to GCStressIncompatible. Please audit the projects you've edited to make sure that the reasons for...

Done in 48bc184. Audited all 78 project files where GCStressIncompatible was removed but the <!-- Needed for GCStressIncompatible, ... --> comment on RequiresProcessIsolation still referenced it. Updated each comment to reflect only the remaining reasons (e.g. UnloadabilityIncompatible, GC.WaitForPendingFinalizers, CMakeProjectReference, etc.). When GCStressIncompatible was the sole reason, the entire comment line was removed.

@jkoritzinsky
Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop

@jkoritzinsky jkoritzinsky marked this pull request as ready for review April 29, 2026 21:28
Copilot AI review requested due to automatic review settings April 29, 2026 21:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request

…lict

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot resolve the merge conflicts in this pull request

Done in 1d065ad. Merged origin/main; the only conflict was in GetTotalAllocatedBytes.cs — I kept the [SkipOnCoreClr] (GC stress) and [ConditionalFact] (HeapVerify on ARM) attributes from our PR, and dropped the [ActiveIssue(..., IsArm)] that main's PR #127583 made obsolete by reducing the thread count on 32-bit.

Copilot AI review requested due to automatic review settings May 8, 2026 03:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@MichalStrehovsky
Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop, runtime-nativeaot-outerloop

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 2 pipeline(s).

@MichalStrehovsky
Copy link
Copy Markdown
Member

@copilot the test src/tests/GC/API/GCHandle/Weak.csproj calls GC.WaitForPendingFinalizers() and is therefore not eligible to drop RequiresProcessIsolation (it also leaks a GCHandle, making it a poor fit for the unisolated runner). Audit all the tests that remove RequiresProcessIsolation=true in this branch against the rules in file docs/workflow/testing/coreclr/requiresprocessisolation.md and make sure we're not dropping RequiresProcessIsolation where we should be keeping it.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/5e94967a-7c80-4e84-9351-ffcf764ab73c

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 8, 2026

@copilot the test src/tests/GC/API/GCHandle/Weak.csproj calls GC.WaitForPendingFinalizers() and is therefore not eligible to drop RequiresProcessIsolation (it also leaks a GCHandle, making it ...

Done in c307c3f. I audited all projects where this branch removed <RequiresProcessIsolation>true</RequiresProcessIsolation> against requiresprocessisolation.md and restored RPI for the remaining matches, including Weak.csproj for GC.WaitForPendingFinalizers. I also restored RPI for largearraytest, b143840, ParallelCrashTester, and R2RDumpTests, and reverted Runtime_72845 to the standalone GCStressIncompatible pattern after the F# XUnit conversion failed targeted builds. Baseline clr+libs and targeted builds passed; representative tests Weak, Runtime_72845, and b143840 passed.

@MichalStrehovsky
Copy link
Copy Markdown
Member

@copilot changes in this branch leave things in non-sensical state.

For example:

src/tests/baseservices/exceptions/regressions/V1/SEH/VJ/MultipleException.csproj‎

Deleted the <GCStressIncompatible>true</GCStressIncompatible> line, left the <!-- Issue #22339 --> comment over it that provides important context for GCStressIncompatible as an orphaned line and failed to add the issue number to the newly added attribute in ‎src/tests/baseservices/exceptions/regressions/V1/SEH/VJ/MultipleException.cs.

Audit all the changes in this branch for this pattern and fix it by moving the important detail from the orphaned comment to the new attribute.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ab28aaa2-5050-4fbd-aed4-0be8580e93f6

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 8, 2026 11:24
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 8, 2026

@copilot changes in this branch leave things in non-sensical state.

For example:
...

Addressed in f4ac6ac. I audited the migrated GCStressIncompatible removals, removed the orphaned project comments, and moved the issue links/rationale into the corresponding SkipOnCoreClr(...AnyGCStress) attribute messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants