Cleanup dotnet test (#49734)

Author: Evangelink

src/Cli/dotnet/Commands/Run/RunCommand.cs

@@ -364,7 +364,7 @@ static void ValidatePreconditions(ProjectInstance project)
 
         static RunProperties ReadRunPropertiesFromProject(ProjectInstance project, string[] applicationArgs)
         {
-            var runProperties = RunProperties.FromProjectAndApplicationArguments(project, applicationArgs, fallbackToTargetPath: false);
+            var runProperties = RunProperties.FromProjectAndApplicationArguments(project, applicationArgs);
             if (string.IsNullOrEmpty(runProperties.RunCommand))
             {
                 ThrowUnableToRunError(project);

src/Cli/dotnet/Commands/Run/RunProperties.cs

@@ -8,18 +8,9 @@ namespace Microsoft.DotNet.Cli.Commands.Run;
 
 internal record RunProperties(string RunCommand, string? RunArguments, string? RunWorkingDirectory)
 {
-    internal static RunProperties FromProjectAndApplicationArguments(ProjectInstance project, string[] applicationArgs, bool fallbackToTargetPath)
+    internal static RunProperties FromProjectAndApplicationArguments(ProjectInstance project, string[] applicationArgs)
     {
         string runProgram = project.GetPropertyValue("RunCommand");
-        if (fallbackToTargetPath &&
-            (string.IsNullOrEmpty(runProgram) || !File.Exists(runProgram)))
-        {
-            // If we can't find the executable that runCommand is pointing to, we simply use TargetPath instead.
-            // In this case, we discard everything related to "Run" (i.e, RunWorkingDirectory and RunArguments) and use only TargetPath
-            runProgram = project.GetPropertyValue("TargetPath");
-            return new(runProgram, null, null);
-        }
-
         string runArguments = project.GetPropertyValue("RunArguments");
         string runWorkingDirectory = project.GetPropertyValue("RunWorkingDirectory");
 

src/Cli/dotnet/Commands/Test/Models.cs

@@ -111,7 +111,8 @@ public bool MoveNext()
     }
 }
 
-internal sealed record TestModule(RunProperties RunProperties, string? ProjectFullPath, string? TargetFramework, bool IsTestingPlatformApplication, bool IsTestProject, ProjectLaunchSettingsModel? LaunchSettings);
+
+internal sealed record TestModule(RunProperties RunProperties, string? ProjectFullPath, string? TargetFramework, bool IsTestingPlatformApplication, bool IsTestProject, ProjectLaunchSettingsModel? LaunchSettings, string TargetPath);
 
 internal sealed record Handshake(Dictionary<byte, string>? Properties);
 

src/Cli/dotnet/Commands/Test/SolutionAndProjectUtility.cs

@@ -230,12 +230,27 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
 
         string targetFramework = project.GetPropertyValue(ProjectProperties.TargetFramework);
         RunProperties runProperties = GetRunProperties(project, loggers);
+
+        // dotnet run throws the same if RunCommand is null or empty.
+        // In dotnet test, we are additionally checking that RunCommand is not dll.
+        // In any "default" scenario, RunCommand is never dll.
+        // If we found it to be dll, that is user explicitly setting RunCommand incorrectly.
+        if (string.IsNullOrEmpty(runProperties.RunCommand) || runProperties.RunCommand.HasExtension(CliConstants.DLLExtension))
+        {
+            throw new GracefulException(
+                string.Format(
+                    CliCommandStrings.RunCommandExceptionUnableToRun,
+                    "dotnet test",
+                    "OutputType",
+                    project.GetPropertyValue("OutputType")));
+        }
+
         string projectFullPath = project.GetPropertyValue(ProjectProperties.ProjectFullPath);
 
         // TODO: Support --launch-profile and pass it here.
         var launchSettings = TryGetLaunchProfileSettings(Path.GetDirectoryName(projectFullPath)!, project.GetPropertyValue(ProjectProperties.AppDesignerFolder), noLaunchProfile, profileName: null);
 
-        return new TestModule(runProperties, PathUtility.FixFilePath(projectFullPath), targetFramework, isTestingPlatformApplication, isTestProject, launchSettings);
+        return new TestModule(runProperties, PathUtility.FixFilePath(projectFullPath), targetFramework, isTestingPlatformApplication, isTestProject, launchSettings, project.GetPropertyValue(ProjectProperties.TargetPath));
 
         static RunProperties GetRunProperties(ProjectInstance project, ICollection<ILogger>? loggers)
         {
@@ -247,12 +262,11 @@ static RunProperties GetRunProperties(ProjectInstance project, ICollection<ILogg
             {
                 if (!project.Build(s_computeRunArgumentsTarget, loggers: null))
                 {
-                    Logger.LogTrace(() => $"The target {s_computeRunArgumentsTarget} failed to build. Falling back to TargetPath.");
-                    return new RunProperties(project.GetPropertyValue(ProjectProperties.TargetPath), null, null);
+                    throw new GracefulException(CliCommandStrings.RunCommandEvaluationExceptionBuildFailed, s_computeRunArgumentsTarget);
                 }
             }
 
-            return RunProperties.FromProjectAndApplicationArguments(project, [], fallbackToTargetPath: true);
+            return RunProperties.FromProjectAndApplicationArguments(project, []);
         }
     }
 

src/Cli/dotnet/Commands/Test/TestApplication.cs

@@ -8,7 +8,6 @@
 using Microsoft.DotNet.Cli.Commands.Test.IPC;
 using Microsoft.DotNet.Cli.Commands.Test.IPC.Models;
 using Microsoft.DotNet.Cli.Commands.Test.IPC.Serializers;
-using Microsoft.DotNet.Cli.Utils;
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
 
@@ -54,12 +53,13 @@ public async Task<int> RunAsync(TestOptions testOptions)
 
     private ProcessStartInfo CreateProcessStartInfo(TestOptions testOptions)
     {
-        bool isDll = Module.RunProperties.RunCommand.HasExtension(CliConstants.DLLExtension);
-
         var processStartInfo = new ProcessStartInfo
         {
-            FileName = GetFileName(testOptions, isDll),
-            Arguments = GetArguments(testOptions, isDll),
+            // We should get correct RunProperties right away.
+            // For the case of dotnet test --test-modules path/to/dll, the TestModulesFilterHandler is responsible
+            // for providing the dotnet muxer as RunCommand, and `exec "path/to/dll"` as RunArguments.
+            FileName = Module.RunProperties.RunCommand,
+            Arguments = GetArguments(testOptions),
             RedirectStandardOutput = true,
             RedirectStandardError = true
         };
@@ -87,23 +87,27 @@ private ProcessStartInfo CreateProcessStartInfo(TestOptions testOptions)
         return processStartInfo;
     }
 
-    private string GetFileName(TestOptions testOptions, bool isDll)
-        => isDll ? Environment.ProcessPath : Module.RunProperties.RunCommand;
-
-    private string GetArguments(TestOptions testOptions, bool isDll)
+    private string GetArguments(TestOptions testOptions)
     {
-        if (testOptions.HasFilterMode || !isDll || !IsArchitectureSpecified(testOptions))
+        // Keep RunArguments first.
+        // In the case of UseAppHost=false, RunArguments is set to `exec $(TargetPath)`:
+        // https://github.com/dotnet/sdk/blob/333388c31d811701e3b6be74b5434359151424dc/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1411
+        // So, we keep that first always.
+        StringBuilder builder = new(Module.RunProperties.RunArguments);
+
+        if (testOptions.IsHelp)
         {
-            return BuildArgs(testOptions, isDll);
+            builder.Append($" {TestingPlatformOptions.HelpOption.Name} ");
         }
 
-        // If we reach here, that means we have a test project that doesn't produce an executable.
-        throw new InvalidOperationException($"A Microsoft.Testing.Platform test project should produce an executable. The file '{Module.RunProperties.RunCommand}' is dll.");
-    }
+        var args = _buildOptions.UnmatchedTokens;
+        builder.Append(args.Count != 0
+            ? args.Aggregate((a, b) => $"{a} {b}")
+            : string.Empty);
 
-    private static bool IsArchitectureSpecified(TestOptions testOptions)
-    {
-        return !string.IsNullOrEmpty(testOptions.Architecture);
+        builder.Append($" {CliConstants.ServerOptionKey} {CliConstants.ServerOptionValue} {CliConstants.DotNetTestPipeOptionKey} {_pipeNameDescription.Name}");
+
+        return builder.ToString();
     }
 
     private void WaitOnTestApplicationPipeConnectionLoop()
@@ -277,35 +281,6 @@ private bool ModulePathExists()
         return true;
     }
 
-    private string BuildArgs(TestOptions testOptions, bool isDll)
-    {
-        StringBuilder builder = new();
-
-        if (isDll)
-        {
-            builder.Append($"exec {Module.RunProperties.RunCommand} ");
-        }
-
-        AppendCommonArgs(builder, testOptions);
-
-        return builder.ToString();
-    }
-
-    private void AppendCommonArgs(StringBuilder builder, TestOptions testOptions)
-    {
-        if (testOptions.IsHelp)
-        {
-            builder.Append($" {TestingPlatformOptions.HelpOption.Name} ");
-        }
-
-        var args = _buildOptions.UnmatchedTokens;
-        builder.Append(args.Count != 0
-            ? args.Aggregate((a, b) => $"{a} {b}")
-            : string.Empty);
-
-        builder.Append($" {CliConstants.ServerOptionKey} {CliConstants.ServerOptionValue} {CliConstants.DotNetTestPipeOptionKey} {_pipeNameDescription.Name} {Module.RunProperties.RunArguments}");
-    }
-
     public void OnHandshakeMessage(HandshakeMessage handshakeMessage)
     {
         HandshakeReceived?.Invoke(this, new HandshakeArgs { Handshake = new Handshake(handshakeMessage.Properties) });

src/Cli/dotnet/Commands/Test/TestApplicationEventHandlers.cs

@@ -26,7 +26,7 @@ public void OnHandshakeReceived(object sender, HandshakeArgs args)
             var executionId = args.Handshake.Properties[HandshakeMessagePropertyNames.ExecutionId];
             var arch = args.Handshake.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower();
             var tfm = TargetFrameworkParser.GetShortTargetFramework(args.Handshake.Properties[HandshakeMessagePropertyNames.Framework]);
-            (string ModulePath, string TargetFramework, string Architecture, string ExecutionId) appInfo = new(testApplication.Module.RunProperties.RunCommand, tfm, arch, executionId);
+            (string ModulePath, string TargetFramework, string Architecture, string ExecutionId) appInfo = new(testApplication.Module.TargetPath, tfm, arch, executionId);
             _executions[testApplication] = appInfo;
             _output.AssemblyRunStarted(appInfo.ModulePath, appInfo.TargetFramework, appInfo.Architecture, appInfo.ExecutionId);
         }
@@ -147,7 +147,7 @@ public void OnTestProcessExited(object sender, TestProcessExitEventArgs args)
         }
         else
         {
-            _output.AssemblyRunCompleted(testApplication.Module.RunProperties.RunCommand ?? testApplication.Module.ProjectFullPath, testApplication.Module.TargetFramework, architecture: null, null, args.ExitCode, string.Join(Environment.NewLine, args.OutputData), string.Join(Environment.NewLine, args.ErrorData));
+            _output.AssemblyRunCompleted(testApplication.Module.TargetPath ?? testApplication.Module.ProjectFullPath, testApplication.Module.TargetFramework, architecture: null, null, args.ExitCode, string.Join(Environment.NewLine, args.OutputData), string.Join(Environment.NewLine, args.ErrorData));
         }
 
         LogTestProcessExit(args);

src/Cli/dotnet/Commands/Test/TestModulesFilterHandler.cs

@@ -7,6 +7,7 @@
 using Microsoft.DotNet.Cli.Commands.Run;
 using Microsoft.DotNet.Cli.Commands.Test.Terminal;
 using Microsoft.DotNet.Cli.Extensions;
+using Microsoft.DotNet.Cli.Utils;
 using Microsoft.Extensions.FileSystemGlobbing;
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
@@ -47,9 +48,17 @@ public bool RunWithTestModulesFilter(ParseResult parseResult)
             return false;
         }
 
+        var muxerPath = new Muxer().MuxerPath;
         foreach (string testModule in testModulePaths)
         {
-            var testApp = new ParallelizableTestModuleGroupWithSequentialInnerModules(new TestModule(new RunProperties(testModule, null, null), null, null, true, true, null));
+            // We want to produce the right RunCommand and RunArguments for TestApplication implementation to consume directly.
+            // We don't want TestApplication class to be concerned about whether it's running dll via test module or not.
+            // If we are given dll, we use dotnet exec. Otherwise, we run the executable directly.
+            RunProperties runProperties = testModule.HasExtension(CliConstants.DLLExtension)
+                ? new RunProperties(muxerPath, $@"exec ""{testModule}""", null)
+                : new RunProperties(testModule, null, null);
+
+            var testApp = new ParallelizableTestModuleGroupWithSequentialInnerModules(new TestModule(runProperties, null, null, true, true, null, testModule));
             // Write the test application to the channel
             _actionQueue.Enqueue(testApp);
         }