Skip to content
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

VsTestConsoleWrapper endsession should shut down vstest console process #2145

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ namespace Microsoft.TestPlatform.VsTestConsole.TranslationLayer
using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers;
using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers.Interfaces;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Threading;
using Resources = Microsoft.VisualStudio.TestPlatform.VsTestConsole.TranslationLayer.Resources.Resources;

/// <summary>
Expand All @@ -39,13 +39,19 @@ internal class VsTestConsoleProcessManager : IProcessManager
/// </summary>
private const string DIAG_ARGUMENT = "/diag:{0};tracelevel={1}";

/// <summary>
/// EndSession timeout
/// </summary>
private const int ENDSESSIONTIMEOUT = 1000;

private string vstestConsolePath;
private object syncObject = new object();
private bool vstestConsoleStarted = false;
private bool vstestConsoleExited = false;
private readonly bool isNetCoreRunner;
private string dotnetExePath;
private Process process;
private ManualResetEvent processExitedEvent = new ManualResetEvent(false);

internal IFileHelper FileHelper { get; set; }

Expand Down Expand Up @@ -143,15 +149,16 @@ public void StartProcess(ConsoleParameters consoleParameters)
public void ShutdownProcess()
{
// Ideally process should die by itself
if (IsProcessInitialized())
if(!processExitedEvent.WaitOne(ENDSESSIONTIMEOUT) && IsProcessInitialized())
vagisha-nidhi marked this conversation as resolved.
Show resolved Hide resolved
{
EqtTrace.Info($"VsTestConsoleProcessManager.ShutDownProcess : Terminating vstest.console process after waiting for {ENDSESSIONTIMEOUT} milliseconds.");
vstestConsoleExited = true;
this.process.OutputDataReceived -= Process_OutputDataReceived;
this.process.ErrorDataReceived -= Process_ErrorDataReceived;
SafelyTerminateProcess();
this.process.Dispose();
this.process = null;
}
}
}

private void SafelyTerminateProcess()
Expand All @@ -173,8 +180,8 @@ private void Process_Exited(object sender, EventArgs e)
{
lock (syncObject)
{
ShutdownProcess();

processExitedEvent.Set();
vstestConsoleExited = true;
this.ProcessExited?.Invoke(sender, e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,10 @@ public void EndSession()
{
this.requestSender.EndSession();
this.requestSender.Close();

// If vstest.console is still hanging around, it should be explicitly killed.
this.vstestConsoleProcessManager.ShutdownProcess();

this.sessionStarted = false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace Microsoft.TestPlatform.AcceptanceTests.TranslationLayerTests
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using VisualStudio.TestPlatform.ObjectModel.Logging;

Expand Down Expand Up @@ -52,6 +53,25 @@ public void RunAllTests(RunnerInfo runnerInfo)
Assert.AreEqual(2, this.runEventHandler.TestResults.Count(t => t.Outcome == TestOutcome.Skipped));
}

[TestMethod]
[NetFullTargetFrameworkDataSource]
[NetCoreTargetFrameworkDataSource]
public void EndSessionShouldEnsureVstestConsoleProcessDies(RunnerInfo runnerInfo)
{
var numOfProcesses = Process.GetProcessesByName("vstest.console").Length;

AcceptanceTestBase.SetTestEnvironment(this.testEnvironment, runnerInfo);
this.Setup();

this.vstestConsoleWrapper.RunTests(this.GetTestAssemblies(), this.GetDefaultRunSettings(), this.runEventHandler);
this.vstestConsoleWrapper?.EndSession();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can vstestConsoleWrapper be null ? :O


// Assert
Assert.AreEqual(numOfProcesses, Process.GetProcessesByName("vstest.console").Length);

this.vstestConsoleWrapper = null;
vagisha-nidhi marked this conversation as resolved.
Show resolved Hide resolved
}

[TestMethod]
[NetFullTargetFrameworkDataSource]
[NetCoreTargetFrameworkDataSource]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ public void EndSessionShouldSucceed()

this.mockRequestSender.Verify(rs => rs.EndSession(), Times.Once);
this.mockRequestSender.Verify(rs => rs.Close(), Times.Once);
this.mockProcessManager.Verify(x => x.ShutdownProcess(), Times.Once);
}
}
}