Print start of testhost standard error message (#1708)

* Print start of testhost standard error message

* update unit tests

* Print datacollector stderr from the begining

* Create StringBuilderExtensions

* Fix nits

* USe full namespace

src/Microsoft.TestPlatform.CoreUtilities/Constants.cs

@@ -22,5 +22,11 @@ public class Constants
         /// Datacollector process name, without file extension(.exe/.dll)
         /// </summary>
         public const string DatacollectorProcessName = "datacollector";
+
+        /// <summary>
+        /// Number of character should be logged on child process exited with
+        /// error message on standard error.
+        /// </summary>
+        public const int StandardErrorMaxLength = 8192; // 8 KB
     }
 }

src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringBuilderExtensions.cs

@@ -0,0 +1,40 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions
+{
+    using System;
+    using System.Text;
+
+    public static class StringBuilderExtensions
+    {
+        /// <summary>
+        /// Append given data from to string builder with new line.
+        /// </summary>
+        /// <param name="result">string builder</param>
+        /// <param name="data">data to be appended.</param>
+        /// <returns></returns>
+        public static void AppendSafeWithNewLine(this StringBuilder result, string data)
+        {
+            if (!string.IsNullOrEmpty(data))
+            {
+                // Don't append more data if already reached max length.
+                if (result.Length >= result.MaxCapacity)
+                {
+                    return;
+                }
+
+                // Add newline for readability.
+                data += Environment.NewLine;
+
+                // Append sub string of data if appending all the data exceeds max capacity.
+                if (result.Length + data.Length >= result.MaxCapacity)
+                {
+                    data = data.Substring(0, result.MaxCapacity - result.Length);
+                }
+
+                result.Append(data);
+            }
+        }
+    }
+}

src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringExtensions.cs

