Migrate to Pester 5

[dan-hughes]

Pull Request (PR) description

Update the tests to use Pester 5. Module changes will be kept to a minimum.

This Pull Request (PR) fixes the following issues

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).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof
  and comment-based help.
• Comment-based help added/updated.
• 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 

[dan-hughes]

@johlju can you azp run please?

[johlju]

Can you update the line installing GitVersion in azure-pipelines.yml to gaelcolas/Sampler#477 (comment)? That will be needed or the build will fail even if I kick it again.

Also revert the other change to azure-pipelines.yml since it won't help for now. Sampler tasks must be updated too.

[dan-hughes]

Done. I set the --version to 5.* to allow any future updates available to v5.

[dan-hughes]

@johlju, what's a gold standard for class implementation and tests I can reference? I'm onto the classes and this uses it's own base class from the bottom up.
I've started inheriting from DscResource.Base, but it's significant change, but it brings many benefits and remove many duplicate functions and tests which are provided by the Base and Common modules.

[johlju]

A bit of a history... This repo started the journey to the module DscResource.Base with the work @Sudman1 started with implementing DsnRecordBase, that inspired ResourceBase that in turn inspired the module DscResource.Base. 🙂

DnsResource.Base is an opiniated way of created class-based resource and can't say that it is the gold standard, but that is what I use when writting class-based resource because as you said, to reduce duplication. The repo SqlServerDsc is where I personally made most work with class-based resource using that opiniated way. In that repo there is also a class SqlResourceBase that all class-based resource inherits from, and SqlResourceBase inherits ResourceBase (DscResource.Base).

II have no issue with class-based resource being modified to use the module DscResource.Base, but the existing can also be kept the way they are and ny class-based resource using another pattern like DscResource.Base. 🤔
Though, class-based resource using the the class ResourceBase that is part of this repo should probably be migrated to DscResource.Base. 🤔

[dan-hughes]

The DnsServerRecord tests are largely the same format, I added some tests that were missing.

I would it be better if I highlighted some that I have concerns about so I can apply any patterns/learning across the board?

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

---

source/DnsServerDsc.psm1 line at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should we remove this? I think this file is there so the .psd1 file has something to reference when it is parsed. It was something that failed when this file was removed, but that was a long time ago and the reason might no longer exist. Thinking if there is a scenarion when this file is used (imported/parsed) in a way that could fail, but can't think of one. So let us remove it and see if something fails down the line. 🙂

I had to remove this for the tests to work, I think when this existed HQRM tests failed.

I checked SqlServerDsc and this file is missing too.

---

source/Enum/1.Ensure.ps1 line 1 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

See you removed this then reverted it so it is kept, do we still need this here even though it is part of DscResource.Base?

Long story, I looked at converting all resources to inherit from DscResource.Base but the DnsServerRecord* resources is way more work so I will do that on another PR.

I can try removing, but this PR is so large that I didn't want to potentially break something else.

Want me to try or leave it to the next PR (which I have here partly complete)?

---

source/Classes/030.DnsServerCache.ps1 line 129 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why do we need all this code? Since DnsServer is a key property it will always be part of $properties. So can't we always set that directly? User can se the DsnServer property to localhost if they want. 🤔

Same comment throughout the resources.

I thought that it was because the property did not have a default value, but you make a good point that as it's a key property then it always will be defined. I shall fix.

[johlju]

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

---

source/DnsServerDsc.psm1 line at r7 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

I had to remove this for the tests to work, I think when this existed HQRM tests failed.

I checked SqlServerDsc and this file is missing too.

Hmm. So it is. Forgot we did that. Cool. All good then. 🙂

---

source/Enum/1.Ensure.ps1 line 1 at r4 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

Long story, I looked at converting all resources to inherit from DscResource.Base but the DnsServerRecord* resources is way more work so I will do that on another PR.

I can try removing, but this PR is so large that I didn't want to potentially break something else.

Want me to try or leave it to the next PR (which I have here partly complete)?

Leave it for the next PR. 🙂

---

source/Classes/030.DnsServerCache.ps1 line 101 at r7 (raw file):

    [DscProperty(NotConfigurable)]
    [Reason[]]

We need to make this class unique otherwise it can fail if two modules define the same class twice. See SqlServerDsc here:
https://github.com/dsccommunity/SqlServerDsc/blob/3c3092976409645d74f58707331f66ffe1967127/source/Classes/011.SqlResourceBase.ps1#L45-L47

And the uniquely class here: https://github.com/dsccommunity/SqlServerDsc/blob/main/source/Classes/001.SqlReason.ps1

And issue explained here: dsccommunity/DscResource.Base#4

This can be fixed in the next PR too.

[dan-hughes]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/Classes/030.DnsServerCache.ps1 line 101 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We need to make this class unique otherwise it can fail if two modules define the same class twice. See SqlServerDsc here:
https://github.com/dsccommunity/SqlServerDsc/blob/3c3092976409645d74f58707331f66ffe1967127/source/Classes/011.SqlResourceBase.ps1#L45-L47

And the uniquely class here: https://github.com/dsccommunity/SqlServerDsc/blob/main/source/Classes/001.SqlReason.ps1

And issue explained here: dsccommunity/DscResource.Base#4

This can be fixed in the next PR too.

Okay, what's the point of [Reasons] in DscResource.Base if each module needs to implement their own? Or is it for the functionality in DscResource.Base to populate the reason it has to exist?

Will it work if this module has a [DnsServerReasons] which inherits from [Reasons]? Or does it need to be it's own isolated implementation?

[johlju]

Sure you can do that.

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

---

source/Classes/030.DnsServerCache.ps1 line 101 at r7 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

Okay, what's the point of [Reasons] in DscResource.Base if each module needs to implement their own? Or is it for the functionality in DscResource.Base to populate the reason it has to exist?

Will it work if this module has a [DnsServerReasons] which inherits from [Reasons]? Or does it need to be it's own isolated implementation?

It cannot inherit, it must be its own implementation.

The reason for it being left in DscResource.Base is that the derived class can use an hashtable array instead. DscResource.base will convert to its own Reasons class. But this method has not been used by a resource yet what I know of. See "Suggested solution" in the issue above.

[DscProperty(NotConfigurable)]
[System.Collections.Hashtable[]]
$Reasons

[dan-hughes]

Reviewable status: 82 of 123 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/Classes/030.DnsServerCache.ps1 line 129 at r7 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

I thought that it was because the property did not have a default value, but you make a good point that as it's a key property then it always will be defined. I shall fix.

Done, but final one on this, keep the splat or save the variable for only one property?

[johlju]

Reviewed 41 of 41 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-hughes)

---

tests/Unit/DSC_DnsServerDiagnostics.Tests.ps1 line 539 at r8 (raw file):

            Set-TargetResource @testParameters
        }
        

Looks like we got some spaces here that Reviewable is indicating.

---

tests/Unit/Classes/DnsRecordCname.tests.ps1 line 467 at r8 (raw file):

                InModuleScope -ScriptBlock {
                    Set-StrictMode -Version 1.0
                    

Empty spaces here too.

---

tests/Unit/Classes/DnsServerScavenging.Tests.ps1 line 537 at r8 (raw file):

                $currentState.LastScavengeTime | Should -Be '2021-01-01 00:00:00'
            }
            

Empty spaces here too.

---

source/Classes/030.DnsServerCache.ps1 line 129 at r7 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

Done, but final one on this, keep the splat or save the variable for only one property?

I good either way. Using splatting makes the line a bit shorter than if we wouldn't.

[dan-hughes]

I checked most things and I think it's good for the time being. Up to you if you want to do any random spot checks?

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

---

tests/Unit/DSC_DnsServerDiagnostics.Tests.ps1 line 539 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Looks like we got some spaces here that Reviewable is indicating.

Weird, this one. Did not show up as spaces. I deleted all the line breaks and it now shows as a change in Git.

---

tests/Unit/Classes/DnsRecordCname.tests.ps1 line 467 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Empty spaces here too.

Done.

