Porting fix for Design mode clients hang for errors

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs

@@ -8,15 +8,21 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel
     using System.Linq;
     using System.Threading.Tasks;
 
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine;
+    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
 
     /// <summary>
     /// ParallelProxyDiscoveryManager that manages parallel discovery
     /// </summary>
     internal class ParallelProxyDiscoveryManager : ParallelOperationManager<IProxyDiscoveryManager, ITestDiscoveryEventsHandler2>, IParallelProxyDiscoveryManager
     {
+        private IDataSerializer dataSerializer;
+
         #region DiscoverySpecificData
 
         private int discoveryCompletedClients = 0;
@@ -42,15 +48,21 @@ internal class ParallelProxyDiscoveryManager : ParallelOperationManager<IProxyDi
         private object discoveryStatusLockObject = new object();
 
         #endregion
-        
+
         public ParallelProxyDiscoveryManager(IRequestData requestData, Func<IProxyDiscoveryManager> actualProxyManagerCreator, int parallelLevel, bool sharedHosts)
+            : this(requestData, actualProxyManagerCreator, JsonDataSerializer.Instance, parallelLevel, sharedHosts)
+        {
+        }
+
+        internal ParallelProxyDiscoveryManager(IRequestData requestData, Func<IProxyDiscoveryManager> actualProxyManagerCreator, IDataSerializer dataSerializer, int parallelLevel, bool sharedHosts)
             : base(actualProxyManagerCreator, parallelLevel, sharedHosts)
         {
             this.requestData = requestData;
+            this.dataSerializer = dataSerializer;
         }
 
         #region IProxyDiscoveryManager
-        
+
         /// <inheritdoc/>
         public void Initialize()
         {
@@ -61,12 +73,12 @@ public void Initialize()
         public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)
         {
             this.actualDiscoveryCriteria = discoveryCriteria;
-                
+
             // Set the enumerator for parallel yielding of sources
             // Whenever a concurrent executor becomes free, it picks up the next source using this enumerator
             this.sourceEnumerator = discoveryCriteria.Sources.GetEnumerator();
             this.availableTestSources = discoveryCriteria.Sources.Count();
-            
+
             if (EqtTrace.IsVerboseEnabled)
             {
                 EqtTrace.Verbose("ParallelProxyDiscoveryManager: Start discovery. Total sources: " + this.availableTestSources);
@@ -210,19 +222,23 @@ private void DiscoverTestsOnConcurrentManager(IProxyDiscoveryManager proxyDiscov
                         // Just in case, the actual discovery couldn't start for an instance. Ensure that
                         // we call discovery complete since we have already fetched a source. Otherwise
                         // discovery will not terminate
-                        if (EqtTrace.IsWarningEnabled)
+                        if (EqtTrace.IsErrorEnabled)
                         {
-                            EqtTrace.Warning("ParallelProxyDiscoveryManager: Failed to trigger discovery. Exception: " + t.Exception);
+                            EqtTrace.Error("ParallelProxyDiscoveryManager: Failed to trigger discovery. Exception: " + t.Exception);
                         }
 
+                        var handler = this.GetHandlerForGivenManager(proxyDiscoveryManager);
+                        var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = t.Exception.ToString() };
+                        handler.HandleRawMessage(this.dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload));
+                        handler.HandleLogMessage(TestMessageLevel.Error, t.Exception.ToString());
+
                         // Send discovery complete. Similar logic is also used in ProxyDiscoveryManager.DiscoverTests.
                         // Differences:
                         // Total tests must be zero here since parallel discovery events handler adds the count
                         // Keep `lastChunk` as null since we don't want a message back to the IDE (discovery didn't even begin)
                         // Set `isAborted` as true since we want this instance of discovery manager to be replaced
                         var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true);
-
-                        this.GetHandlerForGivenManager(proxyDiscoveryManager).HandleDiscoveryComplete(discoveryCompleteEventsArgs, null);
+                        handler.HandleDiscoveryComplete(discoveryCompleteEventsArgs, null);
                     },
                     TaskContinuationOptions.OnlyOnFaulted);
             }

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs

