Added xCertificateExport Resource - Fixes #41

[PlagueHO]

This PR primarily adds the xCertificateExport resource. It makes a few other structural changes to bring it inline with changes in xNetworking and the changes to move AppVeyor and other code to the DSCResource.Tests.

Fixes #41
Fixes

Change in detail:

• Moved shared modules into modules folder.
• xCertificateExport:
  • Added new resource.
• Cleanup xCertificate.psd1 to remove unneccessary properties.
• Converted AppVeyor.yml to use DSCResource.tests shared code.
• Opted-In to markdown rule validation.
• Examples modified to meet standards for auto documentation generation.

Tagging @johlju

---

This change is 

[johlju]

I have not review the files for the ExportCertificate resource yet. Went thru the rest to get them done first. :)

---

Reviewed 32 of 37 files at r1.
Review status: 26 of 31 files reviewed at latest revision, 22 unresolved discussions.

---

ReadMe.md, line 73 at r1 (raw file):

- **`[PSCredential]` Password** (_Write_): Specifies the password used to protect an exported PFX file.
- **`[String[]]` ProtectTo** (_Write_): Specifies an array of strings for the username or group name that can access the private key of an exported PFX file without any password.

Missing the Read parameter IsExported.

---

DSCResources/MSFT_xCertificateImport/MSFT_xCertificateImport.psm1, line 71 at r1 (raw file):

        $Store,

        [Parameter()]

Why did you remove the [Parameter()]? :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1, line 87 at r1 (raw file):

        $CARootName,

        [Parameter()]

Why did you remove the [Parameter()]? :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

DSCResources/MSFT_xPfxImport/MSFT_xPfxImport.psm1, line 77 at r1 (raw file):

        $Store,

        [Parameter()]

Why did you remove the [Parameter()]? :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertificateExport/1-CertByFriendlyName.ps1, line 9 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost'

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertificateExport/2-PfxByFriendlyName.ps1, line 9 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost',

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertificateExport/2-PfxByFriendlyName.ps1, line 11 at r1 (raw file):

        [string[]] $NodeName = 'localhost',

        [Parameter(Mandatory=$true)]

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertificateImport/1-MinimalUsage.ps1, line 9 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost'

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertReq/1-RequestAltSSLCert.ps1, line 15 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost',

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertReq/1-RequestAltSSLCert.ps1, line 19 at r1 (raw file):

        [Parameter(Mandatory = $true)]
        [ValidateNotNullorEmpty()]
        [PSCredential] $Credential

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertReq/2-RequestSSLCert.ps1, line 14 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost',

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xCertReq/2-RequestSSLCert.ps1, line 16 at r1 (raw file):

        [string[]] $NodeName = 'localhost',

        [Parameter(Mandatory=$true)]

Spaces around equal sign

Please format the parameter correctly. :)

---

Examples/Resources/xCertReq/2-RequestSSLCert.ps1, line 18 at r1 (raw file):

        [Parameter(Mandatory=$true)]
        [ValidateNotNullorEmpty()]
        [PSCredential] $Credential

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xPfxImport/1-IIS_WebSite.ps1, line 9 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost',

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xPfxImport/1-IIS_WebSite.ps1, line 13 at r1 (raw file):

        [Parameter(Mandatory=$true)]
        [ValidateNotNullorEmpty()]
        [PSCredential] $Credential

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xPfxImport/2-MinimalUsage.ps1, line 9 at r1 (raw file):

    param
    (
        [string[]] $NodeName = 'localhost',

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Examples/Resources/xPfxImport/2-MinimalUsage.ps1, line 13 at r1 (raw file):

        [Parameter(Mandatory=$true)]
        [ValidateNotNullorEmpty()]
        [PSCredential] $Credential

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

---

Modules/CertificateDsc.Common/CertificateDSc.Common.psm1, line 1 at r1 (raw file):

# Import the Networking Resource Helper Module

Does this file need full review?

---

Modules/CertificateDsc.ResourceHelper/CertificateDsc.ResourceHelper.psm1, line 1 at r1 (raw file):

<#

Does this file need full review?

---

Tests/TestHelpers/CommonTestHelper.psm1, line 1 at r1 (raw file):

<#

Does this file need full review?

---

Tests/Unit/CertificateDsc.Common.tests.ps1, line 1 at r1 (raw file):

$script:ModuleName = 'CertificateDsc.Common'

Rename the file .Tests.ps1 with upper 'T'?

Does this file need full review?

---

Tests/Unit/MSFT_xCertificateExport.tests.ps1, line 3 at r1 (raw file):

$script:DSCModuleName      = 'xCertificate'
$script:DSCResourceName    = 'MSFT_xCertificateExport'

Rename the file .Tests.ps1 with upper 'T'?

---

Comments from Reviewable

[johlju]

@PlagueHO Will continue review tomorrow.

[PlagueHO]

Thanks @johlju!

---

Review status: 26 of 31 files reviewed at latest revision, 22 unresolved discussions.

---

ReadMe.md, line 73 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missing the Read parameter IsExported.

Done.

---

DSCResources/MSFT_xCertificateImport/MSFT_xCertificateImport.psm1, line 71 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why did you remove the [Parameter()]? :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

My bad! I got mixed up with the style guidelines we use at work! :( Fixed.

---

DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1, line 87 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why did you remove the [Parameter()]? :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done! I did every other one as well.

---

DSCResources/MSFT_xPfxImport/MSFT_xPfxImport.psm1, line 77 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why did you remove the [Parameter()]? :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertificateExport/1-CertByFriendlyName.ps1, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertificateExport/2-PfxByFriendlyName.ps1, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertificateExport/2-PfxByFriendlyName.ps1, line 11 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertificateImport/1-MinimalUsage.ps1, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertReq/1-RequestAltSSLCert.ps1, line 15 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertReq/1-RequestAltSSLCert.ps1, line 19 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertReq/2-RequestSSLCert.ps1, line 14 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xCertReq/2-RequestSSLCert.ps1, line 16 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Spaces around equal sign

Please format the parameter correctly. :)

Fixed via global search replace :) Done

