Closed johlju closed 7 years ago
msftclas
commented
7 years ago @johlju, Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request. Thanks, Microsoft Pull Request Bot
codecov-io
commented
7 years ago Merging #108 into dev will increase coverage by
8%. The diff coverage is100%.
@@ Coverage Diff @@
## dev #108 +/- ##
==================================
+ Coverage 90% 98% +8%
==================================
Files 7 7
Lines 379 378 -1
==================================
+ Hits 343 374 +31
+ Misses 36 4 -32
johlju
commented
7 years ago @bgelens And finally the last unit test. Whenever you have to time to review this one.
johlju
commented
7 years ago @PlagueHO do you mind to review this one if you have a chance?
PlagueHO
commented
7 years ago @johlju - will get onto this tonight after work! A pleasure to help!
PlagueHO
commented
7 years ago @Johlju - looking pretty good - just some really minor nitpicks.
Reviewed 1 of 4 files at r1, 3 of 3 files at r2. Review status: all files reviewed at latest revision, 7 unresolved discussions.
CHANGELOG.md, line 25 at r2 (raw file):
- Refactored the unit test for this resource to use stubs and increase coverage ([issue #78](https://github.com/PowerShell/xFailOverCluster/issues/78)). - Now the Test-TargetResource correctly returns false if the domain namn cannot
spelling error here: namn => name
DSCResources/MSFT_xWaitForCluster/MSFT_xWaitForCluster.psm1, line 34 at r2 (raw file):
) @{
Not required, but I think it is better to use add a return as it makes it clearer.
DSCResources/MSFT_xWaitForCluster/MSFT_xWaitForCluster.psm1, line 173 at r2 (raw file):
} $testTargetResourceReturnValue
Not required, but I think it is better to use add a return as it makes it clearer.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 96 at r2 (raw file):
Mock -CommandName Get-CimInstance -MockWith $mockGetCimInstance -ParameterFilter $mockCimInstance_ParameterFilter -Verifiable { Set-TargetResource @mockDefaultParameters } | Should -Throw ('Failover cluster {0} not found after {1} attempts with {2} sec interval' -f $mockClusterName, ($mockRetryCount-1), $mockRetryIntervalSec)
Not something you have to change, but why include the -? Should Be and Should BeOfType are used earlier, but Should -Throw and Should -Not -Throw format are used. Just a minor consistency thing.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 108 at r2 (raw file):
Context 'When Get-Cluster throws an error' { BeforeEach { # This is used for the evaluation of that cluster do not exist.
Nitpic: # This is used for the evaluation of a cluster that does not exist.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 161 at r2 (raw file):
Context 'When Get-Cluster throws an error' { BeforeEach { # This is used for the evaluation of that cluster do not exist.
Nitpic: # This is used for the evaluation of a cluster that does not exist.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 193 at r2 (raw file):
Assert-VerifiableMocks } }
} should be on the next line I think.
Comments from Reviewable
johlju
commented
7 years ago Review status: 2 of 5 files reviewed at latest revision, 7 unresolved discussions.
CHANGELOG.md, line 25 at r2 (raw file):
spelling error here: namn => name
Done. Thanks! That was the Swedish word for name ;)
DSCResources/MSFT_xWaitForCluster/MSFT_xWaitForCluster.psm1, line 34 at r2 (raw file):
Not required, but I think it is better to use add a `return` as it makes it clearer.
I tend to agree. I normally do that. But @bgelens like us to not use it unless it it is used to exit a code path. So all other resource are not using 'return' for these. So I rather keep this as-is as is to be consequent. If we get a guideline to use return for these, then we change them back. :) I saw Katie asked to get return on these as well in a PR in another repo.
DSCResources/MSFT_xWaitForCluster/MSFT_xWaitForCluster.psm1, line 173 at r2 (raw file):
Not required, but I think it is better to use add a `return` as it makes it clearer.
Same as previous comment.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 96 at r2 (raw file):
Not something you have to change, but why include the `-`? `Should Be` and `Should BeOfType` are used earlier, but `Should -Throw` and `Should -Not -Throw` format are used. Just a minor consistency thing.
Done. That's me trying to use the new style of Pester 4.x outside of these repos, and it snuck in. Fixed this an all others in the repo.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 108 at r2 (raw file):
Nitpic: `# This is used for the evaluation of a cluster that does not exist.`
Done. Thanks for proof reading! :smile:
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 161 at r2 (raw file):
Nitpic: `# This is used for the evaluation of a cluster that does not exist.`
Done. Thanks for proof reading! :smile:
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 193 at r2 (raw file):
`}` should be on the next line I think.
Done.
Comments from Reviewable
PlagueHO
commented
7 years ago All looking good!
Reviewed 3 of 3 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions.
CHANGELOG.md, line 25 at r2 (raw file):
Done. Thanks! That was the Swedish word for name ;)
Oh right! I wish I was multilingual! :grin: (listening to lots of Scandinavian extreme metal doesn't give me a good understanding of Swedish).
DSCResources/MSFT_xWaitForCluster/MSFT_xWaitForCluster.psm1, line 34 at r2 (raw file):
I tend to agree. I normally do that. But @bgelens like us to not use it unless it it is used to exit a code path. So all other resource are not using 'return' for these. So I rather keep this as-is as is to be consequent. If we get a guideline to use return for these, then we change them back. :) I saw Katie asked to get return on these as well in a PR in another repo.
All good! There is a good discussion thread on this over at the PoSHCode PowerShellPracticeAndStyle repo - recommended reading on this topic: https://github.com/PoshCode/PowerShellPracticeAndStyle/issues/46#issuecomment-236727676
It generally agrees with what @bgelens is saying, but I think the difference is interpretation of whether or not the last line of the function is "ending execution". I guess it depends on your coding background - I've come from a C/C++ background and so having a return feels more explicit and clear. But that is just me.
Anyway, not and issue here.
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 96 at r2 (raw file):
Done. That's me trying to use the new style of Pester 4.x outside of these repos, and it snuck in. Fixed this an all others in the repo.
Oh! I hadn't seen that style... I didn't realize. I better look into it :grin:
Comments from Reviewable
johlju
commented
7 years ago Reviewed 1 of 4 files at r1, 1 of 3 files at r2, 3 of 3 files at r3. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.
CHANGELOG.md, line 25 at r2 (raw file):
Oh right! I wish I was multilingual! :grin: (listening to lots of Scandinavian extreme metal doesn't give me a good understanding of Swedish).
No wonder, since Scandinavian metal music is in English, Swedish, Norwegian and Danish and probably Sami. :wink: Listen to metal band Lillasyster if you haven't, they sing in Swedish. They also has made the best cover (IMHO) of Umbrella (only English song). :smile:
DSCResources/MSFT_xWaitForCluster/MSFT_xWaitForCluster.psm1, line 34 at r2 (raw file):
All good! There is a good discussion thread on this over at the PoSHCode PowerShellPracticeAndStyle repo - recommended reading on this topic: https://github.com/PoshCode/PowerShellPracticeAndStyle/issues/46#issuecomment-236727676 It generally agrees with what @bgelens is saying, but I think the difference is interpretation of whether or not the last line of the function is "ending execution". I guess it depends on your coding background - I've come from a C/C++ background and so having a return feels more explicit and clear. But that is just me. Anyway, not and issue here.
I come from C/C++/C# as well, and I really like ending with return. :smile:
Tests/Unit/MSFT_xWaitForCluster.Tests.ps1, line 96 at r2 (raw file):
Oh! I hadn't seen that style... I didn't realize. I better look into it :grin:
I tried to find a link top a discussion I saw around it over at Pester repo. But didn't find it. They changed it to use -Be etc. to be able to introduce more parameters if I understood it correctly, because having them set by position was somewhat limiting.
Comments from Reviewable
Pull Request (PR) description
This Pull Request (PR) fixes the following issues: Fixes #78 Fixes #107 Fixes #54
Task list:
This change is