Replace IsOriginalValue() with IsModified and fix merging and casting…

… of default option values (#2201)
pester · Jul 25, 2022 · e949115 · e949115
1 parent a8d4b4d
commit e949115
Show file tree

Hide file tree

Showing 17 changed files with 170 additions and 52 deletions.
diff --git a/build.ps1 b/build.ps1
@@ -215,8 +215,19 @@ if ($Clean) {
         $ViewDef = $formatViewCtor.Invoke(($section.FullName, $listControl, [guid]::NewGuid())) -as [System.Collections.Generic.List[System.Management.Automation.FormatViewDefinition]]
         New-Object -TypeName 'System.Management.Automation.ExtendedTypeDefinition' $section.FullName, $ViewDef
     }
-    Export-FormatData -InputObject $typeDefs -Path "$PSScriptRoot/bin/PesterConfiguration.Format.ps1xml"
 
+    # Create view for Option to ensure Table and hide IsModified
+    $builder = [System.Management.Automation.TableControl]::Create().StartRowDefinition()
+    [Pester.Option[bool]].GetProperties() | Where-Object Name -notin 'IsModified' | ForEach-Object {
+        $builder.AddPropertyColumn($_.Name, [System.Management.Automation.Alignment]::Undefined, $null) > $null
+    }
+    $tableControl = $builder.EndRowDefinition().EndTable()
+
+    $ViewDef = $formatViewCtor.Invoke(('Pester.Option', $tableControl, [guid]::NewGuid())) -as [System.Collections.Generic.List[System.Management.Automation.FormatViewDefinition]]
+    $typeDefs += New-Object -TypeName 'System.Management.Automation.ExtendedTypeDefinition' 'Pester.Option', $ViewDef
+
+    # Export all formatdata
+    Export-FormatData -InputObject $typeDefs -Path "$PSScriptRoot/bin/PesterConfiguration.Format.ps1xml"
 }
 
 if (-not $PSBoundParameters.ContainsKey("Inline")) {

diff --git a/src/csharp/Pester/CodeCoverageConfiguration.cs b/src/csharp/Pester/CodeCoverageConfiguration.cs
@@ -57,17 +57,16 @@ public CodeCoverageConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Enabled = configuration.GetValueOrNull<bool>("Enabled") ?? Enabled;
-                OutputFormat = configuration.GetObjectOrNull<string>("OutputFormat") ?? OutputFormat;
-                OutputPath = configuration.GetObjectOrNull<string>("OutputPath") ?? OutputPath;
-                OutputEncoding = configuration.GetObjectOrNull<string>("OutputEncoding") ?? OutputEncoding;
-                Path = configuration.GetArrayOrNull<string>("Path") ?? Path;
-                ExcludeTests = configuration.GetValueOrNull<bool>("ExcludeTests") ?? ExcludeTests;
-                RecursePaths = configuration.GetValueOrNull<bool>("RecursePaths") ?? RecursePaths;
-                CoveragePercentTarget = configuration.GetValueOrNull<decimal>("CoveragePercentTarget") ?? CoveragePercentTarget;
-
-                SingleHitBreakpoints = configuration.GetValueOrNull<bool>("SingleHitBreakpoints") ?? SingleHitBreakpoints;
-                UseBreakpoints = configuration.GetValueOrNull<bool>("UseBreakpoints") ?? UseBreakpoints;
+                configuration.AssignValueIfNotNull<bool>(nameof(Enabled), v => Enabled = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(OutputFormat), v => OutputFormat = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(OutputPath), v => OutputPath = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(OutputEncoding), v => OutputEncoding = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(Path), v => Path = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(ExcludeTests), v => ExcludeTests = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(RecursePaths), v => RecursePaths = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(UseBreakpoints), v => UseBreakpoints = v);
+                configuration.AssignValueIfNotNull<decimal>(nameof(CoveragePercentTarget), v => CoveragePercentTarget = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(SingleHitBreakpoints), v => SingleHitBreakpoints = v);
             }
         }
 

