Cleanup dotnet test #49734
Cleanup dotnet test #49734
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how test modules and applications construct their RunProperties, removes legacy fallback logic in RunProperties, and adds validation to ensure test commands are executable.
- Introduces conditional creation of
RunPropertiesinTestModulesFilterHandler - Simplifies argument building in
TestApplicationand centralizes run command logic - Adds validation for
RunProperties.RunCommandinSolutionAndProjectUtilityand removes fallback inRunProperties
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Test/TestModulesFilterHandler.cs | New conditional logic for DLL vs. executable modules |
| src/Cli/dotnet/Commands/Test/TestApplication.cs | Consolidated process start info and removed old argument helpers |
| src/Cli/dotnet/Commands/Test/SolutionAndProjectUtility.cs | Throws when run command is missing or a DLL, no longer falls back |
| src/Cli/dotnet/Commands/Run/RunProperties.cs | Removed fallback-to-target-path logic |
| src/Cli/dotnet/Commands/Run/RunCommand.cs | Updated to call FromProjectAndApplicationArguments without fallback |
Comments suppressed due to low confidence (2)
src/Cli/dotnet/Commands/Test/SolutionAndProjectUtility.cs:233
- New exception path for missing or DLL
RunCommandshould be covered by unit tests to ensure correct behavior when encountering libraries or empty commands.
if (string.IsNullOrEmpty(runProperties.RunCommand) || runProperties.RunCommand.HasExtension(CliConstants.DLLExtension))
src/Cli/dotnet/Commands/Test/TestApplication.cs:93
- If
Module.RunProperties.RunArgumentsis null,new StringBuilder(null)will throw an exception. Initialize withModule.RunProperties.RunArguments ?? string.Emptyto guard against null.
StringBuilder builder = new(Module.RunProperties.RunArguments);
| RunProperties runProperties = testModule.HasExtension(CliConstants.DLLExtension) | ||
| ? new RunProperties(new Muxer().MuxerPath, $@"exec ""{testModule}""", null) | ||
| : new RunProperties(testModule, null, null); | ||
|
|
||
| var testApp = new ParallelizableTestModuleGroupWithSequentialInnerModules(new TestModule(runProperties, null, null, true, true, null)); |
There was a problem hiding this comment.
Instead of letting TestApplication class be concerned about whether the data is coming through module filter (by checking if it's dll), I think it's better to just provide the correct RunProperties from the beginning.
Note that I'm here now using new Muxer().MuxerPath, where previously we used Environment.ProcessPath. @elinor-fung @baronfel @dsplaisted Can you please clarify when new Muxer().MuxerPath is different from Environment.ProcessPath and which one is the correct one to use?
There was a problem hiding this comment.
There was a problem hiding this comment.
I think the cases where it is different is when we are running tests of the .NET CLI itself, so dotnet.dll gets loaded as a dependency of our tests instead of as the main application.
They might also be different if the new paths feature of global.json is used, in that case I think the process path could be under program files, while it would load a local copy of dotnet.dll and so the MuxerPath would be relative to that.
I'm not sure if there are other scenarios where it would be different.
There was a problem hiding this comment.
@dsplaisted Hmm, so for the purpose of running an assembly using dotnet exec while running dotnet test, which one of the two should be the right one?
No description provided.