From b83f3d8ef5d0f5f08838edf760acf80bdb205017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Sat, 15 Aug 2026 19:26:36 +0200 Subject: [PATCH] Reject configuration values we cannot use, warn about keys we do not know MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A configuration hashtable ignored anything it did not recognize and never said a word, so a misspelled option left the run on the default with nothing to notice, and so did a correct key holding a value of the wrong type. The helpers read a failed cast as "not specified", which also hits people who spelled everything right, because values from a JSON or psd1 file arrive as strings. A value the option cannot use now throws while the configuration is built, naming the option, what it expects and what it got. That is never intentional, so there is nothing to lose by being strict. An unknown key is collected instead, and Invoke-Pester warns about all of them at once. A hashtable may carry keys meant for something else, so throwing there could break a working setup. A key that is present but null keeps meaning "not set" (#2219). Fix #2975 🤖 --- src/Main.ps1 | 12 ++ src/csharp/Pester/ConfigurationSection.cs | 17 +++ .../Pester/ConfigurationValueException.cs | 14 ++ src/csharp/Pester/DictionaryExtensions.cs | 97 +++++++++++-- src/csharp/Pester/PesterConfiguration.cs | 129 ++++++++++++++++-- tst/PesterConfiguration.Tests.ps1 | 74 ++++++++++ 6 files changed, 321 insertions(+), 22 deletions(-) create mode 100644 src/csharp/Pester/ConfigurationValueException.cs 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 + } + } +}