dsccommunity / dsccommunity.org

DSC community organisation's website
https://dsccommunity.org
MIT License
43 stars 21 forks source link

Blog: Convert tests to Pester 5 for a DSC Community repository #160

Closed johlju closed 3 years ago

johlju commented 3 years ago

This change is Reviewable

PlagueHO commented 3 years ago

Let me know when you're ready for review on this one @johlju !

johlju commented 3 years ago

@PlagueHO I only have one section left, then I need to go through already converted test to see if there anything there to raise that I missed. I hope this is ready by tomorrow night. Writing it as more of a guideline than step-by-step. Writing it as a step-by-step is not possible because there are many variations.

johlju commented 3 years ago

@Fiander

In regards to -Because. You are welcome to use it but if so I think it should give more information than the It-block description. For example if the It-block description in your example above is 'Should not throw and return the correct values for the properties' then I don't think the Because brings brings more value. If Because is used I think it should add more information to why the value must be correct, that is give more information than what the It-block description gives. πŸ€” Also when using Because the text should be phrased so it reads correct in the Pester output. For example the following:

$result | Should -Be 'MyAlertName2' -Because 'the correct alert name must be returned when the alert does not exist so user knows what alert failed'

Would result in the following that reads naturally:

Expected strings to be the same, because the correct alert name must be returned when the alert does not exist so user knows what alert failed, but they were different.
 Expected length: 12
 Actual length:   11
 Strings differ at index 11.
 Expected: 'MyAlertName2'
 But was:  'MyAlertName'

I can add this to the blog post.

In regards to your questions regarding exceptions and localized strings I will cover that in the section I didn't had time to complete yesterday. πŸ™‚ But short story, since you are in the module scope you have access to $script:localizedData, e.g. $script.localizedData.FailedToGetSqlServerProtocol so you can get the expected error message in the test. You also need to use Should -Throw -ExpectedMessage ($mockErrorRecord.Exception.Message + '*'), but you can use your suggestion too "$($mockErrorRecord.Exception.Message)*". I agree the * is necessary because Pester 5 does not longer does a "contains" check on the error message, but instead a "like" comparison.

I think we should get the test helper function Get-InvalidOperationRecord and Get-InvalidResultRecord to add the wildcard * to the message. Maybe two parameters WildcardPrefix and WildcardSuffix. We can do that when we move the test helper function (CommonTestHelper) to DscResource.Test.

johlju commented 3 years ago

@PlagueHO Could you please review (proof read) when you have time. If it is very much changes and if it is faster you are welcome to push commits to the working branch. πŸ™‚ Happy to to add sections if we are missing something.

@Fiander you have new sections around asserting exceptions now. πŸ˜ƒ

johlju commented 3 years ago

@PlagueHO some minor grammar fixed thanks to my girlfriend. Hopefully we proactively squashed a few potential review comments. πŸ˜„

johlju commented 3 years ago

@PlagueHO thanks for all the review comments! Rephrased a couple of sections as per your suggestion. Ready for review again. πŸ˜ƒ Happy new year! πŸŽ†

PlagueHO commented 3 years ago

Happy New Year! I'll review later on today!

johlju commented 3 years ago

@PlagueHO ready for sign off now πŸ™‚