Additional tests, and some fixes, related to AlwaysOn

[johlju]

• Added tests for resources
  • xSQLServerPermission
  • xSQLServerEndpointState
  • xSQLServerEndpointPermission
  • xSQLServerAvailabilityGroupListener
• Fixes in xSQLServerAvailabilityGroupListener
  • In one case the Get-method did not report that DHCP was configured.
  • Now the resource will throw 'Not supported' when IP is changed between Static and DHCP.
  • Fixed an issue where sometimes the listener wasn't removed.

---

This change is

[msftclas]

Hi @johlju, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[narrieta]

What is the test that fails without a fake function?

[johlju]

See the block Describe "$($script:DSCResourceName)\Set-TargetResource" in the file xSQLServerEndpointState.Tests.ps1

[johlju]

If I remove this block

        Get-Module -Name MockSQLPS | Remove-Module -Force
        New-Module -Name MockSQLPS -ScriptBlock {
            function Set-SqlHADREndpoint { return }

            Export-ModuleMember -Function Set-SqlHADREndpoint
        } | Import-Module -Force

I get this error

Describing xSQLServerEndPointState\Set-TargetResource
 [-] Error occurred in Describe block 365ms
   CommandNotFoundException: Could not find Command Set-SqlHADREndpoint
   at Validate-Command, C:\Program Files\WindowsPowerShell\Modules\Pester\3.4.0\Functions\Mock.ps1: line 801
   at Mock, C:\Program Files\WindowsPowerShell\Modules\Pester\3.4.0\Functions\Mock.ps1: line 168
   at <ScriptBlock>, V:\Source\xSqlServer\Tests\Unit\xSQLServerEndpointState.Tests.ps1: line 262

[kwirkykat]

@johlju @aultt is having issues with these same tests in his PR #89 . Have you seen these test issues before? Will this PR fix those issues?

[narrieta]

@johlju OK, so I followed Pester's code and you are correct, the command cannot be resolved (since the SqlServer module is not installed when the UTs run). Your solution will make things work, but I think there should be a comment explaining why the mock module is there.

Some suggestions you may want to consider:

• I would define a psm1 file for the fake SqlServer instead of defining it within the tests. This would allow sharing the fake module across multiple tests.

• I would throw from the fake cmdlets instead of making them no-ops. Those cmdlets should never be reached by any tests so having an error would be good.

  function Set-SqlHADREndpoint { throw 'NotImplemented' }

[BrianFarnhill]

@narrieta That's actually genius - we do the PSM1 file for SharePointDsc but we don't throw inside the calls, we just use them to create our mocks from. I might need to alter my script which generates those stubs to add the exception call in.

[johlju]

@kwirkykat No, those issues in the test run for PR #89 got to do with an issue with the testing framework, see @mbreakey3 and our comments about the problem in issue #92.

Maybe when this PR is merged, and then @aultt resolve conflicts for #89 it might auto heal?

[johlju]

@narrieta I will change so that it used a stub module instead, and that the functions in the stub throws, as per your suggestion.

[aultt]

@johlju looks like you have a pending review can you acknowledge so this can be merged? Waiting on your changes so my PR can go.

[johlju]

@aultt I'm working on the code change which is 90% done, I will try to commit it to the PR tomorrow night (Sweden time). Otherwise on Friday evening. (I'm currently away in another city so have limited time to code :/ )

[aultt]

@johlju sounds good Ill take a look next week.

[johlju]

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

---

Tests/Unit/Stubs/SQLServerStub.psm1, line 0 [r3] (raw file):
This stub file is not actual used by any test yet, but I figured I add this because of issue #91 will make sure that SQLServer module will used before SQLPS, if it exist. So we should actually run tests with both SQLPS and SQLServer stub module.
As of now, these tests I added only run the test using SQLPS stub module.

---

Comments from Reviewable

[mbreakey3]

# Suppressing this rule because these functions are from an external module 
# and are only being used as stubs
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingUserNameAndPassWordParams', '')]
param()

[mbreakey3]

Reviewed 1 of 8 files at r1.
Review status: 1 of 10 files reviewed at latest revision, 12 unresolved discussions.

---

README.md, line 345 [r3] (raw file):

    - modified functions
        - New-TerminatingError - *added optional parameter `InnerException` to be able to give the user more information in the returned message*
* Added tests for resources

Move these comments under 'Unreleased' - I think it got messed up when we released on Wed.

---

