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

Add support for multiple aliases (-Alias) to Should -HaveParameter #2247

Merged
merged 5 commits into from
Oct 31, 2022

Conversation

dtewinkel
Copy link
Contributor

@dtewinkel dtewinkel commented Oct 22, 2022

PR Summary

Add support for multiple mandatory aliases for -HaveParameter. -Alias parameter was changed from a String to a String[].

Allows for Should -HaveParameter Parameter -Alias Alias1, Alias2 to test if parameter Parameter has both the aliases alias1 and Alias2. If either one is missing the assert fails.

Fix #2152

Daniël te Winkel added 2 commits October 22, 2022 19:40
Add support for multiple mandatory aliases for -HaveParameter.
Split execution of tests in `tst\functions\Add-ShouldOperator.ts.ps1` into multiple b sections, each of them reloading the Pester module, to avoid running into the more than 32 operators (parameter sets) limitation.
@dtewinkel
Copy link
Contributor Author

@fflaten, @nohwnd , I see the PR build breaks due to a previously introduced bug in file tst\functions\Add-ShouldOperator.ts.ps1, where it runs into the more than 32 assertion operators limitation. This builds fail since a merge to main on the 3rd of October.
I added a fix, or work-around, to the tests to prevent running into the limitation.

Comment on lines 267 to 269
foreach ($AliasValue in $Alias) {
$testPresenceOfAlias = $ActualValue.Parameters[$ParameterName].Aliases -contains $AliasValue
$filters += "to$(if ($Negate) {" not"}) have an alias '$AliasValue'"
Copy link
Collaborator

@fflaten fflaten Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach ($AliasValue in $Alias) {
$testPresenceOfAlias = $ActualValue.Parameters[$ParameterName].Aliases -contains $AliasValue
$filters += "to$(if ($Negate) {" not"}) have an alias '$AliasValue'"
$filters += "with$(if ($Negate) {'out'}) alias$(if ($Alias.Count -ge 2) {'es'}) $(Join-And ($Alias -replace '^|$', "'"))"
foreach ($AliasValue in $Alias) {
$testPresenceOfAlias = $ActualValue.Parameters[$ParameterName].Aliases -contains $AliasValue

Maybe move the filter-text outside foreach to improve readability? Thoughts?
We can utilize helpers in Format.ps1 like Join-And or Format-Collection to make it pretty.

Current PR:

Expected command Invoke-DummyFunction to have a parameter MandatoryParam, to have an alias 'First' and to have an alias 'Second', but it didn't have an alias 'Second'.

Join-And (suggestion):

Expected command Invoke-DummyFunction to have a parameter MandatoryParam, with aliases 'First' and 'Second', but it didn't have an alias 'Second'.

Format-Collection:

Expected command Invoke-DummyFunction to have a parameter MandatoryParam, with aliases @('First', 'Second'), but it didn't have an alias 'Second'.

$filters += "with$(if ($Negate) {'out'}) alias$(if ($Alias.Count -ge 2) {'es'}) $(Format-Collection $Alias)"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions. They help to make the output more readable. I applied them, including to the formatting of the wrong aliases.

@fflaten
Copy link
Collaborator

fflaten commented Oct 23, 2022

Thanks for submitting the PR!

Also thanks for pointing out the CI-issue! I've submitted a separate PR (#2248) to split the recent help-tests into its own block and to keep this PR clean. Would you mind reverting the workaround? 😇

Help to make the output more readable.
@dtewinkel
Copy link
Contributor Author

Thanks for submitting the PR!

Also thanks for pointing out the CI-issue! I've submitted a separate PR (#2248) to split the recent help-tests into its own block and to keep this PR clean. Would you mind reverting the workaround? 😇

Sure, no problem :-). I'll revert it as soon as it is merged to master. For now, it helps me to show that PS3 and PS4 are failing, and I need to figure out why.

@dtewinkel
Copy link
Contributor Author

Thanks for submitting the PR!

Also thanks for pointing out the CI-issue! I've submitted a separate PR (#2248) to split the recent help-tests into its own block and to keep this PR clean. Would you mind reverting the workaround? 😇

Revert is done.

@fflaten fflaten closed this Oct 25, 2022
@fflaten fflaten reopened this Oct 25, 2022
@fflaten
Copy link
Collaborator

fflaten commented Oct 25, 2022

Revert is done.

Thanks! Closed/reopened to trigger a working CI-run.
LGTM 👏 Waiting on @nohwnd

@@ -8,7 +8,7 @@ InPesterModuleScope {
function Invoke-DummyFunction {
param(
[Parameter(Mandatory = $true)]
[Alias('First')]
[Alias('First', 'Another')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern here is that this changes "data" of another test, would it be possible to keep those functions as they are, and introduce a new function that has two aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests share two functions, one with all possible options that need to be validated, and an empty one, so that was the road I continued on.
I validated that the tests around aliases were still valid for what they were testing. I believe that actually sharing the data in this case 'enhances' the tests without having to add a number of functions to validate all kinds of scenarios. It now also validates that if -Alias First is tested, that it succeeds even if there are multiple aliases.

So, my question is, is your concern a general one and was the setup of the tests perhaps not right in the first place, because they all share the same two functions here, or is there a more specific concern here that you need me to address?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a general concern about sharing test data than eventually become so complicated that no-one knows what is going to break. This is not severe here so let's allow it.

@nohwnd nohwnd merged commit 799d454 into pester:main Oct 31, 2022
@nohwnd
Copy link
Member

nohwnd commented Oct 31, 2022

@dtewinkel Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use "Should -HaveParameter ... -Alias" when function has multiple aliases?
3 participants