BREAKING CHANGE: SqlRole: Resource overhaul for tests

[Fiander]

Major overhaul of SQLRole, added tests, and fix for issue #550

Pull Request (PR) description

some remodeling of SqlRole.
lots of extra tests,
fix for #550

This Pull Request (PR) fixes the following issues

Fixes #550

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 

[Fiander]

This is the same as #1623. when i did #1623, i did everything in the local master branch.
This is a rebase of #1623 now in its own branch. everything else is the same.

i left #1623 open for the comments.

[codecov]

Codecov Report

Merging #1631 (2e4a943) into master (235c064) will decrease coverage by 0%.
The diff coverage is 95%.

@@          Coverage Diff           @@
##           master   #1631   +/-   ##
======================================
- Coverage      98%     98%   -1%     
======================================
  Files          37      37           
  Lines        6014    6040   +26     
======================================
+ Hits         5925    5949   +24     
- Misses         89      91    +2     

Flag	Coverage Δ
unit	98% <95%> (-1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1	98% <95%> (-2%)	⬇️

[johlju]

Awesome work on this! Sorry that I am slow to review this one, that is not normal! I'm usually have the reviews and PR merged within a week. The day job is just overwhelming at the moment (large project) and my head is exhausted when I get home. 🙂

Just two small comments and then we merge this one! 🙂

Reviewed 5 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Fiander)

---

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

Removed decision...

Please add 'BREAKING CHANGE:' to this entry. BREAKING CHANGE: Removed decision...

---

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

    )

    if ($Members)

Can we do the evaluation like this to save a few rows?

if ($ServerRoleName -eq 'sysadmin')
{
    if ($Members)
    {
        if ($Members -notcontains 'SA')
        {
            $Members += 'SA'
        }
    }
    else
    {
        if ($MembersToExclude -contains 'SA')
        {
            $MembersToExclude = $MembersToExclude -ne 'SA'
        }
    }
}

[johlju]

@Fiander I keep an eye for commits on this and I will quickly resolve the comments and merge. 🙂 Thank you for sticking out!

[Fiander]

Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @Fiander and @johlju)

---

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

Previously, johlju (Johan Ljunggren) wrote…

Can we do the evaluation like this to save a few rows?

if ($ServerRoleName -eq 'sysadmin')
{
    if ($Members)
    {
        if ($Members -notcontains 'SA')
        {
            $Members += 'SA'
        }
    }
    else
    {
        if ($MembersToExclude -contains 'SA')
        {
            $MembersToExclude = $MembersToExclude -ne 'SA'
        }
    }
}

Done.

[johlju]

Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

Will merge as soon as the tests passes.

[Fiander]

No problem. Now that is sorted out my own Branches, i am not hindert by my own work anymore. take the time you need.
I have two more resources in the pipeline for you. (#1632 and #1633 )

[Fiander]

damm it. crashed again on SQLLogin.

2020-11-21T11:11:23.7970129Z VERBOSE: [WIN-U9C4GIKUSPU]: LCM: [ Start Get ]
2020-11-21T11:11:24.1770576Z [-] Should be able to call Get-DscConfiguration without throwing 429ms
2020-11-21T11:11:24.2982432Z Expected no exception to be thrown, but an exception "Connecting to remote server localhost failed with the following error message : Illegal operation attempted on a registry key that has been marked for deletion. For more information, see the about_Remote_Troubleshooting Help topic." was thrown from D:\a\1\s\tests\Integration\DSC_SqlLogin.Integration.Tests.ps1:99 char:52
2020-11-21T11:11:24.2997509Z + ... urrentConfiguration = Get-DscConfiguration -Verbose -ErrorAction Stop
2020-11-21T11:11:24.3013810Z + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~.
2020-11-21T11:11:24.3030094Z 100: } | Should -Not -Throw
2020-11-21T11:11:24.3047710Z at , D:\a\1\s\tests\Integration\DSC_SqlLogin.Integration.Tests.ps1: line 98
2020-11-21T11:11:24.3455255Z [-] Should have set the resource and all the parameters should match 29ms
2020-11-21T11:11:24.3494829Z Expected 'Present', but got $null.
2020-11-21T11:11:24.3517690Z 109: $resourceCurrentState.Ensure | Should -Be 'Present'
2020-11-21T11:11:24.3553176Z at , D:\a\1\s\tests\Integration\DSC_SqlLogin.Integration.Tests.ps1: line 109

[johlju]

I will rerun test, but can't do it until the unit tests has ran. Waiting for that.

I think the problem is the tests run too fast and the build worker can't keep up sometimes. Something doesn't return to the correct state fast enough. It become better after adding Wait-ForIdleLcm but still see those issues. Might help if we pasue a bit longer after LCM becomes idle. 🤔

SqlServerDsc/tests/TestHelpers/CommonTestHelper.psm1

Lines 395 to 406 in 56ebcfe

	function Wait-ForIdleLcm
	{
	[CmdletBinding()]
	param ()
	while ((Get-DscLocalConfigurationManager).LCMState -ne 'Idle')
	{
	Write-Verbose -Message 'Waiting for the LCM to become idle'
	Start-Sleep -Seconds 2
	}
	}

[johlju]

Maybe we should call Clear-DscLcmConfiguration inside Wait-ForIdleLcm. 🤔 It is normally run at the end of the tests, but those tests that run multiple integration tests might need top have after each test. 🤔
https://github.com/dsccommunity/DscResource.Test/blob/619249435884093da51d9316e07073d573975154/source/Public/Clear-DscLcmConfiguration.ps1

Adding an issue for this.

[Fiander]

When the build fails, it is always on SqlLogin.

To me, in the test this part is scary: row 39-43
<#
This will install dependencies using other resource modules, and is
done without running extra tests on the result.
This is done to speed up testing.
#>

To me this reads as: were adding logins in an other resource module that we are now depended on.
Could it be possible the tests of other modules are now already been cleaned up sometimes and because of that we randomly have these issues?

[johlju]

That configuration creates local users and groups on the build worker which are then used in the tests for SqlLogin. The text mean that we do not run Get-DscConfiguration or Test-DscConfiguration for that configuration. I think that text could have been worded differently. 🙂

[johlju]

It is documented here what is left behind by this test, and if something is removed then the tests should fail with other error messages.
https://github.com/dsccommunity/SqlServerDsc/blob/master/tests/Integration/README.md#sqllogin

The error messages we are seeing I think is the LCM that is having issues.