diff --git a/src/csharp/Pester/DebugConfiguration.cs b/src/csharp/Pester/DebugConfiguration.cs
@@ -41,11 +41,11 @@ public DebugConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                ShowFullErrors = configuration.GetValueOrNull<bool>("ShowFullErrors") ?? ShowFullErrors;
-                WriteDebugMessages = configuration.GetValueOrNull<bool>("WriteDebugMessages") ?? WriteDebugMessages;
-                WriteDebugMessagesFrom = configuration.GetArrayOrNull<string>("WriteDebugMessagesFrom") ?? WriteDebugMessagesFrom;
-                ShowNavigationMarkers = configuration.GetValueOrNull<bool>("ShowNavigationMarkers") ?? ShowNavigationMarkers;
-                ReturnRawResultObject = configuration.GetValueOrNull<bool>("ReturnRawResultObject") ?? ReturnRawResultObject;
+                configuration.AssignValueIfNotNull<bool>(nameof(ShowFullErrors), v => ShowFullErrors = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(WriteDebugMessages), v => WriteDebugMessages = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(WriteDebugMessagesFrom), v => WriteDebugMessagesFrom = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(ShowNavigationMarkers), v => ShowNavigationMarkers = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(ReturnRawResultObject), v => ReturnRawResultObject = v);
             }
         }
 

diff --git a/src/csharp/Pester/DictionaryExtensions.cs b/src/csharp/Pester/DictionaryExtensions.cs
@@ -111,5 +111,35 @@ public static T[] GetArrayOrNull<T>(this IDictionary dictionary, string key) whe
 
             return null;
         }
+
+        public static void AssignValueIfNotNull<T>(this IDictionary dictionary, string key, Action<T> assign)
+        where T : struct
+        {
+            var value = GetValueOrNull<T>(dictionary, key);
+            if (value != null)
+            {
+                assign(value.Value);
+            }
+        }
+
+        public static void AssignObjectIfNotNull<T>(this IDictionary dictionary, string key, Action<T> assign)
+        where T : class
+        {
+            var value = GetObjectOrNull<T>(dictionary, key);
+            if (value != null)
+            {
+                assign(value);
+            }
+        }
+
+        public static void AssignArrayIfNotNull<T>(this IDictionary dictionary, string key, Action<T[]> assign)
+        where T : class
+        {
+            var value = GetArrayOrNull<T>(dictionary, key);
+            if (value != null)
+            {
+                assign(value);
+            }
+        }
     }
 }
diff --git a/src/csharp/Pester/FilterConfiguration.cs b/src/csharp/Pester/FilterConfiguration.cs
@@ -37,11 +37,11 @@ public FilterConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Tag = configuration.GetArrayOrNull<string>("Tag") ?? Tag;
-                ExcludeTag = configuration.GetArrayOrNull<string>("ExcludeTag") ?? ExcludeTag;
-                Line = configuration.GetArrayOrNull<string>("Line") ?? Line;
-                ExcludeLine = configuration.GetArrayOrNull<string>("ExcludeLine") ?? ExcludeLine;
-                FullName = configuration.GetArrayOrNull<string>("FullName") ?? FullName;
+                configuration.AssignArrayIfNotNull<string>(nameof(Tag), v => Tag = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(ExcludeTag), v => ExcludeTag = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(Line), v => Line = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(ExcludeLine), v => ExcludeLine = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(FullName), v => FullName = v);
             }
         }
         public FilterConfiguration() : base("Filter configuration")

diff --git a/src/csharp/Pester/GenericOption.cs b/src/csharp/Pester/GenericOption.cs
@@ -21,12 +21,12 @@ public abstract class Option<T> : Option
     {
         public Option(Option<T> option, T value) : this(option.Description, option.Default, value)
         {
-            _isOriginalValue = false;
+            _isModified = true;
         }
 
         public Option(string description, T defaultValue, T value)
         {
-            _isOriginalValue = true;
+            _isModified = false;
             Default = defaultValue;
             Value = value;
             Description = description;

diff --git a/src/csharp/Pester/Merger.cs b/src/csharp/Pester/Merger.cs
@@ -29,14 +29,20 @@ internal static class Merger
             foreach (var p in properties.Where(p => p.CanRead && p.CanWrite))
             {
                 object value;
+                var configurationValue = p.GetValue(configuration);
                 var overrideValue = p.GetValue(@override);
-                if (!((Option)overrideValue).IsOriginalValue())
+
+                if (((Option)overrideValue).IsModified)
                 {
                     value = overrideValue;
                 }
                 else
                 {
-                    value = p.GetValue(configuration);
+                    if(!((Option)configurationValue).IsModified)
+                    {
+                        continue;
+                    }
+                    value = configurationValue;
                 }
 
                 p.SetValue(cfg, value);

diff --git a/src/csharp/Pester/Option.cs b/src/csharp/Pester/Option.cs
@@ -15,15 +15,21 @@
 // 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;
+
 namespace Pester
 {
     public abstract class Option
     {
-        protected bool _isOriginalValue;
+        protected bool _isModified;
 
+        [Obsolete("IsOriginalValue() is obsolete and should no longer be used. Use the property IsModified instead.")]
         public bool IsOriginalValue()
         {
-            return _isOriginalValue;
+            return !_isModified;
+        }
+        public bool IsModified {
+            get { return _isModified; }
         }
     }
 }
diff --git a/src/csharp/Pester/OutputConfiguration.cs b/src/csharp/Pester/OutputConfiguration.cs
@@ -36,9 +36,9 @@ public OutputConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Verbosity = configuration.GetObjectOrNull<string>("Verbosity") ?? Verbosity;
-                StackTraceVerbosity = configuration.GetObjectOrNull<string>("StackTraceVerbosity") ?? StackTraceVerbosity;
-                CIFormat = configuration.GetObjectOrNull<string>("CIFormat") ?? CIFormat;
+                configuration.AssignObjectIfNotNull<string>(nameof(Verbosity), v => Verbosity = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(StackTraceVerbosity), v => StackTraceVerbosity = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(CIFormat), v => CIFormat = v);
             }
         }
 

diff --git a/src/csharp/Pester/PesterConfigurationDeserializer.cs b/src/csharp/Pester/PesterConfigurationDeserializer.cs
@@ -61,6 +61,14 @@ private Hashtable ConvertToSectionHashtable(PSObject sourceSection, string secti
 
         foreach (var property in sourceSection.Properties)
         {
+            var IsModified = ((PSObject)property.Value).Properties["IsModified"];
+
+            // Doing this instead of IsModified -> Add to be compatible with saved PesterConfigurations from previous versions
+            // Consider rewriting in next major release
+            if (IsModified != null && !((bool)IsModified.Value)) {
+                continue;
+            }
+
             configurationSection.Add(
                 property.Name,
                 GetPropertyValue(

diff --git a/src/csharp/Pester/RunConfiguration.cs b/src/csharp/Pester/RunConfiguration.cs
@@ -43,16 +43,16 @@ public RunConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Path = configuration.GetArrayOrNull<string>(nameof(Path)) ?? Path;
-                ExcludePath = configuration.GetArrayOrNull<string>(nameof(ExcludePath)) ?? ExcludePath;
-                ScriptBlock = configuration.GetArrayOrNull<ScriptBlock>(nameof(ScriptBlock)) ?? ScriptBlock;
-                Container = configuration.GetArrayOrNull<ContainerInfo>(nameof(Container)) ?? Container;
-                TestExtension = configuration.GetObjectOrNull<string>(nameof(TestExtension)) ?? TestExtension;
-                Exit = configuration.GetValueOrNull<bool>(nameof(Exit)) ?? Exit;
-                Throw = configuration.GetValueOrNull<bool>(nameof(Throw)) ?? Throw;
-                PassThru = configuration.GetValueOrNull<bool>(nameof(PassThru)) ?? PassThru;
-                SkipRun = configuration.GetValueOrNull<bool>(nameof(SkipRun)) ?? SkipRun;
-                SkipRemainingOnFailure = configuration.GetObjectOrNull<string>(nameof(SkipRemainingOnFailure)) ?? SkipRemainingOnFailure;
+                configuration.AssignArrayIfNotNull<string>(nameof(Path), v => Path = v);
+                configuration.AssignArrayIfNotNull<string>(nameof(ExcludePath), v => ExcludePath = v);
+                configuration.AssignArrayIfNotNull<ScriptBlock>(nameof(ScriptBlock), v => ScriptBlock = v);
+                configuration.AssignArrayIfNotNull<ContainerInfo>(nameof(Container), v => Container = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(TestExtension), v => TestExtension = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(Exit), v => Exit = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(Throw), v => Throw = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(PassThru), v => PassThru = v);
+                configuration.AssignValueIfNotNull<bool>(nameof(SkipRun), v => SkipRun = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(SkipRemainingOnFailure), v => SkipRemainingOnFailure = v);
             }
         }
 

diff --git a/src/csharp/Pester/ShouldConfiguration.cs b/src/csharp/Pester/ShouldConfiguration.cs
@@ -40,7 +40,7 @@ public ShouldConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                ErrorAction = configuration.GetObjectOrNull<string>("ErrorAction") ?? ErrorAction;
+                configuration.AssignObjectIfNotNull<string>(nameof(ErrorAction), v => ErrorAction = v);
             }
         }
 

diff --git a/src/csharp/Pester/TestDriveConfiguration.cs b/src/csharp/Pester/TestDriveConfiguration.cs
@@ -21,7 +21,7 @@ public TestDriveConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Enabled = configuration.GetValueOrNull<bool>(nameof(Enabled)) ?? Enabled;
+                configuration.AssignValueIfNotNull<bool>(nameof(Enabled), v => Enabled = v);
             }
         }
 

