Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to xSQLServerMaxDop #317

Merged
merged 23 commits into from
Feb 20, 2017
Merged

Conversation

luigilink
Copy link
Contributor

@luigilink luigilink commented Jan 17, 2017

Changes to xSQLServerMaxDop

  • Added Pester Test

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

  • 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 Reviewable

@luigilink luigilink changed the title Changes to xSQLServerMaxDop BREAKING CHANGE: Changes to xSQLServerMaxDop Jan 17, 2017
@luigilink
Copy link
Contributor Author

Pester failed in Windows Powershell but all is good in PowerShell ISE, any idea ? oO

@johlju
Copy link
Member

johlju commented Jan 17, 2017

ISE seems to handle variables in a different scope and therefor when you do some things in ISE it does not work in the console. I have had that problem more than once, so now days I always run the tests i PowerShell Console (and edit in VS Code).

I will check the code to see if I can help. :)

@johlju johlju added breaking change When used on an issue, the issue has been determined to be a breaking change. waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jan 17, 2017
@luigilink
Copy link
Contributor Author

I found the pester issue, the mock object for connect-sql must define in each describe; Good to know.

@luigilink
Copy link
Contributor Author

I have to rebase, I forgot 😄

@johlju
Copy link
Member

johlju commented Jan 17, 2017

Great that you found the problem. 👍

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jan 17, 2017
@johlju
Copy link
Member

johlju commented Jan 17, 2017

Seems we did not get a purple button to Reviewable for this one. This is the link.

Reviewable link: https://reviewable.io/reviews/powershell/xsqlserver/317

@johlju
Copy link
Member

johlju commented Jan 17, 2017

I will review the test after these comments are solved. Awesome work @luigilink! 😄


Reviewed 6 of 8 files at r1, 1 of 2 files at r2.
Review status: 7 of 8 files reviewed at latest revision, 24 unresolved discussions.


README.md, line 535 at r2 (raw file):

parallelism Server Configuration Option

parallelism server configuration option.


README.md, line 536 at r2 (raw file):

The max degree of parallelism option configure the number of processors to use in parallel plan execution.

The max degree of parallelism option is used to limit the number of processors to use in parallel plan execution.


README.md, line 538 at r2 (raw file):

The max degree of parallelism option configure the number of processors to use in parallel plan execution.
Read more about max degree of parallelism in this article [Configure the max degree of parallelism Server Configuration Option](https://msdn.microsoft.com/en-us/library/ms189094.aspx)

Please add this new section:

Formula for dynamically allocating max degree of parallelism.

  • If the number of configured NUMA nodes configured in SQL Server equals 1, then max degree of parallelism is calculated using number of cores divided in 2 (numberOfCores / 2), then rounded up to the next integer (3.5 > 4).
  • If the number of cores configured in SQL Server are greater than or equal to 8 cores then max degree of parallelism will be set to 8.
  • If the number of configured NUMA nodes configured in SQL Server is greater than 2 and thenumber of cores are less than 8 then max degree of parallelism will be set to the number of cores.

README.md, line 547 at r2 (raw file):

* **[String] SQLInstance** (Key): The name of the SQL instance to be configured
* **[String] Ensure** _(Write)_: An enumerated value that describes if Min and Max memory is configured. { *Present* | Absent }.

Change description to:

When set to 'Present' then max degree of parallelism will be set to either the value in parameter MaxDop or dynamically configured when parameter DynamicAlloc is set to $true. When set to 'Absent' max degree of parallelism will be set to 0 which means no limit in number of processors used in parallel plan execution .


README.md, line 548 at r2 (raw file):

* **[String] SQLInstance** (Key): The name of the SQL instance to be configured
* **[String] Ensure** _(Write)_: An enumerated value that describes if Min and Max memory is configured. { *Present* | Absent }.
* **[Boolean] DyamicAlloc** _(Write)_: Flag to indicate if MaxDop is dynamically configured

Typo in parameter name, missing 'n': DynamicAlloc

Change description to:
If set to $true then max degree of parallelism will be dynamically configured. When this is set parameter is set to $true, the parameter MaxDop must be set to $null or not be configured.


README.md, line 549 at r2 (raw file):

* **[String] Ensure** _(Write)_: An enumerated value that describes if Min and Max memory is configured. { *Present* | Absent }.
* **[Boolean] DyamicAlloc** _(Write)_: Flag to indicate if MaxDop is dynamically configured
* **[Sint32] MaxDop** _(Write)_: Numeric value to configure MaxDop to

A numeric value to limit the number of processors used in parallel plan execution.


README.md, line 550 at r2 (raw file):

* **[Boolean] DyamicAlloc** _(Write)_: Flag to indicate if MaxDop is dynamically configured
* **[Sint32] MaxDop** _(Write)_: Numeric value to configure MaxDop to
* **[String] SQLServer** _(Required)_: The host name of the SQL Server to be configured

Could you move this parameter under the Key, so mandatory parameters are listed together in order of "importance" so to say :)


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

[system.midpointrounding]

Upper letters: [System.MidpointRounding]


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 6 at r2 (raw file):

Server Configuration Option

server configuration option.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 9 at r2 (raw file):

    .PARAMETER SQLServer
    This is a the SQL Server for the database

Please rephrase, database seems wrong :)
Suggestion, use the description from the schema.

Please do for Set- and Test method as well


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 12 at r2 (raw file):

    .PARAMETER SQLInstanceName
    This is a the SQL instance for the database

Please rephrase, database seems wrong :)
Suggestion, use the description from the schema.

Please do for Set- and Test method as well


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 15 at r2 (raw file):

    .PARAMETER Ensure
    This is The Ensure Set to 'present' to specificy that the MAXDOP should be configured.

Please revise. See if you can use from the README.md.
Please do for Set- and Test method as well


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 18 at r2 (raw file):

    .PARAMETER DynamicAlloc
    This is the boolean DynamicAlloc to configure automatically the MAXDOP configuration option

Please revise. See if you can use from the README.md.
Please do for Set- and Test method as well


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 21 at r2 (raw file):

    .PARAMETER DynamicAlloc
    This is the numeric MaxDop to specify the value of the MAXDOP configuration option

This has the wrong parameter name.

Please revise description. See if you can use from the README.md.
Please do for Set- and Test method as well


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 42 at r2 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $Ensure = 'Present',

