From f7f445c191fc1e559e7aa41c035e8dbc81f0f8ce Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 14 Dec 2022 14:12:29 +0100 Subject: [PATCH 1/2] Added support for checking testhost compatibility with test sessions --- .../Client/ProxyOperationManager.cs | 41 ++++++++++++++++++- .../PublicAPI/PublicAPI.Shipped.txt | 2 - .../TestEngine.cs | 12 +++++- .../TestSession/ProxyTestSessionManager.cs | 29 ++++++++++--- .../Hosting/DefaultTestHostManager.cs | 2 + .../Hosting/DotnetTestHostManager.cs | 2 + 6 files changed, 78 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs index 55d11267fe..0a1e17de88 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs @@ -114,7 +114,13 @@ public class ProxyOperationManager /// /// Gets the proxy operation manager id for proxy test session manager internal organization. /// - public int Id { get; set; } = -1; + internal int Id { get; set; } = -1; + + /// + /// Gets or sets a value indicating whether the current proxy operation manager is part of a + /// test session. + /// + internal bool IsTestSessionEnabled { get; set; } /// /// Gets or sets the cancellation token source. @@ -177,6 +183,13 @@ public virtual bool SetupChannel(IEnumerable 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 +416,32 @@ public virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessStartI return updatedRunSettingsXml; } + private 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. + const string dotnetTesthostFriendlyName = "DotnetTestHost"; + const string defaultTesthostFriendlyName = "DefaultTestHost"; + + var friendlyNameAttribute = TestHostManager.GetType().GetCustomAttributes(typeof(FriendlyNameAttribute), true).FirstOrDefault(); + if (friendlyNameAttribute is not null and FriendlyNameAttribute friendlyName) + { + return friendlyName.FriendlyName is (dotnetTesthostFriendlyName or defaultTesthostFriendlyName); + } + + return false; + } + [return: NotNullIfNotNull("logFile")] private static string? GetTimestampedLogFile(string? logFile) { diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt b/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt index b2b5c873ef..7f4ee98236 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt +++ b/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! logMessage) -> string? diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs index 6ba0e18253..e94f8fd47d 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs +++ b/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 GetTestRuntimeProvidersForUniqueConfigurations( diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs index a05b950f97..dc3d4300c9 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs @@ -20,6 +20,12 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine; +internal enum ProxyDisposalOnCreationFailPolicy +{ + DisposeAllOnFailure, + AllowProxySetupFailures +} + /// /// Orchestrates test session operations for the engine communicating with the client. /// @@ -47,6 +53,8 @@ private enum TestSessionState private readonly Dictionary _sourceToRuntimeProviderInfoMap; private Dictionary _testSessionEnvironmentVariables = new(); + internal ProxyDisposalOnCreationFailPolicy DisposalPolicy { get; set; } = ProxyDisposalOnCreationFailPolicy.DisposeAllOnFailure; + private IDictionary TestSessionEnvironmentVariables { get @@ -149,13 +157,24 @@ 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) + { + 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. diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs index 02001d23ab..acaa329dad 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs +++ b/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"; diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs index c3f44f0a9c..6f62385591 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs +++ b/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); From 53bc7c76b40f761bba68f2e6f4054c6fafc98510 Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Tue, 20 Dec 2022 19:57:14 +0100 Subject: [PATCH 2/2] Added tests --- .../Client/ProxyOperationManager.cs | 27 ++++++--- .../TestSession/ProxyTestSessionManager.cs | 3 +- .../Client/ProxyOperationManagerTests.cs | 32 +++++++++++ .../Client/ProxyTestSessionManagerTests.cs | 55 +++++++++++++++++++ 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs index 0a1e17de88..59a5f68a47 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs +++ b/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); @@ -416,7 +419,17 @@ public virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessStartI return updatedRunSettingsXml; } - private bool IsTesthostCompatibleWithTestSessions() + 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. @@ -430,13 +443,13 @@ private bool IsTesthostCompatibleWithTestSessions() // 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. - const string dotnetTesthostFriendlyName = "DotnetTestHost"; - const string defaultTesthostFriendlyName = "DefaultTestHost"; - - var friendlyNameAttribute = TestHostManager.GetType().GetCustomAttributes(typeof(FriendlyNameAttribute), true).FirstOrDefault(); - if (friendlyNameAttribute is not null and FriendlyNameAttribute friendlyName) + var friendlyName = ReadTesthostFriendlyName(); + if (!friendlyName.IsNullOrEmpty()) { - return friendlyName.FriendlyName is (dotnetTesthostFriendlyName or defaultTesthostFriendlyName); + var isSessionSupported = friendlyName is (DotnetTesthostFriendlyName or DefaultTesthostFriendlyName); + EqtTrace.Verbose($"ProxyOperationManager.IsTesthostCompatibleWithTestSessions: Testhost friendly name: {friendlyName}; Sessions support: {isSessionSupported};"); + + return isSessionSupported; } return false; diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs index dc3d4300c9..69f6699855 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs @@ -165,7 +165,8 @@ public virtual bool StartSession(ITestSessionEventsHandler eventsHandler, IReque // to create proxies for. if (_proxySetupFailed) { - if (DisposalPolicy == ProxyDisposalOnCreationFailPolicy.DisposeAllOnFailure) + if (DisposalPolicy == ProxyDisposalOnCreationFailPolicy.DisposeAllOnFailure + || _proxyContainerList.Count == 0) { requestData?.MetricsCollection.Add( TelemetryDataConstants.TestSessionState, diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs index 7f8127035d..f9ec911805 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs +++ b/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(), 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 diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyTestSessionManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyTestSessionManagerTests.cs index 6a73de9005..6d8b822b55 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyTestSessionManagerTests.cs +++ b/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(null, null, null, null); + mockProxyOperationManager.SetupSequence(pom => pom.SetupChannel(It.IsAny>(), It.IsAny())) + .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>(), + It.IsAny()), + Times.Exactly(_fakeTestMultipleSources.Count)); + mockProxyOperationManager.Verify(pom => pom.Close(), Times.Never); + _mockEventsHandler.Verify(eh => eh.HandleStartTestSessionComplete( + It.IsAny()), + Times.Once); + } + + [TestMethod] + public void StartSessionShouldStillFailIfSetupChannelReturnsFalseAndTheProxyDisposalPolicyAllowsFailuresButNoTesthostIsSpawned() + { + var mockProxyOperationManager = new Mock(null, null, null, null); + mockProxyOperationManager.Setup(pom => pom.SetupChannel(It.IsAny>(), It.IsAny())) + .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>(), + It.IsAny()), + Times.Exactly(_fakeTestSources.Count)); + mockProxyOperationManager.Verify(pom => pom.Close(), Times.Never); + _mockEventsHandler.Verify(eh => eh.HandleStartTestSessionComplete( + It.IsAny()), + Times.Never); + } + [TestMethod] public void StartSessionShouldFailIfSetupChannelThrowsException() {