Read asynchronously from test host process. #529
Conversation
|
Hi @mayankbansal018, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
| } | ||
|
|
||
| this.RequestSender.OnClientProcessExit(testHostProcessStdError); | ||
| if (process.HasExited && process.ExitCode != 0) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| { | ||
| if (process.ExitCode != 0) | ||
| if(data != null) |
There was a problem hiding this comment.
Nit: move this to a method.
| /// Returns the current error data in stream | ||
| /// Written purely for UT as of now. | ||
| /// </summary> | ||
| public virtual string GetStandardError() |
There was a problem hiding this comment.
Make it protected. Use a Testable in unit test.
| @@ -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) | |||
There was a problem hiding this comment.
Explicit arguments are better. Please call with 1000 from the public ctor.
| if (data.Length > testHostProcessStdError.MaxCapacity) | ||
| { | ||
| testHostProcessStdError.Remove(0, testHostProcessStdError.Length); | ||
| data = data.Substring(data.Length - testHostProcessStdError.MaxCapacity); |
There was a problem hiding this comment.
We're keeping the first few characters, should it be last few characters?
There was a problem hiding this comment.
we are keeping last few only substring(index) which will give from starting index to end
| { | ||
| this.RequestSender = requestSender; | ||
| this.connectionTimeout = clientConnectionTimeout; | ||
| this.testHostManager = testHostManager; | ||
| this.processHelper = new ProcessHelper(); | ||
| this.errorLength = errorLength; |
There was a problem hiding this comment.
Instance variable is not used any where?
| data = data.Substring(data.Length - testHostProcessStdError.MaxCapacity); | ||
| } | ||
|
|
||
| //remove only what is required, from begining of error stream |
| //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); |
There was a problem hiding this comment.
Which of these is most efficient?
- StringBuilder.Clear followed by StringBuilder.Append
- StringBuilder.Remove(x, y) followed by StringBuilder.Append
- StringBuilder[i] = data[i]
| } | ||
|
|
||
| EqtTrace.Verbose("ProcessHelper: Starting process '{0}' with command line '{1}'", processPath, arguments); | ||
| process.Start(); | ||
|
|
||
| process.BeginErrorReadLine(); |
There was a problem hiding this comment.
Do we require this even if errorCallback is not set?
| [TestMethod] | ||
| public void ErrorMessageShouldBeTruncatedToMatchErrorLength() | ||
| { | ||
| string errorData = "Long Custom Error Strings"; |
There was a problem hiding this comment.
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);
|
Bug: #521 |
No description provided.