pester / Pester

Pester is the ubiquitous test and mock framework for PowerShell.
https://pester.dev/
Other
3.08k stars 470 forks source link

Improve Get-ShouldOperator #1163

Closed nohwnd closed 3 years ago

nohwnd commented 5 years ago

Get-ShouldOperator shows:

NAME
    Should-Be

on the top, and does not show aliases. It would be nice to replace the name section with this:

NAME
    Be

ALIASES
    EQ
    EQUAL
    BeEq

(the two last aliases don't actually exist on Be)

Related to #878

Stephanevg commented 5 years ago

still available? if so, i'll be happy to check this one

nohwnd commented 5 years ago

@Stephanevg yup. Looks like you are the first one.

omiossec commented 5 years ago

I will be happy too

Stephanevg commented 5 years ago

@nohwnd I think you meant: Get-ShouldOperator -Name be Shows:

NAME
    Be

ALIASES
    EQ
    EQUAL
    BeEq

right?

Cause I have this when I use Get-ShouldOperator (Without any parameters):


Get-ShouldOperator

Name                      Alias
----                      -----
Be                        EQ
BeExactly                 CEQ
BeGreaterThan             GT
...
Stephanevg commented 5 years ago

I am sorry to ask, but either I didn't fully understand what problem we need to fix here, or I am missing something.

I originally thought that this issue would be as easy as editing the 'comment Based Help' of the BE operator, but It seems there is a bit more to it.

Perhaps you can give me a hint of where I should look? ;)

Let me explain my rationale:

for example, any other 'ShouldOperator' return the same output. Where .NAMEshows the $InternalName (which is also often the name of the file where the function resides).

Be exactly

[DBG]:   Get-ShouldOperator -Name BeExactly

NAME
    Should-BeExactly

SYNOPSIS
    Compares one object with another for equality and throws if the
    two objects are not the same. This comparison is case sensitive.

    -------------------------- EXAMPLE 1 --------------------------

    PS C:\>$actual = "Actual value"

    PS C:\>$actual | Should -Be "Actual value"

    This test will pass. The two strings are identical.

    -------------------------- EXAMPLE 2 --------------------------

    PS C:\>$actual = "Actual value"

    PS C:\>$actual | Should -Be "actual value"

    This test will fail, as the two strings do not match case sensitivity.

This actually highlights that the issue is not an isoolated issue, as in opposition of what I thought.

Process understanding:

It seems to me, that I missing a (obvious) part

The assertion operator nd it Alias is declared in be.ps1 with the following code:

Add-AssertionOperator -Name               Be `
                      -InternalName       Should-Be `
                      -Test               ${function:Should-Be} `
                      -Alias              'EQ' `
                      -SupportsArrayInput

and later is called via the Get-ShouldOperator function located in Get-ShouldOperator.ps1 using this part:

$operator = $AssertionOperators.Values | Where-Object { $Name -eq $_.Name -or $_.Alias -contains $Name }
$help = Get-Help $operator.InternalName -Examples -ErrorAction SilentlyContinue

Unfortunatley, it is not possible to add simply a '.ALIASES' part to a comment based help section. It seems like there is a limited list, that we can used for Internal comment based help keywords See here for more details. It seems like, if the help content is located in an external file, it would be possible to create that .ALIASES section, but it seems like we are not using it in this module.

With all that said, I can think of two options, which both I don't like: 1) Grab that $Help object, and return build and send a custom object similarly on how it is done when no '-Name' parameter is used. 2) dig more into the external help possibility.

Or perhaps I missed something? in that case, please hint me in the right direction :)

Cheers

nohwnd commented 5 years ago

@Stephanevg

1) Yes I meant the call with -Name parameter.

2) Sorry, I thought we are returning string from it not the help object... ๐Ÿ˜” I see that you can fool the template by setting $o.details.name. But the aliases don't work, there is some type magic going on, and when I assing $o.aliases = $foreach.aliases then the type of object on $o and $foreach is different. Some adaption and projection is going on there. So you could just set the name for now. I will ask around if anyone knows how to fool the aliases as well. (Alternatively we could return string and add -Raw switch that would return the help object, but I don't want to add that yet, because if we can change the help object successfully then no switch will be needed.)

nohwnd commented 5 years ago

@Stephanevg Still interested in implementing this?

Stephanevg commented 5 years ago

Hi @nohwnd I still am, but this week is going to be busy one for me. Also, I think I bumped into an issue, as 'simply' setting the name, actually didn't worked (If i remember correctly) - and that's why there is no PR yet ;)

I'll look into it later this week if ok. If this is urgent, somebody else could potentially do it then.

nohwnd commented 5 years ago

Not urgent :)

Stephanevg commented 5 years ago

Hey @nohwnd sorry It took me ages to come back to this one.

I just looked at it again. Launching a short test, I see this:

image

I asume that this is now fixed, right?

nohwnd commented 5 years ago

I donโ€™t think it is. The issue is about the detail of the operator, not the overview (in your screenshot).

Stephanevg commented 5 years ago

Ok, I have got something working here, but I am not really a big fan of the output, and that is something I think we ned to discuss.

I did a PR (https://github.com/pester/Pester/pull/1272) so we can maybe discuss directly in the code there. Otherwise, here is fine too :)

fflaten commented 3 years ago

So I guess the step forward would be just modifying the command name in the help output as outlined in the related issue.

Originally posted by @nohwnd in https://github.com/pester/Pester/issues/1272#issuecomment-488244227

Should we implement this? Quick modification to $help.details.name could make it look something like:

PS /workspaces/Pester/tst/functions> Get-ShouldOperator be                     

NAME
    Should-Be

    ALIASES: EQ

SYNOPSIS
    Compares one object with another for equality
    and throws if the two objects are not the same.

An option to explore would be to convert the help object to a custom type with ToString or formatdata and customcontrols to format it however we'd like. That way we could possibly recreate this and maybe also provide syntax help to show custom parameters in assertions like -ExceptionType in Should-Throw, the fact that it expect a scriptblock for ActualValue etc.

fflaten commented 3 years ago

I ended up experimenting for a couple of hours and got a PoC working using format-data.

$h = & (Get-Module Pester) { Get-Help Should-Be -Examples }

$obj = [PSCustomObject]@{
    PSTypeName = 'PesterAssertionHelp'
    AssertionName = "Be"
    Aliases = ('EQ','BeEQ')
    Help = @{
        Syntax = $h.syntax
        Synopsis = $h.Synopsis
        Examples = $h.examples
    }
}

# replace internal function name with assertion name in syntax-help
foreach ($syntax in $obj.Help.syntax.syntaxItem) { $syntax.name = $obj.AssertionName }

Update-FormatData /workspaces/Pester/helpformat.ps1xml

$obj

Output:

ASSERTION NAME
    Be

ALIASES
    EQ
    BeEQ

SYNTAX
    Should -Be [-ActualValue <Object>] [-ExpectedValue <Object>] [-Negate] [-Because <String>]

SYNOPSIS
    Compares one object with another for equality
    and throws if the two objects are not the same.

EXAMPLES
    -------------------------- EXAMPLE 1 --------------------------

    $actual = "Actual value"
    PS C:\>$actual | Should -Be "actual value"

    This test will pass. -Be is not case sensitive.
    For a case sensitive assertion, see -BeExactly.

    -------------------------- EXAMPLE 2 --------------------------

    $actual = "Actual value"
    PS C:\>$actual | Should -Be "not actual value"

    This test will fail, as the two strings are not identical.
nohwnd commented 3 years ago

@fflaten yeah go for it.

fflaten commented 3 years ago

Basically ready (https://github.com/fflaten/Pester/tree/assertion-help), but either I'm doing something wrong or I found a bug in Get-FormatData (https://gist.github.com/fflaten/c807d3f4933fc4ff20bcef2b9368451a). Looking into that before submitting PR. I think I can work around it by merging everything into a big viewdefinition, but that makes that xml unnecessary deep and harder to read.

Btw. any reason why .vscode/* is ignored when we already have a settings.json in repo?

nohwnd commented 3 years ago

@fflaten IDK, probably no reason. Maybe there was a problem with powershell extension forcing some settings into workspace, but maybe it is just the default coming from the template. I excluded settings.json, enabled format on save again, it no longer seems to hang all the time. And fixed formatting in all files in #1949

fflaten commented 3 years ago

Ok. I know format on save isn't always great so wasn't gonna ask for that. Just wondering as I'd like to set xml schema for format.ps1xml etc using settings.json ๐Ÿ™‚

nohwnd commented 3 years ago

I think we should try living with format on save again :)