Fixing how the LocalAcccounts tests are skipped in Pester - #2335

Hi Mike Richmond (@mirichmo), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Mike Richmond). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

Mike Richmond (@mirichmo) can you provide a link to this guidelines?
I find this way of skipping them questionable and potentially leading to problems. Would like to discuss.

The guidance is in Jim's PR: https://github.com/JamesWTruher/PowerShell-1/blob/f8db6811a267e8cb7b58ee0b37181716d4317316/docs/testing-guidelines/WritingPesterTests.md

The current approach is definitely better then what it used to be. The problem that I see is that it's too verbose. There are a lot of Describe blocks with few It blocks inside and it creates an overwhelmingly verbose massaging to achieve the goal. I think it's appropriate to use a modification of this technique here: it could be done once per file, at the beginning, and then the rest of the file can be wrapped in try-finally, where previous parameters are restored in finally. It's less idiomatic, but saves a lot of copy-paste.

The motivation for using this technique is that the tests are tracked consistently across all platforms. The original method caused these tests to disappear when run on non-Windows platforms. Now they can at least be tracked as skipped.

Exactly, that's why I said it's better then it used to be. I'm just proposing to use a less verbose variation of the skipping, do it as a top-level thing on the file. James Truher (@JamesWTruher) can you comment?

I uncovered one wrinkle to this test skipping technique when fixing the tests. It becomes less clean when tests need to do actual work in the BeforeAll, AfterAll, BeforeEach, and AfterEach. Even Sergei's suggestion suffers from the same checking requirement. Following the existing guidance, you end up with something like this:

    BeforeAll {
        #skip all tests on non-windows platform
        $originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
        $PSDefaultParameterValues["it:skip"] = ($IsWindows -eq $false)

        if ($PSDefaultParameterValues["it:skip"] -eq $false) {
            <work>
        }
    }
    AfterAll {
        if ($PSDefaultParameterValues["it:skip"] -eq $false) {
            <work>
        }
        $global:PSDefaultParameterValues = $originalDefaultParameterValues
    }
    BeforeEach {
        if ($PSDefaultParameterValues["it:skip"] -eq $false) {
            <work>
        }
    }

    AfterEach {
        if ($PSDefaultParameterValues["it:skip"] -eq $false) {
            <work>
        }
    }

We could use a convention like:

BeforeAll {
        #skip all tests on non-windows platform
        $originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
        $skipIt = ($IsWindows -eq $false)
        $PSDefaultParameterValues["it:skip"] = $skipIt

        if ($skipIt) {
            # work
        }
    }
    AfterAll {
        if ($skipIt) {
            # work
        }
        $global:PSDefaultParameterValues = $originalDefaultParameterValues
    }
    BeforeEach {
        if ($skipIt) {
            # work
        }
    }

    AfterEach {
        if ($skipIt) {
            # work
        }
    }

I'm not really a fan of PSDefaultParameterValues though, so if enough people prefer to explicitly specify -Skip:$skipIt, I think I'd +1 that proposal.

I prefer explicit, because PSDefaultParameters contains global state and if something goes wrong it can mess-up the rest of execution royally. But for the bulk skip, explicit skip is too verbose.

Maybe the right solution is to work with Pester to provide -Skip on Describe in such a way that reports everything we want. In the shorter term, we can live with this solution.

sergei (@vors) or Jason Shirk (@lzybkr) it is ready for final review. Please take a look

We talked offline about the approach with James Truher (@JamesWTruher) and he agreed that it's too verbose. Please implement the top level try-finally.

A top-level try-finally doesn't really shrink it all that much. This is what you end up with:

try {
  #skip all tests on non-windows platform
  $originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
  $skipIt = ($IsWindows -eq $false)
  $PSDefaultParameterValues["it:skip"] = $skipIt

  Describe "Example for skipping a whole file" {
    BeforeAll {
        if ($skipIt) {
            # work
        }
    }
    AfterAll {
        if ($skipIt) {
            # work
        }
    }
    BeforeEach {
        if ($skipIt) {
            # work
        }
    }

    AfterEach {
        if ($skipIt) {
            # work
        }
    }
  }
}
finally {
  $global:PSDefaultParameterValues = $originalDefaultParameterValues
}

James Truher (@JamesWTruher) Please confirm this is what you want

A benefit for this approach is that if you have multiple describe blocks in a single file, it becomes a bit more manageable as you'll have less duplicated code. I don't have a problem with the try/catch approach, and depending on circumstances, I would probably prefer it.

Hi Mike Richmond (@mirichmo), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Mike Richmond). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

sergei (@vors) I made the try-catch change. It's ready for review now