Update Group and GroupSet tests to meet Pester 4 standards

[PlagueHO]

Pull Request (PR) description

This PR refactors Group and GroupSet tests to meet Pester 4 standards. Also made some tweaks to the tests to use Pester It block -TestCases for some tests.

I also tweaked the README.MD style to remove : after resource titles as this is the standard (I had been doing it wrong myself 😁).

This Pull Request (PR) fixes the following issues

• Contributes to Update Tests to meet 4.x Standards #129

Task list

• Added an entry under the Unreleased section of the change log in the README.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov-io]

Codecov Report

Merging #135 into dev will not change coverage.
The diff coverage is 50%.

@@         Coverage Diff         @@
##            dev   #135   +/-   ##
===================================
  Coverage    83%    83%           
===================================
  Files        19     19           
  Lines      2760   2760           
  Branches      4      4           
===================================
  Hits       2305   2305           
  Misses      451    451           
  Partials      4      4

[mhendric]

Reviewed 7 of 8 files at r1.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @mhendric and @PlagueHO)

a discussion (no related file):
Non blocker for this one, but it looks like a lot of these functions need comment based help. Do you think that's worth tracking in a new issue?

---

---

Tests/Unit/MSFT_GroupResource.Tests.ps1, line 103 at r1 (raw file):

Describe 'Get-TargetResource' {

I don't think it's necessary for Pester to function, but I noticed you removed all Context blocks (and replaced them with Describe blocks). So now they all just go straight from Describe to It. Is that an acceptable Pester pattern?

Also, if we're trying to follow the test template, should we change this to something like?:

Describe 'MSFT_GroupResource\Get-TargetResource' -Tag 'Get'

Throughout.

[PlagueHO]

I'm having a bit of a think about this one - the problem I'm seeing is that the unit tests for GroupResource don't match the pattern of all the other resources in this module.

Normally we use the Describe block to define a fixture for the function under test - but for this resource the Context block was used. I can see why though:

There needed to be an outer Describe block within the InModuleScope to contain the BeforeAll/AfterAll blocks that must run before each function test. However, I think the code that runs in the BeforeAll/AfterAll can just be place in-line and therefore enables us to adopt a better pattern (like MsiPackage):

try
 +- InModuleScope
      +- Describe 'Unit tests'
           +- BeforeAll/AfterAll
           +- Context 'When...'
               +- It ...
catch
finally

However, some resources have adopted a different pattern (see MsiPackage) - which I think is better. An outer Describe block is used (rather than the older try/catch/finally) pattern. Inner describe blocks are then used to define the function:

Describe 'Unit tests'
 +- InModuleScope
      +- Describe 'function'
           +- Context 'When...'
               +- It ...

But the big problem I see is there just weren't any Context blocks defined, with the It blocks containing all the assertions in one big "test".

Either way, I'm going to try and rework to create proper contexts in the tests.

[PlagueHO]

It is painful doing all the refactoring on this one, but it has highlighted some incorrect tests and some omissions. So it'll be worthwhile I think. I'm aiming to have this completed tonight, but I've got to spin up a Nano Server to complete testing.

[mhendric]

Cool, thanks @PlagueHO . Let me know once you're ready for another review.

[PlagueHO]

Thanks @mhendric - should be later on today.

And just for info, here is the command I use to create the Nano VHDX to run tests on:
https://gist.github.com/PlagueHO/965a3b2cce2fc3687134ffa0d8bdad69

I'd like to add a helper script that could be used to create a Nano Hyper-V VM, inject the module code and execute tests. Otherwise it is a lot of manual work getting the Nano VM stood up ready for testing. Will raise a separate issue.

[PlagueHO]

Nearly there - I found a few bugs in the Nano tests that must have been there a while. Not sure when the tests were last run on Nano.

[PlagueHO]

@mhendric - It is good to go now. I've finished refactoring all the GroupResource unit tests and fixed the Nano ones so that they all work:

There are additional refactorings that could be made (based on what you did over in xPSDesiredStateConfguration), but we can address that at a later date now that these are closer to Pester standards.

[PlagueHO]

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @mhendric)

a discussion (no related file):

Previously, mhendric (Mike Hendrickson) wrote…

Non blocker for this one, but it looks like a lot of these functions need comment based help. Do you think that's worth tracking in a new issue?

Yes - definitely.

However, I think we should possibly examine the rule around comment based help on every function. Sometimes it is actually not helpful. E.g. if the function is very small, obeys single responsibility principle, has a very clear expressive function name, is private/internal - then perhaps the comment based help works against us (e.g. the help may actually not be true - only code is guaranteed to be true). E.g. We should try and adopt principles from "Clean Code" - make our code the source of truth, not the comments. But that is a discussion for another day.

---

---

Tests/Unit/MSFT_GroupResource.Tests.ps1, line 103 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Describe 'Get-TargetResource' {

I don't think it's necessary for Pester to function, but I noticed you removed all Context blocks (and replaced them with Describe blocks). So now they all just go straight from Describe to It. Is that an acceptable Pester pattern?

Also, if we're trying to follow the test template, should we change this to something like?:

Describe 'MSFT_GroupResource\Get-TargetResource' -Tag 'Get'

Throughout.

Done.

[mhendric]

Reviewed 2 of 3 files at r3.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @PlagueHO)

---

Tests/Unit/MSFT_GroupResource.Tests.ps1, line 237 at r4 (raw file):

Describe 'GroupResource\Assert-GroupNameValid' {

Should this (and all other helper functions) use -Tag 'Helper' ?

[mhendric]

Looking good @PlagueHO . Just one more comment on my part. Up to you if you want to integrate or not.

[PlagueHO]

Thanks again @mhendric! Have made suggested changes.

Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @mhendric)

---

Tests/Unit/MSFT_GroupResource.Tests.ps1, line 237 at r4 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Describe 'GroupResource\Assert-GroupNameValid' {

Should this (and all other helper functions) use -Tag 'Helper' ?

Good idea. Done.

[mhendric]

Great work on this @PlagueHO!

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved