Cancellation timing issue fix. (#1398)

* Fix for a timing issue where a cancel request before test run request has been started is being ignored.
We have the right plumbing in place but just seem to have missed initializing a var. Put that in place.
All interface changes are internal only.

src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs

@@ -247,7 +247,7 @@ public void CancelAsync()
                 else
                 {
                     // Inform the service about run cancellation
-                    this.ExecutionManager.Cancel();
+                    this.ExecutionManager.Cancel(this);
                 }
             }
 
@@ -275,7 +275,7 @@ public void Abort()
                 }
                 else
                 {
-                    this.ExecutionManager.Abort();
+                    this.ExecutionManager.Abort(this);
                 }
             }
 

src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyExecutionManager.cs

@@ -31,12 +31,14 @@ public interface IProxyExecutionManager
         /// <summary>
         /// Cancels the test run. On the test host, this will send a message to adapters.
         /// </summary>
-        void Cancel();
+        // <param name="eventHandler"> EventHandler for handling execution events from Engine. </param> 
+        void Cancel(ITestRunEventsHandler eventHandler);
 
         /// <summary>
         /// Aborts the test operation. This will forcefully terminate the test host.
         /// </summary>
-        void Abort();
+        // <param name="eventHandler"> EventHandler for handling execution events from Engine. </param> 
+        void Abort(ITestRunEventsHandler eventHandler);
 
         /// <summary>
         /// Closes the current test operation by sending a end session message.

src/Microsoft.TestPlatform.Common/Interfaces/Engine/TesthostProtocol/IExecutionManager.cs

@@ -49,11 +49,13 @@ public interface IExecutionManager
         /// <summary>
         /// Cancel the test execution.
         /// </summary>
-        void Cancel();
+        /// <param name="eventHandler"> EventHandler for handling execution events from Engine. </param>
+        void Cancel(ITestRunEventsHandler testRunEventsHandler);
 
         /// <summary>
         /// Aborts the test execution.
         /// </summary>
-        void Abort();
+        /// <param name="eventHandler"> EventHandler for handling execution events from Engine. </param>
+        void Abort(ITestRunEventsHandler testRunEventsHandler);
     }
 }

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

@@ -109,17 +109,19 @@ public int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsHandler e
         /// <summary>
         /// Aborts the test operation.
         /// </summary>
-        public void Abort()
+        /// <param name="eventHandler"> EventHandler for handling execution events from Engine. </param>
+        public void Abort(ITestRunEventsHandler eventHandler)
         {
-            Task.Run(() => this.testHostManagerFactory.GetExecutionManager().Abort());
+            Task.Run(() => this.testHostManagerFactory.GetExecutionManager().Abort(eventHandler));
         }
 
         /// <summary>
         /// Cancels the test run.
         /// </summary>
-        public void Cancel()
+        /// <param name="eventHandler"> EventHandler for handling execution events from Engine. </param>
+        public void Cancel(ITestRunEventsHandler eventHandler)
         {
-            Task.Run(() => this.testHostManagerFactory.GetExecutionManager().Cancel());
+            Task.Run(() => this.testHostManagerFactory.GetExecutionManager().Cancel(eventHandler));
         }
 
         /// <summary>

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

@@ -120,16 +120,16 @@ public int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsHandler e
             return this.StartTestRunPrivate(eventHandler);
         }
 
-        public void Abort()
+        public void Abort(ITestRunEventsHandler runEventsHandler)
         {
             // Test platform initiated abort.
             abortRequested = true;
-            this.DoActionOnAllManagers((proxyManager) => proxyManager.Abort(), doActionsInParallel: true);
+            this.DoActionOnAllManagers((proxyManager) => proxyManager.Abort(runEventsHandler), doActionsInParallel: true);
         }
 
-        public void Cancel()
+        public void Cancel(ITestRunEventsHandler runEventsHandler)
         {
-            this.DoActionOnAllManagers((proxyManager) => proxyManager.Cancel(), doActionsInParallel: true);
+            this.DoActionOnAllManagers((proxyManager) => proxyManager.Cancel(runEventsHandler), doActionsInParallel: true);
         }
 
         public void Close()

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

@@ -168,8 +168,15 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsH
         /// <summary>
         /// Cancels the test run.
         /// </summary>
