xGroup: Ensure group members are always returned as an array (#701)

* xGroup: Ensure group members are always returned as an array PowerShell will unpack single element arrays on function return by default. That's problematic when we explicitly expect an array. Note that prefixing the return operation in Get-MembersOnFullSKU and Get-MembersOnNanoServer with ',' to indicate the array should not be unrolled introduces separate issues as the underlying mechanism will wrap the "inner" array in an "outer" array, the latter being unpacked automatically when passed down the pipeline. That means: - Pester tests will fail when no group members are expected as it will encounter an empty array (the "inner" array) instead of $null. - Pester tests will fail on testing expected group members as it will encounter an array (the "inner" array) instead of the enumerated members from the result passed down the pipeline. Both of the above are known Pester issues (see Pester Issue #1154). We could update the tests, but behind the scenes Pester is complaining about a change in returned data types, and that feels problematic. So instead check if the returned data is a string (indicating a single group member) and convert it to an object array if so. * xGroup: Add additional unit tests to verify single member behaviour This introduces two new tests which verify the expected behaviour on returning the members of a group which has a single member (a single element array). One test is for Nano server and the other for the full SKU as they have different codepaths. The existing two group member tests are renamed to explicitly reflect they're checking behaviour on returning a group with multiple members. * Merge branch 'master' into xgroup-preserve-members-array
dsccommunity · Dec 17, 2020 · 7b50109 · 7b50109
1 parent 2c4d585
commit 7b50109
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - xArchive
   - Removed `Invoke-NewPSDrive` function because it is no longer needed as
     Pester issue has been resolved - Fixes [Issue #698](https://github.com/dsccommunity/xPSDesiredStateConfiguration/issues/698).
+- xGroup
+  - Ensure group membership is always returned as an array - Fixes [Issue #353](https://github.com/dsccommunity/xPSDesiredStateConfiguration/issues/353).
 
 ### Changed
 

diff --git a/source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1 b/source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1
@@ -387,6 +387,11 @@ function Get-TargetResourceOnFullSKU
             $members = Get-MembersOnFullSKU -Group $group -PrincipalContextCache $principalContextCache `
                 -Credential $Credential -Disposables $disposables
 
+            if ($members -is [System.String])
+            {
+                $members = @($members)
+            }
+
             return @{
                 GroupName = $group.Name
                 Ensure = 'Present'
@@ -457,6 +462,11 @@ function Get-TargetResourceOnNanoServer
     # The group was found. Find the group members.
     $members = Get-MembersOnNanoServer -Group $group
 
+    if ($members -is [System.String])
+    {
+        $members = @($members)
+    }
+
     return @{
         GroupName = $group.Name
         Ensure = 'Present'

diff --git a/tests/Unit/DSC_xGroupResource.Tests.ps1 b/tests/Unit/DSC_xGroupResource.Tests.ps1
@@ -324,7 +324,8 @@ try
             if ($script:onNanoServer)
             {
                 Context 'xGroupResource\Get-TargetResourceOnNanoServer' {
-                    $testMembers = @('User1', 'User2')
+                    $testMembersSingle = @('User1')
+                    $testMembersMultiple = @('User1', 'User2')
 
                     Mock -CommandName 'Get-MembersOnNanoServer' -MockWith { return @() }
 
@@ -367,11 +368,11 @@ try
                         $getTargetResourceResult.Members | Should -Be $null
                     }
 
-                    It 'Should return correct hashtable values when Get-LocalGroup returns a valid, existing group with members' {
+                    It 'Should return correct hashtable values when Get-LocalGroup returns a valid, existing group with a single member' {
                         $script:testLocalGroup.Description = $script:testGroupDescription
 
                         Mock -CommandName 'Get-LocalGroup' -MockWith { return $script:testLocalGroup }
-                        Mock -CommandName 'Get-MembersOnNanoServer' -MockWith { return $testMembers }
+                        Mock -CommandName 'Get-MembersOnNanoServer' -MockWith { return $testMembersSingle }
 
                         $getTargetResourceResult = Get-TargetResourceOnNanoServer -GroupName $script:testGroupName
 
@@ -383,7 +384,27 @@ try
                         $getTargetResourceResult.GroupName | Should -Be $script:testGroupName
                         $getTargetResourceResult.Ensure | Should -Be 'Present'
                         $getTargetResourceResult.Description | Should -Be $script:testGroupDescription
-                        $getTargetResourceResult.Members | Should -Be $testMembers
+                        $getTargetResourceResult.Members | Should -Be $testMembersSingle
+                        $getTargetResourceResult.Members -is [System.Object[]] | Should -BeTrue
+                    }
+
+                    It 'Should return correct hashtable values when Get-LocalGroup returns a valid, existing group with multiple members' {
+                        $script:testLocalGroup.Description = $script:testGroupDescription
+
+                        Mock -CommandName 'Get-LocalGroup' -MockWith { return $script:testLocalGroup }
+                        Mock -CommandName 'Get-MembersOnNanoServer' -MockWith { return $testMembersMultiple }
+
+                        $getTargetResourceResult = Get-TargetResourceOnNanoServer -GroupName $script:testGroupName
+
+                        Assert-MockCalled -CommandName 'Get-LocalGroup' -ParameterFilter { $Name -eq $script:testGroupName }
+                        Assert-MockCalled -CommandName 'Get-MembersOnNanoServer' -ParameterFilter { $Group -eq $script:testLocalGroup }
+
+                        $getTargetResourceResult -is [System.Collections.Hashtable] | Should -BeTrue
+                        $getTargetResourceResult.Keys.Count | Should -Be 4
+                        $getTargetResourceResult.GroupName | Should -Be $script:testGroupName
+                        $getTargetResourceResult.Ensure | Should -Be 'Present'
+                        $getTargetResourceResult.Description | Should -Be $script:testGroupDescription
+                        $getTargetResourceResult.Members | Should -Be $testMembersMultiple
                     }
                 }
 
@@ -861,7 +882,8 @@ try
             else
             {
                 Context 'xGroupResource\Get-TargetResourceOnFullSKU' {
-                    $testMembers = @($script:testuserPrincipal1.Name, $script:testuserPrincipal2.Name)
+                    $testMembersSingle = @($script:testuserPrincipal1.Name)
+                    $testMembersMultiple = @($script:testuserPrincipal1.Name, $script:testuserPrincipal2.Name)
 
                     Mock -CommandName 'Get-Group' -MockWith { }
                     Mock -CommandName 'Get-MembersOnFullSKU' -MockWith { return @() }
@@ -898,11 +920,32 @@ try
                         $getTargetResourceResult.Members | Should -Be $null
                     }
 
-                    It 'Should return correct hashtable values when Get-Group returns a valid, existing group with members' {
+                    It 'Should return correct hashtable values when Get-Group returns a valid, existing group with a single member' {
+                        $testGroup.Description = $script:testGroupDescription
+
+                        Mock -CommandName 'Get-Group' -MockWith { return $script:testGroup }
+                        Mock -CommandName 'Get-MembersOnFullSKU' -MockWith { return $testMembersSingle }
+
+                        $getTargetResourceResult = Get-TargetResourceOnFullSKU -GroupName $script:testGroupName
+
+                        Assert-MockCalled -CommandName 'Get-Group' -ParameterFilter { $GroupName -eq $script:testGroupName }
+                        Assert-MockCalled -CommandName 'Get-MembersOnFullSKU' -ParameterFilter { $Group -eq $script:testGroup }
+                        Assert-MockCalled -CommandName 'Remove-DisposableObject'
+
+                        $getTargetResourceResult -is [System.Collections.Hashtable] | Should -BeTrue
+                        $getTargetResourceResult.Keys.Count | Should -Be 4
+                        $getTargetResourceResult.GroupName | Should -Be $script:testGroupName
+                        $getTargetResourceResult.Ensure | Should -Be 'Present'
+                        $getTargetResourceResult.Description | Should -Be $script:testGroupDescription
+                        $getTargetResourceResult.Members | Should -Be $testMembersSingle
+                        $getTargetResourceResult.Members -is [System.Object[]] | Should -BeTrue
+                    }
+
+                    It 'Should return correct hashtable values when Get-Group returns a valid, existing group with multiple members' {
                         $testGroup.Description = $script:testGroupDescription
 
                         Mock -CommandName 'Get-Group' -MockWith { return $script:testGroup }
-                        Mock -CommandName 'Get-MembersOnFullSKU' -MockWith { return $testMembers }
+                        Mock -CommandName 'Get-MembersOnFullSKU' -MockWith { return $testMembersMultiple }
 
                         $getTargetResourceResult = Get-TargetResourceOnFullSKU -GroupName $script:testGroupName
 
@@ -915,7 +958,7 @@ try
                         $getTargetResourceResult.GroupName | Should -Be $script:testGroupName
                         $getTargetResourceResult.Ensure | Should -Be 'Present'
                         $getTargetResourceResult.Description | Should -Be $script:testGroupDescription
-                        $getTargetResourceResult.Members | Should -Be $testMembers
+                        $getTargetResourceResult.Members | Should -Be $testMembersMultiple
                     }
                 }