Fix Pester v4 installation for `Visual Studio 2017` image and use Pester v4 assertion operator syntax #892

bergmeister · 2018-02-16T23:10:33Z

PR Summary

Closes #890

Fix Pester v4 installation that did not work for VS 2017 image (got installed but was still using v3)
Change assertion operators to Pester 4 syntax by pre-pending the dash (-) to Be, Not, Throw, and Match operators (by going through all operators in the documentation of Pester's Should documentation.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
NA User facing documentation needed
Change is not breaking
NA Make sure you've added a new test if existing tests do not effectively test the code changed
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

…in VS2017 image

…ses for invoke-pester

JamesWTruher

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: ${$_} refers to the variable literally named $_ so this would rather be ${_}. But since you're going to change this, it's an opportunity to update the test to use testcases which might be a better way to go!

JamesWTruher · 2018-02-20T18:14:59Z

appveyor.yml

        {
-            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))


Wouldn't it be possible to use Install-Module for either case? We'll find the latest version of Pester when we import.

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.

JamesWTruher · 2018-02-20T18:17:19Z

appveyor.yml

@@ -37,6 +46,7 @@ install:
 build_script:
    - ps: |
        $PSVersionTable
+        (Get-Command Invoke-Pester).Version


this is part of the log, correct? Would get-module -listavailable pester provide more utility than a version being output?

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.

JamesWTruher · 2018-02-20T19:01:16Z

Tests/Engine/Settings.tests.ps1

@@ -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


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 }

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.

JamesWTruher · 2018-02-20T19:02:49Z

Tests/Engine/Settings.tests.ps1

@@ -91,27 +91,27 @@ Describe "Settings Class" {
        }

        It "Should return 2 IncludeRules" {


hmmm, this doesn't seem right, I assume the number is right so the title of these should probably change

I agree. I will make it a variable to future proof.

JamesWTruher · 2018-02-20T19:05:19Z

Tests/Engine/Settings.tests.ps1

-            $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[$_] }


wouldn't this be more easily expressed as:

for($i = 0; $i -lt $rulePaths.Count; $i++ ) { $settings.CustomRulePath[$i] | Should -Be $rulePaths[$i] }

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.

JamesWTruher · 2018-02-20T19:07:40Z

Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1

-            $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 ""


isn't this Should -BeNullOrEmpty?

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

…nalyzer into pester4Syntax

JamesWTruher

the test case example just needs a param statement in order to work.

JamesWTruher · 2018-02-20T22:05:29Z

Tests/Engine/Settings.tests.ps1

+            @{ Name = "ExcludeRules" }
+            @{ Name = "Severity" }
+            @{ Name = "RuleArguments" }
+        ) {


this needs a param statement to work, i believe

{ param ( $name ) ${settings}.${Name}.Count | Should -Be 0 }

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?

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

…nalyzer into pester4Syntax

JamesWTruher

LGTM!

bergmeister added 8 commits February 16, 2018 19:02

Pester 3 -> 4 syntax: 'Should Be' -> 'Should -Be'

9d55622

Pester 3 -> 4 syntax: 'Should Not' -> 'Should -Not'

4222577

Remaining '* -Be*' fixes for Pester v4

54eef94

Pester 3 -> 4: Match -> -Match

9e0076e

Pester 3 -> 4: Throw -> -Throw

242a90f

revert minor mistakes

ad3cadb

more minor reverts

be1bc04

final tweaks for a clean diff

591efd9

bergmeister added the in progress label Feb 16, 2018

bergmeister requested a review from JamesWTruher February 16, 2018 23:14

bergmeister self-assigned this Feb 16, 2018

bergmeister changed the title ~~Use Pester v4 assertion operator syntax~~ WIP Use Pester v4 assertion operator syntax Feb 16, 2018

bergmeister closed this Feb 16, 2018

bergmeister removed the in progress label Feb 16, 2018

bergmeister reopened this Feb 16, 2018

bergmeister added the in progress label Feb 16, 2018

bergmeister added 5 commits February 16, 2018 23:33

add debugging line to figure out why pester v4 does not seem to work …

61cba9a

…in VS2017 image

add another debugging line to figure out which version of pester it u…

93267af

…ses for invoke-pester

try to install pester v4 in a different way

7592fb5

fix wmf4 build

6d429cd

fix wmf4 syntax

b15ffd7

bergmeister changed the title ~~WIP Use Pester v4 assertion operator syntax~~ Fix Pester v4 installation for Visual Studio 2017 image and use Pester v4 assertion operator syntax Feb 17, 2018

bergmeister added Area - Build and removed in progress labels Feb 17, 2018

JamesWTruher suggested changes Feb 20, 2018

View reviewed changes

bergmeister added 2 commits February 20, 2018 19:35

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

7c51559

…nalyzer into pester4Syntax

apply PR suggestions

61ad0a2

JamesWTruher reviewed Feb 20, 2018

View reviewed changes

bergmeister added 2 commits February 20, 2018 22:08

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

36098ca

…nalyzer into pester4Syntax

fix test case syntax (missing param block)

39b81a8

JamesWTruher approved these changes Feb 20, 2018

View reviewed changes

JamesWTruher merged commit 6aeb4f6 into PowerShell:development Feb 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Pester v4 installation for `Visual Studio 2017` image and use Pester v4 assertion operator syntax #892

Fix Pester v4 installation for `Visual Studio 2017` image and use Pester v4 assertion operator syntax #892

bergmeister commented Feb 16, 2018 •

edited

Loading

JamesWTruher left a comment

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018 •

edited

Loading

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018 •

edited

Loading

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018

JamesWTruher left a comment

JamesWTruher Feb 20, 2018

bergmeister Feb 20, 2018 •

edited

Loading

JamesWTruher Feb 20, 2018 •

edited

Loading

JamesWTruher left a comment

		@@ -91,27 +91,27 @@ Describe "Settings Class" {
		}

		It "Should return 2 IncludeRules" {

Fix Pester v4 installation for Visual Studio 2017 image and use Pester v4 assertion operator syntax #892

Fix Pester v4 installation for Visual Studio 2017 image and use Pester v4 assertion operator syntax #892

Conversation

bergmeister commented Feb 16, 2018 • edited Loading

PR Summary

PR Checklist

JamesWTruher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

Fix Pester v4 installation for `Visual Studio 2017` image and use Pester v4 assertion operator syntax #892

Fix Pester v4 installation for `Visual Studio 2017` image and use Pester v4 assertion operator syntax #892

bergmeister commented Feb 16, 2018 •

edited

Loading

bergmeister Feb 20, 2018 •

edited

Loading

bergmeister Feb 20, 2018 •

edited

Loading

bergmeister Feb 20, 2018 •

edited

Loading

JamesWTruher Feb 20, 2018 •

edited

Loading