PackageSettings improvements

src/NUnitCommon/nunit.common/PackageHelper.cs

@@ -165,12 +165,11 @@ private static void ReadSettings(this PackageSettings packageSettings, XmlReader
             {
                 var name = xmlReader.Name;
                 var value = xmlReader.Value;
-                var prop = typeof(SettingDefinitions).GetProperty(name, BindingFlags.Public | BindingFlags.Static);
 
-                if (prop is not null) // Standard SettingDefinition
+                var settingDefinition = SettingDefinitions.Lookup(name);
+                if (settingDefinition is not null) // Known setting name
                 {
-                    var settingDefinition = (SettingDefinition)prop.GetValue(null)!;
-
+                    // A few settings require special handling when deserializing
                     switch (name)
                     {
                         case nameof(SettingDefinitions.InternalTraceWriter):
@@ -190,9 +189,14 @@ private static void ReadSettings(this PackageSettings packageSettings, XmlReader
                         default:
                             switch (settingDefinition.ValueType)
                             {
-                                case Type t when t.IsPrimitive || t.IsAssignableFrom(typeof(string)):
-                                    var data = Convert.ChangeType(value, t);
-                                    packageSettings.Add(settingDefinition.WithValue(data));
+                                case Type t when t.IsAssignableFrom(typeof(string)):
+                                    packageSettings.Add(name, value);
+                                    break;
+                                case Type t when t.IsAssignableFrom(typeof(bool)):
+                                    packageSettings.Add(name, bool.Parse(value));
+                                    break;
+                                case Type t when t.IsAssignableFrom(typeof(int)):
+                                    packageSettings.Add(name, int.Parse(value));
                                     break;
                                 default:
                                     // Setting doesn't match the expected type, ignore or throw an exception?

src/NUnitEngine/nunit.engine.api/PackageSetting.cs

@@ -1,5 +1,7 @@
 // Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt
 
+using System.Xml;
+
 namespace NUnit.Engine
 {
     /// <summary>

src/NUnitEngine/nunit.engine.api/PackageSettings.cs

@@ -1,7 +1,10 @@
 // Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt
 
+using NUnit.Common;
+using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.Xml.Linq;
 
 namespace NUnit.Engine
 {
@@ -65,12 +68,20 @@ public T GetValueOrDefault<T>(SettingDefinition<T> definition)
         public void Add(PackageSetting setting) => _settings.Add(setting.Name, setting);
 
         /// <summary>
-        /// Creates and adds a custom string setting to the list, specifying the name and value.
+        /// Creates and adds a setting to the list, specified by the name and a string value.
+        /// The string value is converted to a typed PackageSetting if the name specifies a known SettingDefinition.
         /// </summary>
         /// <param name="name">The name of the setting.</param>
         /// <param name="value">The corresponding value to set.</param>
         public void Add(string name, string value)
         {
+            var definition = SettingDefinitions.Lookup(name);
+
+            // If this is a known setting but not a string throw an exception
+            if (definition is not null && !definition.ValueType.IsAssignableFrom(typeof(string)))
+                throw (new ArgumentException($"The {name} setting requires a value of type {definition.ValueType.Name}"));
+
+            // Otherwise add it
             Add(new PackageSetting<string>(name, value));
         }
 
@@ -81,6 +92,12 @@ public void Add(string name, string value)
         /// <param name="value">The corresponding value to set.</param>
         public void Add(string name, bool value)
         {
+            var definition = SettingDefinitions.Lookup(name);
+
+            // If this is a known setting but not a boolean throw an exception
+            if (definition is not null && !definition.ValueType.IsAssignableFrom(typeof(bool)))
+                throw (new ArgumentException($"The {name} setting requires a value of type {definition.ValueType.Name}"));
+
             Add(new PackageSetting<bool>(name, value));
         }
 
@@ -91,6 +108,12 @@ public void Add(string name, bool value)
         /// <param name="value">The corresponding value to set.</param>
         public void Add(string name, int value)
         {
+            var definition = SettingDefinitions.Lookup(name);
+
+            // If this is a known setting but not an int throw an exception
+            if (definition is not null && !definition.ValueType.IsAssignableFrom(typeof(int)))
+                throw (new ArgumentException($"The {name} setting requires a value of type {definition.ValueType.Name}"));
+
             Add(new PackageSetting<int>(name, value));
         }
 
@@ -109,5 +132,23 @@ public void Set<T>(string name, T value)
         {
             Set(new PackageSetting<T>(name, value));
         }
+
+        /// <summary>
+        /// Remove a setting
+        /// </summary>
+        /// <param name="setting">The setting to remove</param>
+        public void Remove(SettingDefinition setting)
+        {
+            _settings.Remove(setting.Name);
+        }
+
+        /// <summary>
+        /// Remove a setting
+        /// </summary>
+        /// <param name="settingName">The name of the setting to remove</param>
+        public void Remove(string settingName)
+        {
+            _settings.Remove(settingName);
+        }
     }
 }

src/NUnitEngine/nunit.engine.api/SettingDefinition.cs

@@ -37,7 +37,10 @@ public SettingDefinition(string name, Type valueType)
         public PackageSetting WithValue<T>(T value)
             where T : notnull
         {
-            return new PackageSetting<T>(Name, value);
+            if (ValueType.IsAssignableFrom(typeof(T)))
+                return new PackageSetting<T>(Name, (T)Convert.ChangeType(value, ValueType));
+
+            throw (new ArgumentException($"The {Name} setting requires a value of type {ValueType.Name}"));
         }
     }
 

src/NUnitEngine/nunit.engine.api/SettingDefinitions.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Reflection;
 
 namespace NUnit.Common
 {
@@ -15,6 +16,35 @@ namespace NUnit.Common
     /// </summary>
     public static class SettingDefinitions
     {
+        private static readonly Dictionary<string, SettingDefinition> _knownSettings;
+
+        static SettingDefinitions()
+        {
+            // Use Reflection to build a dictionary of all known setting definitions
+            var properties = typeof(SettingDefinitions).GetProperties(BindingFlags.Public | BindingFlags.Static);
+
+            _knownSettings = new Dictionary<string, SettingDefinition>();
+
+            foreach (var property in properties)
+            {
+                var propValue = property.GetValue(null, null) as SettingDefinition;
+                if (propValue is not null)
+                    _knownSettings.Add(property.Name, propValue);
+            }
+        }
+
+        /// <summary>
+        /// Lookup a known SettingDefinition by name
+        /// </summary>
+        /// <param name="name">The name of the setting</param>
+        /// <returns>A SettingDefinition if there is one, otherwise null.</returns>
+        public static SettingDefinition? Lookup(string name)
+        {
+            if (_knownSettings.TryGetValue(name, out var definition))
+                return definition;
+            return null;
+        }
+
         #region Settings Used by the Engine
 
         /// <summary>

src/NUnitEngine/nunit.engine.tests/Api/PackageSettingsTests.cs

@@ -0,0 +1,154 @@
+// Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using NUnit.Common;
+using NUnit.Framework;
+
+namespace NUnit.Engine.Api
+{
+    public class PackageSettingsTests
+    {
+        private PackageSettings _settings;
+
+        [SetUp]
+        public void SetUp()
+        {
+            _settings = new PackageSettings();
+        }
+
+        [Test]
+        public static void LookupSettingDefinitionSucceeds()
+        {
+            var definition = SettingDefinitions.MaxAgents;
+            var lookup = SettingDefinitions.Lookup("MaxAgents");
+
+            Assert.That(lookup, Is.SameAs(definition));
+        }
+
+        [Test]
+        public static void LookupSettingDefinitionFails()
+        {
+            var lookup = SettingDefinitions.Lookup("NonExistent");
+
+            Assert.That(lookup, Is.Null);
+        }
+
+        [Test]
+        public void AddKnownSettingUsingSettingDefinition()
+        {
+            _settings.Add(SettingDefinitions.MaxAgents.WithValue(42));
+            Assert.That(_settings.GetSetting("MaxAgents"), Is.EqualTo(42));
+        }
+
+        [Test]
+        public void AddKnownSettingUsingSettingDefinitionThrowsWithWrongType()
+        {
+            Assert.That(() => _settings.Add(SettingDefinitions.MaxAgents.WithValue("42")), Throws.ArgumentException);
+        }
+
+        //[TestCase("ActiveConfig", "Release")]
+        //public void AddKnownSettingUsingName<T>(string name, T value)
+        //    where T : notnull
+        //{
+        //    var setting = SettingDefinitions.Lookup(name);
+        //    Assert.That(setting, Is.Not.Null);
+        //    var type = setting.ValueType;
+        //    Assert.That(type, Is.EqualTo(typeof(T)));
+
+        //    _settings.Add<T>(name, value);
+        //}
+
+        [Test]
+        public void AddKnownStringSettingUsingName()
+        {
+            _settings.Add("ActiveConfig", "Release");
+            Assert.That(_settings.GetSetting("ActiveConfig"), Is.EqualTo("Release"));
+        }
+
+        [Test]
+        public void AddKnownStringSettingUsingNameThrowsWithWrongType()
+        {
+            Assert.That(() => _settings.Add("ActiveConfig", 5), Throws.ArgumentException);
+        }
+
+        [Test]
+        public void AddKnownBoolSettingUsingName()
+        {
+            _settings.Add("RunAsX86", true);
+            Assert.That(_settings.GetSetting("RunAsX86"), Is.True);
+        }
+
+        [Test]
+        public void AddKnownBoolSettingUsingNameThrowsWithWrongType()
+        {
+            Assert.That(() => _settings.Add("RunAsX86", 5), Throws.ArgumentException);
+        }
+
+        [Test]
+        public void AddKnownIntSettingUsingName()
+        {
+            _settings.Add("MaxAgents", 42);
+            Assert.That(_settings.GetSetting("MaxAgents"), Is.EqualTo(42));
+        }
+
+        [Test]
+        public void AddKnownintSettingUsingNameThrowsWithWrongType()
+        {
+            Assert.That(() => _settings.Add("MaxAgents", "Bad"), Throws.ArgumentException);
+        }
+
+        [Test]
+        public void AddUnknownStringSettingUsingName()
+        {
+            _settings.Add("MySetting", "Something");
+            Assert.That(_settings.GetSetting("MySetting"), Is.EqualTo("Something"));
+        }
+
+        [Test]
+        public void AddUnknownBoolSettingUsingName()
+        {
+            _settings.Add("MySetting", true);
+            Assert.That(_settings.GetSetting("MySetting"), Is.True);
+        }
+
+        [Test]
+        public void AddUnknownIntSettingUsingName()
+        {
+            _settings.Add("MySetting", 999);
+            Assert.That(_settings.GetSetting("MySetting"), Is.EqualTo(999));
+        }
+
+        [Test]
+        public void CanRemoveSettingUsingSettingDefinition()
+        {
+            var setting = SettingDefinitions.MaxAgents.WithValue(20);
+            _settings.Add(setting);
+            Assert.That(_settings.HasSetting("MaxAgents"));
+            _settings.Remove(SettingDefinitions.MaxAgents);
+            Assert.That(_settings.HasSetting("MaxAgents"), Is.False);
+        }
+
+        [Test]
+        public void CanRemoveSettingUsingName()
+        {
+            var setting = SettingDefinitions.MaxAgents.WithValue(20);
+            _settings.Add(setting);
+            Assert.That(_settings.HasSetting("MaxAgents"));
+            _settings.Remove("MaxAgents");
+            Assert.That(_settings.HasSetting("MaxAgents"), Is.False);
+        }
+
+        // Adding special types is NYI
+        //[Test]
+        //public void AddKnownSpecialSettingUsingName()
+        //{
+        //    var dict = new Dictionary<string, string>();
+        //    _settings.Add("TestParametersDictionary", dict);
+        //    Assert.That(_settings.GetSetting("TestParametersDictionary"), Is.SameAs(dict));
+        //}
+    }
+}