Convert xService to HQRM

[PlagueHO]

This PR converts the xService resource to HQRM standards, but also implements full unit and integration tests. These tests exposed various issues which have also been resolved.

I also fixed the PSSA issues.

Fixes

• Fixes xService Unit Tests are not unit tests but are a "destructive" hybrid #214
• Fixes Bug: xService Get-TargetResource throws when Ensure is 'Absent' #213
• Fixes Fix: xService - Service Restart delayed till next LCM execution cycle #212
• Fixes Feature: Allow xService to enable/disable DesktopInteract #210
• Fixes MSFT_xServiceResource: Fix PSSA Issues #136
• Assists with Make xPSDesiredStateConfiguration a High Quality Resource Module #160

Changes to xService

* Added descriptions to MOF file.
* Added additional details to parameters in Readme.md in a format that can be generated from the MOF.
* Added DesktopInteract parameter.
* Added standard help headers to *-TargetResource functions.
* Changed indent/format of all function help headers to be consistent.
* Fixed line length violations.
* Changed localization code so only a single copy of localization strings are required.
* Removed localization strings from inside module file.
* Updated unit tests to use standard test enviroment configuration and header.
* Recreated unit tests to be non-destructive.
* Created integration tests.
* Allowed service to be restarted immediately rather than wait for next LCM run.
* Changed helper function names to valid verb-noun format.
* Removed New-TestService function from MSFT_xServiceResource.TestHelper.psm1 because it should not be used.
* Fixed error calling Get-TargetResource when service does not exist.
* Fixed bug with Get-TargetResource returning StartupType 'Auto' instead of 'Automatic'.
* Converted to HQRM standards.
* Removed obfuscation of exception in Get-Win32ServiceObject function.
* Fixed bug where service start mode would be set to auto when it already was set to auto.
* Fixed error message content when start mode can not be changed.
* Removed shouldprocess from functions as not required.
* Optimized Test-TargetResource and Set-TargetResource by removing repeated calls to Get-Service and Get-CimInstance.

I apologize (to the reviewers 😄 ) for the size of this PR, but a lot of code changes were required to bring this up to HQRM and to implement decent unit test coverage. I expect similar sized PR's for the other resources in this module as they all have similar problems (destructive unit tests and/or no actual unit tests).

Once this PR is merged I'll feel more comfortable addressing the other xService issues (when I get time - unless someone else gets to them first):

• xService not updating service configuration #126
• xService does not add log on as service #76
• xService: errors creating a service are not logged #68
• xService: How to upgrade new versions of a service #65
• Add Recovery on Failure and Autostart with Delay to xService #60

---

This change is

[johlju]

I had to comment and say that you do some awesome work @PlagueHO !!

[johlju]

@PlagueHO Should the file InstallIncorrectWindowsFeatureLog really be included in the PR?

[johlju]

Same question in regards to the file StdErrorPath.txt

[PlagueHO]

Thank you so much @johlju, I really appreciate the compliment 😄

Good catch on the additional files - some of the tests are a little badly behaved and don't clean up after themselves.

I raised an issue to get the InstallIncorrectWindowsFeatureLog and @Dan1el42 has kindly submitted a PR to fix the problem. But I missed the StdErrorPath.txt one.

I'll remove these files from my PR tomorrow and raise another issue to get the tests that create the StdErrorPath.txt fixed so that they don't create the file.

Cheers for the heads up!

[johlju]

Thought I give it a try to review such a large PR. A few comments. So many changes but I could only find style fixes. Impressive. 😄

---

Reviewed 7 of 12 files at r1.
Review status: 7 of 12 files reviewed at latest revision, 30 unresolved discussions.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 44 [r1] (raw file):

    {
        $service = Get-ServiceResource -Name $Name
        $svcWmi = Get-Win32ServiceObject -Name $Name

You change this variable from $win32ServiceObject to $svcWmi, which is less descriptive. Is there a reason for that?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 211 [r1] (raw file):

    $service = Get-ServiceResource -Name $Name
    $svcWmi = Get-Win32ServiceObject -Name $Name

Could we make this variable more descriptive?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 379 [r1] (raw file):

    if ($PSBoundParameters.ContainsKey('StartupType'))
    {
        # Check that there isn't a conflict with the proposed startup type

Maybe add to the comment that the function Test-StartupType will throw on any conflict?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 408 [r1] (raw file):

        $argumentsToNewService.Add("Name", $Name)
        $argumentsToNewService.Add("BinaryPathName", $Path)
        if($PSBoundParameters.ContainsKey("Credential"))

Could we add a blank line before this if-statement? And also fo the rest of the if-blocks below.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 438 [r1] (raw file):

            Write-Verbose -Message ($LocalizedData.TestStartupTypeMismatch `
                -f $argumentsToNewService["Name"], $_.Exception.Message)
            throw $_

Is $_ needed here? In another place in the code only throw is used to re-throw the error.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 581 [r1] (raw file):

        return 'Auto'
    }  # if
    return $StartupType

Could we add a blank line before this return statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 600 [r1] (raw file):

        $StartMode
    )
    if ($StartMode -eq 'Auto')

Could we add a blank line before this If statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 604 [r1] (raw file):

        return "Automatic"
    } # if
    return $StartMode

Could we add a blank line before this return statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 687 [r1] (raw file):

    param
    (
        [parameter(Mandatory = $true)]

Capital 'P' here

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 712 [r1] (raw file):

    )

    $svcWmi = Get-Win32ServiceObject -Name $Name

Maybe make this local variable more descriptive?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 773 [r1] (raw file):

        [parameter(Mandatory = $true)]
        [ValidateNotNull()]
        $SvcWmi,

Can this parameter be more descriptive?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 800 [r1] (raw file):

    # Get the Username and Password to change to (if applicable)
    $getUserNameAndPasswordArgs = @{}
    if($PSBoundParameters.ContainsKey("BuiltInAccount"))

Can we add a blank line before this If-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 804 [r1] (raw file):

        $null = $getUserNameAndPasswordArgs.Add("BuiltInAccount",$BuiltInAccount)
    } # if
    if($PSBoundParameters.ContainsKey("Credential"))

Can we add a blank line before this If-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 808 [r1] (raw file):

        $null = $getUserNameAndPasswordArgs.Add("Credential",$Credential)
    } # if
    if($getUserNameAndPasswordArgs.Count -gt 1)

Can we add a blank line before this If-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 816 [r1] (raw file):

                -f "Credential","BuiltInAccount")
    } # if
    $userName,$password = Get-UserNameAndPassword @getUserNameAndPasswordArgs

Could we add a blank line before this line?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 847 [r1] (raw file):

            -MethodName Change `
            -Arguments $changeArgs
        if($ret.ReturnValue -ne 0)

Can we add a blank line before this If-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 872 [r1] (raw file):

        [parameter(Mandatory = $true)]
        [ValidateNotNull()]
        $SvcWmi,

Could we make this parameter more descriptive?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 907 [r1] (raw file):

    param
    (
        $SvcWmi,

Could we make this parameter more descriptive?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 918 [r1] (raw file):

<#
    .SYNOPSIS
    Retrieves username and password out of the BuiltInAccount and Credential parameters

Can you also describe here in what order user and password are returned?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 960 [r1] (raw file):

    The name of the service to delete.
#>
function Remove-Service

This function uses a hardcoded value to sleep. Maybe submit an issue so this can be a parameter in the feature (and maybe use something like Start-Job | Wait-Job instead)? Some servers might take longer than others to remove the service.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 971 [r1] (raw file):

    & "sc.exe" "delete" "$Name"
    for($i = 1; $i -lt 1000; $i++)

Could we add a blank line before this for-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 978 [r1] (raw file):

            break
        } # if
        # try again after 2 millisecs if the service is not deleted.

Could we add a blank line before this comment?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 982 [r1] (raw file):

        Start-Sleep -Seconds .002
    } # for
    if (-not $serviceDeletedSuccessfully)

Could we add a blank line before this if-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1124 [r1] (raw file):

        $UserName
    )
    switch ($Username)

Could we add a blank line before the switch-statement?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1146 [r1] (raw file):

        } # default
    } # switch
    return $UserName

Could we add a blank line before the return-statement?

---

DSCResources/MSFT_xServiceResource/en-US/MSFT_xServiceResource.strings.psd1, line 45 [r1] (raw file):

TryDeleteAgain = Wait for 2 milliseconds for a service to get deleted.
WritePropertiesIgnored = Service '{0}' already exists. Write properties such as Status, DisplayName, Description, Dependencies will be ignored for existing services.
ErrorCreatingService = Error creating service "{0}"; Exception Message: "{1}"

Using " in this string instead on ' which is used in the other strings.

---

Tests/Integration/MSFT_xServiceResource_add.config.ps1, line 10 [r1] (raw file):

        $ServiceDependsOn
    )
    Import-DscResource -ModuleName xPSDesiredStateConfiguration

Could we add a blank space before this row? To separate it from the param block.

---

Tests/Integration/MSFT_xServiceResource_add.config.ps1, line 11 [r1] (raw file):

    )
    Import-DscResource -ModuleName xPSDesiredStateConfiguration
    node localhost {

Could we add a blank space before this row? To separate it from the Import-module "block".

---

Tests/Integration/MSFT_xServiceResource_remove.config.ps1, line 6 [r1] (raw file):

        $ServiceName
    )
    Import-DscResource -ModuleName xPSDesiredStateConfiguration

Could we add a blank space before this row? To separate it from the param block.

---

Tests/Integration/MSFT_xServiceResource_remove.config.ps1, line 7 [r1] (raw file):

    )
    Import-DscResource -ModuleName xPSDesiredStateConfiguration
    node localhost {

Could we add a blank space before this row? To separate it from the Import-module "block".

---

Comments from Reviewable

[Dan1el42]

@johlju and @PlagueHO I've submitted PR #218 to deal with the StdErrorPath.txt problem.

[PlagueHO]

@johlju @Dan1el42 - I've removed both these files and added the change to the Readme.md. We still need to fix up the tests so that StdErrorPath.txt doesn't get created. I'll log an issue to get this one solved.

Thank you heaps for the reivew @johlju - really appreciate it (especially as I know how big the change is 😄 )

---

Review status: 7 of 12 files reviewed at latest revision, 30 unresolved discussions.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 44 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You change this variable from $win32ServiceObject to $svcWmi, which is less descriptive. Is there a reason for that?

I changed this for consistency - most of the functions used `$svcWmi` - plus the helper cmdlets use this as the parameter name. I went with `$svcWmi` to bring this all in-line. I'd rather avoid refactoring the parameter names because that means refactoring the tests as well (doable but if I can avoid it I'd like to).

I'll go with whatever the consensus is, but I'm not sure this would really make things that much clearer 😄

If you do reckon changing the variable and parameter name to something else could we maybe change it to something reasonably short? Otherwise it actually pushes lots of the lines over the 100 char limit so will require a bit more messing about. So short but clear would be idea here - any ideas?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 211 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make this variable more descriptive?

See above.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 379 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe add to the comment that the function Test-StartupType will throw on any conflict?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 408 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this if-statement? And also fo the rest of the if-blocks below.

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 438 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Is $_ needed here? In another place in the code only throw is used to re-throw the error.

It isn't required, but I personally think that issuing "Throw $_" is clearer - so I try to always go with that. So I've corrected the other Throw instead 😄 But I can change them both to "Throw" if you think that is the way to go.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 581 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this return statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 600 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this If statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 604 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this return statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 687 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Capital 'P' here

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 712 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe make this local variable more descriptive?

See above.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 773 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can this parameter be more descriptive?

See above.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 800 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add a blank line before this If-statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 804 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add a blank line before this If-statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 808 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add a blank line before this If-statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 816 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this line?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 847 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add a blank line before this If-statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 872 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make this parameter more descriptive?

See above.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 907 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we make this parameter more descriptive?

See above.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 918 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you also describe here in what order user and password are returned?

Done. I added an .OUTPUT section with order of the tuple returned.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 960 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This function uses a hardcoded value to sleep. Maybe submit an issue so this can be a parameter in the feature (and maybe use something like Start-Job | Wait-Job instead)? Some servers might take longer than others to remove the service.

I wasn't too happy with this code myself, but as the original had zero documentation I wasn't too comfortable refactoring it too much. But I've gone and refactored it and used better timeout code (from other resource modules I've created). It also has a timeout parameter that defaults to 5 seconds.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 971 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this for-statement?

The for loop no longer exists.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 978 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this comment?

This function has been mostly rewritten now.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 982 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before this if-statement?

This function has been mostly rewritten now.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1124 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before the switch-statement?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1146 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank line before the return-statement?

Done.

---

DSCResources/MSFT_xServiceResource/en-US/MSFT_xServiceResource.strings.psd1, line 45 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Using " in this string instead on ' which is used in the other strings.

Done.

---

Tests/Integration/MSFT_xServiceResource_add.config.ps1, line 10 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank space before this row? To separate it from the param block.

Done.

---

Tests/Integration/MSFT_xServiceResource_add.config.ps1, line 11 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank space before this row? To separate it from the Import-module "block".

Done.

---

Tests/Integration/MSFT_xServiceResource_remove.config.ps1, line 6 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank space before this row? To separate it from the param block.

Done.

---

Tests/Integration/MSFT_xServiceResource_remove.config.ps1, line 7 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a blank space before this row? To separate it from the Import-module "block".

Done.

---

Comments from Reviewable

[Dan1el42]

@PlagueHO The StdErrorPath.txt problem is history :-). The PR #218 I've submitted yesterday has already been merged into dev.

[johlju]

So far I have only gone thru checking style in the test MSFT_xServiceResource.Tests.ps1. Logical review should be made for this too (and also find the last style issues which I probably missed). That is one BIG test 😄

---

Reviewed 1 of 12 files at r1, 12 of 12 files at r2.
Review status: all files reviewed at latest revision, 145 unresolved discussions.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 44 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I changed this for consistency - most of the functions used $svcWmi - plus the helper cmdlets use this as the parameter name. I went with $svcWmi to bring this all in-line. I'd rather avoid refactoring the parameter names because that means refactoring the tests as well (doable but if I can avoid it I'd like to).

I'll go with whatever the consensus is, but I'm not sure this would really make things that much clearer 😄

If you do reckon changing the variable and parameter name to something else could we maybe change it to something reasonably short? Otherwise it actually pushes lots of the lines over the 100 char limit so will require a bit more messing about. So short but clear would be idea here - any ideas?

I understand what you mean. But I thought, since this is a HQRM (well on it's way at least) I think we should not use abbreviations in variable names and parameter names? If we do then we set that as a standard and allow other resources/modules to do that. In the style guide there is a reference to a "Bad choice" to use abbreviation on parameter name, see `$MySTU`. I think that would extend to variables too if you look at the text at https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#descriptive-names

So even if changing this is a lot of work, I think it worth it to adhere to the guideline for a HQRM resource/module.

I think changing the variable name to $serviceWmi and parameter name to$ServiceWmi would at least be a better. But I'm open to anything other than an abbreviated variable name :)

Changing the parameter name will make this a breaking change, so you should add BREAKING CHANGE: to the PR title and release notes according to https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#breaking-changes
(maybe more stuff is a breaking change in this PR, than the same for those notes in the release notes 😄 )

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 408 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 438 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

It isn't required, but I personally think that issuing "Throw $_" is clearer - so I try to always go with that. So I've corrected the other Throw instead 😄 But I can change them both to "Throw" if you think that is the way to go.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 581 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 600 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 604 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 687 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

There are other `[parameter` that needs to change to `[Parameter` with capital 'P' in the rest of the code as well, can you fix them too? At least one more that I saw (faster for you to search ;)

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 800 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 804 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 808 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 816 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 847 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 918 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done. I added an .OUTPUT section with order of the tuple returned.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 960 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I wasn't too happy with this code myself, but as the original had zero documentation I wasn't too comfortable refactoring it too much. But I've gone and refactored it and used better timeout code (from other resource modules I've created). It also has a timeout parameter that defaults to 5 seconds.

You made it better. Further improvments can be made later in a separate issue if there is an timing problem.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1124 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1146 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1 [r2] (raw file):

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseShouldProcessForStateChangingFunctions', '')]

Add a comment why the test is suppressed?

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 391 [r2] (raw file):

        # The service exists but needs to be deleted
        Stop-ServiceResource -Name $Name -TerminateTimeout $TerminateTimeout
        Remove-Service $Name

Since you refactored Remove-Service it would be a good idea to add the Timeout parameter here with the same value as the default value for the Remove-Service so it clear that it's a hard coded value for timeout (which can be improved later).
Or if the $TerminateTimeout could be used here as well?

---

