Print start of testhost standard error message #1708

smadala · 2018-07-31T11:00:03Z

Description

Prints start of stderr stacktrace
Increase max length of stderr message.

Related issue

abhishkk · 2018-08-01T06:06:40Z

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

@@ -17,5 +19,28 @@ public static string AddDoubleQuote(this string value)
        {
            return "\"" + value + "\"";
        }
+
+        public static void AppendToStringBuilderBasedOnMaxLength(this string data, StringBuilder result)


Doesn't look like string extension. May be a string builder extension.
public static void AppendSafe(this StringBuilder stringBuilder, string data)

Create StringBuilderExtensions class with AppendSafeWithNewLine().

abhishkk · 2018-08-01T06:06:59Z

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

@@ -17,5 +19,28 @@ public static string AddDoubleQuote(this string value)
        {
            return "\"" + value + "\"";
        }
+
+        public static void AppendToStringBuilderBasedOnMaxLength(this string data, StringBuilder result)


Can we write comment as well for this method?

abhishkk · 2018-08-01T06:12:30Z

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

@@ -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);


When setting capacity to zero, will stringBuilder not be perf inefficient? As it needs to keep on copying and pasting the content to new instance whenever grow is required.

We don't expect any stderr most of time. StringBuilder increase size exponentially.

abhishkk · 2018-08-01T06:15:28Z

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

+        }
+
+        [TestMethod]
+        public void ErrorReceivedCallbackShouldAppendEntireStringEvenItReachesToMaxLength()


EvenItReachesToMaxLength? In code, max length is not reached.

In Windows NewLine is two characters.

abhishkk · 2018-08-01T06:17:44Z

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

+                }
+
+                // Add newline for readbility.
+                data += Environment.NewLine;


If we are writing stringBuilder extension, then it should not be job of stringBuilder to add a new line. It should be the user's responsibility to add a new line.

Create StringBuilderExtensions class with AppendSafeWithNewLine().

abhishkk

approved with suggestions.

singhsarab · 2018-08-01T06:40:59Z

src/Microsoft.TestPlatform.CoreUtilities/Constants.cs

+
+        /// <summary>
+        /// Number of character should be logged on child process exited with
+        /// error message on stadard error.


nit:standard

singhsarab · 2018-08-01T07:00:34Z

src/Microsoft.TestPlatform.CoreUtilities/Constants.cs

+        /// Number of character should be logged on child process exited with
+        /// error message on stadard error.
+        /// </summary>
+        public const int StandardErrorMaxLength = 20480; // 20 KB


I don't see a reason for increasing it by 5 times. 4096 was a reasonable buffer, we can probably make ti 8192 (2x), if user wants more info, they can always go ahead check the logs.

Made it 8192

…-debug-assert

…into fix-debug-assert

singhsarab · 2018-08-01T09:06:51Z

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

+        /// <param name="result">string builder</param>
+        /// /// <param name="data">data to be appended.</param>
+        /// <returns></returns>
+


Nit:Extra line
Nit:readability* inside the method below

singhsarab · 2018-08-01T09:08:20Z

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

+    public static class StringBuilderExtensions
+    {
+        /// <summary>
+        /// Add double quote around string. Useful in case of path which has white space in between.


Summary needs to be corrected.

singhsarab · 2018-08-01T09:10:21Z

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

@@ -17,5 +17,28 @@ public static string AddDoubleQuote(this string value)
        {
            return "\"" + value + "\"";
        }
+
+        public static void AppendToStringBuilderBasedOnMaxLength(this string data, StringBuilder result)


Still see this code, please remove this.

mayankbansal018 · 2018-08-01T09:54:40Z

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

+                // 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);


What's the idea here, are we saying the original Sting content will remain, & only add small part of new data?

mayankbansal018 · 2018-08-01T09:55:47Z

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

@@ -17,5 +17,28 @@ public static string AddDoubleQuote(this string value)
        {
            return "\"" + value + "\"";
        }
+
+        public static void AppendToStringBuilderBasedOnMaxLength(this string data, StringBuilder result)


Why is this method needed?

This is removed in latest commits.

mayankbansal018 · 2018-08-01T09:56:12Z

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 CoreUtilities.Extensions;


Nit: Please use full namespace

mayankbansal018 · 2018-08-01T09:58:17Z

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

@@ -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);


Should we initialize it with something other than that of 0 length?

Print start of testhost standard error message

8b865e4

smadala mentioned this pull request Jul 31, 2018

Debug.Assert(false) causes poor experience in tests #1688

Closed

smadala added 2 commits August 1, 2018 10:07

update unit tests

268d353

Print datacollector stderr from the begining

a27b17d

smadala requested review from mayankbansal018, singhsarab and abhishkk August 1, 2018 05:26

Merge branch 'master' into fix-debug-assert

784079a

abhishkk reviewed Aug 1, 2018

View reviewed changes

abhishkk approved these changes Aug 1, 2018

View reviewed changes

singhsarab reviewed Aug 1, 2018

View reviewed changes

smadala added 3 commits August 1, 2018 14:16

Create StringBuilderExtensions

55f8799

Merge branch 'master' of https://github.com/Microsoft/vstest into fix…

0f2f27b

…-debug-assert

Merge branch 'fix-debug-assert' of https://github.com/smadala/vstest …

c405246

…into fix-debug-assert

singhsarab reviewed Aug 1, 2018

View reviewed changes

Fix nits

5a50975

mayankbansal018 reviewed Aug 1, 2018

View reviewed changes

smadala added 2 commits August 1, 2018 15:56

USe full namespace

2fdda1f

Merge branch 'master' into fix-debug-assert

16648ea

smadala merged commit a3a2775 into microsoft:master Aug 1, 2018

smadala deleted the fix-debug-assert branch August 1, 2018 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print start of testhost standard error message #1708

Print start of testhost standard error message #1708

smadala commented Jul 31, 2018

abhishkk Aug 1, 2018

smadala Aug 1, 2018

abhishkk Aug 1, 2018

smadala Aug 1, 2018

abhishkk Aug 1, 2018

smadala Aug 1, 2018

abhishkk Aug 1, 2018

smadala Aug 1, 2018

abhishkk Aug 1, 2018

smadala Aug 1, 2018

abhishkk left a comment

singhsarab Aug 1, 2018

smadala Aug 1, 2018

singhsarab Aug 1, 2018

smadala Aug 1, 2018

singhsarab Aug 1, 2018

singhsarab Aug 1, 2018

singhsarab Aug 1, 2018

mayankbansal018 Aug 1, 2018

smadala Aug 1, 2018

mayankbansal018 Aug 1, 2018

smadala Aug 1, 2018

mayankbansal018 Aug 1, 2018

mayankbansal018 Aug 1, 2018

Print start of testhost standard error message #1708

Print start of testhost standard error message #1708

Conversation

smadala commented Jul 31, 2018

Description

Related issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishkk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment