SystemProtection/SystemRestorePoint: New DSC Resources

[cohdjn]

Pull Request (PR) description

Add new resources SystemProtection and SystemRestorePoint.

This Pull Request (PR) fixes the following issues

• Fixes Merge xWindowsRestore into ComputerManagementDsc #364

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in README.md in resource folder.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov]

Codecov Report

Merging #373 (bd68ad4) into main (e645c7e) will increase coverage by 2%.
The diff coverage is 88%.

@@         Coverage Diff          @@
##           main   #373    +/-   ##
====================================
+ Coverage    88%    90%    +2%     
====================================
  Files        14     19     +5     
  Lines      1469   1915   +446     
====================================
+ Hits       1294   1734   +440     
- Misses      175    181     +6     

Impacted Files	Coverage Δ
...gementDsc.Common/ComputerManagementDsc.Common.psm1	39% <39%> (ø)
...Resources/DSC_ScheduledTask/DSC_ScheduledTask.psm1	86% <78%> (ø)
...ces/DSC_SystemProtection/DSC_SystemProtection.psm1	90% <90%> (ø)
source/DSCResources/DSC_SmbShare/DSC_SmbShare.psm1	98% <92%> (ø)
...DSC_SystemRestorePoint/DSC_SystemRestorePoint.psm1	94% <94%> (ø)
...s/DSC_WindowsCapability/DSC_WindowsCapability.psm1	97% <95%> (ø)
...urces/DSC_WindowsEventLog/DSC_WindowsEventLog.psm1	96% <96%> (ø)
source/DSCResources/DSC_Computer/DSC_Computer.psm1	88% <100%> (ø)
...iguration/DSC_IEEnhancedSecurityConfiguration.psm1	100% <100%> (ø)
...s/DSC_OfflineDomainJoin/DSC_OfflineDomainJoin.psm1	89% <100%> (ø)
... and 17 more

[cohdjn]

A couple of notes on the Pester tests since code coverage is failing...

There are no integration tests because this is designed to work against workstations and not servers. I couldn't figure out a reasonable way to implement them. If you have ideas, I'm all ears!

On the unit tests, there are large portions of the tests that are only run when Pester is on a workstation. You'll see in the scripts that I'm querying the product code and bypassing a lot them for that reason. I've run the unit tests on my Windows 10 workstation and they all pass.

[cohdjn]

I made a minor correction to the Pester tests and it looks like something went wrong with integration tests on the last pipeline build. I don't have any integration tests so there's something else going on there.

[cohdjn]

/azurepipelines run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 373 in repo dsccommunity/ComputerManagementDsc

[cohdjn]

@PlagueHO I know you're busy.... wanted to see if you have some time to work this through with me?

[PlagueHO]

Hi @cohdjn - sorry about the delay. I'll get onto the review this week and see if we can increase code coverage.

[PlagueHO]

HI @cohdjn - the main issue with the test coverage is that the unit tests will only cover the code that operates on the OS that the tests are run on. Unit tests shouldn't really (ideally 😀 ) behave this way. We'd instead design tests that mocked the behavior of each OS and ran them on each.

I think this is actually quite simple to fix here (famous last words).

Example:
This block only runs if we're running tests on a desktop OS - so we'll never get these running in the Azure DevOps pipelines (which only use Server OS):
https://github.com/cohdjn/ComputerManagementDsc/blob/main/tests/Unit/DSC_SystemRestorePoint.Tests.ps1#L96

This block also only sets up certain variables/mocks etc if it is a desktop OS:
https://github.com/cohdjn/ComputerManagementDsc/blob/main/tests/Unit/DSC_SystemRestorePoint.Tests.ps1#L59