DSCResources/MSFT_xServiceResource/en-US/MSFT_xServiceResource.strings.psd1, line 45 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 50 [r1] (raw file):

    Describe "$($script:DSCResourceName)_Add_Integration" {
        #region DEFAULT TESTS
        It 'Should compile without throwing' {

This do not just compile, it also pushes the configuration. Change the text to explain that?

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 96 [r1] (raw file):

    Describe "$($script:DSCResourceName)_Remove_Integration" {
        #region DEFAULT TESTS
        It 'Should compile without throwing' {

Same there, also pushes the configuration.
And here you also uses $null = ... which is not used above? Remove here or add above? (or neither?)

---

Tests/Integration/MSFT_xServiceResource_add.config.ps1, line 10 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

Tests/Integration/MSFT_xServiceResource_add.config.ps1, line 11 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

Tests/Integration/MSFT_xServiceResource_remove.config.ps1, line 6 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

Tests/Integration/MSFT_xServiceResource_remove.config.ps1, line 7 [r1] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

LGTM

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1 [r2] (raw file):

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingConvertToSecureStringWithPlainText', '')]

Add a comment here why this is suppressed

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 53 [r2] (raw file):

            ServicesDependedOn = $script:testServiceDependsOnHash
        }
        Add-Member -InputObject  $script:testServiceMockRunning `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 56 [r2] (raw file):

            -MemberType ScriptMethod `
            -Name Stop -Value { $global:ServiceStopped = $True }
        Add-Member -InputObject  $script:testServiceMockRunning `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 58 [r2] (raw file):

        Add-Member -InputObject  $script:testServiceMockRunning `
            -MemberType ScriptMethod `
            -Name WaitForStatus -Value { param($Status,$WaitTimeSpan) }

Question: is this allowed in Tests? Having a braces and the block like this? I have no objections, since it's a mock. But in question is in regards to HQRM standard. If there is nothing written about this. Leave as-is.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 68 [r2] (raw file):

            ServicesDependedOn = $script:testServiceDependsOnHash
        }
        Add-Member -InputObject  $script:testServiceMockStopped `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 71 [r2] (raw file):

            -MemberType ScriptMethod `
            -Name Start -Value { $global:ServiceStarted = $True }
        Add-Member -InputObject  $script:testServiceMockStopped `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 87 [r2] (raw file):

            State                   = $script:testServiceStatusRunning
        }
        Add-Member -InputObject  $script:testWin32ServiceMockRunningLocalSystem `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 138 [r2] (raw file):

                    -MockWith { $true } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 142 [r2] (raw file):

                    -MockWith { $script:testServiceMockRunning } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 208 [r2] (raw file):

                -MockWith { $true } `
                -Verifiable
            Mock `

Add a blank line here, and also one before for all other mocks below

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 234 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 237 [r2] (raw file):

                    $script:result | Should Be $True
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 266 [r2] (raw file):

                }
                It 'Should return false' {
                    $script:result | Should Be $False

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 268 [r2] (raw file):

                    $script:result | Should Be $False
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 292 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return false' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 295 [r2] (raw file):

                    $script:result | Should Be $False
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 319 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return false' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 322 [r2] (raw file):

                    $script:result | Should Be $False
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 350 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return false' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 353 [r2] (raw file):

                    $script:result | Should Be $False
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 366 [r2] (raw file):

            Context 'Service exists and should not' {
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 381 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return false' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 384 [r2] (raw file):

                    $script:result | Should Be $False
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 402 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 414 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 429 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 447 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 462 [r2] (raw file):

                    -CommandName Start-ServiceResource `
                    -Verifiable
                # Mocks that should not be called

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 463 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 475 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 493 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 508 [r2] (raw file):

                    -CommandName Start-ServiceResource `
                    -Verifiable
                # Mocks that should not be called

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 509 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 540 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 558 [r2] (raw file):

                    -CommandName Stop-ServiceResource `
                    -Verifiable
                # Mocks that should not be called

A blank line before here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 559 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 569 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Blank line before here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 587 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 603 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 616 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

A blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 634 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 650 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 662 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

A blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 680 [r2] (raw file):

                    -CommandName Test-StartupType `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 685 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 708 [r2] (raw file):

                        -Verbose } | Should Throw $errorRecord
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 778 [r2] (raw file):

                }
            }
            Context "StartupType is 'Disabled'" {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 791 [r2] (raw file):

                }
            }
            Context "StartupType is 'Disabled'" {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 943 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 962 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 965 [r2] (raw file):

                    $script:Result | Should Be $false
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 984 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 987 [r2] (raw file):

                    $script:Result | Should Be $false
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1006 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1028 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1031 [r2] (raw file):

                    $script:Result | Should Be $false
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1045 [r2] (raw file):

            Context 'No parameters to be changed passed' {
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1059 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1076 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1089 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1104 [r2] (raw file):

                    -MockWith { $null,$null } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1110 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1121 [r2] (raw file):

                        -Verbose } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1136 [r2] (raw file):

                    -MockWith { $null,$null } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1142 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1161 [r2] (raw file):

                        -Verbose } | Should Throw $errorRecord
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1172 [r2] (raw file):

            Context 'Both credential and buildinaccount passed' {
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1192 [r2] (raw file):

                        -BuiltInAccount 'LocalSystem' } | Should Throw $errorRecord
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1207 [r2] (raw file):

                    -MockWith { $script:testUsername,$script:testPassword } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1213 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1223 [r2] (raw file):

                        -Credential $script:testCredential } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1238 [r2] (raw file):

                    -MockWith { $script:testUsername,$script:testPassword } `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1255 [r2] (raw file):

                        -Credential $script:testCredential } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1270 [r2] (raw file):

                    -MockWith { $script:testUsername,$script:testPassword } `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1295 [r2] (raw file):

                        -Credential $script:testCredential } | Should Throw $errorRecord
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1310 [r2] (raw file):

                    -MockWith { '.\LocalSystem','' } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1316 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1326 [r2] (raw file):

                        -BuiltInAccount 'LocalSystem' } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1341 [r2] (raw file):

                    -MockWith { '.\LocalSystem',$null } `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1351 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1359 [r2] (raw file):

                        -BuiltInAccount 'LocalSystem' } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1374 [r2] (raw file):

                    -MockWith { '.\LocalSystem',$null } `
                    -Verifiable
                Mock `

