Stricter implementation of test session events for MTP (#50703)

Author: Youssef1313

src/Cli/dotnet/Commands/CliCommandStrings.resx

@@ -117,9 +117,6 @@
   <resheader name="writer">
     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
   </resheader>
-  <data name="Aborted" xml:space="preserve">
-    <value>Aborted</value>
-  </data>
   <data name="ActiveTestsRunning_FullTestsCount" xml:space="preserve">
     <value>{0} tests running</value>
   </data>
@@ -2666,4 +2663,23 @@ Proceed?</value>
   <data name="DotnetTestIncompatibleHandshakeVersion" xml:space="preserve">
     <value>Supported protocol versions sent by Microsoft.Testing.Platform are '{0}'. The SDK supports '{1}', which is incompatible.</value>
   </data>
-</root>
+  <data name="ErrorRunningTestModule" xml:space="preserve">
+    <value>The following exception occurred when running the test module with RunCommand '{0}' and RunArguments '{1}':
+
+{2}</value>
+    <comment>{Locked="RunCommand"}{Locked="RunArguments"}
+{2} is the exception that occurred.</comment>
+  </data>
+  <data name="UnexpectedTestSessionEnd" xml:space="preserve">
+    <value>A test session end event was received without a corresponding test session start.</value>
+  </data>
+  <data name="MissingTestSessionEnd" xml:space="preserve">
+    <value>A test session start event was received without a corresponding test session end.</value>
+  </data>
+  <data name="Aborted" xml:space="preserve">
+    <value>Aborted</value>
+  </data>
+  <data name="UnexpectedHelpMessage" xml:space="preserve">
+    <value>An unexpected help-related message was received when `--help` wasn't used.</value>
+  </data>
+</root>

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

@@ -5,13 +5,14 @@
 
 using System.Collections.Concurrent;
 using System.CommandLine;
+using Microsoft.DotNet.Cli.Commands.Test.IPC.Models;
 using Microsoft.TemplateEngine.Cli.Help;
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
 
 internal partial class MicrosoftTestingPlatformTestCommand
 {
-    private readonly ConcurrentDictionary<string, CommandLineOption> _commandLineOptionNameToModuleNames = [];
+    private readonly ConcurrentDictionary<string, CommandLineOptionMessage> _commandLineOptionNameToModuleNames = [];
     private readonly ConcurrentDictionary<bool, List<(string, string[])>> _moduleNamesToCommandLineOptions = [];
     private static readonly string Indent = "  ";
 
@@ -29,9 +30,9 @@ public IEnumerable<Action<HelpContext>> CustomHelpLayout()
                 return;
             }
 
-            Dictionary<bool, List<CommandLineOption>> allOptions = GetAllOptions(context.Command.Options);
-            allOptions.TryGetValue(true, out List<CommandLineOption> builtInOptions);
-            allOptions.TryGetValue(false, out List<CommandLineOption> nonBuiltInOptions);
+            Dictionary<bool, List<CommandLineOptionMessage>> allOptions = GetAllOptions(context.Command.Options);
+            allOptions.TryGetValue(true, out List<CommandLineOptionMessage> builtInOptions);
+            allOptions.TryGetValue(false, out List<CommandLineOptionMessage> nonBuiltInOptions);
 
             Dictionary<bool, List<(string[], string[])>> moduleToMissingOptions = GetModulesToMissingOptions(_moduleNamesToCommandLineOptions, builtInOptions.Select(option => option.Name), nonBuiltInOptions.Select(option => option.Name));
 
@@ -92,22 +93,14 @@ private static string FormatHelpOption(string option)
         return $"[{option.Trim(':').ToLower()}]";
     }
 
