From 77563658778b92102a95203637fd2909232659e5 Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Tue, 24 May 2022 16:58:12 +0200 Subject: [PATCH 1/5] Fixed request data sharing --- .../Discovery/DiscoveryManager.cs | 3 ++ .../Execution/ExecutionManager.cs | 3 ++ .../PublicAPI/PublicAPI.Shipped.txt | 4 +-- .../TestEngine.cs | 32 ++++-------------- .../TestHostManagerFactory.cs | 33 ++++++++++++------- .../TestSession/TestSessionPool.cs | 12 +++++-- src/testhost.x86/DefaultEngineInvoker.cs | 30 +++++------------ .../Client/ProxyDiscoveryManagerTests.cs | 9 +++-- .../Client/ProxyExecutionManagerTests.cs | 9 +++-- .../TestHostManagerFactoryTests.cs | 8 +---- .../TestSession/TestSessionPoolTests.cs | 7 ++-- 11 files changed, 71 insertions(+), 79 deletions(-) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs index 87f35405a3..93fc54fea3 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs @@ -73,6 +73,9 @@ protected DiscoveryManager(IRequestData requestData, ITestPlatformEventSource te /// The path to additional extensions. public void Initialize(IEnumerable pathToAdditionalExtensions, ITestDiscoveryEventsHandler2 eventHandler) { + // Clear the request data metrics left over from a potential previous run. + _requestData.MetricsCollection.Metrics.Clear(); + _testPlatformEventSource.AdapterSearchStart(); _testDiscoveryEventsHandler = eventHandler; if (pathToAdditionalExtensions != null && pathToAdditionalExtensions.Any()) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs index d99b5729c6..fdb1cf587e 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs @@ -64,6 +64,9 @@ protected ExecutionManager(ITestPlatformEventSource testPlatformEventSource, IRe /// The path to additional extensions. public void Initialize(IEnumerable pathToAdditionalExtensions, ITestMessageEventHandler testMessageEventsHandler) { + // Clear the request data metrics left over from a potential previous run. + _requestData.MetricsCollection.Metrics.Clear(); + _testMessageEventsHandler = testMessageEventsHandler; _testPlatformEventSource.AdapterSearchStart(); diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt b/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt index 0bbca9f3a6..c2603f6056 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt @@ -102,7 +102,7 @@ Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestExtensionManager.UseAddi Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.GetDiscoveryManager() -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol.IDiscoveryManager Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.GetExecutionManager() -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol.IExecutionManager -Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.TestHostManagerFactory(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData requestData) -> void +Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.TestHostManagerFactory(bool telemetryOptedIn) -> void Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool override Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManagerWithDataCollection.Initialize(bool skipDefaultAdapters) -> void override Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManagerWithDataCollection.SetupChannel(System.Collections.Generic.IEnumerable sources, string runSettings, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.ITestMessageEventHandler eventHandler) -> bool @@ -120,7 +120,7 @@ virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.ProxyTestSessionMana virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.ProxyTestSessionManager.EnqueueProxy(int proxyId) -> bool virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.AddSession(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.ProxyTestSessionManager proxyManager) -> bool virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.ReturnProxy(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, int proxyId) -> bool -virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.TryTakeProxy(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, string source, string runSettings) -> Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager +virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.TryTakeProxy(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, string source, string runSettings, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData requestData) -> Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.DataCollectionResult Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.DataCollectionResult.Attachments.get -> System.Collections.ObjectModel.Collection Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.DataCollectionResult.DataCollectionResult(System.Collections.ObjectModel.Collection attachments, System.Collections.ObjectModel.Collection invokedDataCollectors) -> void diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs index 20c85c3f2d..3d347cf0a9 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs @@ -78,11 +78,9 @@ public IProxyDiscoveryManager GetDiscoveryManager( if (ShouldRunInNoIsolation(discoveryCriteria.RunSettings, parallelLevel > 1, false)) { - var isTelemetryOptedIn = requestData.IsTelemetryOptedIn; - var newRequestData = GetRequestData(isTelemetryOptedIn); return new InProcessProxyDiscoveryManager( testHostManager, - new TestHostManagerFactory(newRequestData)); + new TestHostManagerFactory(requestData.IsTelemetryOptedIn)); } // Create one data aggregator per parallel discovery and share it with all the proxy discovery managers. @@ -108,7 +106,8 @@ public IProxyDiscoveryManager GetDiscoveryManager( var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy( discoveryCriteria.TestSessionInfo, source, - discoveryCriteria.RunSettings); + discoveryCriteria.RunSettings, + requestData); if (proxyOperationManager == null) { @@ -181,11 +180,9 @@ public IProxyExecutionManager GetExecutionManager( parallelLevel > 1, isDataCollectorEnabled || isInProcDataCollectorEnabled)) { - var isTelemetryOptedIn = requestData.IsTelemetryOptedIn; - var newRequestData = GetRequestData(isTelemetryOptedIn); return new InProcessProxyExecutionManager( testHostManager, - new TestHostManagerFactory(newRequestData)); + new TestHostManagerFactory(requestData.IsTelemetryOptedIn)); } // SetupChannel ProxyExecutionManager with data collection if data collectors are @@ -216,7 +213,8 @@ public IProxyExecutionManager GetExecutionManager( var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy( testRunCriteria.TestSessionInfo, source, - testRunCriteria.TestRunSettings); + testRunCriteria.TestRunSettings, + requestData); if (proxyOperationManager == null) { @@ -494,24 +492,6 @@ private bool ShouldRunInNoIsolation( return false; } - /// - /// Get request data on basis of telemetry opted in or not. - /// - /// - /// A flag indicating if telemetry is opted in. - /// - /// The request data. - private IRequestData GetRequestData(bool isTelemetryOptedIn) - { - return new RequestData - { - MetricsCollection = isTelemetryOptedIn - ? (IMetricsCollection)new MetricsCollection() - : new NoOpMetricsCollection(), - IsTelemetryOptedIn = isTelemetryOptedIn - }; - } - /// /// Gets test sources from test run criteria. /// diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestHostManagerFactory.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestHostManagerFactory.cs index 77adf82a9a..81c660a6c2 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestHostManagerFactory.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/TestHostManagerFactory.cs @@ -1,12 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.VisualStudio.TestPlatform.Common; +using Microsoft.VisualStudio.TestPlatform.Common.Telemetry; using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Discovery; - using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution; - -using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client; - using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol; #nullable disable @@ -20,17 +18,18 @@ public class TestHostManagerFactory : ITestHostManagerFactory { private IDiscoveryManager _discoveryManager; private IExecutionManager _executionManager; - private readonly IRequestData _requestData; + private readonly bool _telemetryOptedIn; /// /// Initializes a new instance of the class. /// - /// - /// Provide common services and data for a discovery/run request. + /// + /// + /// A value indicating if the telemetry is opted in or not. /// - public TestHostManagerFactory(IRequestData requestData!!) + public TestHostManagerFactory(bool telemetryOptedIn) { - _requestData = requestData; + _telemetryOptedIn = telemetryOptedIn; } /// @@ -38,12 +37,24 @@ public TestHostManagerFactory(IRequestData requestData!!) /// /// The discovery manager. public IDiscoveryManager GetDiscoveryManager() - => _discoveryManager ??= new DiscoveryManager(_requestData); + => _discoveryManager ??= new DiscoveryManager(GetRequestData(_telemetryOptedIn)); /// /// The execution manager instance for any discovery related operations inside of the test host. /// /// The execution manager. public IExecutionManager GetExecutionManager() - => _executionManager ??= new ExecutionManager(_requestData); + => _executionManager ??= new ExecutionManager(GetRequestData(_telemetryOptedIn)); + + private RequestData GetRequestData(bool telemetryOptedIn) + { + return new RequestData + { + MetricsCollection = + telemetryOptedIn + ? new MetricsCollection() + : new NoOpMetricsCollection(), + IsTelemetryOptedIn = telemetryOptedIn + }; + } } diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs index db45cebd8e..6bc4f224a5 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs @@ -125,12 +125,14 @@ public virtual bool KillSession(TestSessionInfo testSessionInfo, IRequestData re /// The test session info object. /// The source to be associated to this proxy. /// The run settings. + /// The request data. /// /// The proxy object. public virtual ProxyOperationManager TryTakeProxy( TestSessionInfo testSessionInfo, string source, - string runSettings) + string runSettings, + IRequestData requestData!!) { ProxyTestSessionManager sessionManager = null; lock (_lockObject) @@ -147,7 +149,13 @@ public virtual ProxyOperationManager TryTakeProxy( try { // Deque an actual proxy to do work. - return sessionManager.DequeueProxy(source, runSettings); + var proxy = sessionManager.DequeueProxy(source, runSettings); + + // Make sure we use the per-request request data instead of the request data used when + // creating the test session. + proxy.RequestData = requestData; + + return proxy; } catch (InvalidOperationException ex) { diff --git a/src/testhost.x86/DefaultEngineInvoker.cs b/src/testhost.x86/DefaultEngineInvoker.cs index 1062ae7342..dc1a1d1f47 100644 --- a/src/testhost.x86/DefaultEngineInvoker.cs +++ b/src/testhost.x86/DefaultEngineInvoker.cs @@ -135,13 +135,13 @@ public void Invoke(IDictionary argsDictionary) ConnectToDatacollector(dcPort); } - var requestData = GetRequestData(argsDictionary); + var telemetryOptedIn = GetTelemetryStatusFromArgs(argsDictionary); // Start processing async in a different task EqtTrace.Info("DefaultEngineInvoker.Invoke: Start Request Processing."); try { - StartProcessingAsync(_requestHandler, new TestHostManagerFactory(requestData)).Wait(); + StartProcessingAsync(_requestHandler, new TestHostManagerFactory(telemetryOptedIn)).Wait(); } finally { @@ -155,29 +155,15 @@ public void Invoke(IDictionary argsDictionary) } } - private static RequestData GetRequestData(IDictionary argsDictionary) + private static bool GetTelemetryStatusFromArgs(IDictionary argsDictionary) { - // Checks for Telemetry Opted in or not from Command line Arguments. - // By Default opting out in Test Host to handle scenario when user running old version of vstest.console + // Checks if telemetry is opted in in the command line arguments. + // By default opting out in testhost to handle the scenario when the user is running an old + // version of vstest.console. var telemetryStatus = CommandLineArgumentsHelper.GetStringArgFromDict(argsDictionary, TelemetryOptedIn); - var telemetryOptedIn = false; - if (!string.IsNullOrWhiteSpace(telemetryStatus)) - { - if (telemetryStatus.Equals("true", StringComparison.Ordinal)) - { - telemetryOptedIn = true; - } - } - var requestData = new RequestData - { - MetricsCollection = - telemetryOptedIn - ? new MetricsCollection() - : new NoOpMetricsCollection(), - IsTelemetryOptedIn = telemetryOptedIn - }; - return requestData; + return !string.IsNullOrWhiteSpace(telemetryStatus) + && telemetryStatus.Equals("true", StringComparison.Ordinal); } private void ConnectToDatacollector(int dcPort) diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyDiscoveryManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyDiscoveryManagerTests.cs index f739fac1de..2e93ba850c 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyDiscoveryManagerTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyDiscoveryManagerTests.cs @@ -529,7 +529,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull() var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy( testSessionInfo, source, - _discoveryCriteria.RunSettings); + _discoveryCriteria.RunSettings, + _mockRequestData.Object); return proxyOperationManager; }; @@ -551,7 +552,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull() tsp => tsp.TryTakeProxy( testSessionInfo, It.IsAny(), - It.IsAny())) + It.IsAny(), + _mockRequestData.Object)) .Returns(mockProxyOperationManager.Object); testDiscoveryManager.Initialize(true); @@ -563,7 +565,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull() tsp => tsp.TryTakeProxy( testSessionInfo, It.IsAny(), - It.IsAny()), + It.IsAny(), + _mockRequestData.Object), Times.Once); } finally diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerTests.cs index f380436db7..15fc91e26c 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerTests.cs @@ -742,7 +742,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull() var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy( testSessionInfo, source, - string.Empty); + string.Empty, + _mockRequestData.Object); return proxyOperationManager; }; @@ -765,7 +766,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull() tsp => tsp.TryTakeProxy( testSessionInfo, It.IsAny(), - It.IsAny())) + It.IsAny(), + _mockRequestData.Object)) .Returns(mockProxyOperationManager.Object); testExecutionManager.Initialize(true); @@ -777,7 +779,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull() tsp => tsp.TryTakeProxy( testSessionInfo, It.IsAny(), - It.IsAny()), + It.IsAny(), + _mockRequestData.Object), Times.Once); } finally diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestHostManagerFactoryTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestHostManagerFactoryTests.cs index 786f10c6d0..6a178e2d7d 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestHostManagerFactoryTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestHostManagerFactoryTests.cs @@ -23,13 +23,7 @@ public TestHostManagerFactoryTests() _mockMetricsCollection = new Mock(); _mockRequestData = new Mock(); _mockRequestData.Setup(rd => rd.MetricsCollection).Returns(_mockMetricsCollection.Object); - _testHostManagerFactory = new TestHostManagerFactory(_mockRequestData.Object); - } - - [TestMethod] - public void ConstructorShouldThrowIfRequestDataIsNull() - { - Assert.ThrowsException(() => new TestHostManagerFactory(null)); + _testHostManagerFactory = new TestHostManagerFactory(false); } [TestMethod] diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestSession/TestSessionPoolTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestSession/TestSessionPoolTests.cs index c1a7a165eb..6ae357ce86 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestSession/TestSessionPoolTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestSession/TestSessionPoolTests.cs @@ -66,6 +66,7 @@ public void TakeProxyShouldSucceedIfMatchingCriteriaAreCorrect() TestSessionPool.Instance = null; var testSessionInfo = new TestSessionInfo(); + var mockRequestData = new Mock(); var mockProxyTestSessionManager = new Mock( new StartTestSessionCriteria(), 1, @@ -77,17 +78,17 @@ public void TakeProxyShouldSucceedIfMatchingCriteriaAreCorrect() Assert.IsNotNull(TestSessionPool.Instance); // Take proxy fails because test session is invalid. - Assert.IsNull(TestSessionPool.Instance.TryTakeProxy(new TestSessionInfo(), string.Empty, string.Empty)); + Assert.IsNull(TestSessionPool.Instance.TryTakeProxy(new TestSessionInfo(), string.Empty, string.Empty, mockRequestData.Object)); mockProxyTestSessionManager.Verify(tsm => tsm.DequeueProxy(It.IsAny(), It.IsAny()), Times.Never); Assert.IsTrue(TestSessionPool.Instance.AddSession(testSessionInfo, mockProxyTestSessionManager.Object)); // First TakeProxy fails because of throwing, see setup sequence. - Assert.IsNull(TestSessionPool.Instance.TryTakeProxy(testSessionInfo, string.Empty, string.Empty)); + Assert.IsNull(TestSessionPool.Instance.TryTakeProxy(testSessionInfo, string.Empty, string.Empty, mockRequestData.Object)); mockProxyTestSessionManager.Verify(tsm => tsm.DequeueProxy(It.IsAny(), It.IsAny()), Times.Once); // Second TakeProxy succeeds, see setup sequence. - Assert.IsNotNull(TestSessionPool.Instance.TryTakeProxy(testSessionInfo, string.Empty, string.Empty)); + Assert.IsNotNull(TestSessionPool.Instance.TryTakeProxy(testSessionInfo, string.Empty, string.Empty, mockRequestData.Object)); mockProxyTestSessionManager.Verify(tsm => tsm.DequeueProxy(It.IsAny(), It.IsAny()), Times.Exactly(2)); } From a2de4d0ed5671fa90d2b269e563d9ff6225811d4 Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 25 May 2022 13:27:21 +0200 Subject: [PATCH 2/5] Fixed tests --- .../Discovery/DiscoveryManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs index 93fc54fea3..34ae68976e 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs @@ -74,7 +74,7 @@ protected DiscoveryManager(IRequestData requestData, ITestPlatformEventSource te public void Initialize(IEnumerable pathToAdditionalExtensions, ITestDiscoveryEventsHandler2 eventHandler) { // Clear the request data metrics left over from a potential previous run. - _requestData.MetricsCollection.Metrics.Clear(); + _requestData.MetricsCollection?.Metrics?.Clear(); _testPlatformEventSource.AdapterSearchStart(); _testDiscoveryEventsHandler = eventHandler; From 889b4e89132edc62f061a5862663f131ef949754 Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 25 May 2022 14:34:36 +0200 Subject: [PATCH 3/5] Refactored telemetry status extraction --- src/testhost.x86/DefaultEngineInvoker.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/testhost.x86/DefaultEngineInvoker.cs b/src/testhost.x86/DefaultEngineInvoker.cs index dc1a1d1f47..1c1e666c0a 100644 --- a/src/testhost.x86/DefaultEngineInvoker.cs +++ b/src/testhost.x86/DefaultEngineInvoker.cs @@ -9,8 +9,6 @@ using System.Threading; using System.Threading.Tasks; -using Microsoft.VisualStudio.TestPlatform.Common; -using Microsoft.VisualStudio.TestPlatform.Common.Telemetry; using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities; using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces; using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Helpers; @@ -162,8 +160,7 @@ private static bool GetTelemetryStatusFromArgs(IDictionary args // version of vstest.console. var telemetryStatus = CommandLineArgumentsHelper.GetStringArgFromDict(argsDictionary, TelemetryOptedIn); - return !string.IsNullOrWhiteSpace(telemetryStatus) - && telemetryStatus.Equals("true", StringComparison.Ordinal); + return string.Equals(telemetryStatus, "true", StringComparison.Ordinal); } private void ConnectToDatacollector(int dcPort) From 9dd5cb3c7b90375f4203b7ee61bee6529a02c83a Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Thu, 26 May 2022 16:21:41 +0200 Subject: [PATCH 4/5] Added tests for request data clearing --- .../Discovery/DiscoveryManager.cs | 2 +- .../Execution/ExecutionManager.cs | 2 +- .../TestSession/TestSessionPool.cs | 4 ++- .../Discovery/DiscoveryManagerTests.cs | 28 +++++++++++++++- .../Execution/ExecutionManagerTests.cs | 33 +++++++++++++++++++ 5 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs index 34ae68976e..81bf36b045 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs @@ -45,7 +45,7 @@ public class DiscoveryManager : IDiscoveryManager /// /// Initializes a new instance of the class. /// - public DiscoveryManager(IRequestData requestData) + public DiscoveryManager(IRequestData requestData!!) : this(requestData, TestPlatformEventSource.Instance) { } diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs index fa41f119ec..569a9c0c7c 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs @@ -40,7 +40,7 @@ public class ExecutionManager : IExecutionManager /// /// Initializes a new instance of the class. /// - public ExecutionManager(IRequestData requestData) : this(TestPlatformEventSource.Instance, requestData) + public ExecutionManager(IRequestData requestData!!) : this(TestPlatformEventSource.Instance, requestData) { _sessionMessageLogger = TestSessionMessageLogger.Instance; _sessionMessageLogger.TestRunMessage += TestSessionMessageHandler; diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs index 6bc4f224a5..45fbe3b001 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs @@ -152,7 +152,9 @@ public virtual ProxyOperationManager TryTakeProxy( var proxy = sessionManager.DequeueProxy(source, runSettings); // Make sure we use the per-request request data instead of the request data used when - // creating the test session. + // creating the test session. Otherwise we can end up having irrelevant telemetry for + // the current request being fulfilled or even duplicate telemetry which may cause an + // exception to be thrown. proxy.RequestData = requestData; return proxy; diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Discovery/DiscoveryManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Discovery/DiscoveryManagerTests.cs index 1235622ebd..92d9b539fe 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Discovery/DiscoveryManagerTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Discovery/DiscoveryManagerTests.cs @@ -19,6 +19,7 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using FluentAssertions; using CrossPlatEngineResources = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Resources.Resources; @@ -67,6 +68,32 @@ public void InitializeShouldUpdateAdditionalExtenions() Assert.IsTrue(allDiscoverers.Any()); } + [TestMethod] + public void InitializeShouldClearMetricsCollection() + { + var metricsCollection = new MetricsCollection(); + + metricsCollection.Add("metric", "value"); + _mockRequestData.Setup(rd => rd.MetricsCollection).Returns(metricsCollection); + _mockRequestData.Setup(rd => rd.IsTelemetryOptedIn).Returns(true); + + var discoveryManager = new DiscoveryManager(_mockRequestData.Object); + + metricsCollection.Metrics.Should().ContainKey("metric"); + discoveryManager.Initialize(null, new Mock().Object); + metricsCollection.Metrics.Should().BeEmpty(); + } + + [TestMethod] + public void InitializeShouldNotFailIfMetricsFieldIsNull() + { + _mockRequestData.Object.MetricsCollection.Metrics.Should().BeNull(); + + var action = () => (new DiscoveryManager(_mockRequestData.Object)) + .Initialize(null, new Mock().Object); + + action.Should().NotThrow(); + } #endregion #region DiscoverTests tests @@ -287,6 +314,5 @@ public void DiscoveryTestsShouldSendAbortValuesCorrectlyIfAbortionHappened() Assert.AreEqual(true, receivedDiscoveryCompleteEventArgs!.IsAborted); Assert.AreEqual(-1, receivedDiscoveryCompleteEventArgs.TotalCount); } - #endregion } diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/ExecutionManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/ExecutionManagerTests.cs index ec6a9e271d..cc85007d0b 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/ExecutionManagerTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/ExecutionManagerTests.cs @@ -23,6 +23,7 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using FluentAssertions; using static TestPlatform.CrossPlatEngine.UnitTests.Execution.RunTestsWithSourcesTests; @@ -105,6 +106,38 @@ public void InitializeShouldLoadAndInitializeAllExtensions() } } + [TestMethod] + public void InitializeShouldClearMetricsCollection() + { + var metricsCollection = new MetricsCollection(); + + metricsCollection.Add("metric", "value"); + _mockRequestData.Setup(rd => rd.MetricsCollection).Returns(metricsCollection); + _mockRequestData.Setup(rd => rd.IsTelemetryOptedIn).Returns(true); + + var discoveryManager = new ExecutionManager(_mockRequestData.Object); + + metricsCollection.Metrics.Should().ContainKey("metric"); + discoveryManager.Initialize(null, new Mock().Object); + metricsCollection.Metrics.Should().BeEmpty(); + } + + [TestMethod] + public void InitializeShouldNotFailIfMetricsFieldIsNull() + { + var mockRequestData = new Mock(); + var mockMetricsCollection = new Mock(); + + mockRequestData.Setup(rd => rd.MetricsCollection).Returns(mockMetricsCollection.Object); + + mockRequestData.Object.MetricsCollection.Metrics.Should().BeNull(); + + var action = () => (new ExecutionManager(mockRequestData.Object)) + .Initialize(null, new Mock().Object); + + action.Should().NotThrow(); + } + [TestMethod] public void StartTestRunShouldRunTestsInTheProvidedSources() { From 0260725668965ad10838c0dd44f88f4d871cfede Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Thu, 26 May 2022 17:49:48 +0200 Subject: [PATCH 5/5] Require non-null request data from protected ctors --- .../Discovery/DiscoveryManager.cs | 2 +- .../Execution/ExecutionManager.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs index 81bf36b045..3e697a4e62 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs @@ -59,7 +59,7 @@ public DiscoveryManager(IRequestData requestData!!) /// /// The test platform event source. /// - protected DiscoveryManager(IRequestData requestData, ITestPlatformEventSource testPlatformEventSource) + protected DiscoveryManager(IRequestData requestData!!, ITestPlatformEventSource testPlatformEventSource) { _sessionMessageLogger = TestSessionMessageLogger.Instance; _sessionMessageLogger.TestRunMessage += TestSessionMessageHandler; diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs index 569a9c0c7c..c0e674c91c 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs @@ -50,7 +50,7 @@ public ExecutionManager(IRequestData requestData!!) : this(TestPlatformEventSour /// Initializes a new instance of the class. /// /// Test platform event source. - protected ExecutionManager(ITestPlatformEventSource testPlatformEventSource, IRequestData requestData) + protected ExecutionManager(ITestPlatformEventSource testPlatformEventSource, IRequestData requestData!!) { _testPlatformEventSource = testPlatformEventSource; _requestData = requestData;