---

Examples/Resources/xCertReq/2-RequestSSLCert.ps1, line 18 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xPfxImport/1-IIS_WebSite.ps1, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xPfxImport/1-IIS_WebSite.ps1, line 13 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xPfxImport/2-MinimalUsage.ps1, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Examples/Resources/xPfxImport/2-MinimalUsage.ps1, line 13 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please format the parameter correctly. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-parameter-block

Done.

---

Modules/CertificateDsc.Common/CertificateDSc.Common.psm1, line 1 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Does this file need full review?

Sorry :( Copy paste error. Only the new function should need a review.

---

Modules/CertificateDsc.ResourceHelper/CertificateDsc.ResourceHelper.psm1, line 1 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Does this file need full review?

I don't think so.

---

Tests/TestHelpers/CommonTestHelper.psm1, line 1 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Does this file need full review?

Don't think so.

---

Tests/Unit/CertificateDsc.Common.tests.ps1, line 1 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Rename the file .Tests.ps1 with upper 'T'?

Does this file need full review?

Done. Only the tests for Find-Certificate need review I think.

---

Tests/Unit/MSFT_xCertificateExport.tests.ps1, line 3 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Rename the file .Tests.ps1 with upper 'T'?

Done. I also renamed the other Tests file with capital 'T'

---

Comments from Reviewable

[johlju]

No problem :) Just wish I could do this full time. :) I don't have so much more time tonight. I will review the *Export* files as soon as possible.

---

Reviewed 4 of 37 files at r1, 13 of 15 files at r2.
Review status: 25 of 31 files reviewed at latest revision, 3 unresolved discussions.

---

Tests/Unit/MSFT_xCertificateExport.tests.ps1, line 3 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done. I also renamed the other Tests file with capital 'T'

Hmm. It doesn't seem to take. I think I had to remove and add a file for it to get the file rename. :/ Must be a better way.

---

Comments from Reviewable

[johlju]

Reviewed 1 of 11 files at r3, 3 of 3 files at r4.
Review status: 32 of 34 files reviewed at latest revision, 6 unresolved discussions.

---

Tests/Unit/CertificateDsc.Common.Tests.ps1, line 190 at r4 (raw file):

            Context 'Thumbprint only is passed and matching certificate exists' {
                Mock `

Actually, instead of repeating the same mock, you should add the Mock in a BeforeEach-block inside the Describe-block. That will trigger before each It-block.
I don't know if that will break Assert-VerifiableMocks (giving false positive again) but either way I think you should only use Assert-VerifiableMocks one time at the end of the Describe-block and in the It-block use Assert-MockCalled.

Would this work here?

Inside the InModuleScope-block ($mockCertificateStorePath is dynamically assigned during runtime.

$mockGetChildItemParameterFilter = {
    $Path -eq $mockCertificateStorePath
}

Inside BeforeEach-block in the describe-block.

Mock `
    -CommandName Get-ChildItem `
    -MockWith { @( $validCert ) } `
    -ParameterFilter $mockGetChildItemParameterFilter `
    -Verifiable

Before any needed Context-block or/and It-block to be tested. Dynamically change the store path in the mock parameter filter.

$mockCertificateStorePath = 'cert:\LocalMachine\My'

Inside the Assert It-block

Assert-MockCalled `
    -CommandName Get-ChildItem `
    -ParameterFilter $mockGetChildItemParameterFilter `
    -Scope It

At the end of the Describe-block

Assert-VerifiableMocks

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 128 at r4 (raw file):

                    $Result.IsExported | Should Be $True
                }
                It 'should call the expected mocks' {

Blank rows between the blocks here to please. Throughout the file :)

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 166 at r4 (raw file):

