Skip to content

Changes to improve SOS test perf#5821

Merged
hoyosjs merged 10 commits intodotnet:mainfrom
hoyosjs:juhoyosa/improve-tests-perf
May 1, 2026
Merged

Changes to improve SOS test perf#5821
hoyosjs merged 10 commits intodotnet:mainfrom
hoyosjs:juhoyosa/improve-tests-perf

Conversation

@hoyosjs
Copy link
Copy Markdown
Member

@hoyosjs hoyosjs commented May 1, 2026

This pull request refactors the Windows dump generation test setup to improve reliability and parallelism. The most significant change is replacing the DumpGenerationFixture xUnit fixture with a static module initializer, ensuring registry keys are set up and cleaned up exactly once per test assembly, which allows tests to run in parallel without serialization. It also splits SOS tests into classes to allow for some level of parallelism (xUnit v2 doesn't support Method level parallelism).

hoyosjs added 6 commits May 1, 2026 01:40
…execution

Replace the xUnit ICollectionFixture pattern with a [ModuleInitializer] that
sets up Windows dump generation registry keys once per assembly load, and
cleans up via AppDomain.ProcessExit.

This removes the [Collection("Windows Dump Generation")] attribute from the
SOS test class, allowing xUnit to run tests in parallel instead of serializing
the entire test suite through a single collection.
Configure xUnit runner with parallelizeTestCollections: true and
maxParallelThreads: -1 (uses processor count) to maximize test
throughput now that the serializing collection has been removed.
Use the in-box System.Text.Json APIs (JsonDocument/JsonElement) for crash
report validation. This removes the Newtonsoft.Json package dependency
since the test project targets net8.0+.
The warning about zero-length array allocations no longer triggers with
the current SDK and xUnit version, making this suppression obsolete.
xUnit 2.x parallelizes across test collections (one per class), not
within a single class. Split the monolithic SOS class into 8 feature-
focused classes so tests run in parallel on 8-16 core machines:

- SOSStackTraceTests (4 tests)
- SOSExceptionTests (5 tests)
- SOSGCTests (5 tests)
- SOSDumpTests (3 tests)
- SOSMethodTests (3 tests)
- SOSThreadingTests (3 tests)
- SOSScenarioTests (4 tests)
- SOSPluginTests (1 test)

Moved the shared Configurations property to SOSTestHelpers so all
classes can reference it via MemberData.
Copilot AI review requested due to automatic review settings May 1, 2026 10:58
@hoyosjs hoyosjs requested a review from a team as a code owner May 1, 2026 10:58
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.

Pull request overview

Refactors SOS unit test execution to improve reliability and enable more parallelism by moving Windows dump-generation setup to one-time assembly initialization and splitting tests across multiple xUnit classes/collections.

Changes:

  • Add xunit.runner.json configuration and copy it to test output to adjust xUnit discovery/execution behavior.
  • Refactor dump-generation registry setup from an xUnit fixture/collection into a [ModuleInitializer] + ProcessExit cleanup.
  • Split the monolithic SOS test class into multiple test classes and add timing instrumentation; migrate crash-report JSON parsing to System.Text.Json and drop Newtonsoft.Json.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/tests/SOS.UnitTests/xunit.runner.json Adds xUnit runner configuration (disables theory pre-enumeration).
src/tests/SOS.UnitTests/SOS.cs Splits tests into multiple classes, adds timing logs, introduces Configurations helper, and switches crash-report parsing to System.Text.Json.
src/tests/SOS.UnitTests/SOS.UnitTests.csproj Ensures xunit.runner.json is copied to output; removes Newtonsoft.Json package reference.
src/tests/SOS.UnitTests/DumpGenerationFixture.cs Replaces xUnit fixture/collection with module-initializer-based registry setup and ProcessExit cleanup.
Comments suppressed due to low confidence (1)

src/tests/SOS.UnitTests/SOS.cs:259

  • TestCrashReport catches all exceptions (including assertion failures) and only logs them, which means crash-report validation can silently pass even when required properties are missing/invalid (e.g., GetProperty(...).GetString() returning null or GetProperty throwing). To keep the extra logging while still failing the test, rethrow after logging (similar to LLDBPluginTests) and/or use TryGetProperty + explicit null assertions for required string values.
            using JsonDocument crashReport = JsonDocument.Parse(File.ReadAllText(crashReportPath));
            JsonElement root = crashReport.RootElement;

            Assert.True(root.TryGetProperty("payload", out JsonElement payload));
            Version protocol_version = Version.Parse(payload.GetProperty("protocol_version").GetString());
            Assert.True(protocol_version >= new Version("1.0.0"));
            outputHelper.IndentedOutput.WriteLine($"protocol_version {protocol_version}");

            string process_name = payload.GetProperty("process_name").GetString();
            Assert.NotNull(process_name);
            outputHelper.IndentedOutput.WriteLine($"process_name {process_name}");

            Assert.True(payload.TryGetProperty("threads", out JsonElement threads));
            int threadCount = threads.GetArrayLength();
            Assert.True(threadCount > 0);
            outputHelper.IndentedOutput.WriteLine($"threads # {threadCount}");

            if (OS.Kind == OSKind.OSX)
            {
                Assert.True(root.TryGetProperty("parameters", out JsonElement parameters));
                Assert.True(parameters.TryGetProperty("ExceptionType", out _));
                Assert.True(parameters.TryGetProperty("OSVersion", out _));
                Assert.Equal("apple", parameters.GetProperty("SystemManufacturer").GetString());
            }
        }
        catch (Exception ex)
        {
            // Log the exception
            outputHelper.IndentedOutput.WriteLine(ex.ToString());
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/tests/SOS.UnitTests/SOS.cs
Comment thread src/tests/SOS.UnitTests/DumpGenerationFixture.cs
@hoyosjs hoyosjs merged commit 7bc1e19 into dotnet:main May 1, 2026
19 checks passed
@hoyosjs hoyosjs deleted the juhoyosa/improve-tests-perf branch May 1, 2026 21:39
max-charlamb added a commit to dotnet/runtime that referenced this pull request May 6, 2026
…card support (#127878)

> [!NOTE]
> This PR was created with the assistance of GitHub Copilot
(AI-generated content).

## Problem

[dotnet/diagnostics PR
#5821](dotnet/diagnostics#5821) split the single
``SOS`` test class into 11 ``SOS``-prefixed classes
(``SOSStackTraceTests``, ``SOSExceptionTests``, ``SOSGCTests``,
``SOSDumpTests``, etc.) to enable parallelism. The existing
``classFilter: SOS`` in ``runtime-diagnostics.yml`` no longer matches
any class, so the cDAC, cDAC_no_fallback, and DAC test legs are now
silently running **zero** tests.

A naïve fix to ``classFilter: SOS*`` does not work either: xunit v2's
``-class`` filter is a ``HashSet<string>`` with exact-match
``Contains()`` only; it has no wildcard support. I verified this by
directly invoking ``XunitFilters.Filter()`` against
``xunit.runner.utility`` 2.9.2:

| Filter | Matches ``SOSStackTraceTests.TestMethod1`` |
|--------|--------------------------------------------|
| ``-class SOS*`` | ❌ No (literal HashSet contains) |
| **``-method SOS*``** | ✅ **Yes** |
| Multiple ``-class`` entries | ✅ Yes (would require enumerating all 11)
|

## Fix

xunit v2's ``-method`` filter is regex-based and operates on the fully
qualified ``ClassName.MethodName``, so ``SOS*`` matches every method on
any SOS-prefixed class.

This PR:

1. Adds a ``methodFilter`` parameter to
``eng/pipelines/diagnostics/runtime-diag-job.yml`` (parallel to the
existing ``classFilter``), wiring it through to the diagnostics
``build.ps1``'s already-supported ``-methodfilter`` argument.
2. Switches the three SOS test legs in
``eng/pipelines/runtime-diagnostics.yml`` (cDAC, cDAC_no_fallback, DAC)
to ``methodFilter: SOS*``.

The existing ``classFilter`` parameter is left in place for any other
consumer that needs exact-match class filtering.

Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants