Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix Pester v4 installation for `Visual Studio 2017` image and use Pester v4 assertion operator syntax #892
Conversation
bergmeister
changed the title
bergmeister
changed the title
|
First, this is awesome! Thanks a bunch going through all these and updating the tests. There's really only one thing that has to be fixed: |
| { | ||
| nuget install Pester -Version 4.1.1 -source https://www.powershellgallery.com/api/v2 -outputDirectory "$Env:ProgramFiles\WindowsPowerShell\Modules\." -ExcludeVersion | ||
| if ($null -eq (Get-Module -ListAvailable PowershellGet)) |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
Member
Wouldn't it be possible to use Install-Module for either case? We'll find the latest version of Pester when we import.
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 20, 2018
•
Author
Collaborator
No because Install-Module (which is part of PowerShellGet) only shipped starting with WMF5, hence the special treatment for installing Pester in the WMF4 build.
| @@ -37,6 +46,7 @@ install: | |||
| build_script: | |||
| - ps: | | |||
| $PSVersionTable | |||
| (Get-Command Invoke-Pester).Version | |||
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
Member
this is part of the log, correct? Would get-module -listavailable pester provide more utility than a version being output?
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 20, 2018
Author
Collaborator
Yes, it is part of the log. If I remember correctly, Get-Module Pester -ListAvailable told me that Pester v3 and v4 was installed but only Get-Command Invoke-Pester gave me the 'proof' that it was still using v3 for some reason. Maybe using both commands is the best conclusion.
| @@ -39,7 +39,7 @@ Describe "Settings Class" { | |||
|
|
|||
| 'IncludeRules', 'ExcludeRules', 'Severity', 'RuleArguments' | ForEach-Object { | |||
| It ("Should return empty {0} property" -f $_) { | |||
| $settings.${$_}.Count | Should Be 0 | |||
| $settings.${$_}.Count | Should -Be 0 | |||
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
Member
there's a problem with ${$_} which is not quite right as it is referencing the variable literally named $_, let's use the test case functionality of pester:
It "Should return empty <name> property" -TestCases @(
@{ Name = "IncludeRules" }
@{ Name = "ExcludeRules" }
@{ Name = "Severity" }
@{ Name = "RuleArguments" }
) {
${settings}.${Name}.Count | Should -Be 0
}
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 20, 2018
•
Author
Collaborator
Well spotted. When applying the syntax changes, I did not pay much attention to the test cases themselves but I will certainly improve this one.
| @@ -91,27 +91,27 @@ Describe "Settings Class" { | |||
| } | |||
|
|
|||
| It "Should return 2 IncludeRules" { | |||
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
Member
hmmm, this doesn't seem right, I assume the number is right so the title of these should probably change
This comment has been minimized.
This comment has been minimized.
| $settings.CustomRulePath.Count | Should Be $rulePaths.Count | ||
| 0..($rulePaths.Count - 1) | ForEach-Object { $settings.CustomRulePath[$_] | Should be $rulePaths[$_] } | ||
| $settings.CustomRulePath.Count | Should -Be $rulePaths.Count | ||
| 0..($rulePaths.Count - 1) | ForEach-Object { $settings.CustomRulePath[$_] | Should -Be $rulePaths[$_] } |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
Member
wouldn't this be more easily expressed as:
for($i = 0; $i -lt $rulePaths.Count; $i++ ) {
$settings.CustomRulePath[$i] | Should -Be $rulePaths[$i]
}
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 20, 2018
Author
Collaborator
I find the current way actually quite neat and reasonably readable. If I had written it then it would look like something:
foreach ($i in 0..($rulePaths.Count-1)) {
$settings.CustomRulePath[$i] | Should -Be $rulePaths[$i]
}At the end I don't mind too much but I would rather not touch existing tests too much if they are generally fine. But up to you whatever you prefer. If you want to make the style of all tests consistent, then this work should probably be a separate PR.
| $violations[0].SuggestedCorrections[0].Text | Should Match $expectedText | ||
| Get-ExtentText $violations[0].SuggestedCorrections[0] $violationFilepath | Should Match "" | ||
| $violations[0].SuggestedCorrections[0].Text | Should -Match $expectedText | ||
| Get-ExtentText $violations[0].SuggestedCorrections[0] $violationFilepath | Should -Match "" |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 20, 2018
Author
Collaborator
Lol. Absolutely correct. As I said, I mainly applied some replace algorithms and checked at the end only that it corrects the right operators without looking too much at the tests. Will definitely fix that one
|
the test case example just needs a param statement in order to work. |
| @{ Name = "ExcludeRules" } | ||
| @{ Name = "Severity" } | ||
| @{ Name = "RuleArguments" } | ||
| ) { |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
Member
this needs a param statement to work, i believe
{
param ( $name )
${settings}.${Name}.Count | Should -Be 0
}
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 20, 2018
•
Author
Collaborator
yes, you are right. thanks for pointing it out, I should've spotted that myself instead of just copy-pasting your example. Why did we not get a NullReferenceException on .Count then?
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 20, 2018
•
Member
it's because property references don't provide null reference exceptions unless you're in strict mode. [int]$null.count will return 0 ($null.count will be null, and [int]$null is 0), but $null.GetType() will throw because it's a method invocation. Strict mode will keep that from happening.
PS> Set-StrictMode -Version 2
PS> $null.count
The property 'count' cannot be found on this object. Verify that the property exists.
At line:1 char:1
+ $null.count
+ ~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (:) [], PropertyNotFoundException
+ FullyQualifiedErrorId : PropertyNotFoundStrict
|
LGTM! |
bergmeister commentedFeb 16, 2018
•
edited
PR Summary
Closes #890
VS 2017image (got installed but was still using v3)-) toBe,Not,Throw, andMatchoperators (by going through all operators in the documentation of Pester'sShoulddocumentation.PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready