Adding Additional unit test template

[mbreakey3]

Opening this PR so that we can get feedback on our unit test templates. What do you think about having 2-3 templates available? Are there any other major/minor changes that you think they need?

---

This change is

[kwirkykat]

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

---

Tests.Template/unit_template1.ps1, line 2 [r1] (raw file):

<#
        .Synopsis

SYNOPSIS

---

Tests.Template/unit_template1.ps1, line 3 [r1] (raw file):

<#
        .Synopsis
        A sample template for creating DSC Resource Unit Tests mainly for smaller resources

Indent text below the help keywords

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

# TODO: Customize these parameters...
$script:DSCModuleName      = '<ModuleName>' # Example xNetworking
$script:DSCResourceName    = '<ResourceName>' # Example MSFT_xFirewall

These should be camel case ($script:dscModuleName, $script:dscResourceName)

---

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

#endregion HEADER

# TODO: Other Optional Init Code Goes Here...

We should use Pester's BeforeAll instead of putting init code here.
Norberto recently opened an issue for this in DscResources I believe.
That means a Describe block encapsulating all the tests would be needed.

Which means the template would look more like this:
Describe "$script:dscResourceName Unit Tests" {
BeforeAll {
resource-specific init code here
}

AfterAll {
    resource-specific cleanup code here
}

Context Get-TargetResource {
    ...
}
Context Set-TargetResource {
    ...
}
Context Test-TargetResource {
    ...
}

}

Though Pester may start complaining about script variables used in the Describe block that are declared outside the Describe block then.

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

$script:DSCModuleName = '' # Example xNetworking
$script:DSCResourceName = '' # Example MSFT_xFirewall

What are those 2 variables used for? They are nor referenced by the template.

---

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):

-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests'))) -or `
(-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1'

Do we need the 2 checks here? Isn't the check for TestHelper.psm1 enough?

---

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

We should use Pester's BeforeAll instead of putting init code here.
Norberto recently opened an issue for this in DscResources I believe.
That means a Describe block encapsulating all the tests would be needed.

Which means the template would look more like this:
Describe "$script:dscResourceName Unit Tests" {
BeforeAll {
resource-specific init code here
}

AfterAll {
    resource-specific cleanup code here
}

Context Get-TargetResource {
    ...
}
Context Set-TargetResource {
    ...
}
Context Test-TargetResource {
    ...
}

}

Though Pester may start complaining about script variables used in the Describe block that are declared outside the Describe block then.

I think we will have to hold on on my suggestion of using Before/AfterAll. I wouldn't like to encourage people to use Context for the function name; Context should be used more to describe pre-conditions (scenario) for the tests.

I confirmed with the Pester team that support for nested Describes will come in 4.0. Then we can have a main Describe with Before/AfterAll for the entire test suite,

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 8 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

#region Pester Test Initialization

 
# TODO: Optionally create any variables here for use by your tests
# See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1
 
#endregion Pester Test Initializati

We should probably wrap this in a function (BeforeAllTests?), which would make it easier to move this to a BeforeAll block when support for nested Describes are available.

Similar comment for the code in finally.

---

Tests.Template/unit_template1.ps1, line 82 [r1] (raw file):

    # TODO: Pester Tests for any non-exported Helper Cmdlets
    # If the resource does not contain any non-exported helper cmdlets then
    # this block may be safetly deleted.

"safely"

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):

    #region Example state 1
    Describe 'The system is not in the desired state' {

Context is more appropriate for this.

Describe 'Resource' {
    Context 'When the system is not in the desired state' {
        #TODO: Mock cmdlets here that represent the system not being in the desired state
 
        #TODO: Create a set of parameters to test your get/test/set methods in this state
        $testParameters = @{
            Property1 = 'value'
            Property2 = 'value'
        }
 
        #TODO: Update the assertions below to align with the expected results of this state
        It 'Get method returns something' {
            Get-TargetResource @testParameters | Should Be 'something'
        }
 
        It 'Test method returns false' {
            Test-TargetResource @testParameters | Should be $false
        }
 
        It 'Set method calls Demo-CmdletName' {
            Set-TargetResource @testParameters
 
            #TODO: Assert that the appropriate cmdlets were called
            Assert-MockCalled Demo-CmdletName
        }
    }
    Context 'When the system is in the desired state' {
        ... etc ...
    }
}

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):

