Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExpectedMessage comparison fails #1793

Open
indented-automation opened this issue Nov 28, 2020 · 7 comments
Open

ExpectedMessage comparison fails #1793

indented-automation opened this issue Nov 28, 2020 · 7 comments
Labels
Assertions For issues related with assertions

Comments

@indented-automation
Copy link
Contributor

General summary of the issue

Error message comparison fails.

Describe your environment

Pester version     : 5.1.0-beta1 C:\Users\Chris\Documents\PowerShell\Modules\Pester\5.1.0\Pester.psm1
PowerShell version : 7.1.0
OS version         : Microsoft Windows NT 10.0.18363.0

Steps to reproduce

{ @(1, 2) - 1 } | Should -Throw -ExpectedMessage "Method invocation failed because [System.Object[]] does not contain a method named 'op_Subtraction'."

Expected Behavior

The assertion should be successful.

Using try / catch to perform the same comparison is successful:

try {
    @(1, 2) - 1
} catch {
    $_.Exception.Message | Should -Be "Method invocation failed because [System.Object[]] does not contain a method named 'op_Subtraction'."
}

Current Behavior

The assertion fails.

@indented-automation indented-automation changed the title My descriptive bug title ExpectedMessage comparison fails Nov 28, 2020
@nohwnd
Copy link
Member

nohwnd commented Nov 28, 2020

Maybe because the comparison is using -like and [] is some kind of wildcard?

@indented-automation
Copy link
Contributor Author

Yep that's the one.

{ @(1, 2) - 1 } | Should -Throw -ExpectedMessage "Method invocation failed because ``[System.Object``[``]``] does not contain a method named 'op_Subtraction'."

Hmm.

@aolszowka
Copy link
Contributor

FWIW this is a breaking change between Pester v4 and Pester v5. Might be worth calling out more specifically than just the blurb of:

Should -Throw is using -like to match the exception message instead of .Contains.
Use * or any of the other -like wildcard to match only part of the message.

That already exists on the breaking changes page here: https://pester-docs.netlify.app/docs/migrations/breaking-changes-in-v5

This is the root cause of this person's issue on StackOverflow: https://stackoverflow.com/questions/65219744/pester-v5-1-should-throw-with-message-containing-square-brackets-fails

Pester v4

Import-Module Pester -MaximumVersion '4.10.1'

function Test-Throw {
    throw "I should throw a [message]"
}

Describe 'PesterThrowBug' {
    It 'Should Throw a Message' {
        { Test-Throw } | Should -Throw -ExpectedMessage 'I should throw a [message]'
    }
}

Output:

  [+] Should Throw a Message 2.14s

Pester v5

Import-Module Pester -MinimumVersion '5.0.0'

function Test-Throw {
    throw "I should throw a [message]"
}

Describe 'PesterThrowBug' {
    It 'Should Throw a Message' {
        { Test-Throw } | Should -Throw -ExpectedMessage 'I should throw a [message]'
    }
}

Output:

Starting discovery in 1 files.
Discovery finished in 8ms.
Running tests.
[-] PesterThrowBug.Should Throw a Message 10ms (9ms|1ms)
 Expected an exception, with message 'I should throw a [message]' to be thrown, but the message was 'I should throw a [message]'. from S:\Git\SimpleExamples\Pester\PesterThrowBug.ps1:5 char:5
     +     throw "I should throw a [message]"
     +     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 at { Test-Throw } | Should -Throw -ExpectedMessage 'I should throw a [message]', S:\Git\SimpleExamples\Pester\PesterThrowBug.ps1:10
 at <ScriptBlock>, S:\Git\SimpleExamples\Pester\PesterThrowBug.ps1:10
Tests completed in 98ms
Tests Passed: 0, Failed: 1, Skipped: 0 NotRun: 0

The error message is particularly unhelpful because the expected and but message was is identical.

Work Around

As mentioned above escaping the [] with the backtick works, but this seems weird especially if you're not using string interpolation:

Import-Module Pester -MinimumVersion '5.0.0'

function Test-Throw {
    throw "I should throw a [message]"
}

Describe 'PesterThrowBug' {
    It 'Should Throw a Message' {
        { Test-Throw } | Should -Throw -ExpectedMessage 'I should throw a `[message`]'
    }
}

Output:

Starting discovery in 1 files.
Discovery finished in 10ms.
Running tests.
[+] S:\Git\SimpleExamples\Pester\PesterThrowBug.ps1 103ms (7ms|88ms)
Tests completed in 107ms
Tests Passed: 1, Failed: 0, Skipped: 0 NotRun: 0

@aolszowka
Copy link
Contributor

Here's a search I wrote up to identify affected locations that might be useful to someone else looking to perform a lift:

function Get-ExpectedMessageUsage {
    [CmdletBinding()]
    param (
        [Parameter(ValueFromPipeline)]
        $Path
    )
    process {
        $scriptContent = Get-Content -Path $Path -Raw
        $scriptAst = [ScriptBlock]::Create($scriptContent).Ast
        $possibleAffectedLocations = $scriptAst.FindAll({ $args[0] -is [System.Management.Automation.Language.CommandAst] }, $true) |
            Where-Object { $_.GetCommandName() -eq 'Should' } |
            Where-Object { $null -ne ($_ | Select-Object -ExpandProperty CommandElements | Select-Object -ExpandProperty ParameterName -ErrorAction SilentlyContinue | Where-Object { $_ -eq 'ExpectedMessage' }) }
        # It is only an affected location if it contains a Wild Card Character
        # as defined here: https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_wildcards?view=powershell-7.1
        # that is not escaped.
        $affectedLocations = $possibleAffectedLocations |
            Select-Object -ExpandProperty CommandElements |
            Where-Object { ($_.StringConstantType -eq 'DoubleQuoted' -or $_.StringConstantType -eq 'SingleQuoted') -and ([Regex]::Match($($_.Value), '[^`][\[\]\*\?]')) }
        [PSCustomObject]@{
            Path = $Path
            AffectedCount = ($affectedLocations | Measure-Object).Count
        }
    }
}

@nohwnd
Copy link
Member

nohwnd commented Jul 23, 2021

Reopening this because there are multiple comments, and I don't think I can move them all into a new comment.

There are few options we have:

  • In 5.4 I would like to add diagnostic logging to Should and it's assertions, we could check the message for [ ] and some other problematic -like wildcards if there are any (e.g. ? is imho also a wild card, but because hello? will match hello. people are unlikely to see it a error.), and log that into the diag log with the suggested solution. This is how we do it in the Mock, and it gives a lot of flexibility, because we can be very verbose in that log, and we also have the luxury of doing more expensive checks (e.g. checking if the string contains [.

  • Add -ExpectedMessageLike alias to -ExpectedMessage, and also -ExpectedMessageEq parameter that would do the -eq check. Or more in line with -Be, we would add -ExpectedMessageExactly instead of -ExpectedMessageEq. But this has discoverability problem I think.

@nohwnd nohwnd reopened this Jul 23, 2021
@aolszowka
Copy link
Contributor

Reopening this because there are multiple comments, and I don't think I can move them all into a new comment.

My bad :( I waffled back and forth between opening a new issue or not.

I would like to add diagnostic logging to Should and it's assertions

Sounds great to me.

? is imho also a wild card, but because hello? will match hello.

I completely agree with this; even more so for * because it will match both 2*2 and 2*2*2 the same way. The AST search above checks for all Wildcard characters to cover this.

Add -ExpectedMessageLike alias to -ExpectedMessage, and also -ExpectedMessageEq parameter that would do the -eq check. Or more in line with -Be, we would add -ExpectedMessageExactly instead of -ExpectedMessageEq. But this has discoverability problem I think.

I would vote for this personally, while more verbose also avoids the footgun.

@fflaten
Copy link
Collaborator

fflaten commented Aug 3, 2022

  • In 5.4 I would like to add diagnostic logging to Should and it's assertions, we could check the message for [ ] and some other problematic -like wildcards if there are any (e.g. ? is imho also a wild card, but because hello? will match hello.

Correct, []*?. There's also a helper-method to detect it [System.Management.Automation.WildcardPattern]::ContainsWildcardCharacters('your?string') (need full type-name for PSv3)

Add -ExpectedMessageLike alias to -ExpectedMessage,..

If this road is chosen we'll have to add aliases to operator-parameters. For some reason we check that parameter-aliases don't conflict with other operators, but never add the parameter alias so kinda wasted. 🤷‍♂️ (Would have to extend that test for parameter-aliases)

One disadvantage with parameter-alias in our case is that they're not linked to parameter-sets, so they would also apply for other operators using the same ExpectedMessage parameter-name. Only -Throw using it in Pester, but possibly custom operators.

I'd personally vote for the wildcardpattern-check + diag-log mentioning it and how to escape []*? with backtick for now and revisit ExpectedMessageEq etc with next-gen assertions.

Update: Or add a -UseExactMessage/NoWildcard/SimpleMatch switch to use -eq.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Assertions For issues related with assertions
Projects
None yet
Development

No branches or pull requests

4 participants