DSCResources/xSQLServerAvailabilityGroupListener/xSQLServerAvailabilityGroupListener.psm1, line 288 [r3] (raw file):

        if( -not $($PSBoundParameters.ContainsKey('Ensure')) -or $Ensure -eq "Present" ) { 
            if( ($Port -eq "" -or $listenerState.Port -eq $Port) -and $ipAddressEqual -and ( -not $($PSBoundParameters.ContainsKey('DHCP')) -or $listenerState.DHCP -eq $DHCP ) ) {

can you divide up this line so that each 'and' is on its own line?

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 50 [r3] (raw file):

    Describe "$($script:DSCResourceName)\Get-TargetResource" {
        Context 'When the system is not in the desired state' {
            BeforeAll {

You can remove this block if you're not using it

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 90 [r3] (raw file):

        Context 'When the system is in the desired state, without DHCP' {
            BeforeAll {
                # This has intentially been left blank

Again - this can be removed

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

                return New-Object Object | 
                    Add-Member NoteProperty Name $listnerName -PassThru | 
                    Add-Member NoteProperty PortNumber 5030 -PassThru | 

Can you declare each of these hard coded numbers (port, ipAddress, subnetMask) as variables at the top of the test?

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 143 [r3] (raw file):

        Context 'When the system is in the desired state, with DHCP' {
            BeforeAll {

Remove

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 201 [r3] (raw file):

    Describe "$($script:DSCResourceName)\Test-TargetResource" {
        Context 'When the system is not in the desired state' {
            BeforeAll {

remove

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 209 [r3] (raw file):

                $testParameters += @{
                    Ensure = 'Present'
                    IpAddress = '192.168.10.45/255.255.252.0'

Create variable for hard-coded values

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 260 [r3] (raw file):

                }            

                Mock -CommandName Get-SQLAlwaysOnAvailabilityGroupListener -MockWith {

When you're using the same mock in multiple It statements or context blocks you can just declare once at the top of the Describe block

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 292 [r3] (raw file):

                }            

                Mock -CommandName Get-SQLAlwaysOnAvailabilityGroupListener -MockWith {

Since you have some of these where IsDHCP is set to false and some where it's set to true, divide those into two different 'Context' blocks and declare the mock at the top of each and then include each 'It' statement that uses that particular Mock in that 'Context' block. Alternatively, you could set a [Bool] $mockDHCP variable at the top of the Describe block and then just change that value for each 'It' rather than declaring the mock over and over.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 469 [r3] (raw file):

        Context 'When the system is in the desired state' {
            BeforeAll {
                # This has intentially been left blank

remove

---

Comments from Reviewable

[johlju]

Review status: 1 of 10 files reviewed at latest revision, 12 unresolved discussions.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Can you declare each of these hard coded numbers (port, ipAddress, subnetMask) as variables at the top of the test?

Love to do this. But I have problem doing this... Strange things going on when I add variables for these values. Doesn't work running the test in PowerShell, but it works running the test in ISE. :/ I will update the PR with the change in one place only (to begin with), to see what AppVeyor thinks about it. If it works I update the rest.

---

Comments from Reviewable

[johlju]

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

---

README.md, line 345 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Move these comments under 'Unreleased' - I think it got messed up when we released on Wed.

Done

---

DSCResources/xSQLServerAvailabilityGroupListener/xSQLServerAvailabilityGroupListener.psm1, line 288 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

can you divide up this line so that each 'and' is on its own line?

Done (hope it's better)

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 50 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

You can remove this block if you're not using it

Done. Removed everywhere

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 90 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Again - this can be removed

Done

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Love to do this. But I have problem doing this... Strange things going on when I add variables for these values. Doesn't work running the test in PowerShell, but it works running the test in ISE. :/
I will update the PR with the change in one place only (to begin with), to see what AppVeyor thinks about it. If it works I update the rest.

@mbreakey3 It failed in AppVeyor like it did when I run it in PowerShell. Can you help me debug why the variables don't work in the Mock when run from PowerShell but ISE? Staring me blind. Probably something simple. _The existing variable `$listnerName` don't work either. Never saw that before because the Name property is not used in the code (will remove it from the test in the next commit)._

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 143 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Remove

Done

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 201 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

remove

Done

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 469 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

remove

Done

---

Comments from Reviewable

[johlju]

Review status: 1 of 10 files reviewed at latest revision, 12 unresolved discussions.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

@mbreakey3 It failed in AppVeyor like it did when I run it in PowerShell. Can you help me debug why the variables don't work in the Mock when run from PowerShell but ISE? Staring me blind. Probably something simple.
The existing variable $listnerName don't work either. Never saw that before because the Name property is not used in the code (will remove it from the test in the next commit).

Okay, so if I set these as `$global:` it works. AppVeyor likes it too. Is there a better way? I can go with global, but doesn't feel right.

---

Comments from Reviewable

[mbreakey3]

Reviewed 2 of 8 files at r4.
Review status: 3 of 10 files reviewed at latest revision, 6 unresolved discussions.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Okay, so if I set these as $global: it works. AppVeyor likes it too. Is there a better way? I can go with global, but doesn't feel right.

See my comment above

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 209 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Create variable for hard-coded values

LGTM

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 41 [r4] (raw file):

    # Values used for mocking
    $global:desiredPortNumber = 5030

Yeah, so these have to be within the Describe block to work with Pester as local variables. Could you try making them $script: variables? If that doesn't work, you can move them into each Describe block separately.

---

Comments from Reviewable

[johlju]

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

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

See my comment above

So these variables doesn't want to work for me at all :/ Commited my last test to show the problem.

I tried using $script:. It doesn't work.
I tried using them inside the Describe-block. It doesn't work.
I tried using them inside the Context-block. It doesn't work.
I tried using them inside the Mock-block it self. Well that does work for the $actualPortNumber variable, but doesn't work on $actualIPAddress or $actualSubnetMask. This is the last commit

But each test does work in ISE (not tested $script thou), just not at the PowerShell prompt or in AppVeyor. I guess ISE are scoping these differently or something.

Any more suggestions, anyone? :)

---

Comments from Reviewable

[johlju]

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

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

So these variables doesn't want to work for me at all :/ Commited my last test to show the problem.

I tried using $script:. It doesn't work.
I tried using them inside the Describe-block. It doesn't work.
I tried using them inside the Context-block. It doesn't work.
I tried using them inside the Mock-block it self. Well that does work for the $actualPortNumber variable, but doesn't work on $actualIPAddress or $actualSubnetMask. This is the last commit

But each test does work in ISE (not tested $script thou), just not at the PowerShell prompt or in AppVeyor. I guess ISE are scoping these differently or something.

Any more suggestions, anyone? :)

Seems this is a bug (or missing feature). See pester/Pester#204. @mbreakey3 what are your thoughts?

---

Comments from Reviewable

[aultt]

Congrats @johlju Looks like the build worked!!!

[johlju]

Review status: 3 of 10 files reviewed at latest revision, 6 unresolved discussions.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 99 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Seems this is a bug (or missing feature). See pester/Pester#204. @mbreakey3 what are your thoughts?

Since I had problems with local variables I reverted back to the initial code with hardcoded values in mocks.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 209 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

LGTM

Since I had problems with local variables (see other comments) I reverted back to the initial code with hardcoded values in mocks.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 260 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

When you're using the same mock in multiple It statements or context blocks you can just declare once at the top of the Describe block

Done

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 292 [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Since you have some of these where IsDHCP is set to false and some where it's set to true, divide those into two different 'Context' blocks and declare the mock at the top of each and then include each 'It' statement that uses that particular Mock in that 'Context' block. Alternatively, you could set a [Bool] $mockDHCP variable at the top of the Describe block and then just change that value for each 'It' rather than declaring the mock over and over.

Done (I hope). Did not change the hardcoded values, see other comments.

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 41 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Yeah, so these have to be within the Describe block to work with Pester as local variables. Could you try making them $script: variables? If that doesn't work, you can move them into each Describe block separately.

Since I had problems with local variables (see other comments) I reverted back to the initial code with hardcoded values in mocks.

---

Tests/Unit/Stubs/SQLServerStub.psm1, line [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This stub file is not actual used by any test yet, but I figured I add this because of issue #91 will make sure that SQLServer module will used before SQLPS, if it exist. So we should actually run tests with both SQLPS and SQLServer stub module.
As of now, these tests I added only run the test using SQLPS stub module.

Shall I keep this file in?

---

Comments from Reviewable

[johlju]

@aultt Yes, it worked first time around too... :) Tried to get local variables to work in mocks, as per suggestion of @mbreakey3. That did not work out to well, that's why it failed. Reverted back to hardcoded values again to get AppVeyor happy. :D
And fixed the rest of the review comments.

[mbreakey3]

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 41 [r4] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Since I had problems with local variables (see other comments) I reverted back to the initial code with hardcoded values in mocks.

Yeah... we were just discussing this issue this morning. What about creating a helper function within the try block that gets the default values? That way they still are only defined in one place

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 8 files at r1, 2 of 8 files at r4.
Review status: 6 of 10 files reviewed at latest revision, 6 unresolved discussions.

---

Tests/Unit/Stubs/SQLServerStub.psm1, line [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Shall I keep this file in?

Yeah, I think it's fine to keep it in

---

Comments from Reviewable

[mbreakey3]

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 41 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Yeah... we were just discussing this issue this morning. What about creating a helper function within the try block that gets the default values? That way they still are only defined in one place

Here's a [link](https://github.com/PowerShell/DscResources/issues/175) to the current discussion on a similar topic

---

Comments from Reviewable

[aultt]

---

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

---

Comments from Reviewable

[mbreakey3]

Reviewed 2 of 8 files at r4, 2 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[mbreakey3]

I'm going to merge this for now, since all of the functionality looks good. We can work on the test formatting later once we have a solid workaround.

---

Comments from Reviewable

[mbreakey3]

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

---

Tests/Unit/xSQLServerAvailabilityGroupListener.Tests.ps1, line 41 [r4] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Here's a link to the current discussion on a similar topic

LGTM

---

Tests/Unit/Stubs/SQLServerStub.psm1, line [r3] (raw file):

Previously, mbreakey3 (Mariah) wrote…

Yeah, I think it's fine to keep it in

LGTM

---

Comments from Reviewable