Testhost sharing between discovery & execution

src/Microsoft.TestPlatform.Client/TestPlatform.cs

@@ -157,7 +157,7 @@ public ITestRunRequest CreateTestRunRequest(
         }
 
         /// <inheritdoc/>
-        public void StartTestSession(
+        public bool StartTestSession(
             IRequestData requestData,
             StartTestSessionCriteria testSessionCriteria,
             ITestSessionEventsHandler eventsHandler)
@@ -170,44 +170,23 @@ public void StartTestSession(
             this.AddExtensionAssemblies(testSessionCriteria.RunSettings);
 
             var runConfiguration = XmlRunSettingsUtilities.GetRunConfigurationNode(testSessionCriteria.RunSettings);
-
-            // Update extension assemblies from source when design mode is false.
-            //
-            // TODO (copoiena): Is it possible for this code to run if we're not in design mode ?
-            // An use case for this would be when running tests with "dotnet test". Usually there's
-            // a build involved then.
             if (!runConfiguration.DesignMode)
             {
-                return;
-            }
-
-            // Initialize loggers.
-            var loggerManager = this.TestEngine.GetLoggerManager(requestData);
-            loggerManager.Initialize(testSessionCriteria.RunSettings);
-
-            var testHostManager = this.testHostProviderManager.GetTestHostManagerByRunConfiguration(testSessionCriteria.RunSettings);
-            ThrowExceptionIfTestHostManagerIsNull(testHostManager, testSessionCriteria.RunSettings);
-
-            testHostManager.Initialize(TestSessionMessageLogger.Instance, testSessionCriteria.RunSettings);
-
-            if (testSessionCriteria.TestHostLauncher != null)
-            {
-                testHostManager.SetCustomLauncher(testSessionCriteria.TestHostLauncher);
+                return false;
             }
 
-            var testSessionManager = this.TestEngine.GetTestSessionManager(requestData, testHostManager, testSessionCriteria);
+            var testSessionManager = this.TestEngine.GetTestSessionManager(requestData, testSessionCriteria);
             if (testSessionManager == null)
             {
                 // The test session manager is null because the combination of runsettings and
                 // sources tells us we should run in-process (i.e. in vstest.console). Because
                 // of this no session will be created because there's no testhost to be launched.
                 // Expecting a subsequent call to execute tests with the same set of parameters.
                 eventsHandler.HandleStartTestSessionComplete(null);
-                return;
+                return false;
             }
 
-            testSessionManager.Initialize(false);
-            testSessionManager.StartSession(testSessionCriteria, eventsHandler);
+            return testSessionManager.StartSession(eventsHandler);
         }
 
         /// <summary>
@@ -234,11 +213,11 @@ public void ClearExtensions()
 
         private void ThrowExceptionIfTestHostManagerIsNull(
             ITestRuntimeProvider testHostManager,
-            string settingXml)
+            string settingsXml)
         {
             if (testHostManager == null)
             {
-                EqtTrace.Error("TestPlatform.CreateTestRunRequest: No suitable testHostProvider found for runsettings : {0}", settingXml);
+                EqtTrace.Error("TestPlatform.CreateTestRunRequest: No suitable testHostProvider found for runsettings : {0}", settingsXml);
                 throw new TestPlatformException(string.Format(CultureInfo.CurrentCulture, ClientResources.NoTestHostProviderFound));
             }
         }

src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyTestSessionManager.cs

@@ -11,28 +11,22 @@ namespace Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine
     /// </summary>
     public interface IProxyTestSessionManager
     {
-        /// <summary>
-        /// Initialize the proxy.
-        /// </summary>
-        /// 
-        /// <param name="skipDefaultAdapters">Skip default adapters flag.</param>
-        void Initialize(bool skipDefaultAdapters);
-
         /// <summary>
         /// Starts the test session based on the test session criteria.
         /// </summary>
         /// 
-        /// <param name="criteria">The test session criteria.</param>
         /// <param name="eventsHandler">
         /// Event handler for handling events fired during test session management operations.
         /// </param>
-        void StartSession(
-            StartTestSessionCriteria criteria,
-            ITestSessionEventsHandler eventsHandler);
+        /// 
+        /// <returns>True if the operation succeeded, false otherwise.</returns>
+        bool StartSession(ITestSessionEventsHandler eventsHandler);
 
         /// <summary>
         /// Stops the test session.
         /// </summary>
-        void StopSession();
+        ///
+        /// <returns>True if the operation succeeded, false otherwise.</returns>
+        bool StopSession();
     }
 }

