fix for Issue #8

[J0F3]

Pull Request (PR) description
Added a Check in the CheckForPreReqs Function so that the installation of the Windows Feature "RSAT-Feature-Tools-BitLocker-RemoteAdminTool" on Server Core because it is only available on Full Server.

This Pull Request (PR) fixes the following issues:
Fixes #8

---

This change is 

[johlju]

@J0F3 If I review this PR do you have time to work on it?

[johlju]

Please rebase this PR to get newly merged changes.

---

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

---

README.md, line 8 at r1 (raw file):

This DSC Module allows you to configure Bitlocker on a single disk, configure a TPM chip, or automatically enable Bitlocker on multiple disks.

This project has adopted the [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/).

We should not remove this.

---

README.md, line 102 at r1 (raw file):

### Unreleased
* Corrected an issue in CheckForPreReqs function where on Server Core the installation of the non existing Windows Feature 'RSAT-Feature-Tools-BitLocker-RemoteAdminTool' was erroneously checked.

If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.

* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)`

---

Misc/xBitlockerCommon.psm1, line 424 at r1 (raw file):

"Installed"

We should use single quotes where applicable. Throughout this row.

---

Misc/xBitlockerCommon.psm1, line 523 at r1 (raw file):

function Get-OSEdition
{
    (Get-ItemProperty -Path "HKLM:/software/microsoft/windows nt/currentversion" -Name InstallationType).InstallationType

We should use single quotes where applicable.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 5 at r1 (raw file):

InModuleScope 'xBitlockerCommon' {
        Describe 'xBitlockerCommon' {

This should move back one indent.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 17 at r1 (raw file):

        }

        Context "Verifing PreReqs on Windows Server Core with all required features" {

We should start Context -block with 'When...'. Throughout.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 17 at r1 (raw file):

        }

        Context "Verifing PreReqs on Windows Server Core with all required features" {

We should use single quotes where applicable. Throughout this file.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 33 at r1 (raw file):

                    return $null
                }
                else {

We should have open brace on a separate row.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 66 at r1 (raw file):

            }
            It "Should run the CheckForPreReqs function without exceptions" {
                CheckForPreReqs

This should use `Should -Not -Throw'

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

                    return $null
                }
                else {

We should have open brace on a separate row.

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #9 into dev will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev       #9   +/-   ##
=======================================
  Coverage   26.08%   26.08%           
=======================================
  Files           3        3           
  Lines          92       92           
=======================================
  Hits           24       24           
  Misses         68       68

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 609e54d...91660f1. Read the comment docs.

[johlju]

When you have resolved the review comments, or if you need to discuss a comment. Could you please go into Reviewable and write 'Done' (or click the done-button) on each of the review comments. This allows me to acknowledge (resolve) the review comments and I know that you are done with each change. After you written 'Done' on each review comment, or comment on the review comment if it needs further discussion, then click the big green 'Publish' button on top of the page. That will send all your comments back as one big comment to this PR.
You find Reviewable if you click on the big purple Reviewable-button in the PR description.

[J0F3]

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

README.md, line 8 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should not remove this.

Done.

---

README.md, line 102 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.

* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)`

Done.

---

Misc/xBitlockerCommon.psm1, line 424 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"Installed"

We should use single quotes where applicable. Throughout this row.

Done.

---

Misc/xBitlockerCommon.psm1, line 523 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use single quotes where applicable.

Done.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 5 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should move back one indent.

Done.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 17 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use single quotes where applicable. Throughout this file.

Done.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 17 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should start Context -block with 'When...'. Throughout.

Done.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 33 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have open brace on a separate row.

Done.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 66 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should use `Should -Not -Throw'

Done.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have open brace on a separate row.

Done.

---

Comments from Reviewable

[J0F3]

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Done.

Can not do that because this will alter the logic of the mocked function. (return value will be null and not the hash table)

---

Comments from Reviewable

[J0F3]

Sorry about the delay. I have rebased the PR and looked through the comments now.
Thx

[johlju]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 17 at r1 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Done.

Please also use single quotes around description for Context- and It-blocks. Throughout.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Can not do that because this will alter the logic of the mocked function. (return value will be null and not the hash table)