diff --git a/src/csharp/Pester/TestRegistryConfiguration.cs b/src/csharp/Pester/TestRegistryConfiguration.cs
@@ -21,7 +21,7 @@ public TestRegistryConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Enabled = configuration.GetValueOrNull<bool>(nameof(Enabled)) ?? Enabled;
+                configuration.AssignValueIfNotNull<bool>(nameof(Enabled), v => Enabled = v);
             }
         }
 

diff --git a/src/csharp/Pester/TestResultConfiguration.cs b/src/csharp/Pester/TestResultConfiguration.cs
@@ -47,11 +47,11 @@ public TestResultConfiguration(IDictionary configuration) : this()
         {
             if (configuration != null)
             {
-                Enabled = configuration.GetValueOrNull<bool>("Enabled") ?? Enabled;
-                OutputFormat = configuration.GetObjectOrNull<string>("OutputFormat") ?? OutputFormat;
-                OutputPath = configuration.GetObjectOrNull<string>("OutputPath") ?? OutputPath;
-                OutputEncoding = configuration.GetObjectOrNull<string>("OutputEncoding") ?? OutputPath;
-                TestSuiteName = configuration.GetObjectOrNull<string>("TestSuiteName") ?? TestSuiteName;
+                configuration.AssignValueIfNotNull<bool>(nameof(Enabled), v => Enabled = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(OutputFormat), v => OutputFormat = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(OutputPath), v => OutputPath = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(OutputEncoding), v => OutputEncoding = v);
+                configuration.AssignObjectIfNotNull<string>(nameof(TestSuiteName), v => TestSuiteName = v);
             }
         }
 

diff --git a/tst/Pester.RSpec.Configuration.ts.ps1 b/tst/Pester.RSpec.Configuration.ts.ps1
@@ -262,11 +262,11 @@ i -PassThru:$PassThru {
             { $config.Run.Path.Value = 'invalid' } | Verify-Throw
         }
 
-        t "IsOriginalValue returns false after change even if same as default" {
+        t "IsModified returns true after change even if same as default" {
             $config = [PesterConfiguration]::Default
-            $config.Run.Path.IsOriginalValue() | Verify-True
+            $config.Run.Path.IsModified | Verify-False
             $config.Run.Path = $config.Run.Path.Default
-            $config.Run.Path.IsOriginalValue() | Verify-False
+            $config.Run.Path.IsModified | Verify-True
         }
     }
 
@@ -342,6 +342,16 @@ i -PassThru:$PassThru {
             # override does not touch it
             $result.Filter.Tag.Value | Verify-Equal "abc"
         }
