Switch HelixTestRunner from dotnet test to dotnet vstest#66896
Conversation
Both delegate to vstest.console, but dotnet vstest accepts multiple DLL paths (needed for batching) and has a more predictable arg format. Already used for discovery. Deduplicates quarantine branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at. |
|
@wtgodbe This is in prep for the batching changes. I wanted to break them down into smaller chunks, and switching to vstest allows for passing of multiple test dlls at once. |
The dotnet test --blame-hang-timeout 15m flag kills individual hung tests after 15 minutes and collects a dump. Without it, a single hung test blocks the entire work item until the Helix timeout (45 min) with no dump for diagnosis. The vstest equivalent is --blame:CollectDump;CollectHangDump;TestTimeout=15min. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Not yet ready. |
There was a problem hiding this comment.
Pull request overview
This PR updates the HelixTestRunner’s test execution path to invoke dotnet vstest instead of dotnet test, aligning execution with the already-used discovery approach and enabling future batching scenarios.
Changes:
- Switches test execution to
dotnet vstestwith mapped arguments (--testcasefilter,--diag,--logger, blame options). - Deduplicates quarantined vs non-quarantined execution logic by using a shared filter variable.
The original code intentionally did not set exitCode when quarantined tests failed — quarantined tests are expected to fail and should not cause the Helix work item to be marked as failed. The refactor accidentally started propagating the exit code for both cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // (needed for future batching) and has a more predictable argument format. | ||
| // Argument mapping: --filter → --testcasefilter, --blame-crash → --blame:CollectDump | ||
| var commonTestArgs = $"vstest {Options.Target} --diag:\"{diagLog}\" --logger:xunit --logger:\"console;verbosity=normal\" " + | ||
| "--blame:CollectDump;CollectHangDump;TestTimeout=15min"; |
There was a problem hiding this comment.
Validated locally that these work.
Youssef1313
left a comment
There was a problem hiding this comment.
I'm not sure this is good long-term.
@nohwnd What's the current status of dotnet vstest?
https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-vstest
The dotnet vstest command is superseded by dotnet test, which can now be used to run assemblies. See dotnet test.
Should I read this as "kinda deprecated"?
|
I would like to remove All that dotnet vstest can do, dotnet test should be able to do as well. As soon as you give it a single (or more) dll parameters it will switch to "dotnet vstest" mode. Apart from the issues listed in the description is there anything else?
Since you talk about discovering tests? What is the task here that is being achieved? If you are orchestrating test runs then maybe running via translation layer (like VS does it) might provide more power to what you are doing. And give us more push to implement things like sharding of test runs based on historical results, to optimized utilization of servers etc. |
Ahh interesting! The models do NOT think that I'm not sure what the purpose of the discovery is. |
|
@wtgodbe Do you happen to know what the discovery is for? |
|
Note that roslyn uses |
|
👍 we did not do any analysis on impact of removing vstest recently, so no hurry. I expect many places still using it. There should be some telemetry when revisiting. |
Switch HelixTestRunner from
dotnet testtodotnet vstestWhy
Both commands delegate to
vstest.consoleinternally, butdotnet vstest:CheckTestDiscoveryAsync(dotnet vstest {target} -lt)Argument mapping
dotnet testdotnet vstest--filter "expr"--testcasefilter:"expr"--blame-crash --blame-hang-timeout 15m--blame:CollectDump--logger name--logger:name--diag:path--diag:"path"Other changes
if/elsebranches into a shared filter variable