Sorry, I'm not following you here :/ The code here looks good to me.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 23 at r3 (raw file):

            Mock -CommandName Get-WindowsFeature -MockWith {
                param

We can move the param block to the 'empty function' so we don't have to duplicate that in every mock. Throughout.

---

Comments from Reviewable

[J0F3]

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 23 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We can move the param block to the 'empty function' so we don't have to duplicate that in every mock. Throughout.

Done.

---

Comments from Reviewable

[J0F3]

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Sorry, I'm not following you here :/ The code here looks good to me.

I thought that you mean the open brace of the hash table of the return value should be on a separate row but you probably meant the brace after the else just above. So everything fine anyway :-)

---

Comments from Reviewable

[J0F3]

I also have just realized that we had now two unit tests for xBitlockerCommon.ps1. So I just merged mine into the file under Tests\Unit so we have just one file with all test in it.

[johlju]

Looking good now! Just a tiny review comment left 🙂

---

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

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 193 at r4 (raw file):

                    }
                }
                It 'The CheckForPreReqs function should throw an exceptions about missing Windows Feature' {

If we mock Write-Errorwe can get rid of the text that looks like an error in the tests output
https://ci.appveyor.com/project/PowerShell/xbitlocker/build/1.1.54.0#L614

If we mock Write-Error then we could use this after the test for Should -Throw to verify that Write-Error was called correctly.

`Assert-MockCalled -Command `Write-Error` -Exactly -Time 1 -Scope It

I throw me twice now thinking why the test fail but did not report failure 😃

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

I thought that you mean the open brace of the hash table of the return value should be on a separate row but you probably meant the brace after the else just above. So everything fine anyway :-)

Ah, yes, it was the else open brace I meant :)

---

Comments from Reviewable

[johlju]

There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.

[J0F3]

I have rebased again against dev so merge conflict should be resolved now...

Review status: 2 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 193 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If we mock Write-Errorwe can get rid of the text that looks like an error in the tests output
https://ci.appveyor.com/project/PowerShell/xbitlocker/build/1.1.54.0#L614

If we mock Write-Error then we could use this after the test for Should -Throw to verify that Write-Error was called correctly.

`Assert-MockCalled -Command `Write-Error` -Exactly -Time 1 -Scope It

I throw me twice now thinking why the test fail but did not report failure 😃

Ah that's what I was looking into but could not find a solution how to get rid of the error output.
So lets try. Not sure if this is correct this way. Is it? At least the error output is now gone an the test look nice and green :-)

---

Test/Unit/xBitlockerCommon.tests.ps1, line 86 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah, yes, it was the else open brace I meant :)

👍

---

Comments from Reviewable

[johlju]

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

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 193 at r4 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Ah that's what I was looking into but could not find a solution how to get rid of the error output.
So lets try. Not sure if this is correct this way. Is it? At least the error output is now gone an the test look nice and green :-)

Looking good! Just a few tweaks, and I think we are all good 🙂 See other comments.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 33 at r6 (raw file):

        }

        function Write-Error

This should not be needed for cmdlets that are part of "default PowerShell". LEt's try without this.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 199 at r6 (raw file):

                }
                It 'The CheckForPreReqs function should throw an exceptions about missing Windows Feature' {
                    Mock -CommandName Write-Error -MockWith {

I think it should be enough with Mock -CommandName Write-Error. We should not need -MockWith on this, because we want to test so that throw statement work on line 437 in the code?
If we just do an empty mock of Write-Error then we can test so it gets called with the Assert-MockCalled you added.

If Write-Error gets called more than once, change -Times 1 to -Times 2 in the Assert-MockCalledetc.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 204 at r6 (raw file):

-Time

Should be -Times (ending with 's'). I spelled it wrong in the previous comment. :/

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 204 at r6 (raw file):

                    {CheckForPreReqs} | Should -Throw 'Required Bitlocker features need to be installed before xBitlocker can be used'
                    Assert-MockCalled -Command Write-Error -Exactly -Time 1 -Scope It

This comment is non-blocking, meaning code is good as-is, but mentioning if you want to try it.
If you do want to test if Write-Error is actually called with the right message, then you can add this to the Assert-MockCalled (assuming I got the correct error message :)

Assert-MockCalled -Command Write-Error -Exactly -Times 1 -Scope It -ParameterFilter {
    $Message -eq 'The RSAT-Feature-Tools-BitLocker-RemoteAdminTool feature needs to be installed before the xBitlocker module can be used'
}

---

Comments from Reviewable

[J0F3]

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

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 33 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should not be needed for cmdlets that are part of "default PowerShell". LEt's try without this.

Yup, works without. 👍

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 199 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think it should be enough with Mock -CommandName Write-Error. We should not need -MockWith on this, because we want to test so that throw statement work on line 437 in the code?
If we just do an empty mock of Write-Error then we can test so it gets called with the Assert-MockCalled you added.

If Write-Error gets called more than once, change -Times 1 to -Times 2 in the Assert-MockCalledetc.

Ah, learned again something. thx :-) (removed the -MockWith)

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 204 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Time

Should be -Times (ending with 's'). I spelled it wrong in the previous comment. :/

Done.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 204 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This comment is non-blocking, meaning code is good as-is, but mentioning if you want to try it.
If you do want to test if Write-Error is actually called with the right message, then you can add this to the Assert-MockCalled (assuming I got the correct error message :)

Assert-MockCalled -Command Write-Error -Exactly -Times 1 -Scope It -ParameterFilter {
    $Message -eq 'The RSAT-Feature-Tools-BitLocker-RemoteAdminTool feature needs to be installed before the xBitlocker module can be used'
}

</blockquote></details>

I am stuck here. Can not get to work this. What should be Line 195 be then? Still `Should -Throw 'Required Bitlocker features need to be installed before xBitlocker can be used'` or only `Should -Throw`?

Pester says now `Expected Write-Error in module xBitlockerCommon to be called 1 times exactly but was called 0 times`

But when I change it to the this: 
`                    
Assert-MockCalled -Command Write-Error -Exactly -Times 1 -Scope It -ParameterFilter {
                        $Message
                        $Message -eq 'Required Bitlocker features need to be installed before xBitlocker can be used'
                    }
`
Makes not really sense to me. What I am missing here?

Thanks for you help! :-)

---


*Comments from [Reviewable](https://reviewable.io/reviews/powershell/xbitlocker/9)*
<!-- Sent from Reviewable.io -->

[johlju]

Review status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @johlju)

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 204 at r6 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

I am stuck here. Can not get to work this. What should be Line 195 be then? Still Should -Throw 'Required Bitlocker features need to be installed before xBitlocker can be used' or only Should -Throw?

Pester says now Expected Write-Error in module xBitlockerCommon to be called 1 times exactly but was called 0 times

But when I change it to the this:
Assert-MockCalled -Command Write-Error -Exactly -Times 1 -Scope It -ParameterFilter { $Message $Message -eq 'Required Bitlocker features need to be installed before xBitlocker can be used' }
Makes not really sense to me. What I am missing here?

Thanks for you help! :-)

Happy to help. It should look something like this. throw is called from the code, so | Should -Throw will catch that, we should not mock that. We want to test so the code throws, not our mock.

NOTE! You must remove the stub function Write-Error you have in the beginning of the InModuleScope-block for this to work (see a review comment).
The Mock command will mock all the parameters from the original command, then we can use ParameterFilter. When there are no parameters in the stub function the filter won't work. Also we should not make stub functions for cmdlets that are native to PowerShell (native as exist by default without needing to install a module). :)

It 'The CheckForPreReqs function should throw an exceptions about missing Windows Feature' {
    Mock -CommandName Write-Error

    {CheckForPreReqs} | Should -Throw 'Required Bitlocker features need to be installed before xBitlocker can be used'
    Assert-MockCalled -Command Write-Error -ParameterFilter {
        $Message -match 'The RSAT-Feature-Tools-BitLocker-RemoteAdminTool feature needs to be installed before the xBitlocker module can be used'
    } -Exactly -Times 1 -Scope It
}

---

Comments from Reviewable

[J0F3]

Review status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @johlju)

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 204 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Happy to help. It should look something like this. throw is called from the code, so | Should -Throw will catch that, we should not mock that. We want to test so the code throws, not our mock.

NOTE! You must remove the stub function Write-Error you have in the beginning of the InModuleScope-block for this to work (see a review comment).
The Mock command will mock all the parameters from the original command, then we can use ParameterFilter. When there are no parameters in the stub function the filter won't work. Also we should not make stub functions for cmdlets that are native to PowerShell (native as exist by default without needing to install a module). :)

