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

Rework our Helix Process #76703

Merged
merged 4 commits into from
Jan 11, 2025
Merged

Rework our Helix Process #76703

merged 4 commits into from
Jan 11, 2025

Conversation

jaredpar
Copy link
Member

@jaredpar jaredpar commented Jan 10, 2025

Suggest reviewing this commit by commit:

  1. This refactors RunTests so that the Helix logic is isolated from the traditional test runner logic
  2. This significantly revamps how our helix tests are executed: files now uploaded as artifacts for debugging, use scripts instead of escaped msbuild commands and slim down the work item payloads.

The most impactful change is the slimming down of work item payloads. They moved down in size from 1.8GB to ~100MB. That moved the time to download + unpack on Windows from one minute to one second. This action occurs on every work item so the time savings is multiplied by the number of work items in every job (a lot)

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 10, 2025
@jaredpar jaredpar force-pushed the helix7 branch 7 times, most recently from eef88e7 to ac11040 Compare January 10, 2025 07:41
@jaredpar jaredpar mentioned this pull request Jan 10, 2025
@jaredpar jaredpar force-pushed the helix7 branch 4 times, most recently from 3476088 to 17e1c94 Compare January 10, 2025 18:26
The goal of this change is to cleanly separate out the two behaviors of
RunTests: locally executing tests and scheduling jobs on Helix. This is
a pre-cursor to actually moving these behaviors into separate projects
entirely.
@jaredpar jaredpar changed the title Optimize Helix Work Item Payload Rework our Helix Process Jan 10, 2025
@jaredpar jaredpar marked this pull request as ready for review January 10, 2025 19:40
@jaredpar jaredpar requested a review from a team as a code owner January 10, 2025 19:40
src/Tools/Source/RunTests/HelixTestRunner.cs Show resolved Hide resolved
builder.AppendLine($"""
<HelixWorkItem Include="{helixWorkItem.DisplayName}">
<PayloadDirectory>{workItemPayloadDir}</PayloadDirectory>
<Command>{commandPrefix}{commandFileName}</Command>
Copy link
Member

Choose a reason for hiding this comment

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

much nicer 🥳

src/Tools/Source/RunTests/HelixTestRunner.cs Show resolved Hide resolved
src/Tools/Source/RunTests/HelixTestRunner.cs Show resolved Hide resolved
@RikkiGibson RikkiGibson self-assigned this Jan 10, 2025
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

still need to finish reviewing HelixTestRunner.cs. Should I attempt to compare contents with the bits that were deleted from TestRunner.cs?

edit: just saw Suggest reviewing this commit by commit. So I'll try that.

src/Tools/Source/RunTests/HelixTestRunner.cs Outdated Show resolved Hide resolved
src/Tools/Source/RunTests/HelixTestRunner.cs Outdated Show resolved Hide resolved
src/Tools/Source/RunTests/HelixTestRunner.cs Outdated Show resolved Hide resolved
src/Tools/Source/RunTests/ProcessTestExecutor.cs Outdated Show resolved Hide resolved
@jaredpar jaredpar merged commit 6287206 into dotnet:main Jan 11, 2025
25 checks passed
@jaredpar jaredpar deleted the helix7 branch January 11, 2025 00:15
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jan 11, 2025
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM

333fred added a commit to 333fred/roslyn that referenced this pull request Jan 14, 2025
* upstream/main: (368 commits)
  Cleanup tests
  Add test
  Add test
  Add test
  Fix issue parsing regex category
  Properly simplify pattern when converting to pattern matching
  update publishdata after VS 17.13 snap
  Simplify
  Docs
  Do not lift type parameters in extract method declared within the selected region
  Fix ExtractMethod in VB elseif blocks
  Rework our Helix Process (dotnet#76703)
  Stash and restore original culture in CultureNormalizer (dotnet#76713)
  PR comments
  Adding checks for mutable structs.
  Add additional tests for string escape sequences
  CodeGenerator.EmitStackAlloc - Avoid capturing blob array (dotnet#76660)
  Update comments and exception type for LSP stdio configuration based on review feedback
  Fix race generating Microsoft.Managed.Core.CurrentVersions.targets (dotnet#76701)
  Update FileDownloader.cs
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants