Adding latest version of 'Test-DscParameterState' from JeaDsc #46

raandree · 2020-07-12T13:37:35Z

Pull Request (PR) description

Adding latest version of 'Test-DscParameterState' from JeaDsc.

Test-DscParameterState can now handle scriptblocks.
The parameter 'ValuesToCheck was renamed to 'Properties' but an alias was added so it is not a braking change.
The parameter 'ExcludeProperties' was added.

This Pull Request (PR) fixes the following issues

NA

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

…rties' and added 'ExcludeProperties'

johlju

@raandree Awesome work! A few comments.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @raandree)

README.md, line 551 at r1 (raw file):

[[-ExcludeProperties] <string[]>] [-TurnOffTypeChecking] [-ReverseCheck]

I'm missing the renamed parameterProperties here?

README.md, line 588 at r1 (raw file):

-ExcludeProperties

Should this be Properties and that we should have a new example that shows how to use ExcludeProperties?

source/en-US/DscResource.Common.strings.psd1, line 12 at r1 (raw file):

InvalidExcludePropertiesError = If 'DesiredValues' is a CimInstance then property 'ExcludeProperties' must contain a value. (DRC0016)

Is this text correct? The localization key string is used when evaluating the parameter Properties. 🤔

source/Public/Test-DscParameterState.ps1, line 55 at r1 (raw file):

-ExcludeProperties @(

Should this be Properties and we should have another example for ExcludeProperties?

source/Public/Test-DscParameterState.ps1, line 87 at r1 (raw file):

[switch]

Please change to [System.Management.Automation.SwitchParameter]

source/Public/Test-DscParameterState.ps1, line 91 at r1 (raw file):

[switch]

Please change to [System.Management.Automation.SwitchParameter]

source/Public/Test-DscParameterState.ps1, line 95 at r1 (raw file):

[switch]

Please change to [System.Management.Automation.SwitchParameter]

source/Public/Test-DscParameterState.ps1, line 132 at r1 (raw file):

Quoted 6 lines of code…

    if ($DesiredValues -is [Microsoft.Management.Infrastructure.CimInstance] -and -not $Properties)
    {
        New-InvalidArgumentException `
            -Message $script:localizedData.InvalidExcludePropertiesError `
            -ArgumentName 'ExcludeProperties'
    }

Is this text correct? We evaluate the parameter Properties, but throws an exception for ExcludeProperties. 🤔
Was it meant to have another evaluation for the property ExcludeProperties?

See comment in the localization strings as well.

source/Public/Test-DscParameterState.ps1, line 148 at r1 (raw file):

Where-Object { $_ -notin $ExcludeProperties }

Use named parameters Where-Object -FilterScript { $_ -notin $ExcludeProperties }

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 741 at r1 (raw file):

                }
            }

Remove extra blank line

johlju

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @raandree)

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 227 at r1 (raw file):

