Open enoorden opened 2 years ago
Thanks for reporting this, @lipkau wanna fix this?
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.
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).
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
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.
We're unable block -NotMandatory -Mandatory now due to how Should works
Would it work saying Should -HaveParameter -Mandatory:$false
🤔
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.
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.
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"
}
}
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 butBar
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.
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.
Yeah, you're absolutely right. Looked at the wrong sample when I answered.
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
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