Skip to content
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

Read asynchronously from test host process. #529

Merged
merged 3 commits into from
Feb 22, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using Microsoft.VisualStudio.TestPlatform.Utilities;

using CrossPlatEngineResources = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Resources.Resources;
using System.Text;

/// <summary>
/// Base class for any operations that the client needs to drive through the engine.
Expand All @@ -29,8 +30,12 @@ public abstract class ProxyOperationManager

private readonly int connectionTimeout;

private readonly int errorLength;

private readonly IProcessHelper processHelper;

private StringBuilder testHostProcessStdError;

#region Constructors

/// <summary>
Expand All @@ -39,12 +44,14 @@ public abstract class ProxyOperationManager
/// <param name="requestSender">Request Sender instance.</param>
/// <param name="testHostManager">Test host manager instance.</param>
/// <param name="clientConnectionTimeout">Client Connection Timeout.</param>
protected ProxyOperationManager(ITestRequestSender requestSender, ITestHostManager testHostManager, int clientConnectionTimeout)
protected ProxyOperationManager(ITestRequestSender requestSender, ITestHostManager testHostManager, int clientConnectionTimeout, int errorLength = 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit arguments are better. Please call with 1000 from the public ctor.

{
this.RequestSender = requestSender;
this.connectionTimeout = clientConnectionTimeout;
this.testHostManager = testHostManager;
this.processHelper = new ProcessHelper();
this.errorLength = errorLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instance variable is not used any where?

testHostProcessStdError = new StringBuilder(errorLength, errorLength);
this.initialized = false;
}

Expand All @@ -68,8 +75,6 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestHostManag
/// <param name="sources">List of test sources.</param>
public virtual void SetupChannel(IEnumerable<string> sources)
{
string testHostProcessStdError = string.Empty;

if (!this.initialized)
{
var portNumber = this.RequestSender.InitializeCommunication();
Expand All @@ -82,16 +87,35 @@ public virtual void SetupChannel(IEnumerable<string> sources)

if (testHostStartInfo != null)
{
// Monitor testhost exit.
testHostStartInfo.ExitCallback = (process) =>
// Monitor testhost error callbacks.
testHostStartInfo.ErrorReceivedCallback = (process, data) =>
{
if (process.ExitCode != 0)
if(data != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: move this to a method.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if ( instead if(.

{
testHostProcessStdError = process.StandardError.ReadToEnd();
EqtTrace.Error("Test host exited with error: {0}", testHostProcessStdError);
//if incoming data stream is huge empty entire testError stream, & limit data stream to MaxCapacity
if (data.Length > testHostProcessStdError.MaxCapacity)
{
testHostProcessStdError.Remove(0, testHostProcessStdError.Length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Which of these is most efficient?

  1. StringBuilder.Clear followed by StringBuilder.Append
  2. StringBuilder.Remove(x, y) followed by StringBuilder.Append
  3. StringBuilder[i] = data[i]

data = data.Substring(data.Length - testHostProcessStdError.MaxCapacity);
Copy link
Contributor

Choose a reason for hiding this comment

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

We're keeping the first few characters, should it be last few characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are keeping last few only substring(index) which will give from starting index to end

}

//remove only what is required, from begining of error stream
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spelling

else
{
int required = data.Length + testHostProcessStdError.Length - testHostProcessStdError.MaxCapacity;
if (required > 0)
{
testHostProcessStdError.Remove(0, required);
}
}

testHostProcessStdError.Append(data);
}

this.RequestSender.OnClientProcessExit(testHostProcessStdError);
if (process.HasExited && process.ExitCode != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

process.HasExited can throw InvalidOperationException if no process associated with object, process.ExitCode can throw IOE if the process has not exited. is there a guaranty process.HasExited doesn't throw in callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested process.HasExited out, the process object is valid, even after the actual process has ended.
Process.ExitCode is only called if the process.HasExited is successful.

{
EqtTrace.Error("Test host exited with error: {0}", testHostProcessStdError);
this.RequestSender.OnClientProcessExit(testHostProcessStdError.ToString());
}
};
}

Expand All @@ -112,7 +136,7 @@ public virtual void SetupChannel(IEnumerable<string> sources)
{
var errorMsg = CrossPlatEngineResources.InitializationFailed;

if (!string.IsNullOrWhiteSpace(testHostProcessStdError))
if (!string.IsNullOrWhiteSpace(testHostProcessStdError.ToString()))
{
// Testhost failed with error
errorMsg = string.Format(CrossPlatEngineResources.TestHostExitedWithError, testHostProcessStdError);
Expand Down Expand Up @@ -146,5 +170,14 @@ private string GetTimestampedLogFile(string logFile)
string.Format("host.{0}_{1}{2}", DateTime.Now.ToString("yy-MM-dd_HH-mm-ss_fffff"),
Thread.CurrentThread.ManagedThreadId, Path.GetExtension(logFile)));
}

/// <summary>
/// Returns the current error data in stream
/// Written purely for UT as of now.
/// </summary>
public virtual string GetStandardError()
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it protected. Use a Testable in unit test.

{
return testHostProcessStdError.ToString();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ internal interface IProcessHelper
/// <param name="workingDirectory">The working directory for this process.</param>
/// <param name="exitCallback">Call back for on process exit</param>
/// <returns>The process created.</returns>
Process LaunchProcess(string processPath, string arguments, string workingDirectory, Action<Process> exitCallback);
Process LaunchProcess(string processPath, string arguments, string workingDirectory, Action<Process, string> errorCallback);

/// <summary>
/// Gets the current process file path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ internal class ProcessHelper : IProcessHelper
/// <param name="processPath">The path to the process.</param>
/// <param name="arguments">Process arguments.</param>
/// <param name="workingDirectory">Working directory of the process.</param>
/// <param name="exitCallback"></param>
/// <param name="errorCallback"></param>
/// <returns>The process spawned.</returns>
/// <exception cref="Exception">Throws any exception that could result as part of the launch.</exception>
public Process LaunchProcess(string processPath, string arguments, string workingDirectory, Action<Process> exitCallback)
public Process LaunchProcess(string processPath, string arguments, string workingDirectory, Action<Process, string> errorCallback)
{
var process = new Process();
try
Expand All @@ -39,13 +39,15 @@ public Process LaunchProcess(string processPath, string arguments, string workin
process.StartInfo.RedirectStandardError = true;
process.EnableRaisingEvents = true;

if (exitCallback != null)
if (errorCallback != null)
{
process.Exited += (sender, args) => exitCallback(sender as Process);
process.ErrorDataReceived += (sender, args) => errorCallback(sender as Process, args.Data);
}

EqtTrace.Verbose("ProcessHelper: Starting process '{0}' with command line '{1}'", processPath, arguments);
process.Start();

process.BeginErrorReadLine();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we require this even if errorCallback is not set?

}
catch (Exception exception)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public int LaunchTestHost(TestProcessStartInfo testHostStartInfo)

if (this.customTestHostLauncher == null)
{
this.testHostProcess = this.processHelper.LaunchProcess(testHostStartInfo.FileName, testHostStartInfo.Arguments, testHostStartInfo.WorkingDirectory, testHostStartInfo.ExitCallback);
this.testHostProcess = this.processHelper.LaunchProcess(testHostStartInfo.FileName, testHostStartInfo.Arguments, testHostStartInfo.WorkingDirectory, testHostStartInfo.ErrorReceivedCallback);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo)
defaultTestHostStartInfo.FileName,
defaultTestHostStartInfo.Arguments,
defaultTestHostStartInfo.WorkingDirectory,
defaultTestHostStartInfo.ExitCallback).Id;
defaultTestHostStartInfo.ErrorReceivedCallback).Id;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ public class TestProcessStartInfo
/// <summary>
/// Callback on process exit
/// </summary>
public Action<Process> ExitCallback { get; set; }
public Action<Process, string> ErrorReceivedCallback { get; set; }
}
}
3 changes: 2 additions & 1 deletion src/VSIXProject/source.extension.vsixmanifest
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011">
<Metadata>
<Identity Id="Microsoft.TestPlatform.VSIX.Microsoft.0e51abf6-9f5f-401e-a812-4043b87acabf" Version="$version$" Language="en-US" Publisher="Microsoft" />
<Identity Id="Microsoft.TestPlatform.VSIX.Microsoft.0e51abf6-9f5f-401e-a812-4043b87acabf" Version="15.0.3" Language="en-US" Publisher="Microsoft" />
<DisplayName>Microsoft Visual Studio Test Platform</DisplayName>
<Description xml:space="preserve">Extensible Test Platform that allows developers to discover, execute and analyze automated tests.</Description>
<License>License.rtf</License>
Expand All @@ -25,3 +25,4 @@
<Prerequisite Id="Microsoft.VisualStudio.Component.CoreEditor" Version="[15.0,16.0)" DisplayName="Visual Studio core editor" />
</Prerequisites>
</PackageManifest>

Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ public void SetupChannelShouldAddExitCallbackToTestHostStartInfo()

this.testOperationManager.SetupChannel(Enumerable.Empty<string>());

Assert.IsNotNull(startInfo.ExitCallback);
Assert.IsNotNull(startInfo.ErrorReceivedCallback);
}

[TestMethod]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace TestPlatform.CrossPlatEngine.UnitTests
{
using Microsoft.VisualStudio.TestTools.UnitTesting;

using Moq;
using System.Diagnostics;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Helpers.Interfaces;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using System;
using System.Reflection;
using System.Linq;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Helpers;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting;

[TestClass]
public class ProcessHelperTests
{
private readonly ProxyOperationManager testOperationManager;

private readonly TestableTestHostManager testHostManager;

private readonly Mock<ITestRequestSender> mockRequestSender;

private TestableProcessHelper processHelper;

private int connectionTimeout = 400;

private int errorLength = 20;

public ProcessHelperTests()
{
this.mockRequestSender = new Mock<ITestRequestSender>();
this.mockRequestSender.Setup(rs => rs.WaitForRequestHandlerConnection(this.connectionTimeout)).Returns(true);

this.processHelper = new TestableProcessHelper();
this.testHostManager = new TestableTestHostManager(Architecture.X64, Framework.DefaultFramework, this.processHelper, true);

this.testOperationManager = new TestableProxyOperationManager(this.mockRequestSender.Object, this.testHostManager, this.connectionTimeout, this.errorLength);
}

[TestMethod]
public void ErrorMessageShouldBeReadAsynchronously()
{
string errorData = "Custom Error Strings";
this.processHelper.SetErrorMessage(errorData);

this.mockRequestSender.Setup(rs => rs.InitializeCommunication()).Returns(123);

this.testOperationManager.SetupChannel(Enumerable.Empty<string>());

Assert.AreEqual(this.testOperationManager.GetStandardError(), errorData);
}

[TestMethod]
public void ErrorMessageShouldBeTruncatedToMatchErrorLength()
{
string errorData = "Long Custom Error Strings";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to organize unit tests as follows for readability (comments are not required, newlines are only between blocks):

// Block of arrange
int x = 10;
double y = 9.0

// Block of act
CallFoo(y);

// Block of assert
Assert(12.0, y);
Assert(29, x);

this.processHelper.SetErrorMessage(errorData);

this.mockRequestSender.Setup(rs => rs.InitializeCommunication()).Returns(123);

this.testOperationManager.SetupChannel(Enumerable.Empty<string>());

Assert.AreEqual(this.testOperationManager.GetStandardError().Length, errorLength);

Assert.AreEqual(this.testOperationManager.GetStandardError(), errorData.Substring(5));
}

[TestMethod]
public void ErrorMessageShouldBeTruncatedFromBeginingShouldDisplayTrailingData()
{
string errorData = "Error Strings";
this.processHelper.SetErrorMessage(errorData);

this.mockRequestSender.Setup(rs => rs.InitializeCommunication()).Returns(123);

this.testOperationManager.SetupChannel(Enumerable.Empty<string>());

Assert.AreEqual(this.testOperationManager.GetStandardError(), "StringsError Strings");
}

private class TestableProxyOperationManager : ProxyOperationManager
{
public TestableProxyOperationManager(
ITestRequestSender requestSender,
ITestHostManager testHostManager,
int clientConnectionTimeout,
int errorLength) : base(requestSender, testHostManager, clientConnectionTimeout, errorLength)
{
}

public override string GetStandardError()
{
return base.GetStandardError();
}
}

private class TestableTestHostManager : DefaultTestHostManager
{
public TestableTestHostManager(
Architecture architecture,
Framework framework,
IProcessHelper processHelper,
bool shared) : base(architecture, framework, processHelper, shared)
{
}

public override TestProcessStartInfo GetTestHostProcessStartInfo(IEnumerable<string> sources, IDictionary<string, string> environmentVariables, TestRunnerConnectionInfo connectionInfo)
{
return new TestProcessStartInfo();
}
}
private class TestableProcessHelper : IProcessHelper
{
private string ErrorMessage;

public void SetErrorMessage(string errorMessage)
{
this.ErrorMessage = errorMessage;
}
public string GetCurrentProcessFileName()
{
throw new NotImplementedException();
}

public int GetCurrentProcessId()
{
throw new NotImplementedException();
}

public string GetTestEngineDirectory()
{
throw new NotImplementedException();
}

public Process LaunchProcess(string processPath, string arguments, string workingDirectory, Action<Process, string> errorCallback)
{
var process = Process.GetCurrentProcess();

errorCallback(process, this.ErrorMessage);
errorCallback(process, this.ErrorMessage);
errorCallback(process, this.ErrorMessage);
errorCallback(process, this.ErrorMessage);

return process;
}
}
}
}