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.

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

Adding CancellationToken to Testhost launch async #917

Merged
merged 9 commits into from Jul 11, 2017
@@ -7,6 +7,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework;
@@ -26,6 +27,7 @@ public class ProxyDiscoveryManager : ProxyOperationManager, IProxyDiscoveryManag
{
private readonly ITestRuntimeProvider testHostManager;
private IDataSerializer dataSerializer;
private CancellationTokenSource cancellationTokenSource;
#region Constructors
@@ -50,6 +52,7 @@ public ProxyDiscoveryManager(ITestRequestSender testRequestSender, ITestRuntimeP
/// <param name="testHostManager">
/// Test host Manager instance
/// </param>
/// <param name="dataSerializer"></param>
/// <param name="clientConnectionTimeout">
/// The client Connection Timeout
/// </param>
@@ -62,6 +65,7 @@ public ProxyDiscoveryManager(ITestRequestSender testRequestSender, ITestRuntimeP
{
this.dataSerializer = dataSerializer;
this.testHostManager = testHostManager;
this.cancellationTokenSource = new CancellationTokenSource();
}
#endregion
@@ -97,7 +101,7 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve
this.InitializeExtensions(discoveryCriteria.Sources);
}
this.SetupChannel(discoveryCriteria.Sources);
this.SetupChannel(discoveryCriteria.Sources, this.cancellationTokenSource.Token);
this.RequestSender.DiscoverTests(discoveryCriteria, eventHandler);
}
catch (Exception exception)
@@ -146,7 +150,7 @@ private void InitializeExtensions(IEnumerable<string> sources)
// Only send this if needed.
if (platformExtensions.Any())
{
this.SetupChannel(sourceList);
this.SetupChannel(sourceList, this.cancellationTokenSource.Token);
this.RequestSender.InitializeDiscovery(platformExtensions, TestPluginCache.Instance.LoadOnlyWellKnownExtensions);
}
@@ -7,7 +7,10 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
@@ -20,8 +23,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
using Constants = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Constants;
using System.Text.RegularExpressions;
using Microsoft.VisualStudio.TestPlatform.Common;
/// <summary>
/// Orchestrates test execution operations for the engine communicating with the client.
@@ -30,17 +31,17 @@ internal class ProxyExecutionManager : ProxyOperationManager, IProxyExecutionMan
{
private readonly ITestRuntimeProvider testHostManager;
private IDataSerializer dataSerializer;
private CancellationTokenSource cancellationTokenSource;
/// <inheritdoc/>
public bool IsInitialized { get; private set; } = false;
#region Constructors
/// <summary>
/// Initializes a new instance of the <see cref="ProxyExecutionManager"/> class.
/// </summary>
/// <param name="testRequestSender">Test request sender instance.</param>
/// <param name="requestSender">Test request sender instance.</param>
/// <param name="testHostManager">Test host manager for this proxy.</param>
public ProxyExecutionManager(ITestRequestSender requestSender, ITestRuntimeProvider testHostManager) : this(requestSender, testHostManager, JsonDataSerializer.Instance, Constants.ClientConnectionTimeout)
{
@@ -52,12 +53,14 @@ public ProxyExecutionManager(ITestRequestSender requestSender, ITestRuntimeProvi
/// </summary>
/// <param name="requestSender">Request Sender instance</param>
/// <param name="testHostManager">Test host manager instance</param>
/// <param name="dataSerializer"></param>
/// <param name="clientConnectionTimeout">The client Connection Timeout</param>
internal ProxyExecutionManager(ITestRequestSender requestSender, ITestRuntimeProvider testHostManager, IDataSerializer dataSerializer, int clientConnectionTimeout)
: base(requestSender, testHostManager, clientConnectionTimeout)
{
this.testHostManager = testHostManager;
this.dataSerializer = dataSerializer;
this.cancellationTokenSource = new CancellationTokenSource();
}
#endregion
@@ -76,6 +79,7 @@ public virtual void Initialize()
EqtTrace.Verbose("ProxyExecutionManager: Test host is shared. SetupChannel it early.");
this.InitializeExtensions(Enumerable.Empty<string>());
}
this.IsInitialized = true;
}
@@ -105,7 +109,7 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsH
this.InitializeExtensions(testSources);
}
this.SetupChannel(testRunCriteria.Sources);
this.SetupChannel(testRunCriteria.Sources, this.cancellationTokenSource.Token);

This comment has been minimized.

@codito

codito Jul 10, 2017

Contributor

Should we consider defining this CancellationTokenSource in ProxyOperationManager?

@codito

codito Jul 10, 2017

Contributor

Should we consider defining this CancellationTokenSource in ProxyOperationManager?

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

next PR

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

next PR

var executionContext = new TestExecutionContext(
testRunCriteria.FrequencyOfRunStatsChangeEvent,
@@ -191,7 +195,7 @@ private void InitializeExtensions(IEnumerable<string> sources)
// Only send this if needed.
if (platformExtensions.Any())
{
this.SetupChannel(sourceList);
this.SetupChannel(sourceList, this.cancellationTokenSource.Token);
this.RequestSender.InitializeExecution(platformExtensions, TestPluginCache.Instance.LoadOnlyWellKnownExtensions);
}
@@ -7,20 +7,19 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;
using Microsoft.VisualStudio.TestPlatform.Utilities;
using CrossPlatEngineResources = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Resources.Resources;
using System.Reflection;
using System.Linq;
/// <summary>
/// Base class for any operations that the client needs to drive through the engine.
@@ -32,10 +31,11 @@ public abstract class ProxyOperationManager
private readonly int connectionTimeout;
private readonly string versionCheckPropertyName = "IsVersionCheckRequired";
private readonly ManualResetEventSlim testHostExited = new ManualResetEventSlim(false);
private readonly ManualResetEventSlim testHostConnectionTimeOut = new ManualResetEventSlim(false);

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

nit: timeout is one word

@codito

codito Jul 11, 2017

Contributor

nit: timeout is one word

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

Looking at usage, suggest to name it as channelConnected.

@codito

codito Jul 11, 2017

Contributor

Looking at usage, suggest to name it as channelConnected.

private bool initialized;
private string testHostProcessStdError;
private int testHostProcessId;
private bool testHostLaunchSuccess;

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

nit: testHostLaunched

@codito

codito Jul 11, 2017

Contributor

nit: testHostLaunched

#region Constructors
@@ -52,7 +52,7 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr
this.testHostManager = testHostManager;
this.processHelper = new ProcessHelper();
this.initialized = false;
this.testHostProcessId = -1;
this.testHostLaunchSuccess = false;
}
#endregion
@@ -70,15 +70,16 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr
#region IProxyOperationManager implementation.
// ToDo: change SetupChannel to return bool, which will specify that a successfull connection with testhost was established or not

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

nit: inside SetupChannel method body.

@codito

codito Jul 11, 2017

Contributor

nit: inside SetupChannel method body.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

/// <summary>
/// Ensure that the engine is ready for test operations.
/// Usually includes starting up the test host process.
/// </summary>
/// <param name="sources">List of test sources.</param>
public virtual void SetupChannel(IEnumerable<string> sources)
/// <param name="cancellationToken"></param>
public virtual void SetupChannel(IEnumerable<string> sources, CancellationToken cancellationToken)
{
var connTimeout = this.connectionTimeout;
if (!this.initialized)
{
this.testHostProcessStdError = string.Empty;
@@ -91,41 +92,27 @@ public virtual void SetupChannel(IEnumerable<string> sources)
// Subscribe to TestHost Event
this.testHostManager.HostLaunched += this.TestHostManagerHostLaunched;
this.testHostManager.HostExited += this.TestHostManagerHostExited;

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

nit: remove extra space

@codito

codito Jul 11, 2017

Contributor

nit: remove extra space

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

// Get the test process start info
var testHostStartInfo = this.UpdateTestProcessStartInfo(this.testHostManager.GetTestHostProcessStartInfo(sources, null, connectionInfo));
// Launch the test host.
var hostLaunchedTask = this.testHostManager.LaunchTestHostAsync(testHostStartInfo);
try
{
this.testHostProcessId = hostLaunchedTask.Result;
// Launch the test host.
var hostLaunchedTask = this.testHostManager.LaunchTestHostAsync(testHostStartInfo, cancellationToken);
this.testHostLaunchSuccess = hostLaunchedTask.Result;
}
catch (OperationCanceledException ex)
catch (Exception ex)
{
throw new TestPlatformException(string.Format(CultureInfo.CurrentUICulture, ex.Message));
}
// Warn the user that execution will wait for debugger attach.
var hostDebugEnabled = Environment.GetEnvironmentVariable("VSTEST_HOST_DEBUG");
if (!string.IsNullOrEmpty(hostDebugEnabled) && hostDebugEnabled.Equals("1", StringComparison.Ordinal))
{
ConsoleOutput.Instance.WriteLine(CrossPlatEngineResources.HostDebuggerWarning, OutputLevel.Warning);
ConsoleOutput.Instance.WriteLine(
string.Format("Process Id: {0}, Name: {1}", hostLaunchedTask.Result, this.processHelper.GetProcessName(hostLaunchedTask.Result)),
OutputLevel.Information);
// Increase connection timeout when debugging is enabled.
connTimeout = 5 * this.connectionTimeout;
}
// Wait for a timeout for the client to connect.
if (!this.RequestSender.WaitForRequestHandlerConnection(connTimeout))
// test host launched failed because of user cancellation or any other runtime exception

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

nit: comments start with Caps

@codito

codito Jul 11, 2017

Contributor

nit: comments start with Caps

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

// or connection with test host did not happen
if (!this.testHostLaunchSuccess || this.testHostConnectionTimeOut.Wait(0))

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

ManualResetEventSlim.Wait == a bool type?

@codito

codito Jul 11, 2017

Contributor

ManualResetEventSlim.Wait == a bool type?

{
var errorMsg = CrossPlatEngineResources.InitializationFailed;
if (!string.IsNullOrWhiteSpace(this.testHostProcessStdError.ToString()))
if (!string.IsNullOrWhiteSpace(this.testHostProcessStdError))
{
// Testhost failed with error
errorMsg = string.Format(CrossPlatEngineResources.TestHostExitedWithError, this.testHostProcessStdError);
@@ -160,7 +147,11 @@ public virtual void Close()
{
try
{
this.RequestSender.EndSession();
// do not send message if host did not launch
if (this.testHostLaunchSuccess)
{
this.RequestSender.EndSession();
}
}
catch (Exception ex)
{
@@ -172,12 +163,13 @@ public virtual void Close()
{
this.initialized = false;
// We don't need to terminate if the test host has already terminated. The upper bound
// for wait should be 100ms.
if (this.testHostProcessId != -1 && !this.testHostExited.Wait(100))
// We don't need to terminate if the test host has already terminated.
// If the host launch fails, please clean test host.

This comment has been minimized.

@codito

codito Jul 10, 2017

Contributor

Can the pattern be simple?

  • If the consumer is exiting, always call CleanTestHostAsync. It is the responsibility of Clean to ensure that there are no lingering child processes

If there were no test host, it is up to CleanTestHostAsync to do what it chooses.

Thinking aloud, does IDisposable or Close make more sense than CleanXyz?

@codito

codito Jul 10, 2017

Contributor

Can the pattern be simple?

  • If the consumer is exiting, always call CleanTestHostAsync. It is the responsibility of Clean to ensure that there are no lingering child processes

If there were no test host, it is up to CleanTestHostAsync to do what it chooses.

Thinking aloud, does IDisposable or Close make more sense than CleanXyz?

// The upper bound for wait should be 100ms.
if (!this.testHostExited.Wait(100))

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

Clean should be called irrespective of whether the test host has exited or not.

@codito

codito Jul 11, 2017

Contributor

Clean should be called irrespective of whether the test host has exited or not.

{
EqtTrace.Warning("ProxyOperationManager: Timed out waiting for test host to exit. Will terminate process.");
this.testHostManager.TerminateAsync(this.testHostProcessId, CancellationToken.None).Wait();
this.testHostManager.CleanTestHostAsync(CancellationToken.None).Wait();
}
this.testHostManager.HostExited -= this.TestHostManagerHostExited;
@@ -205,7 +197,9 @@ protected virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessSta
protected string GetTimestampedLogFile(string logFile)
{
if (string.IsNullOrWhiteSpace(logFile))
{
return null;
}
return Path.ChangeExtension(
logFile,
@@ -216,9 +210,28 @@ protected string GetTimestampedLogFile(string logFile)
Path.GetExtension(logFile))).AddDoubleQuote();
}
private void TestHostManagerHostLaunched(object sender, HostProviderEventArgs e)
public void TestHostManagerHostLaunched(object sender, HostProviderEventArgs e)

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

Why is this public?

@codito

codito Jul 11, 2017

Contributor

Why is this public?

{
EqtTrace.Verbose(e.Data);
var connTimeout = this.connectionTimeout;
// Warn the user that execution will wait for debugger attach.
var hostDebugEnabled = Environment.GetEnvironmentVariable("VSTEST_HOST_DEBUG");
if (!string.IsNullOrEmpty(hostDebugEnabled) && hostDebugEnabled.Equals("1", StringComparison.Ordinal))
{
ConsoleOutput.Instance.WriteLine(CrossPlatEngineResources.HostDebuggerWarning, OutputLevel.Warning);
ConsoleOutput.Instance.WriteLine(
string.Format("Process Id: {0}, Name: {1}", e.ProcessId, this.processHelper.GetProcessName(e.ProcessId)),
OutputLevel.Information);
// Increase connection timeout when debugging is enabled.
connTimeout = 5 * this.connectionTimeout;
}
// Wait for a timeout for the client to connect.
if (!this.RequestSender.WaitForRequestHandlerConnection(connTimeout))
{
this.testHostConnectionTimeOut.Set();
}
}
private void TestHostManagerHostExited(object sender, HostProviderEventArgs e)
@@ -9,6 +9,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine
using Microsoft.VisualStudio.TestPlatform.Common.Hosting;
using Microsoft.VisualStudio.TestPlatform.Common.Logging;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection;
@@ -17,7 +18,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
/// <summary>
/// Cross Platform test engine entry point for the client.
@@ -5,12 +5,12 @@ namespace Microsoft.VisualStudio.TestPlatform.ObjectModel.Host
{
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
using System.Threading;
/// <summary>
/// Interface for TestRuntimeProvider which manages test host processes for test engine.
@@ -20,11 +20,12 @@ public interface ITestRuntimeProvider
#region events
/// <summary>
/// Raised when host is launched successfully
/// Consumed by TestPlatform to initialize connection b/w test host and testplatform
/// </summary>
event EventHandler<HostProviderEventArgs> HostLaunched;
/// <summary>
/// Raised when host is reports Error
/// Raised when host is cleaned up and removes all it's dependecies

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

nit: spell check

@codito

codito Jul 11, 2017

Contributor

nit: spell check

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

/// </summary>
event EventHandler<HostProviderEventArgs> HostExited;
@@ -65,8 +66,9 @@ public interface ITestRuntimeProvider
/// Launches the test host for discovery/execution.
/// </summary>
/// <param name="testHostStartInfo">Start parameters for the test host.</param>
/// <param name="cancellationToken"></param>
/// <returns>ProcessId of launched Process. 0 means not launched.</returns>

This comment has been minimized.

@Faizan2304

Faizan2304 Jul 11, 2017

Contributor

Need to update return string as the signature has been updated.

@Faizan2304

Faizan2304 Jul 11, 2017

Contributor

Need to update return string as the signature has been updated.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 11, 2017

Contributor

resolved

Task<int> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo);
Task<bool> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken);

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

Update documentation

@codito

codito Jul 11, 2017

Contributor

Update documentation

/// <summary>
/// Gets the start parameters for the test host.
@@ -83,15 +85,20 @@ public interface ITestRuntimeProvider
/// mechanism. E.g. for .net core, extensions are discovered from the <c>testproject.deps.json</c> file.
/// </summary>
/// <param name="sources">List of test sources.</param>
/// <param name="extensions"></param>
/// <returns>List of paths to extension assemblies.</returns>
IEnumerable<string> GetTestPlatformExtensions(IEnumerable<string> sources, IEnumerable<string> extensions);
/// <summary>
/// Terminate the test host process.
/// </summary>
/// <param name="processId">Process identifier for the test host.</param>
/// <param name="cancellationToken">Cancellation token.</param>
Task TerminateAsync(int processId, CancellationToken cancellationToken);
/// <param name="cancellationToken">
/// Cancellation token.
/// </param>
/// <returns>
/// The <see cref="Task"/>.
/// </returns>
Task CleanTestHostAsync(CancellationToken cancellationToken);
}
/// <summary>
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.