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

Clean up of ITestRuntimeProvider

  • Loading branch information...
mayankbansal018 committed Jul 10, 2017
commit 6798fa3435c42b130a88ff44d45e2dcd35f0e042
@@ -31,11 +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 testHostLaunchError = 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,6 +70,8 @@ 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.
@@ -78,8 +80,6 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr
/// <param name="cancellationToken"></param>
public virtual void SetupChannel(IEnumerable<string> sources, CancellationToken cancellationToken)
{
var connTimeout = this.connectionTimeout;
if (!this.initialized)
{
this.testHostProcessStdError = string.Empty;
@@ -92,36 +92,23 @@ public virtual void SetupChannel(IEnumerable<string> sources, CancellationToken
// Subscribe to TestHost Event
this.testHostManager.HostLaunched += this.TestHostManagerHostLaunched;
this.testHostManager.HostExited += this.TestHostManagerHostExited;
this.testHostManager.HostLaunchFailure += this.TestHostManagerHostLaunchFailure;

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));
try
{
// Launch the test host.
var hostLaunchedTask = this.testHostManager.LaunchTestHostAsync(testHostStartInfo, cancellationToken);
this.testHostProcessId = hostLaunchedTask.Result;
this.testHostLaunchSuccess = hostLaunchedTask.Result;
}
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}", this.testHostProcessId, this.processHelper.GetProcessName(this.testHostProcessId)),
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;
@@ -161,7 +148,7 @@ public virtual void Close()
try
{
// do not send message if host did not launch
if (this.testHostProcessId != -1)
if (this.testHostLaunchSuccess)
{
this.RequestSender.EndSession();
}
@@ -179,15 +166,14 @@ public virtual void Close()
// 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.testHostProcessId != -1 && !this.testHostExited.Wait(100)) || this.testHostLaunchError.Wait(0))
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.CleanTestHostAsync(this.testHostProcessId, CancellationToken.None).Wait();
this.testHostManager.CleanTestHostAsync(CancellationToken.None).Wait();
}
this.testHostManager.HostExited -= this.TestHostManagerHostExited;
this.testHostManager.HostLaunched -= this.TestHostManagerHostLaunched;
this.testHostManager.HostLaunchFailure -= this.TestHostManagerHostLaunchFailure;
}
}
@@ -224,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)
@@ -237,11 +242,5 @@ private void TestHostManagerHostExited(object sender, HostProviderEventArgs e)
this.testHostExited.Set();
}
private void TestHostManagerHostLaunchFailure(object sender, HostProviderEventArgs e)
{
EqtTrace.Verbose(e.Data);
this.testHostLaunchError.Set();
}
}
}
@@ -20,14 +20,10 @@ 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 launch reports an Error
/// </summary>
event EventHandler<HostProviderEventArgs> HostLaunchFailure;
/// <summary>
/// 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>
@@ -72,7 +68,7 @@ public interface ITestRuntimeProvider
/// <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, CancellationToken cancellationToken);
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.
@@ -96,16 +92,13 @@ public interface ITestRuntimeProvider
/// <summary>
/// Terminate the test host process.
/// </summary>
/// <param name="processId">
/// Process identifier for the test host.
/// </param>
/// <param name="cancellationToken">
/// Cancellation token.
/// </param>
/// <returns>
/// The <see cref="Task"/>.
/// </returns>
Task CleanTestHostAsync(int processId, CancellationToken cancellationToken);
Task CleanTestHostAsync(CancellationToken cancellationToken);
}
/// <summary>
@@ -78,9 +78,6 @@ internal DefaultTestHostManager(IProcessHelper processHelper, IEnvironment envir
/// <inheritdoc/>
public event EventHandler<HostProviderEventArgs> HostExited;
/// <inheritdoc/>
public event EventHandler<HostProviderEventArgs> HostLaunchFailure;
/// <inheritdoc/>
public bool Shared { get; private set; }
@@ -117,7 +114,7 @@ public void SetCustomLauncher(ITestHostLauncher customLauncher)
}
/// <inheritdoc/>
public async Task<int> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
public virtual async Task<bool> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
return await Task.Run(() => this.LaunchHost(testHostStartInfo, cancellationToken), cancellationToken);
}
@@ -205,11 +202,11 @@ public void Initialize(IMessageLogger logger, string runsettingsXml)
}
/// <inheritdoc/>
public Task CleanTestHostAsync(int processId, CancellationToken cancellationToken)
public Task CleanTestHostAsync(CancellationToken cancellationToken)
{
try
{
this.processHelper.TerminateProcess(processId);
this.processHelper.TerminateProcess(this.testHostProcess.Id);

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

Can testHostProcess be null?

@codito

codito Jul 11, 2017

Contributor

Can testHostProcess be null?

}
catch (Exception ex)
{
@@ -225,7 +222,7 @@ public Task CleanTestHostAsync(int processId, CancellationToken cancellationToke
/// Raises HostLaunched event
/// </summary>
/// <param name="e">hostprovider event args</param>
private void OnHostLaunched(HostProviderEventArgs e)
protected virtual void OnHostLaunched(HostProviderEventArgs e)

This comment has been minimized.

@codito

codito Jul 11, 2017

Contributor

I am not sure if making a event handler virtual is a good idea.

@codito

codito Jul 11, 2017

Contributor

I am not sure if making a event handler virtual is a good idea.

{
this.HostLaunched.SafeInvoke(this, e, "HostProviderEvents.OnHostLaunched");
}
@@ -234,7 +231,7 @@ private void OnHostLaunched(HostProviderEventArgs e)
/// Raises HostExited event
/// </summary>
/// <param name="e">hostprovider event args</param>
private void OnHostExited(HostProviderEventArgs e)
protected virtual void OnHostExited(HostProviderEventArgs e)
{
if (!this.hostExitedEventRaised)
{
@@ -243,12 +240,7 @@ private void OnHostExited(HostProviderEventArgs e)
}
}
private void OnHostLaunchFailure(HostProviderEventArgs e)
{
this.HostLaunchFailure.SafeInvoke(this, e, "HostProviderEvents.HostLaunchError");
}
private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
try
{
@@ -270,13 +262,12 @@ private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken
}
catch (OperationCanceledException ex)
{
this.OnHostLaunchFailure(new HostProviderEventArgs(ex.Message, -1, 0));
return -1;
this.messageLogger.SendMessage(TestMessageLevel.Error, ex.Message);
return false;
}
var pId = this.testHostProcess != null ? this.testHostProcess.Id : 0;
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched with Pid: " + pId));
return pId;
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched", 0, this.testHostProcess.Id));
return this.testHostProcess != null;
}
}
}
@@ -93,9 +93,6 @@ public DotnetTestHostManager()
/// <inheritdoc />
public event EventHandler<HostProviderEventArgs> HostExited;
/// <inheritdoc />
public event EventHandler<HostProviderEventArgs> HostLaunchFailure;
/// <summary>
/// Gets a value indicating whether gets a value indicating if the test host can be shared for multiple sources.
/// </summary>
@@ -145,7 +142,7 @@ public void SetCustomLauncher(ITestHostLauncher customLauncher)
}
/// <inheritdoc/>
public virtual async Task<int> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
public virtual async Task<bool> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
return await Task.Run(() => this.LaunchHost(testHostStartInfo, cancellationToken), cancellationToken);
}
@@ -270,11 +267,11 @@ public bool CanExecuteCurrentRunConfiguration(string runsettingsXml)
}
/// <inheritdoc/>
public Task CleanTestHostAsync(int processId, CancellationToken cancellationToken)
public Task CleanTestHostAsync(CancellationToken cancellationToken)
{
try
{
this.processHelper.TerminateProcess(processId);
this.processHelper.TerminateProcess(this.testHostProcess.Id);
}
catch (Exception ex)
{
@@ -308,12 +305,7 @@ private void OnHostExited(HostProviderEventArgs e)
}
}
private void OnHostLaunchFailure(HostProviderEventArgs e)
{
this.HostLaunchFailure.SafeInvoke(this, e, "HostProviderEvents.HostLaunchFailure");
}
private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
try
{
@@ -333,14 +325,13 @@ private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken
}
catch (OperationCanceledException ex)
{
this.OnHostLaunchFailure(new HostProviderEventArgs(ex.Message, -1, 0));
return -1;
this.messageLogger.SendMessage(TestMessageLevel.Error, ex.Message);
return false;
}
var pId = this.testHostProcess != null ? this.testHostProcess.Id : 0;
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched with Pid: " + pId));
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched", 0, this.testHostProcess.Id));
return pId;
return this.testHostProcess != null;
}
private string GetTestHostPath(string runtimeConfigDevPath, string depsFilePath, string sourceDirectory)
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.