Please remove unused non-mandatory parameter.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 51 at r2 (raw file):

    )

    $sql = Connect-SQL -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Please change this to $sqlServerObject


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 63 at r2 (raw file):

            {
                Write-Warning -Message 'MaxDop paramater must be null if DynamicAlloc set to true'
                throw New-TerminatingError -ErrorType ParameterConflict `

This error type does not seem to exist. Could you change this so the warning text is actually thrown instead?


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 158 at r2 (raw file):

    )

    $sql = Connect-SQL -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Please change this to $sqlServerObject


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 162 at r2 (raw file):

Server Configuration Option

server configuration option


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 172 at r2 (raw file):

                    {
                        Write-Warning -Message 'MaxDop paramater must be null if DynamicAlloc set to true'
                        throw New-TerminatingError -ErrorType ParameterConflict `

Same here. Please throw the warning instead.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 201 at r2 (raw file):

        catch
        {
            throw New-TerminatingError -ErrorType MaxDopGetError `

This error has no localized string either. Could you add that?


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 256 at r2 (raw file):

Server Configuration Option

server configuration option


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.schema.mof, line 7 at r2 (raw file):

    [Write, Description("Flag to Dynamically allocate Maxdop based on Best Practices")] Boolean DynamicAlloc;
    [Write, Description("Numeric value to configure Maxdop to")] Sint32 MaxDop;
    [Required, Description("The host name of the SQL Server to be configured.")] String SQLServer;

I have starting to questioning if SQLServer should be Required/Key, but instead it maybe should be Write, with the default value of env:COMPUTERNAME.
I submit an issue so we can discuss this.

Leave it as-is for now


Examples/Resources/xSQLServerMaxDop/2-SetMaxDopToAuto.ps1, line 2 at r2 (raw file):

<#
.EXAMPLE

Please add an example to set the Ensure to 'Absent'


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Jan 17, 2017
@luigilink luigilink force-pushed the Add-TestUnit-SQLMaxDop branch 2 times, most recently from f9f1746 to e23dd4f Compare January 24, 2017 13:50
@luigilink
Copy link
Contributor Author

Review status: 4 of 8 files reviewed at latest revision, 24 unresolved discussions.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 63 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This error type does not seem to exist. Could you change this so the warning text is actually thrown instead?

Could you help me on this ?
Do I have to delete the Cmdlet Write-Warning and put text in a parameter for New-TerminatingError Cmdlet?


Comments from Reviewable

@luigilink
Copy link
Contributor Author

Review status: 4 of 8 files reviewed at latest revision, 24 unresolved discussions.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 201 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This error has no localized string either. Could you add that?

How I did this ? I never used localized 😢


Comments from Reviewable

@luigilink
Copy link
Contributor Author

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


README.md, line 535 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

parallelism Server Configuration Option

parallelism server configuration option.

Done.


README.md, line 536 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The max degree of parallelism option configure the number of processors to use in parallel plan execution.

The max degree of parallelism option is used to limit the number of processors to use in parallel plan execution.

Done.


README.md, line 538 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this new section:

Formula for dynamically allocating max degree of parallelism.

  • If the number of configured NUMA nodes configured in SQL Server equals 1, then max degree of parallelism is calculated using number of cores divided in 2 (numberOfCores / 2), then rounded up to the next integer (3.5 > 4).
  • If the number of cores configured in SQL Server are greater than or equal to 8 cores then max degree of parallelism will be set to 8.
  • If the number of configured NUMA nodes configured in SQL Server is greater than 2 and thenumber of cores are less than 8 then max degree of parallelism will be set to the number of cores.

Done.


README.md, line 547 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change description to:

When set to 'Present' then max degree of parallelism will be set to either the value in parameter MaxDop or dynamically configured when parameter DynamicAlloc is set to $true. When set to 'Absent' max degree of parallelism will be set to 0 which means no limit in number of processors used in parallel plan execution .

Done.


README.md, line 548 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Typo in parameter name, missing 'n': DynamicAlloc

Change description to:
If set to $true then max degree of parallelism will be dynamically configured. When this is set parameter is set to $true, the parameter MaxDop must be set to $null or not be configured.

Done.


README.md, line 549 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A numeric value to limit the number of processors used in parallel plan execution.

Done.


README.md, line 550 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you move this parameter under the Key, so mandatory parameters are listed together in order of "importance" so to say :)

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

[system.midpointrounding]

Upper letters: [System.MidpointRounding]

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 6 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Server Configuration Option

server configuration option.

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 9 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please rephrase, database seems wrong :)
Suggestion, use the description from the schema.

Please do for Set- and Test method as well

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 12 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please rephrase, database seems wrong :)
Suggestion, use the description from the schema.

Please do for Set- and Test method as well

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 15 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please revise. See if you can use from the README.md.
Please do for Set- and Test method as well

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 18 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please revise. See if you can use from the README.md.
Please do for Set- and Test method as well

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 21 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This has the wrong parameter name.

Please revise description. See if you can use from the README.md.
Please do for Set- and Test method as well

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 42 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please remove unused non-mandatory parameter.

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 51 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change this to $sqlServerObject

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 63 at r2 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

Could you help me on this ?
Do I have to delete the Cmdlet Write-Warning and put text in a parameter for New-TerminatingError Cmdlet?

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 158 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change this to $sqlServerObject

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 162 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Server Configuration Option

server configuration option

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 172 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same here. Please throw the warning instead.

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 201 at r2 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

How I did this ? I never used localized 😢

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 256 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Server Configuration Option

server configuration option

Done.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.schema.mof, line 7 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I have starting to questioning if SQLServer should be Required/Key, but instead it maybe should be Write, with the default value of env:COMPUTERNAME.
I submit an issue so we can discuss this.

Leave it as-is for now

Ok 😄


Examples/Resources/xSQLServerMaxDop/2-SetMaxDopToAuto.ps1, line 2 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add an example to set the Ensure to 'Absent'

Done.


Comments from Reviewable

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jan 25, 2017
@johlju
Copy link
Member

johlju commented Jan 29, 2017

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


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 42 at r2 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

Done.

The parameter is still here. $Ensure is not used in the Get-method (?) so it does not need to be a parameter.
If it is only here so you the Get-method could be called with @PSBoundParameters, could you call it with splatting the parameters in a variable instead? Then we don't need the unused non-mandatory parameter in the Get-method parameter list :)


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 63 at r2 (raw file):

MaxDopParamMustBeNull

Add this to the SQLServer.strings.psd1 file, like you did with 'MaxDopSetError'.

MaxDopParamMustBeNull = 'MaxDop parameter must be set to $null or not assigned if DynamicAlloc parameter is set to $true.'

DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 123 at r2 (raw file):

    .PARAMETER Ensure
    This is The Ensure Set to 'present' to specificy that the MAXDOP should be configured.

Please revise this text. Maybe the phrases from the README.md could be used instead?


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 189 at r5 (raw file):

'Ensure is absent - MAXDOP reset to default value'

Change to something like:

'Desired state should be absent - MAXDOP is reset to the default value.'


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 197 at r5 (raw file):

"Set MaxDop to $targetMaxDop

Change to something like:

"Setting MAXDOP value to $targetMaxDop."


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 220 at r5 (raw file):

    .PARAMETER Ensure
    This is The Ensure Set to 'present' to specificy that the MAXDOP should be configured.

Please revise this text. Maybe the phrases from the README.md could be used instead?


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.schema.mof, line 7 at r2 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

Ok 😄

When I deliberated with my self in the issue I submitted (issue #319) I don't yet see a point adding Required or Key to SQLServer. If you disagree then please comment why on the issue #319. :)

For this PR please

  • Change SQLServer back to Write (and non-mandatory in the Set-, Get- and Test-moethods).
  • Add env:COMPUTERNAME as the default value for SQLServer. Make sure to add that default value to the descriptions in the README.md, schema and comment-based help.
  • This makes this change no longer a breaking change, so change the PR title, and remove the line from the CHANGELOG as well.

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 41 at r5 (raw file):

    #endregion Pester Test Initialization

    Describe "$($script:DSCResourceName)\Get-TargetResource" {

There are some rows that are missed in the tests, could you add a test for them as well?

Method Line Command
Get-TargetResource 98 $currentEnsure = 'Absent'

https://ci.appveyor.com/project/PowerShell/xsqlserver/build/4.0.774.0#L4971


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 43 at r5 (raw file):

    Describe "$($script:DSCResourceName)\Get-TargetResource" {
        Mock -CommandName Connect-SQL -MockWith {
            $mockSqlServerObject = [pscustomobject]@{

PSCustomObject


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 80 at r5 (raw file):

            }
            
            It 'Should return wrong MaxDop' {

'Should return the wrong MaxDop value'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 112 at r5 (raw file):

            }
            
            It 'Should return right MaxDop' {

'Should return the correct MaxDop value'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 186 at r5 (raw file):

        }

        Context 'When the MaxDop paramater is not null and DynamicAlloc set to true' {

Misspelled: parameter


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 193 at r5 (raw file):

            }

            It 'Should Throw when MaxDop paramater not null if DynamicAlloc set to true' {

'Should throw the correct error'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 194 at r5 (raw file):

            It 'Should Throw when MaxDop paramater not null if DynamicAlloc set to true' {
                { Get-TargetResource @testParameters } | Should Throw

Make sure | Should Throw catches the correct error, not just any error.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 196 at r5 (raw file):

                { Get-TargetResource @testParameters } | Should Throw
            }
            It 'Should call the mock function Connect-SQL' {

Add a blank line before this one


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 236 at r5 (raw file):

            }      

            It 'Should return the state as false when desired MaxDop is not present' {

'...MaxDop is the wrong value'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 257 at r5 (raw file):

            }      

            It 'Should return the state as true when desired MaxDop is not present' {

'...MaxDop is the correct value'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 277 at r5 (raw file):

            }      

            It 'Should return the state as true when desired MaxDop is present' {

'...MaxDop is the correct value'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 301 at r5 (raw file):

            }      

            It 'Should return the state as true when desired MaxDop is not present' {

'Should return the state as false then desired MaxDop is the wrong value'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 318 at r5 (raw file):

    }
    
    Describe "$($script:DSCResourceName)\Set-TargetResource" {

There are some rows that are missed in the tests, could you add a test for them as well?

Method Line Command
Set-TargetResource 201 throw New-TerminatingError -ErrorType MaxDopSetError `...
Set-TargetResource 202 $SQLServer,$SQLInstanceName,$targetMaxDop

https://ci.appveyor.com/project/PowerShell/xsqlserver/build/4.0.774.0#L4971


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 343 at r5 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context 'When the MaxDop paramater is not null and DynamicAlloc set to true' {

Missspelled: parameter


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 351 at r5 (raw file):

            }

            It 'Should Throw when MaxDop paramater not null if DynamicAlloc set to true' {

Should throw the correct error.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 352 at r5 (raw file):

            It 'Should Throw when MaxDop paramater not null if DynamicAlloc set to true' {
                { Set-TargetResource @testParameters } | Should Throw

Please make sure to catch the correct error here, not just any error.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 360 at r5 (raw file):

        }

        Context 'When the Ensure paramater is set to Absent' {

Misspelled: parameter


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 383 at r5 (raw file):

            }

            It 'Should Not Throw when MaxDop paramater is not null and DynamicAlloc set to false' {

Misspelled: parameter


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 384 at r5 (raw file):

            It 'Should Not Throw when MaxDop paramater is not null and DynamicAlloc set to false' {
                { Set-TargetResource @testParameters } | Should Not Throw

Could you improve on this test to see that the value you are setting with Alter() is actually what you expect to set? Like you did in the `Set-SqlDatabaseRecoveryModel' test.
https://github.com/PowerShell/xSQLServer/blob/dev/Tests/Unit/xSQLServerHelper.Tests.ps1#L341


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 399 at r5 (raw file):

            }      

            It 'Should Not Throw when MaxDop paramater is not null and DynamicAlloc set to false' {

Misspelled: parameter


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 400 at r5 (raw file):

            It 'Should Not Throw when MaxDop paramater is not null and DynamicAlloc set to false' {
                { Set-TargetResource @testParameters } | Should Not Throw

Same as the comment above. Make sure the value you are setting with Alter() is what you expected to set.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 422 at r5 (raw file):

    #endregion
}

Please add test for the helper function Get-SqlDscDynamicMaxDop


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Jan 29, 2017
@luigilink luigilink changed the title BREAKING CHANGE: Changes to xSQLServerMaxDop Changes to xSQLServerMaxDop Feb 2, 2017
@codecov-io
Copy link

codecov-io commented Feb 17, 2017

Codecov Report

Merging #317 into dev will increase coverage by 1%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##            dev   #317   +/-   ##
===================================
+ Coverage    63%    65%   +1%     
===================================
  Files        31     31           
  Lines      3092   3102   +10     
===================================
+ Hits       1978   2036   +58     
+ Misses     1114   1066   -48

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4304e00...c332298. Read the comment docs.

@luigilink
Copy link
Contributor Author

Reviewed 3 of 3 files at r8, 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 29 unresolved discussions.


Comments from Reviewable

@luigilink
Copy link
Contributor Author

luigilink commented Feb 17, 2017

@johlju Ready for review. Waiting for the author can be removed

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Feb 18, 2017
@johlju
Copy link
Member

johlju commented Feb 19, 2017

Great work hitting every line of code! 😄
https://codecov.io/gh/PowerShell/xSQLServer/pull/317/src/DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1

Just a few changes left.


Reviewed 1 of 7 files at r6, 3 of 3 files at r8, 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 23 unresolved discussions.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 42 at r9 (raw file):

Ensure = $currentEnsure
DynamicAlloc = $DynamicAlloc

Why did you remove these from the hash table that is returned, and all the logic that assigned them? I thought that this was good information to have when running Get-DscConfiguration.

If you like you can add it back again, or submit an issue so that can be fixed later :)


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 384 at r5 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

In progress, I have an error 😢

You solved it right? We can resolve this comment? If so, write Done. :)


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 400 at r5 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

In progress

You solved it right? We can resolve this comment? If so, write Done. :)


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 101 at r9 (raw file):

$result.MaxDop | Should Be 4

$result.MaxDop | Should Be $mockMaxDegreeOfParallelism


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 120 at r9 (raw file):

            Mock -CommandName Connect-SQL -MockWith $mockConnectSQL -Verifiable

            Mock -CommandName Get-CimInstance -MockWith $mockCimInstance_Win32Processor -ParameterFilter { $ClassName -eq 'Win32_Processor' } -Verifiable

Put these two mocks in a BeforeEach {} block, then you don't have to repeat the mock when you change the "dynamic variable". The BeforeEach-block will run before each of the Context-blocks.

See here:
https://github.com/PowerShell/xSQLServer/blob/dev/Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1#L779


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 139 at r9 (raw file):

                }

                It 'Should not call the mock function Get-SqlDscDynamicMaxDop' {

Change description to 'Should not call the mock function Get-CimInstance', or mock Get-SqlDscDynamicMaxDop in this Context-block instead.

If you change the description, then please add a second mock for Get-CimInstance (where you mock the first one). This is so that the assertion work. Currently it would give a false positive if Get-CimInstance would be called with another set of parameter filters than the mock currently have.
Do not use `-Verifiable' on this one, since we don't expect this to ever to be called.

Mock -CommandName Get-CimInstance

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 161 at r9 (raw file):

                It 'Should not call the mock function Get-CimInstance' {
                    Assert-MockCalled Get-CimInstance -Exactly -Times 0 -Scope Context

if you choose to mock Get-SqlDscDynamicMaxDop above, then mock that here as well. If you changed description above, then no change is needed here.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 164 at r9 (raw file):

                }
            }

Could you add a test so that you also have to set dynamic variable $mockMaxDegreeOfParallelism to a higher value when DynamicAlloc = $false? Seems you only test $mockMaxDegreeOfParallelism = 4 right now?


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 189 at r9 (raw file):

            $mockNumberOfCores = 2

            Mock -CommandName Get-CimInstance -MockWith $mockCimInstance_Win32Processor -ParameterFilter { $ClassName -eq 'Win32_Processor' } -Verifiable

When you add a BeforeEach-block, this could be removed.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 191 at r9 (raw file):

            Mock -CommandName Get-CimInstance -MockWith $mockCimInstance_Win32Processor -ParameterFilter { $ClassName -eq 'Win32_Processor' } -Verifiable

            Context 'When the system is not in the desired state, DynamicAlloc is set to true and NumberOfLogicalProcessors = 4' {

'... and NumberOfCores = 2'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 197 at r9 (raw file):

                }      

                It 'Should return the state as true when desired MaxDop is the correct value' {

This description does not seems correct. Please revise.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 215 at r9 (raw file):

            $mockNumberOfLogicalProcessors = 1

            Mock -CommandName Get-CimInstance -MockWith $mockCimInstance_Win32Processor -ParameterFilter { $ClassName -eq 'Win32_Processor' } -Verifiable

When you add a BeforeEach-block, this could be removed.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 223 at r9 (raw file):

                }      

                It 'Should return the state as true when desired MaxDop is the correct value' {

This description does not seems correct. Please revise.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 239 at r9 (raw file):

            }

            $mockNumberOfLogicalProcessors = 4

Could you change this test so that you also have to set dynamic variable $mockMaxDegreeOfParallelism to a higher value when DynamicAlloc = $true? Seems you only test $mockMaxDegreeOfParallelism = 4 right now?


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 242 at r9 (raw file):

            $mockNumberOfCores = 8

            Mock -CommandName Get-CimInstance -MockWith $mockCimInstance_Win32Processor -ParameterFilter { $ClassName -eq 'Win32_Processor' } -Verifiable

When you add a BeforeEach-block, this could be removed.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 244 at r9 (raw file):

'...set to true and NumberOfCores = 8'

Maybe?
'...set to true, NumberOfLogicalProcessors = 4 and NumberOfCores = 8'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 250 at r9 (raw file):

                }      

                It 'Should return the state as true when desired MaxDop is the correct value' {

This description does not seems correct. Please revise.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 284 at r9 (raw file):

            $mockMaxDegreeOfParallelism = 0

            Mock -CommandName Connect-SQL -MockWith $mockConnectSQL -Verifiable

When you add a BeforeEach-block, this could be removed.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 292 at r9 (raw file):

                }      

                It 'Should return the state as false when desired MaxDop is the correct value' {

This description does not seems correct. Please revise.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 334 at r9 (raw file):

                }

                It 'Should Throw when MaxDop parameter not null if DynamicAlloc set to true' {

'Should throw the correct error'


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 375 at r9 (raw file):

            }

            $mockNumberOfLogicalProcessors = 4

Could you change these to a higher value so that you need to set a higher value for dynamic variable $mockExpectedMaxDopForAlterMethod?


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 432 at r9 (raw file):

'Should Throw when Alter Method was called with invalid operation'

'Shoud throw the correct error when Alter() method was called with invalid operation'


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Feb 19, 2017
@luigilink
Copy link
Contributor Author

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


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 42 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ensure = $currentEnsure
DynamicAlloc = $DynamicAlloc

Why did you remove these from the hash table that is returned, and all the logic that assigned them? I thought that this was good information to have when running Get-DscConfiguration.

If you like you can add it back again, or submit an issue so that can be fixed later :)

You tell me to remove Ensure and DynamicAlloc from parameter 😢
I thought that parameter with $false should be removed from Get-TargetResource.
I can fixed it but I have to fix test too, just tell me.


Comments from Reviewable

@luigilink
Copy link
Contributor Author

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


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 384 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You solved it right? We can resolve this comment? If so, write Done. :)

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 400 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You solved it right? We can resolve this comment? If so, write Done. :)