Needs to be done because Export-Certificate requires a real cert object

Don't follow you here.
Do you mean 'Needs to be done because the original Export-Certificate cmdlet requires a real certificate object, so it is not possible to mock the original cmdlet'?

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 177 at r4 (raw file):

                    )
                }
                Mock `

Blank row before this row, and between all the other Mocks. (align with guide, and makes it easier for the eyes to review too)

Curious, is this a style of coding you are using? I have seen several contributors putting blocks together. That can't be a coincidence. :) Since my C coding days I always been taught to leave room between each block, so I do it without thinking 😃

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 187 at r4 (raw file):

                    -CommandName Export-PfxCertificate

                It 'should not throw exception' {

Just marking a point where I should continue the review.

---

Comments from Reviewable

[johlju]

@PlagueHO You're welcome! 😄 I'm kind of used to bigger changes from xSQLServer 😄

Okay, just one more left! Looking forward to this one. First time reviewing an integration test I think. Looking forward to learning a lot.

---

Reviewed 2 of 11 files at r3.
Review status: 33 of 34 files reviewed at latest revision, 37 unresolved discussions.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 26 at r4 (raw file):

        $DSCResourceName = 'MSFT_xCertificateExport'

        $certPath = Join-Path -Path $ENV:Temp -ChildPath 'xCertificateExportTestCert.cer'

Minor: I rather see all these variables with 'cert' written out to 'certificate'. This and the 8 below.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 36 at r4 (raw file):

        $certStore = 'My'

        $validCert = New-Object -TypeName PSObject -Property @{

Please change to $validCertificate

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 56 at r4 (raw file):

        }

        $validCertParams = @{

Please change to $validCertificateParameters

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 71 at r4 (raw file):

        }

        $validCertMatchSourceParams = @{} + $validCertParams

Please change to $validCertificateMatchSourceParameters

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 76 at r4 (raw file):

        $pfxPlainTextPassword = 'P@ssword!1'
        $pfxPassword = ConvertTo-SecureString -String $pfxPlainTextPassword -AsPlainText -Force
        $pfxCred = New-Object -TypeName System.Management.Automation.PSCredential `

Please change to $pfxCredential, or $pfxCredentialObject, or even $pfxPasswordCredentialObject (since the parameter is Password and is an credential object 😄 )

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 79 at r4 (raw file):

            -ArgumentList ('Dummy',$pfxPassword)

        $validPfxParams = @{

Please change to $validPfxParameters

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 96 at r4 (raw file):

        }

        $validPfxMatchSourceParams = @{} + $validPfxParams

Please change to $validPfxMatchSourceParameters

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 106 at r4 (raw file):

            Import($Path,$Password,$Flags) { }
        }
        class X509Certificate2CollectionDummyNoMatch:System.Object {

Please add a blank row before this one.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 113 at r4 (raw file):

        }

        $importedCertMatch = New-Object -Type X509Certificate2CollectionDummyMatch

Please change to $importedCertificateMatch

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 114 at r4 (raw file):

        $importedCertMatch = New-Object -Type X509Certificate2CollectionDummyMatch
        $importedCertNoMatch = New-Object -Type X509Certificate2CollectionDummyNoMatch