@@ -4,7 +4,7 @@
 namespace Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions
 {
     using System;
-    using System.Net;
+    using System.Text;
 
     public static class StringExtensions
     {

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs

@@ -70,8 +70,6 @@ protected ProxyOperationManager(IRequestData requestData, ITestRequestSender req
 
         #region Properties
 
-        protected int ErrorLength { get; set; } = 1000;
-
         /// <summary>
         /// Gets or sets the server for communication.
         /// </summary>

src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/DataCollectionLauncher.cs

@@ -6,7 +6,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection
     using System;
     using System.Collections.Generic;
     using System.Text;
-
+    using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions;
     using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.Interfaces;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
@@ -36,17 +36,12 @@ public DataCollectionLauncher(IProcessHelper processHelper, IMessageLogger messa
         {
             this.processHelper = processHelper;
             this.messageLogger = messageLogger;
-            this.processStdError = new StringBuilder(this.ErrorLength, this.ErrorLength);
+            this.processStdError = new StringBuilder(0, CoreUtilities.Constants.StandardErrorMaxLength);
         }
 
         /// <inheritdoc />
         public int DataCollectorProcessId { get; protected set; }
 
-        /// <summary>
-        /// Gets or sets the error length for data collector error stream.
-        /// </summary>
-        protected int ErrorLength { get; set; } = 4096;
-
         /// <summary>
         /// Gets callback on process exit
         /// </summary>
@@ -76,35 +71,13 @@ public DataCollectionLauncher(IProcessHelper processHelper, IMessageLogger messa
         /// Gets callback to read from process error stream
         /// </summary>
         protected Action<object, string> ErrorReceivedCallback => (process, data) =>
-            {
-                if (!string.IsNullOrEmpty(data))
-                {
-                    // Log all standard error message because on too much data we ignore starting part.
-                    // This is helpful in abnormal failure of process.
-                    EqtTrace.Warning("DataCollectionLauncher.ErrorReceivedCallback: Data collector standard error line: {0}", data);
-
-                    // Add newline for readbility.
-                    data += Environment.NewLine;
-
-                    // if incoming data stream is huge empty entire testError stream, & limit data stream to MaxCapacity
-                    if (data.Length > this.processStdError.MaxCapacity)
-                    {
-                        this.processStdError.Clear();
-                        data = data.Substring(data.Length - this.processStdError.MaxCapacity);
-                    }
-                    else
-                    {
-                        // remove only what is required, from beginning of error stream
-                        int required = data.Length + this.processStdError.Length - this.processStdError.MaxCapacity;
-                        if (required > 0)
-                        {
-                            this.processStdError.Remove(0, required);
-                        }
-                    }
+        {
+            // Log all standard error message because on too much data we ignore starting part.
+            // This is helpful in abnormal failure of datacollector.
+            EqtTrace.Warning("DataCollectionLauncher.ErrorReceivedCallback datacollector standard error line: {0}", data);
 
-                    this.processStdError.Append(data);
-                }
-            };
+            this.processStdError.AppendSafeWithNewLine(data);
+        };
 
         /// <summary>
         /// The launch data collector.

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs

@@ -96,11 +96,6 @@ internal DefaultTestHostManager(IProcessHelper processHelper, IFileHelper fileHe
         /// </summary>
         public IDictionary<string, string> Properties => new Dictionary<string, string>();
 
-        /// <summary>
-        /// Gets or sets the error length for runtime error stream.
-        /// </summary>
-        protected int ErrorLength { get; set; } = 4096;
-
         /// <summary>
         /// Gets callback on process exit
         /// </summary>
@@ -367,7 +362,7 @@ private void OnHostExited(HostProviderEventArgs e)
 
         private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
         {
-            this.testHostProcessStdError = new StringBuilder(this.ErrorLength, this.ErrorLength);
+            this.testHostProcessStdError = new StringBuilder(0, CoreUtilities.Constants.StandardErrorMaxLength);
             EqtTrace.Verbose("Launching default test Host Process {0} with arguments {1}", testHostStartInfo.FileName, testHostStartInfo.Arguments);
 
             if (this.customTestHostLauncher == null)

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs

@@ -113,11 +113,6 @@ internal DotnetTestHostManager(
         /// </summary>
         internal bool MakeRunsettingsCompatible => this.hostPackageVersion.StartsWith("15.0.0-preview");
 
-        /// <summary>
-        /// Gets or sets the error length for runtime error stream.
-        /// </summary>
-        protected int ErrorLength { get; set; } = 4096;
-
         /// <summary>
         /// Gets callback on process exit
         /// </summary>
@@ -326,7 +321,7 @@ private void OnHostExited(HostProviderEventArgs e)
 
         private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
         {
-            this.testHostProcessStdError = new StringBuilder(this.ErrorLength, this.ErrorLength);
+            this.testHostProcessStdError = new StringBuilder(0, CoreUtilities.Constants.StandardErrorMaxLength);
             if (this.customTestHostLauncher == null)
             {
                 EqtTrace.Verbose("DotnetTestHostManager: Starting process '{0}' with command line '{1}'", testHostStartInfo.FileName, testHostStartInfo.Arguments);

src/Microsoft.TestPlatform.TestHostProvider/Hosting/TestHostManagerCallbacks.cs

@@ -9,39 +9,17 @@ namespace Microsoft.TestPlatform.TestHostProvider.Hosting
     using Microsoft.VisualStudio.TestPlatform.ObjectModel;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
     using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;
+    using VisualStudio.TestPlatform.CoreUtilities.Extensions;
 
     internal class TestHostManagerCallbacks
     {
         public static void ErrorReceivedCallback(StringBuilder testHostProcessStdError, string data)
         {
-            if (!string.IsNullOrEmpty(data))
-            {
-                // Log all standard error message because on too much data we ignore starting part.
-                // This is helpful in abnormal failure of testhost.
-                EqtTrace.Warning("TestHostManagerCallbacks.ErrorReceivedCallback Test host standard error line: {0}", data);
-
-                // Add newline for readbility.
-                data += Environment.NewLine;
-
-                // if incoming data stream is huge empty entire testError stream, & limit data stream to MaxCapacity
-                if (data.Length > testHostProcessStdError.MaxCapacity)
-                {
-                    testHostProcessStdError.Clear();
-                    data = data.Substring(data.Length - testHostProcessStdError.MaxCapacity);
-                }
+            // Log all standard error message because on too much data we ignore starting part.
+            // This is helpful in abnormal failure of testhost.
+            EqtTrace.Warning("TestHostManagerCallbacks.ErrorReceivedCallback Test host standard error line: {0}", data);
 
-                // remove only what is required, from beginning of error stream
-                else
-                {
-                    int required = data.Length + testHostProcessStdError.Length - testHostProcessStdError.MaxCapacity;
-                    if (required > 0)
-                    {
-                        testHostProcessStdError.Remove(0, required);
-                    }
-                }
-
-                testHostProcessStdError.Append(data);
-            }
+            testHostProcessStdError.AppendSafeWithNewLine(data);
         }
 
         public static void ExitCallBack(

test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs

@@ -1,7 +1,7 @@
 // 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.TestHostProvider.UnitTests.Hosting
+namespace TestPlatform.TestHostProvider.Hosting.UnitTests
 {
     using System;
     using System.Collections.Generic;
@@ -40,7 +40,6 @@ public class DefaultTestHostManagerTests
 
         private DefaultTestHostManager testHostManager;
         private TestableTestHostManager testableTestHostManager;
-        private int maxStdErrStringLength = 22;
         private string errorMessage;
         private int exitCode;
         private int testHostId;
@@ -344,7 +343,6 @@ public void LaunchTestHostAsyncShouldNotStartHostProcessIfCancellationTokenIsSet
                 Framework.DefaultFramework,
                 this.mockProcessHelper.Object,
                 true,
-                this.maxStdErrStringLength,
                 this.mockMessageLogger.Object);
 
             CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
@@ -423,19 +421,6 @@ public async Task ErrorMessageShouldBeReadAsynchronously()
             Assert.AreEqual(errorData, this.errorMessage);
         }
 
-        [TestMethod]
-        public async Task ErrorMessageShouldBeTruncatedToMatchErrorLength()
-        {
-            string errorData = "Long Custom Error Strings";
-            this.ErrorCallBackTestHelper(errorData, -1);
-
-            await this.testableTestHostManager.LaunchTestHostAsync(this.GetDefaultStartInfo(), CancellationToken.None);
-
-            // Ignore new line chars
-            Assert.AreEqual(this.maxStdErrStringLength - Environment.NewLine.Length, this.errorMessage.Length);
-            Assert.AreEqual(errorData.Substring(5), this.errorMessage);
-        }
-
         [TestMethod]
         public async Task NoErrorMessageIfExitCodeZero()
         {
@@ -527,7 +512,6 @@ private void ErrorCallBackTestHelper(string errorMessage, int exitCode)
                 Framework.DefaultFramework,
                 this.mockProcessHelper.Object,
                 true,
-                this.maxStdErrStringLength,
                 this.mockMessageLogger.Object);
 
             this.testableTestHostManager.HostExited += this.TestHostManagerHostExited;
@@ -560,7 +544,6 @@ private void ExitCallBackTestHelper(int exitCode)
                 Framework.DefaultFramework,
                 this.mockProcessHelper.Object,
                 true,
-                this.maxStdErrStringLength,
                 this.mockMessageLogger.Object);
 
             this.testableTestHostManager.HostExited += this.TestableTestHostManagerHostExited;
@@ -596,11 +579,9 @@ public TestableTestHostManager(
                 Framework framework,
                 IProcessHelper processHelper,
                 bool shared,
-                int errorLength,
                 IMessageLogger logger)
                 : base(processHelper, new FileHelper(), new PlatformEnvironment(), new DotnetHostHelper())
             {
-                this.ErrorLength = errorLength;
                 this.Initialize(logger, $"<?xml version=\"1.0\" encoding=\"utf-8\"?><RunSettings> <RunConfiguration> <TargetPlatform>{architecture}</TargetPlatform> <TargetFrameworkVersion>{framework}</TargetFrameworkVersion> <DisableAppDomain>{!shared}</DisableAppDomain> </RunConfiguration> </RunSettings>");
             }
         }

test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs

@@ -49,7 +49,6 @@ public class DotnetTestHostManagerTests
         private readonly TestableDotnetTestHostManager dotnetHostManager;
 
         private string errorMessage;
-        private int maxStdErrStringLength = 22;
 
         private int exitCode;
 
@@ -69,8 +68,7 @@ public DotnetTestHostManagerTests()
             this.dotnetHostManager = new TestableDotnetTestHostManager(
                                          this.mockProcessHelper.Object,
                                          this.mockFileHelper.Object,
-                                         new DotnetHostHelper(this.mockFileHelper.Object, mockEnvironment.Object),
-                                         this.maxStdErrStringLength);
+                                         new DotnetHostHelper(this.mockFileHelper.Object, mockEnvironment.Object));
             this.dotnetHostManager.Initialize(this.mockMessageLogger.Object, string.Empty);
 
             this.dotnetHostManager.HostExited += this.DotnetHostManagerHostExited;
@@ -620,19 +618,6 @@ public async Task DotNetCoreErrorMessageShouldBeReadAsynchronouslyAsync()
             Assert.AreEqual(errorData, this.errorMessage);
         }
 
-        [TestMethod]
-        public async Task DotNetCoreErrorMessageShouldBeTruncatedToMatchErrorLength()
-        {
-            var errorData = "Long Custom Error Strings";
-            this.ErrorCallBackTestHelper(errorData, -1);
-
-            await this.dotnetHostManager.LaunchTestHostAsync(this.defaultTestProcessStartInfo, CancellationToken.None);
-
-            // Ignore new line chars
-            Assert.AreEqual(this.maxStdErrStringLength - Environment.NewLine.Length, this.errorMessage.Length);
-            Assert.AreEqual(errorData.Substring(5), this.errorMessage);
-        }
-
         [TestMethod]
         public async Task DotNetCoreNoErrorMessageIfExitCodeZero()
         {
@@ -778,10 +763,12 @@ private TestProcessStartInfo GetDefaultStartInfo()
 
         internal class TestableDotnetTestHostManager : DotnetTestHostManager
         {
-            public TestableDotnetTestHostManager(IProcessHelper processHelper, IFileHelper fileHelper, IDotnetHostHelper dotnetTestHostHelper, int errorLength)
+            public TestableDotnetTestHostManager(
+                IProcessHelper processHelper,
+                IFileHelper fileHelper,
+                IDotnetHostHelper dotnetTestHostHelper)
                 : base(processHelper, fileHelper, dotnetTestHostHelper)
             {
-                this.ErrorLength = errorLength;
             }
         }
     }