+
+        t 'IsModified returns False after merging two original values' {
+            $one = [PesterConfiguration]::Default
+            $two = [PesterConfiguration]::Default
+            $result = [PesterConfiguration]::Merge($one, $two)
+
+            # has the same value as default but was written so it will override
+            $result.Output.Verbosity.Value | Verify-Equal $one.Output.Verbosity.Value
+            $result.Output.Verbosity.IsModified | Verify-False
+        }
     }
 
     b "Advanced interface - Run paths" {
@@ -574,6 +584,23 @@ i -PassThru:$PassThru {
             $config.Filter.Tag.Value -contains 'Core' | Verify-True
         }
 
+        t 'IsModified is only True on modified properties after merging Hashtable' {
+            $MyOptions = @{
+                Run    = @{
+                    PassThru = $true
+                }
+                Filter = @{
+                    Tag = 'Core'
+                }
+            }
+            $config = New-PesterConfiguration -Hashtable $MyOptions
+
+            $config.Run.PassThru.Value | Verify-Equal $true
+            $config.Filter.Tag.Value -contains 'Core' | Verify-True
+            $config.Run.PassThru.IsModified | Verify-True
+            $config.Run.SkipRun.IsModified | Verify-False
+        }
+
         t "Merges configuration when a hashtable has been serialized" {
             $BeforeSerialization = @{
                 Run    = @{
@@ -590,6 +617,9 @@ i -PassThru:$PassThru {
 
             $config.Run.PassThru.Value | Verify-Equal $true
             $config.Filter.Tag.Value -contains 'Core' | Verify-True
+            $config.Run.PassThru.IsModified | Verify-True
+            $config.Run.SkipRun.IsModified | Verify-False
+            $config.Output.Verbosity.IsModified | Verify-False
         }
 
         t "Merges configuration when a PesterConfiguration object has been serialized" {
@@ -609,6 +639,9 @@ i -PassThru:$PassThru {
 
             $config.Run.PassThru.Value | Verify-Equal $true
             $config.Filter.Tag.Value -contains 'Core' | Verify-True
+            $config.Run.PassThru.IsModified | Verify-True
+            $config.Run.SkipRun.IsModified | Verify-False
+            $config.Output.Verbosity.IsModified | Verify-False
         }
 
         t "Merges configuration when a PesterConfiguration object includes an array of values" {

diff --git a/tst/PesterConfiguration.Tests.ps1 b/tst/PesterConfiguration.Tests.ps1
@@ -25,6 +25,31 @@ Describe "PesterConfiguration.Format.ps1xml" {
             $missingOptions | Should -Be @()
         }
     }
+
+    Context "Testing format data for 'Pester.Option[T]'" {
+        BeforeAll {
+            $formatData = Get-FormatData -TypeName 'Pester.Option'
+            $options = [Pester.Option[bool]].GetProperties() | Where-Object Name -notin 'IsModified'
+        }
+        It 'Has a single view defined of type TableControl' {
+            $formatData | Should -Not -BeNullOrEmpty
+            $formatData.FormatViewDefinition.Count | Should -Be 1
+            $formatData.FormatViewDefinition[0].Name | Should -BeExactly 'Pester.Option'
+            $formatData.FormatViewDefinition[0].Control | Should -BeOfType [System.Management.Automation.TableControl]
+        }
+
+        It 'View includes all options' {
+            $propertiesInView = @($formatData.FormatViewDefinition[0].Control.Rows.Columns.DisplayEntry | Where-Object ValueType -EQ 'Property')
+            $propertiesInView.Count | Should -Be $options.Count
+            $missingOptions = $options.Name | Where-Object { $propertiesInView.Value -notcontains $_ }
+            $missingOptions | Should -Be @()
+        }
+
+        It 'View does not include IsModified' {
+            $propertiesInView = @($formatData.FormatViewDefinition[0].Control.Rows.Columns.DisplayEntry | Where-Object ValueType -EQ 'Property')
+            $propertiesInView.Value | Should -Not -Contain 'IsModified'
+        }
+    }
 }
 
 Describe 'PesterConfiguration' {