@@ -11,16 +11,22 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel
     using System.Threading;
     using System.Threading.Tasks;
 
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine;
+    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
 
     /// <summary>
     /// ParallelProxyExecutionManager that manages parallel execution
     /// </summary>
     internal class ParallelProxyExecutionManager : ParallelOperationManager<IProxyExecutionManager, ITestRunEventsHandler>, IParallelProxyExecutionManager
     {
+        private IDataSerializer dataSerializer;
+
         #region TestRunSpecificData
 
         // This variable id to differentiate between implicit (abort requested by testPlatform) and explicit (test host aborted) abort.
@@ -60,15 +66,20 @@ internal class ParallelProxyExecutionManager : ParallelOperationManager<IProxyEx
         #endregion
 
         public ParallelProxyExecutionManager(IRequestData requestData, Func<IProxyExecutionManager> actualProxyManagerCreator, int parallelLevel)
-            : base(actualProxyManagerCreator, parallelLevel, true)
+            : this(requestData, actualProxyManagerCreator, JsonDataSerializer.Instance, parallelLevel, true)
         {
-            this.requestData = requestData;
         }
 
         public ParallelProxyExecutionManager(IRequestData requestData, Func<IProxyExecutionManager> actualProxyManagerCreator, int parallelLevel, bool sharedHosts)
+            : this(requestData, actualProxyManagerCreator, JsonDataSerializer.Instance, parallelLevel, sharedHosts)
+        {
+        }
+
+        internal ParallelProxyExecutionManager(IRequestData requestData, Func<IProxyExecutionManager> actualProxyManagerCreator, IDataSerializer dataSerializer, int parallelLevel, bool sharedHosts)
             : base(actualProxyManagerCreator, parallelLevel, sharedHosts)
         {
             this.requestData = requestData;
+            this.dataSerializer = dataSerializer;
         }
 
         #region IProxyExecutionManager
@@ -325,18 +336,23 @@ private void StartTestRunOnConcurrentManager(IProxyExecutionManager proxyExecuti
                     // Just in case, the actual execution couldn't start for an instance. Ensure that
                     // we call execution complete since we have already fetched a source. Otherwise
                     // execution will not terminate
-                    if (EqtTrace.IsWarningEnabled)
+                    if (EqtTrace.IsErrorEnabled)
                     {
-                        EqtTrace.Warning("ParallelProxyExecutionManager: Failed to trigger execution. Exception: " + t.Exception);
+                        EqtTrace.Error("ParallelProxyExecutionManager: Failed to trigger execution. Exception: " + t.Exception);
                     }
 
+                    var handler = this.GetHandlerForGivenManager(proxyExecutionManager);
+                    var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = t.Exception.ToString() };
+                    handler.HandleRawMessage(this.dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload));
+                    handler.HandleLogMessage(TestMessageLevel.Error, t.Exception.ToString());
+
                     // Send a run complete to caller. Similar logic is also used in ProxyExecutionManager.StartTestRun
                     // Differences:
                     // Aborted is sent to allow the current execution manager replaced with another instance
                     // Ensure that the test run aggregator in parallel run events handler doesn't add these statistics
                     // (since the test run didn't even start)
                     var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection<AttachmentSet>(), TimeSpan.Zero);
-                    this.GetHandlerForGivenManager(proxyExecutionManager).HandleTestRunComplete(completeArgs, null, null, null);
+                    handler.HandleTestRunComplete(completeArgs, null, null, null);
                 },
                 TaskContinuationOptions.OnlyOnFaulted);
             }

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs

@@ -122,8 +122,14 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve
                 // and the test host is lost as well.
                 this.HandleLogMessage(TestMessageLevel.Error, exception.ToString());
 
+                var discoveryCompletePayload = new DiscoveryCompletePayload()
+                {
+                    IsAborted = true,
+                    LastDiscoveredTests = null,
+                    TotalTests = -1
+                };
+                this.HandleRawMessage(this.dataSerializer.SerializePayload(MessageType.DiscoveryComplete, discoveryCompletePayload));
                 var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true);
-
                 this.HandleDiscoveryComplete(discoveryCompleteEventsArgs, new List<ObjectModel.TestCase>());
             }
         }

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs

@@ -152,13 +152,19 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsH
             catch (Exception exception)
             {
                 EqtTrace.Error("ProxyExecutionManager.StartTestRun: Failed to start test run: {0}", exception);
+
+                // Log error message to design mode and CLI.
+                var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = exception.ToString() };
+                this.HandleRawMessage(this.dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload));
                 this.LogMessage(TestMessageLevel.Error, exception.ToString());
 
                 // Send a run complete to caller. Similar logic is also used in ParallelProxyExecutionManager.StartTestRunOnConcurrentManager
                 // Aborted is `true`: in case of parallel run (or non shared host), an aborted message ensures another execution manager
                 // created to replace the current one. This will help if the current execution manager is aborted due to irreparable error
                 // and the test host is lost as well.
