diff --git a/src/Main.ps1 b/src/Main.ps1 index c723bea50..a50d4df9d 100644 --- a/src/Main.ps1 +++ b/src/Main.ps1 @@ -535,6 +535,18 @@ function Invoke-Pester { & $SafeCommands['Get-Variable'] 'Configuration' -Scope Local | Remove-Variable + # Keys from the configuration hashtable that match no section or option. They are + # reported and not thrown on, because a hashtable may carry keys meant for something + # else, but a misspelled option would otherwise leave the run on the default with + # nothing to notice (#2975). A value the option cannot use throws instead, when the + # configuration is built, because that is never intentional. + $unknownConfigurationKeys = $PesterPreference.GetUnknownKeys() + if (0 -lt $unknownConfigurationKeys.Count) { + $quotedKeys = @(foreach ($unknownKey in $unknownConfigurationKeys) { "'$unknownKey'" }) -join ', ' + $reason = if (1 -eq $unknownConfigurationKeys.Count) { "key $quotedKeys, there is no such option" } else { "keys $quotedKeys, there are no such options" } + & $SafeCommands['Write-Warning'] "Ignoring configuration $reason. Check the spelling, 'Get-Help about_PesterConfiguration' lists all the options." + } + Resolve-AutoEnabledConfiguration -PesterPreference $PesterPreference # $sessionState = Set-SessionStateHint -PassThru -Hint "Caller - Captured in Invoke-Pester" -SessionState $PSCmdlet.SessionState diff --git a/src/csharp/Pester/ConfigurationSection.cs b/src/csharp/Pester/ConfigurationSection.cs index cf6a297ed..3830a7729 100644 --- a/src/csharp/Pester/ConfigurationSection.cs +++ b/src/csharp/Pester/ConfigurationSection.cs @@ -15,6 +15,7 @@ // to have in "type accelerator" form, but without the hassle of actually adding it as a type accelerator // that way you can easily do `[PesterConfiguration]::Default` and then inspect it, or cast a hashtable to it +using System.Collections.Generic; using System.Reflection; namespace Pester @@ -32,6 +33,22 @@ public override string ToString() return _description; } + /// + /// Names of the options this section has. A method and not a property so it stays out of the + /// section's console output, which lists the options and their documentation. + /// + public string[] GetOptionNames() + { + var names = new List(); + foreach (var property in GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance)) + { + if (typeof(Option).IsAssignableFrom(property.PropertyType)) + names.Add(property.Name); + } + + return names.ToArray(); + } + /// /// If this section has an Enabled option that was not explicitly modified, /// and any other option in the section was modified, auto-enable the section. diff --git a/src/csharp/Pester/ConfigurationValueException.cs b/src/csharp/Pester/ConfigurationValueException.cs new file mode 100644 index 000000000..2f82d82fb --- /dev/null +++ b/src/csharp/Pester/ConfigurationValueException.cs @@ -0,0 +1,14 @@ +using System; + +namespace Pester +{ + /// + /// Thrown when a configuration key holds a value the option cannot use, for example a string + /// where a bool is expected. Its own type so PesterConfiguration can recognize it while + /// building the sections and prefix the message with the section name. + /// + public class ConfigurationValueException : ArgumentException + { + public ConfigurationValueException(string message) : base(message) { } + } +} diff --git a/src/csharp/Pester/DictionaryExtensions.cs b/src/csharp/Pester/DictionaryExtensions.cs index 5aa34d25d..66c8b4c1f 100644 --- a/src/csharp/Pester/DictionaryExtensions.cs +++ b/src/csharp/Pester/DictionaryExtensions.cs @@ -24,12 +24,69 @@ namespace Pester { internal static class DictionaryExtensions { + // A value we cannot use is never intentional, so we say so instead of leaving the option on + // its default and letting the run behave as if the option was never set (#2975). A key that + // is present but null keeps meaning "not set", because that is how it has always worked and + // it is how an unset variable arrives here (#2219). + private static ConfigurationValueException NotUsable(string key, object value, string expected) + { + return new ConfigurationValueException($"{key} expects {expected}, but got {Describe(value)}."); + } + + // Name the type the way it is written in PowerShell, and show the value itself only when it + // is something worth printing. 'the string 'yes'' helps, 'the hashtable + // 'System.Collections.Hashtable'' does not. + private static string Describe(object value) + { + if (value is PSObject pso) + value = pso.BaseObject; + + if (value == null) + return "nothing"; + + var name = TypeName(value.GetType()); + return value is string || value.GetType().IsPrimitive || value is decimal + ? $"the {name} '{value}'" + : $"a {name}"; + } + + private static string TypeName(Type type) + { + if (type == typeof(bool)) return "bool"; + if (type == typeof(int)) return "int"; + if (type == typeof(decimal)) return "decimal"; + if (type == typeof(string)) return "string"; + if (type == typeof(ScriptBlock)) return "scriptblock"; + if (type == typeof(Hashtable)) return "hashtable"; + if (type == typeof(ContainerInfo)) return "container"; + return type.Name; + } + + private static string ExpectedValue(Type type) + { + var name = TypeName(type); + return name == "int" ? "an int" : $"a {name}"; + } + + private static string ExpectedArray(Type type) + { + if (type == typeof(string)) return "an array of strings"; + if (type == typeof(ScriptBlock)) return "an array of scriptblocks"; + if (type == typeof(ContainerInfo)) return "an array of containers"; + return $"an array of {TypeName(type)}"; + } + public static T? GetValueOrNull(this IDictionary dictionary, string key) where T : struct { if (!dictionary.Contains(key)) return null; var value = dictionary[key]; + if (value is null) + return null; + + if (value is PSObject unwrapped) + value = unwrapped.BaseObject; if (typeof(T) == typeof(decimal)) { @@ -37,7 +94,11 @@ internal static class DictionaryExtensions return (T)Convert.ChangeType(value, typeof(decimal)); } - return value as T?; + var converted = value as T?; + if (converted == null) + throw NotUsable(key, value, ExpectedValue(typeof(T))); + + return converted; } public static T GetObjectOrNull(this IDictionary dictionary, string key) where T : class @@ -45,11 +106,19 @@ public static T GetObjectOrNull(this IDictionary dictionary, string key) wher if (!dictionary.Contains(key)) return null; + var value = dictionary[key]; + if (value is null) + return null; + if (typeof(T) == typeof(string)) - if (dictionary[key] is PSObject o) + if (value is PSObject o) return (T) Convert.ChangeType(o.ToString(), typeof(string)); - return dictionary[key] as T; + var converted = value as T; + if (converted == null) + throw NotUsable(key, value, ExpectedValue(typeof(T))); + + return converted; } public static IDictionary GetIDictionaryOrNull(this IDictionary dictionary, string key) @@ -57,14 +126,18 @@ public static IDictionary GetIDictionaryOrNull(this IDictionary dictionary, stri if (!dictionary.Contains(key)) return null; - if (dictionary[key] is PSObject pso) - { - return pso.BaseObject as IDictionary; - } - else - { - return dictionary[key] as IDictionary; - } + var value = dictionary[key]; + if (value is null) + return null; + + if (value is PSObject pso) + value = pso.BaseObject; + + var converted = value as IDictionary; + if (converted == null) + throw NotUsable(key, value, "a dictionary of options"); + + return converted; } public static T[] GetArrayOrNull(this IDictionary dictionary, string key) where T : class @@ -120,7 +193,7 @@ public static T[] GetArrayOrNull(this IDictionary dictionary, string key) whe return new T[] { (T)value }; } - return null; + throw NotUsable(key, value, ExpectedArray(typeof(T))); } public static void AssignValueIfNotNull(this IDictionary dictionary, string key, Action assign) diff --git a/src/csharp/Pester/PesterConfiguration.cs b/src/csharp/Pester/PesterConfiguration.cs index 6dc85b30e..35c11964e 100644 --- a/src/csharp/Pester/PesterConfiguration.cs +++ b/src/csharp/Pester/PesterConfiguration.cs @@ -1,5 +1,9 @@ using Pester; +using System; using System.Collections; +using System.Collections.Generic; +using System.Management.Automation; +using System.Reflection; // those types implement Pester configuration in a way that allows it to show information about each item // in the powershell console without making it difficult to use. there are two tricks being used: @@ -36,6 +40,7 @@ public static PesterConfiguration ShallowClone(PesterConfiguration configuration cfg.TestDrive = TestDriveConfiguration.ShallowClone(configuration.TestDrive); cfg.TestRegistry = TestRegistryConfiguration.ShallowClone(configuration.TestRegistry); cfg.Mock = MockConfiguration.ShallowClone(configuration.Mock); + cfg._unknownKeys = configuration._unknownKeys; return cfg; } @@ -52,6 +57,15 @@ public static PesterConfiguration Merge(PesterConfiguration configuration, Peste cfg.TestDrive = Merger.Merge(configuration.TestDrive, @override.TestDrive); cfg.TestRegistry = Merger.Merge(configuration.TestRegistry, @override.TestRegistry); cfg.Mock = Merger.Merge(configuration.Mock, @override.Mock); + // Invoke-Pester merges onto the default configuration before it reports anything, so the + // unknown keys have to survive the merge or the warning is lost. + var unknown = new List(configuration._unknownKeys); + foreach (var key in @override._unknownKeys) + { + if (!unknown.Contains(key)) + unknown.Add(key); + } + cfg._unknownKeys = unknown.ToArray(); return cfg; } @@ -59,17 +73,112 @@ public PesterConfiguration(IDictionary configuration) { if (configuration != null) { - Run = new RunConfiguration(configuration.GetIDictionaryOrNull(nameof(Run))); - Filter = new FilterConfiguration(configuration.GetIDictionaryOrNull(nameof(Filter))); - CodeCoverage = new CodeCoverageConfiguration(configuration.GetIDictionaryOrNull(nameof(CodeCoverage))); - TestResult = new TestResultConfiguration(configuration.GetIDictionaryOrNull(nameof(TestResult))); - Should = new ShouldConfiguration(configuration.GetIDictionaryOrNull(nameof(Should))); - Debug = new DebugConfiguration(configuration.GetIDictionaryOrNull(nameof(Debug))); - Output = new OutputConfiguration(configuration.GetIDictionaryOrNull(nameof(Output))); - TestDrive = new TestDriveConfiguration(configuration.GetIDictionaryOrNull(nameof(TestDrive))); - TestRegistry = new TestRegistryConfiguration(configuration.GetIDictionaryOrNull(nameof(TestRegistry))); - Mock = new MockConfiguration(configuration.GetIDictionaryOrNull(nameof(Mock))); + Run = Section(configuration, nameof(Run), d => new RunConfiguration(d)); + Filter = Section(configuration, nameof(Filter), d => new FilterConfiguration(d)); + CodeCoverage = Section(configuration, nameof(CodeCoverage), d => new CodeCoverageConfiguration(d)); + TestResult = Section(configuration, nameof(TestResult), d => new TestResultConfiguration(d)); + Should = Section(configuration, nameof(Should), d => new ShouldConfiguration(d)); + Debug = Section(configuration, nameof(Debug), d => new DebugConfiguration(d)); + Output = Section(configuration, nameof(Output), d => new OutputConfiguration(d)); + TestDrive = Section(configuration, nameof(TestDrive), d => new TestDriveConfiguration(d)); + TestRegistry = Section(configuration, nameof(TestRegistry), d => new TestRegistryConfiguration(d)); + Mock = Section(configuration, nameof(Mock), d => new MockConfiguration(d)); + + _unknownKeys = CollectUnknownKeys(configuration); + } + } + + // Build one section, and put the section name in front of the message when the section rejects + // a value, so the user is told 'Run.Parallel expects ...' and not just 'Parallel expects ...'. + private static T Section(IDictionary configuration, string name, Func create) + where T : ConfigurationSection + { + // Resolved outside the try, its own message already names the section. + var options = configuration.GetIDictionaryOrNull(name); + try + { + return create(options); + } + catch (ConfigurationValueException e) + { + throw new ConfigurationValueException($"{name}.{e.Message}"); + } + } + + // Keys the configuration does not have an option for. They are collected rather than thrown on, + // because a hashtable may legitimately be shared with something else, and reported by the caller + // (Invoke-Pester warns) so a misspelled option does not quietly do nothing (#2975). + private static string[] CollectUnknownKeys(IDictionary configuration) + { + var unknown = new List(); + var sections = new PesterConfiguration(); + + foreach (var key in configuration.Keys) + { + var name = key as string ?? key?.ToString(); + var property = Match(sections.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance), name, configuration); + if (property == null) + { + unknown.Add(name); + continue; + } + + // A known section, so check the options inside it the same way. + var section = property.GetValue(sections) as ConfigurationSection; + var options = configuration[property.Name] as IDictionary + ?? (configuration[property.Name] as PSObject)?.BaseObject as IDictionary; + if (section == null || options == null) + continue; + + var known = section.GetOptionNames(); + foreach (var optionKey in options.Keys) + { + var optionName = optionKey as string ?? optionKey?.ToString(); + if (Find(known, optionName, options) == null) + unknown.Add($"{property.Name}.{optionName}"); + } + } + + return unknown.ToArray(); + } + + // A key counts as known only when looking it up by the option's own name finds it. Comparing + // case-insensitively alone is not enough: a dictionary with a case-sensitive comparer holds + // 'run' without answering to 'Run', so the value would never be read and the key is unknown. + private static PropertyInfo Match(PropertyInfo[] properties, string name, IDictionary dictionary) + { + foreach (var property in properties) + { + if (!typeof(ConfigurationSection).IsAssignableFrom(property.PropertyType)) + continue; + + if (string.Equals(property.Name, name, StringComparison.OrdinalIgnoreCase) && dictionary.Contains(property.Name)) + return property; + } + + return null; + } + + private static string Find(string[] names, string name, IDictionary dictionary) + { + foreach (var known in names) + { + if (string.Equals(known, name, StringComparison.OrdinalIgnoreCase) && dictionary.Contains(known)) + return known; } + + return null; + } + + private string[] _unknownKeys = new string[0]; + + /// + /// Keys found in the hashtable this configuration was built from that do not match any section + /// or option. A method and not a property so it stays out of the configuration's console output. + /// + public string[] GetUnknownKeys() + { + return _unknownKeys; } public PesterConfiguration() diff --git a/tst/PesterConfiguration.Tests.ps1 b/tst/PesterConfiguration.Tests.ps1 index f45f5db2f..c3a02902c 100644 --- a/tst/PesterConfiguration.Tests.ps1 +++ b/tst/PesterConfiguration.Tests.ps1 @@ -51,3 +51,77 @@ Describe "PesterConfiguration.Format.ps1xml" { } } } + +Describe "Building a configuration from a hashtable" { + Context "Values the option cannot use" { + It "Throws for with a " -ForEach @( + @{ Key = 'Run.Parallel'; Description = 'string instead of a bool'; Hashtable = @{ Run = @{ Parallel = 'yes' } }; Expected = "*Run.Parallel expects a bool, but got the string 'yes'*" } + @{ Key = 'Run.ParallelThrottleLimit'; Description = 'string instead of an int'; Hashtable = @{ Run = @{ ParallelThrottleLimit = 'five' } }; Expected = "*Run.ParallelThrottleLimit expects an int, but got the string 'five'*" } + @{ Key = 'Output.Verbosity'; Description = 'int instead of a string'; Hashtable = @{ Output = @{ Verbosity = 42 } }; Expected = "*Output.Verbosity expects a string, but got the int '42'*" } + @{ Key = 'Run.Path'; Description = 'hashtable instead of an array'; Hashtable = @{ Run = @{ Path = @{ a = 1 } } }; Expected = '*Run.Path expects an array of strings, but got a hashtable*' } + @{ Key = 'Run'; Description = 'string instead of a section'; Hashtable = @{ Run = 'nonsense' }; Expected = "*Run expects a dictionary of options, but got the string 'nonsense'*" } + ) { + { [PesterConfiguration]$Hashtable } | Should -Throw -ExpectedMessage $Expected + } + + It "Keeps the default when the value is null, so an unset variable does not throw" { + $configuration = [PesterConfiguration]@{ Run = @{ Parallel = $null; Path = $null } } + $configuration.Run.Parallel.Value | Should -BeFalse + $configuration.Run.Path.Value | Should -Be '.' + } + + It "Accepts an int for a decimal option" { + ([PesterConfiguration]@{ CodeCoverage = @{ CoveragePercentTarget = 80 } }).CodeCoverage.CoveragePercentTarget.Value | Should -Be 80 + } + } + + Context "Keys that match no option" { + It "Collects a misspelled option" { + ([PesterConfiguration]@{ Run = @{ Paralel = $true } }).GetUnknownKeys() | Should -Be @('Run.Paralel') + } + + It "Collects a misspelled section" { + ([PesterConfiguration]@{ Runn = @{ Parallel = $true } }).GetUnknownKeys() | Should -Be @('Runn') + } + + It "Collects nothing when every key is an option" { + ([PesterConfiguration]@{ Run = @{ Parallel = $true }; Output = @{ Verbosity = 'None' } }).GetUnknownKeys() | Should -BeNullOrEmpty + } + + It "Matches option names without regard to case" { + ([PesterConfiguration]@{ run = @{ parallel = $true } }).GetUnknownKeys() | Should -BeNullOrEmpty + } + + It "Survives the merge Invoke-Pester does before it reports them" { + $merged = [PesterConfiguration]::Merge([PesterConfiguration]::Default, [PesterConfiguration]@{ Run = @{ Paralel = $true } }) + $merged.GetUnknownKeys() | Should -Be @('Run.Paralel') + } + } + + Context "Invoke-Pester" { + BeforeAll { + $testFile = "$(Join-Path ([IO.Path]::GetTempPath()) ([Guid]::NewGuid().Guid)).Tests.ps1" + Set-Content -Path $testFile -Value "Describe 'a' { It 'b' { 1 | Should -Be 1 } }" + } + + AfterAll { + Remove-Item -Path $testFile -Force + } + + It "Warns about a key that matches no option" { + $warnings = Invoke-Pester -Configuration @{ Run = @{ Path = $testFile; Paralel = $true }; Output = @{ Verbosity = 'None' } } 3>&1 + $warnings | Should -BeLike "*Ignoring configuration key 'Run.Paralel', there is no such option*" + } + + It "Warns once, listing every key, when there are several" { + $warnings = @(Invoke-Pester -Configuration @{ Nonsense = 1; Run = @{ Path = $testFile; Paralel = $true }; Output = @{ Verbosity = 'None' } } 3>&1) + $warnings.Count | Should -Be 1 + $warnings[0].Message | Should -BeLike "*keys 'Run.Paralel', 'Nonsense', there are no such options*" + } + + It "Does not warn when every key is an option" { + $warnings = @(Invoke-Pester -Configuration @{ Run = @{ Path = $testFile }; Output = @{ Verbosity = 'None' } } 3>&1) + $warnings | Should -BeNullOrEmpty + } + } +}