Context 'When a value is mismatched but ExcludeProperties is used to exclude then' {

This test test ValuesToCheck before, that was renamed to Properties. So should this test the parameter Properties and there should be another test (this test) to test the parameter ExcludeProperties?

Also, we should add a test the calls using the alias ValuesToCheck as well so that works as expected.

raandree

Reviewable status: 1 of 6 files reviewed, 12 unresolved discussions (waiting on @johlju)

README.md, line 551 at r1 (raw file):

ExcludeProperties
Sorry, I was thinking that the readme.ms is auto-generated using the help. I have updated the section.

README.md, line 588 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-ExcludeProperties
Should this be Properties and that we should have a new example that shows how to use ExcludeProperties?

Done.

source/en-US/DscResource.Common.strings.psd1, line 12 at r1 (raw file):

InvalidExcludePropertiesError

source/en-US/DscResource.Common.strings.psd1, line 12 at r1 (raw file):

InvalidExcludePropertiesError 
Right, thanks for the correction!

source/Public/Test-DscParameterState.ps1, line 55 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-ExcludeProperties @(
Should this be Properties and we should have another example for ExcludeProperties?

We have not one sample for the parameter 'Properties' and one for 'ExcludeProperties'. If the examples should be more detailed, please let me know.

source/Public/Test-DscParameterState.ps1, line 87 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[switch]
Please change to [System.Management.Automation.SwitchParameter]

Done.

source/Public/Test-DscParameterState.ps1, line 91 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[switch]
Please change to [System.Management.Automation.SwitchParameter]

Done.

source/Public/Test-DscParameterState.ps1, line 95 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[switch]
Please change to [System.Management.Automation.SwitchParameter]

Done.

source/Public/Test-DscParameterState.ps1, line 132 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    if ($DesiredValues -is [Microsoft.Management.Infrastructure.CimInstance] -and -not $Properties)
    {
        New-InvalidArgumentException `
            -Message $script:localizedData.InvalidExcludePropertiesError `
            -ArgumentName 'ExcludeProperties'
    }
Is this text correct? We evaluate the parameter Properties, but throws an exception for ExcludeProperties. 🤔
Was it meant to have another evaluation for the property ExcludeProperties?

See comment in the localization strings as well.

Done.

source/Public/Test-DscParameterState.ps1, line 148 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Where-Object { $_ -notin $ExcludeProperties }
Use named parameters Where-Object -FilterScript { $_ -notin $ExcludeProperties }

Done.

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 227 at r1 (raw file):

When a value is mismatched but
This test makes more sense as only one property of the desired values is different from the current values. I have added another test for 'Properties'

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 741 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove extra blank line

Done.

raandree · 2020-07-12T22:28:42Z

What is the difference between 'Compare-ResourcePropertyState' in #48 and 'Test-DscParameterState'? Do we have two functions now doing the same thing?

johlju · 2020-07-13T10:49:37Z

What is the difference between 'Compare-ResourcePropertyState' in #48 and 'Test-DscParameterState'? Do we have two functions now doing the same thing?

First I'm very happy we get this PR that updates Test-DscParameterState which many DSC modules uses.

The difference is that they are two different design patterns. While Test-DscParameterState is designed to be used only in Test the Compare-ResourcePropertyState is designed to be used in both Test and Set (and can also be used in Get, but that is an edge case). I tried to explain it a bit here in the proposed PR; https://github.com/dsccommunity/DscResource.Common/blob/f34b8e8770d56bc5ac02a2a50cfebaa3f4677b45/README.md#compare-resourcepropertystate, but probably need a blog post around this in the future (on the todo list if the PR is accepted).
This new design pattern is basically meant to handle that Set does not set values that are already in desired state. Having one compare function for both Set and Test means there is no need to duplicate code in Set to evaluate if a property should be set or not.

There are many modules that are using Test-DscParameterState for its resources and that will probably not change. So the other design pattern can be used for new modules/resources if a contributor/maintainer so chooses. Currently the only resources that uses this new design pattern are new resources in ActiveDirectoryDsc and SqlServerDsc. But SqlServerDsc also uses the design pattern Test-DscParameterState for many resources (and I don't see those being refactored).

Btw. I tried to reuse Test-DscParameterState in the new design but that was not possible how it was written. I had to break out the actual property testing in a separate function (private function Test-DscPropertyState) that is also calling itself recursively (when testing CIM instances). Maybe both design patterns can use that function, although Test-DscParameterState have more functionality that need to be moved first.

johlju

Just a few more tiny changes. Then it will be ready to merge I think.

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @raandree)

README.md, line 551 at r1 (raw file):

Previously, raandree (Raimund Andrée [MSFT]) wrote…

ExcludeProperties
Sorry, I was thinking that the readme.ms is auto-generated using the help. I have updated the section.

Unfortunately no , I want that in the future. I'm thinking of adding auto-documentation for this in the future (maybe part of the module DscResource.DocGenerator). But so many things that would be fun to do, but so little time. 😄

Looks good to me now.

source/Public/Test-DscParameterState.ps1, line 55 at r1 (raw file):

Previously, raandree (Raimund Andrée [MSFT]) wrote…

We have not one sample for the parameter 'Properties' and one for 'ExcludeProperties'. If the examples should be more detailed, please let me know.

Could we add the example you added in the README.md to here too? Then we have the correct examples if we do auto-doc in the future.

source/Public/Test-DscParameterState.ps1, line 133 at r2 (raw file):

-ArgumentName 'ExcludeProperties'

This should be 'Properties' now?

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 310 at r2 (raw file):

                It 'Should return $false' {
                    $script:result | Should -BeFalse
                }

Could we add a nested context-block after the It-block that regression tests the ValuesToCheck alias?

Example:

    Context 'When using the alias ValuesToCheck' {
                It 'Should not throw exception' {
                    { $script:result = Test-DscParameterState `
                            -CurrentValues $currentValues `
                            -DesiredValues $desiredValues `
                            -Properties $properties `
                            -Verbose:$verbose } | Should -Not -Throw
                }
                It 'Should return $false' {
                    $script:result | Should -BeFalse
                }
    }

raandree

Thanks for the explanation. IMHO we should merge both patterns to remove redundancy. Another task on the list...

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

source/en-US/DscResource.Common.strings.psd1, line 12 at r1 (raw file):

InvalidExcludePropertiesError
I am not sure what this particular section was about. I have renamed the message 'InvalidExcludePropertiesError' to 'InvalidPropertiesError'.

source/Public/Test-DscParameterState.ps1, line 55 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add the example you added in the README.md to here too? Then we have the correct examples if we do auto-doc in the future.

Done.

source/Public/Test-DscParameterState.ps1, line 133 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-ArgumentName 'ExcludeProperties'
This should be 'Properties' now?

Done.

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 310 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a nested context-block after the It-block that regression tests the ValuesToCheck alias?

Example:

    Context 'When using the alias ValuesToCheck' {
                It 'Should not throw exception' {
                    { $script:result = Test-DscParameterState `
                            -CurrentValues $currentValues `
                            -DesiredValues $desiredValues `
                            -Properties $properties `
                            -Verbose:$verbose } | Should -Not -Throw
                }
                It 'Should return $false' {
                    $script:result | Should -BeFalse
                }
    }

Done.

gaelcolas · 2020-07-19T04:39:04Z

Really looking forward to this (& compare-), I think it will really help improve the DSC Resources quality & reduce effort in the longer run.

johlju

Reviewed 2 of 3 files at r3, 3 of 3 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion

johlju

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

tests/Unit/Public/Test-DscParameterState.Tests.ps1, line 289 at r4 (raw file):

                    It 'Should return $false' {
                        $script:result | Should -BeFalse

I think this should be $true instead, similar to the test that follows this.

johlju · 2020-07-19T16:12:17Z

@raandree the last test is failing the tests, see review comment.

johlju

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion

johlju

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion

raandree added 4 commits July 12, 2020 01:15

Removed an old string

9765f7d

Added support for scriptblocks and, renamed 'ValuesToCheck' to 'Prope…

69e7e42

…rties' and added 'ExcludeProperties'

Updated Changelog

ad62a3a

Removed unused lines

54d908c

johlju requested changes Jul 12, 2020

View reviewed changes

johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 12, 2020

raandree added 2 commits July 12, 2020 23:07

Changes as requested in code review

f8c4bcd

Added a test for parameter 'Properties'

ba2b462

raandree commented Jul 12, 2020

View reviewed changes

johlju requested changes Jul 13, 2020

View reviewed changes

Changes per code review

3d03d18

raandree commented Jul 18, 2020

View reviewed changes

Merge branch 'master' into master

e37b339

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jul 19, 2020

johlju approved these changes Jul 19, 2020

View reviewed changes

johlju requested changes Jul 19, 2020

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 19, 2020

Fix unit test

c73f52d

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jul 22, 2020

johlju approved these changes Jul 22, 2020

View reviewed changes

Merge branch 'master' into master

04e8b16

johlju approved these changes Jul 22, 2020

View reviewed changes

johlju merged commit e914604 into dsccommunity:master Jul 22, 2020

johlju removed the needs review The pull request needs a code review. label Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding latest version of 'Test-DscParameterState' from JeaDsc #46

Adding latest version of 'Test-DscParameterState' from JeaDsc #46

raandree commented Jul 12, 2020 •

edited by johlju

Loading

johlju left a comment

johlju left a comment

raandree left a comment

raandree commented Jul 12, 2020

johlju commented Jul 13, 2020 •

edited

Loading

johlju left a comment

raandree left a comment

gaelcolas commented Jul 19, 2020

johlju left a comment

johlju left a comment

johlju commented Jul 19, 2020

johlju left a comment

johlju left a comment

Adding latest version of 'Test-DscParameterState' from JeaDsc #46

Adding latest version of 'Test-DscParameterState' from JeaDsc #46

Conversation

raandree commented Jul 12, 2020 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

raandree left a comment

Choose a reason for hiding this comment

raandree commented Jul 12, 2020

johlju commented Jul 13, 2020 • edited Loading

johlju left a comment

Choose a reason for hiding this comment

raandree left a comment

Choose a reason for hiding this comment

gaelcolas commented Jul 19, 2020

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jul 19, 2020

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

raandree commented Jul 12, 2020 •

edited by johlju

Loading

johlju commented Jul 13, 2020 •

edited

Loading