-        public virtual void Cancel()
+        /// <param name="eventHandler"> EventHandler for handling execution events from Engine. </param>
+        public virtual void Cancel(ITestRunEventsHandler eventHandler)
         {
+            // Just in case ExecuteAsync isn't called yet, set the eventhandler
+            if(this.baseTestRunEventsHandler == null)
+            {
+                this.baseTestRunEventsHandler = eventHandler;
+            }
+
             // Cancel fast, try to stop testhost deployment/launch
             this.cancellationTokenSource.Cancel();
             if (this.isCommunicationEstablished)
@@ -187,8 +194,15 @@ public virtual int LaunchProcessWithDebuggerAttached(TestProcessStartInfo testPr
         /// <summary>
         /// Aborts the test run.
         /// </summary>
-        public void Abort()
+        /// <param name="eventHandler"> EventHandler for handling execution events from Engine. </param>
+        public void Abort(ITestRunEventsHandler eventHandler)
         {
+            // Just in case ExecuteAsync isn't called yet, set the eventhandler
+            if(this.baseTestRunEventsHandler == null)
+            {
+                this.baseTestRunEventsHandler = eventHandler;
+            }
+
             this.RequestSender.SendTestRunAbort();
         }
 

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

@@ -122,15 +122,15 @@ public override int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEvents
         }
 
         /// <inheritdoc/>
-        public override void Cancel()
+        public override void Cancel(ITestRunEventsHandler eventHandler)
         {
             try
             {
                 this.ProxyDataCollectionManager.AfterTestRunEnd(isCanceled: true, runEventsHandler: this.DataCollectionRunEventsHandler);
             }
             finally
             {
-                base.Cancel();
+                base.Cancel(eventHandler);
             }
         }
 

src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs

@@ -321,7 +321,7 @@ public void OnMessageReceived(object sender, MessageReceivedEventArgs messageRec
                 case MessageType.CancelTestRun:
                     jobQueue.Pause();
                     this.testHostManagerFactoryReady.Wait();
-                    testHostManagerFactory.GetExecutionManager().Cancel();
+                    testHostManagerFactory.GetExecutionManager().Cancel(new TestRunEventsHandler(this));
                     break;
 
                 case MessageType.LaunchAdapterProcessWithDebuggerAttachedCallback:
@@ -331,7 +331,7 @@ public void OnMessageReceived(object sender, MessageReceivedEventArgs messageRec
                 case MessageType.AbortTestRun:
                     jobQueue.Pause();
                     this.testHostManagerFactoryReady.Wait();
-                    testHostManagerFactory.GetExecutionManager().Abort();
+                    testHostManagerFactory.GetExecutionManager().Abort(new TestRunEventsHandler(this));
                     break;
 
                 case MessageType.SessionEnd:

src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs

@@ -22,8 +22,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution
     /// </summary>
     public class ExecutionManager : IExecutionManager
     {
-        private ITestRunEventsHandler testRunEventsHandler;
-
         private readonly ITestPlatformEventSource testPlatformEventSource;
 
         private BaseRunTests activeTestRun;
@@ -85,7 +83,6 @@ public void StartTestRun(
             ITestCaseEventsHandler testCaseEventsHandler,
             ITestRunEventsHandler runEventsHandler)
         {
-            this.testRunEventsHandler = runEventsHandler;
             try
             {
                 this.activeTestRun = new RunTestsWithSources(this.requestData, adapterSourceMap, package, runSettings, testExecutionContext, testCaseEventsHandler, runEventsHandler);
@@ -94,8 +91,8 @@ public void StartTestRun(
             }
             catch(Exception e)
             {
-                this.testRunEventsHandler.HandleLogMessage(ObjectModel.Logging.TestMessageLevel.Error, e.ToString());
-                this.Abort();
+                runEventsHandler.HandleLogMessage(ObjectModel.Logging.TestMessageLevel.Error, e.ToString());
+                this.Abort(runEventsHandler);
             }
             finally
             {
@@ -120,8 +117,6 @@ public void StartTestRun(
             ITestCaseEventsHandler testCaseEventsHandler,
             ITestRunEventsHandler runEventsHandler)
         {
-            this.testRunEventsHandler = runEventsHandler;
-
             try
             {
                 this.activeTestRun = new RunTestsWithTests(this.requestData, tests, package, runSettings, testExecutionContext, testCaseEventsHandler, runEventsHandler);
@@ -130,8 +125,8 @@ public void StartTestRun(
             }
             catch(Exception e)
             {
-                this.testRunEventsHandler.HandleLogMessage(ObjectModel.Logging.TestMessageLevel.Error, e.ToString());
-                this.Abort();
+                runEventsHandler.HandleLogMessage(ObjectModel.Logging.TestMessageLevel.Error, e.ToString());
+                this.Abort(runEventsHandler);
             }
             finally
             {
@@ -142,12 +137,12 @@ public void StartTestRun(
         /// <summary>
         /// Cancel the test execution.
         /// </summary>
-        public void Cancel()
+        public void Cancel(ITestRunEventsHandler testRunEventsHandler)
         {
             if (this.activeTestRun == null)
             {
                 var testRunCompleteEventArgs = new TestRunCompleteEventArgs(null, true, false, null, null, TimeSpan.Zero);
-                this.testRunEventsHandler.HandleTestRunComplete(testRunCompleteEventArgs, null, null, null);
+                testRunEventsHandler.HandleTestRunComplete(testRunCompleteEventArgs, null, null, null);
             }
             else
             {
@@ -158,12 +153,12 @@ public void Cancel()
         /// <summary>
         /// Aborts the test execution.
         /// </summary>
-        public void Abort()
+        public void Abort(ITestRunEventsHandler testRunEventsHandler)
         {
             if (this.activeTestRun == null)
             {
                 var testRunCompleteEventArgs = new TestRunCompleteEventArgs(null, false, true, null, null, TimeSpan.Zero);
-                this.testRunEventsHandler.HandleTestRunComplete(testRunCompleteEventArgs, null, null, null);
+                testRunEventsHandler.HandleTestRunComplete(testRunCompleteEventArgs, null, null, null);
             }
             else
             {

src/vstest.console/TestPlatformHelpers/TestRequestManager.cs

@@ -60,7 +60,7 @@ internal class TestRequestManager : ITestRequestManager
         /// </summary>
         private ITestRunRequest currentTestRunRequest;
 
-        private readonly EventWaitHandle runRequestCreatedEventHandle = new AutoResetEvent(false);
+        private readonly EventWaitHandle runRequestStartedEventHandle = new AutoResetEvent(false);
 
         private object syncobject = new object();
 
@@ -322,7 +322,7 @@ public void CancelTestRun()
         {
             EqtTrace.Info("TestRequestManager.CancelTestRun: Sending cancel request.");
 
-            this.runRequestCreatedEventHandle.WaitOne(runRequestTimeout);
+            this.runRequestStartedEventHandle.WaitOne(runRequestTimeout);
             this.currentTestRunRequest?.CancelAsync();
         }
 
@@ -333,7 +333,7 @@ public void AbortTestRun()
         {
             EqtTrace.Info("TestRequestManager.AbortTestRun: Sending abort request.");
 
-            this.runRequestCreatedEventHandle.WaitOne(runRequestTimeout);
+            this.runRequestStartedEventHandle.WaitOne(runRequestTimeout);
             this.currentTestRunRequest?.Abort();
         }
 
@@ -457,7 +457,6 @@ private bool RunTests(IRequestData requestData, TestRunCriteria testRunCriteria,
                 try
                 {
                     this.currentTestRunRequest = this.testPlatform.CreateTestRunRequest(requestData, testRunCriteria);
-                    this.runRequestCreatedEventHandle.Set();
 
                     this.testLoggerManager.RegisterTestRunEvents(this.currentTestRunRequest);
                     this.testRunResultAggregator.RegisterTestRunEvents(this.currentTestRunRequest);
@@ -466,6 +465,8 @@ private bool RunTests(IRequestData requestData, TestRunCriteria testRunCriteria,
                     this.testPlatformEventSource.ExecutionRequestStart();
 
                     this.currentTestRunRequest.ExecuteAsync();
+
+                    this.runRequestStartedEventHandle.Set();
 
                     // Wait for the run completion event
                     this.currentTestRunRequest.WaitForCompletion();

test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs

@@ -106,7 +106,7 @@ public void AbortIfTestRunStateIsNotInProgressShouldNotCallExecutionManagerAbort
         {
             //ExecuteAsync has not been called, so State is not InProgress
             testRunRequest.Abort();
-            executionManager.Verify(dm => dm.Abort(), Times.Never);
+            executionManager.Verify(dm => dm.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Never);
         }
 
         [TestMethod]
@@ -116,7 +116,7 @@ public void AbortIfDiscoveryIsinProgressShouldCallDiscoveryManagerAbort()
             testRunRequest.ExecuteAsync();
 
             testRunRequest.Abort();
-            executionManager.Verify(dm => dm.Abort(), Times.Once);
+            executionManager.Verify(dm => dm.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Once);
         }
 
         [TestMethod]
@@ -143,15 +143,15 @@ public void CancelAsyncIfTestRunRequestDisposedShouldNotThrowException()
         public void CancelAsyncIfTestRunStateNotInProgressWillNotCallExecutionManagerCancel()
         {
             testRunRequest.CancelAsync();
-            executionManager.Verify(dm => dm.Cancel(), Times.Never);
+            executionManager.Verify(dm => dm.Cancel(It.IsAny<ITestRunEventsHandler>()), Times.Never);
         }
 
         [TestMethod]
         public void CancelAsyncIfTestRunStateInProgressCallsExecutionManagerCancel()
         {
             testRunRequest.ExecuteAsync();
             testRunRequest.CancelAsync();
-            executionManager.Verify(dm => dm.Cancel(), Times.Once);
+            executionManager.Verify(dm => dm.Cancel(It.IsAny<ITestRunEventsHandler>()), Times.Once);
         }
 
 
@@ -160,7 +160,7 @@ public void OnTestSessionTimeoutShouldCallAbort()
         {
             this.testRunRequest.ExecuteAsync();
             this.testRunRequest.OnTestSessionTimeout(null);
-            this.executionManager.Verify(o => o.Abort(), Times.Once);
+            this.executionManager.Verify(o => o.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Once);
         }
 
         [TestMethod]
@@ -202,12 +202,12 @@ public void OnTestSessionTimeoutShouldGetCalledWhenExecutionCrossedTestSessionTi
 
             ManualResetEvent onTestSessionTimeoutCalled = new ManualResetEvent(true);
             onTestSessionTimeoutCalled.Reset();
-            executionManager.Setup(o => o.Abort()).Callback(() => onTestSessionTimeoutCalled.Set());
+            executionManager.Setup(o => o.Abort(It.IsAny<ITestRunEventsHandler>())).Callback(() => onTestSessionTimeoutCalled.Set());
 
             testRunRequest.ExecuteAsync();
             onTestSessionTimeoutCalled.WaitOne(20 * 1000);
 
-            executionManager.Verify(o => o.Abort(), Times.Once);
+            executionManager.Verify(o => o.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Once);
         }
 
         /// <summary>
@@ -232,7 +232,7 @@ public void OnTestSessionTimeoutShouldNotGetCalledWhenTestSessionTimeoutIsZero()
 
             testRunRequest.ExecuteAsync();
 
-            executionManager.Verify(o => o.Abort(), Times.Never);
+            executionManager.Verify(o => o.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Never);
         }
 
         [TestMethod]

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/InProcessProxyexecutionManagerTests.cs

@@ -179,10 +179,10 @@ public void AbortShouldCallExecutionManagerAbort()
         {
             var manualResetEvent = new ManualResetEvent(true);
 
-            this.mockExecutionManager.Setup(o => o.Abort()).Callback(
+            this.mockExecutionManager.Setup(o => o.Abort(It.IsAny<ITestRunEventsHandler>())).Callback(
                 () => manualResetEvent.Set());
 
-            this.inProcessProxyExecutionManager.Abort();
+            this.inProcessProxyExecutionManager.Abort(It.IsAny<ITestRunEventsHandler>());
 
             Assert.IsTrue(manualResetEvent.WaitOne(5000), "IExecutionManager.Abort should get called");
         }
@@ -192,10 +192,10 @@ public void CancelShouldCallExecutionManagerCancel()
         {
             var manualResetEvent = new ManualResetEvent(true);
 
-            this.mockExecutionManager.Setup(o => o.Cancel()).Callback(
+            this.mockExecutionManager.Setup(o => o.Cancel(It.IsAny<ITestRunEventsHandler>())).Callback(
                 () => manualResetEvent.Set());
 
-            this.inProcessProxyExecutionManager.Cancel();
+            this.inProcessProxyExecutionManager.Cancel(It.IsAny<ITestRunEventsHandler>());
 
             Assert.IsTrue(manualResetEvent.WaitOne(5000), "IExecutionManager.Abort should get called");
         }