-
Notifications
You must be signed in to change notification settings - Fork 319
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// 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 | ||
{ | ||
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will show up in IDE, console runner. Does it require localization? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,25 +17,30 @@ namespace Microsoft.TestPlatform.VsTestConsole.TranslationLayer.UnitTests | |
[TestClass] | ||
public class VsTestConsoleWrapperTests | ||
{ | ||
private readonly IVsTestConsoleWrapper consoleWrapper; | ||
private IVsTestConsoleWrapper consoleWrapper; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 consoleParamters; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spell |
||
|
||
[TestInitialize] | ||
public void TestInitialize() | ||
{ | ||
this.consoleParamters = 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.consoleParamters); | ||
|
||
this.mockRequestSender.Setup(rs => rs.WaitForRequestHandlerConnection(It.IsAny<int>())).Returns(true); | ||
this.mockRequestSender.Setup(rs => rs.InitializeCommunication()).Returns(100); | ||
|
@@ -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.consoleParamters.ParentProcessId, "Parent process Id must be set"); | ||
Assert.AreEqual(inputPort, this.consoleParamters.PortNumber, "Port number must be set"); | ||
|
||
this.mockProcessManager.Verify(pm => pm.StartProcess(this.consoleParamters), Times.Once); | ||
} | ||
|
||
[TestMethod] | ||
|
@@ -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] | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?