New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perf fix for ProcessHelper.TerminateProcess #928

Merged
merged 8 commits into from Jul 14, 2017

Conversation

Projects
None yet
4 participants
@harshjain2
Member

harshjain2 commented Jul 12, 2017

Modified the TerminateProcess API to pass Process as argument instead of Process.Id.
This gives a perf benefit of around 100 milliseconds as we used to get Process object using ProcessId which we already have.

Show outdated Hide outdated ...crosoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs
{
var process = Process.GetProcessById(processId);
process.Kill();
((Process)process).Kill();

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 12, 2017

Contributor

please add a null check after Typecasting, if something else is passed other than process object

@mayankbansal018

mayankbansal018 Jul 12, 2017

Contributor

please add a null check after Typecasting, if something else is passed other than process object

This comment has been minimized.

@harshjain2

harshjain2 Jul 13, 2017

Member

done.

@harshjain2
Show outdated Hide outdated ...atform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs
@@ -303,19 +303,19 @@ public void LaunchTestHostShouldUseCustomHostIfSet()
await this.testableTestHostManager.LaunchTestHostAsync(this.GetDefaultStartInfo(), CancellationToken.None);
await this.testableTestHostManager.CleanTestHostAsync(CancellationToken.None);
this.mockProcessHelper.Verify(ph => ph.TerminateProcess(Process.GetCurrentProcess().Id), Times.Once);
this.mockProcessHelper.Verify(ph => ph.TerminateProcess(It.IsAny<Process>()), Times.Once);

This comment has been minimized.

@codito

codito Jul 12, 2017

Contributor

How do we validate that it is the same process object that's provided?

@codito

codito Jul 12, 2017

Contributor

How do we validate that it is the same process object that's provided?

@harshjain2

This comment has been minimized.

Show comment
Hide comment
@harshjain2

harshjain2 Jul 14, 2017

Member

@dotnet-bot test Windows_NT / Release Build

Member

harshjain2 commented Jul 14, 2017

@dotnet-bot test Windows_NT / Release Build

@harshjain2 harshjain2 merged commit e2ac8ce into Microsoft:master Jul 14, 2017

4 checks passed

Ubuntu16.04 / Debug Build Build finished.
Details
Ubuntu16.04 / Release Build Build finished.
Details
Windows_NT / Debug Build Build finished.
Details
Windows_NT / Release Build Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment