`Should-Throw`: It is not possible using positional parameters when also using pipeline input

johlju commented 1 week ago

Checklist

[X] Issue has a meaningful title
[X] I have searched the existing issues. See all issues
[X] I have tested using the latest version of Pester. See Installation and update guide.

What is the issue?

When having a test like this in Pester 5 syntax, that is using positional parameters:

It 'Should throw using only positional parameters' {
    {
        Write-Error -Message 'MockErrorMessage' -ErrorId 'MockErrorId' -Category 'InvalidOperation' -TargetObject 'MockTargetObject' -ErrorAction 'Stop'
    } | Should -Throw 'MockErrorMessage' 'MockErrorId' ([Microsoft.PowerShell.Commands.WriteErrorException]) 'MockBecauseString'
}

Trying to convert it to Pester 6 syntax:

It 'Should throw when using pipeline input and the usage of positional parameters' {
    {
        Write-Error -Message 'MockErrorMessage' -ErrorId 'MockErrorId' -Category 'InvalidOperation' -TargetObject 'MockTargetObject' -ErrorAction 'Stop'
    } | Should-Throw ([Microsoft.PowerShell.Commands.WriteErrorException]) 'MockErrorMessage' 'MockErrorId' 'MockBecauseString'
}

Will fail with error:

[-] Should throw when using pipeline input and the usage of positional parameters 11ms (7ms|4ms)
    PSInvalidCastException: Cannot convert the "Microsoft.PowerShell.Commands.WriteErrorException" value of type "System.RuntimeType" to type "System.Management.Automation.ScriptBlock".
    ArgumentTransformationMetadataException: Cannot convert the "Microsoft.PowerShell.Commands.WriteErrorException" value of type "System.RuntimeType" to type "System.Management.Automation.ScriptBlock".
    ParameterBindingArgumentTransformationException: Cannot process argument transformation on parameter 'ScriptBlock'. Cannot convert the "Microsoft.PowerShell.Commands.WriteErrorException" value of type "System.RuntimeType" to type "System.Management.Automation.ScriptBlock".

This is due to that parameter ScriptBlock is expected to be the first positional parameter. As far as I can see this will be an issue when converting between Pester 5 to Pester 6 syntax and wanting to using pipeline input and using positional parameters.

Expected Behavior

With Pester 6 syntax be able to have a scriptblock as pipeline input and also be able to only use positional parameters.

Steps To Reproduce

Run this:

It 'Should throw when using pipeline input and the usage of positional parameters' {
    {
        Write-Error -Message 'MockErrorMessage' -ErrorId 'MockErrorId' -Category 'InvalidOperation' -TargetObject 'MockTargetObject' -ErrorAction 'Stop'
    } | Should-Throw ([Microsoft.PowerShell.Commands.WriteErrorException]) 'MockErrorMessage' 'MockErrorId' 'MockBecauseString'
}

Describe your environment

Pester version     : 6.0.0-alpha3 /Users/johlju/source/PesterConverter/output/RequiredModules/Pester/6.0.0/Pester.psm1  
PowerShell version : 7.4.3
OS version         : Unix 14.5.0

Possible Solution?

If the ScriptBlock positional parameter would be moved last that could solve this? Or, is using ScriptBlock as pipeline input something we should move away from with Pester 6 syntax?

fflaten commented 1 week ago

Thanks for feedback. Scriptblock is expected as pipeline input just like in v5.

Will have to look into the positional arguments compared to v5 behavior.

fflaten commented 1 week ago

If the ScriptBlock positional parameter would be moved last that could solve this?

Yeah I think this is the closest we can do. Maybe also move a few parameters to match v5, unless the changes were intentional (@nohwnd ?). E.g.

    param (
        [Parameter(ValueFromPipeline = $true, Mandatory = $true, Position = 5)]
        [ScriptBlock]$ScriptBlock,
        [Parameter(Position = 0)]
        [String]$ExceptionMessage,
        [Parameter(Position = 1)]
        [String]$FullyQualifiedErrorId,
        [Parameter(Position = 2)]
        [Type]$ExceptionType,
        [Parameter(Position = 3)]
        [String]$Because,
        [Switch]$AllowNonTerminatingError
    )

This will also be relevant for the future -HaveParameter and -Invoke replacements as they also have multiple parameters and have used positional parameters in our docs.

We avoided this issue in Should as -ActualValue was always provided as a named parameter internally to the assertions.

In general I'd recommend sticking with named parameters in these commands. Multiple positional parameters are hard to read for others.

fflaten commented 1 week ago

@johlju For the conversion be aware that -PassThru is removed and default

johlju commented 1 week ago

Maybe also move a few parameters to match v5

For the conversion module I'm currently writing I can shift these around, so not an issue there at least. 😊 It is only an issue for my module when there is a new positional parameter put before or between those that exist in the Pester 5 syntax, like ScriptBlock.

We avoided this issue in Should...

For Should-Be I had to switch things around since the positions are different between 5 and 6. See my notes here: https://github.com/johlju/PesterConverter/blob/2531f521897f50c6deeba370a5814b49eacc83a3/source/Private/Convert-ShouldBe.ps1#L28

In general I'd recommend sticking with named parameters in these commands.

Personally historically I have done a mix of positional and named parameters. I'm trying to support all scenarios I can think of in the conversion module (maybe not all edge cases). Also to be able to convert from positional to named parameters or vice versa (where positional is supported). In the end I'm hoping I can make a module that can be also be for example used ScriptAnalyzer to switch between named or positional depending on preference. Then we could have QA tests that make sure correct style is used (for example only named parameters). But that is a future project.

For the conversion be aware that -PassThru is removed and default

Good to know! I saw that in the syntax, but did not know it was the default now.

johlju commented 1 week ago

Will have to look into the positional arguments compared to v5 behavior.

@fflaten You are correct, ExceptionMessage should have first position in Pester 6 syntax too, because Should-Throw 'myMessage' works in Pester 5, but does not work in Pester 6 with Should-Throw 'myMessage', so can't convert it 1:1. For now I will assume (hope) positional parameters in v6 will be same as in v5. 🙂

pester / Pester