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

Allow TranslationLayer to specify Diag parameters #296

Merged
merged 3 commits into from
Dec 23, 2016
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
@@ -0,0 +1,56 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.TestPlatform.VsTestConsole.TranslationLayer
{
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Text;

/// <summary>
/// Class which defines additional specifiable parameters for vstest.console.exe
/// </summary>
public class ConsoleParameters
{
internal static readonly ConsoleParameters Default = new ConsoleParameters();

private string logFilePath = null;

public string LogFilePath
{
get
{
return logFilePath;
}

set
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a blank line before set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? - Is this a style cop/resharper thing?

{
ValidateArg.NotNullOrEmpty(value, "LogFilePath");
if(!Directory.Exists(Path.GetDirectoryName(value)))
{
throw new ArgumentException("LogFilePath must point to a valid directory for logging!");
}

this.logFilePath = value;
}
}

/// <summary>
/// Port Number for communication
/// vstest.console will need this port number to communicate with this component - translation layer
/// Currently Internal as we are not intentionally exposing this to consumers of translation layer
/// </summary>
internal int PortNumber { get; set; }

/// <summary>
/// Parent Process ID of the process whose lifetime should dictate the life time of vstest.console.exe
/// vstest.console will need this process ID to know when the process exits.
/// If parent process dies/crashes without invoking EndSession, vstest.console should exit immediately
/// Currently Internal as we are not intentionally exposing this to consumers of translation layer
/// </summary>
internal int ParentProcessId { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal interface IProcessManager
/// <summary>
/// Starts the Process
/// </summary>
void StartProcess(string[] args);
void StartProcess(ConsoleParameters consoleParameters);

/// <summary>
/// Is Process Initialized
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace Microsoft.TestPlatform.VsTestConsole.TranslationLayer
using Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;

Expand All @@ -26,6 +27,23 @@ internal class VsTestConsoleProcessManager : IProcessManager

public event EventHandler ProcessExited;

/// <summary>
/// Port number for communicating with Vstest CLI
/// </summary>
private const string PORT_ARGUMENT = "/port:{0}";

/// <summary>
/// Process Id of the Current Process which is launching Vstest CLI
/// Helps Vstest CLI in auto-exit if current process dies without notifying it
/// </summary>
private const string PARENT_PROCESSID_ARGUMENT = "/parentprocessid:{0}";

/// <summary>
/// Diagnostics argument for Vstest CLI
/// Enables Diagnostic logging for Vstest CLI and TestHost - Optional
/// </summary>
private const string DIAG_ARGUMENT = "/diag:{0}";

#region Constructor

public VsTestConsoleProcessManager(string vstestConsolePath)
Expand All @@ -47,14 +65,12 @@ public bool IsProcessInitialized()
/// <summary>
/// Call xUnit.console.exe with the parameters previously specified
/// </summary>
public void StartProcess(string[] args)
public void StartProcess(ConsoleParameters consoleParameters)
{
this.process = new Process();
process.StartInfo.FileName = vstestConsolePath;
if (args != null)
{
process.StartInfo.Arguments = args.Length < 2 ? args[0] : string.Join(" ", args);
}
process.StartInfo.Arguments = string.Join(" ", BuildArguments(consoleParameters));

//process.StartInfo.WorkingDirectory = WorkingDirectory;
process.StartInfo.UseShellExecute = false;
process.StartInfo.CreateNoWindow = true;
Expand Down Expand Up @@ -95,5 +111,22 @@ private void Process_Exited(object sender, EventArgs e)
this.ProcessExited?.Invoke(sender, e);
}
}

private string[] BuildArguments(ConsoleParameters parameters)
{
var args = new List<string>();

// Start Vstest.console.exe with args: --parentProcessId|/parentprocessid:<ppid> --port|/port:<port>
args.Add(string.Format(CultureInfo.InvariantCulture, PARENT_PROCESSID_ARGUMENT, parameters.ParentProcessId));
args.Add(string.Format(CultureInfo.InvariantCulture, PORT_ARGUMENT, parameters.PortNumber));

if(!string.IsNullOrEmpty(parameters.LogFilePath))
{
// Extra args: --diag|/diag:<PathToLogFile>
args.Add(string.Format(CultureInfo.InvariantCulture, DIAG_ARGUMENT, parameters.LogFilePath));
}

return args.ToArray();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,14 @@ public class VsTestConsoleWrapper : IVsTestConsoleWrapper
private bool sessionStarted;

/// <summary>
/// Port number for communicating with Vstest CLI
/// Path to additional extensions to reinitialize vstest.console
/// </summary>
private const string PORT_ARGUMENT = "/port:{0}";
private IEnumerable<string> pathToAdditionalExtensions;

/// <summary>
/// Process Id of the Current Process which is launching Vstest CLI
/// Helps Vstest CLI in auto-exit if current process dies without notifying it
/// Additional parameters for vstest.console.exe
/// </summary>
private const string PARENT_PROCESSID_ARGUMENT = "/parentprocessid:{0}";

/// <summary>
/// Path to additional extensions to reinitialize vstest.console
/// </summary>
private IEnumerable<string> pathToAdditionalExtensions;
private ConsoleParameters consoleParameters;

#endregion

Expand All @@ -56,7 +50,18 @@ public class VsTestConsoleWrapper : IVsTestConsoleWrapper
/// Path to the test runner <c>vstest.console.exe</c>.
/// </param>
public VsTestConsoleWrapper(string vstestConsolePath) :
this(new VsTestConsoleRequestSender(), new VsTestConsoleProcessManager(vstestConsolePath))
this(new VsTestConsoleRequestSender(), new VsTestConsoleProcessManager(vstestConsolePath), ConsoleParameters.Default)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="VsTestConsoleWrapper"/> class.
/// </summary>
/// <param name="vstestConsolePath">
/// Path to the test runner <c>vstest.console.exe</c>.
/// </param>
public VsTestConsoleWrapper(string vstestConsolePath, ConsoleParameters consoleParameters) :
this(new VsTestConsoleRequestSender(), new VsTestConsoleProcessManager(vstestConsolePath), consoleParameters)
{
}

Expand All @@ -65,10 +70,12 @@ public VsTestConsoleWrapper(string vstestConsolePath) :
/// </summary>
/// <param name="requestSender">Sender for test messages.</param>
/// <param name="processManager">Process manager.</param>
internal VsTestConsoleWrapper(ITranslationLayerRequestSender requestSender, IProcessManager processManager)
internal VsTestConsoleWrapper(ITranslationLayerRequestSender requestSender, IProcessManager processManager, ConsoleParameters consoleParameters)
{
this.requestSender = requestSender;
this.vstestConsoleProcessManager = processManager;
this.consoleParameters = consoleParameters;

this.vstestConsoleProcessManager.ProcessExited += (sender, args) => this.requestSender.OnProcessExited();
this.sessionStarted = false;
}
Expand All @@ -85,10 +92,12 @@ public void StartSession()

if (port > 0)
{
// Start Vstest.console.exe with args: --parentProcessId|/parentprocessid:<ppid> --port|/port:<port>
string parentProcessIdArgs = string.Format(CultureInfo.InvariantCulture, PARENT_PROCESSID_ARGUMENT, Process.GetCurrentProcess().Id);
string portArgs = string.Format(CultureInfo.InvariantCulture, PORT_ARGUMENT, port);
this.vstestConsoleProcessManager.StartProcess(new string[2] { parentProcessIdArgs, portArgs });
// Fill the parameters
this.consoleParameters.ParentProcessId = Process.GetCurrentProcess().Id;
this.consoleParameters.PortNumber = port;

// Start vstest.console.exe process
this.vstestConsoleProcessManager.StartProcess(this.consoleParameters);
}
else
{
Expand Down
8 changes: 7 additions & 1 deletion src/testhost.x86/DefaultEngineInvoker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.VisualStudio.TestPlatform.TestHost
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol;

using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
using System;
using System.Collections.Generic;
using System.Diagnostics;
Expand Down Expand Up @@ -64,6 +64,12 @@ public void Invoke(IDictionary<string, string> argsDictionary)
EqtTrace.Info("DefaultEngineInvoker: Initialize communication on port number: '{0}'", portNumber);
requestHandler.InitializeCommunication(portNumber);

// Can only do this after InitializeCommunication because TestHost cannot "Send Log" unless communications are initialized
if (!string.IsNullOrEmpty(EqtTrace.LogFile))
{
requestHandler.SendLog(TestMessageLevel.Informational, string.Format("Logging TestHost Diagnostics in file: {0}", EqtTrace.LogFile));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will show up in IDE, console runner. Does it require localization?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. It does not. It only shows up in IDE if diagnostics are enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

And Manish's Verbose log is not localized - so we don't have to. With TestWindow, I am planning to add this if "VS_UTE_DIAGNOSTICS = 1" which definitely does not require localization.

}

// Start processing async in a different task
EqtTrace.Info("DefaultEngineInvoker: Start Request Processing.");
var processingTask = StartProcessingAsync(requestHandler, new TestHostManagerFactory());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,30 @@ namespace Microsoft.TestPlatform.VsTestConsole.TranslationLayer.UnitTests
[TestClass]
public class VsTestConsoleWrapperTests
{
private readonly IVsTestConsoleWrapper consoleWrapper;
private IVsTestConsoleWrapper consoleWrapper;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need to reinitialize these things in TestInitialize everytime to reset stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why in test initialize? We could do those resets in constructor.


private readonly Mock<IProcessManager> mockProcessManager;
private Mock<IProcessManager> mockProcessManager;

private readonly Mock<ITranslationLayerRequestSender> mockRequestSender;
private Mock<ITranslationLayerRequestSender> mockRequestSender;

private readonly List<string> testSources = new List<string> { "Hello", "World" };
private List<string> testSources = new List<string> { "Hello", "World" };

private readonly List<TestCase> testCases = new List<TestCase>
private List<TestCase> testCases = new List<TestCase>
{
new TestCase("a.b.c", new Uri("d://uri"), "a.dll"),
new TestCase("d.e.f", new Uri("g://uri"), "d.dll")
};

public VsTestConsoleWrapperTests()
private ConsoleParameters consoleParameters;

[TestInitialize]
public void TestInitialize()
{
this.consoleParameters = new ConsoleParameters();

this.mockRequestSender = new Mock<ITranslationLayerRequestSender>();
this.mockProcessManager = new Mock<IProcessManager>();
this.consoleWrapper = new VsTestConsoleWrapper(this.mockRequestSender.Object, this.mockProcessManager.Object);
this.consoleWrapper = new VsTestConsoleWrapper(this.mockRequestSender.Object, this.mockProcessManager.Object, this.consoleParameters);

this.mockRequestSender.Setup(rs => rs.WaitForRequestHandlerConnection(It.IsAny<int>())).Returns(true);
this.mockRequestSender.Setup(rs => rs.InitializeCommunication()).Returns(100);
Expand All @@ -50,7 +55,10 @@ public void StartSessionShouldStartVsTestConsoleWithCorrectArguments()

this.consoleWrapper.StartSession();

this.mockProcessManager.Verify(pm => pm.StartProcess(new[] { $"/parentprocessid:{expectedParentProcessId}", $"/port:{inputPort}" }), Times.Once);
Assert.AreEqual(expectedParentProcessId, this.consoleParameters.ParentProcessId, "Parent process Id must be set");
Assert.AreEqual(inputPort, this.consoleParameters.PortNumber, "Port number must be set");

this.mockProcessManager.Verify(pm => pm.StartProcess(this.consoleParameters), Times.Once);
}

[TestMethod]
Expand All @@ -69,7 +77,7 @@ public void StartSessionShouldCallWhenProcessNotInitialized()
// To call private method EnsureInitialize call InitializeExtensions
this.consoleWrapper.InitializeExtensions(new[] { "path/to/adapter" });

this.mockProcessManager.Verify(pm => pm.StartProcess(It.IsAny<string[]>()));
this.mockProcessManager.Verify(pm => pm.StartProcess(It.IsAny<ConsoleParameters>()));
}

[TestMethod]
Expand Down