GetDesiredState handle zeroed enum values using Get-DscProperty

[johlju]

Pull Request (PR) description

• Removed Clear-ZeroedEnumPropertyValue as it was moved and implemented
  the Get-DscProperty command in the DscResource.Common module.
• ResourceBase
  • Refactor GetDesiredState method to handle zeroed enum values using
    Get-DscProperty when the property FeatureOptionalEnums is set to
    $true.

This Pull Request (PR) fixes the following issues

• Fixes Get-DscProperty: Add optional switch parameter IgnoreZeroEnumValue DscResource.Common#150 (comment)

Task list

• Added an entry to the change log under the Unreleased section of the
  file CHANGELOG.md. Entry should say what was changed and how that
  affects users (if applicable), and reference the issue being resolved
  (if applicable).
• Documentation added/updated in README.md.
• Comment-based help added/updated for all new/changed functions.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Community Testing Guidelines.
• Integration tests added/updated (where possible). See
  DSC Community Testing Guidelines.
• New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[johlju]

@dan-hughes if and when you have time, can you review this as you are more familiar with the change necessary. 🙂

[dan-hughes]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 223 at r2 (raw file):

                }

                Should -Invoke -CommandName Get-DscProperty -ParameterFilter {

Is the ParameterFilter required?
It does not match up to a Mock with a ParameterFilter present.

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 313 at r2 (raw file):

                }

                Should -Invoke -CommandName Get-DscProperty -ParameterFilter {

Same here.

[johlju]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-hughes)

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 223 at r2 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

Is the ParameterFilter required?
It does not match up to a Mock with a ParameterFilter present.

It is not required but it makes sure that the call to Get-DscProperty was actually called with the expected parameter and not without it.

I suggest that we should not remove the parameter filter on the Should -Invoke because it verifies the expected call the same why we verified Clear-ZeroedEnumPropertyValue previously.

TL;DR: I don't mind adding a parameter filter to the mock, but I don't see it being necessary.

There is no need to add the parameter filter to the mock unless the mock should only be used when the specified parameter filter is true. In this case we mock Get-DscProperty regardless of how it is called, then we verify that it was actually called with the expected parameters.

Adding a parameter filter to the mock just adds another check that Pester needs to do (when it determines what mock to use). Also, if it for some reason goes into the wrong code (in a future change) path then Get-DscProperty would not be mocked at all and it would call the real function.

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 313 at r2 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

Same here.

See previous comment. 🙂

[dan-hughes]

For another issue, but it looks like there are some unnecessary calls to both Assert and Normalize.
These are called in the Set and Test methods, both of these call Compare which calls Get which again calls Assert and Normalize.

It looks like Assert and Normalize can be removed from both Test and Set as they are called later on.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 223 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It is not required but it makes sure that the call to Get-DscProperty was actually called with the expected parameter and not without it.

I suggest that we should not remove the parameter filter on the Should -Invoke because it verifies the expected call the same why we verified Clear-ZeroedEnumPropertyValue previously.

TL;DR: I don't mind adding a parameter filter to the mock, but I don't see it being necessary.

There is no need to add the parameter filter to the mock unless the mock should only be used when the specified parameter filter is true. In this case we mock Get-DscProperty regardless of how it is called, then we verify that it was actually called with the expected parameters.

Adding a parameter filter to the mock just adds another check that Pester needs to do (when it determines what mock to use). Also, if it for some reason goes into the wrong code (in a future change) path then Get-DscProperty would not be mocked at all and it would call the real function.

I think its the overuse of ParameterFilter on some of the Pester 5 conversions I've undertaken that has clouded my view on them.

I can see why you would want to do that but should it not be in the GetDesiredState method test?

[johlju]

We can fix that in another PR, tracked in #35.

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @dan-hughes)

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 223 at r2 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

I think its the overuse of ParameterFilter on some of the Pester 5 conversions I've undertaken that has clouded my view on them.

I can see why you would want to do that but should it not be in the GetDesiredState method test?

Done. You are correct, I just change the existing test, didn't even realized we were missing tests for GetDesiredState.

---

tests/Unit/Classes/ResourceBase.Tests.ps1 line 313 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

See previous comment. 🙂

Done

[johlju]

@dan-hughes ready for review again.

[dan-hughes]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johlju)

[johlju]

Thanks for the review, great to have one more over the finish line 🙂