Repository navigation
Conversation
… to different test testing similar things
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Removing the theory leaves explicit assignment of the Default mode untested.
Review effort: Balanced
Findings: 1
What changed in this PR
Removes a flaky shared-process GC setting test and shifts validation to an isolated LOH compaction test.
Changes:
- Removes the parallel-run-sensitive round-trip theory.
- Adds LOH mode checks and corrects an expected-mode diagnostic.
| File | Description |
|---|---|
lohcompactapi2.cs |
Adds compaction-mode validation. |
GCTests.cs |
Removes the flaky round-trip test. |
|
Tagging subscribers to this area: @anicka-net, @dotnet/gc |
| @@ -80,6 +85,11 @@ public static bool Test1() | |||
| int GCCount = 0; | |||
| int initialGCCount = GetBlockingGen2Count(); | |||
| GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce; | |||
| if (GCSettings.LargeObjectHeapCompactionMode != GCLargeObjectHeapCompactionMode.CompactOnce) | |||
There was a problem hiding this comment.
There is a race condition still. While threads allocating, blocking full GC can occur between the CompactOnce setter and getter. In that case, the getter returns Default, and the new code fails the test.
| @@ -128,12 +138,36 @@ public static bool Test2() | |||
|
|
|||
| Console.WriteLine("Setting GCLargeObjectHeapCompactionMode.CompactOnce"); | |||
| GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce; | |||
| if (GCSettings.LargeObjectHeapCompactionMode != GCLargeObjectHeapCompactionMode.CompactOnce) | |||
| @@ -24,6 +24,11 @@ public static int Main(string[] args) | |||
| int retVal = 100; | |||
| int iterations = 10; | |||
|
|
|||
| if (!TestRoundTrip()) | |||
| { | |||
| return false; | |||
There was a problem hiding this comment.
Will be warning or error? Method expects int.
Remove test sensitive to parallel test run and move checked invariants to different test testing similar things.
Fix #134997