Changes from review

nunit · CharliePoole · Jan 20, 2025 · Jan 4, 2025 · Jan 5, 2025 · Jan 6, 2025
commit ff04e8e55c066620269d7ba6bd8d1f495ce8462a
diff --git a/package-tests.cake b/package-tests.cake
@@ -158,7 +158,7 @@ StandardRunnerTests.Add(new PackageTest(
 StandardRunnerTests.Add(new PackageTest(
     1, "NUnit301Test",
     "Run a test under NUnit 3.0.1 using 2009 API",
-    "testdata/NUnit3.0/net462/NUnit3.0.1.dll",
+    "testdata/NUnit3.0.1/net462/NUnit3.0.1.dll",
     new ExpectedResult("Passed")
     {
         Assemblies = new[] { new ExpectedAssemblyResult("NUnit3.0.1.dll", "net462") }
@@ -167,7 +167,7 @@ StandardRunnerTests.Add(new PackageTest(
 StandardRunnerTests.Add(new PackageTest(
     1, "NUnit32Test",
     "Run a test under NUnit 3.2 using 20018 API",
-    "testdata/NUnit3.0/net462/NUnit3.2.dll",
+    "testdata/NUnit3.2/net462/NUnit3.2.dll",
     new ExpectedResult("Passed")
     {
         Assemblies = new[] { new ExpectedAssemblyResult("NUnit3.2.dll", "net462") }
@@ -176,7 +176,7 @@ StandardRunnerTests.Add(new PackageTest(
 StandardRunnerTests.Add(new PackageTest(
     1, "NUnit310Test",
     "Run a test under NUnit 3.10 using 2018 API",
-    "testdata/NUnit3.0/net462/NUnit3.10.dll",
+    "testdata/NUnit3.10/net462/NUnit3.10.dll",
     new ExpectedResult("Passed")
     {
         Assemblies = new[] { new ExpectedAssemblyResult("NUnit3.10.dll", "net462") }

diff --git a/src/NUnitEngine/nunit.engine.api/Extensibility/IFrameworkDriver.cs b/src/NUnitEngine/nunit.engine.api/Extensibility/IFrameworkDriver.cs
@@ -18,7 +18,7 @@ public interface IFrameworkDriver
         /// Gets and sets the unique identifier for this driver,
         /// used to ensure that test ids are unique across drivers.
         /// </summary>
-        string ID { get; set; }
+        string ID { get; }
 
         /// <summary>
         /// Loads the tests in an assembly.

diff --git a/src/NUnitEngine/nunit.engine.core.tests/Drivers/NUnitFrameworkDriverTests.cs b/src/NUnitEngine/nunit.engine.core.tests/Drivers/NUnitFrameworkDriverTests.cs
@@ -126,14 +126,41 @@ public void RunTestsAction_WithoutLoad_ThrowsInvalidOperationException()
         }
 
 #if NETFRAMEWORK
-        //[Test]
-        public void RunTestsAction_WithInvalidFilterElement_ThrowsNUnitEngineException()
+        // Nested Class tests Api Selection in the driver
+        public class ApiSelectionTests()
+        {
+            [TestCase("4.2.2", "2018")]
+            [TestCase("3.14.0", "2018")]
+            [TestCase("3.2.0", "2018")]
+            [TestCase("3.0.1", "2009")]
+            [TestCase("3.0.0", "2009")]
+            public void CorrectApiIsSelected(string nunitVersion, string apiVersion)
+            {
+                var driver = new NUnitFrameworkDriver(AppDomain.CurrentDomain, "99", new AssemblyName()
+                {
+                    Name = "nunit.framework",
+                    Version = new Version(nunitVersion)
+                });
+
+                Assert.That(driver.API, Is.EqualTo(apiVersion));
+            }
+        }
+
+        [Test]
+        public void RunTestsAction_WithInvalidFilterElement_ThrowsException()
         {
             _driver.Load(_mockAssemblyPath, _settings);
 
             var invalidFilter = "<filter><invalidElement>foo</invalidElement></filter>";
             var ex = Assert.Catch(() => _driver.Run(new NullListener(), invalidFilter));
-            Assert.That(ex, Is.TypeOf<NUnitEngineException>());
+
+            if (_whichApi == "2018")
+            {
+                Assert.That(ex, Is.TypeOf<TargetInvocationException>());
+                Assert.That(ex.InnerException, Is.TypeOf<ArgumentException>());
+            }
+            else
+                Assert.That(ex, Is.TypeOf<NUnitEngineException>());
         }
 
         private class CallbackEventHandler : System.Web.UI.ICallbackEventHandler

diff --git a/src/NUnitEngine/nunit.engine.core.tests/Drivers/NotRunnableFrameworkDriverTests.cs b/src/NUnitEngine/nunit.engine.core.tests/Drivers/NotRunnableFrameworkDriverTests.cs
@@ -14,7 +14,6 @@ namespace NUnit.Engine.Drivers
     // Functional tests of the NUnitFrameworkDriver calling into the framework.
     public abstract class NotRunnableFrameworkDriverTests
     {
-        private const string DRIVER_ID = "99";
         private const string EXPECTED_ID = "99-1";
 
         protected string? _expectedRunState;
@@ -95,7 +94,6 @@ public void Run(string filePath, string expectedType)
         private IFrameworkDriver GetDriver(string filePath)
         {
             IFrameworkDriver driver = CreateDriver(filePath);
-            driver.ID = DRIVER_ID;
             return driver;
         }
 
@@ -126,7 +124,7 @@ public InvalidAssemblyFrameworkDriverTests()
 
         protected override IFrameworkDriver CreateDriver(string filePath)
         {
-            return new InvalidAssemblyFrameworkDriver(filePath, _expectedReason ?? "Not Specified");
+            return new InvalidAssemblyFrameworkDriver(filePath, "99", _expectedReason ?? "Not Specified");
         }
     }
 
@@ -142,7 +140,7 @@ public SkippedAssemblyFrameworkDriverTests()
 
         protected override IFrameworkDriver CreateDriver(string filePath)
         {
-            return new SkippedAssemblyFrameworkDriver(filePath);
+            return new SkippedAssemblyFrameworkDriver(filePath, "99");
         }
     }
 }
diff --git a/src/NUnitEngine/nunit.engine.core.tests/Runners/TestAgentRunnerTests.cs b/src/NUnitEngine/nunit.engine.core.tests/Runners/TestAgentRunnerTests.cs
@@ -69,7 +69,7 @@ public void Run()
             CheckPackageLoading();
         }
 
-        //[Test]
+        [Test]
         public void RunAsync()
         {
             var asyncResult = _runner.RunAsync(null, TestFilter.Empty);

diff --git a/src/NUnitEngine/nunit.engine.core.tests/Services/DriverServiceTests.cs b/src/NUnitEngine/nunit.engine.core.tests/Services/DriverServiceTests.cs
@@ -7,7 +7,7 @@
 using NUnit.Engine.Drivers;
 using NUnit.Engine.Extensibility;
 
-namespace NUnit.Engine.Services.Tests
+namespace NUnit.Engine.Services
 {
     [TestFixture]
     public class DriverServiceTests
@@ -30,11 +30,10 @@ public void CorrectDriverIsUsed(string fileName, bool skipNonTestAssemblies, Typ
 
         static TestCaseData[] DriverSelectionTestCases = new[]
         {
-            // TODO: make commented tests work
             new TestCaseData("mock-assembly.dll", false, typeof(NUnitFrameworkDriver)),
             new TestCaseData("mock-assembly.dll", true, typeof(NUnitFrameworkDriver)),
-            //new TestCaseData("notest-assembly.dll", false, typeof(NUnitFrameworkDriver)),
-            //new TestCaseData"notest-assembly.dll", true, typeof(SkippedAssemblyFrameworkDriver))
+            new TestCaseData("notest-assembly.dll", false, typeof(NUnitFrameworkDriver)).Ignore("Assembly not present"),
+            new TestCaseData("notest-assembly.dll", true, typeof(SkippedAssemblyFrameworkDriver)).Ignore("Assembly not present"),
 
             // Invalid cases should work with all target runtimes
             new TestCaseData("mock-assembly.pdb", false, typeof(InvalidAssemblyFrameworkDriver)),

diff --git a/src/NUnitEngine/nunit.engine.core/Drivers/DriverService.cs b/src/NUnitEngine/nunit.engine.core/Drivers/DriverService.cs
@@ -54,10 +54,10 @@ public DriverService()
         public IFrameworkDriver GetDriver(AppDomain domain, TestPackage package, string assemblyPath, string? targetFramework, bool skipNonTestAssemblies)
         {
             if (!File.Exists(assemblyPath))
-                return new InvalidAssemblyFrameworkDriver(assemblyPath, "File not found: " + assemblyPath);
+                return new InvalidAssemblyFrameworkDriver(assemblyPath, package.ID, "File not found: " + assemblyPath);
 
             if (!PathUtils.IsAssemblyFileType(assemblyPath))
-                return new InvalidAssemblyFrameworkDriver(assemblyPath, "File type is not supported");
+                return new InvalidAssemblyFrameworkDriver(assemblyPath, package.ID, "File type is not supported");
 
             if (targetFramework != null)
             {
@@ -70,9 +70,9 @@ public IFrameworkDriver GetDriver(AppDomain domain, TestPackage package, string
 
                 if (platform == "Silverlight" || platform == ".NETPortable" || platform == ".NETStandard" || platform == ".NETCompactFramework")
                     if (skipNonTestAssemblies)
-                        return new SkippedAssemblyFrameworkDriver(assemblyPath);
+                        return new SkippedAssemblyFrameworkDriver(assemblyPath, "X");
                     else
-                        return new InvalidAssemblyFrameworkDriver(assemblyPath, platform + 
+                        return new InvalidAssemblyFrameworkDriver(assemblyPath, package.ID, platform + 
                             " test assemblies are not supported by this version of the engine");
             }
 
@@ -84,7 +84,7 @@ public IFrameworkDriver GetDriver(AppDomain domain, TestPackage package, string
                     {
                         foreach (var attr in assemblyDef.CustomAttributes)
                             if (attr.AttributeType.FullName == "NUnit.Framework.NonTestAssemblyAttribute")
-                                return new SkippedAssemblyFrameworkDriver(assemblyPath);
+                                return new SkippedAssemblyFrameworkDriver(assemblyPath, package.ID);
                     }
 
                     foreach (var factory in _factories)
@@ -108,14 +108,14 @@ public IFrameworkDriver GetDriver(AppDomain domain, TestPackage package, string
             }
             catch (BadImageFormatException ex)
             {
-                return new InvalidAssemblyFrameworkDriver(assemblyPath, ex.Message);
+                return new InvalidAssemblyFrameworkDriver(assemblyPath, package.ID, ex.Message);
             }
 
             if (skipNonTestAssemblies)
-                return new SkippedAssemblyFrameworkDriver(assemblyPath);
+                return new SkippedAssemblyFrameworkDriver(assemblyPath, package.ID);
             else
-                return new InvalidAssemblyFrameworkDriver(assemblyPath, string.Format("No suitable tests found in '{0}'.\n" +
-                                                                              "Either assembly contains no tests or proper test driver has not been found.", assemblyPath));
+                return new InvalidAssemblyFrameworkDriver(assemblyPath, package.ID, 
+                    $"No suitable tests found in '{assemblyPath}'.\r\nEither assembly contains no tests or proper test driver has not been found.");
         }
     }
 }
diff --git a/src/NUnitEngine/nunit.engine.core/Drivers/NUnit3DriverFactory.cs b/src/NUnitEngine/nunit.engine.core/Drivers/NUnit3DriverFactory.cs
@@ -46,6 +46,7 @@ public IFrameworkDriver GetDriver(AppDomain domain, string id, AssemblyName refe
         /// <returns></returns>
         public IFrameworkDriver GetDriver(string id, AssemblyName reference)
         {
+            Guard.ArgumentNotNullOrEmpty(id, nameof(id));
             Guard.ArgumentValid(IsSupportedTestFramework(reference), "Invalid framework", "reference");
             log.Info("Using NUnitFrameworkDriver");
             return new NUnitFrameworkDriver(id, reference);

diff --git a/src/NUnitEngine/nunit.engine.core/Drivers/NUnitFrameworkApi.cs b/src/NUnitEngine/nunit.engine.core/Drivers/NUnitFrameworkApi.cs
@@ -21,13 +21,18 @@ public interface NUnitFrameworkApi
         int CountTestCases(string filter);
 
         /// <summary>
-        /// Executes the tests in an assembly.
+        /// Executes the tests in an assembly synchronously.
         /// </summary>
         /// <param name="listener">An ITestEventHandler that receives progress notices</param>
         /// <param name="filter">A XML string representing the filter that controls which tests are executed</param>
         /// <returns>An Xml string representing the result</returns>
         string Run(ITestEventListener? listener, string filter);
 
+        /// <summary>
+        /// Executes the tests in an assembly asynchronously.
+        /// </summary>
+        /// <param name="callback">A callback that receives XML progress notices</param>
+        /// <param name="filter">A filter that controls which tests are executed</param>
         void RunAsync(Action<string> callback, string filter);
 
         /// <summary>

diff --git a/src/NUnitEngine/nunit.engine.core/Drivers/NUnitFrameworkApi2009.cs b/src/NUnitEngine/nunit.engine.core/Drivers/NUnitFrameworkApi2009.cs
@@ -1,5 +1,6 @@
 // Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt
 
+#if NETFRAMEWORK
 using System;
 using System.Collections.Generic;
 using System.IO;
@@ -10,7 +11,6 @@
 
 namespace NUnit.Engine.Drivers
 {
-#if NETFRAMEWORK
         /// <summary>
         /// This is the original NUnit 3 API, which only works for .NET Framework.
         /// As far as I can discover, it first appeared in pre-release 2.9.1,
@@ -27,15 +27,15 @@ class NUnitFrameworkApi2009 : NUnitFrameworkApi
 
             const string CONTROLLER_TYPE = "NUnit.Framework.Api.FrameworkController";
 
-            string _driverId;
+            private readonly string _driverId;
 
-            AppDomain _testDomain;
-            AssemblyName _nunitRef;
+            private readonly AppDomain _testDomain;
+            private readonly AssemblyName _nunitRef;
 
-            string? _testAssemblyPath;
+            private string? _testAssemblyPath;
 
-            object? _frameworkController;
-            Type? _frameworkControllerType;
+            private object? _frameworkController;
+            private Type? _frameworkControllerType;
 
             public NUnitFrameworkApi2009(AppDomain testDomain, string driverId, AssemblyName nunitRef)
             {
@@ -60,11 +60,19 @@ public string Load(string testAssemblyPath, IDictionary<string, object> settings
                 var requestedRuntime = settings.ContainsKey(EnginePackageSettings.RequestedRuntimeFramework)
                     ? settings[EnginePackageSettings.RequestedRuntimeFramework] : null;
 
-                var idPrefix = string.IsNullOrEmpty(_driverId) ? "" : _driverId + "-";
+                var idPrefix = _driverId + "-";
 
                 try
                 {
-                    _frameworkController = CreateObject(CONTROLLER_TYPE, _testAssemblyPath, idPrefix, settings);
+                _frameworkController = _testDomain.CreateInstanceAndUnwrap(
+                    _nunitRef.FullName,
+                    CONTROLLER_TYPE,
+                    false,
+                    0,
+                    null,
+                    new object[] { _testAssemblyPath, idPrefix, settings },
+                    null,
+                    null).ShouldNotBeNull();
                 }
                 catch (BadImageFormatException ex) when (requestedRuntime != null)
                 {
@@ -144,14 +152,16 @@ private object CreateObject(string typeName, params object?[]? args)
             {
                 try
                 {
-                    return _testDomain.CreateInstanceAndUnwrap(
-                        _nunitRef.FullName, typeName, false, 0, null, args, null, null)!;
+                return _testDomain.CreateInstanceAndUnwrap(
+                    _nunitRef.FullName, typeName, false, 0, null, args, null, null)!;
+                //return _testDomain.CreateInstanceAndUnwrap(
+                //        _nunitRef.FullName, typeName, false, 0, null, args, null, null)!;
                 }
                 catch (TargetInvocationException ex)
                 {
                     throw new NUnitEngineException("The NUnit 3 driver encountered an error while executing reflected code.", ex.InnerException);
                 }
             }
         }
-#endif
 }
+#endif