Fixes from code review

nunit · Apr 25, 2021 · 71cca23 · 71cca23
1 parent 924d0cf
commit 71cca23
Show file tree

Hide file tree

Showing 20 changed files with 61 additions and 45 deletions.
diff --git a/package-tests.cake b/package-tests.cake
@@ -173,7 +173,7 @@ public abstract class NetFXPackageTester : PackageTester
         PackageTests.Add(new PackageTest(
             "netcoreapp3.1",
             "Run mock-assembly.dll under .NET Core 3.1",
-            "netcoreapp3.1/mock-assembly.dll --trace:Debug",
+            "netcoreapp3.1/mock-assembly.dll",
             new ExpectedResult("Failed")
             {
                 Total = 37,

diff --git a/src/NUnitEngine/nunit-agent-x86/Properties/AssemblyInfo.cs b/src/NUnitEngine/nunit-agent-x86/Properties/AssemblyInfo.cs
@@ -5,7 +5,7 @@
 // set of attributes. Change these attribute values to modify the information
 // associated with an assembly.
 [assembly: AssemblyTitle("NUnit Agent (X86)")]
-[assembly: AssemblyDescription("Runs tests in a separate process when necessary")]
+[assembly: AssemblyDescription("An agent to run tests in a child process created by the engine")]
 [assembly: AssemblyCulture("")]
 
 // Setting ComVisible to false makes the types in this assembly not visible 

diff --git a/src/NUnitEngine/nunit-agent/Program.cs b/src/NUnitEngine/nunit-agent/Program.cs
@@ -3,6 +3,7 @@
 using System;
 using System.Diagnostics;
 using System.IO;
+using System.Runtime.InteropServices;
 using System.Security;
 using NUnit.Common;
 using NUnit.Engine;
@@ -26,7 +27,6 @@ public class NUnitTestAgent
         [STAThread]
         public static void Main(string[] args)
         {
-            Console.WriteLine("Agent Process Starting");
             AgentId = new Guid(args[0]);
             AgencyUrl = args[1];
 
@@ -71,10 +71,12 @@ public static void Main(string[] args)
 
             LocateAgencyProcess(agencyPid);
 
-#if NETFRAMEWORK
-            log.Info("Running under version {0}, {1}",
-                Environment.Version,
-                RuntimeFramework.CurrentFramework.DisplayName);
+#if NETCOREAPP3_1
+            log.Info($"Running .NET Core 3.1 agent under {RuntimeInformation.FrameworkDescription}");
+#elif NET40
+            log.Info($"Running .NET 4.0 agent under {RuntimeFramework.CurrentFramework.DisplayName}");
+#elif NET20
+            log.Info($"Running .NET 2.0 agent under {RuntimeFramework.CurrentFramework.DisplayName}");
 #endif
 
             // Create CoreEngine

diff --git a/src/NUnitEngine/nunit-agent/Properties/AssemblyInfo.cs b/src/NUnitEngine/nunit-agent/Properties/AssemblyInfo.cs
@@ -5,7 +5,7 @@
 // set of attributes. Change these attribute values to modify the information
 // associated with an assembly.
 [assembly: AssemblyTitle("NUnit Agent")]
-[assembly: AssemblyDescription("Runs tests in a separate process when necessary")]
+[assembly: AssemblyDescription("An agent to run tests in a child process created by the engine")]
 [assembly: AssemblyCulture("")]
 
 // Setting ComVisible to false makes the types in this assembly not visible 

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Messages/CommandMessage.cs b/src/NUnitEngine/nunit.engine.core/Communication/Messages/CommandMessage.cs
@@ -4,9 +4,7 @@
 
 namespace NUnit.Engine.Communication.Messages
 {
-#if !NETSTANDARD1_6
     [Serializable]
-#endif
     public class CommandMessage : TestEngineMessage
     {
         public CommandMessage(string commandName, params object[] arguments)

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Messages/CommandReturnMessage.cs b/src/NUnitEngine/nunit.engine.core/Communication/Messages/CommandReturnMessage.cs
@@ -4,9 +4,7 @@
 
 namespace NUnit.Engine.Communication.Messages
 {
-#if !NETSTANDARD1_6
     [Serializable]
-#endif
     public class CommandReturnMessage : TestEngineMessage
     {
         public CommandReturnMessage(object returnValue)

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Messages/ProgressMessage.cs b/src/NUnitEngine/nunit.engine.core/Communication/Messages/ProgressMessage.cs
@@ -4,9 +4,7 @@
 
 namespace NUnit.Engine.Communication.Messages
 {
-#if !NETSTANDARD1_6
     [Serializable]
-#endif
     public class ProgressMessage : TestEngineMessage
     {
         public ProgressMessage(string report)

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Messages/TestEngineMessage.cs b/src/NUnitEngine/nunit.engine.core/Communication/Messages/TestEngineMessage.cs
@@ -4,9 +4,7 @@
 
 namespace NUnit.Engine.Communication.Messages
 {
-#if !NETSTANDARD1_6
     [Serializable]
-#endif
     public abstract class TestEngineMessage
     {
     }

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Protocols/BinarySerializationProtocol.cs b/src/NUnitEngine/nunit.engine.core/Communication/Protocols/BinarySerializationProtocol.cs
@@ -1,6 +1,5 @@
 // Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt
 
-#if !NETSTANDARD1_6
 using System;
 using System.Collections.Generic;
 using System.IO;
@@ -246,4 +245,3 @@ private static byte[] ReadByteArray(Stream stream, int length)
         }
     }
 }
-#endif
diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Transports/ITestAgentTransport.cs b/src/NUnitEngine/nunit.engine.core/Communication/Transports/ITestAgentTransport.cs
@@ -4,6 +4,10 @@
 
 namespace NUnit.Engine.Communication.Transports
 {
+    /// <summary>
+    /// The ITestAgentTransport interface is implemented by a
+    /// class providing communication for a TestAgent.
+    /// </summary>
     public interface ITestAgentTransport : ITransport
     {
         TestAgent Agent { get; }

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Transports/ITransport.cs b/src/NUnitEngine/nunit.engine.core/Communication/Transports/ITransport.cs
@@ -2,6 +2,10 @@
 
 namespace NUnit.Engine.Communication.Transports
 {
+    /// <summary>
+    /// The ITransport interface is implemented by a class
+    /// providing a communication interface for another class.
+    /// </summary>
     public interface ITransport
     {
         bool Start();

diff --git a/...tEngine/nunit.engine.core/Communication/Transports/Remoting/TestAgentRemotingTransport.cs b/...tEngine/nunit.engine.core/Communication/Transports/Remoting/TestAgentRemotingTransport.cs
@@ -12,6 +12,11 @@
 namespace NUnit.Engine.Communication.Transports.Remoting
 {
 
+    /// <summary>
+    /// TestAgentRemotingTransport uses remoting to support
+    /// a TestAgent in communicating with a TestAgency and
+    /// with the runners that make use of it.
+    /// </summary>
     public class TestAgentRemotingTransport : MarshalByRefObject, ITestAgentTransport, ITestAgent, ITestEngineRunner
     {
         private static readonly Logger log = InternalTrace.GetLogger(typeof(TestAgentRemotingTransport));

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Transports/Tcp/SocketReader.cs b/src/NUnitEngine/nunit.engine.core/Communication/Transports/Tcp/SocketReader.cs
@@ -8,6 +8,10 @@
 
 namespace NUnit.Engine.Communication.Transports.Tcp
 {
+    /// <summary>
+    /// SocketReader reads a socket and composes Messages
+    /// for consumption using a specific wire protocol.
+    /// </summary>
     public class SocketReader
     {
         private const int BUFFER_SIZE = 1024;

diff --git a/src/NUnitEngine/nunit.engine.core/Communication/Transports/Tcp/TestAgentTcpTransport.cs b/src/NUnitEngine/nunit.engine.core/Communication/Transports/Tcp/TestAgentTcpTransport.cs
@@ -1,6 +1,5 @@
 // Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt
 
-#if !NETSTANDARD1_6
 using System.Net;
 using System.Net.Sockets;
 using System.Threading;
@@ -12,6 +11,11 @@
 
 namespace NUnit.Engine.Communication.Transports.Tcp
 {
+    /// <summary>
+    /// TestAgentRemotingTransport uses TCP to support
+    /// a TestAgent in communicating with a TestAgency and
+    /// with the runners that make use of it.
+    /// </summary>
     public class TestAgentTcpTransport : ITestAgentTransport, ITestEventListener
     {
         private static readonly Logger log = InternalTrace.GetLogger(typeof(TestAgentTcpTransport));
@@ -131,4 +135,3 @@ public void OnTestEvent(string report)
         }
     }
 }
-#endif
diff --git a/src/NUnitEngine/nunit.engine.core/RuntimeFramework.cs b/src/NUnitEngine/nunit.engine.core/RuntimeFramework.cs
@@ -143,6 +143,7 @@ private Version GetClrVersionForFramework(Version frameworkVersion)
                     {
                         case 1:
                         case 2:
+                            // For pre-3.0 versions of .NET Core, Environment.Version returns 4.0.30319.42000
                             return new Version(4, 0, 30319);
                         case 3:
                             return new Version(3, 1, 10);
@@ -698,7 +699,7 @@ private static void FindDotNetCoreFrameworks()
             if (!File.Exists(Path.Combine(INSTALL_DIR, "dotnet.exe")))
                 return;
 
-            string runtimeDir = Path.Combine(INSTALL_DIR, "shared\\Microsoft.NETCore.App");
+            string runtimeDir = Path.Combine(INSTALL_DIR, Path.Combine("shared", "Microsoft.NETCore.App"));
             if (!Directory.Exists(runtimeDir))
                 return;
 
@@ -711,7 +712,7 @@ private static void FindDotNetCoreFrameworks()
             _availableFrameworks.AddRange(runtimes);
         }
 
-        // Internal for testing
+        // Deal with oddly named directories, which may sometimes appear when previews are installed
         internal static IList<RuntimeFramework> GetNetCoreRuntimesFromDirectoryNames(IEnumerable<string> dirNames)
         {
             const string VERSION_CHARS = ".0123456789";

diff --git a/src/NUnitEngine/nunit.engine/Communication/Transports/ITestAgencyTransport.cs b/src/NUnitEngine/nunit.engine/Communication/Transports/ITestAgencyTransport.cs
@@ -2,6 +2,10 @@
 
 namespace NUnit.Engine.Communication.Transports
 {
+    /// <summary>
+    /// The ITestAgencyTransport interface is implemented by a
+    /// class providing communication for a TestAgency.
+    /// </summary>
     public interface ITestAgencyTransport : ITransport
     {
         string ServerUrl { get; }

diff --git a/...NUnitEngine/nunit.engine/Communication/Transports/Remoting/TestAgencyRemotingTransport.cs b/...NUnitEngine/nunit.engine/Communication/Transports/Remoting/TestAgencyRemotingTransport.cs
@@ -15,6 +15,10 @@
 
 namespace NUnit.Engine.Communication.Transports.Remoting
 {
+    /// <summary>
+    /// TestAgencyRemotingTransport uses the remoting to connect a
+    /// TestAgency with its agents.
+    /// </summary>
     public class TestAgencyRemotingTransport : MarshalByRefObject, ITestAgencyTransport, ITestAgency, IDisposable
     {
         private static readonly Logger log = InternalTrace.GetLogger(typeof(TestAgencyRemotingTransport));

diff --git a/src/NUnitEngine/nunit.engine/Communication/Transports/Tcp/TcpServer.cs b/src/NUnitEngine/nunit.engine/Communication/Transports/Tcp/TcpServer.cs
@@ -10,7 +10,7 @@ namespace NUnit.Engine.Communication.Transports.Tcp
 {
     public class TcpServer
     {
-        private static readonly Logger log = InternalTrace.GetLogger(typeof(TestAgentTcpTransport));
+        private static readonly Logger log = InternalTrace.GetLogger(typeof(TcpServer));
 
         private const int GUID_BUFFER_SIZE = 16;
 

diff --git a/src/NUnitEngine/nunit.engine/Communication/Transports/Tcp/TestAgencyTcpTransport.cs b/src/NUnitEngine/nunit.engine/Communication/Transports/Tcp/TestAgencyTcpTransport.cs
@@ -9,7 +9,8 @@
 namespace NUnit.Engine.Communication.Transports.Tcp
 {
     /// <summary>
-    /// Summary description for TestAgencyTcpTransport.
+    /// TestAgencyTcpTransport uses the TCP protocol to connect a
+    /// TestAgency with its agents.
     /// </summary>
     public class TestAgencyTcpTransport : ITestAgencyTransport, ITestAgency, IDisposable
     {

diff --git a/src/NUnitEngine/nunit.engine/Services/RuntimeFrameworkService.cs b/src/NUnitEngine/nunit.engine/Services/RuntimeFrameworkService.cs
@@ -53,40 +53,34 @@ public bool IsAvailable(string name)
 
         private static readonly Version AnyVersion = new Version(0, 0);
 
-        private static bool FrameworksMatch(RuntimeFramework f1, RuntimeFramework f2)
+        private static bool FrameworksMatch(RuntimeFramework requested, RuntimeFramework available)
         {
-            var rt1 = f1.Runtime;
-            var rt2 = f2.Runtime;
-
-            if (!RuntimesMatch(rt1, rt2))
+            if (!RuntimesMatch(requested.Runtime, available.Runtime))
                 return false;
 
-            var v1 = f1.ClrVersion;
-            var v2 = f2.ClrVersion;
+            var requestedVersion = requested.FrameworkVersion;
+            var availableVersion = available.FrameworkVersion;
 
-            if (v1 == AnyVersion || v2 == AnyVersion)
+            if (requestedVersion == AnyVersion)
                 return true;
 
-            return v1.Major == v2.Major &&
-                   v1.Minor == v2.Minor &&
-                   (v1.Build < 0 || v2.Build < 0 || v1.Build == v2.Build) &&
-                   (v1.Revision < 0 || v2.Revision < 0 || v1.Revision == v2.Revision) &&
-                   f1.FrameworkVersion.Major == f2.FrameworkVersion.Major &&
-                   f1.FrameworkVersion.Minor == f2.FrameworkVersion.Minor;
+            return requestedVersion.Major == availableVersion.Major &&
+                   requestedVersion.Minor == availableVersion.Minor &&
+                   (requestedVersion.Build < 0 || availableVersion.Build < 0 || requestedVersion.Build == availableVersion.Build) &&
+                   (requestedVersion.Revision < 0 || availableVersion.Revision < 0 || requestedVersion.Revision == availableVersion.Revision) &&
+                   requestedVersion.Major == availableVersion.Major &&
+                   requestedVersion.Minor == availableVersion.Minor;
         }
 
-        private static bool RuntimesMatch(RuntimeType rt1, RuntimeType rt2)
+        private static bool RuntimesMatch(RuntimeType requested, RuntimeType available)
         {
-            if (rt1 == rt2)
-                return true;
-
-            if (rt1 == RuntimeType.Any || rt2 == RuntimeType.Any)
+            if (requested == available || requested == RuntimeType.Any)
                 return true;
 
-            if (rt1 == RuntimeType.Net && rt2 == RuntimeType.Mono)
+            if (requested == RuntimeType.Net && available == RuntimeType.Mono)
                 return true;
 
-            if (rt1 == RuntimeType.Mono && rt2 == RuntimeType.Net)
+            if (requested == RuntimeType.Mono && available == RuntimeType.Net)
                 return true;
 
             return false;