support logging in CommandRunner when tests are executed in parallel #3732
Conversation
| bool waitForExit, | ||
| int timeOutInMilliseconds = 60000, | ||
| Action<StreamWriter> inputAction = null, | ||
| bool shareProcessObject = false, |
There was a problem hiding this comment.
I found no callers in the entire codebase who is passing this optional parameter. Hence removed it. If there are any concerns from others, I can add it back.
| Process p = null; | ||
|
|
||
| try | ||
| using (p = new Process()) |
There was a problem hiding this comment.
Can you shortly elaborate in more detail how your change helps?
I know you linked the issues and I appreciate that, but I was hoping you could save us some time digging through the linked docs if possible.
There was a problem hiding this comment.
Updated PR description with additional information. Pls let me know your feedback.
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
Thanks for the link.
Great find!
Consider adding a note on what not just the why in the PR title.
|
🔔 @NuGet/nuget-client. Any feedback on this PR is helpful. Waiting for second approval. |
Bug
Fixes: NuGet/Client.Engineering#613
Regression: No
Fix
Details: When we run
NuGet.CommandLine.Tests, every test in the test suite execute creates anuget.exeprocess as shown below. Everynuget.exeprocess executing test command publishesoutput and error streamto the parent process so that test can check the result to verify the test status.The way we read output and error stream from a Process while running tests in
CommandRunner.Runmethod doesn't work well when tests are executed in parallel because of following reasons.outputorerrorstring builder here the results were inconsistent.Work is in progress to support running test suites in parallel. see this issue for more info. This change is required to execute tests in parallel.
I found this doc and issue extremely helpful while working on this fix.
Testing/Validation
Tests Added: No
Reason for not adding tests: Fixing test utility method.
Validation: