ticketmaster / poshspec

Infrastructure Testing DSL running in Pester
MIT License
183 stars 32 forks source link

Purpose of scriptblock parser in Get-Poshspecparam? #4

Open devblackops opened 8 years ago

devblackops commented 8 years ago

I'm not really sure of the purpose of parsing $TestExpression in Get-Poshspecparam, turning around and constructing a string from it, then invoking it with Invoke-Expression. Can you explain why this is needed?

I'm getting some errors when $TestExpression starts to become a little complex. I don't have much experience dealing with language parsing in PowerShell but my gut feeling is the logic in Get-Poshspecparam may need to be considerably more complex in order to deal with the various scriptblocks thrown at it.

The code below would be an enhancement to the LocalGroup resource which will return the group name and members but fails with a parse exception.

$expression = {
    Get-CimInstance Win32_Group -Filter "Name = '$target' and LocalAccount = 'True'" |
        Select-Object Name, @{Name = 'Members'; Expression = {
            (Get-CimAssociatedInstance -InputObject $_ -ResultClassName Win32_UserAccount).Name}}
}

The members property of the object returned should be able to be inspected with the test below.

LocalGroup 'Administrators' members { should contain 'Administrator' }

This is the error I receive when testing this resource. poshspec_error

Thoughts? Brandon

juanfperezperez commented 8 years ago

This would likely be better served by LocalGroupMembership resource and maybe a LocalUser resource since the Win32_Group class does not have a members property and the Win32_UserAccount does not have a memberof property. Also in Pester, the "Contain" assertion does not work with arrays, however "Be" does work.

Juan

cdhunt commented 8 years ago

@devblackops The goal was to get a lot of the logic outside of the scope of the Pester functions to enable testing. The parsing of the scriptblock replaces the variables with their value - sort of half-evaluating the expression. There's probably a better way to do it. You could move that snippet into a new Private function like Get-LocalGroupMembers that you call from LocalGroup.

That is a good example. I'll see about making it supported without a function.

devblackops commented 8 years ago

@cdhunt Thanks. I'll mess around with extending LocalGroup or creating a new test for group membership.

Sam-Martin commented 8 years ago

The other downside to parsing $TestExpression as a string is that when you have a variable in your expression it isn't interpolated in the printed output. image

EDIT: I've quickly and hackily worked around this like so:

function Fix-PoshSpecInterpolation{
    param($CodeBlock)
    $matches = @();
    $CodeString = $CodeBlock.ToString().Trim()
    if($CodeString -match '\$[\w:]*'){
        foreach($variable in $matches.Values){
            $CodeString = $CodeString.replace($variable, "`"$(iex $variable)`"")
        }
    }
    return [scriptblock]::Create($CodeString)
}

Describe 'Website Host Entry' {
    File "$Env:SYSTEMROOT\system32\drivers\etc\hosts" $(Fix-PoshSpecInterpolation{ Should Contain $SiteName})
}

Which produces image

Which is ugly as all hell and won't deal with quotes gracefully, but someone may find it useful.

cdhunt commented 8 years ago

I assumed the value in the Should block would be a constant, but it should be possible to expand variables with $ExecutionContext.InvokeCommand.ExpandString() the same way we do on the other part of the expression.

Sam-Martin commented 8 years ago

@cdhunt Hah, amazing, I didn't realise that's what that was doing.... So much cleaner now.

function Fix-PoshSpecInterpolation{
    param($CodeBlock)
    $CodeString = $CodeBlock.ToString().Trim()
    return [scriptblock]::Create($ExecutionContext.InvokeCommand.ExpandString($CodeString))
}

Describe 'Website Host Entry' {
    File "$Env:SYSTEMROOT\system32\drivers\etc\hosts" $(Fix-PoshSpecInterpolation{ Should Contain $SiteName})
}

Thank you! Would you be up for me submitting a PR with this fixed in the module?

cdhunt commented 8 years ago

I just tested adding $assertion = $ExecutionContext.InvokeCommand.ExpandString($assertion) to Get-PoshSpecParam and it works with one caveat, the variable must be defined in Global scope. The way you solved it works really well, but does add a fair amount of extra "stuff" to the line.

I shortened it with an alias:

Set-Alias -Name exp -Value Fix-PoshSpecInterpolation

Describe "test should block" {
    $running = "Running"
    Service w32time Status (Exp{Should Be $running})
}

Which is easier on the user?