WIP: Use new Pester syntax: -Parameter for Modules/Microsoft.PowerShell.LocalAccounts tests by KevinMarquette · Pull Request #6499 · PowerShell/PowerShell

KevinMarquette · 2018-03-26T08:02:36Z

PR Summary

Use new Pester syntax: -Parameter for Pester in Modules/Microsoft.PowerShell.LocalAccounts
Resolves part of #6245

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- [NA] User-facing Documentation needed
  - [NA] Issue filed - Issue link:
Testing - New and feature
- Not Applicable or can only be tested interactively
- OR
- [NA] Make sure you've added a new test if existing tests do not effectively test the code changed
  - [NA] Add [feature] if the change is significant or affects feature tests

KevinMarquette · 2018-03-26T08:05:30Z

...Microsoft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroup.Tests.ps1

-    }
-    catch {
-        $_.FullyQualifiedErrorId | Should Be $expectedFqeid
+        {& $sb} | Should -Throw -ErrorId $expectedFqeid


I'm not sure if this is the best way to refactor this with the use of try/finally here to toggle ErrorActionPreference. There is a hand full of instances of this pattern in this file

I believe we could do:

{ $script:ErrorActionPreference = "Stop" try { & $sb } finally { $script:ErrorActionPreference = $backupEAP } } Should -Throw -ErrorId $expectedFqeid

KevinMarquette · 2018-03-26T08:12:07Z

...Microsoft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroup.Tests.ps1

            {
                # Force failing the test because an unexpected outcome occurred
-                $false | Should Be $true
+                $false | Should -BeTrue


Can I remove the if/else and just $result | Should -Not -BeNullOrEmpty instead

KevinMarquette · 2018-03-26T08:14:02Z

...Microsoft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroup.Tests.ps1


            $finalCount = (Get-LocalGroup).Count
-            $initialCount -eq $finalCount + 1 | Should Be true
+            $initialCount | Should -Be ($finalCount + 1)


Is this an appropriate refactor to add to this pull request?

KevinMarquette · 2018-03-26T08:30:17Z

@iSazonov I have a few questions on this PR that I called out in the code before someone does a full review.

Overal, there was a lot of left side logic that was piping to | Should be true. Lots of counts, matches and contains that all have Pester support now, so I did those refactor this time.

iSazonov · 2018-03-26T09:08:15Z

...Microsoft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroup.Tests.ps1

-                $result[2] -match "TestGroupGet1" | Should Be true
+                $result[0] | Should -Be 1
+                $result[1] | Should -Match "GroupNotFound"
+                $result[2] | Should -match "TestGroupGet1"


-match -> -MatchExactly

iSazonov · 2018-03-26T09:12:05Z

...oft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroupMember.Tests.ps1

-    }
-    catch {
-        $_.FullyQualifiedErrorId | Should Be $expectedFqeid
+        {& $sb} | Should -Throw -ErrorId $expectedFqeid


The same pattern as above.

iSazonov · 2018-03-26T09:14:22Z

...oft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroupMember.Tests.ps1

-            $result[0].Name -match ($OptDomainPrefix + "TestUser1") | Should Be $true
-            $result[1].Name -match ($OptDomainPrefix + "TestUser2") | Should Be $true
+            $result[0].Name | Should -Match ($OptDomainPrefix + "TestUser1")
+            $result[1].Name | Should -Match ($OptDomainPrefix + "TestUser2")


-MatchExactly for strings for all commits.

iSazonov · 2018-03-26T09:16:58Z

.../Microsoft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalUser.Tests.ps1

-    }
-    catch {
-        $_.FullyQualifiedErrorId | Should Be $expectedFqeid
+        {& $sb} | Should -Throw -ErrorId $expectedFqeid


The same pattern as above

daxian-dbw · 2018-03-26T22:02:05Z

@adityapatwardhan Do we even run LocalAccount tests? I think we disabled building the assembly, and thus we shouldn't be running the tests anymore ...

adityapatwardhan · 2018-03-26T23:02:31Z

@daxian-dbw Yes, you are correct, we do not run these tests anymore.

SteveL-MSFT · 2018-03-27T05:53:05Z

@KevinMarquette I appreciate you taking the time to clean up these tests, however, to validate the changes are correct, you'll need to run them on Windows PowerShell 5.1 where the LocalAccounts module still exists. These tests all have a return statement early in the script so none of the tests actually run.

KevinMarquette · 2018-03-27T13:24:09Z

I was relying on the CI triggers for the build. I didn't even notice the return statement.

stale · 2018-04-26T13:47:31Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

iSazonov · 2018-04-26T14:43:02Z

Up.

SteveL-MSFT · 2018-04-26T17:01:44Z

...Microsoft.PowerShell.LocalAccounts/Pester.Command.Cmdlets.LocalAccounts.LocalGroup.Tests.ps1

-            }
-            catch {
-                $_.FullyQualifiedErrorId | Should Be "ParameterArgumentValidationError,Microsoft.PowerShell.Commands.NewLocalGroupCommand"
+                {$shouldBeNull = New-LocalGroup -Name $name -Description $desc} |


NIT: whitespace after opening brace and before closing brace

stale · 2018-05-26T17:52:54Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

stale · 2018-06-05T18:50:00Z

This PR has be automatically close because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it.
Thanks again for your contribution.
Community members are welcome to grab these works.

KevinMarquette added 12 commits March 24, 2018 13:25

Should -Contain

569e697

Be $null -> -BeNullOrEmpty

3b46790

Should -BeExactly

7c11742

Should -Match

2976c63

-Not -BeNullOrEmpty

d829565

-BeExactly 'User'

9d38ff0

-BeExactly

085e986

-BeTrue and -BeFalse

161e8bc

Should -Be

8be84f7

-HaveCount and -Contain

2f8e9be

Should -Throw

6291046

Should -Be

c65ff5f

KevinMarquette commented Mar 26, 2018

View reviewed changes

-BeGreaterThan

d30c6b4

KevinMarquette mentioned this pull request Mar 26, 2018

Clean up test code to use -Parameter syntax for Pester tests #6245

Closed

20 tasks

iSazonov reviewed Mar 26, 2018

View reviewed changes

iSazonov requested review from SteveL-MSFT and adityapatwardhan March 26, 2018 09:35

KevinMarquette added 2 commits March 26, 2018 20:12

-MatchExactly

76a3fa5

Should -Throw

c83a9fb

iSazonov approved these changes Mar 27, 2018

View reviewed changes

iSazonov assigned adityapatwardhan Mar 27, 2018

KevinMarquette changed the title ~~WIP: Use new Pester syntax: -Parameter for Modules/Microsoft.PowerShell.LocalAccounts tests~~ Use new Pester syntax: -Parameter for Modules/Microsoft.PowerShell.LocalAccounts tests Mar 27, 2018

KevinMarquette changed the title ~~Use new Pester syntax: -Parameter for Modules/Microsoft.PowerShell.LocalAccounts tests~~ WIP: Use new Pester syntax: -Parameter for Modules/Microsoft.PowerShell.LocalAccounts tests Mar 27, 2018

stale bot added the Stale label Apr 26, 2018

stale bot removed the Stale label Apr 26, 2018

SteveL-MSFT reviewed Apr 26, 2018

View reviewed changes

stale bot added the Stale label May 26, 2018

stale bot closed this Jun 5, 2018

xtqqczze mentioned this pull request Jan 10, 2020

Update pester syntax to v4 #11544

Merged

8 tasks

Conversation

KevinMarquette commented Mar 26, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinMarquette commented Mar 26, 2018

Uh oh!

iSazonov Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Mar 26, 2018

Uh oh!

adityapatwardhan commented Mar 26, 2018

Uh oh!

SteveL-MSFT commented Mar 27, 2018

Uh oh!

KevinMarquette commented Mar 27, 2018

Uh oh!

stale bot commented Apr 26, 2018

Uh oh!

iSazonov commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented May 26, 2018

Uh oh!

stale bot commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iSazonov Mar 26, 2018 •

edited

Loading

iSazonov Mar 26, 2018 •

edited

Loading