xRegistry: Merge with In-Box Resource, Add Tests, and Fix Key/Value Removal

[kwirkykat]

This is for #160.

I merged the Registry resource with our current in-box resource. This was a fairly small change.
I also ported the Registry unit tests.

While porting these tests, I found that removal of registry keys and values with the resource was not working, so I fixed this as well.

---

This change is

[kwirkykat]

Made a small update to the code so that tests now all pass with StrictMode -Version 4.
Also noticed a discrepancy between the ValueType values in the README vs. in the acutal resource, so I fixed that as well.

[mbreakey3]

Reviewed 5 of 7 files at r1.
Review status: 5 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 373 at r1 (raw file):

        }

        $null = New-PSDrive @newPSDriveParams -Name $mappingKey -Root $registryDriveRootMappings[$mappingKey]

Why is this being set to null? Can you add a comment in this function?

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 400 at r1 (raw file):

    $driveName = Split-Path -Path $KeyPath -Qualifier
    Write-Verbose -Message "Mount-RegistryDrive - Drive name: $driveName"

Do you mean 'Dismount' here in this Verbose message?

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 449 at r1 (raw file):

    )

    $retString = ''

maybe change this to $hexString for clarity?

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 18 at r1 (raw file):

                $script:registryKeyPath = $baseRegistryKeyPath

                while (Test-RegistryKeyExists -KeyPath $script:registryKeyPath)

Could there ever be a situation where this infinite loops?

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 255 at r1 (raw file):

                }

                It 'Should return true for a registry value that does not exist' {

It says true testing value says $false

---

Comments from Reviewable

[kwirkykat]

Review status: 5 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 373 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Why is this being set to null? Can you add a comment in this function?

This is 'nullifying' the output of New-PSDrive. It's the same thing as piping to Out-Null but with less overhead.

Added some comments as well as the 2 missing parameters from the 2nd call to New-PSDrive

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 400 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Do you mean 'Dismount' here in this Verbose message?

Done.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 449 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

maybe change this to $hexString for clarity?

Done.

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 18 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Could there ever be a situation where this infinite loops?

It's very unlikely, but I added a timeout of 1 minute just in case.

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 255 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

It says true testing value says $false

Done.

---

Comments from Reviewable

[mbreakey3]

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[mbreakey3]

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 3 at r2 (raw file):

read-only
Please update the synopsis since it is not 100% accurate. $WriteAccess parameter opens the key with write access.

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 111 at r2 (raw file):

$registryValueExists

Check the value here. If it is false, then return $false here. This way the rest of the function code does not need to check for this every time.

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 168 at r2 (raw file):

Test-Path -Path $parentPath

Why not use Test-RegistryKeyExists here too?

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 318 at r2 (raw file):

$registryDrivePath = Split-Path -Path $KeyPath -Qualifier
$registryDrive = Get-Item -Path $registryDrivePath
 
$subKeyPath = Split-Path -Path $KeyPath -NoQualifier
$subKeyPath = $subKeyPath.Substring(1)
 
$registryKey = $registryDrive.OpenSubKey($subKeyPath, $true)

Use Get-RegistryKey.

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 376 at r2 (raw file):

$registryDriveRootMappings[$mappingKey]

This is the same as $driveName. So no need to lookup $mappingKey for this translation. Just use $driveName.

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 376 at r2 (raw file):

@newPSDriveParams

Where is this coming from?

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 18 at r1 (raw file):

Previously, kwirkykat (Katie Keim) wrote…

It's very unlikely, but I added a timeout of 1 minute just in case.

Katie is right. It is very unlikely. So I believe the timeout is overkill. Also, you can use a GUID and/or some format of the current date and time.

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 31 at r2 (raw file):

if (Test-RegistryKeyExists -KeyPath $script:registryKeyPath)

What's the point of this IF statement here?

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 33 at r2 (raw file):

$script:doNotDeleteRegistryKey = $true
throw "Timed out while attempting to set up a non-destructive registry key for testing. Last testing key attempted: $script:registryKeyPath"
return

Why set a variable right before throwing an exception? And what's the return for?

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 56 at r2 (raw file):

$script:doNotDeleteRegistryKey

I don't see the need for this variable. If the key is supposed to be a test key created for this test suite only, why should we not delete it afterwards?

---

Comments from Reviewable

[zjalali]

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 114 at r2 (raw file):

$registryKeyTreePath

Why not define this in BeforeAll too. It is common between at least two tests.

---

Comments from Reviewable

[kwirkykat]

@zjalali I will submit a new PR with changes per your comments 👍

---

Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 3 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

read-only
Please update the synopsis since it is not 100% accurate. $WriteAccess parameter opens the key with write access.

Will fix in new PR.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 111 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

$registryValueExists

Check the value here. If it is false, then return $false here. This way the rest of the function code does not need to check for this every time.

Will fix in new PR.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 168 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

Test-Path -Path $parentPath

Why not use Test-RegistryKeyExists here too?

Will fix in new PR.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 318 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

$registryDrivePath = Split-Path -Path $KeyPath -Qualifier
$registryDrive = Get-Item -Path $registryDrivePath
 
$subKeyPath = Split-Path -Path $KeyPath -NoQualifier
$subKeyPath = $subKeyPath.Substring(1)
 
$registryKey = $registryDrive.OpenSubKey($subKeyPath, $true)

Use Get-RegistryKey.

Will fix in new PR.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 376 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

@newPSDriveParams

Where is this coming from?

Ick. This is leftover from copying code. Will fix in new PR.

---

Tests/Unit/MSFT_xRegistryResource.TestHelper.psm1, line 376 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

$registryDriveRootMappings[$mappingKey]

This is the same as $driveName. So no need to lookup $mappingKey for this translation. Just use $driveName.

Will fix in new PR.

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 31 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

if (Test-RegistryKeyExists -KeyPath $script:registryKeyPath)

What's the point of this IF statement here?

This is to check if we have timed out or if we have found a registry key that does not exist.

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 33 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

$script:doNotDeleteRegistryKey = $true
throw "Timed out while attempting to set up a non-destructive registry key for testing. Last testing key attempted: $script:registryKeyPath"
return

Why set a variable right before throwing an exception? And what's the return for?

Pester still runs the AfterAll block even after I throw the error and return. The return is just making sure that Pester STOPS after the error rather than continuing the tests. It's probably overkill.

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 56 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

$script:doNotDeleteRegistryKey

I don't see the need for this variable. If the key is supposed to be a test key created for this test suite only, why should we not delete it afterwards?

Pester runs this block last ALWAYS. Even if the timeout occurs and an error is thrown, it still runs this block. When I tested the timeout it was still removing the existing key which would be considered destructive to the test-runner's system.

---

Tests/Unit/MSFT_xRegistryResource.Tests.ps1, line 114 at r2 (raw file):

Previously, zjalali (Zia Jalali) wrote…

$registryKeyTreePath

Why not define this in BeforeAll too. It is common between at least two tests.

Eh. It's only in two tests. I don't want to clutter up the BeforeAll even more just for the two tests.

---

Comments from Reviewable

[ArieHein]

should have probably dealt with #114 as well, while at it, its not a big change either.