Please change to $importedCertificateNoMatch

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 152 at r4 (raw file):

        Describe "$DSCResourceName\Set-TargetResource" {
            Context 'Certificate is not found' {
                Mock `

Could you move these to a BeforeEach-block and make them dynamically instead?

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 161 at r4 (raw file):

                }
                It 'should call the expected mocks' {
                    Assert-VerifiableMocks

Mock Export-Certificate and Export-PfxCertificate to make sure the don't get called. (will be done if you add them to the BeforeEach-block in previous comment)
Change to
Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 0
Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 0
Assert-MockCalled -CommandName Export-PfxCertificate -Exactly -Times 0

Move Assert-VerifiableMocks to the end of the Describe-block

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 188 at r4 (raw file):

                It 'should not throw exception' {
                    { Set-TargetResource @validCertParams -Verbose } | Should Not Throw

You are mocking everything, so is could not possible throw (?). Suggest to change the mock so that if verifies the incoming parameters are assigned the expected parameters (below $mockExpectedFilePath is dynamically set)

i.e (inside the InModuleScope-block)

$mockExportCertificate = {
    if ($FilePath -ne $mockExpectedFilePath)
    {
        throw ' Expected mock to be called with {0}, but was {1}' -f $mockExpectedFilePath,$FilePath
    }
}

Mock the cmdlet

Mock -CommandName Export-Certificate -MockWith $mockExportCertificate -Verifiable

Before calling Set-TargetResource

$mockExpectedFilePath = 'TestDrive:\exportedcertificate.cer'

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 191 at r4 (raw file):

                }
                It 'should call the expected mocks' {
                    Assert-VerifiableMocks

Change to `Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 1

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 192 at r4 (raw file):

                It 'should call the expected mocks' {
                    Assert-VerifiableMocks
                    Assert-MockCalled -CommandName Export-PfxCertificate -Times 0

Must have -Exactly on this one.
https://github.com/pester/Pester/wiki/Assert-MockCalled#exactly

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 223 at r4 (raw file):

                It 'should not throw exception' {
                    { Set-TargetResource @validPfxParams -Verbose } | Should Not Throw

Same as previous comment regarding mocking

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 225 at r4 (raw file):

                    { Set-TargetResource @validPfxParams -Verbose } | Should Not Throw
                }
                It 'should call the expected mocks' {

Same as the previous comments regarding the assertions

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 234 at r4 (raw file):

        Describe "$DSCResourceName\Test-TargetResource" {
            Context 'Certificate is not found' {
                Mock `

You could do these dynamically in a BeforeEach-block as well. Maybe? I'm good with how they are here in the Test Describe-block. If making it dynamically it makes more code. But was hoping it could use same code as in the Set-method testing.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 240 at r4 (raw file):

                It 'should return true' {
                    Test-TargetResource @validCertParams -Verbose | Should Be $True

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 243 at r4 (raw file):

                }
                It 'should call the expected mocks' {
                    Assert-VerifiableMocks

Rather have Assert-MockCalled here, and move this to the end of the Describe-block

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 255 at r4 (raw file):

                Mock `
                    -CommandName Test-Path `
                    -MockWith { $False } `

$false (lower 'f')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 259 at r4 (raw file):

                It 'should return false' {
                    Test-TargetResource @validCertParams -Verbose | Should Be $False

$false (lower 'f')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 273 at r4 (raw file):

                Mock `
                    -CommandName Test-Path `
                    -MockWith { $True } `

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 277 at r4 (raw file):

                It 'should return true' {
                    Test-TargetResource @validCertParams -Verbose | Should Be $True

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 291 at r4 (raw file):

                Mock `
                    -CommandName Test-Path `
                    -MockWith { $True } `

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 299 at r4 (raw file):

                It 'should return true' {
                    Test-TargetResource @validCertMatchSourceParams -Verbose | Should Be $True

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 313 at r4 (raw file):

                Mock `
                    -CommandName Test-Path `
                    -MockWith { $True } `

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 321 at r4 (raw file):

                It 'should return false' {
                    Test-TargetResource @validCertMatchSourceParams -Verbose | Should Be $False

$false (lower 'f')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 335 at r4 (raw file):

                Mock `
                    -CommandName Test-Path `
                    -MockWith { $True } `

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 343 at r4 (raw file):

                It 'should return true' {
                    Test-TargetResource @validPfxMatchSourceParams -Verbose | Should Be $True

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 357 at r4 (raw file):

                Mock `
                    -CommandName Test-Path `
                    -MockWith { $True } `

$true (lower 't')

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 365 at r4 (raw file):

                It 'should return false' {
                    Test-TargetResource @validPfxMatchSourceParams -Verbose | Should Be $False

$false(lower 'f')

---

Comments from Reviewable

[johlju]

I think it would be better to have a parameter to where to install/save the script (and not hardcode to $env:Temp. Then the integration test will not break if this function changes.

[johlju]

Adding this Reviewable instead.

[johlju]

Reviewed 1 of 39 files at r1.
Review status: all files reviewed at latest revision, 57 unresolved discussions.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 26 at r4 (raw file):

$ConfigFile

Please change to $configurationFile

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 31 at r4 (raw file):

    Describe "$($script:DSCResourceName)_Integration" {
        # Download and dot source the New-SelfSignedCertificateEx script
        . (Install-NewSelfSignedCertificateExScript)

Function should provide a parameter Path, OutputPath or similar where the script will be found when the function returns.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 34 at r4 (raw file):

        # Prepare CER certificate properties
        $script:certPath = Join-Path -Path $ENV:Temp -ChildPath 'xCertificateExportTestCert.cer'

Nitpick: Should we write 'env:' or 'ENV:'. I'm leaning to 'env:'. But that just because I normally write it with lower case. 😄
https://msdn.microsoft.com/en-us/powershell/reference/4.0/microsoft.powershell.core/providers/environment-provider

Rename $script:certPath to $script:certificatePath

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 38 at r4 (raw file):

        # Prepare PFX certificate properties
        $script:pfxPath = Join-Path -Path $ENV:Temp -ChildPath 'xCertificateExportTestCert.pfx'

Why $script on some, and not all or none? I have seen that Katie and Mariah is using $script on some but not all too.
Here: https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_GroupResource.Tests.ps1#L30
But not here: https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_GroupResource.Tests.ps1#L34

This says we should use $script on all so I guess those that does not use it, is just a miss in the review. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#script-environment-and-global-variable-names-include-scope

I forget to enforce this one. If we should enforce this one, and we should since your aiming at HQRM, then this is wrong in all your tests in this module.
But please. Let's fix all files in another PR. This PR is big enough 😉 Please submit an issue to get this fixed.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 42 at r4 (raw file):

        $pfxPlainTextPassword = 'P@ssword!1'
        $pfxPassword = ConvertTo-SecureString -String $pfxPlainTextPassword -AsPlainText -Force
        $pfxCred = New-Object -TypeName System.Management.Automation.PSCredential `

Please change to $pfxCredential, or $pfxPasswordCredentialObject

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 46 at r4 (raw file):

        # Generate the Valid certificate for testing
        $certDNSNames = @('www.fabrikam.com', 'www.contoso.com')

Please change 'cert' to 'certificate' for this and the below 5.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 60 at r4 (raw file):

            -StoreLocation 'LocalMachine' `
            -Exportable
        $script:validThumbprint = $validCert.Thumbprint

Change to $script:validCertificateThumbprint

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 64 at r4 (raw file):

        Context 'Export CERT' {
            #region DEFAULT TESTS
            It 'Should compile without throwing' {

I doesn't just compiles it applies the configuration as well.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 87 at r4 (raw file):

                        -OutputPath $TestDrive `
                        -ConfigurationData $ConfigData
                    Start-DscConfiguration -Path $TestDrive -ComputerName localhost -Wait -Verbose -Force

Blank row before this one

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 88 at r4 (raw file):

                        -ConfigurationData $ConfigData
                    Start-DscConfiguration -Path $TestDrive -ComputerName localhost -Wait -Verbose -Force
                } | Should not throw

Not (upper 'N')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 91 at r4 (raw file):

            }

            It 'should be able to call Get-DscConfiguration without throwing' {

Should (upper 'S')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 92 at r4 (raw file):

            It 'should be able to call Get-DscConfiguration without throwing' {
                { $script:currentCert = Get-DscConfiguration -Verbose -ErrorAction Stop } | Should Not throw

Throw (upper 'T')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 96 at r4 (raw file):

            #endregion

            It 'should have exported a Cert certificate' {

Should (upper 'S')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 97 at r4 (raw file):

            It 'should have exported a Cert certificate' {
                $script:currentCert.IsExported | Should Be $True

$true (lower 't')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 109 at r4 (raw file):

        Context 'Export PFX' {
            #region DEFAULT TESTS
            It 'Should compile without throwing' {

I doesn't just compiles it applies the configuration as well.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 135 at r4 (raw file):

                        -OutputPath $TestDrive `
                        -ConfigurationData $ConfigData
                    Start-DscConfiguration -Path $TestDrive -ComputerName localhost -Wait -Verbose -Force

Blank row before this one.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 136 at r4 (raw file):

                        -ConfigurationData $ConfigData
                    Start-DscConfiguration -Path $TestDrive -ComputerName localhost -Wait -Verbose -Force
                } | Should not throw

Not (Upper 'N')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 139 at r4 (raw file):

            }

            It 'should be able to call Get-DscConfiguration without throwing' {

Should (upper 'S')

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 144 at r4 (raw file):

            #endregion

            It 'should have exported a PFX certificate' {

Should (upper 'S')

---

Comments from Reviewable

[johlju]

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

---

Modules/CertificateDsc.Common/CertificateDSc.Common.psm1, line 264 at r4 (raw file):

        $certFilters += @('($_.Thumbprint -eq $Thumbprint)')
    } # if
    if ($PSBoundParameters.ContainsKey('FriendlyName'))

Could you add blank row after each if-block here?

---

Comments from Reviewable

[johlju]

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

---

Tests/TestHelpers/CommonTestHelper.psm1, line 110 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Adding this Reviewable instead.

Ah, Reviewable caught the comment made from Github. Very nice!

---

Comments from Reviewable

[PlagueHO]

Thanks for reviewing @johlju - I've still got some changes to make to the unit tests, but everything else should be done (except for the changes I don't think should be made). Thanks again! I know how long all this takes 😁

---

Review status: 28 of 38 files reviewed at latest revision, 57 unresolved discussions.

---

Modules/CertificateDsc.Common/CertificateDSc.Common.psm1, line 264 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add blank row after each if-block here?

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 26 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ConfigFile

Please change to $configurationFile

I'm not sure about this one - I'm wondering if it is better to stay inline with the naming used in the Template because that is the convention used in all other repos.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 31 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Function should provide a parameter Path, OutputPath or similar where the script will be found when the function returns.

Done. But using the default Temp folder so no change to this line.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 34 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Nitpick: Should we write 'env:' or 'ENV:'. I'm leaning to 'env:'. But that just because I normally write it with lower case. 😄
https://msdn.microsoft.com/en-us/powershell/reference/4.0/microsoft.powershell.core/providers/environment-provider

Rename $script:certPath to $script:certificatePath

I don't have a preference for $env: . But I am fairly obsessive about consistency within a repo - so I've done a global search/replace to set them all to $env: - so this has resulted in additional files being changed 😢 sorry about that.

I have also changed to $script:certificatePath

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 38 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why $script on some, and not all or none? I have seen that Katie and Mariah is using $script on some but not all too.
Here: https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_GroupResource.Tests.ps1#L30
But not here: https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_GroupResource.Tests.ps1#L34

This says we should use $script on all so I guess those that does not use it, is just a miss in the review. :)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#script-environment-and-global-variable-names-include-scope

I forget to enforce this one. If we should enforce this one, and we should since your aiming at HQRM, then this is wrong in all your tests in this module.
But please. Let's fix all files in another PR. This PR is big enough 😉 Please submit an issue to get this fixed.

Any variable that needs to be accessible in an AfterAll or BeforeAll pester block needs to be in the script scope. I don't want to put every variable as $script scope as that strikes me as bad practice (akin to using global scope when not necessary). So I only put the needed variables in there.

I think this rule is not clear because I don't think the intent is for every variable to be scoped to $Env, $Script or $Global. I think it just means that if a variable is scoped (to anything but $Local) then you must specify the scope everywhere it is used. @kwirkykat - would you like to confirm this?

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 42 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $pfxCredential, or $pfxPasswordCredentialObject

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 46 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change 'cert' to 'certificate' for this and the below 5.

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 60 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to $script:validCertificateThumbprint

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 64 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I doesn't just compiles it applies the configuration as well.

I've changed this (and all the other integration tests) to match the text in the template:
https://github.com/PowerShell/DscResources/blob/master/Tests.Template/integration_template.ps1#L47

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 87 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this one

Done. And in all other integration tests

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 88 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not (upper 'N')

Done. And in all other integration tests

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 91 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should (upper 'S')

Done. And in all other integration tests

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 92 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Throw (upper 'T')

Done. And in all other integration tests

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 109 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I doesn't just compiles it applies the configuration as well.

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 135 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this one.

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 136 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not (Upper 'N')

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 139 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should (upper 'S')

Done.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 144 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should (upper 'S')

Done.

---

Tests/TestHelpers/CommonTestHelper.psm1, line 110 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah, Reviewable caught the comment made from Github. Very nice!

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 255 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$false (lower 'f')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 259 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$false (lower 'f')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 273 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 277 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 291 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 299 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 313 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 321 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$false (lower 'f')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 335 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 343 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 357 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 365 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$false(lower 'f')

Done.

---

Comments from Reviewable

[PlagueHO]

Review status: 28 of 38 files reviewed at latest revision, 57 unresolved discussions.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 96 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should (upper 'S')

Done. And in all other integration tests

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 97 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done. And in all other integration tests

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 26 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Minor: I rather see all these variables with 'cert' written out to 'certificate'. This and the 8 below.

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 36 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $validCertificate

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 71 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $validCertificateMatchSourceParameters

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 76 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $pfxCredential, or $pfxCredentialObject, or even $pfxPasswordCredentialObject (since the parameter is Password and is an credential object 😄 )

Done. Used $pfxCredential

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 79 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $validPfxParameters

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 106 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank row before this one.

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 113 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $importedCertificateMatch

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 114 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $importedCertificateNoMatch

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 128 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank rows between the blocks here to please. Throughout the file :)

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 166 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Needs to be done because Export-Certificate requires a real cert object

Don't follow you here.
Do you mean 'Needs to be done because the original Export-Certificate cmdlet requires a real certificate object, so it is not possible to mock the original cmdlet'?

I have clarified this in the comment. So yes, that is essentially the problem.

But for more detail: I could create a self-signed x509 cert in the Windows certificate store and export it and then use that [x509Certificate2] object to return from the Get-ChildItem mock, but this would be a destructive change to the host system and therefore not acceptable for unit tests (but A-OK for integration tests). So I've opted to do it this way and perform a full functional test in the integration tests.

---

Comments from Reviewable

[PlagueHO]

Review status: 28 of 38 files reviewed at latest revision, 56 unresolved discussions.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 56 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $validCertificateParameters

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 96 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to $validPfxMatchSourceParameters

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 161 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Mock Export-Certificate and Export-PfxCertificate to make sure the don't get called. (will be done if you add them to the BeforeEach-block in previous comment)
Change to
Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 0
Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 0
Assert-MockCalled -CommandName Export-PfxCertificate -Exactly -Times 0

Move Assert-VerifiableMocks to the end of the Describe-block

I've removed all the Assert-MockVerifiable and replaced them with Assert-MockCalled ... I'd rather be consistent and just not call Assert-MockVerifiable at all.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 177 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank row before this row, and between all the other Mocks. (align with guide, and makes it easier for the eyes to review too)

Curious, is this a style of coding you are using? I have seen several contributors putting blocks together. That can't be a coincidence. :) Since my C coding days I always been taught to leave room between each block, so I do it without thinking 😃

Done.

I agree that a blank line after a block is better - but I sometimes forget 😁

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 191 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to `Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 1

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 192 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Must have -Exactly on this one.
https://github.com/pester/Pester/wiki/Assert-MockCalled#exactly

I've removed all the Verifiable Mocks and replaced them with Assert-MockCalled -Exactly...

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 240 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$true (lower 't')

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 255 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Done.

---

Comments from Reviewable

[PlagueHO]

Review status: 28 of 38 files reviewed at latest revision, 56 unresolved discussions.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 188 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You are mocking everything, so is could not possible throw (?). Suggest to change the mock so that if verifies the incoming parameters are assigned the expected parameters (below $mockExpectedFilePath is dynamically set)

i.e (inside the InModuleScope-block)

$mockExportCertificate = {
    if ($FilePath -ne $mockExpectedFilePath)
    {
        throw ' Expected mock to be called with {0}, but was {1}' -f $mockExpectedFilePath,$FilePath
    }
}

Mock the cmdlet

Mock -CommandName Export-Certificate -MockWith $mockExportCertificate -Verifiable

Before calling Set-TargetResource

$mockExpectedFilePath = 'TestDrive:\exportedcertificate.cer'

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 223 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as previous comment regarding mocking

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 225 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as the previous comments regarding the assertions

I've got rid of all the Verifiable Mocks and replaced them with -Exactly...

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 243 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Rather have Assert-MockCalled here, and move this to the end of the Describe-block

Done by removing all the Verifiable mocks.

---

Comments from Reviewable

[PlagueHO]

@johlju - OK! All done now I think (hopefully). Thanks again for the great review and suggestions. I've done all but two that I didn't think should be done.

---

Review status: 28 of 38 files reviewed at latest revision, 56 unresolved discussions.

---

Tests/Unit/CertificateDsc.Common.Tests.ps1, line 190 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Actually, instead of repeating the same mock, you should add the Mock in a BeforeEach-block inside the Describe-block. That will trigger before each It-block.
I don't know if that will break Assert-VerifiableMocks (giving false positive again) but either way I think you should only use Assert-VerifiableMocks one time at the end of the Describe-block and in the It-block use Assert-MockCalled.

Would this work here?

Inside the InModuleScope-block ($mockCertificateStorePath is dynamically assigned during runtime.

$mockGetChildItemParameterFilter = {
    $Path -eq $mockCertificateStorePath
}

Inside BeforeEach-block in the describe-block.

Mock `
    -CommandName Get-ChildItem `
    -MockWith { @( $validCert ) } `
    -ParameterFilter $mockGetChildItemParameterFilter `
    -Verifiable

Before any needed Context-block or/and It-block to be tested. Dynamically change the store path in the mock parameter filter.

$mockCertificateStorePath = 'cert:\LocalMachine\My'

Inside the Assert It-block

Assert-MockCalled `
    -CommandName Get-ChildItem `
    -ParameterFilter $mockGetChildItemParameterFilter `
    -Scope It

At the end of the Describe-block

Assert-VerifiableMocks

Good idea - and done. Except I've removed the Verifiable mocks and opted for -exactly mocks

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 152 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you move these to a BeforeEach-block and make them dynamically instead?

Done.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 234 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You could do these dynamically in a BeforeEach-block as well. Maybe? I'm good with how they are here in the Test Describe-block. If making it dynamically it makes more code. But was hoping it could use same code as in the Set-method testing.

Done.

---

Comments from Reviewable

[johlju]

Just a few comments now. :)

---

Reviewed 2 of 11 files at r3, 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 26 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I'm not sure about this one - I'm wondering if it is better to stay inline with the naming used in the Template because that is the convention used in all other repos.

OK. Ah, I agree, did know it was part of the template. :)

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 34 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I don't have a preference for $env: . But I am fairly obsessive about consistency within a repo - so I've done a global search/replace to set them all to $env: - so this has resulted in additional files being changed 😢 sorry about that.

I have also changed to $script:certificatePath

LGTM.

No worries. I'm happy to review the extra files if it means it will be consistent. :)

---

Tests/Unit/CertificateDsc.Common.Tests.ps1, line 190 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Good idea - and done. Except I've removed the Verifiable mocks and opted for -exactly mocks

I can't see this file was changed. Did the file get lost in the push somehow? :)

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 161 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I've removed all the Assert-MockVerifiable and replaced them with Assert-MockCalled ... I'd rather be consistent and just not call Assert-MockVerifiable at all.

LGTM.

Adding Assert-MockVerifiable to the bottom could be a way to catch mocks that has not been called (as long as the have -Verifiablethough), either by bug in test or just missed remove the mock when logic changed.
But leave as-is is good for me.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 26 at r5 (raw file):

        $DSCResourceName = 'MSFT_xCertificateExport'

        $certificatePath = Join-Path -Path $ENV:Temp -ChildPath 'xCertificateExportTestCert.cer'

$env:

---

Comments from Reviewable

[PlagueHO]

Ok! I think everything is done now! 😁

---

Review status: 36 of 38 files reviewed at latest revision, 3 unresolved discussions.

---

Tests/Unit/CertificateDsc.Common.Tests.ps1, line 190 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I can't see this file was changed. Did the file get lost in the push somehow? :)

Doh! Didn't press save! Saved now. Should be fully dynamic mocks now.

---

Tests/Unit/MSFT_xCertificateExport.Tests.ps1, line 26 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$env:

Done.

---

Comments from Reviewable

[johlju]

Just a tiny fix, then this is all good to me. 😄

---

Reviewed 2 of 11 files at r3, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 38 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Any variable that needs to be accessible in an AfterAll or BeforeAll pester block needs to be in the script scope. I don't want to put every variable as $script scope as that strikes me as bad practice (akin to using global scope when not necessary). So I only put the needed variables in there.

I think this rule is not clear because I don't think the intent is for every variable to be scoped to $Env, $Script or $Global. I think it just means that if a variable is scoped (to anything but $Local) then you must specify the scope everywhere it is used. @kwirkykat - would you like to confirm this?

Maybe you could move this question to DscResources instead? So you can merge this.

---

Tests/Unit/CertificateDsc.Common.Tests.ps1, line 197 at r6 (raw file):

                        return @( $validCert )
                    }
                    'cert:\LocalMachine\NoCert'

Could you add a blank row between each switch-value-block?

---

Comments from Reviewable

[PlagueHO]

Ok! Hopefully all ready to go now 😁

---

Review status: 37 of 38 files reviewed at latest revision, 2 unresolved discussions.

---

Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 38 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe you could move this question to DscResources instead? So you can merge this.

Done! Raised it here:
PowerShell/DscResources#250

---

Tests/Unit/CertificateDsc.Common.Tests.ps1, line 197 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank row between each switch-value-block?

Done.

---

Comments from Reviewable

[johlju]

@PlagueHO Reviewable seems down for the moment, so can't review the last bits :/ I will do that as soon as the site is working again.

[johlju]

---

Reviewed 1 of 11 files at r3, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[PlagueHO]

Thank you so much @johlju

---

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

---

Comments from Reviewable