xCluster: Refactored unit test

[johlju]

Pull Request (PR) description

• Changes to xCluster
  • Added new stubs for FailoverClusters module
    (Tests\Unit\Stubs\FailoverClusters.stubs.psm1) to be able to run unit tests
    on a computer that does not have or can install Failover Clustering
    PowerShell module.
  • Added a script file (Tests\Unit\Stubs\Write-ModuleStubFile.ps1) to be able
    to rebuild the stub file (FailoverClusters.stubs.psm1) whenever needed.
  • Added settings file for VS Code extension markdownlint (.markdownlint.json).
    This will help keeping the line-length (MD013) in markdown file for contributors
    using VS Code.
• Changes to xCluster
  • Refactored the unit test for this resource to use stubs and increase coverage
    (issue xCluster: Unit tests does not use the correct template and folder structure is wrong #73).
  • Removed the password file (MSFT_xCluster.password.txt) which seemed unnecessary.
  • Test-TargetResource now throws and error if domain name cannot be evaluated
    (issue xCluster: Test-TargetResource never throws an error if domain name can't be evaluated #72).
  • Set-TargetResource now correctly throws and error if domain name cannot be
    evaluated (issue xCluster: Set-TargetResource never throws an error if domain name can't be evaluated #71).

This Pull Request (PR) fixes the following issues:
Fixes #73
Fixes #72
Fixes #71

Task list:

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

---

This change is 

[msftclas]

@johlju,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

[codecov-io]

Codecov Report

Merging #82 into dev will increase coverage by 7%.
The diff coverage is 96%.

@@        Coverage Diff        @@
##           dev   #82   +/-   ##
=================================
+ Coverage   57%   64%   +7%     
=================================
  Files        6     6           
  Lines      293   292    -1     
=================================
+ Hits       168   188   +20     
+ Misses     125   104   -21

[johlju]

@bgelens or @RichardSiddaway Whenever you have a chance, could you review this one? 😄

[johlju]

@bgelens if possible, could you fit this one into your schedule? Back at it now, can fix any review comments you find! 😄

[bgelens]

Excuse all the nitpicking 😉 I've maybe overdone it for this PR a bit but I thought it would be best to map all the obvious shortcomings straight away

---

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 49 unresolved discussions, some commit checks failed.

---

.markdownlint.json, line 2 at r1 (raw file):

{
    "MD013": true

Not sure about this... E.g. I don't use this extension. I'd like your opinion @PlagueHO should we / shouldn't we do stuff like this besides the formatting rules for the ps ext?

---

CHANGELOG.md, line 60 at r1 (raw file):

  - Fixed typo in xCluster parameter description.
  - Added links to examples from README.md
  - Refactored the unit test for this resource to use stubs and increase coverage

Is this the way to go instead of creating stubs in the tests themselves?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 7 at r1 (raw file):

#
# The Get-TargetResource cmdlet.

Should be comment based help with synopsis and parameters

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 14 at r1 (raw file):

    param
    (
        [parameter(Mandatory)]

Mandatory = $true (I'm not a fan of this one, but it's what needs to be done)

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 15 at r1 (raw file):

    (
        [parameter(Mandatory)]
        [string] $Name,

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 17 at r1 (raw file):

        [string] $Name,

        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 18 at r1 (raw file):

        [parameter(Mandatory)]
        [string] $StaticIPAddress,

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 20 at r1 (raw file):

        [string] $StaticIPAddress,

        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 21 at r1 (raw file):

        [parameter(Mandatory)]
        [PSCredential] $DomainAdministratorCredential

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 24 at r1 (raw file):

    )

    $ComputerInfo = Get-WmiObject Win32_ComputerSystem

positional bindindg

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 25 at r1 (raw file):

    $ComputerInfo = Get-WmiObject Win32_ComputerSystem
    if (($ComputerInfo -eq $null) -or ($ComputerInfo.Domain -eq $null))

$null should be on the left of the evaluation

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 27 at r1 (raw file):

    if (($ComputerInfo -eq $null) -or ($ComputerInfo.Domain -eq $null))
    {
        throw "Can't find machine's domain name"

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 36 at r1 (raw file):

        if ($null -eq $cluster)
        {
            throw "Can't find the cluster $Name"

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 39 at r1 (raw file):

        }

        $address = Get-ClusterGroup -Cluster $Name -Name "Cluster IP Address" | Get-ClusterParameter "Address"

Positional binding for Get-ClusterParameter. Should include the parameter instead

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 51 at r1 (raw file):

    }

    return  @{

The use of the return keyword serves no purpose here and can be removed.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 53 at r1 (raw file):

    return  @{
        Name            = $Name
        StaticIPAddress = $address.Value

Missing the DomainAdministratorCredential key.
The output should match what is defined in the schema

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 58 at r1 (raw file):

#
# The Set-TargetResource cmdlet.

Should be comment based help with synopsis and parameters

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 64 at r1 (raw file):

    param
    (
        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 65 at r1 (raw file):

    (
        [parameter(Mandatory)]
        [string] $Name,

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 67 at r1 (raw file):

        [string] $Name,

        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 68 at r1 (raw file):

        [parameter(Mandatory)]
        [string] $StaticIPAddress,

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 70 at r1 (raw file):

        [string] $StaticIPAddress,

        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 71 at r1 (raw file):

        [parameter(Mandatory)]
        [PSCredential] $DomainAdministratorCredential

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 78 at r1 (raw file):

    Write-Verbose -Message "Checking if Cluster $Name is present ..."

    $ComputerInfo = Get-WmiObject Win32_ComputerSystem

positional binding

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 81 at r1 (raw file):

    if (($ComputerInfo -eq $null) -or ($ComputerInfo.Domain -eq $null))
    {
        throw "Can't find machine's domain name"

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 110 at r1 (raw file):

            if (!(Get-Cluster))
            {
                throw "Cluster creation failed. Please verify output of 'Get-Cluster' command"

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 117 at r1 (raw file):

        else
        {
            Write-Verbose -Message "Add node to Cluster $Name ..."

Overall, should implement localization for messages.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 135 at r1 (raw file):

            }

            Add-ClusterNode $env:COMPUTERNAME -Cluster $Name -NoStorage

positional binding

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 154 at r1 (raw file):

# Test-TargetResource
#
# The code will check the following in order:

should be comment based help

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 167 at r1 (raw file):

    param
    (
        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 168 at r1 (raw file):

    (
        [parameter(Mandatory)]
        [string] $Name,

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 170 at r1 (raw file):

        [string] $Name,

        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 171 at r1 (raw file):

        [parameter(Mandatory)]
        [string] $StaticIPAddress,

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 173 at r1 (raw file):

        [string] $StaticIPAddress,

        [parameter(Mandatory)]

Mandatory = $true

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 174 at r1 (raw file):

        [parameter(Mandatory)]
        [PSCredential] $DomainAdministratorCredential

Parameter Names should be moved to newline

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 181 at r1 (raw file):

    Write-Verbose -Message "Checking if Cluster $Name is present ..."

    $ComputerInfo = Get-WmiObject Win32_ComputerSystem

positional binding

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 182 at r1 (raw file):

    $ComputerInfo = Get-WmiObject Win32_ComputerSystem
    if (($ComputerInfo -eq $null) -or ($ComputerInfo.Domain -eq $null))

$null should be on the left

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 184 at r1 (raw file):

    if (($ComputerInfo -eq $null) -or ($ComputerInfo.Domain -eq $null))
    {
        throw "Can't find machine's domain name"

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 16 at r1 (raw file):

Import-Module -Name (Join-Path -Path $script:moduleRoot -ChildPath (Join-Path -Path 'DSCResource.Tests' -ChildPath 'TestHelper.psm1')) -Force

# TODO: Insert the correct <ModuleName> and <ResourceName> for your resource

TODO should be removed

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 41 at r1 (raw file):

    InModuleScope $script:DSCResourceName {
        $mockAdministratorUserName = "COMPANY\clusteradmin"
        $mockAdministratorPassword = "dummyPassw0rd" | ConvertTo-SecureString -AsPlainText -Force

I would prefer ConvertTo-SecureString -String 'dummyPassw0rd' -AsPlainText -Force over using the pipeline

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 42 at r1 (raw file):

        $mockAdministratorUserName = "COMPANY\clusteradmin"
        $mockAdministratorPassword = "dummyPassw0rd" | ConvertTo-SecureString -AsPlainText -Force
        $mockAdministratorCredential = New-Object System.Management.Automation.PSCredential( $mockAdministratorUserName, $mockAdministratorPassword )

positional binding

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 131 at r1 (raw file):

            Context 'When the system is not in the desired state' {
                BeforeEach {

the BeforeEach isn't necessary

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 171 at r1 (raw file):

            Context 'When the system is not in the desired state' {
                BeforeEach {

Not necessary

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 243 at r1 (raw file):

                Context 'When the cluster exist and the node is down' {
                    BeforeEach {

Not necessary

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 262 at r1 (raw file):

            Context 'When the system is in the desired state' {
                BeforeEach {

not necessary

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 285 at r1 (raw file):

                Context 'When the node already exist' {
                    # This test is skipped because due to a logic error it's not possible to test this (issue #79)

Still the case?

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 314 at r1 (raw file):

            Context 'When the system is not in the desired state' {
                BeforeEach {

Not necessary

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 378 at r1 (raw file):

            Context 'When the system is in the desired state' {
                BeforeEach {

Not necessary

---

Tests/Unit/Stubs/FailoverClusters.stubs.psm1, line 18 at r1 (raw file):

param()

function Add-ClusterCheckpoint {

Should style guideline be applied to this file? In case not, I'm not evaluating it now 😄

---

Comments from Reviewable

[johlju]

That was a good review! I was hoping to wait for the style changes for the next PR though 😉 I didn't want to make to many changes at once. 😄 I commented on some, and I will fix the rest of the review comments.

My plan was this

1. Fix unit tests using new template for all resource.
2. Fix style changes for all resources.
3. Fix all PSSA rule warnings for all resources (fixing WMI to CIM and others).
4. Fix localization for all resource (I will fix issues for this).

---

Review status: all files reviewed at latest revision, 49 unresolved discussions, some commit checks failed.

---

.markdownlint.json, line 2 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Not sure about this... E.g. I don't use this extension. I'd like your opinion @PlagueHO should we / shouldn't we do stuff like this besides the formatting rules for the ps ext?

This extension is the same that is used by the common test to test for lint errors. So my plan is that this one is here to help fixing lint errors before PR is sent in (if contributors is using VS Code, and the markdownlint extension). Though, I all for discussing if this should be a general thing (part of the formatting rules) or if it should not be in the repos at all.
If we going this route, it would be good if we could write in the contributor documentation that the markdownlint extension is good to have installed. I did that in the "specific contributor" documentation over at xSQLServer (I really like documenting stuff).

We have a discussion here as as well, that is somewhat connected to this; PowerShell/DscResource.Tests#98. But today I'm leaning towards have the rules in this file exactly as common test is testing them. In this issue I'm talking about "overrides" because of the big technical debt I had in xSQLServer. But the technical debt has since then been lowered significantly, so if a month or so I think we can do there what I'm trying to do here. :)
So maybe that particular issue might be obsolete, but will get back to it once I activate all the common tests in xSQLServer.

---

CHANGELOG.md, line 60 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Is this the way to go instead of creating stubs in the tests themselves?

I think this is better because the stub file is auto-generated, using the exact parameters. And the same stub can be reused and not duplicating code in several tests.
Also it is possible to test different version of the Failover Cluster module (from different operating systems) which might not have all the cmdlets. So unit tests can potentially test compatibility between different version by using stubs generated from each version.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 27 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

Could we fix this when we add localization?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 36 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

Could we fix this when we add localization?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 51 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

The use of the return keyword serves no purpose here and can be removed.

I would rather use the return keyword to be clear that we are returning the hash table, that will also correlate with the OutputType.
Don't think there is a guideline for this, but rather our personal preference. But I'm open to not using the return keyword if we do that for the entire module. But if so, then I think we should add that to a specific resource contributor guideline if so, so we can enforce it. As of now I usually don't enforce it either way. But my personal preference is to used return keyword because it's more readable.
Are you still in favour of removing it?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 81 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

Could we fix this when we add localization?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 110 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

Could we fix this when we add localization?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 117 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Overall, should implement localization for messages.

Could we fix this when we add localization?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 184 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should output a descent constructed exception so it can be tested. (other modules make use of helper function to achieve this. See example at xHyper-V

Could we fix this when we add localization?

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 131 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

the BeforeEach isn't necessary

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 171 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Not necessary

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 243 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Not necessary

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 262 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

not necessary

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 285 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Still the case?

Yes, updated issue #79. I will fix this in a later PR (if someone else does not beat me to it). :)

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 314 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Not necessary

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 378 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Not necessary

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

---

Tests/Unit/Stubs/FailoverClusters.stubs.psm1, line 18 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should style guideline be applied to this file? In case not, I'm not evaluating it now 😄

Nah, I think not. I don't think you should review this file since it generate dynamically from the script that is part of this PR.
But I tried to make it return code that is according to the style guideline. But don't know if I can get the comment-based help for example :)

---

Comments from Reviewable

[bgelens]

Thanks for clarifying some of the points. I acknowledged everything that can be moved to later PRs as stated in your comment 😄

---

Review status: all files reviewed at latest revision, 35 unresolved discussions, some commit checks failed.

---

.markdownlint.json, line 2 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This extension is the same that is used by the common test to test for lint errors. So my plan is that this one is here to help fixing lint errors before PR is sent in (if contributors is using VS Code, and the markdownlint extension). Though, I all for discussing if this should be a general thing (part of the formatting rules) or if it should not be in the repos at all.
If we going this route, it would be good if we could write in the contributor documentation that the markdownlint extension is good to have installed. I did that in the "specific contributor" documentation over at xSQLServer (I really like documenting stuff).

We have a discussion here as as well, that is somewhat connected to this; PowerShell/DscResource.Tests#98. But today I'm leaning towards have the rules in this file exactly as common test is testing them. In this issue I'm talking about "overrides" because of the big technical debt I had in xSQLServer. But the technical debt has since then been lowered significantly, so if a month or so I think we can do there what I'm trying to do here. :)
So maybe that particular issue might be obsolete, but will get back to it once I activate all the common tests in xSQLServer.

I started using the extension because of this and I must say, it's really helpful!
I added some rules as these where flagged for me:

{
    "MD013": true,
    "MD004": true,
    "MD012": true,
    "MD022": true,
    "MD026": true,
    "MD031": true
}

I'm still in doubt if we need to add these files to the repo's. It basically says we endorse (prefer) the use of vs code + extension x, y, z.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 51 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I would rather use the return keyword to be clear that we are returning the hash table, that will also correlate with the OutputType.
Don't think there is a guideline for this, but rather our personal preference. But I'm open to not using the return keyword if we do that for the entire module. But if so, then I think we should add that to a specific resource contributor guideline if so, so we can enforce it. As of now I usually don't enforce it either way. But my personal preference is to used return keyword because it's more readable.
Are you still in favour of removing it?

Indeed personal.

IMO return keyword is coupled with certain behavior besides sending something to the output stream (e.g. when used in Test-TargetResource, it halts processing of the rest of the code. In other examples it could break out of a certain loop construct, etc.).

If you want to be explicit about what is going on, it's better to use Write-Output or $PSCmdlet.WriteObject() as these don't behave different in other circumstances. That said, everything that instantiates an objects is outputted to the output stream by default and everything else is actually overhead.

Still, one could argue for testing purposes it would be a good thing you can Assert that Write-Output is called (with return this is not possible I think).

So yes, my preference is, use anything besides return unless return is used to influence the behavior of execution of code paths.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 131 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It's necessary to use "dynamic mocks", setting a variable will change the mocks behaviour. For this to work the mock need to run before each It-block. So I rather use this logic throughout to be consequent.
See unit template these are there as well. If i remember correctly.

OK. Makes sense. Thanks for clarifying 😄

---

Comments from Reviewable

[johlju]

Review status: all files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

---

.markdownlint.json, line 2 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

I started using the extension because of this and I must say, it's really helpful!
I added some rules as these where flagged for me:

{
    "MD013": true,
    "MD004": true,
    "MD012": true,
    "MD022": true,
    "MD026": true,
    "MD031": true
}

I'm still in doubt if we need to add these files to the repo's. It basically says we endorse (prefer) the use of vs code + extension x, y, z.

I added MD013 because that is disabled by default.
https://github.com/DavidAnson/vscode-markdownlint#configure

I think the other are enable by default?

I understand how you are thinking, and I agree in general, but I think this is a special case since we are using the same code in the common test as the vs-code extension is using. We essentially helping the user to get past the common tests by making sure the extension is configured correctly.
This is used by the common test: https://github.com/DavidAnson/markdownlint/

Also. The issue below will enable the use of this file to override the behavior of the common test
PowerShell/DscResource.Tests#98

But I can remove it now and we can see if we need to add it in the future. 😄

---

Comments from Reviewable

[johlju]

Review status: 5 of 9 files reviewed at latest revision, 34 unresolved discussions.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 7 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should be comment based help with synopsis and parameters

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 14 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true (I'm not a fan of this one, but it's what needs to be done)

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 15 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 17 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 18 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 20 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 21 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 24 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

positional bindindg

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 25 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

$null should be on the left of the evaluation

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 39 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Positional binding for Get-ClusterParameter. Should include the parameter instead

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 51 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Indeed personal.

IMO return keyword is coupled with certain behavior besides sending something to the output stream (e.g. when used in Test-TargetResource, it halts processing of the rest of the code. In other examples it could break out of a certain loop construct, etc.).

If you want to be explicit about what is going on, it's better to use Write-Output or $PSCmdlet.WriteObject() as these don't behave different in other circumstances. That said, everything that instantiates an objects is outputted to the output stream by default and everything else is actually overhead.

Still, one could argue for testing purposes it would be a good thing you can Assert that Write-Output is called (with return this is not possible I think).

So yes, my preference is, use anything besides return unless return is used to influence the behavior of execution of code paths.

Good points! I'm agreeing with you. 😄. Removed.

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 53 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Missing the DomainAdministratorCredential key.
The output should match what is defined in the schema

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 58 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Should be comment based help with synopsis and parameters

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 64 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 65 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 67 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 68 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 70 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 71 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 78 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

positional binding

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 135 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

positional binding

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 154 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

should be comment based help

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 167 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 168 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 170 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 171 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 173 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Mandatory = $true

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 174 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

Parameter Names should be moved to newline

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 181 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

positional binding

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 182 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

$null should be on the left

Done.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 16 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

TODO should be removed

Done.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 41 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

I would prefer ConvertTo-SecureString -String 'dummyPassw0rd' -AsPlainText -Force over using the pipeline

Done.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 42 at r1 (raw file):

Previously, bgelens (Ben Gelens) wrote…

positional binding

Done.

---

.markdownlint.json, line 2 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I added MD013 because that is disabled by default.
https://github.com/DavidAnson/vscode-markdownlint#configure

I think the other are enable by default?

I understand how you are thinking, and I agree in general, but I think this is a special case since we are using the same code in the common test as the vs-code extension is using. We essentially helping the user to get past the common tests by making sure the extension is configured correctly.
This is used by the common test: https://github.com/DavidAnson/markdownlint/

Also. The issue below will enable the use of this file to override the behavior of the common test
PowerShell/DscResource.Tests#98

But I can remove it now and we can see if we need to add it in the future. 😄

Done.

---

Comments from Reviewable

[johlju]

@bgelens Will I was in the progress of fixing style changes, I did a few more. 😄

[bgelens]

Looking good! Just missing 1 decoration.

---

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 345 at r2 (raw file):

    param
    (
        [Parameter()]

This param is mandatory. Should be decorated as such

---

Comments from Reviewable

[johlju]

Review status: 7 of 9 files reviewed at latest revision, 1 unresolved discussion.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 345 at r2 (raw file):

Previously, bgelens (Ben Gelens) wrote…

This param is mandatory. Should be decorated as such

Done.

---

Comments from Reviewable

[johlju]

@bgelens Could you sing off on the last change when you got a chance, 😄

Thanks for reviewing!!

[bgelens]

---

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[johlju]

When I was about to merge this one I saw that I missed removing .markdownlint.json from the CHANGELOG.md.

I will merge this one as soon as the tests passes.

---

Reviewed 4 of 8 files at r1, 3 of 5 files at r2, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable