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

Run NetFramework tests inside vstest.console.exe process #1009

Merged
merged 12 commits into from
Aug 28, 2017
Prev Previous commit
Next Next commit
1) Making in process default for net46
2) Enable InIsolation argument to disable default in process.
3) Address PR comment
  • Loading branch information
faahmad authored and faahmad committed Aug 25, 2017
commit 2f76f3c93cbcd975b1ef55ef51aef63e35dc9257
3 changes: 2 additions & 1 deletion src/Microsoft.TestPlatform.Client/TestPlatform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ static TestPlatform()
/// <summary>
/// Initializes a new instance of the <see cref="TestPlatform"/> class.
/// </summary>
public TestPlatform() : this(new TestEngine(), new FileHelper(), TestRuntimeProviderManager.Instance)
/// <param name="isInIsolation">inIsolation command line arg value</param>
public TestPlatform(bool isInIsolation = false) : this(new TestEngine(isInIsolation), new FileHelper(), TestRuntimeProviderManager.Instance)
{
}

Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.TestPlatform.Client/TestPlatformFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ public class TestPlatformFactory
/// Gets an instance of the test platform.
/// </summary>
/// <returns> The <see cref="ITestPlatform"/> instance. </returns>
public static ITestPlatform GetTestPlatform()
public static ITestPlatform GetTestPlatform(bool isInIsolation)
{
return testPlatform ?? (testPlatform = new TestPlatform());
return testPlatform ?? (testPlatform = new TestPlatform(isInIsolation));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;

class InProcessProxyDiscoveryManager : IProxyDiscoveryManager
internal class InProcessProxyDiscoveryManager : IProxyDiscoveryManager
{
private ITestHostManagerFactory testHostManagerFactory;
IDiscoveryManager discoveryManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit annotation for types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next commit.

public bool IsInitialized { get; private set; } = false;

/// <summary>
Expand All @@ -32,6 +33,7 @@ public InProcessProxyDiscoveryManager() : this(new TestHostManagerFactory())
internal InProcessProxyDiscoveryManager(ITestHostManagerFactory testHostManagerFactory)
{
this.testHostManagerFactory = testHostManagerFactory;
this.discoveryManager = this.testHostManagerFactory.GetDiscoveryManager();
}

/// <summary>
Expand All @@ -41,11 +43,9 @@ public void Initialize()
{
if(!this.IsInitialized)
{
var discoveryManager = this.testHostManagerFactory.GetDiscoveryManager();

// We don't need to pass list of extension as we are running inside vstest.console and
// it will use TestPluginCache of vstest.console
discoveryManager.Initialize(Enumerable.Empty<string>());
this.discoveryManager.Initialize(Enumerable.Empty<string>());
this.IsInitialized = true;
}
}
Expand All @@ -57,25 +57,20 @@ public void Initialize()
/// <param name="eventHandler">EventHandler for handling discovery events from Engine</param>
public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler eventHandler)
{
var discoveryManager = this.testHostManagerFactory.GetDiscoveryManager();

Task.Run(() =>
{
try
{
// Initialize extension before discovery if it’s not initialized
if (!this.IsInitialized)
{
discoveryManager.Initialize(Enumerable.Empty<string>());
}
discoveryManager.DiscoverTests(discoveryCriteria, eventHandler);
this.Initialize();
this.discoveryManager.DiscoverTests(discoveryCriteria, eventHandler);
}
catch (Exception exception)
{
EqtTrace.Error("InProcessProxyDiscoveryManager.DiscoverTests: Failed to discover tests: {0}", exception);

// Send a discovery complete to caller.
eventHandler.HandleLogMessage(TestMessageLevel.Error, exception.Message);
eventHandler.HandleLogMessage(TestMessageLevel.Error, exception.ToString());
eventHandler.HandleDiscoveryComplete(-1, new List<TestCase>(), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Enumerable.Empty<TestCase>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;

class InProcessProxyExecutionManager : IProxyExecutionManager
internal class InProcessProxyExecutionManager : IProxyExecutionManager
{
private ITestHostManagerFactory testHostManagerFactory;
IExecutionManager executionManager;
public bool IsInitialized { get; private set; } = false;

/// <summary>
Expand All @@ -35,32 +36,30 @@ public InProcessProxyExecutionManager() : this(new TestHostManagerFactory())
internal InProcessProxyExecutionManager(ITestHostManagerFactory testHostManagerFactory)
{
this.testHostManagerFactory = testHostManagerFactory;
this.executionManager = this.testHostManagerFactory.GetExecutionManager();
}

/// <summary>
/// Initialize adapters.
/// </summary>
public void Initialize()
{
if (!this.IsInitialized)
{
var executionManager = this.testHostManagerFactory.GetExecutionManager();

// We don't need to pass list of extension as we are running inside vstest.console and
// it will use TestPluginCache of vstest.console
executionManager.Initialize(Enumerable.Empty<string>());
this.IsInitialized = true;
}
}

/// <inheritdoc/>
public int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsHandler eventHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

{
try
{
var executionManager = this.testHostManagerFactory.GetExecutionManager();

// Initialize extension before execution if not already initialized
if (!this.IsInitialized)
{
executionManager.Initialize(Enumerable.Empty<string>());
}
this.Initialize();

var executionContext = new TestExecutionContext(
testRunCriteria.FrequencyOfRunStatsChangeEvent,
Expand Down Expand Up @@ -89,8 +88,8 @@ public int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsHandler e
{
EqtTrace.Error("InProcessProxyexecutionManager.StartTestRun: Failed to start test run: {0}", exception);

// Send a discovery complete to caller.
eventHandler.HandleLogMessage(TestMessageLevel.Error, exception.Message);
// Send exception message.
eventHandler.HandleLogMessage(TestMessageLevel.Error, exception.ToString());

// Send a run complete to caller.
var completeArgs = new TestRunCompleteEventArgs(null, false, true, exception, new Collection<AttachmentSet>(), TimeSpan.Zero);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void Initialize(IEnumerable<string> pathToAdditionalExtensions)
{
this.testPlatformEventSource.AdapterSearchStart();

if (pathToAdditionalExtensions != null && pathToAdditionalExtensions.Count() > 0)
if (pathToAdditionalExtensions != null && pathToAdditionalExtensions.Any())
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use pathToAdditionalExtensions.Any() instead of pathToAdditionalExtensions.Count() > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can use

// Start using these additional extensions
TestPluginCache.Instance.DefaultExtensionPaths = pathToAdditionalExtensions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public void Initialize(IEnumerable<string> pathToAdditionalExtensions)
{
this.testPlatformEventSource.AdapterSearchStart();

if (pathToAdditionalExtensions != null && pathToAdditionalExtensions.Count() > 0)
if (pathToAdditionalExtensions != null && pathToAdditionalExtensions.Any())
{
// Start using these additional extensions
TestPluginCache.Instance.DefaultExtensionPaths = pathToAdditionalExtensions;
Expand Down
41 changes: 32 additions & 9 deletions src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,19 @@ public class TestEngine : ITestEngine
private readonly TestRuntimeProviderManager testHostProviderManager;
private ITestExtensionManager testExtensionManager;
private IProcessHelper processHelper;
private bool isInIsolation = false;

#endregion

public TestEngine() : this(TestRuntimeProviderManager.Instance, new ProcessHelper())
public TestEngine(bool isInIsolation = false) : this(TestRuntimeProviderManager.Instance, new ProcessHelper(), isInIsolation)
{
}

protected TestEngine(TestRuntimeProviderManager testHostProviderManager, IProcessHelper processHelper)
protected TestEngine(TestRuntimeProviderManager testHostProviderManager, IProcessHelper processHelper, bool isInIsolation)
{
this.testHostProviderManager = testHostProviderManager;
this.processHelper = processHelper;
this.isInIsolation = isInIsolation;
}

#region ITestEngine implementation
Expand All @@ -63,7 +65,7 @@ public IProxyDiscoveryManager GetDiscoveryManager(ITestRuntimeProvider testHostM
{
var parallelLevel = this.VerifyParallelSettingAndCalculateParallelLevel(discoveryCriteria.Sources.Count(), discoveryCriteria.RunSettings);
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be moved below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next commit.


if (this.ShouldRunInNoIsolation(discoveryCriteria.RunSettings))
if (this.ShouldRunInNoIsolation(discoveryCriteria.RunSettings, parallelLevel > 1, false))
{
return new InProcessProxyDiscoveryManager();
}
Expand Down Expand Up @@ -95,7 +97,7 @@ public IProxyExecutionManager GetExecutionManager(ITestRuntimeProvider testHostM

var isDataCollectorEnabled = XmlRunSettingsUtilities.IsDataCollectionEnabled(testRunCriteria.TestRunSettings);

if (parallelLevel <= 1 && !isDataCollectorEnabled && this.ShouldRunInNoIsolation(testRunCriteria.TestRunSettings))
if (this.ShouldRunInNoIsolation(testRunCriteria.TestRunSettings, parallelLevel > 1, isDataCollectorEnabled))
{
return new InProcessProxyExecutionManager();
}
Expand Down Expand Up @@ -208,11 +210,20 @@ private int VerifyParallelSettingAndCalculateParallelLevel(int sourceCount, stri
return parallelLevelToUse;
}

private bool ShouldRunInNoIsolation(string runsettings)
private bool ShouldRunInNoIsolation(string runsettings, bool isParallelEnabled, bool isDataCollectorEnabled)
{
if(this.isInIsolation == true)
{
if (EqtTrace.IsInfoEnabled)
{
EqtTrace.Info("TestEngine.ShouldRunInNoIsolation: running test in isolation");
}
return false;
}

var currentProcessPath = this.processHelper.GetCurrentProcessFileName();

// If running with the dotnet executable, then dont run in NoIsolation
// If running with the dotnet executable, then dont run in InProcess
if (currentProcessPath.EndsWith("dotnet", StringComparison.OrdinalIgnoreCase)
|| currentProcessPath.EndsWith("dotnet.exe", StringComparison.OrdinalIgnoreCase))
{
Expand All @@ -221,12 +232,24 @@ private bool ShouldRunInNoIsolation(string runsettings)

var runConfiguration = XmlRunSettingsUtilities.GetRunConfigurationNode(runsettings);

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment specifying when will we enable InProc run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if (runConfiguration.InProcess &&
// Return true if
// 1) Not running in parallel
// 2) Data collector is not enabled
// 3) Target framework is x86 or anyCpu
// 4) DisableAppDomain is false
// 5) Not running in design mode
// 6) target framework is NETFramework (Desktop test)
if (!isParallelEnabled &&
!isDataCollectorEnabled &&
(runConfiguration.TargetPlatform == Architecture.X86 || runConfiguration.TargetPlatform == Architecture.AnyCPU) &&
!runConfiguration.DisableAppDomain &&
!runConfiguration.DesignMode &&
!(runConfiguration.TargetFrameworkVersion.Name.IndexOf("netstandard", StringComparison.OrdinalIgnoreCase) >= 0
|| runConfiguration.TargetFrameworkVersion.Name.IndexOf("netcoreapp", StringComparison.OrdinalIgnoreCase) >= 0))
runConfiguration.TargetFrameworkVersion.Name.IndexOf("netframework", StringComparison.OrdinalIgnoreCase) >= 0)
{
if(EqtTrace.IsInfoEnabled)
{
EqtTrace.Info("TestEngine.ShouldRunInNoIsolation: running test in process(inside vstest.console.exe process)");
}
return true ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Log eqttrace message on successful/unsuccessful using of inproc. Give warning on using inproc option for not supported target frameworks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,6 @@ public class RunConfiguration : TestRunSettings
/// </summary>
private bool disableAppDomain;

/// <summary>
/// Specify to not run tests in isolation.
/// </summary>
private bool inProcess;

/// <summary>
/// Indication to adapters to disable parallelization.
/// </summary>
Expand Down Expand Up @@ -100,7 +95,6 @@ public RunConfiguration() : base(Constants.RunConfigurationSettingsName)
this.batchSize = Constants.DefaultBatchSize;
this.testSessionTimeout = 0;
this.disableAppDomain = false;
this.inProcess = false;
this.disableParallelization = false;
this.designMode = false;
this.shouldCollectSourceInformation = false;
Expand Down Expand Up @@ -236,23 +230,6 @@ public bool DisableAppDomain
}
}

/// <summary>
/// Gets or sets a value indicating whether to run tests in isolation or not.
/// </summary>
public bool InProcess
{
get
{
return this.inProcess;
}

set
{
this.inProcess = value;
this.InProcessSet = true;
}
}

/// <summary>
/// Gets a value indicating whether parallelism needs to be disabled by the adapters.
/// </summary>
Expand Down Expand Up @@ -398,15 +375,6 @@ public bool DisableAppDomainSet
private set;
}

/// <summary>
/// Gets a value indicating whether InProcess is set.
/// </summary>
public bool InProcessSet
{
get;
private set;
}

/// <summary>
/// Gets a value indicating whether parallelism needs to be disabled by the adapters.
/// </summary>
Expand Down Expand Up @@ -496,10 +464,6 @@ public override XmlElement ToXml()
disableAppDomain.InnerXml = this.DisableAppDomain.ToString();
root.AppendChild(disableAppDomain);

XmlElement inProcess = doc.CreateElement("InProcess");
inProcess.InnerXml = this.InProcess.ToString();
root.AppendChild(inProcess);

XmlElement disableParallelization = doc.CreateElement("DisableParallelization");
disableParallelization.InnerXml = this.DisableParallelization.ToString();
root.AppendChild(disableParallelization);
Expand Down Expand Up @@ -657,19 +621,6 @@ public static RunConfiguration FromXml(XmlReader reader)
runConfiguration.DisableAppDomain = disableAppDomainCheck;
break;

case "InProcess":
XmlRunSettingsUtilities.ThrowOnHasAttributes(reader);

string inProcessValueString = reader.ReadElementContentAsString();
bool inProcessCheck;
if (!bool.TryParse(inProcessValueString, out inProcessCheck))
{
throw new SettingsException(String.Format(CultureInfo.CurrentCulture,
Resources.Resources.InvalidSettingsIncorrectValue, Constants.RunConfigurationSettingsName, inProcessValueString, elementName));
}
runConfiguration.InProcess = inProcessCheck;
break;

case "DisableParallelization":
XmlRunSettingsUtilities.ThrowOnHasAttributes(reader);

Expand Down
4 changes: 2 additions & 2 deletions src/vstest.console/CommandLine/CommandLineOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ protected CommandLineOptions()
public bool Parallel { get; set; }

/// <summary>
/// Specifies whether InProcess is on or off.
/// Specifies whether InIsolation is on or off.
/// </summary>
public bool InProcess { get; set; }
public bool InIsolation { get; set; }

/// <summary>
/// Readonly collection of all available test sources
Expand Down
Loading