Enable some rules with no impact on public API (#3299)

Co-authored-by: Amaury Levé <amauryleve@microsoft.com>

.editorconfig

@@ -45,10 +45,10 @@ indent_size = 2
 # C# files
 [*.cs]
 
-# Do not set 'end_of_line = crlf' otherwise the rule IDE0055: Fix formatting will trigger on Linux/MAC OS.
-
 #### Core EditorConfig Options ####
 
+# Do not set 'end_of_line = crlf' otherwise the rule IDE0055: Fix formatting will trigger on Linux/MAC OS.
+
 # Indentation and spacing
 indent_size = 4
 tab_width = 4
@@ -116,6 +116,33 @@ dotnet_code_quality_unused_parameters = all:suggestion
 # Suppression preferences
 dotnet_remove_unnecessary_suppression_exclusions = none
 
+# IDE0090: Use 'new(...)'
+dotnet_diagnostic.IDE0090.severity = warning # not default, increased severity to go with simpler declarations
+
+# IDE0005: Remove unnecessary import
+dotnet_diagnostic.IDE0005.severity = warning # not default, increased severity to ensure it is used
+
+# 'using' directive preferences
+# Keep this in sync with the related C# rule: csharp_using_directive_placement
+dotnet_diagnostic.IDE0065.severity = none
+
+# Use simple using statements
+# Keep this in sync with th related C# rule: csharp_prefer_simple_using_statement
+dotnet_diagnostic.IDE0063.severity = warning
+
+# CA2208: Instantiate argument exceptions correctly
+dotnet_diagnostic.CA2208.severity = warning # not default, increased severity to ensure it is always applied
+
+# CA2241: Provide correct arguments to formatting methods
+dotnet_diagnostic.CA2241.severity = warning # not default, increased severity to ensure it is always applied
+
+# IDE0053: Use expression body for lambda expressions
+# Keep this in sync with csharp_style_expression_bodied_lambdas
+dotnet_diagnostic.IDE0053.severity = warning # not default, increased severity to ensure it is applied
+
+# CA2016: Forward the 'CancellationToken' parameter to methods
+dotnet_diagnostic.CA2016.severity = warning # not default, increased severity to ensure it is applied
+
 #### C# Coding Conventions ####
 
 # var preferences
@@ -127,7 +154,8 @@ csharp_style_var_when_type_is_apparent = false:silent   # not default, turned of
 csharp_style_expression_bodied_accessors = true:silent
 csharp_style_expression_bodied_constructors = false:silent
 csharp_style_expression_bodied_indexers = true:silent
-csharp_style_expression_bodied_lambdas = true:suggestion
+# Keep this in sync with IDE0053
+csharp_style_expression_bodied_lambdas = true:warning # not default, increased severity to ensure it is applied
 csharp_style_expression_bodied_local_functions = false:silent
 csharp_style_expression_bodied_methods = false:silent
 csharp_style_expression_bodied_operators = false:silent
@@ -149,8 +177,8 @@ csharp_preferred_modifier_order = public,private,protected,internal,static,exter
 
 # Code-block preferences
 csharp_prefer_braces = true:silent
+# Keep this in sync with the related .NET rule: IDE0063
 csharp_prefer_simple_using_statement = true:warning # not default, default is true:suggestion, increased severity to ensure it is used
-dotnet_diagnostic.IDE0063.severity = warning
 
 # Expression-level preferences
 csharp_prefer_simple_default_expression = true:suggestion
@@ -164,23 +192,16 @@ csharp_style_unused_value_assignment_preference = discard_variable:suggestion
 csharp_style_unused_value_expression_statement_preference = discard_variable:silent
 
 # 'using' directive preferences
+# Keep this in sync with the related .NET rule: IDE0065
 csharp_using_directive_placement = outside_namespace:silent
-dotnet_diagnostic.IDE0065.severity = none
-
-# CA1822: Mark members as static
-dotnet_diagnostic.CA1822.severity = suggestion # TODO: increase severity and fix related entries
-
-# IDE0090: Use 'new(...)'
-dotnet_diagnostic.IDE0090.severity = warning # not default, increased severity to go with simpler declarations
-
-# IDE0005: Remove unnecessary import
-dotnet_diagnostic.IDE0005.severity = warning # not default, increased severity to ensure it is used
 
-#### C# Formatting Rules ####
+#### .NET Formatting Rules ####
 
 # IDE0055: Fix formatting
 dotnet_diagnostic.IDE0055.severity = warning
 
+#### C# Formatting Rules ####
+
 # New line preferences
 csharp_new_line_before_catch = true
 csharp_new_line_before_else = true
@@ -226,7 +247,7 @@ csharp_space_between_square_brackets = false
 csharp_preserve_single_line_blocks = true
 csharp_preserve_single_line_statements = true
 
-#### Naming styles ####
+#### .NET Naming styles ####
 
 # not default, we have a lot of multiplatform code in compiler directives that defines methods, but keeps the bodies empty
 # IDE0052: Remove unused private members
@@ -239,15 +260,10 @@ dotnet_diagnostic.IDE0052.severity = silent
 # IDE1006: Naming Styles
 dotnet_diagnostic.IDE1006.severity = warning
 dotnet_diagnostic.IDE0073.severity = warning
-csharp_style_prefer_method_group_conversion = true:silent
-csharp_style_prefer_null_check_over_type_check = true:suggestion
-csharp_style_implicit_object_creation_when_type_is_apparent = true:suggestion
-csharp_style_prefer_tuple_swap = true:suggestion
 
 # Naming rules
 
 dotnet_style_namespace_match_folder = true:suggestion
-csharp_style_namespace_declarations = file_scoped:warning # not default, default is block_scoped:silent, changed to follow modern preferences
 
 dotnet_naming_rule.types_and_namespaces_should_be_pascalcase.severity = suggestion
 dotnet_naming_rule.types_and_namespaces_should_be_pascalcase.symbols = types_and_namespaces
@@ -434,3 +450,14 @@ dotnet_naming_style.s_camelcase.required_prefix = s_
 dotnet_naming_style.s_camelcase.required_suffix =
 dotnet_naming_style.s_camelcase.word_separator =
 dotnet_naming_style.s_camelcase.capitalization = camel_case
+
+#### C# Naming styles ####
+
+csharp_style_prefer_method_group_conversion = true:silent
+csharp_style_prefer_null_check_over_type_check = true:suggestion
+csharp_style_implicit_object_creation_when_type_is_apparent = true:suggestion
+csharp_style_prefer_tuple_swap = true:suggestion
+
+# Naming rules
+
+csharp_style_namespace_declarations = file_scoped:warning # not default, default is block_scoped:silent, changed to follow modern preferences

src/DataCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogDataCollector.cs

@@ -379,7 +379,7 @@ private void OnTestCaseStart(object sender, TestCaseStartEventArgs e)
         if (!e.Context.HasTestCase)
         {
             Debug.Fail("Context is not for a test case");
-            throw new ArgumentNullException("TestCaseStartEventArgs.Context.HasTestCase");
+            ValidateArg.NotNull(e.Context.TestExecId, "TestCaseStartEventArgs.Context.HasTestCase");
         }
 
         if (EqtTrace.IsVerboseEnabled)

src/Microsoft.TestPlatform.Client/TestPlatform.cs

@@ -242,7 +242,7 @@ private void AddExtensionAssemblies(string runSettings)
                 {
                     if (EqtTrace.IsWarningEnabled)
                     {
-                        EqtTrace.Warning(string.Format("AdapterPath Not Found:", adapterPath));
+                        EqtTrace.Warning($"AdapterPath Not Found: {adapterPath}");
                     }
 
                     continue;

src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestHandler.cs

@@ -285,7 +285,7 @@ private void AddExtensionAssemblies(BeforeTestRunStartPayload payload)
                     Path.GetFullPath(Environment.ExpandEnvironmentVariables(datacollectorSearchPath));
                 if (!_fileHelper.DirectoryExists(adapterPath))
                 {
-                    EqtTrace.Warning(string.Format("AdapterPath Not Found:", adapterPath));
+                    EqtTrace.Warning($"AdapterPath Not Found: {adapterPath}");
                     continue;
                 }
 

src/Microsoft.TestPlatform.CoreUtilities/Utilities/MulticastDelegateUtilities.cs

@@ -27,12 +27,12 @@ public static void SafeInvoke(this Delegate delegates, object sender, EventArgs
     {
         if (args == null)
         {
-            throw new ArgumentNullException(Resources.CannotBeNullOrEmpty, "args");
+            throw new ArgumentNullException(nameof(args));
         }
 
         if (string.IsNullOrWhiteSpace(traceDisplayName))
         {
-            throw new ArgumentException(Resources.CannotBeNullOrEmpty, traceDisplayName);
+            throw new ArgumentException(Resources.CannotBeNullOrEmpty, nameof(traceDisplayName));
         }
 
         if (delegates != null)

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

@@ -61,7 +61,7 @@ public CancellationTokenSource CancellationTokenSource
     /// <summary>
     /// Initializes a new instance of the <see cref="ProxyExecutionManager"/> class.
     /// </summary>
-    /// 
+    ///
     /// <param name="testSessionInfo">The test session info.</param>
     /// <param name="proxyOperationManagerCreator">The proxy operation manager creator.</param>
     /// <param name="debugEnabledForTestSession">
@@ -88,7 +88,7 @@ public ProxyExecutionManager(
     /// <summary>
     /// Initializes a new instance of the <see cref="ProxyExecutionManager"/> class.
     /// </summary>
-    /// 
+    ///
     /// <param name="requestData">
     /// The request data for providing services and data for run.
     /// </param>
@@ -110,11 +110,11 @@ public ProxyExecutionManager(
     /// <summary>
     /// Initializes a new instance of the <see cref="ProxyExecutionManager"/> class.
     /// </summary>
-    /// 
+    ///
     /// <remarks>
     /// Constructor with dependency injection. Used for unit testing.
     /// </remarks>
-    /// 
+    ///
     /// <param name="requestData">The request data for common services and data for run.</param>
     /// <param name="requestSender">Request sender instance.</param>
     /// <param name="testHostManager">Test host manager instance.</param>
@@ -211,7 +211,7 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsH
                 // This is workaround for the bug https://github.com/Microsoft/vstest/issues/970
                 var runsettings = _proxyOperationManager.RemoveNodesFromRunsettingsIfRequired(
                     testRunCriteria.TestRunSettings,
-                    (testMessageLevel, message) => { LogMessage(testMessageLevel, message); });
+                    LogMessage);
 
                 if (testRunCriteria.HasSpecificSources)
                 {
@@ -396,10 +396,10 @@ public virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessStartI
     /// Ensures that the engine is ready for test operations. Usually includes starting up the
     /// test host process.
     /// </summary>
-    /// 
+    ///
     /// <param name="sources">List of test sources.</param>
     /// <param name="runSettings">Run settings to be used.</param>
-    /// 
+    ///
     /// <returns>
     /// Returns true if the communication is established b/w runner and host, false otherwise.
     /// </returns>

src/Microsoft.TestPlatform.ObjectModel/InvokedDataCollector.cs

@@ -17,10 +17,10 @@ public sealed class InvokedDataCollector : IEquatable<InvokedDataCollector>
     /// <param name="hasAttachmentProcessor">True if data collector registers an attachment processor</param>
     public InvokedDataCollector(Uri uri, string friendlyName, string assemblyQualifiedName, string filePath, bool hasAttachmentProcessor)
     {
-        Uri = uri ?? throw new ArgumentException(nameof(uri));
-        FriendlyName = friendlyName ?? throw new ArgumentException(nameof(friendlyName));
-        AssemblyQualifiedName = assemblyQualifiedName ?? throw new ArgumentException(nameof(assemblyQualifiedName)); ;
-        FilePath = filePath ?? throw new ArgumentException(nameof(filePath)); ;
+        Uri = uri ?? throw new ArgumentNullException(nameof(uri));
+        FriendlyName = friendlyName ?? throw new ArgumentNullException(nameof(friendlyName));
+        AssemblyQualifiedName = assemblyQualifiedName ?? throw new ArgumentNullException(nameof(assemblyQualifiedName)); ;
+        FilePath = filePath ?? throw new ArgumentNullException(nameof(filePath)); ;
         HasAttachmentProcessor = hasAttachmentProcessor;
     }
 

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/AttachmentsProcessing/TestRunAttachmentsProcessingManagerTests.cs

@@ -424,7 +424,7 @@ public async Task ProcessTestRunAttachmentsAsync_ShouldReturnInitialAttachmentsT
                 {
                     for (int i = 0; i < 100; ++i)
                     {
-                        Task.Delay(100).Wait();
+                        Task.Delay(100, cancellation).Wait(cancellation);
                         Console.WriteLine($"Iteration: {i}");
                         logger.SendMessage(TestMessageLevel.Informational, $"Iteration: {i}");
 
@@ -434,7 +434,7 @@ public async Task ProcessTestRunAttachmentsAsync_ShouldReturnInitialAttachmentsT
                         if (i == 3)
                         {
                             _cancellationTokenSource.Cancel();
-                            Task.Delay(500).Wait();
+                            Task.Delay(500, cancellation).Wait(cancellation);
                         }
                     }
                 }
@@ -490,7 +490,7 @@ public async Task ProcessTestRunAttachmentsAsync_ShouldReturnInitialAttachments_
                 {
                     for (int i = 0; i < 1000; ++i)
                     {
-                        Task.Delay(100).Wait();
+                        Task.Delay(100, cancellation).Wait(cancellation);
                         Console.WriteLine($"Iteration: {i}");
                         logger.SendMessage(TestMessageLevel.Informational, $"Iteration: {i}");
 
@@ -499,7 +499,7 @@ public async Task ProcessTestRunAttachmentsAsync_ShouldReturnInitialAttachments_
                         if (i == 3)
                         {
                             _cancellationTokenSource.Cancel();
-                            Task.Delay(500).Wait();
+                            Task.Delay(500, cancellation).Wait(cancellation);
                         }
                     }
                 }

test/TranslationLayer.UnitTests/VsTestConsoleRequestSenderTests.cs

@@ -676,7 +676,7 @@ public void DiscoverTestsShouldLogErrorWhenProcessExited()
         _mockCommunicationManager.Setup(cm => cm.ReceiveMessageAsync(It.IsAny<CancellationToken>())).Callback(
             (CancellationToken c) =>
             {
-                Task.Run(() => _requestSender.OnProcessExited()).Wait();
+                Task.Run(() => _requestSender.OnProcessExited(), c).Wait(c);
 
                 Assert.IsTrue(c.IsCancellationRequested);
             }).Returns(Task.FromResult((Message)null));
@@ -703,7 +703,7 @@ public async Task DiscoverTestsAsyncShouldLogErrorWhenProcessExited()
         _mockCommunicationManager.Setup(cm => cm.ReceiveMessageAsync(It.IsAny<CancellationToken>())).Callback(
             (CancellationToken c) =>
             {
-                Task.Run(() => _requestSender.OnProcessExited()).Wait();
+                Task.Run(() => _requestSender.OnProcessExited(), c).Wait(c);
 
                 Assert.IsTrue(c.IsCancellationRequested);
             }).Returns(Task.FromResult((Message)null));
@@ -1815,7 +1815,7 @@ public void StartTestRunShouldLogErrorOnProcessExited()
         _mockCommunicationManager.Setup(cm => cm.ReceiveMessageAsync(It.IsAny<CancellationToken>()))
             .Callback((CancellationToken c) =>
             {
-                Task.Run(() => _requestSender.OnProcessExited()).Wait();
+                Task.Run(() => _requestSender.OnProcessExited(), c).Wait(c);
 
                 Assert.IsTrue(c.IsCancellationRequested);
             }).Returns(Task.FromResult((Message)null));
@@ -1846,7 +1846,7 @@ public async Task StartTestRunAsyncShouldLogErrorOnProcessExited()
         _mockCommunicationManager.Setup(cm => cm.ReceiveMessageAsync(It.IsAny<CancellationToken>()))
             .Callback((CancellationToken c) =>
             {
-                Task.Run(() => _requestSender.OnProcessExited()).Wait();
+                Task.Run(() => _requestSender.OnProcessExited(), c).Wait(c);
 
                 Assert.IsTrue(c.IsCancellationRequested);
             }).Returns(Task.FromResult((Message)null));