-                var completeArgs = new TestRunCompleteEventArgs(null, false, true, exception, new Collection<AttachmentSet>(), TimeSpan.Zero);
+                var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection<AttachmentSet>(), TimeSpan.Zero);
+                var testRunCompletePayload = new TestRunCompletePayload { TestRunCompleteArgs = completeArgs };
+                this.HandleRawMessage(this.dataSerializer.SerializePayload(MessageType.ExecutionComplete, testRunCompletePayload));
                 this.HandleTestRunComplete(completeArgs, null, null, null);
             }
 
@@ -262,4 +268,4 @@ private void InitializeExtensions(IEnumerable<string> sources)
             }
         }
     }
-}
+}

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyDiscoveryManagerTests.cs

@@ -10,12 +10,14 @@ namespace TestPlatform.CrossPlatEngine.UnitTests.Client
 
     using Microsoft.VisualStudio.TestPlatform.Common.Telemetry;
     using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
+    using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client;
     using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
+    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
     using Microsoft.VisualStudio.TestTools.UnitTesting;
 
     using Moq;
@@ -134,6 +136,50 @@ public void DiscoveryTestsShouldProcessAllSourceIfOneDiscoveryManagerIsStarved()
             Assert.AreEqual(1, processedSources.Count, "All Sources must be processed.");
         }
 
+        [TestMethod]
+        public void DiscoveryTestsShouldCatchExceptionAndHandleLogMessageOfError()
+        {
+            // Ensure that second discovery manager never starts. Expect 10 total tests.
+            // Override DiscoveryComplete since overall aborted should be true
+            var parallelDiscoveryManager = this.SetupDiscoveryManager(this.proxyManagerFunc, 2, false, totalTests: 10);
+            this.createdMockManagers[1].Reset();
+            this.createdMockManagers[1].Setup(dm => dm.DiscoverTests(It.IsAny<DiscoveryCriteria>(), It.IsAny<ITestDiscoveryEventsHandler2>()))
+                .Throws<NotImplementedException>();
+            this.mockHandler.Setup(mh => mh.HandleDiscoveryComplete(It.IsAny<DiscoveryCompleteEventArgs>(), null))
+                .Callback<DiscoveryCompleteEventArgs, IEnumerable<TestCase>>((t, l) => { this.discoveryCompleted.Set(); });
+
+            Task.Run(() =>
+            {
+                parallelDiscoveryManager.DiscoverTests(this.testDiscoveryCriteria, this.mockHandler.Object);
+            });
+
+            // Processed sources should be 1 since the 2nd source is never discovered
+            Assert.IsTrue(this.discoveryCompleted.Wait(ParallelProxyDiscoveryManagerTests.taskTimeout), "Test discovery not completed.");
+            mockHandler.Verify(s => s.HandleLogMessage(TestMessageLevel.Error, It.IsAny<string>()), Times.Once);
+        }
+
+        [TestMethod]
+        public void DiscoveryTestsShouldCatchExceptionAndHandleRawMessageOfTestMessage()
+        {
+            // Ensure that second discovery manager never starts. Expect 10 total tests.
+            // Override DiscoveryComplete since overall aborted should be true
+            var parallelDiscoveryManager = this.SetupDiscoveryManager(this.proxyManagerFunc, 2, false, totalTests: 10);
+            this.createdMockManagers[1].Reset();
+            this.createdMockManagers[1].Setup(dm => dm.DiscoverTests(It.IsAny<DiscoveryCriteria>(), It.IsAny<ITestDiscoveryEventsHandler2>()))
+                .Throws<NotImplementedException>();
+            this.mockHandler.Setup(mh => mh.HandleDiscoveryComplete(It.IsAny<DiscoveryCompleteEventArgs>(), null))
+                .Callback<DiscoveryCompleteEventArgs, IEnumerable<TestCase>>((t, l) => { this.discoveryCompleted.Set(); });
+
+            Task.Run(() =>
+            {
+                parallelDiscoveryManager.DiscoverTests(this.testDiscoveryCriteria, this.mockHandler.Object);
+            });
+
+            // Processed sources should be 1 since the 2nd source is never discovered
+            Assert.IsTrue(this.discoveryCompleted.Wait(ParallelProxyDiscoveryManagerTests.taskTimeout), "Test discovery not completed.");
+            mockHandler.Verify(s => s.HandleRawMessage(It.Is<string>(str => str.Contains(MessageType.TestMessage))));
+        }
+
         [TestMethod]
         public void HandlePartialDiscoveryCompleteShouldCreateANewProxyDiscoveryManagerIfIsAbortedIsTrue()
         {
@@ -204,4 +250,4 @@ private void AssertMissingAndDuplicateSources(List<string> processedSources)
             }
         }
     }
-}
+}