"Should -HaveParameter 'param' -Not -Mandatory" passes tests if parameter is not present

[enoorden]

General summary of the issue

"Should -HaveParameter 'param' -Not -Mandatory" passes tests if parameter is not present

Describe your environment

Pester version : 5.3.1 C:\Users___\Documents\PowerShell\Modules\pester\5.3.1\Pester.psm1
PowerShell version : 7.2.0
OS version : Microsoft Windows NT 10.0.19043.0

Steps to reproduce

function hi {
    param(
        [parameter(mandatory)]
        [string]$name
    )
    "hi $name"
}

Context 'ctx' {
    It 'test missing param' {
        $fnc = Get-Command 'hi'
        $fnc | Should -HaveParameter 'anotherParam' -Not -Mandatory
    }
}

Context ctx
  [+] test missing param 6ms (3ms|3ms)

Expected Behavior

would expect the test to fail.

I can see how a parameter which is not declared is actually not mandatory. But i would still expect the 'Should -HaveParameter' to first check the actual presence of the parameter.

Current Behavior

test succeeds

Possible Solution? (optional)

use two tests

Context 'ctx' {
    It 'test missing param' {
        $fnc = Get-Command 'hi'
        $fnc | Should -HaveParameter 'anotherParam'
        $fnc | Should -HaveParameter 'anotherParam' -Not -Mandatory
    }
}

 [-] test missing param 17ms (8ms|9ms)
   Expected command hi to have a parameter anotherParam, but the parameter is missing.

[nohwnd]

Thanks for reporting this, @lipkau wanna fix this?

[fflaten]

So only Should -Not -HaveParameter abc (no other parameters) should pass if the parameter is actually missing?

While if -Alias/Mandatory/etc is also used we expect the parameter itself to exist, but not the remaining criterias?

I both understand it and find it a bit confusing. Would you have expected the behavior your describe if the code was $fnc | Should -Not -HaveParameter 'anotherParam' -Mandatory? Remember that the position of -Not is irrelevant for a PowerShell function.

[nohwnd]

I agree the syntax is confusing. Only now that you spelled it out I realized what the problem is. Should -NotHaveParameter and Should -HaveParameter -NotMandatory would probably be better ways express this (if Should -HaveParameter, and implied non-mandatory by not having that switch.

I guess there is nothing that can be fixed now, and we just need to remember to do it like that for Should that is based on distinct functions. Should-HaveParameter, and Should-NotHaveParameter (this one would not have the -Mandatory switch at all).

[enoorden]

Just adding -NotMandatory would fix the problem.

It just doesn't make sense to test for a parameter to not exist and at the same time test whether it is mandatory or not or if it has an alias. And you wouldn't test for a function not having an alias, you would test for it having the alias you want

so i guess only Mandatory/NotMandatory would be needed to cover all logical use cases

[fflaten]

Just adding -NotMandatory would fix the problem.

I agree. We're unable block -NotMandatory -Mandatory now due to how Should works, but I'll add this to the milestone for new Should-functions in the future.

[johlju]

We're unable block -NotMandatory -Mandatory now due to how Should works

Would it work saying Should -HaveParameter -Mandatory:$false 🤔

[fflaten]

Would it work saying Should -HaveParameter -Mandatory:$false 🤔

Atm. no, but it's a valid suggestion. We do inherit a $BoundParameters-dictionary in the function, so we could technically separate undefined from -Mandatory:$false without any breaking changes. Not the pretties syntax though.

[krokofant]

Should -HaveParameter -Mandatory:$false

Supporting this would enable the following structure for -ForEach tests:

    It 'Has parameter <_.Name>' -TestCases @(
        @{ Name = 'Foo'; Mandatory = $true }
        @{ Name = 'Bar'; }
    ) {
        $command | Should -HaveParameter $Name -Mandatory:([bool]$Mandatory) -Type $Type
    }

Where I want to make sure Foo is mandatory but Bar is not.

[krokofant]

There are a lot of nots and not negate in this code so it's a brain melter.

I think instead of this (current):

if ($Mandatory) {
    $testMandatory = $attributes | & Where-Object { $_ -is [System.Management.Automation.ParameterAttribute] -and $_.Mandatory }
    $filters += "which is$(if ($Negate) {" not"}) mandatory"

    if (-not $Negate -and -not $testMandatory) {
        $buts += "it wasn't mandatory"
    }
    elseif ($Negate -and $testMandatory) {
        $buts += "it was mandatory"
    }
}

It should be this:

if ($PSBoundParameters.Keys -contains "Mandatory") {
    $testMandatory = $attributes | & Where-Object { $_ -is [System.Management.Automation.ParameterAttribute] -and $_.Mandatory }
    $filters += "which is$(if ($Negate -or -not $Mandatory) {" not"}) mandatory"

    if (-not ($Negate -or -not $Mandatory) -and -not $testMandatory) {
        $buts += "it wasn't mandatory"
    }
    elseif (($Negate -or -not $Mandatory) -and $testMandatory) {
        $buts += "it was mandatory"
    }
}

Can it be simplified a bit? 🤔

Or just add another if:

if (-not $Mandatory -and $PSBoundParameters.Keys -contains "Mandatory") {
    $testMandatory = $attributes | & Where-Object { $_ -is [System.Management.Automation.ParameterAttribute] -and $_.Mandatory }
    $filters += "which is$(if (-not $Negate) {" not"}) mandatory"

    if ($Negate -and -not $testMandatory) {
        $buts += "it wasn't mandatory"
    }
    elseif (-not $Negate -and $testMandatory) {
        $buts += "it was mandatory"
    }
}

[fflaten]

Should -HaveParameter -Mandatory:$false

Supporting this would enable the following structure for -ForEach tests:

    It 'Has parameter <_.Name>' -TestCases @(
        @{ Name = 'Foo'; Mandatory = $true }
        @{ Name = 'Bar'; }
    ) {
        $command | Should -HaveParameter $Name -Mandatory:([bool]$Mandatory) -Type $Type
    }

Where I want to make sure Foo is mandatory but Bar is not.

That already works, but I'd recommend being explicit with Mandatory = $false in the second testcase. The inconvenience is only when -Not is used at the same time.

Both Last sample in comment above have breaking changes when parameter is mandatory but you run Should -HaveParameter abc (not defined -Mandatory) like your second testcase. The $PSBoundParameters.ContainsKey() should probably be in the inner if-else.

Note to future assignee:
Any change before new Should/Assert are introduced should also be applied to $HasArgumentCompleter + ensure we have tests for all scenarios (with/without -Not, parameter exists vs not etc.) BEFORE changing anything to avoid regression now and later.

[krokofant]

That already works, but I'd recommend being explicit with Mandatory = $false in the second testcase. The inconvenience is only when -Not is used at the same time.

The second case does not work from what I've tried without the changes I mentioned. Since the current Pester code just has an if that checks if $Mandatory is true. The explicit -Mandatory:$false is never tested.

I thank you for the recommendation, but please re-iterate what you mean should work. -Mandatory:([bool]$false) does not seem to work in the sense that it should break if the parameter is actually mandatory. It just skips checking anything if $Mandatory is false.

[fflaten]

Yeah, you're absolutely right. Looked at the wrong sample when I answered.