However, we've got some code that sets up mocks and other test fixtures that is dependent on classes that only exist on the desktop OS:

            $srClass = New-Object -TypeName System.Management.ManagementClass `
                -ArgumentList ('root\default', 'SystemRestore', $null)

            $getComputerRestorePoint = New-Object -TypeName System.Management.ManagementObject
            $getComputerRestorePoint = $srClass.CreateInstance()

            $getComputerRestorePoint.Description      = 'DSC Unit Test'
            $getComputerRestorePoint.SequenceNumber   = 1
            $getComputerRestorePoint.RestorePointType = 12

To solve this you wouldn't rely on the actual class, you'd create a fake/mock for the object:

            $getComputerRestorePoint = New-Object PSObject
            $getComputerRestorePoint = Add-Member -MemberType NoteProperty -Name Description -Value 'DSC Unit Test'
            $getComputerRestorePoint = Add-Member -MemberType NoteProperty -Name SequenceNumber -Value 1
            $getComputerRestorePoint = Add-Member -MemberType NoteProperty -Name RestorePointType -Value 12

This object isn't the same type (System.Management.ManagementObject), but as long as the code in the resource doesn't check it or call specific methods on the object.

The next problem is that anywhere there is a if ($productType -eq 1) indicates a problem. For example, change:

        if ($productType -eq 1)
        {
            $srClass = New-Object -TypeName System.Management.ManagementClass `
                -ArgumentList ('root\default', 'SystemRestore', $null)

            $getComputerRestorePoint = New-Object -TypeName System.Management.ManagementObject
            $getComputerRestorePoint = $srClass.CreateInstance()

            $getComputerRestorePoint.Description      = 'DSC Unit Test'
            $getComputerRestorePoint.SequenceNumber   = 1
            $getComputerRestorePoint.RestorePointType = 12
        }

to

            $getComputerRestorePoint = New-Object PSObject
            $getComputerRestorePoint = Add-Member -MemberType NoteProperty -Name Description -Value 'DSC Unit Test'
            $getComputerRestorePoint = Add-Member -MemberType NoteProperty -Name SequenceNumber -Value 1
            $getComputerRestorePoint = Add-Member -MemberType NoteProperty -Name RestorePointType -Value 12

We also need to run tests on both the path followed when run on a desktop OS and when run on a Server OS. So we need to change the describe block for each *-TargetResource to run each path regardless of OS:

Describe 'DSC_SystemRestorePoint\Get-TargetResource' -Tag 'Get' {
    Context 'When running on a workstation OS' {
        Context 'When getting the target resource' {
                    It 'Should return Absent when requested restore point does not exist' {
                        Mock -CommandName Get-ComputerRestorePoint -MockWith { return $getComputerRestorePoint }
                        Mock -CommandName Get-CimInstance @workstationMock

                        $result = Get-TargetResource -Ensure 'Present' -Description 'Does Not Exist'

                        $result | Should -BeOfType Hashtable
                        $result.Ensure | Should -Be 'Absent'
                    }

                    It 'Should return present when requested restore point exists' {
                        Mock -CommandName Get-ComputerRestorePoint -MockWith { return $getComputerRestorePoint }
                        Mock -CommandName Get-CimInstance @workstationMock

                        $result = Get-TargetResource -Ensure 'Present' -Description 'DSC Unit Test'

                        $result | Should -BeOfType Hashtable
                        $result.Ensure | Should -Be 'Present'
                    }
        }
    }

    Context 'When running on a server OS' {
        Context 'When getting the target resource' {
                It 'Should return Absent and write a warning on a server operating system' {
                    Mock -CommandName Write-Warning

                   # There is a gap in testing here - we need to validate that Get-ComputerRestorePoint is NOT called (need a mock and an assert).
                    Mock -CommandName Get-CimInstance @serverMock

                    $protectionSettings = Get-TargetResource -Ensure 'Present' -Description 'DSC Unit Test'

                    $protectionSettings.Ensure | Should -Be 'Absent'
                    Assert-MockCalled -CommandName Write-Warning -Times 1
                }
        }
    }

Fundamentally, if you can change the tests like the above then it will increase the coverage to allow the PR to be merged - because at the moment we're not really testing the most important code (when run on desktop).

Note: sometimes it can be fairly difficult to mock objects that don't exist (especially where we need to call methods), in which case we need to wrap this code in smaller helper functions and mock those instead. But we can deal with that if encountered.

DSC was never really designed with desktop OS's in mind (e.g. OS drift is pretty normal because of users and we had GPO to deal with a lot of stuff) - so we are going to have to do a lot more work to mock elements of the OS.

Does that make sense?

Happy to run through additional issues/blockers as the come back?

[cohdjn]

I can't believe I didn't think of that! The tests have bene updated and code coverage appears to be better.