Done.


Comments from Reviewable

@luigilink
Copy link
Contributor Author

Thx. Codecov is awesome tool 👍


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


Comments from Reviewable

@luigilink
Copy link
Contributor Author

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 22 unresolved discussions.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 101 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$result.MaxDop | Should Be 4

$result.MaxDop | Should Be $mockMaxDegreeOfParallelism

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 120 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Put these two mocks in a BeforeEach {} block, then you don't have to repeat the mock when you change the "dynamic variable". The BeforeEach-block will run before each of the Context-blocks.

See here:
https://github.com/PowerShell/xSQLServer/blob/dev/Tests/Unit/MSFT_xSQLServerSetup.Tests.ps1#L779

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 139 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change description to 'Should not call the mock function Get-CimInstance', or mock Get-SqlDscDynamicMaxDop in this Context-block instead.

If you change the description, then please add a second mock for Get-CimInstance (where you mock the first one). This is so that the assertion work. Currently it would give a false positive if Get-CimInstance would be called with another set of parameter filters than the mock currently have.
Do not use `-Verifiable' on this one, since we don't expect this to ever to be called.

Mock -CommandName Get-CimInstance

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 161 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if you choose to mock Get-SqlDscDynamicMaxDop above, then mock that here as well. If you changed description above, then no change is needed here.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 189 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

When you add a BeforeEach-block, this could be removed.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 197 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This description does not seems correct. Please revise.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 215 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

When you add a BeforeEach-block, this could be removed.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 223 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This description does not seems correct. Please revise.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 239 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you change this test so that you also have to set dynamic variable $mockMaxDegreeOfParallelism to a higher value when DynamicAlloc = $true? Seems you only test $mockMaxDegreeOfParallelism = 4 right now?

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 242 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

When you add a BeforeEach-block, this could be removed.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 244 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'...set to true and NumberOfCores = 8'

Maybe?
'...set to true, NumberOfLogicalProcessors = 4 and NumberOfCores = 8'

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 250 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This description does not seems correct. Please revise.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 284 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

When you add a BeforeEach-block, this could be removed.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 292 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This description does not seems correct. Please revise.

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 334 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Should throw the correct error'

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 432 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Should Throw when Alter Method was called with invalid operation'

'Shoud throw the correct error when Alter() method was called with invalid operation'

Done.


Comments from Reviewable

@luigilink
Copy link
Contributor Author

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


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 164 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a test so that you also have to set dynamic variable $mockMaxDegreeOfParallelism to a higher value when DynamicAlloc = $false? Seems you only test $mockMaxDegreeOfParallelism = 4 right now?

Done.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 375 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you change these to a higher value so that you need to set a higher value for dynamic variable $mockExpectedMaxDopForAlterMethod?

Done.


Comments from Reviewable

@luigilink
Copy link
Contributor Author

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 22 unresolved discussions.


Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 191 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'... and NumberOfCores = 2'

Done.


Comments from Reviewable

@luigilink
Copy link
Contributor Author

@johlju "Waiting for the author" can be removed

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Feb 20, 2017
@johlju
Copy link
Member

johlju commented Feb 20, 2017

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 42 at r9 (raw file):

Previously, luigilink (Jean-Cyril Drouhin) wrote…

You tell me to remove Ensure and DynamicAlloc from parameter 😢
I thought that parameter with $false should be removed from Get-TargetResource.
I can fixed it but I have to fix test too, just tell me.

LGTM

Sorry, I meant just to remove them from the parameter list of the Get-TargetResource, not the logic returning the Ensure. Sorry, it was me being unclear 😞
The reason from removing them from the parameter list of the Get-methof was because the parameters were not being used in the code.
But the logic that returned the status of Ensure and DynamicAlloc was great addition to the resource.

You don't have to add the logic back, we can fix that in another PR after this is merged. I will submit an issue for this. Issue #396 submitted.

Sorry again, my friend. :/ I learn by my mistakes. :)


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Feb 20, 2017

@luigilink I will merge this now. Great work!

@johlju johlju merged commit af092da into dsccommunity:dev Feb 20, 2017
@vors vors removed the needs review The pull request needs a code review. label Feb 20, 2017
@luigilink luigilink deleted the Add-TestUnit-SQLMaxDop branch February 26, 2017 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xSQLServerMaxDop: Tests need to be implemented for this resource
5 participants