Cleanup test command (#51883)

Author: Youssef1313

src/Cli/dotnet/Commands/Test/MTP/MSBuildUtility.cs

@@ -153,7 +153,7 @@ private static bool BuildOrRestoreProjectOrSolution(string filePath, BuildOption
             msbuildArgs.Add($"-verbosity:quiet");
         }
 
-        var parsedMSBuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(msbuildArgs, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, TestCommandDefinition.MTPTargetOption, TestCommandDefinition.VerbosityOption, CommonOptions.NoLogoOption());
+        var parsedMSBuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(msbuildArgs, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, MicrosoftTestingPlatformOptions.MTPTargetOption, TestCommandDefinition.VerbosityOption, CommonOptions.NoLogoOption());
 
         int result = new RestoringCommand(parsedMSBuildArgs, buildOptions.HasNoRestore).Execute();
 

src/Cli/dotnet/Commands/Test/MTP/MicrosoftTestingPlatformOptions.cs

@@ -66,10 +66,6 @@ internal static class MicrosoftTestingPlatformOptions
         HelpName = CliCommandStrings.CmdNumberName
     };
 
-    public static readonly Option<string?> ConfigurationOption = CommonOptions.ConfigurationOption(CliCommandStrings.TestConfigurationOptionDescription);
-
-    public static readonly Option<string> FrameworkOption = CommonOptions.FrameworkOption(CliCommandStrings.TestFrameworkOptionDescription);
-
     public static readonly Option<bool> NoBuildOption = new("--no-build")
     {
         Description = CliCommandStrings.CmdNoBuildDescription
@@ -109,6 +105,8 @@ internal static class MicrosoftTestingPlatformOptions
         Description = CliCommandStrings.CmdListTestsDescription,
         Arity = ArgumentArity.Zero
     };
+
+    public static readonly Option<string[]> MTPTargetOption = CommonOptions.RequiredMSBuildTargetOption(CliConstants.MTPTarget);
 }
 
 internal enum OutputOptions

src/Cli/dotnet/Commands/Test/MTP/ValidationUtility.cs

