xVirtualMemory: Refactor and improve unit test coverage and HQRM - Fixes #95 #96

PlagueHO · 2017-08-17T09:42:23Z

Pull Request (PR) description
This PR refactors xVirtualMemory to:

reduce code duplication
improve testability (it was very difficult to unit test)
meet HQRM standards
increase unit test coverage to 95%
convert exception calling to use CommonResourceHelper.psm1
moved strings into localization file

This Pull Request (PR) fixes the following issues:
Fixes #95
Fixes #81

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?

@johlju - sorry about the size of this PR, but once I took a look at the existing code and unit tests, I found it wasn't going to be easy to get decent coverage without refactoring. The same with HQRM - the resource needed a lot of work to get it into decent shape.

This change is

codecov-io · 2017-08-17T10:03:30Z

Codecov Report

Merging #96 into dev will increase coverage by 9%.
The diff coverage is 94%.

@@        Coverage Diff        @@
##           dev   #96   +/-   ##
=================================
+ Coverage   68%   78%   +9%     
=================================
  Files        6     6           
  Lines      725   706   -19     
=================================
+ Hits       499   552   +53     
+ Misses     226   154   -72

johlju · 2017-08-17T18:18:43Z

I have some problems over at xSQLServer that I need to focus on. But will get to this as soon as those bugs are squashed.

PlagueHO · 2017-08-18T20:57:49Z

Absolutely no problem or rush @johlju - just whenever! I'm planning another PR against this repo to fix #85 so that one would take precedence in getting reviewed over this one.

johlju · 2017-08-23T17:07:55Z

Awesome work as usual. Just minor things.

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

DSCResources/MSFT_xVirtualMemory/MSFT_xVirtualMemory.psm1, line 45 at r1 (raw file):

[bool]

Change to [System.Boolean]?

DSCResources/MSFT_xVirtualMemory/MSFT_xVirtualMemory.psm1, line 50 at r1 (raw file):

    {
        $returnValue.Type = 'AutoManagePagingFile'
        return $returnValue

Maybe use else instead of returning here, and in another location, and only return at the end? But good as is too. Non-blocking.

DSCResources/MSFT_xVirtualMemory/MSFT_xVirtualMemory.psm1, line 385 at r1 (raw file):

}

function Set-PageFileSetting

Missed comment-based help here.

DSCResources/MSFT_xVirtualMemory/en-US/MSFT_xVirtualMemory.strings.psd1, line 7 at r1 (raw file):

Specifting

This was a new word for me ;)

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 197 at r1 (raw file):

                    -Verifiable

                It 'Should not return a valid type and drive letter' {

"Should not..." seems wrong here, it does return something.

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 237 at r1 (raw file):

                    -Verifiable

                It 'Should not return a valid type and drive letter' {

Same here

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 351 at r1 (raw file):

                        -Verifiable

                    It 'Should return false' {

Wrong text here.

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 428 at r1 (raw file):

                        -Verifiable

                    It 'Should return false' {

Same here

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 504 at r1 (raw file):

                            -Verifiable

                        It 'Should return false' {

Same here

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 576 at r1 (raw file):

                        -Verifiable

                    It 'Should return false' {

Same here

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 639 at r1 (raw file):

                            -CommandName Remove-CimInstance

                        It 'Should return false' {

Same here

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 696 at r1 (raw file):

                        -Verifiable

                    It 'Should return false' {

Same here

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 734 at r1 (raw file):

With

When?

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 764 at r1 (raw file):

With

When?

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 1133 at r1 (raw file):

                        -Verifiable

                    It 'Should not return the expected object' {

Add a test when it do return the correct object?

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 1141 at r1 (raw file):

                    It 'Should call the correct mocks' {
                        Assert-VerifiableMocks

Do you need this? Throughout

Tests/Unit/MSFT_xVirtualMemory.Tests.ps1, line 1206 at r1 (raw file):

throw exception

unsure, but maybe "throw an exception"? Throughout

Comments from Reviewable

PlagueHO · 2017-08-25T09:34:52Z

All fixed I think! Great review, thanks @johlju

Review status: 1 of 4 files reviewed at latest revision, 17 unresolved discussions.

DSCResources/MSFT_xVirtualMemory/MSFT_xVirtualMemory.psm1, line 45 at r1 (raw file):