Skip to content

Commit

Permalink
Porting fix for Design mode clients hang for errors (#1451)
Browse files Browse the repository at this point in the history 
* Fix: Design mode clients hang for errors

* spacing

* Update ProxyDiscoveryManager.cs
  • Loading branch information
@abhishkk
abhishkk authored Mar 16, 2018
1 parent 891387d commit 3391a4c
Show file tree
Hide file tree
Showing 8 changed files with 313 additions and 28 deletions.
32 changes: 24 additions & 8 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()
{
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
26 changes: 21 additions & 5 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand Down
7 changes: 7 additions & 0 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ 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>());
Expand Down
10 changes: 8 additions & 2 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -262,4 +268,4 @@ private void InitializeExtensions(IEnumerable<string> sources)
}
}
}
}
}
48 changes: 47 additions & 1 deletion ...tPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyDiscoveryManagerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -204,4 +250,4 @@ private void AssertMissingAndDuplicateSources(List<string> processedSources)
}
}
}
}
}
Loading

0 comments on commit 3391a4c

Please sign in to comment.