PowerShell / PSDscResources

MIT License
129 stars 53 forks source link

Update Group and GroupSet tests to meet Pester 4 standards #135

Closed PlagueHO closed 5 years ago

PlagueHO commented 5 years ago

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 :grin:).

This Pull Request (PR) fixes the following issues

Task list


This change is Reviewable

codecov-io commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@         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
PlagueHO commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

PlagueHO commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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: image

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.

mhendric commented 5 years ago

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