Could you add a blank line before each mock here?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1384 [r2] (raw file):

                # Mocks that should not be called
                Mock `

Can you remove the ` and make all mock on one row?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1400 [r2] (raw file):

                        -BuiltInAccount 'LocalSystem' } | Should Throw $errorRecord
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1417 [r2] (raw file):

                        -Path $script:testServiceExecutablePath } | Should Not Throw
                }
                It 'Should return false' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1428 [r2] (raw file):

                        -Path 'c:\NewServicePath.exe' } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1431 [r2] (raw file):

                    $script:result = $true
                }
                It 'Change method was called' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1435 [r2] (raw file):

                }
            }
            Context 'Path needs to be changed but an error occurs changing it' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1451 [r2] (raw file):

                        -Path 'c:\NewServicePath.exe' } | Should Throw $errorRecord
                }
                It 'Change method was called' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1464 [r2] (raw file):

                        -Username $script:testUsername } | Should Not Throw
                }
                It 'Should return true' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1468 [r2] (raw file):

                }
            }
            Context 'Username does not match' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1474 [r2] (raw file):

                        -Username 'mismatch' } | Should Not Throw
                }
                It 'Should return false' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1489 [r2] (raw file):

                }
            }
            Context 'Credential provided' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1497 [r2] (raw file):

                }
            }
            Context 'Neither built-in account or credential provided' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1555 [r2] (raw file):

                    -MockWith { $script:testServiceMockRunning } `
                    -Verifiable
                # Mocks that should not be called

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1556 [r2] (raw file):

                    -Verifiable
                # Mocks that should not be called
                Mock `

Can you remove the ` and make all of these mocks on one row each?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1558 [r2] (raw file):

                Mock `
                    -CommandName New-Object
                It 'Should not throw exception' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1561 [r2] (raw file):

                    { Start-ServiceResource -Name $script:testServiceName -StartUpTimeout 30000 } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1574 [r2] (raw file):

                    -MockWith { $script:testServiceMockStopped } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1577 [r2] (raw file):

                    -CommandName New-Object `
                    -Verifiable
                $global:ServiceStarted = $false

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1578 [r2] (raw file):

                    -Verifiable
                $global:ServiceStarted = $false
                It 'Should not throw exception' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1581 [r2] (raw file):

                    { Start-ServiceResource -Name $script:testServiceName -StartUpTimeout 30000 } | Should Not Throw
                }
                It 'Called start method' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1584 [r2] (raw file):

                    $global:ServiceStarted | Should Be $true
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1589 [r2] (raw file):

                    Assert-MockCalled -CommandName New-Object -Exactly 1
                }
                Remove-Variable -Name ServiceStarted -Scope Global

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1600 [r2] (raw file):

                    -MockWith { $script:testServiceMockStopped } `
                    -Verifiable
                # Mocks that should not be called

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1603 [r2] (raw file):

                Mock `
                    -CommandName New-Object
                It 'Should not throw exception' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1606 [r2] (raw file):

                    { Stop-ServiceResource -Name $script:testServiceName -TerminateTimeout 30000 } | Should Not Throw
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1619 [r2] (raw file):

                    -MockWith { $script:testServiceMockRunning } `
                    -Verifiable
                Mock `

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1622 [r2] (raw file):

                    -CommandName New-Object `
                    -Verifiable
                $global:ServiceStopped = $false

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1626 [r2] (raw file):

                    { Stop-ServiceResource -Name $script:testServiceName -TerminateTimeout 30000 } | Should Not Throw
                }
                It 'Called stop method' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1629 [r2] (raw file):

                    $global:ServiceStopped | Should Be $true
                }
                It 'Should call expected Mocks' {

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1634 [r2] (raw file):

                    Assert-MockCalled -CommandName New-Object -Exactly 1
                }
                Remove-Variable -Name ServiceStopped -Scope Global

Add a blank line here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1644 [r2] (raw file):

                }
            }
            Context "Username is 'LocalService'" {

Add a blank line here, and for all other context blocks below

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1722 [r2] (raw file):

                }