src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/ITestEngine.cs

@@ -53,15 +53,13 @@ IProxyExecutionManager GetExecutionManager(
         /// <param name="requestData">
         /// The request data for providing test session services and data.
         /// </param>
-        /// <param name="testHostManager">Test host manager for the current test session.</param>
         /// <param name="testSessionCriteria">
         /// Test session criteria of the current test session.
         /// </param>
         /// 
         /// <returns>An IProxyTestSessionManager object that can manage test sessions.</returns>
         IProxyTestSessionManager GetTestSessionManager(
             IRequestData requestData,
-            ITestRuntimeProvider testHostManager,
             StartTestSessionCriteria testSessionCriteria);
 
         /// <summary>

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

@@ -127,6 +127,8 @@ internal TestRequestSender(
         {
         }
 
+        public bool CloseConnectionOnOperationComplete { get; set; } = true;
+
         /// <inheritdoc />
         public int InitializeCommunication()
         {
@@ -727,6 +729,16 @@ private bool IsOperationComplete()
 
         private void SetOperationComplete()
         {
+            // When sharing the testhost between discovery and execution we must keep the
+            // testhost alive after completing the operation it was spawned for. As such we
+            // suppress the test request sender channel close taking place here. This channel
+            // will be closed when the test session owner decides to dispose of the test session
+            // object.
+            if (!this.CloseConnectionOnOperationComplete)
+            {
+                return;
+            }
+
             // Complete the currently ongoing operation (Discovery/Execution)
             if (EqtTrace.IsVerboseEnabled)
             {

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs

@@ -6,6 +6,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
     using System;
     using System.Collections.Generic;
     using System.Linq;
+    using System.Threading;
 
     using Microsoft.VisualStudio.TestPlatform.Common;
     using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework;
@@ -25,17 +26,65 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
     /// </summary>
     public class ProxyDiscoveryManager : IProxyDiscoveryManager, IBaseProxy, ITestDiscoveryEventsHandler2
     {
-        private ProxyOperationManager proxyOperationManager;
-        private readonly ITestRuntimeProvider testHostManager;
-        private IDataSerializer dataSerializer;
-        private bool isCommunicationEstablished;
+        private readonly TestSessionInfo testSessionInfo = null;
+        private readonly string runSettings;
+        private readonly IRequestData backupRequestData;
+        private readonly ITestRequestSender backupTestRequestSender;
+        private readonly ITestRuntimeProvider backupTestHostManager;
+
+        private ITestRuntimeProvider testHostManager;
         private IRequestData requestData;
+
+        private readonly IFileHelper fileHelper;
+        private readonly IDataSerializer dataSerializer;
+        private bool isCommunicationEstablished;
+
+        private ManualResetEvent proxyOperationManagerInitializedEvent = new ManualResetEvent(false);
+        private ProxyOperationManager proxyOperationManager = null;
         private ITestDiscoveryEventsHandler2 baseTestDiscoveryEventsHandler;
         private bool skipDefaultAdapters;
-        private readonly IFileHelper fileHelper;
 
         #region Constructors
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ProxyDiscoveryManager"/> class.
+        /// </summary>
+        /// 
+        /// <param name="testSessionInfo">The test session info.</param>
+        /// <param name="runSettings">The run settings.</param>
+        /// <param name="backupRequestData">
+        /// The backup request data to be used to create a proxy operation manager should acquire
+        /// an existent proxy fail.
+        /// </param>
+        /// <param name="backupTestRequestSender">
+        /// The backup test request sender to be used to create a proxy operation manager should
+        /// acquire an existent proxy fail.
+        /// </param>
+        /// <param name="backupTestHostManager">
+        /// The backup testhost manager to be used to create a proxy operation manager should
+        /// acquire an existent proxy fail.
+        /// </param>
+        public ProxyDiscoveryManager(
+            TestSessionInfo testSessionInfo,
+            string runSettings,
+            IRequestData backupRequestData,
+            ITestRequestSender backupTestRequestSender,
+            ITestRuntimeProvider backupTestHostManager)
+        {
+            // Filling in test session info and proxy information.
+            this.testSessionInfo = testSessionInfo;
+            this.runSettings = runSettings;
+            this.backupRequestData = backupRequestData;
+            this.backupTestRequestSender = backupTestRequestSender;
+            this.backupTestHostManager = backupTestHostManager;
+
+            this.requestData = null;
+            this.testHostManager = null;
+            this.dataSerializer = JsonDataSerializer.Instance;
+            this.fileHelper = new FileHelper();
+            this.isCommunicationEstablished = false;
+        }
+
         /// <summary>
         /// Initializes a new instance of the <see cref="ProxyDiscoveryManager"/> class.
         /// </summary>
@@ -55,9 +104,7 @@ public ProxyDiscoveryManager(
                   testHostManager,
                   JsonDataSerializer.Instance,
                   new FileHelper())
-        {
-            this.testHostManager = testHostManager;
-        }
+        { }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ProxyDiscoveryManager"/> class.
@@ -81,14 +128,16 @@ internal ProxyDiscoveryManager(
             IDataSerializer dataSerializer,
             IFileHelper fileHelper)
         {
-            this.dataSerializer = dataSerializer;
-            this.testHostManager = testHostManager;
-            this.isCommunicationEstablished = false;
             this.requestData = requestData;
+            this.testHostManager = testHostManager;
+
+            this.dataSerializer = dataSerializer;
             this.fileHelper = fileHelper;
+            this.isCommunicationEstablished = false;
 
             // Create a new proxy operation manager.
             this.proxyOperationManager = new ProxyOperationManager(requestData, requestSender, testHostManager, this);
+            this.proxyOperationManagerInitializedEvent.Set();
         }
 
         #endregion
@@ -104,6 +153,37 @@ public void Initialize(bool skipDefaultAdapters)
         /// <inheritdoc/>
         public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)
         {
+            if (this.proxyOperationManager == null)
+            {
+                // In case we have an active test session, we always prefer the already
+                // created proxies instead of the ones that need to be created on the spot.
+                this.proxyOperationManager = TestSessionPool.Instance.TakeProxy(
+                    this.testSessionInfo,
+                    discoveryCriteria.Sources.First(),
+                    runSettings);
+
+                if (this.proxyOperationManager == null)
+                {
+                    // If the proxy creation process based on test session info failed, then
+                    // we'll proceed with the normal creation process as if no test session
+                    // info was passed in in the first place.
+                    // 
+                    // WARNING: This should not normally happen and it raises questions
+                    // regarding the test session pool operation and consistency.
+                    EqtTrace.Warning("ProxyDiscoveryManager creation with test session failed.");
+
+                    this.proxyOperationManager = new ProxyOperationManager(
+                        this.backupRequestData,
+                        this.backupTestRequestSender,
+                        this.backupTestHostManager,
+                        this);
+                }
+
+                this.proxyOperationManagerInitializedEvent.Set();
+                this.testHostManager = this.proxyOperationManager.TestHostManager;
+                this.requestData = this.proxyOperationManager.RequestData;
+            }
+
             this.baseTestDiscoveryEventsHandler = eventHandler;
             try
             {
@@ -149,6 +229,9 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve
         /// <inheritdoc/>
         public void Abort()
         {
+            // Make sure the proxy operation manager is initialized before anything.
+            this.proxyOperationManagerInitializedEvent.WaitOne();
+
             // Cancel fast, try to stop testhost deployment/launch
             this.proxyOperationManager.CancellationTokenSource.Cancel();
             this.Close();
@@ -157,7 +240,22 @@ public void Abort()
         /// <inheritdoc/>
         public void Close()
         {
-            this.proxyOperationManager.Close();
+            // Make sure the proxy operation manager is initialized before anything.
+            this.proxyOperationManagerInitializedEvent.WaitOne();
+
+            // In compatibility mode (no test session used) we don't share the testhost
+            // between test discovery and test run. The testhost is closed upon
+            // successfully completing the operation it was spawned for.
+            //
+            // In contrast, the new workflow (using test sessions) means we should keep
+            // the testhost alive until explicitly closed by the test session owner.
+            if (this.testSessionInfo == null)
+            {
+                this.proxyOperationManager.Close();
+                return;
+            }
+
+            TestSessionPool.Instance.ReturnProxy(this.testSessionInfo, this.proxyOperationManager.Id);
         }
 
         /// <inheritdoc/>