xFailOverCluster: Fixed failing tests in AppVeyor

johlju commented 7 years ago

Changes to xClusterPreferredOwner
- Script Analyzer warnings have been fixed (issue #50). This also failed the tests for the resource.
Changes to xClusterDisk
- Fixed test that was failing in AppVeyor (issue #55).

This change is

msftclas commented 7 years ago

@johlju, Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request. Thanks, Microsoft Pull Request Bot

johlju commented 7 years ago

@bgelens If you have time to review this one. This fixes the tests failing in AppVeyor. Unfortunately the files contained a lot of spaces where there shouldn't so VSCode trimmed them. So there are more changes than I wanted. :/

bgelens commented 7 years ago

Some minor comments since you are on a roll already ;)

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

DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 40 at r1 (raw file):


        Write-Verbose -Message "Retrieving Owner information for Cluster Group $ClusterGroup"
        (((Get-ClusterGroup -cluster $Clustername)| Where-Object {$_.name -like "$ClusterGroup"} | Get-ClusterOwnerNode).ownernodes).name

Parameter ClusterName has been fixed with correct PascalCase. Now the variable needs to fixed as well

DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 47 at r1 (raw file):

            {
                Write-Verbose -Message "Retrieving Owner information for Cluster Resource $resource"
                (((Get-ClusterResource -cluster $Clustername)| Where-Object {$_.name -like "$resource"} | Get-ClusterOwnerNode).ownernodes).name

ClusterName

DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 55 at r1 (raw file):

    $returnValue = @{
        ClusterGroup = $ClusterGroup
        Clustername = $Clustername

ClusterName

DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 190 at r1 (raw file):


    if ($Ensure -eq 'Present')
        {

Please correct indentation where applicable like here

DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 210 at r1 (raw file):


    if ($Ensure -eq 'Absent')
        {

and here

Tests/MSFT_xClusterDisk.Tests.ps1, line 87 at r1 (raw file):


        Mock -CommandName 'Get-ClusterParameter' -ParameterFilter { $Name -eq 'DiskIdGuid' } -MockWith {
            #write-host $args.count -ForegroundColor Cyan

these can me removed I think

Tests/MSFT_xClusterDisk.Tests.ps1, line 88 at r1 (raw file):

        Mock -CommandName 'Get-ClusterParameter' -ParameterFilter { $Name -eq 'DiskIdGuid' } -MockWith {
            #write-host $args.count -ForegroundColor Cyan
            #write-host $args -ForegroundColor Cyan

these can me removed I think

Tests/MSFT_xClusterDisk.Tests.ps1, line 107 at r1 (raw file):


            It 'Returns a [System.Collection.Hashtable] type' {

Empty lines can be removed when they make not a lot of sense (multiple occasions, won't repeat remark)

Tests/MSFT_xClusterDisk.Tests.ps1, line 121 at r1 (raw file):

                $Result.Label  | Should Be $TestParameter.Label

                $Result -is [System.Collections.Hashtable] | Should Be $true

Not sure why this same test is in every It?

Comments from Reviewable

johlju commented 7 years ago

Well, since I'm on a roll I fixed more style issues. :wink: There is more to be done, but don't want to do too much at once.

Review status: 1 of 3 files reviewed at latest revision, 9 unresolved discussions.

DSCResources/MSFT_xClusterPreferredOwner/MSFT_xClusterPreferredOwner.psm1, line 40 at r1 (raw file):