Added CodeCoverage for Class based resources - Fixes #173 #212

limiteddenial · 2018-03-10T23:01:08Z

Added Invoke-AppveyorTestScriptTask cmdlet functionality for CodeCoverage for Class based resources (issue #173)
Added tests for module AppVoyer.psm1 and tested Invoke-AppveyorTestScriptTask to verify it adds all the folders needed for CodeCoverage when running Invoke-Pester

This change is

…odeCoverage PowerShell#173

msftclas · 2018-03-10T23:01:19Z

All CLA requirements met.

… of APPVEYOR environment

codecov-io · 2018-03-10T23:38:59Z

Codecov Report

Merging #212 into dev will increase coverage by 11%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           dev   #212    +/-   ##
===================================
+ Coverage   20%    31%   +11%     
===================================
  Files        4      4            
  Lines      476    477     +1     
===================================
+ Hits        99    152    +53     
+ Misses     377    325    -52

johlju · 2018-03-11T13:02:08Z

Great work on this one! Glad to see this issue being fixed! 😄 Just a few minor style comments.

Reviewed 3 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed.

AppVeyor.psm1, line 256 at r2 (raw file):

                # Define the folders to check, if found add the path for codecoverage
                $possibleModulePaths = @(
                    "DSCResources",

Please use single quote on this string, and on the string below.

AppVeyor.psm1, line 262 at r2 (raw file):

                foreach ($possibleModulePath in $possibleModulePaths)
                {
                    if(Test-Path -Path $MainModulePath\$possibleModulePath)

Please add a space between if and parenthesis (if ().

Tests/Unit/AppVeyor.Tests.ps1, line 15 at r1 (raw file):

        Function Resolve-CoverageInfo { }
        Function Invoke-UploadCoveCoveIoReport { }
        Describe 'Invoke-AppveyorTestScriptTask' {

Can we add the module to the description, i.e 'AppVeyor\Invoke-AppveyorTestScriptTask'.

Tests/Unit/AppVeyor.Tests.ps1, line 15 at r1 (raw file):

        Function Resolve-CoverageInfo { }
        Function Invoke-UploadCoveCoveIoReport { }
        Describe 'Invoke-AppveyorTestScriptTask' {

Non-blocking personal opinion. Would it be possible to add a blank row before this row and in the code below where appropriate to create a natural separation of code blocks. It would be easier to review.

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

        Function Invoke-UploadCoveCoveIoReport { }
        Describe 'Invoke-AppveyorTestScriptTask' { 
            context 'CodeCoverage' {

This context block seems unnecessary. If this should be kept, then please change the description to what scenario is being tested 'When...' . Also, change to Context - upper 'C'.

Tests/Unit/AppVeyor.Tests.ps1, line 22 at r1 (raw file):

-MockWith { }

This is not necessary when no code is being mocked. The -MockWith { } can be removed. Throughout.

Tests/Unit/AppVeyor.Tests.ps1, line 3 at r2 (raw file):

$script:ModuleName = 'AppVeyor'
$script:moduleRootPath = Split-Path -Path (Split-Path -Path $PSScriptRoot -Parent) -Parent
if($null -eq $env:APPVEYOR_BUILD_FOLDER)

Please add a space between if and parenthesis (if ().

Tests/Unit/AppVeyor.Tests.ps1, line 7 at r2 (raw file):

    $env:APPVEYOR_BUILD_FOLDER = 'C:\projects\dscresource-tests'
}
Describe "$($script:ModuleName) Unit Tests" {

This is non-blocking since there is not guideline for this, just my personal opinion, and an attempt for the tests to be similar. 😄

I suggest we don't do the Describe-blocks recursive, I think it is mean that the Context blocks can be made recursive. It do work this way though, but I have seen it giving strange output in the Pester result object. Could we instead start with InModuleScope as proposed in these tests (currently in PR).
https://github.com/johlju/DscResource.Tests/blob/da656f5b41d00ac402b0d5f643d4feba7b2d5931/Tests/Unit/TestHelper.Tests.ps1

Comments from Reviewable

johlju · 2018-03-11T13:05:23Z

@limiteddenial to be able to merge this change you need to have sign the CLA.

johlju · 2018-03-17T12:41:46Z

Just a few tiny review comments left, then this looks good for me.

@limiteddenial When you are done with the review comments, please go into Reviewable (the purple button in the PR description) and write 'Done' on all the comments, then press the green Publish button and the comments will be sent back to GitHub. We know then that you are finished with the change, and also make it possible for me to acknowledge (resolve) the review comment. 😄
The previous review comments I resolved by withdrawing the comment when I saw that the change was made, so all good with them. 😄

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Tests/Unit/AppVeyor.Tests.ps1, line 18 at r3 (raw file):

direcotries

minor typo:" directories"

Tests/Unit/AppVeyor.Tests.ps1, line 27 at r3 (raw file):

$False

$false (lower 'f')

Tests/Unit/AppVeyor.Tests.ps1, line 28 at r3 (raw file):

"file.Tests.ps1"

Can we use single quotes here?

Tests/Unit/AppVeyor.Tests.ps1, line 32 at r3 (raw file):

                Mock -CommandName Resolve-CoverageInfo
                Mock -CommandName Invoke-Pester -MockWith { return $pesterReturnedValues }
                Mock -CommandName Invoke-Pester -MockWith { return $pesterReturnedValues } -ParameterFilter {$CodeCoverage -contains "$env:APPVEYOR_BUILD_FOLDER\DSCClassResources\**\*.psm1"}

Could we use a space after the opening brace and before the closing brace, for all occurrences, to be consequent? Throughout the file.

Tests/Unit/AppVeyor.Tests.ps1, line 73 at r3 (raw file):

direcotries

minor typo:" directories"

Comments from Reviewable

limiteddenial · 2018-03-17T14:47:00Z

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Tests/Unit/AppVeyor.Tests.ps1, line 18 at r3 (raw file):