Merge pull request #1664 from nunit/Issue-1590_NetAnalyzers

Enable Minimum .NET Analyzers

Author: CharliePoole

.editorconfig

@@ -13,4 +13,9 @@ indent_size = 4
 
 [*.{proj,csproj,vcxproj,xproj,json,config,nuspec,xml,yml}]
 indent_style = space
-indent_size = 2
+indent_size = 2
+
+[*.cs]
+# This relates to classes that have finalizers, but we should not have any finalizer
+# CA1816: Dispose methods should call SuppressFinalize
+dotnet_diagnostic.CA1816.severity = none

src/Directory.Build.props

@@ -6,6 +6,9 @@
         <LangVersion>12</LangVersion>
         <Features>strict</Features>
         <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+        <EnableNETAnalyzers>true</EnableNETAnalyzers>
+        <AnalysisMode>Minimum</AnalysisMode>
+        <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
         <Version Condition="'$(Version)'==''">4.0.0.0</Version>
         <OutputPath>$(MSBuildThisFileDirectory)\..\bin\$(Configuration)\</OutputPath>
         <CheckEolTargetFramework>false</CheckEolTargetFramework>

src/NUnitCommon/nunit.agent.core.tests/AgentOptionTests.cs

@@ -25,7 +25,7 @@ public void DefaultOptionSettings<T>(string propertyName, T defaultValue)
             var options = new AgentOptions();
             var prop = typeof(AgentOptions).GetProperty(propertyName);
             Assert.That(prop, Is.Not.Null, $"Property {propertyName} does not exist");
-            Assert.That(prop.GetValue(options, new object[0]), Is.EqualTo(defaultValue));
+            Assert.That(prop.GetValue(options, Array.Empty<object>()), Is.EqualTo(defaultValue));
         }
 
         static readonly Guid AGENT_GUID = Guid.NewGuid();
@@ -53,7 +53,7 @@ public void ValidOptionSettings<T>(string option, string propertyName, T expecte
             var options = new AgentOptions(option);
             var prop = typeof(AgentOptions).GetProperty(propertyName);
             Assert.That(prop, Is.Not.Null, $"Property {propertyName} does not exist");
-            Assert.That(prop.GetValue(options, new object[0]), Is.EqualTo(expectedValue));
+            Assert.That(prop.GetValue(options, Array.Empty<object>()), Is.EqualTo(expectedValue));
         }
 
         [Test]

src/NUnitCommon/nunit.agent.core.tests/Runners/DomainManagerTests.cs

@@ -71,7 +71,7 @@ public void CanCreateDomainWithApplicationBaseSpecified()
         public void CanUnloadDomain()
         {
             var domain = _domainManager.CreateDomain(_package);
-            _domainManager.Unload(domain);
+            DomainManager.Unload(domain);
 
             CheckDomainIsUnloaded(domain);
         }
@@ -80,14 +80,14 @@ public void CanUnloadDomain()
         public void UnloadingTwiceThrowsNUnitEngineUnloadException()
         {
             var domain = _domainManager.CreateDomain(_package);
-            _domainManager.Unload(domain);
+            DomainManager.Unload(domain);
 
-            Assert.That(() => _domainManager.Unload(domain), Throws.TypeOf<NUnitEngineUnloadException>());
+            Assert.That(() => DomainManager.Unload(domain), Throws.TypeOf<NUnitEngineUnloadException>());
 
             CheckDomainIsUnloaded(domain);
         }
 
-        private void CheckDomainIsUnloaded(AppDomain domain)
+        private static void CheckDomainIsUnloaded(AppDomain domain)
         {
             // HACK: Either the Assert will succeed or the
             // exception should be thrown.

src/NUnitCommon/nunit.agent.core.tests/Runners/TestAgentRunnerTests.cs

@@ -91,26 +91,26 @@ private void CheckPackageLoading()
                 Assert.That(_runner.IsPackageLoaded, Is.False, "Package should not be loaded automatically");
         }
 
-        private void CheckBasicResult(TestEngineResult result)
+        private static void CheckBasicResult(TestEngineResult result)
         {
             foreach (var node in result.XmlNodes)
                 CheckBasicResult(node);
         }
 
-        private void CheckBasicResult(XmlNode node)
+        private static void CheckBasicResult(XmlNode node)
         {
             Assert.That(node.Name, Is.EqualTo("test-suite"));
             Assert.That(node.GetAttribute("testcasecount", 0), Is.EqualTo(MockAssembly.Tests));
             Assert.That(node.GetAttribute("runstate"), Is.EqualTo("Runnable"));
         }
 
-        private void CheckRunResult(TestEngineResult result)
+        private static void CheckRunResult(TestEngineResult result)
         {
             foreach (var node in result.XmlNodes)
                 CheckRunResult(node);
         }
 
-        private void CheckRunResult(XmlNode result)
+        private static void CheckRunResult(XmlNode result)
         {
             CheckBasicResult(result);
             Assert.That(result.GetAttribute("passed", 0), Is.EqualTo(MockAssembly.PassedInAttribute));

src/NUnitCommon/nunit.agent.core/Agents/RemoteTestAgent.cs

@@ -26,8 +26,6 @@ public RemoteTestAgent(Guid agentId) : base(agentId) { }
 
         public ITestAgentTransport? Transport;
 
-        public int ProcessId => System.Diagnostics.Process.GetCurrentProcess().Id;
-
         public override bool Start()
         {
             Guard.OperationValid(Transport != null, "Transport must be set before calling Start().");

src/NUnitCommon/nunit.agent.core/Communication/Transports/Tcp/TestAgentTcpTransport.cs

@@ -17,6 +17,7 @@ namespace NUnit.Engine.Communication.Transports.Tcp
     public class TestAgentTcpTransport : ITestAgentTransport, ITestEventListener
     {
         private static readonly Logger log = InternalTrace.GetLogger(typeof(TestAgentTcpTransport));
+        private static readonly char[] PortSeparator = [':'];
 
         private readonly string _agencyUrl;
         private Socket? _clientSocket;
@@ -30,7 +31,7 @@ public TestAgentTcpTransport(RemoteTestAgent agent, string serverUrl)
             Guard.ArgumentNotNullOrEmpty(serverUrl, nameof(serverUrl));
             _agencyUrl = serverUrl;
 
-            var parts = serverUrl.Split(new char[] { ':' });
+            var parts = serverUrl.Split(PortSeparator);
             Guard.ArgumentValid(parts.Length == 2, "Invalid server address specified. Must be a valid endpoint including the port number", nameof(serverUrl));
             ServerEndPoint = new IPEndPoint(IPAddress.Parse(parts[0]), int.Parse(parts[1]));
         }

src/NUnitCommon/nunit.agent.core/Drivers/DriverService.cs

@@ -17,7 +17,9 @@ public class DriverService : IDriverService
     {
         static readonly Logger log = InternalTrace.GetLogger("DriverService");
 
-        readonly IList<IDriverFactory> _factories = new List<IDriverFactory>();
+        private static readonly char[] CommaSeparator = [','];
+
+        readonly List<IDriverFactory> _factories = new List<IDriverFactory>();
 
         public DriverService()
         {
@@ -63,7 +65,7 @@ public IFrameworkDriver GetDriver(AppDomain domain, TestPackage package, string
                 // any true Portable assembly would have a Profile as part of its name.
                 var platform = targetFramework == ".NETPortable,Version=v5.0"
                     ? ".NETStandard"
-                    : targetFramework.Split(new char[] { ',' })[0];
+                    : targetFramework.Split(CommaSeparator)[0];
 
                 if (platform == "Silverlight" || platform == ".NETPortable" || platform == ".NETStandard" || platform == ".NETCompactFramework")
                     if (skipNonTestAssemblies)

src/NUnitCommon/nunit.agent.core/Drivers/NUnit2DriverFactory.cs

@@ -45,7 +45,7 @@ public bool IsSupportedTestFramework(AssemblyName reference)
         public IFrameworkDriver GetDriver(AppDomain domain, string id, AssemblyName reference)
         {
             if (!IsSupportedTestFramework(reference))
-                throw new ArgumentException("Invalid framework", "reference");
+                throw new ArgumentException("Invalid framework", nameof(reference));
 
             if (!_resolverInstalled)
             {

src/NUnitCommon/nunit.agent.core/Drivers/NUnit3DriverFactory.cs

@@ -31,7 +31,7 @@ public bool IsSupportedTestFramework(AssemblyName reference)
         public IFrameworkDriver GetDriver(AppDomain domain, string id, AssemblyName reference)
         {
             Guard.ArgumentNotNullOrEmpty(id, nameof(id));
-            Guard.ArgumentValid(IsSupportedTestFramework(reference), "Invalid framework", "reference");
+            Guard.ArgumentValid(IsSupportedTestFramework(reference), "Invalid framework", nameof(reference));
 
             log.Info("Using NUnitFrameworkDriver");
             return new NUnitFrameworkDriver(domain, id, reference);

src/NUnitCommon/nunit.agent.core/Drivers/NUnitFrameworkApi2009.cs

@@ -48,15 +48,14 @@ public NUnitFrameworkApi2009(AppDomain testDomain, string driverId, AssemblyName
 
             public string Load(string testAssemblyPath, IDictionary<string, object> settings)
             {
-                Guard.ArgumentValid(File.Exists(testAssemblyPath), "Framework driver called with a file name that doesn't exist.", "testAssemblyPath");
+                Guard.ArgumentValid(File.Exists(testAssemblyPath), "Framework driver called with a file name that doesn't exist.", nameof(testAssemblyPath));
 
                 log.Info($"Loading {testAssemblyPath} - see separate log file");
 
                 _testAssemblyPath = testAssemblyPath;
 
                 // Normally, the caller should check for an invalid requested runtime, but we make sure here
-                var requestedRuntime = settings.ContainsKey(EnginePackageSettings.RequestedRuntimeFramework)
-                    ? settings[EnginePackageSettings.RequestedRuntimeFramework] : null;
+                settings.TryGetValue(EnginePackageSettings.RequestedRuntimeFramework, out object? requestedRuntime);
 
                 var idPrefix = _driverId + "-";
 

src/NUnitCommon/nunit.agent.core/Drivers/NUnitFrameworkDriver.cs

@@ -17,9 +17,9 @@ public class NUnitFrameworkDriver : IFrameworkDriver
         static readonly Version MINIMUM_NUNIT_VERSION = new(3, 2, 0);
         static readonly Logger log = InternalTrace.GetLogger(nameof(NUnitFrameworkDriver));
 
+#if NETFRAMEWORK
         readonly NUnitFrameworkApi _api;
 
-#if NETFRAMEWORK
         /// <summary>
         /// Construct an NUnitFrameworkDriver
         /// </summary>
@@ -82,6 +82,8 @@ internal NUnitFrameworkDriver(AppDomain testDomain, string api, string id, Assem
                 : new NUnitFrameworkApi2009(testDomain, ID, nunitRef);
         }
 #else
+        readonly NUnitFrameworkApi2018 _api;
+
         /// <summary>
         /// Construct an NUnitFrameworkDriver
         /// </summary>

src/NUnitCommon/nunit.agent.core/NUnitAgent.cs

@@ -19,7 +19,11 @@ public class NUnitAgent<TAgent>
     {
         static Process? AgencyProcess;
         static RemoteTestAgent? Agent;
+#if NET6_0_OR_GREATER
+        static readonly int _pid = Environment.ProcessId;
+#else
         static readonly int _pid = Process.GetCurrentProcess().Id;
+#endif
         static readonly Logger log = InternalTrace.GetLogger(typeof(TestAgent));
 
         /// <summary>

src/NUnitCommon/nunit.agent.core/RunTestsCallbackHandler.cs

@@ -41,7 +41,7 @@ private void ReportProgress(string state)
             _listener.OnTestEvent(state);
         }
 
-        private bool IsFinalResult(string eventArgument)
+        private static bool IsFinalResult(string eventArgument)
         {
             // TODO: If we add a prefix to the final result in the next framework
             // release, then we can immediately recognize the final result but we

src/NUnitCommon/nunit.agent.core/Runners/DomainManager.cs

@@ -59,7 +59,7 @@ public AppDomain CreateDomain( TestPackage package )
         }
 
         // Made separate and internal for testing
-        AppDomainSetup CreateAppDomainSetup(TestPackage package)
+        static AppDomainSetup CreateAppDomainSetup(TestPackage package)
         {
             AppDomainSetup setup = new AppDomainSetup();
 
@@ -99,7 +99,7 @@ AppDomainSetup CreateAppDomainSetup(TestPackage package)
             return setup;
         }
 
-        public void Unload(AppDomain domain)
+        public static void Unload(AppDomain domain)
         {
             new DomainUnloader(domain).Unload();
         }

src/NUnitCommon/nunit.agent.core/Runners/TestDomainRunner.cs

@@ -30,7 +30,7 @@ public override void Unload()
         {
             if (this.TestDomain != null)
             {
-                _domainManager.Unload(this.TestDomain);
+                DomainManager.Unload(this.TestDomain);
                 this.TestDomain = null;
             }
         }

src/NUnitCommon/nunit.agent.core/TestAssemblyResolver.cs

@@ -91,7 +91,7 @@ public void Dispose()
 
         private Assembly? OnResolving(AssemblyLoadContext loadContext, AssemblyName assemblyName)
         {
-            if (loadContext == null) throw new ArgumentNullException("context");
+            Guard.ArgumentNotNull(loadContext, nameof(loadContext));
 
             Assembly? loadedAssembly;
             foreach (var strategy in ResolutionStrategies)
@@ -152,7 +152,7 @@ bool FileMatchesAssembly(string fileName) =>
         public class RuntimeLibrariesStrategy : ResolutionStrategy
         {
             private DependencyContext? _dependencyContext;
-            private readonly ICompilationAssemblyResolver _assemblyResolver;
+            private readonly CompositeCompilationAssemblyResolver _assemblyResolver;
 
             public RuntimeLibrariesStrategy(AssemblyLoadContext loadContext, string testAssemblyPath)
             {
@@ -304,7 +304,7 @@ private static bool TryGetVersionFromString(string text, out Version newVersion)
             int len = 0;
             foreach (char c in text)
             {
-                if (VERSION_CHARS.IndexOf(c) >= 0)
+                if (VERSION_CHARS.Contains(c))
                     len++;
                 else
                     break;

src/NUnitCommon/nunit.common.tests/FileSystemAccess/DirectoryFinderTests.cs

@@ -441,15 +441,15 @@ public void GetDirectories_StartDirectoryIsNull()
         {
             var finder = new DirectoryFinder(Substitute.For<IFileSystem>());
 
-            Assert.That(() => finder.GetDirectories((IDirectory)null!, "notused"), Throws.ArgumentNullException.With.Message.Contains(" startDirectory "));
+            Assert.That(() => finder.GetDirectories((IDirectory)null!, "notused"), Throws.ArgumentNullException.With.Message.Contains("startDirectory"));
         }
 
         [Test]
         public void GetDirectories_PatternIsNull()
         {
             var finder = new DirectoryFinder(Substitute.For<IFileSystem>());
 
-            Assert.That(() => finder.GetDirectories(Substitute.For<IDirectory>(), null!), Throws.ArgumentNullException.With.Message.Contains(" pattern "));
+            Assert.That(() => finder.GetDirectories(Substitute.For<IDirectory>(), null!), Throws.ArgumentNullException.With.Message.Contains("pattern"));
         }
 
         [Test]
@@ -594,23 +594,23 @@ public void GetFiles_StartDirectoryIsNull()
         {
             var finder = new DirectoryFinder(Substitute.For<IFileSystem>());
 
-            Assert.That(() => finder.GetFiles((IDirectory)null!, "notused"), Throws.ArgumentNullException.With.Message.Contains(" startDirectory "));
+            Assert.That(() => finder.GetFiles((IDirectory)null!, "notused"), Throws.ArgumentNullException.With.Message.Contains("startDirectory"));
         }
 
         [Test]
         public void GetFiles_PatternIsNull()
         {
             var finder = new DirectoryFinder(Substitute.For<IFileSystem>());
 
-            Assert.That(() => finder.GetDirectories(Substitute.For<IDirectory>(), null!), Throws.ArgumentNullException.With.Message.Contains(" pattern "));
+            Assert.That(() => finder.GetDirectories(Substitute.For<IDirectory>(), null!), Throws.ArgumentNullException.With.Message.Contains("pattern"));
         }
 
         [Test]
         public void GetFiles_PatternIsEmpty()
         {
             var finder = new DirectoryFinder(Substitute.For<IFileSystem>());
 
-            Assert.That(() => finder.GetFiles(Substitute.For<IDirectory>(), string.Empty), Throws.ArgumentException.With.Message.Contains(" pattern "));
+            Assert.That(() => finder.GetFiles(Substitute.For<IDirectory>(), string.Empty), Throws.ArgumentException.With.Message.Contains("pattern"));
         }
 
         private static string CreateAbsolutePath(IEnumerable<string> parts)

src/NUnitCommon/nunit.common.tests/FileSystemAccess/FileSystemTests.cs

@@ -38,7 +38,7 @@ public void GetDirectory_DirectoryDoesNotExist()
         [Test]
         public void GetFile()
         {
-            var path = this.GetTestFileLocation();
+            var path = GetTestFileLocation();
             var parent = SIO.Path.GetDirectoryName(path);
             var fileSystem = new FileSystem();
 
@@ -66,7 +66,7 @@ public void GetFile_DirectoryDoesNotExist()
         [Test]
         public void Exists_FileExists()
         {
-            var path = this.GetTestFileLocation();
+            var path = GetTestFileLocation();
             var file = new File(path);
             var fileSystem = new FileSystem();
 
@@ -78,7 +78,7 @@ public void Exists_FileExists()
         [Test]
         public void Exists_FileDoesNotExist()
         {
-            var path = this.GetTestFileLocation();
+            var path = GetTestFileLocation();
             while (SIO.File.Exists(path))
             {
                 path += "x";
@@ -137,7 +137,7 @@ public void Exists_DirectoryIsNull()
             Assert.That(() => fileSystem.Exists((IDirectory)null!), Throws.ArgumentNullException);
         }
 
-        private string GetTestFileLocation()
+        private static string GetTestFileLocation()
         {
             return Assembly.GetAssembly(typeof(FileTests))!.Location;
         }