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

Purpose of scriptblock parser in Get-Poshspecparam? #4

Open
devblackops opened this issue May 15, 2016 · 7 comments
Open

Purpose of scriptblock parser in Get-Poshspecparam? #4

devblackops opened this issue May 15, 2016 · 7 comments

Comments

@devblackops
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

cdhunt commented May 16, 2016

@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
Copy link
Contributor Author

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

@cdhunt cdhunt added this to the Next + 1 milestone May 18, 2016
@cdhunt cdhunt added the RFC label May 18, 2016
@Sam-Martin
Copy link
Contributor

Sam-Martin commented Jun 24, 2016

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
Copy link
Contributor

cdhunt commented Jun 24, 2016

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
Copy link
Contributor

Sam-Martin commented Jun 24, 2016

@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
Copy link
Contributor

cdhunt commented Jun 24, 2016

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?

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

No branches or pull requests

5 participants
@cdhunt @Sam-Martin @devblackops @juanfperezperez and others