Split runfile tests up to improve helix usage#53452
Open
marcpopMSFT wants to merge 4 commits intomainfrom
Open
Split runfile tests up to improve helix usage#53452marcpopMSFT wants to merge 4 commits intomainfrom
marcpopMSFT wants to merge 4 commits intomainfrom
Conversation
RunFileTests was a single 6,210-line class with ~150 IL methods. The AssemblyScheduler (methodLimit=16) cannot split within a class, so the entire class landed in one Helix shard (shard 19) taking ~42 minutes while all other dotnet.Tests.dll shards completed in under 8 minutes. Split into 5 test classes inheriting from a shared RunFileTestBase: - RunFileTests (31 tests): path resolution, precedence, stdin, multifile - RunFileTests_BuildOptions (28 tests): working dir, Dir.Build.props, arguments, binary log, verbosity, embedded resources - RunFileTests_BuildCommands (28 tests): restore, build, publish, pack, clean, launch profiles - RunFileTests_Directives (24 tests): defines, package/SDK/project refs, include directive, user secrets, CSC arguments - RunFileTests_CscOnlyAndApi (42 tests): up-to-date checks, csc-only mode, API tests, entry points, MSBuild get RunFileTestBase holds shared static fields (changed from private to internal), helper methods (DirectiveError, VerifyBinLogEvaluationDataCount), and the Build() instance method used across multiple test classes. Total test count preserved: 153 methods (123 Fact + 27 Theory + 3 PlatformSpecificFact). Expected impact: the single 42-minute shard should now distribute across ~5 shards of ~8-10 minutes each, reducing the critical path of TestBuild: linux (x64) by ~30 minutes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR splits the previously large RunFileTests coverage into multiple smaller xUnit classes to improve Helix sharding (since the scheduler won’t split a single test class across shards), and adds a short write-up documenting the pipeline/sharding motivation.
Changes:
- Introduces
RunFileTestBaseto centralize shared helpers/test constants used across the split test classes. - Moves RunFileTests coverage into separate classes (
*_BuildOptions,*_BuildCommands,*_Directives,*_CscOnlyAndApi) to reduce shard imbalance. - Adds
documentation/general/pr-build-performance-analysis.mdexplaining the Helix shard imbalance and expected impact.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs | Adds directive-focused tests (defines/package/sdk/project refs, include, user-secrets, CSC argument equivalence). |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs | Adds CSC-only, up-to-date, and run-api/MSBuild-get related tests. |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs | Adds tests for run/build options behavior (working dir, args passthrough, binlog/verbosity, resources). |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildCommands.cs | Adds tests for build-command behaviors (restore/build/publish/pack/clean/launch profiles). |
| test/dotnet.Tests/CommandTests/Run/RunFileTestBase.cs | New shared base class holding helpers/constants used by the split test classes. |
| documentation/general/pr-build-performance-analysis.md | Documents the Helix shard imbalance and the rationale/expected impact of splitting RunFileTests. |
test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
Outdated
Show resolved
Hide resolved
…ildperf # Conflicts: # test/dotnet.Tests/CommandTests/Run/RunFileTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See the .md file included in this PR. I asked copilot to do an analysis of our PR builds to look for easy wins. This seemed like the lowest risk change to initially make to improve some of the PR times. Splitting the test classes by time would be better but that's more complicated to implement. I'll see if there are other recommendations that are low hanging fruit to attempt.
Problem:
RunFileTestswas a single class with ~150 IL methods. The scheduler placed it entirely in shard 19 alongsideRunCommandTests(4 methods), creating a 42-minute shard while all other dotnet.Tests.dll shards completed in under 8 minutes.