F/support pester 5 #15

Fiander · 2020-12-27T20:37:11Z

Pull Request (PR) description

@johlju Two fixes for pester 3 dsccommunity#1550

This Pull Request (PR) fixes the following issues

Fixes SqlRole
Fixes SqlTraceFlag

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation updated in the resource's README.md.
Resource parameter descriptions updated in schema.mof.
Comment-based help updated, including parameter descriptions.
Localization strings updated.
Examples updated.
Unit tests updated. See DSC Community Testing Guidelines.
Integration tests updated (where possible). See DSC Community Testing Guidelines.
Code changes adheres to DSC Community Style Guidelines.

This change is

johlju · 2020-12-27T20:46:12Z

Can you try to rebase the PR against my fork. In your local branch do:

git fetch origin f/support-pester-5
git rebase origin/f/support-pester-5

When everything looks okay, make sure your changes (commit) to the tests are still there, run

# This will overwrite the commits in the branch in your fork.
git push my --force

Fiander · 2020-12-27T20:50:44Z

Was already trying to do that, but your example made it a lot simpler.
i sometimes get really confused with all the git therms

johlju

Thank you so much for helping in this effort! 😃

I made some comments on the first test. We should rewrite the test to be optimized for Pester 5 (not just so they work). I think I need to write some guidance around this, I can put together a blog post on dsccommunity.org.

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 9 unresolved discussions (waiting on @Fiander)

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 76 at r2 (raw file):

 -ErrorRecord $_

We should keep this as it will return the actual error as an inner exception.

Also I suggest we don't change the code when we convert to Pester 5, it is a big enough change as it is. I suggest needed changes to code should be changed in the main repo first, then we rebase those changes into the Pester 5 working branch. 🙂

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 200 at r2 (raw file):

                        -f $ServerName, $InstanceName, $ServerRoleName

                    New-InvalidOperationException -Message $errorMessage -ErrorRecord $_

Same as the previous comment.

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 227 at r2 (raw file):

                            -f $ServerName, $InstanceName, $ServerRoleName

                        New-InvalidOperationException -Message $errorMessage -ErrorRecord $_

Same as previous comment.

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 534 at r2 (raw file):

            -f $ServerName, $InstanceName, $ServerRoleName, $SecurityPrincipal

        New-InvalidOperationException -Message $errorMessage -ErrorRecord $_

Same as previous comment.

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 589 at r2 (raw file):

            -f $ServerName, $InstanceName, $ServerRoleName, $SecurityPrincipal

        New-InvalidOperationException -Message $errorMessage -ErrorRecord $_

Same as previous comment.

tests/Unit/DSC_SqlRole.Tests.ps1, line 9 at r2 (raw file):

Quoted 4 lines of code…

if (-not (Test-BuildCategory -Type 'Unit'))
{
    return
}

We can remove this, not needed with the new pipeline.

tests/Unit/DSC_SqlRole.Tests.ps1, line 15 at r2 (raw file):

Quoted 23 lines of code…

$script:dscModuleName = 'SqlServerDsc'
$script:dscResourceName = 'DSC_SqlRole'
function Invoke-TestSetup
{
    try
    {
        Import-Module -Name DscResource.Test -Force -ErrorAction 'Stop'
    }
    catch [System.IO.FileNotFoundException]
    {
        throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -Tasks build" first.'
    }
    $script:testEnvironment = Initialize-TestEnvironment `
        -DSCModuleName $script:dscModuleName `
        -DSCResourceName $script:dscResourceName `
        -ResourceType 'Mof' `
        -TestType 'Unit'
}
function Invoke-TestCleanup
{
    Restore-TestEnvironment -TestEnvironment $script:testEnvironment
}
Invoke-TestSetup

This should be replaced with

SqlServerDsc/tests/Unit/DSC_SqlProtocol.Tests.ps1

Lines 6 to 40 in a7d8e44

    
           BeforeAll { 
        
               $script:dscModuleName = 'SqlServerDsc' 
        
               $script:dscResourceName = 'DSC_SqlProtocol' 
        
               try 
        
               { 
        
                   Import-Module -Name DscResource.Test -Force -ErrorAction 'Stop' 
        
               } 
        
               catch [System.IO.FileNotFoundException] 
        
               { 
        
                   throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -Tasks build" first.' 
        
               } 
        
               $script:testEnvironment = Initialize-TestEnvironment ` 
        
                   -DSCModuleName $script:dscModuleName ` 
        
                   -DSCResourceName $script:dscResourceName ` 
        
                   -ResourceType 'Mof' ` 
        
                   -TestType 'Unit' 
        
               Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath '..\TestHelpers\CommonTestHelper.psm1') 
        
               $PSDefaultParameterValues['InModuleScope:ModuleName'] = $script:dscResourceName 
        
           } 
        
           AfterAll { 
        
               $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') 
        
               Restore-TestEnvironment -TestEnvironment $script:testEnvironment 
        
               # Unload the module being tested so that it doesn't impact any other tests. 
        
               Get-Module -Name $script:dscResourceName -All | Remove-Module -Force 
        
               # Remove module common test helper. 
        
               Get-Module -Name 'CommonTestHelper' -All | Remove-Module -Force 
        
           }

tests/Unit/DSC_SqlRole.Tests.ps1, line 42 at r2 (raw file):

try
{

Try-finally-block should be removed.

tests/Unit/DSC_SqlRole.Tests.ps1, line 44 at r2 (raw file):

InModuleScope $script:dscResourceName {

InModuleScope should only be inside Before*-/After*-blocks and It-blocks. If they are not that code is run during discovery which we should avoid.

See this:

BeforeAll-block that uses InModuleScope to set script variables in the module's scope (that will be used by it-blocks' InModuleScope):

SqlServerDsc/tests/Unit/DSC_SqlProtocol.Tests.ps1

Line 43 in a7d8e44

BeforeAll {
It-block that uses InModuleScope:

SqlServerDsc/tests/Unit/DSC_SqlProtocol.Tests.ps1

Line 126 in a7d8e44

It 'Should return the correct values' {
It-block that uses InModuleScopewith parameters:

SqlServerDsc/tests/Unit/DSC_SqlProtocol.Tests.ps1

Line 169 in a7d8e44

It 'Should return the correct values' {
It-block that assert Should -Invoke (outside InModuleScope):

SqlServerDsc/tests/Unit/DSC_SqlProtocol.Tests.ps1

Line 290 in a7d8e44

It 'Should return $true' {

Fiander

Reviewable status: 1 of 3 files reviewed, 9 unresolved discussions (waiting on @Fiander and @johlju)

source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1, line 76 at r2 (raw file):