-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SystemProtection/SystemRestorePoint: New DSC Resources #373
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
====================================
Coverage 90% 90%
====================================
Files 17 19 +2
Lines 1682 1915 +233
====================================
+ Hits 1520 1734 +214
- Misses 162 181 +19
|
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. |
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. |
/azurepipelines run |
Commenter does not have sufficient privileges for PR 373 in repo dsccommunity/ComputerManagementDsc |
@PlagueHO I know you're busy.... wanted to see if you have some time to work this through with me? |
Hi @cohdjn - sorry about the delay. I'll get onto the review this week and see if we can increase code coverage. |
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 also only sets up certain variables/mocks etc if it is a desktop OS: 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)
{
$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 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? |
I can't believe I didn't think of that! The tests have bene updated and code coverage appears to be better. |
/azp run |
Commenter does not have sufficient privileges for PR 373 in repo dsccommunity/ComputerManagementDsc |
/azp run |
Pull request contains merge conflicts. |
@cohdjn when you have time can you resolve the merge conflicts, and then I think we can merge this one as it does not break or change any existing resources. 🤔 |
@cohdjn can you look at the failing tests? We need those to pass before we can merge. 🙂 |
Pull Request (PR) description
Add new resources SystemProtection and SystemRestorePoint.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is