It 'Get method returns something'

oh! and It should use 'Should'

It 'The Get method should return something'

---

Comments from Reviewable

[johlju]

What are the differences between the templates in that case? What is a small resource?
My thought is that it would be better to use the same template as the base for all resource.

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

It 'Get method returns something'

oh! and It should use 'Should'

It 'The Get method should return something'

I suggest we use `Describe "$($script:DSCResourceName)` or `Describe "$($script:DSCResourceName)\Get-TargetResource"`

Should we use BeforeAll like this?

    Context 'When the system is not in the desired state' {
        BeforeAll {
            #TODO: Mock cmdlets here that represent the system not being in the desired state
         }

It-blocks that test the Get-method could use It 'Should...' like this

            $result =  Get-TargetResource @testParameters

            It 'Should return the desired state as absent' {
                  $result.Ensure | Should Be 'Absent'
            }

            It 'Should return the same values as passed as parameters' {
                $result.MyProperty1 | Should Be 'something'
                $result.MyProperty2 | Should Be 'another thing'
            }

---

Comments from Reviewable

[narrieta]

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

---

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):
Agree on the Describe label. 'Resource' was just a dummy example.

Agree on use of BeforeAll wrapping this code:

    #TODO: Mock cmdlets here that represent the system not being in the desired state

 

Quoted 5 lines of code…

    #TODO: Create a set of parameters to test your get/test/set methods in this state
    $testParameters = @{
        Property1 = 'value'
        Property2 = 'value'
    }</details>

---

Comments from Reviewable

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

$script:DSCModuleName = '' # Example xNetworking
$script:DSCResourceName = '' # Example MSFT_xFirewall

What are those 2 variables used for? They are nor referenced by the template.

There used to set up the Test environment, and for the titles of the Describe blocks mainly.

