diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Constants.cs b/src/Microsoft.TestPlatform.CoreUtilities/Constants.cs index d9856906a0..1af6ef5371 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Constants.cs +++ b/src/Microsoft.TestPlatform.CoreUtilities/Constants.cs @@ -22,5 +22,11 @@ public class Constants /// Datacollector process name, without file extension(.exe/.dll) /// public const string DatacollectorProcessName = "datacollector"; + + /// + /// Number of character should be logged on child process exited with + /// error message on standard error. + /// + public const int StandardErrorMaxLength = 8192; // 8 KB } } diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringBuilderExtensions.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringBuilderExtensions.cs new file mode 100644 index 0000000000..52e27472bf --- /dev/null +++ b/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 + { + /// + /// Append given data from to string builder with new line. + /// + /// string builder + /// data to be appended. + /// + 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); + } + } + } +} diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringExtensions.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringExtensions.cs index f7a618c5fe..ffef131a7c 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringExtensions.cs +++ b/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 { diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs index 681db2a770..fd6c67a53c 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs +++ b/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; - /// /// Gets or sets the server for communication. /// diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/DataCollectionLauncher.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/DataCollectionLauncher.cs index 1fb1745a19..5496f46a51 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/DataCollectionLauncher.cs +++ b/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); } /// public int DataCollectorProcessId { get; protected set; } - /// - /// Gets or sets the error length for data collector error stream. - /// - protected int ErrorLength { get; set; } = 4096; - /// /// Gets callback on process exit /// @@ -76,35 +71,13 @@ public DataCollectionLauncher(IProcessHelper processHelper, IMessageLogger messa /// Gets callback to read from process error stream /// protected Action 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); + }; /// /// The launch data collector. diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs index 83550821cd..d73a6739e1 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs +++ b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs @@ -96,11 +96,6 @@ internal DefaultTestHostManager(IProcessHelper processHelper, IFileHelper fileHe /// public IDictionary Properties => new Dictionary(); - /// - /// Gets or sets the error length for runtime error stream. - /// - protected int ErrorLength { get; set; } = 4096; - /// /// Gets callback on process exit /// @@ -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) diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs index 2f7150244d..a962a82f91 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs +++ b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs @@ -113,11 +113,6 @@ internal DotnetTestHostManager( /// internal bool MakeRunsettingsCompatible => this.hostPackageVersion.StartsWith("15.0.0-preview"); - /// - /// Gets or sets the error length for runtime error stream. - /// - protected int ErrorLength { get; set; } = 4096; - /// /// Gets callback on process exit /// @@ -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); diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/TestHostManagerCallbacks.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/TestHostManagerCallbacks.cs index 5a4082efe6..33c09bd210 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/TestHostManagerCallbacks.cs +++ b/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( diff --git a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs index f3405e1bc0..a1456f7010 100644 --- a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs +++ b/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, $" {architecture} {framework} {!shared} "); } } diff --git a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs index 285dcda804..26d3055238 100644 --- a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs +++ b/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; } } } diff --git a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/TestHostManagerCallbacksTests.cs b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/TestHostManagerCallbacksTests.cs new file mode 100644 index 0000000000..98e8f2d9cf --- /dev/null +++ b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/TestHostManagerCallbacksTests.cs @@ -0,0 +1,97 @@ +// 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.Hosting.UnitTests +{ + using System; + using System.Text; + using Microsoft.TestPlatform.TestHostProvider.Hosting; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class TestHostManagerCallbacksTests + { + private StringBuilder testHostProcessStdError; + + public TestHostManagerCallbacksTests() + { + this.testHostProcessStdError = new StringBuilder(0, Microsoft.VisualStudio.TestPlatform.CoreUtilities.Constants.StandardErrorMaxLength); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendNoDataOnNullDataReceived() + { + this.testHostProcessStdError.Append("NoDataShouldAppend"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, null); + + Assert.AreEqual("NoDataShouldAppend", this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendNoDataOnEmptyDataReceived() + { + this.testHostProcessStdError.Append("NoDataShouldAppend"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, string.Empty); + + Assert.AreEqual("NoDataShouldAppend", this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendWhiteSpaceString() + { + this.testHostProcessStdError.Append("OldData"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, " "); + + Assert.AreEqual("OldData " + Environment.NewLine, this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendGivenData() + { + this.testHostProcessStdError.Append("NoDataShouldAppend"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, "new data"); + + Assert.AreEqual("NoDataShouldAppendnew data" + Environment.NewLine, this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldNotAppendNewDataIfErrorMessageAlreadyReachedMaxLength() + { + this.testHostProcessStdError = new StringBuilder(0, 5); + this.testHostProcessStdError.Append("12345"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, "678"); + + Assert.AreEqual("12345", this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendSubStringOfDataIfErrorMessageReachedMaxLength() + { + this.testHostProcessStdError = new StringBuilder(0, 5); + this.testHostProcessStdError.Append("1234"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, "5678"); + + Assert.AreEqual("12345", this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendEntireStringEvenItReachesToMaxLength() + { + this.testHostProcessStdError = new StringBuilder(0, 5); + this.testHostProcessStdError.Append("12"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, "3"); + + Assert.AreEqual("123" + Environment.NewLine, this.testHostProcessStdError.ToString()); + } + + [TestMethod] + public void ErrorReceivedCallbackShouldAppendNewLineApproprioritlyWhenReachingMaxLength() + { + this.testHostProcessStdError = new StringBuilder(0, 5); + this.testHostProcessStdError.Append("123"); + TestHostManagerCallbacks.ErrorReceivedCallback(this.testHostProcessStdError, "4"); + + Assert.AreEqual("1234" + Environment.NewLine.Substring(0, 1), this.testHostProcessStdError.ToString()); + } + } +}