@@ -40,8 +40,8 @@ static void ValidateOptionsIrrelevantToModulesFilter(ParseResult parseResult)
             }
 
             if (parseResult.HasOption(CommonOptions.ArchitectureOption) ||
-                parseResult.HasOption(MicrosoftTestingPlatformOptions.ConfigurationOption) ||
-                parseResult.HasOption(MicrosoftTestingPlatformOptions.FrameworkOption) ||
+                parseResult.HasOption(TestCommandDefinition.ConfigurationOption) ||
+                parseResult.HasOption(TestCommandDefinition.FrameworkOption) ||
                 parseResult.HasOption(CommonOptions.OperatingSystemOption) ||
                 parseResult.HasOption(CommonOptions.RuntimeOptionName))
             {

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

@@ -5,7 +5,6 @@
 using System.Text.Json;
 using System.Text.Json.Serialization;
 using Microsoft.DotNet.Cli.CommandLine;
-using Microsoft.DotNet.Cli.Extensions;
 using Command = System.CommandLine.Command;
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
@@ -33,145 +32,11 @@ private sealed class GlobalJsonTestNode
     public const string Name = "test";
     public static readonly string DocsLink = "https://aka.ms/dotnet-test";
 
-    public static readonly Option<string> SettingsOption = new Option<string>("--settings", "-s")
-    {
-        Description = CliCommandStrings.CmdSettingsDescription,
-        HelpName = CliCommandStrings.CmdSettingsFile
-    }.ForwardAsSingle(o => $"-property:VSTestSetting={SurroundWithDoubleQuotes(CommandDirectoryContext.GetFullPath(o))}");
-
-    public static readonly Option<bool> ListTestsOption = new Option<bool>("--list-tests", "-t")
-    {
-        Description = CliCommandStrings.CmdListTestsDescription,
-        Arity = ArgumentArity.Zero
-    }.ForwardAs("-property:VSTestListTests=true");
-
-    public static readonly Option<string> FilterOption = new Option<string>("--filter")
-    {
-        Description = CliCommandStrings.CmdTestCaseFilterDescription,
-        HelpName = CliCommandStrings.CmdTestCaseFilterExpression
-    }.ForwardAsSingle(o => $"-property:VSTestTestCaseFilter={SurroundWithDoubleQuotes(o!)}");
-
-    public static readonly Option<IEnumerable<string>> AdapterOption = new Option<IEnumerable<string>>("--test-adapter-path")
-    {
-        Description = CliCommandStrings.CmdTestAdapterPathDescription,
-        HelpName = CliCommandStrings.CmdTestAdapterPath
-    }.ForwardAsSingle(o => $"-property:VSTestTestAdapterPath={SurroundWithDoubleQuotes(string.Join(";", o!.Select(CommandDirectoryContext.GetFullPath)))}")
-    .AllowSingleArgPerToken();
-
-    public static readonly Option<IEnumerable<string>> LoggerOption = new Option<IEnumerable<string>>("--logger", "-l")
-    {
-        Description = CliCommandStrings.CmdLoggerDescription,
-        HelpName = CliCommandStrings.CmdLoggerOption
-    }.ForwardAsSingle(o =>
-    {
-        var loggersString = string.Join(";", GetSemiColonEscapedArgs(o!));
-
-        return $"-property:VSTestLogger={SurroundWithDoubleQuotes(loggersString)}";
-    })
-    .AllowSingleArgPerToken();
-
-    public static readonly Option<string> OutputOption = new Option<string>("--output", "-o")
-    {
-        Description = CliCommandStrings.CmdOutputDescription,
-        HelpName = CliCommandStrings.TestCmdOutputDir
-    }
-    .ForwardAsOutputPath("OutputPath", true);
-
-    public static readonly Option<string> DiagOption = new Option<string>("--diag", "-d")
-    {
-        Description = CliCommandStrings.CmdPathTologFileDescription,
-        HelpName = CliCommandStrings.CmdPathToLogFile
-    }
-    .ForwardAsSingle(o => $"-property:VSTestDiag={SurroundWithDoubleQuotes(CommandDirectoryContext.GetFullPath(o))}");
-
-    public static readonly Option<bool> NoBuildOption = new Option<bool>("--no-build")
-    {
-        Description = CliCommandStrings.CmdNoBuildDescription,
-        Arity = ArgumentArity.Zero
-    }.ForwardAs("-property:VSTestNoBuild=true");
-
-    public static readonly Option<string> ResultsOption = new Option<string>("--results-directory")
-    {
-        Description = CliCommandStrings.CmdResultsDirectoryDescription,
-        HelpName = CliCommandStrings.CmdPathToResultsDirectory
-    }.ForwardAsSingle(o => $"-property:VSTestResultsDirectory={SurroundWithDoubleQuotes(CommandDirectoryContext.GetFullPath(o))}");
-
-    public static readonly Option<IEnumerable<string>> CollectOption = new Option<IEnumerable<string>>("--collect")
-    {
-        Description = CliCommandStrings.cmdCollectDescription,
-        HelpName = CliCommandStrings.cmdCollectFriendlyName
-    }.ForwardAsSingle(o => $"-property:VSTestCollect=\"{string.Join(";", GetSemiColonEscapedArgs(o!))}\"")
-    .AllowSingleArgPerToken();
-
-    public static readonly Option<bool> BlameOption = new Option<bool>("--blame")
-    {
-        Description = CliCommandStrings.CmdBlameDescription,
-        Arity = ArgumentArity.Zero
-    }.ForwardIfEnabled("-property:VSTestBlame=true");
-
-    public static readonly Option<bool> BlameCrashOption = new Option<bool>("--blame-crash")
-    {
-        Description = CliCommandStrings.CmdBlameCrashDescription,
-        Arity = ArgumentArity.Zero
-    }.ForwardIfEnabled("-property:VSTestBlameCrash=true");
-
-    public static readonly Option<string> BlameCrashDumpOption = CreateBlameCrashDumpOption();
-
-    private static Option<string> CreateBlameCrashDumpOption()
-    {
-        Option<string> result = new Option<string>("--blame-crash-dump-type")
-        {
-            Description = CliCommandStrings.CmdBlameCrashDumpTypeDescription,
-            HelpName = CliCommandStrings.CrashDumpTypeArgumentName,
-        }
-        .ForwardAsMany(o => ["-property:VSTestBlameCrash=true", $"-property:VSTestBlameCrashDumpType={o}"]);
-        result.AcceptOnlyFromAmong(["full", "mini"]);
-        return result;
-    }
-
-    public static readonly Option<bool> BlameCrashAlwaysOption = new Option<bool>("--blame-crash-collect-always")
-    {
-        Description = CliCommandStrings.CmdBlameCrashCollectAlwaysDescription,
-        Arity = ArgumentArity.Zero
-    }.ForwardIfEnabled(["-property:VSTestBlameCrash=true", "-property:VSTestBlameCrashCollectAlways=true"]);
-
-    public static readonly Option<bool> BlameHangOption = new Option<bool>("--blame-hang")
-    {
-        Description = CliCommandStrings.CmdBlameHangDescription,
-        Arity = ArgumentArity.Zero
-    }.ForwardAs("-property:VSTestBlameHang=true");
-
-    public static readonly Option<string> BlameHangDumpOption = CreateBlameHangDumpOption();
-
-    private static Option<string> CreateBlameHangDumpOption()
-    {
-        Option<string> result = new Option<string>("--blame-hang-dump-type")
-        {
-            Description = CliCommandStrings.CmdBlameHangDumpTypeDescription,
-            HelpName = CliCommandStrings.HangDumpTypeArgumentName
-        }
-        .ForwardAsMany(o => ["-property:VSTestBlameHang=true", $"-property:VSTestBlameHangDumpType={o}"]);
-        result.AcceptOnlyFromAmong(["full", "mini", "none"]);
-        return result;
-    }
-
-    public static readonly Option<string> BlameHangTimeoutOption = new Option<string>("--blame-hang-timeout")
-    {
-        Description = CliCommandStrings.CmdBlameHangTimeoutDescription,
-        HelpName = CliCommandStrings.HangTimeoutArgumentName
-    }.ForwardAsMany(o => ["-property:VSTestBlameHang=true", $"-property:VSTestBlameHangTimeout={o}"]);
-
-    public static readonly Option<bool> NoLogoOption = CommonOptions.NoLogoOption(forwardAs: "--property:VSTestNoLogo=true", description: CliCommandStrings.TestCmdNoLogo);
-
-    public static readonly Option<bool> NoRestoreOption = CommonOptions.NoRestoreOption;
-
     public static readonly Option<string> FrameworkOption = CommonOptions.FrameworkOption(CliCommandStrings.TestFrameworkOptionDescription);
 
-    public static readonly Option ConfigurationOption = CommonOptions.ConfigurationOption(CliCommandStrings.TestConfigurationOptionDescription);
+    public static readonly Option<string?> ConfigurationOption = CommonOptions.ConfigurationOption(CliCommandStrings.TestConfigurationOptionDescription);
 
     public static readonly Option<Utils.VerbosityOptions?> VerbosityOption = CommonOptions.VerbosityOption();
-    public static readonly Option<string[]> VsTestTargetOption = CommonOptions.RequiredMSBuildTargetOption("VSTest");
-    public static readonly Option<string[]> MTPTargetOption = CommonOptions.RequiredMSBuildTargetOption(CliConstants.MTPTarget);
 
     public static TestRunner GetTestRunner()
     {
@@ -223,28 +88,7 @@ public static TestRunner GetTestRunner()
         return null;
     }
 
-    public static Command Create()
-    {
-        var command = new Command(Name);
-
-        switch (GetTestRunner())
-        {
-            case TestRunner.VSTest:
-                ConfigureVSTestCommand(command);
-                break;
-
-            case TestRunner.MicrosoftTestingPlatform:
-                ConfigureTestingPlatformCommand(command);
-                break;
-
-            default:
-                throw new InvalidOperationException();
-        };
-
-        return command;
-    }
-
-    public static void ConfigureTestingPlatformCommand(Command command)
+    public static void ConfigureMicrosoftTestingPlatformCommand(Command command)
     {
         command.Description = CliCommandStrings.DotnetTestCommandMTPDescription;
         command.Options.Add(MicrosoftTestingPlatformOptions.ProjectOrSolutionOption);
@@ -259,8 +103,8 @@ public static void ConfigureTestingPlatformCommand(Command command)
         command.Options.Add(CommonOptions.ArchitectureOption);
         command.Options.Add(CommonOptions.EnvOption);
         command.Options.Add(CommonOptions.PropertiesOption);
-        command.Options.Add(MicrosoftTestingPlatformOptions.ConfigurationOption);
-        command.Options.Add(MicrosoftTestingPlatformOptions.FrameworkOption);
+        command.Options.Add(ConfigurationOption);
+        command.Options.Add(FrameworkOption);
         command.Options.Add(CommonOptions.OperatingSystemOption);
         command.Options.Add(CommonOptions.RuntimeOption(CliCommandStrings.TestRuntimeOptionDescription));
         command.Options.Add(VerbosityOption);
@@ -272,7 +116,7 @@ public static void ConfigureTestingPlatformCommand(Command command)
         command.Options.Add(MicrosoftTestingPlatformOptions.ListTestsOption);
         command.Options.Add(MicrosoftTestingPlatformOptions.NoLaunchProfileOption);
         command.Options.Add(MicrosoftTestingPlatformOptions.NoLaunchProfileArgumentsOption);
-        command.Options.Add(MTPTargetOption);
+        command.Options.Add(MicrosoftTestingPlatformOptions.MTPTargetOption);
     }
 
     public static void ConfigureVSTestCommand(Command command)
@@ -284,98 +128,36 @@ public static void ConfigureVSTestCommand(Command command)
         // We are on purpose not capturing the solution, project or directory here. We want to pass it to the
         // MSBuild command so we are letting it flow.
 
-        command.Options.Add(SettingsOption);
-        command.Options.Add(ListTestsOption);
+        command.Options.Add(VSTestOptions.SettingsOption);
+        command.Options.Add(VSTestOptions.ListTestsOption);
         command.Options.Add(CommonOptions.TestEnvOption);
-        command.Options.Add(FilterOption);
-        command.Options.Add(AdapterOption);
-        command.Options.Add(LoggerOption);
-        command.Options.Add(OutputOption);
+        command.Options.Add(VSTestOptions.FilterOption);
+        command.Options.Add(VSTestOptions.AdapterOption);
+        command.Options.Add(VSTestOptions.LoggerOption);
+        command.Options.Add(VSTestOptions.OutputOption);
         command.Options.Add(CommonOptions.ArtifactsPathOption);
-        command.Options.Add(DiagOption);
-        command.Options.Add(NoBuildOption);
-        command.Options.Add(ResultsOption);
-        command.Options.Add(CollectOption);
-        command.Options.Add(BlameOption);
-        command.Options.Add(BlameCrashOption);
-        command.Options.Add(BlameCrashDumpOption);
-        command.Options.Add(BlameCrashAlwaysOption);
-        command.Options.Add(BlameHangOption);
-        command.Options.Add(BlameHangDumpOption);
-        command.Options.Add(BlameHangTimeoutOption);
-        command.Options.Add(NoLogoOption);
+        command.Options.Add(VSTestOptions.DiagOption);
+        command.Options.Add(VSTestOptions.NoBuildOption);
+        command.Options.Add(VSTestOptions.ResultsOption);
+        command.Options.Add(VSTestOptions.CollectOption);
+        command.Options.Add(VSTestOptions.BlameOption);
+        command.Options.Add(VSTestOptions.BlameCrashOption);
+        command.Options.Add(VSTestOptions.BlameCrashDumpOption);
+        command.Options.Add(VSTestOptions.BlameCrashAlwaysOption);
+        command.Options.Add(VSTestOptions.BlameHangOption);
+        command.Options.Add(VSTestOptions.BlameHangDumpOption);
+        command.Options.Add(VSTestOptions.BlameHangTimeoutOption);
+        command.Options.Add(VSTestOptions.NoLogoOption);
         command.Options.Add(ConfigurationOption);
         command.Options.Add(FrameworkOption);
         command.Options.Add(CommonOptions.RuntimeOption(CliCommandStrings.TestRuntimeOptionDescription));
-        command.Options.Add(NoRestoreOption);
+        command.Options.Add(CommonOptions.NoRestoreOption);
         command.Options.Add(CommonOptions.InteractiveMsBuildForwardOption);
         command.Options.Add(VerbosityOption);
         command.Options.Add(CommonOptions.ArchitectureOption);
         command.Options.Add(CommonOptions.OperatingSystemOption);
         command.Options.Add(CommonOptions.PropertiesOption);
         command.Options.Add(CommonOptions.DisableBuildServersOption);
-        command.Options.Add(VsTestTargetOption);
-    }
-
-    private static string GetSemiColonEscapedstring(string arg)
-    {
-        if (arg.IndexOf(";") != -1)
-        {
-            return arg.Replace(";", "%3b");
-        }
-
-        return arg;
-    }
-
-    private static string[] GetSemiColonEscapedArgs(IEnumerable<string> args)
-    {
-        int counter = 0;
-        string[] array = new string[args.Count()];
-
-        foreach (string arg in args)
-        {
-            array[counter++] = GetSemiColonEscapedstring(arg);
-        }
-
-        return array;
-    }
-
-    /// <summary>
-    /// Adding double quotes around the property helps MSBuild arguments parser and avoid incorrect splits on ',' or ';'.
-    /// </summary>
-    internal /* for testing purposes */ static string SurroundWithDoubleQuotes(string input)
-    {
-        if (input is null)
-        {
-            throw new ArgumentNullException(nameof(input));
-        }
-
-        // If already escaped by double quotes then return original string.
-        if (input.StartsWith("\"", StringComparison.Ordinal)
-            && input.EndsWith("\"", StringComparison.Ordinal))
-        {
-            return input;
-        }
-
-        // We want to count the number of trailing backslashes to ensure
-        // we will have an even number before adding the final double quote.
-        // Otherwise the last \" will be interpreted as escaping the double
-        // quote rather than a backslash and a double quote.
-        var trailingBackslashesCount = 0;
-        for (int i = input.Length - 1; i >= 0; i--)
-        {
-            if (input[i] == '\\')
-            {
-                trailingBackslashesCount++;
-            }
-            else
-            {
-                break;
-            }
-        }
-
-        return trailingBackslashesCount % 2 == 0
-            ? string.Concat("\"", input, "\"")
-            : string.Concat("\"", input, "\\\"");
+        command.Options.Add(VSTestOptions.VsTestTargetOption);
     }
 }

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

@@ -27,7 +27,7 @@ private static Command CreateCommand()
     private static Command CreateTestingPlatformCommand()
     {
         var command = new MicrosoftTestingPlatformTestCommand(TestCommandDefinition.Name);
-        TestCommandDefinition.ConfigureTestingPlatformCommand(command);
+        TestCommandDefinition.ConfigureMicrosoftTestingPlatformCommand(command);
         command.SetAction(parseResult => command.Run(parseResult));
         return command;
     }