diff --git a/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyDiscoveryManager.cs b/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyDiscoveryManager.cs index 412ba6818f..ac4e79e47c 100644 --- a/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyDiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyDiscoveryManager.cs @@ -16,6 +16,15 @@ public interface IProxyDiscoveryManager /// void Initialize(bool skipDefaultAdapters); + /// + /// Initializes test discovery. Create the test host, setup channel and initialize extensions. + /// + /// + /// Settings, parameters for the discovery request + /// EventHandler for handling discovery events from Engine + /// Skip default adapters flag + void InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, bool skipDefaultAdapters); + /// /// Discovers tests /// diff --git a/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyExecutionManager.cs b/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyExecutionManager.cs index 66857ec088..903e1a27ac 100644 --- a/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyExecutionManager.cs +++ b/src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/IProxyExecutionManager.cs @@ -21,6 +21,14 @@ public interface IProxyExecutionManager /// void Initialize(bool skipDefaultAdapters); + /// + /// Initializes test execution. Create the test host, setup channel and initialize extensions. + /// + /// + /// The settings/options for the test run. + /// EventHandler for handling execution events from Engine. + void InitializeTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler); + /// /// Starts the test run. /// diff --git a/src/Microsoft.TestPlatform.Common/PublicAPI/PublicAPI.Shipped.txt b/src/Microsoft.TestPlatform.Common/PublicAPI/PublicAPI.Shipped.txt index b3be22736e..b0f38dbb28 100644 --- a/src/Microsoft.TestPlatform.Common/PublicAPI/PublicAPI.Shipped.txt +++ b/src/Microsoft.TestPlatform.Common/PublicAPI/PublicAPI.Shipped.txt @@ -263,3 +263,5 @@ virtual Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework.TestPlugin virtual Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework.Utilities.TestPluginInformation.IdentifierData.get -> string? virtual Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework.Utilities.TestPluginInformation.Metadata.get -> System.Collections.Generic.ICollection! virtual Microsoft.VisualStudio.TestPlatform.Common.Hosting.TestRuntimeProviderManager.GetTestHostManagerByRunConfiguration(string? runConfiguration, System.Collections.Generic.List? _) -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Host.ITestRuntimeProvider? +Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.IProxyDiscoveryManager.InitializeDiscovery(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.DiscoveryCriteria! discoveryCriteria, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.ITestDiscoveryEventsHandler2! eventHandler, bool skipDefaultAdapters) -> void +Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.IProxyExecutionManager.InitializeTestRun(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestRunCriteria! testRunCriteria, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IInternalTestRunEventsHandler! eventHandler) -> void diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyDiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyDiscoveryManager.cs index b61c129ce9..a416e5a2f6 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyDiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyDiscoveryManager.cs @@ -113,4 +113,12 @@ private void InitializeExtensions(IEnumerable sources) // it will use TestPluginCache of vstest.console _discoveryManager.Initialize(Enumerable.Empty(), null); } + + public void InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, bool skipDefaultAdapters) + { + // Leaving this empty as it is not really relevant to the in-process proxy managers since + // there's no external testhost to be started. The idea of pre-initializing the test run + // makes sense only for out-of-process proxies like ProxyExecutionManager or + // ProxyDiscoveryManager. + } } diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyexecutionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyexecutionManager.cs index de7e5d2a41..27def5dde1 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyexecutionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/InProcessProxyexecutionManager.cs @@ -133,7 +133,6 @@ public void Close() { } - private void InitializeExtensions(IEnumerable sources) { var extensionsFromSource = _testHostManager.GetTestPlatformExtensions(sources, Enumerable.Empty()); @@ -146,4 +145,12 @@ private void InitializeExtensions(IEnumerable sources) // it will use TestPluginCache of vstest.console _executionManager.Initialize(Enumerable.Empty(), null); } + + public void InitializeTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) + { + // Leaving this empty as it is not really relevant to the in-process proxy managers since + // there's no external testhost to be started. The idea of pre-initializing the test run + // makes sense only for out-of-process proxies like ProxyExecutionManager or + // ProxyDiscoveryManager. + } } diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelOperationManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelOperationManager.cs index ab73b23f53..ff8e1058a7 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelOperationManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelOperationManager.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Linq; using System.Threading.Tasks; @@ -16,6 +17,13 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client; /// internal sealed class ParallelOperationManager : IDisposable { + private const int PreStart = 2; + private readonly static int VSTEST_HOSTPRESTART_COUNT = + int.TryParse( + Environment.GetEnvironmentVariable(nameof(VSTEST_HOSTPRESTART_COUNT)), + out int num) + ? num + : PreStart; private readonly Func _createNewManager; /// @@ -23,7 +31,8 @@ internal sealed class ParallelOperationManager private TEventHandler? _eventHandler; private Func? _getEventHandler; - private Action? _runWorkload; + private Func? _initializeWorkload; + private Action? _runWorkload; private bool _acceptMoreWork; private readonly List> _workloads = new(); private readonly List _managerSlots = new(); @@ -33,6 +42,7 @@ internal sealed class ParallelOperationManager /// Creates new instance of ParallelOperationManager. @@ -44,6 +54,10 @@ public ParallelOperationManager(Func createNe { _createNewManager = createNewManager; MaxParallelLevel = parallelLevel; + // pre-start only when we don't run in parallel, if we do run in parallel, + // then pre-starting has no additional value because while one host is starting, + // another is running tests. + PreStartCount = MaxParallelLevel == 1 ? VSTEST_HOSTPRESTART_COUNT : 0; ClearSlots(acceptMoreWork: true); } @@ -53,14 +67,14 @@ private void ClearSlots(bool acceptMoreWork) { _acceptMoreWork = acceptMoreWork; _managerSlots.Clear(); - _managerSlots.AddRange(Enumerable.Range(0, MaxParallelLevel).Select(_ => new Slot())); + _managerSlots.AddRange(Enumerable.Range(0, MaxParallelLevel + PreStartCount).Select(i => new Slot { Index = i })); SetOccupiedSlotCount(); } } private void SetOccupiedSlotCount() { - AvailableSlotCount = _managerSlots.Count(s => s.IsAvailable); + AvailableSlotCount = _managerSlots.Count(s => !s.HasWork); OccupiedSlotCount = _managerSlots.Count - AvailableSlotCount; } @@ -68,18 +82,17 @@ private void SetOccupiedSlotCount() List> workloads, TEventHandler eventHandler, Func getEventHandler, - Action runWorkload) + Func initializeWorkload, + Action runWorkload) { _ = workloads ?? throw new ArgumentNullException(nameof(workloads)); _eventHandler = eventHandler ?? throw new ArgumentNullException(nameof(eventHandler)); _getEventHandler = getEventHandler ?? throw new ArgumentNullException(nameof(getEventHandler)); + _initializeWorkload = initializeWorkload ?? throw new ArgumentNullException(nameof(initializeWorkload)); _runWorkload = runWorkload ?? throw new ArgumentNullException(nameof(runWorkload)); _workloads.AddRange(workloads); - // This creates as many slots as possible even though we might not use them when we get less workloads to process, - // this is not a big issue, and not worth optimizing, because the parallel level is determined by the logical CPU count, - // so it is a small number. ClearSlots(acceptMoreWork: true); RunWorkInParallel(); } @@ -101,14 +114,10 @@ private bool RunWorkInParallel() if (_runWorkload == null) throw new InvalidOperationException($"{nameof(_runWorkload)} was not provided."); - // Reserve slots and assign them work under the lock so we keep - // the slots consistent. - List workToRun = new(); + // Reserve slots and assign them work under the lock so we keep the slots consistent. + Slot[] slots; lock (_lock) { - if (_workloads.Count == 0) - return false; - // When HandlePartialDiscovery or HandlePartialRun are in progress, and we call StopAllManagers, // it is possible that we will clear all slots, and have RunWorkInParallel waiting on the lock, // so when it is allowed to enter it will try to add more work, but we already cancelled, @@ -116,51 +125,104 @@ private bool RunWorkInParallel() if (!_acceptMoreWork) return false; - var availableSlots = _managerSlots.Where(slot => slot.IsAvailable).ToList(); - var availableWorkloads = _workloads.Where(workload => workload != null).ToList(); - var amount = Math.Min(availableSlots.Count, availableWorkloads.Count); - var workloadsToRun = availableWorkloads.Take(amount).ToList(); - + // We grab all empty slots. + var availableSlots = _managerSlots.Where(slot => !slot.HasWork).ToImmutableArray(); + var occupiedSlots = MaxParallelLevel - (availableSlots.Length - PreStartCount); + // We grab all available workloads. + var availableWorkloads = _workloads.Where(workload => workload != null).ToImmutableArray(); + // We take the amount of workloads to fill all the slots, or just as many workloads + // as there are if there are less workloads than slots. + var amount = Math.Min(availableSlots.Length, availableWorkloads.Length); + var workloadsToAdd = availableWorkloads.Take(amount).ToImmutableArray(); + + // We associate each workload to a slot, if we reached the max parallel + // level, then we will run only initalize step of the given workload. for (int i = 0; i < amount; i++) { var slot = availableSlots[i]; - slot.IsAvailable = false; - var workload = workloadsToRun[i]; - workToRun.Add(new SlotWorkloadPair(slot, workload)); + slot.HasWork = true; + var workload = workloadsToAdd[i]; + slot.ShouldPreStart = occupiedSlots + i + 1 > MaxParallelLevel; + + var manager = _createNewManager(workload.Provider); + var eventHandler = _getEventHandler(_eventHandler, manager); + slot.EventHandler = eventHandler; + slot.Manager = manager; + slot.ManagerInfo = workload.Provider; + slot.Work = workload.Work; + _workloads.Remove(workload); } + slots = _managerSlots.ToArray(); SetOccupiedSlotCount(); - - foreach (var pair in workToRun) - { - var manager = _createNewManager(pair.Workload.Provider); - var eventHandler = _getEventHandler(_eventHandler, manager); - pair.Slot.EventHandler = eventHandler; - pair.Slot.Manager = manager; - pair.Slot.ManagerInfo = pair.Workload.Provider; - pair.Slot.Work = pair.Workload.Work; - } } // Kick of the work in parallel outside of the lock so if we have more requests to run // that come in at the same time we only block them from reserving the same slot at the same time // but not from starting their assigned work at the same time. - foreach (var pair in workToRun) + + // Kick of all pre-started hosts from the ones that had the longest time to initialize. + // + // This code should be safe even outside the lock since HasWork is only changed when work is + // complete and only for the slot that completed work. It is not possible to complete work before + // starting it (which is what we are trying to do here). + var startedWork = 0; + foreach (var slot in slots.Where(s => s.HasWork && !s.IsRunning && s.IsPreStarted).OrderBy(s => s.PreStartTime)) + { + startedWork++; + slot.IsRunning = true; + EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: Running on pre-started host: {(DateTime.Now.TimeOfDay - slot.PreStartTime).TotalMilliseconds}ms {slot.InitTask?.Status}"); + _runWorkload(slot.Manager!, slot.EventHandler!, slot.Work!, slot.IsPreStarted, slot.InitTask); + + // We already started as many as we were allowed, jump out; + if (startedWork == MaxParallelLevel) + { + break; + } + } + + // We already started as many pre-started testhosts as we are allowed by the max parallel level + // skip running more work. + if (startedWork < MaxParallelLevel) { - try + foreach (var slot in slots) { - _runWorkload(pair.Slot.Manager!, pair.Slot.EventHandler!, pair.Workload.Work!); + if (slot.HasWork && !slot.IsRunning) + { + if (!slot.ShouldPreStart) + { + startedWork++; + slot.IsRunning = true; + EqtTrace.Verbose("ParallelOperationManager.RunWorkInParallel: Started work on a host."); + _runWorkload(slot.Manager!, slot.EventHandler!, slot.Work!, slot.IsPreStarted, slot.InitTask); + } + } + + // We already started as many as we were allowed, jump out; + if (startedWork == MaxParallelLevel) + { + break; + } } - finally + } + + var preStartedWork = 0; + foreach (var slot in slots) + { + if (slot.HasWork && slot.ShouldPreStart && !slot.IsPreStarted) { - // clean the slot or something, to make sure we don't keep it reserved. + preStartedWork++; + slot.PreStartTime = DateTime.Now.TimeOfDay; + slot.IsPreStarted = true; + EqtTrace.Verbose("ParallelOperationManager.RunWorkInParallel: Pre-starting a host."); + slot.InitTask = _initializeWorkload!(slot.Manager!, slot.EventHandler!, slot.Work!); } } // Return true when we started more work. Or false, when there was nothing more to do. // This will propagate to handling of partial discovery or partial run. - return workToRun.Count > 0; + return preStartedWork + startedWork > 0; } public bool RunNextWork(TManager completedManager) @@ -174,12 +236,12 @@ private void ClearCompletedSlot(TManager completedManager) { lock (_lock) { - var completedSlot = _managerSlots.Where(s => ReferenceEquals(completedManager, s.Manager)).ToList(); + var completedSlot = _managerSlots.Where(s => ReferenceEquals(completedManager, s.Manager)).ToImmutableArray(); // When HandlePartialDiscovery or HandlePartialRun are in progress, and we call StopAllManagers, // it is possible that we will clear all slots, while ClearCompletedSlot is waiting on the lock, // so when it is allowed to enter it will fail to find the respective slot and fail. In this case it is // okay that the slot is not found, and we do nothing, because we already stopped all work and cleared the slots. - if (completedSlot.Count == 0) + if (completedSlot.Length == 0) { if (_acceptMoreWork) { @@ -191,13 +253,21 @@ private void ClearCompletedSlot(TManager completedManager) } } - if (completedSlot.Count > 1) + if (completedSlot.Length > 1) { throw new InvalidOperationException("The provided manager was found in multiple slots."); } var slot = completedSlot[0]; - slot.IsAvailable = true; + slot.PreStartTime = TimeSpan.Zero; + slot.Work = default(TWorkload); + slot.HasWork = false; + slot.ShouldPreStart = false; + slot.IsPreStarted = false; + slot.InitTask = null; + slot.IsRunning = false; + slot.Manager = default(TManager); + slot.EventHandler = default(TEventHandler); SetOccupiedSlotCount(); } @@ -207,9 +277,9 @@ public void DoActionOnAllManagers(Action action, bool doActionsInParal { // We don't need to lock here, we just grab the current list of // slots that are occupied (have managers) and run action on each one of them. - var managers = _managerSlots.Where(slot => !slot.IsAvailable).Select(slot => slot.Manager).ToList(); + var managers = _managerSlots.Where(slot => slot.HasWork).Select(slot => slot.Manager).ToImmutableArray(); int i = 0; - var actionTasks = new Task[managers.Count]; + var actionTasks = new Task[managers.Length]; foreach (var manager in managers) { if (manager == null) @@ -260,7 +330,14 @@ public void Dispose() private class Slot { - public bool IsAvailable { get; set; } = true; + public int Index { get; set; } + public bool HasWork { get; set; } + + public bool ShouldPreStart { get; set; } + + public Task? InitTask { get; set; } + + public bool IsRunning { get; set; } public TManager? Manager { get; set; } @@ -269,16 +346,12 @@ private class Slot public TEventHandler? EventHandler { get; set; } public TWorkload? Work { get; set; } - } + public bool IsPreStarted { get; internal set; } + public TimeSpan PreStartTime { get; internal set; } - private class SlotWorkloadPair - { - public SlotWorkloadPair(Slot slot, ProviderSpecificWorkload workload) + public override string ToString() { - Slot = slot; - Workload = workload; + return $"{Index}: HasWork: {HasWork}, ShouldPreStart: {ShouldPreStart}, IsPreStarted: {IsPreStarted}, IsRunning: {IsRunning}"; } - public Slot Slot { get; } - public ProviderSpecificWorkload Workload { get; } } } diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs index f175c16789..bb8d1ac0f2 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs @@ -99,17 +99,7 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve // marked as NotDiscovered. _dataAggregator.MarkSourcesWithStatus(discoveryCriteria.Sources, DiscoveryStatus.NotDiscovered); - if (nonRunnableWorkloads.Count > 0) - { - // We found some sources that don't associate to any runtime provider and so they cannot run. - // Mark the sources as skipped. - - _dataAggregator.MarkSourcesWithStatus(nonRunnableWorkloads.SelectMany(w => w.Work.Sources), DiscoveryStatus.SkippedDiscovery); - // TODO: in strict mode keep them as non-discovered, and mark the run as aborted. - // _dataAggregator.MarkAsAborted(); - } - - _parallelOperationManager.StartWork(runnableWorkloads, eventHandler, GetParallelEventHandler, DiscoverTestsOnConcurrentManager); + _parallelOperationManager.StartWork(workloads, eventHandler, GetParallelEventHandler, InitializeDiscoverTestsOnConcurrentManager, DiscoverTestsOnConcurrentManager); } private ITestDiscoveryEventsHandler2 GetParallelEventHandler(ITestDiscoveryEventsHandler2 eventHandler, IProxyDiscoveryManager concurrentManager) @@ -261,43 +251,84 @@ static DiscoveryCriteria NewDiscoveryCriteriaFromSourceAndSettings(IEnumerable /// Proxy discovery manager instance. - private void DiscoverTestsOnConcurrentManager(IProxyDiscoveryManager proxyDiscoveryManager, ITestDiscoveryEventsHandler2 eventHandler, DiscoveryCriteria discoveryCriteria) + private Task InitializeDiscoverTestsOnConcurrentManager(IProxyDiscoveryManager proxyDiscoveryManager, ITestDiscoveryEventsHandler2 eventHandler, DiscoveryCriteria discoveryCriteria) + { + // Kick off another discovery task for the next source + return Task.Run(() => + { + EqtTrace.Verbose("ProxyParallelDiscoveryManager.InitializeDiscoverTestsOnConcurrentManager: Discovery preparation started."); + + proxyDiscoveryManager.Initialize(_skipDefaultAdapters); + proxyDiscoveryManager.InitializeDiscovery(discoveryCriteria, eventHandler, _skipDefaultAdapters); + + EqtTrace.Verbose($"ProxyParallelDiscoveryManager.InitializeDiscoverTestsOnConcurrentManager: Init only: {string.Join(", ", discoveryCriteria.Sources)}"); + }); + } + + /// + /// Triggers the discovery for the next data object on the concurrent discoverer + /// Each concurrent discoverer calls this method, once its completed working on previous data + /// + /// Proxy discovery manager instance. + private void DiscoverTestsOnConcurrentManager( + IProxyDiscoveryManager proxyDiscoveryManager, + ITestDiscoveryEventsHandler2 eventHandler, + DiscoveryCriteria discoveryCriteria, + bool initialized, + Task? task) { // Kick off another discovery task for the next source Task.Run(() => { EqtTrace.Verbose("ParallelProxyDiscoveryManager: Discovery started."); + if (!initialized) + { + EqtTrace.Verbose($"ProxyParallelDiscoveryManager.DiscoverTestsOnConcurrentManager: Initialize right before run: {string.Join(", ", discoveryCriteria.Sources)}"); + proxyDiscoveryManager.Initialize(_skipDefaultAdapters); + proxyDiscoveryManager.InitializeDiscovery(discoveryCriteria, eventHandler, _skipDefaultAdapters); + } + else + { + task?.Wait(); + } - proxyDiscoveryManager.Initialize(_skipDefaultAdapters); + EqtTrace.Verbose($"ProxyParallelDiscoveryManager.DiscoverTestsOnConcurrentManager: Run: {string.Join(", ", discoveryCriteria.Sources)}"); proxyDiscoveryManager.DiscoverTests(discoveryCriteria, eventHandler); - }) - .ContinueWith(t => - { - // Just in case, the actual discovery couldn't start for an instance. Ensure that - // we call discovery complete since we have already fetched a source. Otherwise - // discovery will not terminate - EqtTrace.Error("ParallelProxyDiscoveryManager: Failed to trigger discovery. Exception: " + t.Exception); - - var handler = eventHandler; - var exceptionToString = t.Exception?.ToString(); - var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = exceptionToString }; - handler.HandleRawMessage(_dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload)); - handler.HandleLogMessage(TestMessageLevel.Error, exceptionToString); - - // Send discovery complete. Similar logic is also used in ProxyDiscoveryManager.DiscoverTests. - // Differences: - // Total tests must be zero here since parallel discovery events handler adds the count - // Keep `lastChunk` as null since we don't want a message back to the IDE (discovery didn't even begin) - // Set `isAborted` as true since we want this instance of discovery manager to be replaced - // TODO: the comment above mentions 0 tests but sends -1. Make sense of this. - var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true); - handler.HandleDiscoveryComplete(discoveryCompleteEventsArgs, null); - }, - TaskContinuationOptions.OnlyOnFaulted); + }).ContinueWith(t => HandleError(eventHandler, t), TaskContinuationOptions.OnlyOnFaulted); EqtTrace.Verbose("ProxyParallelDiscoveryManager.DiscoverTestsOnConcurrentManager: No sources available for discovery."); } + private void HandleError(ITestDiscoveryEventsHandler2 eventHandler, Task t) + { + // Just in case, the actual discovery couldn't start for an instance. Ensure that + // we call discovery complete since we have already fetched a source. Otherwise + // discovery will not terminate + EqtTrace.Error("ParallelProxyDiscoveryManager: Failed to trigger discovery. Exception: " + t.Exception); + + var handler = eventHandler; + var exceptionString = t.Exception?.ToString(); + var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = exceptionString }; + handler.HandleRawMessage(_dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload)); + handler.HandleLogMessage(TestMessageLevel.Error, exceptionString); + + // Send discovery complete. Similar logic is also used in ProxyDiscoveryManager.DiscoverTests. + // Differences: + // Total tests must be zero here since parallel discovery events handler adds the count + // Keep `lastChunk` as null since we don't want a message back to the IDE (discovery didn't even begin) + // Set `isAborted` as true since we want this instance of discovery manager to be replaced + // TODO: the comment above mentions 0 tests but sends -1. Make sense of this. + var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true); + handler.HandleDiscoveryComplete(discoveryCompleteEventsArgs, null); + } + + public void InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, bool skipDefaultAdapters) + { + // Leaving this empty as it is not really relevant to the parallel proxy managers. + // The idea of pre-initializing the test run makes sense only for single proxies like + // ProxyExecutionManager or ProxyDiscoveryManager. + } + public void Dispose() { if (!_isDisposed) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs index 6c75d587c4..792ddef3ce 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs @@ -115,7 +115,7 @@ public int StartTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsH // _currentRunDataAggregator.MarkAsAborted(); } - _parallelOperationManager.StartWork(runnableWorkloads, eventHandler, GetParallelEventHandler, StartTestRunOnConcurrentManager); + _parallelOperationManager.StartWork(workloads, eventHandler, GetParallelEventHandler, PrepareTestRunOnConcurrentManager, StartTestRunOnConcurrentManager); // Why 1? Because this is supposed to be a processId, and that is just the default that was chosen by someone before me, // and maybe is checked somewhere, but I don't see it checked in our codebase. @@ -388,24 +388,58 @@ private ParallelRunEventsHandler GetParallelEventHandler(IInternalTestRunEventsH _currentRunDataAggregator); } + private Task PrepareTestRunOnConcurrentManager(IProxyExecutionManager proxyExecutionManager, IInternalTestRunEventsHandler eventHandler, TestRunCriteria testRunCriteria) + { + return Task.Run(() => + { + if (!proxyExecutionManager.IsInitialized) + { + proxyExecutionManager.Initialize(_skipDefaultAdapters); + } + + // NOTE: No need to increment the number of started clients on initialization since the + // client doesn't really count as started unless some work is done on it. Incrementing + // the number of clients will result in failing acceptance tests because they expect all + // clients to be done running their workloads when aborting/cancelling and that doesn't + // happen with an initialized workload that is never run. + // + // Interlocked.Increment(ref _runStartedClients); + proxyExecutionManager.InitializeTestRun(testRunCriteria, eventHandler); + }); + } + /// /// Triggers the execution for the next data object on the concurrent executor /// Each concurrent executor calls this method, once its completed working on previous data /// /// Proxy execution manager instance. /// True, if execution triggered - private void StartTestRunOnConcurrentManager(IProxyExecutionManager proxyExecutionManager, IInternalTestRunEventsHandler eventHandler, TestRunCriteria testRunCriteria) + private void StartTestRunOnConcurrentManager( + IProxyExecutionManager proxyExecutionManager, + IInternalTestRunEventsHandler eventHandler, + TestRunCriteria testRunCriteria, + bool initialized, + Task? initTask) { if (testRunCriteria != null) { - if (!proxyExecutionManager.IsInitialized) - { - proxyExecutionManager.Initialize(_skipDefaultAdapters); - } - Task.Run(() => { - Interlocked.Increment(ref _runStartedClients); + if (!initialized) + { + if (!proxyExecutionManager.IsInitialized) + { + proxyExecutionManager.Initialize(_skipDefaultAdapters); + } + + Interlocked.Increment(ref _runStartedClients); + proxyExecutionManager.InitializeTestRun(testRunCriteria, eventHandler); + } + else + { + initTask!.Wait(); + } + EqtTrace.Verbose("ParallelProxyExecutionManager: Execution started. Started clients: " + _runStartedClients); proxyExecutionManager.StartTestRun(testRunCriteria, eventHandler); @@ -437,6 +471,13 @@ private void StartTestRunOnConcurrentManager(IProxyExecutionManager proxyExecuti EqtTrace.Verbose("ProxyParallelExecutionManager: No sources available for execution."); } + public void InitializeTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) + { + // Leaving this empty as it is not really relevant to the parallel proxy managers. + // The idea of pre-initializing the test run makes sense only for single proxies like + // ProxyExecutionManager or ProxyDiscoveryManager. + } + public void Dispose() { if (!_isDisposed) diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs index 1df0a6032f..caa2643b24 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs @@ -117,14 +117,13 @@ public class ProxyDiscoveryManager : IProxyDiscoveryManager, IBaseProxy, ITestDi #region IProxyDiscoveryManager implementation. - /// public void Initialize(bool skipDefaultAdapters) { _skipDefaultAdapters = skipDefaultAdapters; } /// - public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler) + public void InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, bool skipDefaultAdapters) { // Multiple method calls will iterate over this sources collection so we want to ensure // it's built once. @@ -151,9 +150,33 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve if (_isCommunicationEstablished) { - InitializeExtensions(discoverySources); - TPDebug.Assert(_testHostManager is not null, "_testHostManager is null"); - discoveryCriteria.UpdateDiscoveryCriteria(_testHostManager); + InitializeExtensions(discoverySources, skipDefaultAdapters); + } + } + catch (Exception exception) + { + HandleException(exception); + } + } + + public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler) + { + // Multiple method calls will iterate over this sources collection so we want to ensure + // it's built once. + var discoverySources = discoveryCriteria.Sources.ToArray(); + + try + { + if (!_isCommunicationEstablished) + { + InitializeDiscovery(discoveryCriteria, eventHandler, _skipDefaultAdapters); + } + + TPDebug.Assert(_proxyOperationManager is not null, "ProxyOperationManager is null."); + + if (_isCommunicationEstablished) + { + discoveryCriteria.UpdateDiscoveryCriteria(_testHostManager!); // Consider the first source as the previous source so that if we are discovering a source // with no tests, we will always consider the source as fully discovered when reaching the @@ -165,31 +188,36 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve } catch (Exception exception) { - EqtTrace.Error("ProxyDiscoveryManager.DiscoverTests: Failed to discover tests: {0}", exception); + HandleException(exception); + } + } + + private void HandleException(Exception exception) + { + EqtTrace.Error("ProxyDiscoveryManager.DiscoverTests: Failed to discover tests: {0}", exception); - // Log to vs ide test output - var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = exception.ToString() }; - var rawMessage = _dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload); - HandleRawMessage(rawMessage); + // Log to vs ide test output + var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = exception.ToString() }; + var rawMessage = _dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload); + HandleRawMessage(rawMessage); - // Log to vstest.console - // Send a discovery complete to caller. Similar logic is also used in ParallelProxyDiscoveryManager.DiscoverTestsOnConcurrentManager - // Aborted is `true`: in case of parallel discovery (or non shared host), an aborted message ensures another discovery manager - // created to replace the current one. This will help if the current discovery manager is aborted due to irreparable error - // and the test host is lost as well. - HandleLogMessage(TestMessageLevel.Error, exception.ToString()); + // Log to vstest.console + // Send a discovery complete to caller. Similar logic is also used in ParallelProxyDiscoveryManager.DiscoverTestsOnConcurrentManager + // Aborted is `true`: in case of parallel discovery (or non shared host), an aborted message ensures another discovery manager + // created to replace the current one. This will help if the current discovery manager is aborted due to irreparable error + // and the test host is lost as well. + HandleLogMessage(TestMessageLevel.Error, exception.ToString()); - var discoveryCompletePayload = new DiscoveryCompletePayload() - { - IsAborted = true, - LastDiscoveredTests = null, - TotalTests = -1 - }; - HandleRawMessage(_dataSerializer.SerializePayload(MessageType.DiscoveryComplete, discoveryCompletePayload)); - var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true); - - HandleDiscoveryComplete(discoveryCompleteEventsArgs, new List()); - } + var discoveryCompletePayload = new DiscoveryCompletePayload + { + IsAborted = true, + LastDiscoveredTests = null, + TotalTests = -1 + }; + HandleRawMessage(_dataSerializer.SerializePayload(MessageType.DiscoveryComplete, discoveryCompletePayload)); + var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true); + + HandleDiscoveryComplete(discoveryCompleteEventsArgs, new List()); } /// @@ -317,9 +345,9 @@ public virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessStartI } #endregion - private void InitializeExtensions(IEnumerable sources) + private void InitializeExtensions(IEnumerable sources, bool skipDefaultAdapters) { - var extensions = TestPluginCache.Instance.GetExtensionPaths(TestPlatformConstants.TestAdapterEndsWithPattern, _skipDefaultAdapters); + var extensions = TestPluginCache.Instance.GetExtensionPaths(TestPlatformConstants.TestAdapterEndsWithPattern, skipDefaultAdapters); // Filter out non existing extensions var nonExistingExtensions = extensions.Where(extension => !_fileHelper.Exists(extension)); diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs index ed9df4fe67..9623975e60 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs @@ -155,8 +155,7 @@ public virtual void Initialize(bool skipDefaultAdapters) IsInitialized = true; } - /// - public virtual int StartTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) + public virtual void InitializeTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) { if (_proxyOperationManager == null) { @@ -195,6 +194,53 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, IInternalTestRu _proxyOperationManager.CancellationTokenSource.Token.ThrowTestPlatformExceptionIfCancellationRequested(); InitializeExtensions(_testSources); + } + } + catch (Exception ex) + { + HandleError(ex); + } + } + /// + public virtual int StartTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) + { + try + { + if (!_isCommunicationEstablished) + { + InitializeTestRun(testRunCriteria, eventHandler); + } + + // In certain scenarios (like the one for non-parallel dotnet runs) we may end up + // using the incorrect events handler which can have nasty side effects, like failing to + // properly terminate the communication with any data collector. The reason for this is + // that the initialization and the actual test run have been decoupled and are now two + // separate operations. In the initialization phase we already provide an events handler + // to be invoked when data flows back from the testhost, but the "correct" handler is + // only provided in the run phase which may occur later on. This was not a problem when + // initialization was part of the normal test run workflow. However, now that the two + // operations are separate and because initialization could've already taken place, the + // communication channel could be properly set up, which means that we don't get to + // overwrite the old events handler anymore. + // The solution to this is to make sure that we always use the most "up-to-date" + // handler, and that would be the handler we got as an argument when this method was + // called. When initialization and test run are part of the same operation the behavior + // is still correct, since the two events handler will be equal and there'll be no need + // for an overwrite. + if (eventHandler != _baseTestRunEventsHandler) + { + _baseTestRunEventsHandler = eventHandler; + } + + TPDebug.Assert(_proxyOperationManager is not null, "ProxyOperationManager is null."); + + if (_isCommunicationEstablished) + { + var testSources = new List( + testRunCriteria.HasSpecificSources + ? testRunCriteria.Sources + // If the test execution is with a test filter, group them by sources. + : testRunCriteria.Tests.GroupBy(tc => tc.Source).Select(g => g.Key)); // This code should be in sync with InProcessProxyExecutionManager.StartTestRun // execution context. @@ -242,36 +288,41 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, IInternalTestRu } catch (Exception exception) { - EqtTrace.Error("ProxyExecutionManager.StartTestRun: Failed to start test run: {0}", exception); - - // Log error message to design mode and CLI. - // TestPlatformException is expected exception, log only the message. - // For other exceptions, log the stacktrace as well. - var errorMessage = exception is TestPlatformException ? exception.Message : exception.ToString(); - var testMessagePayload = new TestMessagePayload() - { - MessageLevel = TestMessageLevel.Error, - Message = errorMessage - }; - HandleRawMessage(_dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload)); - LogMessage(TestMessageLevel.Error, errorMessage); - - // Send a run complete to caller. Similar logic is also used in - // ParallelProxyExecutionManager.StartTestRunOnConcurrentManager. - // - // Aborted is `true`: in case of parallel run (or non shared host), an aborted - // message ensures another execution manager created to replace the current one. - // This will help if the current execution manager is aborted due to irreparable - // error and the test host is lost as well. - var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection(), new Collection(), TimeSpan.Zero); - var testRunCompletePayload = new TestRunCompletePayload { TestRunCompleteArgs = completeArgs }; - HandleRawMessage(_dataSerializer.SerializePayload(MessageType.ExecutionComplete, testRunCompletePayload)); - HandleTestRunComplete(completeArgs, null, null, null); + HandleError(exception); } return 0; } + private void HandleError(Exception exception) + { + EqtTrace.Error("ProxyExecutionManager.StartTestRun: Failed to start test run: {0}", exception); + + // Log error message to design mode and CLI. + // TestPlatformException is expected exception, log only the message. + // For other exceptions, log the stacktrace as well. + var errorMessage = exception is TestPlatformException ? exception.Message : exception.ToString(); + var testMessagePayload = new TestMessagePayload + { + MessageLevel = TestMessageLevel.Error, + Message = errorMessage + }; + HandleRawMessage(_dataSerializer.SerializePayload(MessageType.TestMessage, testMessagePayload)); + LogMessage(TestMessageLevel.Error, errorMessage); + + // Send a run complete to caller. Similar logic is also used in + // ParallelProxyExecutionManager.StartTestRunOnConcurrentManager. + // + // Aborted is `true`: in case of parallel run (or non shared host), an aborted + // message ensures another execution manager created to replace the current one. + // This will help if the current execution manager is aborted due to irreparable + // error and the test host is lost as well. + var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection(), new Collection(), TimeSpan.Zero); + var testRunCompletePayload = new TestRunCompletePayload { TestRunCompleteArgs = completeArgs }; + HandleRawMessage(_dataSerializer.SerializePayload(MessageType.ExecutionComplete, testRunCompletePayload)); + HandleTestRunComplete(completeArgs, null, null, null); + } + /// public virtual void Cancel(IInternalTestRunEventsHandler eventHandler) { diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt b/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt index 7f4ee98236..49374b7453 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt @@ -137,3 +137,4 @@ virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.AddS virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.KillSession(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo! testSessionInfo, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData! requestData) -> 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.ObjectModel.Client.IRequestData! requestData) -> Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager? +Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.InitializeDiscovery(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.DiscoveryCriteria! discoveryCriteria, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.ITestDiscoveryEventsHandler2! eventHandler, bool skipDefaultAdapters) -> void diff --git a/test/Microsoft.TestPlatform.AcceptanceTests/TranslationLayerTests/DiscoverTests.cs b/test/Microsoft.TestPlatform.AcceptanceTests/TranslationLayerTests/DiscoverTests.cs index eb9b078493..b84914d873 100644 --- a/test/Microsoft.TestPlatform.AcceptanceTests/TranslationLayerTests/DiscoverTests.cs +++ b/test/Microsoft.TestPlatform.AcceptanceTests/TranslationLayerTests/DiscoverTests.cs @@ -164,7 +164,7 @@ public void DiscoverTestsUsingEventHandler1AndBatchSize(RunnerInfo runnerInfo) [TestMethod] [NetCoreTargetFrameworkDataSource] [NetFullTargetFrameworkDataSource] - public void DisoverTestUsingEventHandler2ShouldContainAllSourcesAsFullyDiscovered(RunnerInfo runnerInfo) + public void DiscoverTestUsingEventHandler2ShouldContainAllSourcesAsFullyDiscovered(RunnerInfo runnerInfo) { SetTestEnvironment(_testEnvironment, runnerInfo); Setup(); diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelOperationManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelOperationManagerTests.cs index 0d55a40695..cf65b349ee 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelOperationManagerTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelOperationManagerTests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using Microsoft.VisualStudio.TestTools.UnitTesting; using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client; @@ -32,7 +33,7 @@ public void OperationManagerShouldRunOnlyMaximumParallelLevelOfWorkInParallelEve List workerCounts = new(); Func getEventHandler = (handler, _) => handler; - Action runWorkload = (manager, _, _) => + Action runWorkload = (manager, _, _, _, _) => { // Every time we run a workload check how many slots are occupied, // we should see 3 slots at max, because that is our max parallel level, we should NOT see 4 or more: @@ -57,9 +58,11 @@ public void OperationManagerShouldRunOnlyMaximumParallelLevelOfWorkInParallelEve // and pass on the current manager that is done. parallelOperationManager.RunNextWork(manager); }; + Func initializeWorkload = (_, _, _) => + Task.Run(() => System.Threading.Thread.Sleep(100)); // Act - parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, runWorkload); + parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, initializeWorkload, runWorkload); // Assert workerCounts.Should().BeEquivalentTo(new[] { 3, 3, 3, 2, 1 }); @@ -82,7 +85,7 @@ public void OperationManagerShouldCreateOnlyAsManyParallelWorkersAsThereAreWorkl List workerCounts = new(); Func getEventHandler = (handler, _) => handler; - Action runWorkload = (manager, _, _) => + Action runWorkload = (manager, _, _, _, _) => { // See comments in test above for explanation. workerCounts.Add(parallelOperationManager.OccupiedSlotCount); @@ -90,9 +93,11 @@ public void OperationManagerShouldCreateOnlyAsManyParallelWorkersAsThereAreWorkl parallelOperationManager.RunNextWork(manager); }; + Func initializeWorkload = (_, _, _) => + Task.Run(() => System.Threading.Thread.Sleep(100)); // Act - parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, runWorkload); + parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, initializeWorkload, runWorkload); // Assert workerCounts.Should().BeEquivalentTo(new[] { 2, 1 }); @@ -117,7 +122,7 @@ public void OperationManagerShouldCreateAsManyMaxParallelLevel() List availableWorkerCounts = new(); Func getEventHandler = (handler, _) => handler; - Action runWorkload = (manager, _, _) => + Action runWorkload = (manager, _, _, _, _) => { // See comments in test above for explanation. workerCounts.Add(parallelOperationManager.OccupiedSlotCount); @@ -126,9 +131,11 @@ public void OperationManagerShouldCreateAsManyMaxParallelLevel() parallelOperationManager.RunNextWork(manager); }; + Func initializeWorkload = (_, _, _) => + Task.Run(() => System.Threading.Thread.Sleep(100)); // Act - parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, runWorkload); + parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, initializeWorkload, runWorkload); // Assert workerCounts.Should().BeEquivalentTo(new[] { 2, 1 }); @@ -155,7 +162,7 @@ public void OperationManagerMovesToTheNextWorkloadOnlyWhenRunNextWorkIsCalled() List workloadsProcessed = new(); Func getEventHandler = (handler, _) => handler; - Action runWorkload = (manager, _, workload) => + Action runWorkload = (manager, _, workload, _, _) => { // See comments in test above for explanation. System.Threading.Thread.Sleep(100); @@ -167,9 +174,11 @@ public void OperationManagerMovesToTheNextWorkloadOnlyWhenRunNextWorkIsCalled() parallelOperationManager.RunNextWork(manager); } }; + Func initializeWorkload = (_, _, _) => + Task.Run(() => System.Threading.Thread.Sleep(100)); // Act - parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, runWorkload); + parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, initializeWorkload, runWorkload); // Assert // We start by scheduling 2 workloads (1 and 2) becuase that is the max parallel level. @@ -204,7 +213,7 @@ public void OperationManagerRunsAnOperationOnAllActiveManagersWhenDoActionOnAllM var eventHandler = new SampleHandler(); Func getEventHandler = (handler, _) => handler; - Action runWorkload = (manager, _, workload) => + Action runWorkload = (manager, _, workload, _, _) => { // See comments in test above for explanation. @@ -220,9 +229,11 @@ public void OperationManagerRunsAnOperationOnAllActiveManagersWhenDoActionOnAllM parallelOperationManager.RunNextWork(manager); } }; + Func initializeWorkload = (_, _, _) => + Task.Run(() => System.Threading.Thread.Sleep(100)); // Start the work, so we process workload 1 and then move to 2. - parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, runWorkload); + parallelOperationManager.StartWork(workloads, eventHandler, getEventHandler, initializeWorkload, runWorkload); // Act parallelOperationManager.DoActionOnAllManagers(manager => manager.Abort(), doActionsInParallel: true); diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyExecutionManagerTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyExecutionManagerTests.cs index e20b0434dd..86806f8158 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyExecutionManagerTests.cs +++ b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelProxyExecutionManagerTests.cs @@ -284,7 +284,12 @@ public void StartTestRunShouldProcessAllSourcesOnExecutionAbortsForAnySource() Task.Run(() => parallelExecutionManager.StartTestRun(_testRunCriteriaWith2Sources, _mockEventHandler.Object)); Assert.IsTrue(_executionCompleted.Wait(Timeout3Seconds), "Test run not completed."); - Assert.AreEqual(2, _processedSources.Count, "Abort should stop all sources execution."); + + // Even though we start the test run for two sources, because of the current setup where + // we initialize a proxy if no more slots are available, we end up with abort notice being + // sent only to the running manager. This leaves the initialized manager in limbo and the + // assert will fail because of this. + Assert.AreEqual(1, _processedSources.Count, "Abort should stop all sources execution."); } [TestMethod]