Add exit call back to custom test host process #1358
Conversation
|
I think the issue also shows when debugging a simple unittest and then
terminate the process from VS (debug - terminate). Can we confirm? Is it a
dependent on child process or the debugger attached?
Why the additional arg in exit callback?
…On 04-Jan-2018 5:59 PM, "Satya Madala" ***@***.***> wrote:
@smadala <https://github.com/smadala> requested your review on:
microsoft/vstest#1358 <#1358> Add
exit call back to custom test host process.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1358 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABwXgmc9NnMeEstD0JnLamDAvXNzpNIks5tHMQlgaJpZM4RSyq6>
.
|
|
In normal debug stop scenario, TcpClient.Poll() throwing IOException(remote host closed the connection...). Because there was no exitcall back we wait for 10 sec(Which causes #1183) to avoid race condition between communication break and exit callback.
Indefinite time on child process case, 10s on debugger attached.
Sometimes call back depends on exit code of process. I choose one method rather than adding overload method. |
| @@ -90,7 +90,7 @@ public interface IProcessHelper | |||
| /// <param name="callbackAction"> | |||
| /// Callback on process exit. | |||
| /// </param> | |||
| void SetExitCallback(int processId, Action callbackAction); | |||
| void SetExitCallback(int processId, Action<object> callbackAction); | |||
There was a problem hiding this comment.
object [](start = 51, length = 6)
explicitly as Process or have an eventArgs..
There was a problem hiding this comment.
Since Process API's are not available in UWP, using it explicitly here would mean we cannot built uwp platform abstraction.
| { | ||
| var process = Process.GetProcessById(processId); | ||
| process.EnableRaisingEvents = true; | ||
| process.Exited += (sender, args) => callbackAction.Invoke(process); |
There was a problem hiding this comment.
callbackAction.Invoke(process); [](start = 52, length = 31)
if there's a process crash before the hookup.. call the handler..
| { | ||
| var process = Process.GetProcessById(processId); | ||
| process.EnableRaisingEvents = true; | ||
| process.Exited += (sender, args) => callbackAction.Invoke(process); |
There was a problem hiding this comment.
In line 140, try sender as Process within Invoke (reference: https://github.com/smadala/vstest/blob/0848eafca059e3e49790a8a91ea2b9919970de83/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs#L55)
|
Looks good. Please cover LUT, UWP scenarios explicitly. These may listen to testhost process exit as well. So depending on which event callback is called, good to validate there are no race/premature cleanup conditions possible. Also good to cover cases where the debugged testhost produces bunch of stdout/stderr messages. |
Description
TcpClient.Poll()not throwing the exception on testhost process exit if testhost process have child process like geckodriver.exe(Which do some socket operations), but normal processes ( notepad.exe, etc..).TcpClient.Poll(), Have process exit call back for custom testhost.Related issue