It 'The CheckForPreReqs function should throw an exceptions about missing Windows Feature' {
    Mock -CommandName Write-Error

    {CheckForPreReqs} | Should -Throw 'Required Bitlocker features need to be installed before xBitlocker can be used'
    Assert-MockCalled -Command Write-Error -ParameterFilter {
        $Message -match 'The RSAT-Feature-Tools-BitLocker-RemoteAdminTool feature needs to be installed before the xBitlocker module can be used'
    } -Exactly -Times 1 -Scope It
}

Thank you very much! I got it now (I think :-) ). The error output was actually from the Write-Error statements in the code and not from the Throw. I miss interpreted that. So I rewritten the whole this a little bit and added also a test case for server core without the needed features which was completely missing before. Further I added also some test for the Get-OSEdition function so we have that also covered.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 197 at r7 (raw file):

                It 'Should give an error that Bitlocker Windows Feature needs to be installed' {
                    {CheckForPreReqs} | Should -Throw

Not sure if this is the right solution. Actually I testing here always two things. But could not find any other way to suppress the exception from throw so I can only test for the Write-Error messages.
And, I think, all together in one It statement would also not be ideal.
So open for suggestion if there is a better way to test only the Write-Error messages while ignoring the throw exception (which get tested separate).
Thx!

---

Comments from Reviewable

[johlju]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed, 3 unresolved discussions (waiting on @J0F3)

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 141 at r7 (raw file):

{}

We don't need this when we don't mock any logic. This can be removed. Throughout the mocks.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 143 at r7 (raw file):

-Time 0

Should be -Times 0. Please change to -Times (with 's') throughout the tests.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 197 at r7 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Not sure if this is the right solution. Actually I testing here always two things. But could not find any other way to suppress the exception from throw so I can only test for the Write-Error messages.
And, I think, all together in one It statement would also not be ideal.
So open for suggestion if there is a better way to test only the Write-Error messages while ignoring the throw exception (which get tested separate).
Thx!

I don't think there is a way to mock throw, so separating them is not possible.

I think this is the correct way. You want to test what happens in the code, and the code calls Write-Error and then throws. So it's correct to test that in the same It-block.

The problem is probably the code. It should just build a string and throw one correct error message, or use Write-Warning instead of Write-Error. Using Write-Error several times and then throw seems wrong. But that can be another PR. 🙂

---

Comments from Reviewable

[J0F3]

Review status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 141 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

{}

We don't need this when we don't mock any logic. This can be removed. Throughout the mocks.

Done.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 143 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Time 0

Should be -Times 0. Please change to -Times (with 's') throughout the tests.

Doh! Missed that again... 🤦‍♂️
But interesting that it work also without the 's'.

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 197 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I don't think there is a way to mock throw, so separating them is not possible.

I think this is the correct way. You want to test what happens in the code, and the code calls Write-Error and then throws. So it's correct to test that in the same It-block.

The problem is probably the code. It should just build a string and throw one correct error message, or use Write-Warning instead of Write-Error. Using Write-Error several times and then throw seems wrong. But that can be another PR. 🙂

Ok, fine.
Yes I thought the same. The code is a little bit strange. However it was already there an I only added the adjustment for the missing Windows Features on Core Server.
Maybe it would also make more sense when the function just returns $true or $false and the exception is throw in the code which calls the function. But as you wrote that would be an other PR. :-)

---

Comments from Reviewable

[johlju]

---

Reviewed 1 of 1 files at r8.
Review status: complete! all files reviewed, all discussions resolved

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 143 at r7 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Doh! Missed that again... 🤦‍♂️
But interesting that it work also without the 's'.

You only have to specify the name of a named parameter until it's unique, like Write-Verbose -M 'hej' -V works. But for the style guideline we have to write out the entire named parameter. 🙂

---

Tests/Unit/xBitlockerCommon.tests.ps1, line 197 at r7 (raw file):

Previously, J0F3 (Jonas Feller) wrote…

Ok, fine.
Yes I thought the same. The code is a little bit strange. However it was already there an I only added the adjustment for the missing Windows Features on Core Server.
Maybe it would also make more sense when the function just returns $true or $false and the exception is throw in the code which calls the function. But as you wrote that would be an other PR. :-)

That solution returning a Boolean would be even better yes. 🙂 Good as-is for now.

---

Comments from Reviewable

[johlju]

@J0F3 Awesome work on this one! Thanks! 😃

[J0F3]

Thanks @johlju and thank you for your support! 😊