---

tests/Unit/Classes/DnsServerScavenging.Tests.ps1 line 537 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Empty spaces here too.

Done.

[johlju]

Did a read through on a integration test and unit test, they looked good. Just had some suggestions. But up to you if you want to improve them. I could LGTM this one now too.

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-hughes)

---

tests/Integration/Classes/DnsRecordCname.integration.tests.ps1 line 67 at r9 (raw file):

        BeforeAll {
            $configurationName = "$($script:dscResourceName)_CreateRecord_Config"
        }

Instead of duplicating the configurationName in both discovery phase and run. phase it is possible to get it from. the discovery phase by doing this:

https://github.com/dsccommunity/SqlServerDsc/blob/3c3092976409645d74f58707331f66ffe1967127/tests/Integration/Resources/DSC_SqlAudit.Integration.Tests.ps1#L58-L63

Code quote:

    BeforeDiscovery {
        $configurationName = "$($script:dscResourceName)_CreateRecord_Config"
    }

    Context ('When using configuration {0}' -f $configurationName) {
        BeforeAll {
            $configurationName = "$($script:dscResourceName)_CreateRecord_Config"
        }

---

tests/Unit/Classes/DnsServerCache.Tests.ps1 line 94 at r9 (raw file):

                Set-StrictMode -Version 1.0

                $script:instance = [DnsServerCache] @{

A suggestion is always prefix variables with mock, e,g. $script:mockInstance. This prevents setting a variable name that is actually used in classes by mistake, since we are setting the variable in the module scope and the modules script scope.

Code quote:

$script:instance

[dan-hughes]

@johlju, I don't mind doing it via this PR. Already got it open and they're minor. It also helps get a good reference for me or anyone else in the future. Keep an eye out over the next few days.

[dan-hughes]

Reviewable status: 89 of 123 files reviewed, all discussions resolved (waiting on @johlju)

---

tests/Integration/Classes/DnsRecordCname.integration.tests.ps1 line 67 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instead of duplicating the configurationName in both discovery phase and run. phase it is possible to get it from. the discovery phase by doing this:

https://github.com/dsccommunity/SqlServerDsc/blob/3c3092976409645d74f58707331f66ffe1967127/tests/Integration/Resources/DSC_SqlAudit.Integration.Tests.ps1#L58-L63

This was a much larger change than I originally thought :-).

[johlju]

Reviewed 34 of 34 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-hughes)

---

tests/Integration/Classes/DnsRecordCname.integration.tests.ps1 line 67 at r9 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…

This was a much larger change than I originally thought :-).

Yes, that is why I gave the option to not do it 😉 Awesome work!

Not for this PR, but.. A thought I had several times... Ultimately I think it would be great if we could pass in a hashtable array with all the configuration names and a key ExpectedResult that has a hashtable or PSCustomObject with the expected result. Then one Context-block could loop through all configurations and not need to duplicate all the It-blocks... But maybe it would also make the tests less readable when debugging so haven't put in the time. 🙂

This looks good now!

[johlju]

Awesome work on this! Thank you!

[dan-hughes]

Reviewed 34 of 34 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-hughes)

tests/Integration/Classes/DnsRecordCname.integration.tests.ps1 line 67 at r9 (raw file):

Previously, dan-hughes (Daniel Hughes) wrote…
Yes, that is why I gave the option to not do it 😉 Awesome work!

Not for this PR, but.. A thought I had several times... Ultimately I think it would be great if we could pass in a hashtable array with all the configuration names and a key ExpectedResult that has a hashtable or PSCustomObject with the expected result. Then one Context-block could loop through all configurations and not need to duplicate all the It-blocks... But maybe it would also make the tests less readable when debugging so haven't put in the time. 🙂

This looks good now!

I think it's a pick your poison situation. Yes the tests are verbose but they're flexible and easy enough for someone like me to read and pick up in most cases.
It's possibly an option here with class resources that all inherit from the same base class, as making broad changes like a Pester major version upgrade would be faster but it feels a little cleaner knowing only working on a resource could only break the tests for that resource.