Added support for checking testhost compatibility with test sessions (#…

…4199)

Added support for checking testhost compatibility with test sessions

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

@@ -34,6 +34,9 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client;
 [SuppressMessage("Design", "CA1001:Types that own disposable fields should be disposable", Justification = "Would cause a breaking change if users are inheriting this class and implement IDisposable")]
 public class ProxyOperationManager
 {
+    internal const string DotnetTesthostFriendlyName = "DotnetTestHost";
+    internal const string DefaultTesthostFriendlyName = "DefaultTestHost";
+
     private readonly string _versionCheckPropertyName = "IsVersionCheckRequired";
     private readonly string _makeRunsettingsCompatiblePropertyName = "MakeRunsettingsCompatible";
     private readonly ManualResetEventSlim _testHostExited = new(false);
@@ -114,7 +117,13 @@ public class ProxyOperationManager
     /// <summary>
     /// Gets the proxy operation manager id for proxy test session manager internal organization.
     /// </summary>
-    public int Id { get; set; } = -1;
+    internal int Id { get; set; } = -1;
+
+    /// <summary>
+    /// Gets or sets a value indicating whether the current proxy operation manager is part of a
+    /// test session.
+    /// </summary>
+    internal bool IsTestSessionEnabled { get; set; }
 
     /// <summary>
     /// Gets or sets the cancellation token source.
@@ -177,6 +186,13 @@ public virtual bool SetupChannel(IEnumerable<string> sources, string? runSetting
             return true;
         }
 
+        // Check whether test sessions are supported if the current proxy operation manager is to
+        // be part of one.
+        if (IsTestSessionEnabled && !IsTesthostCompatibleWithTestSessions())
+        {
+            return false;
+        }
+
         var connTimeout = EnvironmentHelper.GetConnectionTimeout();
 
         _testHostProcessStdError = string.Empty;
@@ -403,6 +419,42 @@ public virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessStartI
         return updatedRunSettingsXml;
     }
 
+    internal virtual string ReadTesthostFriendlyName()
+    {
+        var friendlyNameAttribute = TestHostManager.GetType().GetCustomAttributes(
+                typeof(FriendlyNameAttribute), true)
+            .FirstOrDefault();
+
+        return (friendlyNameAttribute is not null and FriendlyNameAttribute friendlyName)
+            ? friendlyName.FriendlyName : string.Empty;
+    }
+
+    internal bool IsTesthostCompatibleWithTestSessions()
+    {
+        // These constants should be kept in line with the friendly names found in
+        // DotnetTestHostManager.cs, respectively DefaultTestHostManager.cs.
+        //
+        // We agreed on checking the test session compatibility this way (i.e. by reading the
+        // friendly name and making sure it's one of the testhosts we control) instead of a more
+        // generic alternative that was initially proposed (i.e. by decorating each testhost
+        // manager with a capability attribute that could tell us if the test session scenario
+        // is supported for the testhost in discussion) because of the breaking risks associated
+        // with the latter approach. Also, there is no formal specification for now of what it
+        // means to support test sessions. Should extending session functionality to 3rd party
+        // testhosts be something we want to address in the future, we should come up with such
+        // a specification first.
+        var friendlyName = ReadTesthostFriendlyName();
+        if (!friendlyName.IsNullOrEmpty())
+        {
+            var isSessionSupported = friendlyName is (DotnetTesthostFriendlyName or DefaultTesthostFriendlyName);
+            EqtTrace.Verbose($"ProxyOperationManager.IsTesthostCompatibleWithTestSessions: Testhost friendly name: {friendlyName}; Sessions support: {isSessionSupported};");
+
+            return isSessionSupported;
+        }
+
+        return false;
+    }
+
     [return: NotNullIfNotNull("logFile")]
     private static string? GetTimestampedLogFile(string? logFile)
     {

src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt

@@ -41,8 +41,6 @@ Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.CancellationTokenSource.get -> System.Threading.CancellationTokenSource!
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.CancellationTokenSource.set -> void
-Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.Id.get -> int
-Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.Id.set -> void
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.ProxyOperationManager(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData? requestData, Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces.ITestRequestSender! requestSender, Microsoft.VisualStudio.TestPlatform.ObjectModel.Host.ITestRuntimeProvider! testHostManager, Microsoft.VisualStudio.TestPlatform.ObjectModel.Framework! testhostManagerFramework) -> void
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.ProxyOperationManager(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData? requestData, Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces.ITestRequestSender! requestSender, Microsoft.VisualStudio.TestPlatform.ObjectModel.Host.ITestRuntimeProvider! testHostManager, Microsoft.VisualStudio.TestPlatform.ObjectModel.Framework? testhostManagerFramework, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IBaseProxy? baseProxy) -> void
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.RemoveNodesFromRunsettingsIfRequired(string? runsettingsXml, System.Action<Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging.TestMessageLevel, string!>! logMessage) -> string?

src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs

@@ -420,14 +420,22 @@ internal IProxyExecutionManager CreateNonParallelExecutionManager(IRequestData r
                     requestSender,
                     hostManager,
                     // There is always at least one, and all of them have the same framework and architecture.
-                    testRuntimeProviderInfo.SourceDetails[0].Framework!);
+                    testRuntimeProviderInfo.SourceDetails[0].Framework!)
+                {
+                    IsTestSessionEnabled = true
+                };
         };
 
         // TODO: This condition should be returning the maxParallel level to avoid pre-starting way too many testhosts, because maxParallel level,
         // can be smaller than the number of sources to run.
         var maxTesthostCount = isParallelRun ? testSessionCriteria.Sources.Count : 1;
 
-        return new ProxyTestSessionManager(testSessionCriteria, maxTesthostCount, proxyCreator, testRuntimeProviders);
+        return new ProxyTestSessionManager(testSessionCriteria, maxTesthostCount, proxyCreator, testRuntimeProviders)
+        {
+            // Individual proxy setup failures are tolerated since SetupChannel may fail if the
+            // testhost it tries to start is not compatible with the test session feature.
+            DisposalPolicy = ProxyDisposalOnCreationFailPolicy.AllowProxySetupFailures
+        };
     }
 
     private List<TestRuntimeProviderInfo> GetTestRuntimeProvidersForUniqueConfigurations(

src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs

@@ -20,6 +20,12 @@
 
 namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine;
 
+internal enum ProxyDisposalOnCreationFailPolicy
+{
+    DisposeAllOnFailure,
+    AllowProxySetupFailures
+}
+
 /// <summary>
 /// Orchestrates test session operations for the engine communicating with the client.
 /// </summary>
@@ -47,6 +53,8 @@ private enum TestSessionState
     private readonly Dictionary<string, TestRuntimeProviderInfo> _sourceToRuntimeProviderInfoMap;
     private Dictionary<string, string?> _testSessionEnvironmentVariables = new();
 
+    internal ProxyDisposalOnCreationFailPolicy DisposalPolicy { get; set; } = ProxyDisposalOnCreationFailPolicy.DisposeAllOnFailure;
+
     private IDictionary<string, string?> TestSessionEnvironmentVariables
     {
         get
@@ -149,13 +157,25 @@ public virtual bool StartSession(ITestSessionEventsHandler eventsHandler, IReque
             stopwatch.Elapsed.TotalSeconds);
 
         // Dispose of all proxies if even one of them failed during setup.
+        //
+        // Update: With the introduction of the proxy creation fail disposal policy, we now support
+        // the scenario of individual proxy setup failures. What this means is that we don't mark
+        // the whole session as failed if a single proxy fails, but instead we'll reuse the spinned
+        // off testhosts when possible and create on-demand testhosts for the sources that we failed
+        // to create proxies for.
         if (_proxySetupFailed)
         {
-            requestData?.MetricsCollection.Add(
-                TelemetryDataConstants.TestSessionState,
-                TestSessionState.Error.ToString());
-            DisposeProxies();
-            return false;
+            if (DisposalPolicy == ProxyDisposalOnCreationFailPolicy.DisposeAllOnFailure
+                || _proxyContainerList.Count == 0)
+            {
+                requestData?.MetricsCollection.Add(
+                    TelemetryDataConstants.TestSessionState,
+                    TestSessionState.Error.ToString());
+                DisposeProxies();
+                return false;
+            }
+
+            EqtTrace.Info($"ProxyTestSessionManager.StartSession: At least one proxy setup failed, but failures are tolerated by policy.");
         }
 
         // Make the session available.

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs

@@ -45,6 +45,8 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting;
 public class DefaultTestHostManager : ITestRuntimeProvider2
 {
     private const string DefaultTestHostUri = "HostProvider://DefaultTestHost";
+    // Should the friendly name ever change, please make sure to change the corresponding constant
+    // inside ProxyOperationManager::IsTesthostCompatibleWithTestSessions().
     private const string DefaultTestHostFriendlyName = "DefaultTestHost";
     private const string TestAdapterEndsWithPattern = @"TestAdapter.dll";
 

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs

@@ -50,6 +50,8 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting;
 public class DotnetTestHostManager : ITestRuntimeProvider2
 {
     private const string DotnetTestHostUri = "HostProvider://DotnetTestHost";
+    // Should the friendly name ever change, please make sure to change the corresponding constant
+    // inside ProxyOperationManager::IsTesthostCompatibleWithTestSessions().
     private const string DotnetTestHostFriendlyName = "DotnetTestHost";
     private const string TestAdapterRegexPattern = @"TestAdapter.dll";
     private const string PROCESSOR_ARCHITECTURE = nameof(PROCESSOR_ARCHITECTURE);

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs

@@ -102,6 +102,31 @@ public void SetupChannelShouldCreateTimestampedLogFileForHost()
 #endif
     }
 
+    [TestMethod]
+    [DataRow("Dummy", true, false, false)]
+    [DataRow(ProxyOperationManager.DefaultTesthostFriendlyName, true, true, true)]
+    [DataRow(ProxyOperationManager.DotnetTesthostFriendlyName, true, true, true)]
+    public void SetupChannelOutcomeShouldTakeTesthostSessionSupportIntoAccount(
+        string testhostFriendlyName,
+        bool isTestSessionEnabled,
+        bool expectedCompatibilityCheckResult,
+        bool expectedSetupResult)
+    {
+        _mockRequestSender.Setup(rs => rs.InitializeCommunication()).Returns(123);
+
+        var testOperationManager = new TestableProxyOperationManager(
+            _mockRequestData.Object,
+            _mockRequestSender.Object,
+            _mockTestHostManager.Object)
+        {
+            IsTestSessionEnabled = isTestSessionEnabled,
+            TesthostFriendlyName = testhostFriendlyName
+        };
+
+        Assert.IsTrue(testOperationManager.IsTesthostCompatibleWithTestSessions() == expectedCompatibilityCheckResult);
+        Assert.IsTrue(testOperationManager.SetupChannel(Enumerable.Empty<string>(), DefaultRunSettings) == expectedSetupResult);
+    }
+
     [TestMethod]
     public void SetupChannelShouldAddRunnerProcessIdForTestHost()
     {
@@ -519,6 +544,13 @@ private class TestableProxyOperationManager : ProxyOperationManager
         {
             CancellationTokenSource = cancellationTokenSource;
         }
+
+        public string TesthostFriendlyName { get; set; } = "Dummy";
+
+        internal override string ReadTesthostFriendlyName()
+        {
+            return TesthostFriendlyName;
+        }
     }
 
     private class TestableDotnetTestHostManager : DotnetTestHostManager

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyTestSessionManagerTests.cs

@@ -206,6 +206,61 @@ public void StartSessionShouldFailIfSetupChannelReturnsFalse()
             Times.Never);
     }
 
+    [TestMethod]
+    public void StartSessionShouldNotFailIfSetupChannelReturnsFalseButTheProxyDisposalPolicyAllowsFailures()
+    {
+        var mockProxyOperationManager = new Mock<ProxyOperationManager>(null, null, null, null);
+        mockProxyOperationManager.SetupSequence(pom => pom.SetupChannel(It.IsAny<IEnumerable<string>>(), It.IsAny<string>()))
+            .Returns(true)
+            .Returns(false)
+            .Returns(false)
+            .Returns(false)
+            .Returns(false)
+            .Returns(false)
+            .Returns(false)
+            .Returns(false);
+        mockProxyOperationManager.Setup(pom => pom.Close()).Callback(() => { });
+
+        var testSessionCriteria = CreateTestSession(_fakeTestMultipleSources, _fakeRunSettings);
+        var proxyManager = CreateProxy(testSessionCriteria, mockProxyOperationManager.Object);
+        proxyManager.DisposalPolicy = ProxyDisposalOnCreationFailPolicy.AllowProxySetupFailures;
+
+        // Call fails because SetupChannel returns false.
+        Assert.IsTrue(proxyManager.StartSession(_mockEventsHandler.Object, _mockRequestData.Object));
+        mockProxyOperationManager.Verify(pom => pom.SetupChannel(
+                It.IsAny<IEnumerable<string>>(),
+                It.IsAny<string>()),
+            Times.Exactly(_fakeTestMultipleSources.Count));
+        mockProxyOperationManager.Verify(pom => pom.Close(), Times.Never);
+        _mockEventsHandler.Verify(eh => eh.HandleStartTestSessionComplete(
+                It.IsAny<StartTestSessionCompleteEventArgs>()),
+            Times.Once);
+    }
+
+    [TestMethod]
+    public void StartSessionShouldStillFailIfSetupChannelReturnsFalseAndTheProxyDisposalPolicyAllowsFailuresButNoTesthostIsSpawned()
+    {
+        var mockProxyOperationManager = new Mock<ProxyOperationManager>(null, null, null, null);
+        mockProxyOperationManager.Setup(pom => pom.SetupChannel(It.IsAny<IEnumerable<string>>(), It.IsAny<string>()))
+            .Returns(false);
+        mockProxyOperationManager.Setup(pom => pom.Close()).Callback(() => { });
+
+        var testSessionCriteria = CreateTestSession(_fakeTestSources, _fakeRunSettings);
+        var proxyManager = CreateProxy(testSessionCriteria, mockProxyOperationManager.Object);
+        proxyManager.DisposalPolicy = ProxyDisposalOnCreationFailPolicy.AllowProxySetupFailures;
+
+        // Call fails because SetupChannel returns false.
+        Assert.IsFalse(proxyManager.StartSession(_mockEventsHandler.Object, _mockRequestData.Object));
+        mockProxyOperationManager.Verify(pom => pom.SetupChannel(
+                It.IsAny<IEnumerable<string>>(),
+                It.IsAny<string>()),
+            Times.Exactly(_fakeTestSources.Count));
+        mockProxyOperationManager.Verify(pom => pom.Close(), Times.Never);
+        _mockEventsHandler.Verify(eh => eh.HandleStartTestSessionComplete(
+                It.IsAny<StartTestSessionCompleteEventArgs>()),
+            Times.Never);
+    }
+
     [TestMethod]
     public void StartSessionShouldFailIfSetupChannelThrowsException()
     {