One blank row to much here

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1869 [r2] (raw file):

        Describe "$DSCResourceName\Set-LogOnAsServicePolicy" {
            # Need a a non-destructive method of testing this function

Should we add the the comment to an It block so it shows that this is not implemented yet. But we are not allowed to have "TODO"'s in a HQRM. Do should this be removed? I don't know... Comment this entire block?
At least it should be an issue for this, that this is not done?

---

Comments from Reviewable

[PlagueHO]

I did try to keep the logic to the original resource (in all it's terrible glory). I've fixed up any obvious issues (there were several). I'd love someone to do a logic review on it, but I think a far better plan would be to extend the Integration tests further as that is likely to catch more problems than a logic review would catch. I'm not overly fond of the way this resource was written, but there is no way we should even think of refactoring it until the test coverage is in place. This includes fixing issues (which is why I've put so much time into getting tests in place).

Anyway, thanks again for the very thorough review 😄

---

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

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 44 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I understand what you mean. But I thought, since this is a HQRM (well on it's way at least) I think we should not use abbreviations in variable names and parameter names? If we do then we set that as a standard and allow other resources/modules to do that.
In the style guide there is a reference to a "Bad choice" to use abbreviation on parameter name, see $MySTU. I think that would extend to variables too if you look at the text at https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#descriptive-names

So even if changing this is a lot of work, I think it worth it to adhere to the guideline for a HQRM resource/module.

I think changing the variable name to $serviceWmi and parameter name to$ServiceWmi would at least be a better. But I'm open to anything other than an abbreviated variable name :)

Changing the parameter name will make this a breaking change, so you should add BREAKING CHANGE: to the PR title and release notes according to https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#breaking-changes
(maybe more stuff is a breaking change in this PR, than the same for those notes in the release notes 😄 )

Fair enough 😄 I'm Ok with `$serviceWmi`. Changed.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 687 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

There are other [parameter that needs to change to [Parameter with capital 'P' in the rest of the code as well, can you fix them too? At least one more that I saw (faster for you to search ;)

Doh! I should have used Search/Replace. There were 5 more. All fixes now.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 1 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a comment why the test is suppressed?

Done.

---

DSCResources/MSFT_xServiceResource/MSFT_xServiceResource.psm1, line 391 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Since you refactored Remove-Service it would be a good idea to add the Timeout parameter here with the same value as the default value for the Remove-Service so it clear that it's a hard coded value for timeout (which can be improved later).
Or if the $TerminateTimeout could be used here as well?

I've used terminatetimeout here.

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 50 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This do not just compile, it also pushes the configuration. Change the text to explain that?

Good point. This is the text that is used in the template, so I've raised an issue to get that changed in the DSCResources Repo (I'll raise a PR as well).

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 96 [r1] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same there, also pushes the configuration.
And here you also uses $null = ... which is not used above? Remove here or add above? (or neither?)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a comment here why this is suppressed

Done. I actually just added a link back to the https://github.com/PowerShell/DscResources/blob/master/PSSARuleSeverities.md guidelines which explains why it is suppressed 😄

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 53 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 56 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 58 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Question: is this allowed in Tests? Having a braces and the block like this? I have no objections, since it's a mock. But in question is in regards to HQRM standard. If there is nothing written about this. Leave as-is.

The HQRM guidelines don't mention style directly. However, the style guidelines do recommend a specific style for parameter blocks in functions/methods. I tried spacing them out as per the styleguidelines (one param per line with a blank line in between), but it really made the code hard to read. So I settled for a partial solution. @kwirkykat - what do you reckon about mock methods declared in Unit tests? Should they strictly adhere to style guidelines?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 68 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 71 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 87 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 138 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 142 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 208 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here, and also one before for all other mocks below

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 234 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 237 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 266 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 268 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 292 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 295 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 319 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 322 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 350 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 353 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 366 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 381 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 384 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 402 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 414 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 429 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 447 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 462 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 463 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 475 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 493 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 508 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 509 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 540 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 558 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A blank line before here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 559 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 569 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Blank line before here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 587 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 603 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 616 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 634 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 650 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 662 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 680 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 685 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 708 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 778 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 791 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 943 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 962 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 965 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 984 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 987 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1006 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1028 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1031 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1045 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1059 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1076 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1089 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1104 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1110 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1121 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1136 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1142 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1161 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1172 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1192 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1207 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1213 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1223 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1238 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1255 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1270 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1295 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1310 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1316 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1326 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1341 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1351 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1359 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1374 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you add a blank line before each mock here?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1384 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all mock on one row?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1400 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1417 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1428 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1431 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1435 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1451 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1464 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1468 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1474 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1489 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1497 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1555 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1556 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can you remove the ` and make all of these mocks on one row each?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1558 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1561 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1574 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1577 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1578 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1581 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1584 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1589 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1600 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1603 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1606 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1619 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1622 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1626 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1629 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1634 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1644 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here, and for all other context blocks below

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1722 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

One blank row to much here

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1869 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should we add the the comment to an It block so it shows that this is not implemented yet. But we are not allowed to have "TODO"'s in a HQRM. Do should this be removed? I don't know... Comment this entire block?
At least it should be an issue for this, that this is not done?

The correct thing to do is to remove the whole block. The integration tests still cover it, so I don't think it exposes any additional risk. So I've removed it. HQRM doesn't say 100% code coverage is required for HQRM.

---

Comments from Reviewable

[johlju]

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

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 59 [r3] (raw file):

Start-DscConfiguration
This does not use $null = ... which is used at the location in the next comment? (see next comment)

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 101 [r3] (raw file):

$null = Start-DscConfiguration
This uses $null = ... which is not used in the previous comment? Remove here or add above? (or neither?)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1136 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

You missed this one :) But a few misses in ~140 comments is pretty good score ;)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1207 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one too. :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1238 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed all of these too. :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1310 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1341 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one too :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1374 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one too :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1574 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1589 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1619 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1634 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Missed this one :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 415 [r3] (raw file):

                Mock -CommandName Get-ServiceResource

                Mock -CommandName Get-Win32ServiceObject

You can remove the blank lines between the 4 mocks on and above this row. You have not separated them in any other place in the code of what I saw. Doesn't feel necessary to separate them with blank lines either, I think it's clearly readably putting these together. Do you agree?

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1170 [r3] (raw file):

                    -MockWith { $null,$null } `
                    -Verifiable
                Mock `

Add a blank line here

---

Comments from Reviewable

[PlagueHO]

Hopefully I've got all the changes now (I said that last time though :) ). Thanks again @johlju for reviewing

---

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

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 59 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Start-DscConfiguration
This does not use $null = ... which is used at the location in the next comment? (see next comment)

Good catch. The template doesn't include the $null so it should be removed.

---

Tests/Integration/MSFT_xServiceResource.Integration.Tests.ps1, line 101 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$null = Start-DscConfiguration
This uses $null = ... which is not used in the previous comment? Remove here or add above? (or neither?)

I've removed this $null because the template doesn't include it.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1136 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You missed this one :) But a few misses in ~140 comments is pretty good score ;)

Doh! Sorry about that. I missed a few more even than the ones you picked up. I think I've got them all now :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1207 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one too. :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1238 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed all of these too. :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1310 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1341 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one too :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1374 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one too :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1574 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1589 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1619 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1634 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed this one :)

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 415 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You can remove the blank lines between the 4 mocks on and above this row. You have not separated them in any other place in the code of what I saw. Doesn't feel necessary to separate them with blank lines either, I think it's clearly readably putting these together. Do you agree?

Done.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1170 [r3] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add a blank line here

Done.

---

Comments from Reviewable

[johlju]

Almost... two left :)

---

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

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1589 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Still here :)

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1634 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Done.

Still here :)

---

Comments from Reviewable

[PlagueHO]

Hopefully this time :) Committing now.

---

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

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1589 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Still here :)

Sorry for missing these two! Good catch. Fixed now.

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 1634 [r2] (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Still here :)

Done.

---

Comments from Reviewable

[johlju]

I think this can be as good as it can be. Left one comment for @kwirkykat to reply to.
@kwirkykat could you glance this over and if it looks good then merge.

---

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

---

Comments from Reviewable

[kwirkykat]

@PlagueHO This is awesome.

---

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

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 58 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

The HQRM guidelines don't mention style directly. However, the style guidelines do recommend a specific style for parameter blocks in functions/methods. I tried spacing them out as per the styleguidelines (one param per line with a blank line in between), but it really made the code hard to read. So I settled for a partial solution. @kwirkykat - what do you reckon about mock methods declared in Unit tests? Should they strictly adhere to style guidelines?

Mock methods are usually short enough that style doesn't matter. This one is short and sweet so I don't really care. If it was longer or more complicated, I would probably ask that it match the style guidelines.

The HQRM guidelines actually do mention the style guidelines at the very bottom, but in my opinion they are more relevant to the resource .psm1 files than the tests and examples.

---

Comments from Reviewable

[PlagueHO]

Thanks @kwirkykat - I really appreciate that 😄

---

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

---

Tests/Unit/MSFT_xServiceResource.Tests.ps1, line 58 [r2] (raw file):

Previously, kwirkykat (Katie Keim) wrote…

Mock methods are usually short enough that style doesn't matter.
This one is short and sweet so I don't really care.
If it was longer or more complicated, I would probably ask that it match the style guidelines.

The HQRM guidelines actually do mention the style guidelines at the very bottom, but in my opinion they are more relevant to the resource .psm1 files than the tests and examples.

Cool - I'll keep that in mind. I've had to add a few more extensive mocks in xStorage so I've stuck to the style guidelines there. I though HQRM did state that all style guidelines were met - I just meant it didn't implement any additional ones. 😄

---

Comments from Reviewable