---

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests'))) -or `
(-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1'

Do we need the 2 checks here? Isn't the check for TestHelper.psm1 enough?

If the DSCResource.Tests folder doesn't exist than it would throw an error if we only do the one check. This code will be replaced in the near future anyway

---

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

I think we will have to hold on on my suggestion of using Before/AfterAll. I wouldn't like to encourage people to use Context for the function name; Context should be used more to describe pre-conditions (scenario) for the tests.

I confirmed with the Pester team that support for nested Describes will come in 4.0. Then we can have a main Describe with Before/AfterAll for the entire test suite,

Okay, so should we leave it as is for now?

---

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

#region Pester Test Initialization

 
# TODO: Optionally create any variables here for use by your tests
# See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1
 
#endregion Pester Test Initializati

We should probably wrap this in a function (BeforeAllTests?), which would make it easier to move this to a BeforeAll block when support for nested Describes are available.

Similar comment for the code in finally.

Done.

---

Tests.Template/unit_template1.ps1, line 82 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

"safely"

Done.

---

Comments from Reviewable

[mbreakey3]

@johlju I agree that it would be easier to use the same template, however there are different styles of writing unit tests and some are much better than others depending on the nature of the functions in the resource

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

There used to set up the Test environment, and for the titles of the Describe blocks mainly.

I'd suggest removing them to simplify the template and also because there is a $script:DSCResourceName and a $global:DSCResourceName .

Also, I think Initialize-TestEnvironment can infer these values from the filename.

---

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

If the DSCResource.Tests folder doesn't exist than it would throw an error if we only do the one check. This code will be replaced in the near future anyway

Who throws the error? Both Test-Path and Join-Path seem to work fine if the paths do not exist

PS> Test-Path c:\foo\bar
False
PS> Join-Path c:\foo\ bar
c:\foo\bar

The intention here being simplifying the template. Having a single check improves readability.

---

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Okay, so should we leave it as is for now?

Yes, just with the change below to wrap this is a function.

---

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Done.

Thanks

---

Tests.Template/unit_template1.ps1, line 82 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Done.

Thanks

---

Comments from Reviewable

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 2 [r1] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

SYNOPSIS

Done.

---

Tests.Template/unit_template1.ps1, line 3 [r1] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

Indent text below the help keywords

Done.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

These should be camel case ($script:dscModuleName, $script:dscResourceName)

Done.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

I'd suggest removing them to simplify the template and also because there is a $script:DSCResourceName and a $global:DSCResourceName .

Also, I think Initialize-TestEnvironment can infer these values from the filename.

Okay, so what are you suggesting we replace them with since $script:dscResourceName is being used throughout the file

---

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

Who throws the error? Both Test-Path and Join-Path seem to work fine if the paths do not exist

PS> Test-Path c:\foo\bar
False
PS> Join-Path c:\foo\ bar
c:\foo\bar

The intention here being simplifying the template. Having a single check improves readability.

Okay, I can remove it

---

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

Yes, just with the change below to wrap this is a function.

Done.

---

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Done.

Done.

---

Tests.Template/unit_template2.ps1, line 66 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

Agree on the Describe label. 'Resource' was just a dummy example.

Agree on use of BeforeAll wrapping this code:

    #TODO: Mock cmdlets here that represent the system not being in the desired state

 
#TODO: Create a set of parameters to test your get/test/set methods in this state
$testParameters = @{
Property1 = 'value'
Property2 = 'value'
}

Done.

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Okay, so what are you suggesting we replace them with since $script:dscResourceName is being used throughout the file

I'd suggest removing them altogether

# TODO: Customize these parameters...

$TestEnvironment = Initialize-TestEnvironment `
     -DSCModuleName  '<ModuleName>'`
    -DSCResourceName '<ResourceName>' `
    -TestType Unit

$script:dscResourceName is not being used throughout the file. $global:dscResourceName is. They are different variables.

Also, our convention for naming the test file is .Tests.ps1, correct? (or is it ..Tests.ps1?) No need to have the test author specify those. Initialize-TestEnvironment can grab them (or at least one of them, depending on our convention) from the filename,

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 39 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Done.

thanks

---

Comments from Reviewable

[johlju]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 100 [r3] (raw file):

        #region Example state 2
        Context 'When the system is in the desired state' {
            #TODO: Mock cmdlets here that represent the system being in the desired state

Shoudl use BeforeAll{} here to like above to be consistent.

---

Comments from Reviewable

[johlju]

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

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

try
{
    Initialize-PesterTests

It's not obvious that this function should be used to add init code. When your read it it feels like a function for the initialization of the pester tests for the template. Unless you read to the bottom.
A suggestion is that we add a comment like # In this function, at the end of the script, you can add your own optional init code

---

Comments from Reviewable

[narrieta]

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

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It's not obvious that this function should be used to add init code. When your read it it feels like a function for the initialization of the pester tests for the template. Unless you read to the bottom.
A suggestion is that we add a comment like # In this function, at the end of the script, you can add your own optional init code

Agree.

BeforeAllTests?

---

Comments from Reviewable

[KarolKaczmarek]

Review status: 0 of 2 files reviewed at latest revision, 17 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

I'd suggest removing them altogether

# TODO: Customize these parameters...

$TestEnvironment = Initialize-TestEnvironment `
     -DSCModuleName  '<ModuleName>'`
    -DSCResourceName '<ResourceName>' `
    -TestType Unit

$script:dscResourceName is not being used throughout the file. $global:dscResourceName is. They are different variables.

Also, our convention for naming the test file is .Tests.ps1, correct? (or is it ..Tests.ps1?) No need to have the test author specify those. Initialize-TestEnvironment can grab them (or at least one of them, depending on our convention) from the filename,

I see $script:dscResourceName being used in multiple places in the template. It makes sense to have variable for that and initialize it at the beginning, so I suggest leaving it as it is.

---

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

TODO: Optionally create any variables here for use by your tests

# See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1

These two lines should be removed since we call function to initialize tests

---

Tests.Template/unit_template1.ps1, line 19 [r3] (raw file):

$script:dscModuleName      = '<ModuleName>' # Example xNetworking
$script:dscResourceName    = '<ResourceName>' # Example MSFT_xFirewall
# /TODO

let's remove these # /TODO closing tags.. they don't add any value (it's quite clear what TODO refers to) and it's just one more thing to remove before committing

---

Tests.Template/unit_template1.ps1, line 27 [r3] (raw file):

if (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1')))
{
    & git @('clone','https://github.com/PowerShell/DscResource.Tests.git',(Join-Path -Path $script:moduleRoot -ChildPath '\DSCResource.Tests\'))

We should not use git in our test templates: #107

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

Agree.

BeforeAllTests?

Invoke-TestSetup? or Initialize-TestSetup (to be consistent with Initialize-TestEnvironment)

---

Tests.Template/unit_template1.ps1, line 101 [r3] (raw file):

}

function Complete-PesterTests {

We should rename it...
my proposition would be Invoke-TestCleanup, but that'll depend what we rename Initialize-PesterTests to

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

<#
    .SYNOPSIS
       A sample template for creating DSC Resource Unit Tests mainly for larger resources

I'm not sure what does "larger resources" mean here...
This seems as if it's just a slightly more advanced version of the unit_template1 and we could easily add the simplified approach from that first template here in just another region (#Region example 3)... perhaps better than maintaining two templates which would only cause confusion?

---

Tests.Template/unit_template2.ps1, line 19 [r3] (raw file):

$script:dscModuleName      = '<ModuleName>' # Example xNetworking
$script:dscResourceName    = '<ResourceName>' # Example MSFT_xFirewall
# /TODO

Please remove /TODO

---

Tests.Template/unit_template2.ps1, line 27 [r3] (raw file):

if (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1')))
{
    & git @('clone','https://github.com/PowerShell/DscResource.Tests.git',(Join-Path -Path $script:moduleRoot -ChildPath '\DSCResource.Tests\'))

We should not use git #107

---

Tests.Template/unit_template2.ps1, line 46 [r3] (raw file):

    Initialize-PesterTests

    #region Pester Test Initialization

This region can be removed after adding Initialize-PesterTests

---

Comments from Reviewable

[KarolKaczmarek]

I'm not sure we really need two unit test templates...

unit_template2 is a slightly more advanced version of unit_template1, but 80% of it is the same, so perhaps we could do with just this one? We could add additional region which would contain 'describe' structure from unit_template1 as an alternative approach and would be commented out by default.

Single template would be easier to maintain and cause less confusion. Currently it's not even clear which template should be used, as the only distinction is "large" vs "not large" module which can be very subjective.

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 17 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 25 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Okay, I can remove it

Done.

---

Tests.Template/unit_template1.ps1, line 47 [r1] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

TODO: Optionally create any variables here for use by your tests

# See https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/MSFT_xDhcpClient.Tests.ps1

These two lines should be removed since we call function to initialize tests

Done.

---

Tests.Template/unit_template1.ps1, line 19 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

let's remove these # /TODO closing tags.. they don't add any value (it's quite clear what TODO refers to) and it's just one more thing to remove before committing

Done.

---

Tests.Template/unit_template2.ps1, line 46 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

This region can be removed after adding Initialize-PesterTests

Done.

---

Tests.Template/unit_template2.ps1, line 100 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Sjoudl use BeforeAll{] here to like above to be consistent.

Done.

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

I see $script:dscResourceName being used in multiple places in the template. It makes sense to have variable for that and initialize it at the beginning, so I suggest leaving it as it is.

$global:dscResourceName is being used, not $script:dscResourceName

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

Invoke-TestSetup? or Initialize-TestSetup (to be consistent with Initialize-TestEnvironment)

We are thinking about replacing this with BeforeAll once Pester supports nested Describes (V 4.0).

Not a big issue, though

---

Comments from Reviewable

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

$global:dscResourceName is being used, not $script:dscResourceName

That should have been taken out... do you have the most updated version?

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

We are thinking about replacing this with BeforeAll once Pester supports nested Describes (V 4.0).

Not a big issue, though

Doesn't the function still need to follow the verb-noun format though?

---

Tests.Template/unit_template2.ps1, line 19 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

Please remove /TODO

Done.

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Doesn't the function still need to follow the verb-noun format though?

I'd follow Pester's conventions

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

That should have been taken out... do you have the most updated version?

OK, I see the change.

My next question :) is now: why do we need a global variable? both the resource and module names are part of the test environment returned by Initialize-TestEnvironment. Why not fetch them from there?

---

Comments from Reviewable

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

OK, I see the change.

My next question :) is now: why do we need a global variable? both the resource and module names are part of the test environment returned by Initialize-TestEnvironment. Why not fetch them from there?

There shouldn't be any globals in the unit tests. just script variables

---

Comments from Reviewable

[narrieta]

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 18 [r1] (raw file):

Previously, mbreakey3 (Mariah) wrote…

There shouldn't be any globals in the unit tests. just script variables

My bad, but same question: why do we even need a script variable? Thanks

---

Comments from Reviewable

[KarolKaczmarek]

Review status: 0 of 2 files reviewed at latest revision, 12 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

We are thinking about replacing this with BeforeAll once Pester supports nested Describes (V 4.0).

Not a big issue, though

Definitely, we should use BeforeAll once it's available, but for now I suggest renaming Initialize-PesterTests and Complete-PesterTests to Invoke-TestSetup and Invoke-TestCleanup respectively

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

I'm not sure what does "larger resources" mean here...
This seems as if it's just a slightly more advanced version of the unit_template1 and we could easily add the simplified approach from that first template here in just another region (#Region example 3)... perhaps better than maintaining two templates which would only cause confusion?

Saw @johlju raised the same concern before. Fully agree.

---

Comments from Reviewable

[nanalakshmanan]

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

Saw @johlju raised the same concern before. Fully agree.

I agree with the comments that @KarolKaczmarek has specified. What is the basis for determining what is a "large resource"?

---

Tests.Template/unit_template2.ps1, line 43 [r4] (raw file):

{

    Initialize-PesterTests

Noun in PowerShell are singular. This should be Initialize-PesterTest

---

Comments from Reviewable

[johlju]

Reviewed 1 of 3 files at r1, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

Definitely, we should use BeforeAll once it's available, but for now I suggest renaming Initialize-PesterTests and Complete-PesterTests to Invoke-TestSetup and Invoke-TestCleanup respectively

I agree with the rename. Let's follow the approved verb-noun rules.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, nanalakshmanan (Nana Lakshmanan) wrote…

I agree with the comments that @KarolKaczmarek has specified. What is the basis for determining what is a "large resource"?

If you want input from a newbie of writing Pester tests (me) I vote for one template that are informative than several and I don't know which to choice. Then there won't be a concern If I choose wrong template for the resource I'm writing. In that sense it's better to remove stuff from a template than add.

---

Tests.Template/unit_template2.ps1, line 60 [r4] (raw file):

    #region Example state 1
    Describe "$($script:dscResourceName)" {

In template number one, the describe block has "$($script:dscResourceName)\Get-TargetResource". But not template number 2.
Do we want to have it grouped differently? I would say not. I know we are discussing one or more templates. But if we are gonna go with more than one template I think that the output of the tests, regardless of template, should be the same to easily read the output after all the test have ran. It's like we have a style guide for code. DOn't we need a style guide for tests output too?

---

Comments from Reviewable

[mbreakey3]

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

---

Tests.Template/unit_template1.ps1, line 43 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I agree with the rename. Let's follow the approved verb-noun rules.

Done.

---

Tests.Template/unit_template2.ps1, line 43 [r4] (raw file):

Previously, nanalakshmanan (Nana Lakshmanan) wrote…

Noun in PowerShell are singular. This should be Initialize-PesterTest

Done.

---

Comments from Reviewable

[mbreakey3]

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If you want input from a newbie of writing Pester tests (me) I vote for one template that are informative than several and I don't know which to choice. Then there won't be a concern If I choose wrong template for the resource I'm writing.
In that sense it's better to remove stuff from a template than add.

I'm going to leave the SYNOPSIS comment as-is until we make a decision on how we want to format this. @johlju thanks for your feedback! Should we come up with some hybrid of these two templates then? Because I see pros and cons in each and the 'better' one I think depends on the format/size/number of functions in the resource.

---

Comments from Reviewable

[KarolKaczmarek]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

I'm going to leave the SYNOPSIS comment as-is until we make a decision on how we want to format this. @johlju thanks for your feedback! Should we come up with some hybrid of these two templates then? Because I see pros and cons in each and the 'better' one I think depends on the format/size/number of functions in the resource.

I suggest having the first, simpler template as the main (and only) one and include alternative structure of "describe" from the second, more advanced template as a comment with some explanation when we could use that instead. This ways out-of-the-box test template is as simple as possible for first-time users but provides more advanced structure for people who already have some experience with developing DSC resources and writing tests for those. Alternatively, we could adopt more advanced template alltogether - it covers superset of scenarios.

---

Comments from Reviewable

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

I suggest having the first, simpler template as the main (and only) one and include alternative structure of "describe" from the second, more advanced template as a comment with some explanation when we could use that instead. This ways out-of-the-box test template is as simple as possible for first-time users but provides more advanced structure for people who already have some experience with developing DSC resources and writing tests for those. Alternatively, we could adopt more advanced template alltogether - it covers superset of scenarios.

@KarolKaczmarek I agree. That sounds like the best solution. Especially since the templates are more for less experienced contributors

---

Comments from Reviewable

[KarolKaczmarek]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

@KarolKaczmarek I agree. That sounds like the best solution. Especially since the templates are more for less experienced contributors

Any progress on this?

---

Comments from Reviewable

[mbreakey3]

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

Any progress on this?

I was working on releasing WebAdminDsc, so I haven't had time to update this yet. We're also now discussing the issue of using [InModuleScope](https://github.com//issues/175). So I think we should incorporate a solution for that in our template as well

---

Comments from Reviewable

[KarolKaczmarek]

Review status: 0 of 2 files reviewed at latest revision, 14 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

I was working on releasing WebAdminDsc, so I haven't had time to update this yet. We're also now discussing the issue of using InModuleScope. So I think we should incorporate a solution for that in our template as well

I'd suggest getting rid of second template as this seems the last open issue here and getting this one merged in, so we have a base to work with.

The InModuleScope and not using git can be added in subsequent PRs, cause they require separate discussion and thorough review.

---

Comments from Reviewable

[mbreakey3]

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

I'd suggest getting rid of second template as this seems the last open issue here and getting this one merged in, so we have a base to work with.

The InModuleScope and not using git can be added in subsequent PRs, cause they require separate discussion and thorough review.

Okay, so get rid of unit_template2.ps1?

---

Comments from Reviewable

[KarolKaczmarek]

Review status: 0 of 2 files reviewed at latest revision, 15 unresolved discussions.

---

Tests.Template/unit_template.ps1, line 18 [r1] (raw file):

Previously, narrieta (Norberto Arrieta) wrote…

My bad, but same question: why do we even need a script variable? Thanks

They are used e.g. in "Describe" sections

---

Tests.Template/unit_template.ps1, line 51 [r5] (raw file):

    #region Function Get-TargetResource

The second template is now removed but it can't be found anywhere now. It's still valuable in certain scenarios, I'd suggest adding it to this template as a optional (commented out) section with information when it should be used instead.
Just make sure it's clear that part should be removed if we go with the simpler (default) version.

---

Comments from Reviewable

[mbreakey3]

@kwirkykat - can you take a look at the most recent changes I made?

[kwirkykat]

I like this setup with the patterns in test guidelines much better 👍

---

Reviewed 1 of 3 files at r1, 2 of 3 files at r5, 1 of 2 files at r6.
Review status: 1 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

TestsGuidelines.md, line 93 [r6] (raw file):

-------------

```powershell

All these region definitions are not needed.

Can this be more in the style of documentation rather than code comments?
Sorry that's kinda vague.
I mean more like this:

Pattern 1:
The goal of this pattern should be to describe the potential states a system could be in for each function.
 

Describe 'Get-TargetResource' {
        # TODO: Complete Get-TargetResource Tests...
}
Describe 'Set-TargetResource' {
        # TODO: Complete Set-TargetResource Tests...
}
Describe 'Test-TargetResource' {
        # TODO: Complete Test-TargetResource Tests...
}

Pattern 2:
The goal of this pattern should be to describe the potential states a system could be in so that the get/set/test cmdlets can be tested in those states. Any mocks that relate to that specific state can be included in the relevant Describe block. For a more detailed description of this approach please review #143
 
Add as many of these example 'states' as required to simulate the scenarions that the DSC resource is designed to work with, below a simple 'is in desired state' and 'is not in desired state' are used, but there may be more complex combinations of factors, depending on how complex your resource is.
 

Describe 'The system is not in the desired state' {
    #TODO: Mock cmdlets here that represent the system not being in the desired state
 
    #TODO: Create a set of parameters to test your get/set/test methods in this state
    $testParameters = @{
        Property1 = 'value'
        Property2 = 'value'
    }
 
    #TODO: Update the assertions below to align with the expected results of this state
    It 'Should return something' {
        Get-TargetResource @testParameters | Should Be 'something'
    }
 
    It 'Should return false' {
        Test-TargetResource @testParameters | Should be $false
    }
 
    It 'Should call Demo-CmdletName' {
        Set-TargetResource @testParameters

        #TODO: Assert that the appropriate cmdlets were called
        Assert-MockCalled Demo-CmdletName
    }
}

or
 ```powershell
Describe 'The system is in the desired state' {
        #TODO: Mock cmdlets here that represent the system being in the desired state
 
        #TODO: Create a set of parameters to test your get/set/test methods in this state
        $testParameters = @{
            Property1 = 'value'
            Property2 = 'value'
        }
 
        #TODO: Update the assertions below to align with the expected results of this state
        It 'Should return something' {
            Get-TargetResource @testParameters | Should Be 'something'
        }
 
        It 'Should return true' {
            Test-TargetResource @testParameters | Should be $true
        }
}

---

Tests.Template/unit_template.ps1, line 51 [r5] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

The second template is now removed but it can't be found anywhere now. It's still valuable in certain scenarios, I'd suggest adding it to this template as a optional (commented out) section with information when it should be used instead.
Just make sure it's clear that part should be removed if we go with the simpler (default) version.

It's been moved to the test guidelines because we don't want to clutter this file with large blocks of commented-out code.

---

Tests.Template/unit_template.ps1, line 13 [r6] (raw file):

        There are multiple methods for writing unit tests. This template provides a few examples
        which you are welcome to follow but depending on your resource, you may want to
        design it differently.

Can we add a link to the test guidelines in the NOTES section here?

---

Comments from Reviewable

[mbreakey3]

Review status: 1 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

Tests.Template/unit_template2.ps1, line 3 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Okay, so get rid of unit_template2.ps1?

Done.

---

Comments from Reviewable

[mbreakey3]

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

TestsGuidelines.md, line 93 [r6] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

All these region definitions are not needed.

Can this be more in the style of documentation rather than code comments?
Sorry that's kinda vague.
I mean more like this:

Pattern 1:
The goal of this pattern should be to describe the potential states a system could be in for each function.
 

Describe 'Get-TargetResource' {
        # TODO: Complete Get-TargetResource Tests...
}
Describe 'Set-TargetResource' {
        # TODO: Complete Set-TargetResource Tests...
}
Describe 'Test-TargetResource' {
        # TODO: Complete Test-TargetResource Tests...
}

Pattern 2:
The goal of this pattern should be to describe the potential states a system could be in so that the get/set/test cmdlets can be tested in those states. Any mocks that relate to that specific state can be included in the relevant Describe block. For a more detailed description of this approach please review #143
 
Add as many of these example 'states' as required to simulate the scenarions that the DSC resource is designed to work with, below a simple 'is in desired state' and 'is not in desired state' are used, but there may be more complex combinations of factors, depending on how complex your resource is.
 

Describe 'The system is not in the desired state' {
    #TODO: Mock cmdlets here that represent the system not being in the desired state
 
    #TODO: Create a set of parameters to test your get/set/test methods in this state
    $testParameters = @{
        Property1 = 'value'
        Property2 = 'value'
    }
 
    #TODO: Update the assertions below to align with the expected results of this state
    It 'Should return something' {
        Get-TargetResource @testParameters | Should Be 'something'
    }
 
    It 'Should return false' {
        Test-TargetResource @testParameters | Should be $false
    }
 
    It 'Should call Demo-CmdletName' {
        Set-TargetResource @testParameters

        #TODO: Assert that the appropriate cmdlets were called
        Assert-MockCalled Demo-CmdletName
    }
}

or
 ```powershell
Describe 'The system is in the desired state' {
        #TODO: Mock cmdlets here that represent the system being in the desired state
 
        #TODO: Create a set of parameters to test your get/set/test methods in this state
        $testParameters = @{
            Property1 = 'value'
            Property2 = 'value'
        }
 
        #TODO: Update the assertions below to align with the expected results of this state
        It 'Should return something' {
            Get-TargetResource @testParameters | Should Be 'something'
        }
 
        It 'Should return true' {
            Test-TargetResource @testParameters | Should be $true
        }
}

Done.

---

Tests.Template/unit_template.ps1, line 13 [r6] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

Can we add a link to the test guidelines in the NOTES section here?

Done.

---

Comments from Reviewable

[kwirkykat]

---

Reviewed 1 of 3 files at r5, 1 of 2 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

Tests.Template/unit_template.ps1, line 27 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

We should not use git in our test templates: #107

This can be fixed at a later date along with #107. It's not a priority at the moment.

---

Tests.Template/unit_template2.ps1, line 27 [r3] (raw file):

Previously, KarolKaczmarek (Karol Kaczmarek) wrote…

We should not use git #107

This can be fixed at a later date along with #107. It's not a priority at the moment.

---

Tests.Template/unit_template2.ps1, line 60 [r4] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

In template number one, the describe block has "$($script:dscResourceName)\Get-TargetResource". But not template number 2.
Do we want to have it grouped differently? I would say not. I know we are discussing one or more templates. But if we are gonna go with more than one template I think that the output of the tests, regardless of template, should be the same to easily read the output after all the test have ran. It's like we have a style guide for code. DOn't we need a style guide for tests output too?

Ideally, yes. But there's currently a debate about whether the tests should be written by function or by machine state (that's why we are providing both patterns right now). _Technically_ unit tests should be by function, but that can be very inconvenient for some of our tests where we repeat a mocked machine state for every function. I would prefer not making contributors rewrite tests to the other format right now if we're on the fence about which one we should use,but I don't see a common format that the Describe blocks can use if we allow both patterns, so for now we will just have to live with the inconsistent Describe block names across different tests.

We're going to have to come back to these templates when Pester has released nested Describe blocks so we can also remove the InModuleScope. I think when that happens we can re-visit what to do about this issue.

The Context blocks should always describe the 'context'/environment/specific state they set up.
The It blocks at least should always have a common format. They should all be named 'Should...' and describe the functionality that the It is testing such as 'Should return false when package is not installed' under a 'Test-TargetResource' Describe block.

---

Comments from Reviewable