-    private void OnHelpRequested(object sender, HelpEventArgs args)
+    private void OnHelpRequested(CommandLineOptionMessages commandLineOptionMessages)
     {
-        var testApp = (TestApplication)sender;
-        if (!testApp.TestOptions.IsHelp)
-        {
-            // TODO: Better to throw exception?
-            return;
-        }
-        
-        CommandLineOption[] commandLineOptionMessages = args.CommandLineOptions;
-        string moduleName = args.ModulePath;
+        string moduleName = commandLineOptionMessages.ModulePath;
 
         List<string> builtInOptions = [];
         List<string> nonBuiltInOptions = [];
 
-        foreach (CommandLineOption commandLineOption in commandLineOptionMessages)
+        foreach (CommandLineOptionMessage commandLineOption in commandLineOptionMessages.CommandLineOptionMessageList)
         {
             if (commandLineOption.IsHidden.HasValue && commandLineOption.IsHidden.Value) continue;
 
@@ -135,19 +128,19 @@ private void OnHelpRequested(object sender, HelpEventArgs args)
            (isBuiltIn, value) => [.. value, (moduleName, nonBuiltInOptions.ToArray())]);
     }
 
-    private Dictionary<bool, List<CommandLineOption>> GetAllOptions(IList<Option> commandOptions)
+    private Dictionary<bool, List<CommandLineOptionMessage>> GetAllOptions(IList<Option> commandOptions)
     {
-        Dictionary<bool, List<CommandLineOption>> filteredOptions = [];
+        Dictionary<bool, List<CommandLineOptionMessage>> filteredOptions = [];
 
         // Create a set of option names from the command's options for efficient lookup
         var commandOptionNames = commandOptions.Select(o => o.Name.TrimStart('-')).ToHashSet(StringComparer.OrdinalIgnoreCase);
 
-        foreach (KeyValuePair<string, CommandLineOption> option in _commandLineOptionNameToModuleNames)
+        foreach (KeyValuePair<string, CommandLineOptionMessage> option in _commandLineOptionNameToModuleNames)
         {
             // Only include options that are NOT already present in the command's options
             if (!commandOptionNames.Contains(option.Value.Name))
             {
-                if (!filteredOptions.TryGetValue(option.Value.IsBuiltIn.Value, out List<CommandLineOption> value))
+                if (!filteredOptions.TryGetValue(option.Value.IsBuiltIn.Value, out List<CommandLineOptionMessage> value))
                 {
                     filteredOptions.Add(option.Value.IsBuiltIn.Value, [option.Value]);
                 }

src/Cli/dotnet/Commands/Test/MTP/MicrosoftTestingPlatformTestCommand.Help.cs

@@ -42,8 +42,7 @@ private int RunInternal(ParseResult parseResult, bool isHelp)
         ValidationUtility.ValidateSolutionOrProjectOrDirectoryOrModulesArePassedCorrectly(parseResult);
 
         int degreeOfParallelism = GetDegreeOfParallelism(parseResult);
-        bool filterModeEnabled = parseResult.HasOption(MicrosoftTestingPlatformOptions.TestModulesFilterOption);
-        var testOptions = new TestOptions(filterModeEnabled, IsHelp: isHelp, IsDiscovery: parseResult.HasOption(MicrosoftTestingPlatformOptions.ListTestsOption));
+        var testOptions = new TestOptions(IsHelp: isHelp, IsDiscovery: parseResult.HasOption(MicrosoftTestingPlatformOptions.ListTestsOption));
 
         InitializeOutput(degreeOfParallelism, parseResult, testOptions);
 
@@ -55,7 +54,8 @@ private int RunInternal(ParseResult parseResult, bool isHelp)
 
         var msBuildHandler = new MSBuildHandler(buildOptions, actionQueue, _output);
 
-        if (testOptions.HasFilterMode)
+        bool filterModeEnabled = parseResult.HasOption(MicrosoftTestingPlatformOptions.TestModulesFilterOption);
+        if (filterModeEnabled)
         {
             var testModulesFilterHandler = new TestModulesFilterHandler(actionQueue, _output);
             if (!testModulesFilterHandler.RunWithTestModulesFilter(parseResult))
@@ -93,11 +93,7 @@ private int RunInternal(ParseResult parseResult, bool isHelp)
 
     private TestApplicationActionQueue InitializeActionQueue(int degreeOfParallelism, TestOptions testOptions, BuildOptions buildOptions)
     {
-        return new TestApplicationActionQueue(degreeOfParallelism, buildOptions, testOptions, _output, async (TestApplication testApp) =>
-        {
-            testApp.HelpRequested += OnHelpRequested;
-            return await testApp.RunAsync();
-        });
+        return new TestApplicationActionQueue(degreeOfParallelism, buildOptions, testOptions, _output, OnHelpRequested);
     }
 
     private void SetupCancelKeyPressHandler()

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

@@ -113,5 +113,3 @@ public bool MoveNext()
 
 
 internal sealed record TestModule(RunProperties RunProperties, string? ProjectFullPath, string? TargetFramework, bool IsTestingPlatformApplication, bool IsTestProject, ProjectLaunchSettingsModel? LaunchSettings, string TargetPath, string? DotnetRootArchVariableName);
-
-internal sealed record CommandLineOption(string Name, string Description, bool? IsHidden, bool? IsBuiltIn);

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

@@ -3,7 +3,7 @@
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
 
-internal record TestOptions(bool HasFilterMode, bool IsHelp, bool IsDiscovery);
+internal record TestOptions(bool IsHelp, bool IsDiscovery);
 
 internal record PathOptions(string? ProjectPath, string? SolutionPath, string? ResultsDirectoryPath, string? ConfigFilePath, string? DiagnosticOutputDirectoryPath);
 

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

@@ -6,6 +6,7 @@
 using System.Text.RegularExpressions;
 using Microsoft.CodeAnalysis;
 using Microsoft.Testing.Platform.OutputDevice.Terminal;
+using Microsoft.DotNet.Cli.Commands.Test.IPC.Models;
 
 namespace Microsoft.DotNet.Cli.Commands.Test.Terminal;
 
@@ -999,8 +1000,8 @@ public void TestInProgress(
     }
 
     public void WritePlatformAndExtensionOptions(HelpContext context,
-        IEnumerable<CommandLineOption> builtInOptions,
-        IEnumerable<CommandLineOption> nonBuiltInOptions,
+        IEnumerable<CommandLineOptionMessage> builtInOptions,
+        IEnumerable<CommandLineOptionMessage> nonBuiltInOptions,
         Dictionary<bool, List<(string[], string[])>> moduleToMissingOptions)
     {
         if (_wasCancelled)
@@ -1022,15 +1023,15 @@ public void WritePlatformAndExtensionOptions(HelpContext context,
         WriteModulesToMissingOptionsToConsole(moduleToMissingOptions);
     }
 
-    private void WriteOtherOptionsSection(HelpContext context, string title, IEnumerable<CommandLineOption> options)
+    private void WriteOtherOptionsSection(HelpContext context, string title, IEnumerable<CommandLineOptionMessage> options)
     {
         List<TwoColumnHelpRow> optionRows = [];
 
         foreach (var option in options)
         {
             if (option.IsHidden != true)
             {
-                optionRows.Add(new TwoColumnHelpRow($"--{option.Name}", option.Description));
+                optionRows.Add(new TwoColumnHelpRow($"--{option.Name}", option.Description ?? string.Empty));
             }
         }
 

src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs

@@ -18,9 +18,11 @@ internal sealed class TestApplication(
     TestModule module,
     BuildOptions buildOptions,
     TestOptions testOptions,
-    TerminalTestReporter output) : IDisposable
+    TerminalTestReporter output,
+    Action<CommandLineOptionMessages> onHelpRequested) : IDisposable
 {
     private readonly BuildOptions _buildOptions = buildOptions;
+    private readonly Action<CommandLineOptionMessages> _onHelpRequested = onHelpRequested;
     private readonly TestApplicationHandler _handler = new(output, module, testOptions);
 
     private readonly List<string> _outputData = [];
@@ -32,8 +34,6 @@ internal sealed class TestApplication(
     private readonly List<NamedPipeServer> _testAppPipeConnections = [];
     private readonly Dictionary<NamedPipeServer, HandshakeMessage> _handshakes = new();
 
-    public event EventHandler<HelpEventArgs> HelpRequested;
-
     public TestModule Module { get; } = module;
     public TestOptions TestOptions { get; } = testOptions;
 
@@ -43,19 +43,19 @@ public async Task<int> RunAsync()
     {
         // TODO: RunAsync is probably expected to be executed exactly once on each TestApplication instance.
         // Consider throwing an exception if it's called more than once.
-        if (TestOptions.HasFilterMode && !ModulePathExists())
-        {
-            return ExitCode.GenericFailure;
-        }
-
         var processStartInfo = CreateProcessStartInfo();
 
         _testAppPipeConnectionLoop = Task.Run(async () => await WaitConnectionAsync(_cancellationToken.Token), _cancellationToken.Token);
-        var testProcessResult = await StartProcess(processStartInfo);
+        var testProcessExitCode = await StartProcess(processStartInfo);
 
         WaitOnTestApplicationPipeConnectionLoop();
 
-        return testProcessResult;
+        if (_handler.HasMismatchingTestSessionEventCount())
+        {
+            throw new InvalidOperationException(CliCommandStrings.MissingTestSessionEnd);
+        }
+
+        return testProcessExitCode;
     }
 
     private ProcessStartInfo CreateProcessStartInfo()
@@ -221,6 +221,15 @@ private Task<IResponse> OnRequest(NamedPipeServer server, IRequest request)
         }
         catch (Exception ex)
         {
+            // BE CAREFUL:
+            // When handling some of the messages, we may throw an exception in unexpected state.
+            // (e.g, OnSessionEvent may throw if we receive TestSessionEnd without TestSessionStart).
+            // (or if we receive help-related messages when not in help mode)
+            // In that case, we FailFast.
+            // The lack of FailFast *might* have unintended consequences, such as breaking the internal loop of pipe server.
+            // In that case, maybe MTP app will continue waiting for response, but we don't send the response and are waiting for
+            // MTP app process exit (which doesn't happen).
+            // So, we explicitly FailFast here.
             string exAsString = ex.ToString();
             Logger.LogTrace(exAsString);
             Environment.FailFast(exAsString);
@@ -298,24 +307,17 @@ private void StoreOutputAndErrorData(Process process)
         process.BeginErrorReadLine();
     }
 
-    private bool ModulePathExists()
-    {
-        if (!File.Exists(Module.RunProperties.Command))
-        {
-            // TODO: The error should be shown to the user, not just logged to trace.
-            Logger.LogTrace($"Test module '{Module.RunProperties.Command}' not found. Build the test application before or run 'dotnet test'.");
-
-            return false;
-        }
-        return true;
-    }
-
     public void OnHandshakeMessage(HandshakeMessage handshakeMessage, bool gotSupportedVersion)
         => _handler.OnHandshakeReceived(handshakeMessage, gotSupportedVersion);
 
     private void OnCommandLineOptionMessages(CommandLineOptionMessages commandLineOptionMessages)
     {
-        HelpRequested?.Invoke(this, new HelpEventArgs { ModulePath = commandLineOptionMessages.ModulePath, CommandLineOptions = [.. commandLineOptionMessages.CommandLineOptionMessageList.Select(message => new CommandLineOption(message.Name, message.Description, message.IsHidden, message.IsBuiltIn))] });
+        if (!TestOptions.IsHelp)
+        {
+            throw new InvalidOperationException(CliCommandStrings.UnexpectedHelpMessage);
+        }
+
+        _onHelpRequested(commandLineOptionMessages);
     }
 
     private void OnDiscoveredTestMessages(DiscoveredTestMessages discoveredTestMessages)
@@ -324,7 +326,7 @@ private void OnDiscoveredTestMessages(DiscoveredTestMessages discoveredTestMessa
     private void OnTestResultMessages(TestResultMessages testResultMessage)
         => _handler.OnTestResultsReceived(testResultMessage);
 
-    internal void OnFileArtifactMessages(FileArtifactMessages fileArtifactMessages)
+    private void OnFileArtifactMessages(FileArtifactMessages fileArtifactMessages)
         => _handler.OnFileArtifactsReceived(fileArtifactMessages);
 
     private void OnSessionEvent(TestSessionEvent sessionEvent)

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

@@ -4,7 +4,9 @@
 #nullable disable
 
 using System.Threading.Channels;
+using Microsoft.DotNet.Cli.Commands.Test.IPC.Models;
 using Microsoft.DotNet.Cli.Commands.Test.Terminal;
+using Microsoft.DotNet.Cli.Utils;
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
 
@@ -17,14 +19,14 @@ internal class TestApplicationActionQueue
 
     private static readonly Lock _lock = new();
 
-    public TestApplicationActionQueue(int degreeOfParallelism, BuildOptions buildOptions, TestOptions testOptions, TerminalTestReporter output, Func<TestApplication, Task<int>> action)
+    public TestApplicationActionQueue(int degreeOfParallelism, BuildOptions buildOptions, TestOptions testOptions, TerminalTestReporter output, Action<CommandLineOptionMessages> onHelpRequested)
     {
         _channel = Channel.CreateUnbounded<ParallelizableTestModuleGroupWithSequentialInnerModules>(new UnboundedChannelOptions { SingleReader = false, SingleWriter = false });
         _readers = new Task[degreeOfParallelism];
 
         for (int i = 0; i < degreeOfParallelism; i++)
         {
-            _readers[i] = Task.Run(async () => await Read(action, buildOptions, testOptions, output));
+            _readers[i] = Task.Run(async () => await Read(buildOptions, testOptions, output, onHelpRequested));
         }
     }
 
@@ -51,23 +53,27 @@ public void EnqueueCompleted()
         _channel.Writer.Complete();
     }
 
-    private async Task Read(Func<TestApplication, Task<int>> action, BuildOptions buildOptions, TestOptions testOptions, TerminalTestReporter output)
+    private async Task Read(BuildOptions buildOptions, TestOptions testOptions, TerminalTestReporter output, Action<CommandLineOptionMessages> onHelpRequested)
     {
         await foreach (var nonParallelizedGroup in _channel.Reader.ReadAllAsync())
         {
             foreach (var module in nonParallelizedGroup)
             {
                 int result = ExitCode.GenericFailure;
-                var testApp = new TestApplication(module, buildOptions, testOptions, output);
-                using (testApp)
+                var testApp = new TestApplication(module, buildOptions, testOptions, output, onHelpRequested);
+                try
                 {
-                    // TODO: If this throws, we will loose "one" degree of parallelism.
-                    // Then, if we ended up losing all degree of parallelisms, we will not be able to process all test apps.
-                    // At the end, WaitAllActions will likely just throw this exception wrapped in AggregateException, and
-                    // it will bubble up to CLI Main and crash the process.
-                    // We should try to work on a better handling for this, and better understand what are the possibilities for this to throw.
-                    // e.g, RunCommand pointing out to some file that doesn't exist? Process.Start failing for some reason?
-                    result = await action(testApp);
+                    using (testApp)
+                    {
+                        result = await testApp.RunAsync();
+                    }
+                }
+                catch (Exception ex)
+                {
+                    var exAsString = ex.ToString();
+                    Logger.LogTrace($"Exception running test module {module.RunProperties?.Command} {module.RunProperties?.Arguments}: {exAsString}");
+                    Reporter.Error.WriteLine(string.Format(CliCommandStrings.ErrorRunningTestModule, module.RunProperties?.Command, module.RunProperties?.Arguments, exAsString));
+                    result = ExitCode.GenericFailure;
                 }
 
                 if (result == ExitCode.Success && testApp.HasFailureDuringDispose)