Skip to content

Fix for a Cancellation timing issue #1397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AbhitejJohn wants to merge 1 commit into from
Closed

Fix for a Cancellation timing issue #1397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ public void CancelAsync()
else
{
// Inform the service about run cancellation
this.ExecutionManager.Cancel();
this.ExecutionManager.Cancel(this);
}
}

Expand Down Expand Up @@ -279,7 +279,7 @@ public void Abort()
}
else
{
this.ExecutionManager.Abort();
this.ExecutionManager.Abort(this);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyExecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ public interface IProxyExecutionManager
/// <summary>
/// Cancels the test run. On the test host, this will send a message to adapters.
/// </summary>
void Cancel();
void Cancel(ITestRunEventsHandler eventHandler);

/// <summary>
/// Aborts the test operation. This will forcefully terminate the test host.
/// </summary>
void Abort();
void Abort(ITestRunEventsHandler eventHandler);

/// <summary>
/// Closes the current test operation by sending a end session message.
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.TestPlatform.Common/Interfaces/Engine/TesthostProtocol/IExecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
10 changes: 6 additions & 4 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyexecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@ 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)
{
// Cancel fast, try to stop testhost deployment/launch

@singhsarab singhsarab Jan 31, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on our assumption everywhere else, that is Cancel comes before StartTestRun.
If Cancel comes, it will result in a HandleTestRunComplete, where we have a baseTestRunEventsHandler which only gets set if Start was called, else it could also potentially cause null reference.

this.cancellationTokenSource.Cancel();
Expand All @@ -187,7 +188,8 @@ 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)
{
this.RequestSender.SendTestRunAbort();
}
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManagerWithDataCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down
21 changes: 8 additions & 13 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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
{
Expand All @@ -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);
Expand All @@ -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
{
Expand All @@ -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
{
Expand All @@ -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
{
Expand Down
16 changes: 8 additions & 8 deletions test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,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]
Expand All @@ -118,7 +118,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]
Expand All @@ -145,15 +145,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);
}


Expand All @@ -162,7 +162,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]
Expand Down Expand Up @@ -204,12 +204,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>
Expand All @@ -234,7 +234,7 @@ public void OnTestSessionTimeoutShouldNotGetCalledWhenTestSessionTimeoutIsZero()

testRunRequest.ExecuteAsync();

executionManager.Verify(o => o.Abort(), Times.Never);
executionManager.Verify(o => o.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Never);
}

[TestMethod]
Expand Down
8 changes: 4 additions & 4 deletions ...soft.TestPlatform.CrossPlatEngine.UnitTests/Client/InProcessProxyexecutionManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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");
}
Expand Down
10 changes: 5 additions & 5 deletions ...tPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyExecutionManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,21 +89,21 @@ public void AbortShouldCallAllConcurrentManagersOnce()
{
var parallelExecutionManager = new ParallelProxyExecutionManager(this.mockRequestData.Object, this.proxyManagerFunc, 4);

parallelExecutionManager.Abort();
parallelExecutionManager.Abort(It.IsAny<ITestRunEventsHandler>());

Assert.AreEqual(4, createdMockManagers.Count, "Number of Concurrent Managers created should be 4");
createdMockManagers.ForEach(em => em.Verify(m => m.Abort(), Times.Once));
createdMockManagers.ForEach(em => em.Verify(m => m.Abort(It.IsAny<ITestRunEventsHandler>()), Times.Once));
}

[TestMethod]
public void CancelShouldCallAllConcurrentManagersOnce()
{
var parallelExecutionManager = new ParallelProxyExecutionManager(this.mockRequestData.Object, this.proxyManagerFunc, 4);

parallelExecutionManager.Cancel();
parallelExecutionManager.Cancel(It.IsAny<ITestRunEventsHandler>());

Assert.AreEqual(4, createdMockManagers.Count, "Number of Concurrent Managers created should be 4");
createdMockManagers.ForEach(em => em.Verify(m => m.Cancel(), Times.Once));
createdMockManagers.ForEach(em => em.Verify(m => m.Cancel(It.IsAny<ITestRunEventsHandler>()), Times.Once));
}

[TestMethod]
Expand Down Expand Up @@ -215,7 +215,7 @@ public void StartTestRunShouldNotProcessAllSourcesOnExecutionAborted()
this.SetupMockManagers(this.processedSources, isCanceled: false, isAborted: false);
SetupHandleTestRunComplete(this.executionCompleted);

parallelExecutionManager.Abort();
parallelExecutionManager.Abort(It.IsAny<ITestRunEventsHandler>());
Task.Run(() => { parallelExecutionManager.StartTestRun(this.testRunCriteriaWithSources, this.mockHandler.Object); });

Assert.IsTrue(this.executionCompleted.Wait(taskTimeout), "Test run not completed.");
Expand Down
2 changes: 1 addition & 1 deletion test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ public void CancelShouldNotSendSendTestRunCancelIfCommunicationFails()

this.testExecutionManager.StartTestRun(this.mockTestRunCriteria.Object, mockTestRunEventsHandler.Object);

this.testExecutionManager.Cancel();
this.testExecutionManager.Cancel(It.IsAny<ITestRunEventsHandler>());

this.mockRequestSender.Verify(s => s.SendTestRunCancel(), Times.Never);
}
Expand Down
4 changes: 2 additions & 2 deletions ...Platform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerWithDataCollectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public void InitializeShouldSaveExceptionMessagesIfThrownByDataCollectionProcess
[TestMethod]
public void CancelShouldInvokeAfterTestCaseEnd()
{
this.proxyExecutionManager.Cancel();
this.proxyExecutionManager.Cancel(It.IsAny<ITestRunEventsHandler>());

this.mockDataCollectionManager.Verify(x => x.AfterTestRunEnd(true, It.IsAny<ITestMessageEventHandler>()), Times.Once);
}
Expand All @@ -127,7 +127,7 @@ public void CancelShouldThrowExceptionIfThrownByProxyDataCollectionManager()

Assert.ThrowsException<Exception>(() =>
{
this.proxyExecutionManager.Cancel();
this.proxyExecutionManager.Cancel(It.IsAny<ITestRunEventsHandler>());
});
}

Expand Down
Loading