From b90d305331d172222e070ecd73118d51b74f4878 Mon Sep 17 00:00:00 2001 From: nohwnd Date: Thu, 27 Jan 2022 13:42:31 +0100 Subject: [PATCH 1/8] Log callbacks to delegates better --- .../LengthPrefixCommunicationChannel.cs | 11 +++++ .../SocketCommunicationManager.cs | 12 +++++- .../TestRequestSender.cs | 7 ++++ .../Utilities/MulticastDelegateUtilities.cs | 42 +++++++++++++++---- 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs index 63785782a1..95e262e744 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs @@ -63,6 +63,13 @@ public Task Send(string data) /// public Task NotifyDataAvailable() { + // TODO: Review the comment below, because it says something different than what is + // actually happening, and doing what it suggests would potentially lose messages. + // For example in the case where we start testhost process, send it version, and + // it responds, we then replace the handler with a new one, and there is quite a long time + // (tens of milliseconds) when there is no handler present, which would pump the message + // and dump it. + // // Try read data even if no one is listening to the data stream. Some server // implementations (like Sockets) depend on the read operation to determine if a // connection is closed. @@ -71,6 +78,10 @@ public Task NotifyDataAvailable() var data = this.reader.ReadString(); this.MessageReceived.SafeInvoke(this, new MessageReceivedEventArgs { Data = data }, "LengthPrefixCommunicationChannel: MessageReceived"); } + else + { + EqtTrace.Verbose("LengthPrefixCommunicationChannel.NotifyDataAvailable: New data are waiting to be received, but there is no subscriber to be notified. Not reading them from the stream."); + } return Task.FromResult(0); } diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs index f9209e20f1..1787c632c8 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs @@ -138,7 +138,11 @@ public async Task AcceptClientAsync() /// True if Client is connected, false otherwise public bool WaitForClientConnection(int clientConnectionTimeout) { - return this.clientConnectedEvent.WaitOne(clientConnectionTimeout); + Stopwatch stopWatch = Stopwatch.StartNew(); + var result = this.clientConnectedEvent.WaitOne(clientConnectionTimeout); + EqtTrace.Verbose("SocketCommunicationManager.WaitForClientConnection took: {0} ms, with {1} ms timeout, and finished with {2}", stopWatch.ElapsedMilliseconds, clientConnectionTimeout, result); + + return result; } /// @@ -214,7 +218,11 @@ public async Task SetupClientAsync(IPEndPoint endpoint) /// True, if Server got a connection from client public bool WaitForServerConnection(int connectionTimeout) { - return this.clientConnectionAcceptedEvent.WaitOne(connectionTimeout); + Stopwatch stopWatch = Stopwatch.StartNew(); + var result = this.clientConnectionAcceptedEvent.WaitOne(connectionTimeout); + EqtTrace.Verbose("SocketCommunicationManager.WaitForServerConnection took: {0} ms, with {1} ms timeout, and finished with {2}", stopWatch.ElapsedMilliseconds, connectionTimeout, result); + + return result; } /// diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs index 4e2cb19bf0..dd3db4b365 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs @@ -5,6 +5,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities { using System; using System.Collections.Generic; + using System.Diagnostics; using System.Globalization; using System.Threading; using CoreUtilities.Helpers; @@ -163,6 +164,7 @@ public int InitializeCommunication() /// public bool WaitForRequestHandlerConnection(int connectionTimeout, CancellationToken cancellationToken) { + var sw = Stopwatch.StartNew(); if (EqtTrace.IsVerboseEnabled) { EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting for connection with timeout: {0}", connectionTimeout); @@ -173,6 +175,11 @@ public bool WaitForRequestHandlerConnection(int connectionTimeout, CancellationT // or testhost exits unexpectedly, handled by clientExited.WaitHandle var waitIndex = WaitHandle.WaitAny(new WaitHandle[] { this.connected.WaitHandle, cancellationToken.WaitHandle, this.clientExited.WaitHandle }, connectionTimeout); + if (EqtTrace.IsVerboseEnabled) + { + EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting took {0} ms, with timeout {1} ms, and result {2}, which is {3}", sw.ElapsedMilliseconds, connectionTimeout, waitIndex, waitIndex == 0 ? "success" : "failure"); + } + // Return true if connection was successful. return waitIndex == 0; } diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs index 5d12e4ae24..444b9792f9 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs +++ b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs @@ -4,8 +4,8 @@ namespace Microsoft.VisualStudio.TestPlatform.Utilities { using System; + using System.Diagnostics; using System.Reflection; - using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Resources; using Microsoft.VisualStudio.TestPlatform.ObjectModel; @@ -22,40 +22,64 @@ public static class MulticastDelegateUtilities /// Sender to use when raising the event. /// Arguments to provide. /// Name to use when tracing out errors. + // Using [CallerMemberName] for the traceDisplayName is a possibility, but in few places we call through other + // methods until we reach here. And it would change the public API. public static void SafeInvoke(this Delegate delegates, object sender, EventArgs args, string traceDisplayName) { if (args == null) { - throw new ArgumentNullException(Resources.CannotBeNullOrEmpty, "args"); + throw new ArgumentNullException(Resources.CannotBeNullOrEmpty, nameof(args)); } if (string.IsNullOrWhiteSpace(traceDisplayName)) { - throw new ArgumentException(Resources.CannotBeNullOrEmpty, traceDisplayName); + throw new ArgumentException(Resources.CannotBeNullOrEmpty, nameof(traceDisplayName)); } if (delegates != null) { - foreach (Delegate handler in delegates.GetInvocationList()) + var invocationList = delegates.GetInvocationList(); + var i = 0; + foreach (Delegate handler in invocationList) { + Stopwatch stopwatch = Stopwatch.StartNew(); + try { handler.DynamicInvoke(sender, args); + if (EqtTrace.IsVerboseEnabled) + { + stopwatch = Stopwatch.StartNew(); + EqtTrace.Verbose("MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callback {1}/{2} for {3}.{4}, took {5} ms.", + traceDisplayName, + ++i, + invocationList.Length, + handler.Target ?? "static", + handler.Method.Name, + stopwatch.ElapsedMilliseconds); + } } - catch (TargetInvocationException e) + catch (TargetInvocationException exception) { if (EqtTrace.IsErrorEnabled) { EqtTrace.Error( - "{0}: Exception occurred while calling handler of type {1} for {2}: {3}", + "MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callback {1}/{2} for {3}.{4}, failed after {5} ms with: {6}", + ++i, + invocationList.Length, + handler.Target ?? "static", + handler.Method.Name, traceDisplayName, - handler.Target.GetType().FullName, - args.GetType().Name, - e); + stopwatch.ElapsedMilliseconds, + exception); } } } } + else + { + EqtTrace.Verbose("MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callbacks was skipped because there are no subscribers.", traceDisplayName); + } } } } From 25e7db37731b5ab23ef5c3e6c30fd07912580a23 Mon Sep 17 00:00:00 2001 From: nohwnd Date: Thu, 27 Jan 2022 14:09:20 +0100 Subject: [PATCH 2/8] Use SafeInvoke where possible --- .../Discovery/DiscoveryRequest.cs | 2 +- .../Execution/TestRunRequest.cs | 2 +- .../DataCollectionAttachmentManager.cs | 2 +- .../PublicAPI/PublicAPI.Unshipped.txt | 2 +- .../Utilities/MulticastDelegateUtilities.cs | 15 +++++++++++++++ 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.TestPlatform.Client/Discovery/DiscoveryRequest.cs b/src/Microsoft.TestPlatform.Client/Discovery/DiscoveryRequest.cs index d63a2508e2..d809474fe4 100644 --- a/src/Microsoft.TestPlatform.Client/Discovery/DiscoveryRequest.cs +++ b/src/Microsoft.TestPlatform.Client/Discovery/DiscoveryRequest.cs @@ -406,7 +406,7 @@ public void HandleRawMessage(string rawMessage) HandleLoggerManagerDiscoveryComplete(discoveryCompletePayload); } - this.OnRawMessageReceived?.Invoke(this, rawMessage); + this.OnRawMessageReceived?.SafeInvoke(this, rawMessage, "DiscoveryRequest.RawMessageReceived"); } /// diff --git a/src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs b/src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs index e3efa01cb8..3b0459c98a 100644 --- a/src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs +++ b/src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs @@ -561,7 +561,7 @@ public void HandleRawMessage(string rawMessage) HandleLoggerManagerTestRunComplete(testRunCompletePayload); } - this.OnRawMessageReceived?.Invoke(this, rawMessage); + this.OnRawMessageReceived?.SafeInvoke(this, rawMessage, "TestRunRequest.RawMessageReceived"); } /// diff --git a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs index 50eaa48b6b..0ca07ea0a8 100644 --- a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs +++ b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs @@ -333,7 +333,7 @@ private void AddNewFileTransfer(FileTransferInformation fileTransferInfo, AsyncC } } - sendFileCompletedCallback?.Invoke(this, new AsyncCompletedEventArgs(t.Exception, false, fileTransferInfo.UserToken)); + sendFileCompletedCallback?.SafeInvoke(this, new AsyncCompletedEventArgs(t.Exception, false, fileTransferInfo.UserToken), "DataCollectionManager.AddNewFileTransfer"); } catch (Exception e) { diff --git a/src/Microsoft.TestPlatform.CoreUtilities/PublicAPI/PublicAPI.Unshipped.txt b/src/Microsoft.TestPlatform.CoreUtilities/PublicAPI/PublicAPI.Unshipped.txt index 5f282702bb..64339d8507 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/Microsoft.TestPlatform.CoreUtilities/PublicAPI/PublicAPI.Unshipped.txt @@ -1 +1 @@ - \ No newline at end of file +static Microsoft.VisualStudio.TestPlatform.Utilities.MulticastDelegateUtilities.SafeInvoke(this System.Delegate delegates, object sender, object args, string traceDisplayName) -> void \ No newline at end of file diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs index 444b9792f9..fd53014d22 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs +++ b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs @@ -25,6 +25,21 @@ public static class MulticastDelegateUtilities // Using [CallerMemberName] for the traceDisplayName is a possibility, but in few places we call through other // methods until we reach here. And it would change the public API. public static void SafeInvoke(this Delegate delegates, object sender, EventArgs args, string traceDisplayName) + { + SafeInvoke(delegates, sender, (object)args, traceDisplayName); + } + + /// + /// Invokes each of the subscribers of the event and handles exceptions which are thrown + /// ensuring that each handler is invoked even if one throws. + /// + /// Event handler to invoke. + /// Sender to use when raising the event. + /// Arguments to provide. + /// Name to use when tracing out errors. + // Using [CallerMemberName] for the traceDisplayName is a possibility, but in few places we call through other + // methods until we reach here. And it would change the public API. + public static void SafeInvoke(this Delegate delegates, object sender, object args, string traceDisplayName) { if (args == null) { From 5e5e0466df3b0c77a5a5653ab5cdf36c5586b148 Mon Sep 17 00:00:00 2001 From: nohwnd Date: Thu, 27 Jan 2022 14:29:25 +0100 Subject: [PATCH 3/8] Conditionally get name --- .../Utilities/MulticastDelegateUtilities.cs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs index fd53014d22..589c005d64 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs +++ b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs @@ -69,8 +69,8 @@ public static void SafeInvoke(this Delegate delegates, object sender, object arg traceDisplayName, ++i, invocationList.Length, - handler.Target ?? "static", - handler.Method.Name, + handler.GetTargetName(), + handler.GetMethodName(), stopwatch.ElapsedMilliseconds); } } @@ -82,8 +82,8 @@ public static void SafeInvoke(this Delegate delegates, object sender, object arg "MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callback {1}/{2} for {3}.{4}, failed after {5} ms with: {6}", ++i, invocationList.Length, - handler.Target ?? "static", - handler.Method.Name, + handler.GetTargetName(), + handler.GetMethodName(), traceDisplayName, stopwatch.ElapsedMilliseconds, exception); @@ -96,5 +96,19 @@ public static void SafeInvoke(this Delegate delegates, object sender, object arg EqtTrace.Verbose("MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callbacks was skipped because there are no subscribers.", traceDisplayName); } } + + internal static string GetMethodName(this Delegate @delegate) + { +#if NETSTANDARD2_0 + return @delegate.Method.Name; +#else + return null; +#endif + } + + internal static string GetTargetName(this Delegate @delegate) + { + return @delegate.Target?.ToString() ?? "static"; + } } } From 772d5de5cfb4d8e5da8f71062a3588e95ca391cc Mon Sep 17 00:00:00 2001 From: nohwnd Date: Thu, 27 Jan 2022 15:17:26 +0100 Subject: [PATCH 4/8] Fix NRE on raw message in test --- .../Execution/TestRunRequestTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs b/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs index 15a5c2a44c..a5d8fe0b08 100644 --- a/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs +++ b/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs @@ -170,6 +170,10 @@ public void OnTestSessionTimeoutShouldLogMessage() bool handleLogMessageCalled = false; bool handleRawMessageCalled = false; + this.mockDataSerializer + .Setup(s => s.SerializePayload(It.IsAny(), It.IsAny())) + .Returns("non-empty rawMessage"); + this.testRunRequest.TestRunMessage += (object sender, TestRunMessageEventArgs e) => { handleLogMessageCalled = true; From 348260df2b847454501d815856bbd25971e8c7f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Fri, 28 Jan 2022 02:30:33 -0800 Subject: [PATCH 5/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- .../Utilities/MulticastDelegateUtilities.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs index 589c005d64..d0652518c2 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs +++ b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs @@ -37,18 +37,16 @@ public static void SafeInvoke(this Delegate delegates, object sender, EventArgs /// Sender to use when raising the event. /// Arguments to provide. /// Name to use when tracing out errors. - // Using [CallerMemberName] for the traceDisplayName is a possibility, but in few places we call through other - // methods until we reach here. And it would change the public API. public static void SafeInvoke(this Delegate delegates, object sender, object args, string traceDisplayName) { if (args == null) { - throw new ArgumentNullException(Resources.CannotBeNullOrEmpty, nameof(args)); + throw new ArgumentNullException(nameof(args)); } if (string.IsNullOrWhiteSpace(traceDisplayName)) { - throw new ArgumentException(Resources.CannotBeNullOrEmpty, nameof(traceDisplayName)); + throw new ArgumentException(nameof(traceDisplayName)); } if (delegates != null) @@ -57,14 +55,13 @@ public static void SafeInvoke(this Delegate delegates, object sender, object arg var i = 0; foreach (Delegate handler in invocationList) { - Stopwatch stopwatch = Stopwatch.StartNew(); + var stopwatch = Stopwatch.StartNew(); try { handler.DynamicInvoke(sender, args); if (EqtTrace.IsVerboseEnabled) { - stopwatch = Stopwatch.StartNew(); EqtTrace.Verbose("MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callback {1}/{2} for {3}.{4}, took {5} ms.", traceDisplayName, ++i, @@ -79,7 +76,7 @@ public static void SafeInvoke(this Delegate delegates, object sender, object arg if (EqtTrace.IsErrorEnabled) { EqtTrace.Error( - "MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callback {1}/{2} for {3}.{4}, failed after {5} ms with: {6}", + "MulticastDelegateUtilities.SafeInvoke: {0}: Invoking callback {1}/{2} for {3}.{4}, failed after {5} ms with: {6}.", ++i, invocationList.Length, handler.GetTargetName(), From d8e74c23ff7af42fb896dc5d06a2b410c5b44ef6 Mon Sep 17 00:00:00 2001 From: nohwnd Date: Fri, 28 Jan 2022 14:51:52 +0100 Subject: [PATCH 6/8] Fix tiny issues --- .../SocketCommunicationManager.cs | 4 ++-- .../TestRequestSender.cs | 4 ++-- .../TestRunAttachmentsProcessingManager.cs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs index 9f7dff392d..9dc746aada 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs @@ -139,7 +139,7 @@ public async Task AcceptClientAsync() /// True if Client is connected, false otherwise public bool WaitForClientConnection(int clientConnectionTimeout) { - Stopwatch stopWatch = Stopwatch.StartNew(); + var stopWatch = Stopwatch.StartNew(); var result = _clientConnectedEvent.WaitOne(clientConnectionTimeout); EqtTrace.Verbose("SocketCommunicationManager.WaitForClientConnection took: {0} ms, with {1} ms timeout, and finished with {2}", stopWatch.ElapsedMilliseconds, clientConnectionTimeout, result); @@ -219,7 +219,7 @@ public async Task SetupClientAsync(IPEndPoint endpoint) /// True, if Server got a connection from client public bool WaitForServerConnection(int connectionTimeout) { - Stopwatch stopWatch = Stopwatch.StartNew(); + var stopWatch = Stopwatch.StartNew(); var result = _clientConnectionAcceptedEvent.WaitOne(connectionTimeout); EqtTrace.Verbose("SocketCommunicationManager.WaitForServerConnection took: {0} ms, with {1} ms timeout, and finished with {2}", stopWatch.ElapsedMilliseconds, connectionTimeout, result); diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs index eeab3d46d3..5c8b810f11 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs @@ -168,7 +168,7 @@ public bool WaitForRequestHandlerConnection(int connectionTimeout, CancellationT var sw = Stopwatch.StartNew(); if (EqtTrace.IsVerboseEnabled) { - EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting for connection with timeout: {0}", connectionTimeout); + EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting for connection with timeout: {0}.", connectionTimeout); } // Wait until either connection is successful, handled by connected.WaitHandle @@ -178,7 +178,7 @@ public bool WaitForRequestHandlerConnection(int connectionTimeout, CancellationT if (EqtTrace.IsVerboseEnabled) { - EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting took {0} ms, with timeout {1} ms, and result {2}, which is {3}", sw.ElapsedMilliseconds, connectionTimeout, waitIndex, waitIndex == 0 ? "success" : "failure"); + EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting took {0} ms, with timeout {1} ms, and result {2}, which is {3}.", sw.ElapsedMilliseconds, connectionTimeout, waitIndex, waitIndex == 0 ? "success" : "failure"); } // Return true if connection was successful. diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/AttachmentsProcessing/TestRunAttachmentsProcessingManager.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/AttachmentsProcessing/TestRunAttachmentsProcessingManager.cs index 60d672ea86..6f68bca4fe 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/AttachmentsProcessing/TestRunAttachmentsProcessingManager.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/AttachmentsProcessing/TestRunAttachmentsProcessingManager.cs @@ -54,7 +54,7 @@ public Task> ProcessTestRunAttachmentsAsync(string run private async Task> InternalProcessTestRunAttachmentsAsync(string runSettingsXml, IRequestData requestData, Collection attachments, IEnumerable invokedDataCollector, ITestRunAttachmentsProcessingEventsHandler eventHandler, CancellationToken cancellationToken) { - Stopwatch stopwatch = Stopwatch.StartNew(); + var stopwatch = Stopwatch.StartNew(); try { From a5a627e81399aa50aae5d48325e6b0f2ae845905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Fri, 28 Jan 2022 06:25:56 -0800 Subject: [PATCH 7/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- .../DataCollection/DataCollectionAttachmentManager.cs | 2 +- .../SocketCommunicationManager.cs | 4 ++-- .../Execution/TestRunRequestTests.cs | 5 +---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs index 5749400ba6..3cc7f5dac5 100644 --- a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs +++ b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs @@ -346,7 +346,7 @@ private void AddNewFileTransfer(FileTransferInformation fileTransferInfo, AsyncC } } }, - _cancellationTokenSource.Token); + _cancellationTokenSource.Token); _attachmentTasks[fileTransferInfo.Context].Add(continuationTask); } diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs index 9dc746aada..68225ef2c7 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs @@ -141,7 +141,7 @@ public bool WaitForClientConnection(int clientConnectionTimeout) { var stopWatch = Stopwatch.StartNew(); var result = _clientConnectedEvent.WaitOne(clientConnectionTimeout); - EqtTrace.Verbose("SocketCommunicationManager.WaitForClientConnection took: {0} ms, with {1} ms timeout, and finished with {2}", stopWatch.ElapsedMilliseconds, clientConnectionTimeout, result); + EqtTrace.Verbose("SocketCommunicationManager.WaitForClientConnection took: {0} ms, with {1} ms timeout, and finished with {2}.", stopWatch.ElapsedMilliseconds, clientConnectionTimeout, result); return result; } @@ -221,7 +221,7 @@ public bool WaitForServerConnection(int connectionTimeout) { var stopWatch = Stopwatch.StartNew(); var result = _clientConnectionAcceptedEvent.WaitOne(connectionTimeout); - EqtTrace.Verbose("SocketCommunicationManager.WaitForServerConnection took: {0} ms, with {1} ms timeout, and finished with {2}", stopWatch.ElapsedMilliseconds, connectionTimeout, result); + EqtTrace.Verbose("SocketCommunicationManager.WaitForServerConnection took: {0} ms, with {1} ms timeout, and finished with {2}.", stopWatch.ElapsedMilliseconds, connectionTimeout, result); return result; } diff --git a/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs b/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs index c8faec9d56..8f3f3e8596 100644 --- a/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs +++ b/test/Microsoft.TestPlatform.Client.UnitTests/Execution/TestRunRequestTests.cs @@ -174,10 +174,7 @@ public void OnTestSessionTimeoutShouldLogMessage() .Setup(s => s.SerializePayload(It.IsAny(), It.IsAny())) .Returns("non-empty rawMessage"); - _testRunRequest.TestRunMessage += (object sender, TestRunMessageEventArgs e) => - { - handleLogMessageCalled = true; - }; + _testRunRequest.TestRunMessage += (object sender, TestRunMessageEventArgs e) => handleLogMessageCalled = true; _testRunRequest.OnRawMessageReceived += (object sender, string message) => handleRawMessageCalled = true; From 371a8da8bc846dcb152db42416d3dbecb7bafcc7 Mon Sep 17 00:00:00 2001 From: nohwnd Date: Tue, 1 Feb 2022 14:00:15 +0100 Subject: [PATCH 8/8] Fix build issues --- .../Utilities/MulticastDelegateUtilities.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs index 5ebbd2f4c3..dd7e68a95b 100644 --- a/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs +++ b/src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs @@ -46,7 +46,7 @@ public static void SafeInvoke(this Delegate delegates, object sender, object arg if (string.IsNullOrWhiteSpace(traceDisplayName)) { - throw new ArgumentException(nameof(traceDisplayName)); + throw new ArgumentNullException(nameof(traceDisplayName)); } if (delegates != null)