xSQLServerHelper: Create helper function Test-ClusterPermissions #830

randomnote1 · 2017-09-19T17:35:20Z

Pull Request (PR) description

Added new helper function "Test-ClusterPermissions".
Changes to xSQLServerAlwaysOnAvailabilityGroup
- Use the new helper function "Test-ClusterPermissions".
- Refactored the unit tests to allow them to be more user friendly.
Changes to xSQLServerAlwaysOnAvailabilityGroupReplica
- Use the new helper function "Test-ClusterPermissions".
Changes to SMO.cs
- Added default properties to the Server class
  - AvailabilityGroups
  - Databases
  - EndpointCollection
- Added a new overload to the Login class
- Added default properties to the AvailabilityReplicas class
  - AvailabilityDatabases
  - AvailabilityReplicas

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

Task list:

Change details added to Unreleased section of CHANGELOG.md?
Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
Examples appropriately updated?
New/changed code adheres to Style Guidelines?
Unit and (optional) Integration tests created/updated where possible?

This change is

codecov-io · 2017-09-19T18:02:06Z

Codecov Report

Merging #830 into dev will decrease coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##            dev   #830   +/-   ##
===================================
- Coverage    96%    96%   -1%     
===================================
  Files        31     31           
  Lines      3316   3301   -15     
===================================
- Hits       3199   3184   -15     
  Misses      117    117

…oupReplica resource

johlju · 2017-09-20T16:23:59Z

Man, your did some complex refactor of the tests here! 😄 Awesome work on the test cases! Very clean code!

Reviewed 6 of 8 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

CHANGELOG.md, line 26 at r2 (raw file):

      environment variable in the (LCM) PowerShell session did not contain the new
      module path.
  - Added new helper function "Test-ClusterPermissions". ([issue #446](https://github.com/PowerShell/xSQLServer/issues/446))

Move the full stop ('.') to the end of the text (after the parenthesis).

xSQLServerHelper.psm1, line 1199 at r2 (raw file):

        The server object on which to perform the test.
#>
function Test-ClusterPermissions

This function (with the verb 'Test') should have and output type of System.Boolean and return $true if the cluster permission exist, or $false if they don't.

xSQLServerHelper.psm1, line 1236 at r2 (raw file):

'recommended '

This must be localized as well. I think it is better to have two different localized strings instead.

xSQLServerHelper.psm1, line 1236 at r2 (raw file):

Write-Verbose

For all Write-Verbose please add -Verbose parameter. It is a bug (or feature, unknown at this point) that LCM does not pass the VerbosePreference correctly to (helper) modules. There is an issue about that somewhere (in the DscResources) also. The workaround is to add -Verbose on all Write-Verbose

xSQLServerHelper.psm1, line 1252 at r2 (raw file):

'recommended ',$loginName,"Trying with '$ntAuthoritySystemName'.

This must be localized as well.

xSQLServerHelper.psm1, line 1267 at r2 (raw file):

    {
        throw ($script:localizedData.ClusterPermissionsMissing -f $sqlServer,$sqlInstanceName )
    }

Add an else here to write a verbose message saying that the correct permission was found for account X?

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 27 at r2 (raw file):

    InModuleScope $script:DSCResourceName {

        #region mock server object variables

Please remove the extra indent inside the regions. Throughout.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 382 at r2 (raw file):

=

Missing space after equal sign. Throughout.
If you use VS Code you can apply formatting and fix all these (I think).

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 466 at r2 (raw file):

        Describe 'xSQLServerAlwaysOnAvailabilityGroup\Set-TargetResource' {
            BeforeAll {
                Mock -CommandName Connect-SQL -MockWith $mockConnectSqlServer1 -Verifiable -ParameterFilter { $SQLServer -eq $mockServer1Name }

Please change so braces are not on the same row. For both Mock and Assert-MockCalled. Throughout.

Tests/Unit/xSQLServerHelper.Tests.ps1, line 1536 at r2 (raw file):

Mock -CommandName Test-LoginEffectivePermissions -MockWith { $mockClusterServicePermissionsPresent } -Verifiable -ParameterFilter { $LoginName -eq $clusterServiceName }

Please write this more readable

Mock -CommandName Test-LoginEffectivePermissions -MockWith {
    $mockClusterServicePermissionsPresent
} -ParameterFilter {
    $LoginName -eq $clusterServiceName
} -Verifiable

Throughout.

Tests/Unit/xSQLServerHelper.Tests.ps1, line 1542 at r2 (raw file):

            $systemAccountName= 'NT AUTHORITY\System'
        }
        BeforeEach {

New row before this one.

Tests/Unit/xSQLServerHelper.Tests.ps1, line 1548 at r2 (raw file):

New-Object -TypeName Microsoft.SqlServer.Management.Smo.Login($mockServerObject,$clusterServiceName)

This seems to work, but this should use named parameters. I think this is the more correct way?

New-Object -TypeName Microsoft.SqlServer.Management.Smo.Login -ArgumentList $mockServerObject, $clusterServiceName

Throughout.

Tests/Unit/xSQLServerHelper.Tests.ps1, line 1564 at r2 (raw file):

Assert-MockCalled -CommandName Test-LoginEffectivePermissions -Scope It -Times 0 -Exactly -ParameterFilter { $LoginName -eq $clusterServiceName }

make this more readable. Throughout.

Assert-MockCalled -CommandName Test-LoginEffectivePermissions -ParameterFilter {
    $LoginName -eq $clusterServiceName
} -Exactly -Times 0 -Scope It

Comments from Reviewable

Updated the return value for Test-ClusterPermissions

randomnote1 · 2017-09-20T18:01:40Z

Thanks! It was really bothering me that I couldn't "easily" update the tests with the minor change that occurred. Now it should be a little easier! :)

Review status: 4 of 9 files reviewed at latest revision, 14 unresolved discussions.

CHANGELOG.md, line 26 at r2 (raw file):