pester / Pester

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

Unbound container scriptblock unexpectedly runs in Pester scope #2411

Open GitHubEddie opened 9 months ago

GitHubEddie commented 9 months ago

Checklist

What is the issue?

I am having an odd issue that I am hoping someone can explain.

I am creating a script block using the method [scriptblock]::create . This script block is being passed into a new-pestercontainer as a -scriptblock parameter. I am also passing in a -data parameter. The issue is that the key pairs in the -data parameter are being discarded during the run phase. However, this is not an issue if the script block is created using a different method.

Expected Behavior

I am assuming that regardless of how the script block is created, the key pairs in the new-pestercontainer -Data parameter should be available in both the run and discovery phases.

Steps To Reproduce

$Scriptblock = [scriptblock]::Create(
{
    param($PARAM1)

    BeforeDiscovery {
        write-host ('BeforeDiscovery PARAM1: {0}' -f $PARAM1)
    }

    BeforeAll {
        write-host ('BeforeAll PARAM1: {0}' -f $PARAM1)
    }

    Describe '<PARAM1> attempt' {
        It '<PARAM1> attempt' {
            $PARAM1  | Should -Be 'param1 test'
        }
    }
})

$container = New-PesterContainer -ScriptBlock $Scriptblock -Data @{ PARAM1 = 'param1 test' }
$config = New-PesterConfiguration
$config.run.Container = @($container)
$config.Output.Verbosity = 'Detailed'
invoke-pester -configuration $config

This produces the below output, with PARAM1 only showing up in discovery:

Pester v5.5.0

Starting discovery in 1 files.
BeforeDiscovery PARAM1: param1 test
Discovery found 1 tests in 83ms.
Running tests.
BeforeAll PARAM1: 
Describing  attempt
  [-]  attempt 294ms (287ms|7ms)
   Expected 'param1 test', but got $null.
   at $PARAM1  | Should -Be 'param1 test', :14
   at <ScriptBlock>, <No file>:14
Tests completed in 658ms
Tests Passed: 0, Failed: 1, Skipped: 0 NotRun: 0

However, changing only how the script block is created (without the [scriptblock]::create method).

$Scriptblock = {
    param($PARAM1)

    BeforeDiscovery {
        write-host ('BeforeDiscovery PARAM1: {0}' -f $PARAM1)
    }

    BeforeAll {
        write-host ('BeforeAll PARAM1: {0}' -f $PARAM1)
    }

    Describe '<PARAM1> attempt' {
        It '<PARAM1> attempt' {
            $PARAM1  | Should -Be 'param1 test'
        }
    }
}

The output now has PARAM1 in the run phase:

Pester v5.5.0

Starting discovery in 1 files.
BeforeDiscovery PARAM1: param1 test
Discovery found 1 tests in 62ms.
Running tests.
BeforeAll PARAM1: param1 test
Describing param1 test attempt
  [+] param1 test attempt 10ms (7ms|3ms)
Tests completed in 138ms
Tests Passed: 1, Failed: 0, Skipped: 0 NotRun: 0

Describe your environment

Pester version     : 5.5.0 C:\Program Files\WindowsPowerShell\Modules\Pester\5.5.0\Pester.psm1
PowerShell version : 5.1.22621.2506
OS version         : Microsoft Windows NT 10.0.22621.0

Possible Solution?

No response

csandfeld commented 9 months ago

Interesting find @GitHubEddie. I had a look at the AST for each "type" of scriptblock, and I guess the problem is caused by missing data in the AST Parent property - check this out.

There IS data in the Parent if scriptblock is created using curly braces ...

{ 'Script block using curly braces' }.Ast

# Output
Attributes         : {}
UsingStatements    : {}
ParamBlock         :
BeginBlock         :
ProcessBlock       :
EndBlock           : 'Script block using curly braces'
DynamicParamBlock  :
ScriptRequirements :
Extent             : { 'Script block using curly braces' }
Parent             : { 'Script block using curly braces' }

... while there is NO data in the Parent property if scriptblock is created using the [scriptblock] accelerator ...

[scriptblock]::Create('Script block using accelerator').Ast

# Output
Attributes         : {}
UsingStatements    : {}
ParamBlock         :
BeginBlock         :
ProcessBlock       :
EndBlock           : Script block using accelerator
DynamicParamBlock  :
ScriptRequirements :
Extent             : Script block using accelerator
Parent             :

... and there is also NO data in the Parent property if scriptblock is created using the full [System.Management.Automation.ScriptBlock] class

[System.Management.Automation.ScriptBlock]::Create('Script block using full class name').Ast

# Output
Attributes         : {}
UsingStatements    : {}
ParamBlock         :
BeginBlock         :
ProcessBlock       :
EndBlock           : Script block using full class name
DynamicParamBlock  :
ScriptRequirements :
Extent             : Script block using full class name
Parent             :

Verified this on Windows PowerShell 5.1.22621.2506 and PowerShell 7.4.0

csandfeld commented 9 months ago

... or by the difference in the Extent property for that matter

fflaten commented 9 months ago

Thanks for the detailed report.

@GitHubEddie: May I ask why you'd want to use [scriptblock]::Create(string script) in the first place when you already have a scriptblock? It will convert your scriptblock to a string, then create a scriptblock from it - which is really redundant. Instead you get weird issues like this and broken debugging. Are you getting the real code as text from external data/system?

@nohwnd The unbound scriptblock is executed in Pester's session state. Anything we can fix without breaking other scenarios?

fflaten commented 9 months ago

@csandfeld Interesting find, but it's not related to this issue. I'm curious why they don't provide the same Ast though, so might ask on the PowerShell discord or github-repo 🤔

The root cause is that Pester assumes that all provided container scriptblocks are bound to the same session state used when calling Invoke-Pester ("caller state"). It injects the provided parameters (Data) in that session state only, so if the scriptblocks are executed in a different session state they won't see those variables.

Using [scriptblock]::Create or {}.Ast.GetScriptblock() creates an unbound scriptblock, meaning it doesn't have a session state. As a result it is mistakenly executed in Pester's internal module state, while the variables are set in the default "terminal/script/global" state.

If possible Pester should detect unbound container scriptblocks and set the caller state to avoid being executed in Pester's internal state.

Bonus issue: The same issue would occur by either generating the scriptblocks OR calling Invoke-Pester from a module as both cases would cause mismatching session states. So a follow up improvement might be to always set the variables in the container's session state.

GitHubEddie commented 9 months ago

@fflaten, thanks for the explanation, it has been enlightening.

May I ask why you'd want to use [scriptblock]::Create(string script) in the first place when you already have a scriptblock?

The example given was for brevity, this is what I am actually doing.

I have a series of data driven tests that relate to individual systems, Active Directory for example. I keep these tests along with their pester configuration settings in PowerShell data files (.psd1). The data files end up looking something like this:

# PesterConfiguration.Demo.Tests.psd1
@{
    TestResult = @{
        Enabled = $true
        OutputFormat = 'JUnitXml'
    }

    Output = @{
        Verbosity = 'Detailed'
        CIFormat = 'Auto'
    }

    Run = @{
        Exit = $false
        SkipRemainingOnFailure = 'None'
        PassThru = $true

        Container = @(
            @{

               Scriptblock = {
                    param($PARAM1)

                    BeforeDiscovery {
                        write-host ('BeforeDiscovery PARAM1: {0}' -f $PARAM1)
                    }

                    BeforeAll {
                        write-host ('BeforeAll PARAM1: {0}' -f $PARAM1)
                    }

                    Describe '<PARAM1> attempt' {
                        It '<PARAM1> attempt' {
                            $PARAM1  | Should -Be 'param1 test'
                        }
                    }
                }

                Data = @{
                    PARAM1 = 'param1 test'
                }
            }

        )
    }

}

I safely import the data file and add it to new-pestercontainer and new-pesterconfiguration objects. This is where the odd scriptblock behaviour begins.

If you run this for example, where the imported scriptblock is injected directly into the pester container:

$import = Import-PowerShellDataFile -Path 'C:\PesterConfiguration.Demo.Tests.psd1'
$importScriptblock = $import.Run.Container[0].Scriptblock
$importData = $import.Run.Container[0].Data
$container = New-PesterContainer -ScriptBlock $importScriptblock -Data $importData
$config = New-PesterConfiguration -Hashtable $import
$config.run.Container = $container
$Result = invoke-pester -configuration $config

The test does not appear in the result:

Pester v5.5.0

Starting discovery in 1 files.
Discovery found 0 tests in 42ms.
Running tests.
Tests completed in 39ms
Tests Passed: 0, Failed: 0, Skipped: 0 NotRun: 0

If we look at the script block in a working container we get:

PS C:\> $config.Run.Container[0].Value[0].item

    param($PARAM1)

    BeforeDiscovery {
        write-host ('BeforeDiscovery PARAM1: {0}' -f $PARAM1)
    }

    BeforeAll {
        write-host ('BeforeAll PARAM1: {0}' -f $PARAM1)
    }

    Describe '<PARAM1> attempt' {
        It '<PARAM1> attempt' {
            $PARAM1  | Should -Be 'param1 test'
        }
    }

However, the script block in the container where the test is not in the result we see the issue is 'extra' curly braces:

PS C:\> $config.Run.Container[0].Value[0].item
{
                    param($PARAM1)

                    BeforeDiscovery {
                        write-host ('BeforeDiscovery PARAM1: {0}' -f $PARAM1)
                    }

                    BeforeAll {
                        write-host ('BeforeAll PARAM1: {0}' -f $PARAM1)
                    }

                    Describe '<PARAM1> attempt' {
                        It '<PARAM1> attempt' {
                            $PARAM1  | Should -Be 'param1 test'
                        }
                    }
                }

These 'extra' curly braces come from the Import-PowerShellDataFile function. To accommodate this, I convert the script block to a string, remove the 'extra' curly braces, then convert it back to a script block. This is why I have been using the [scriptblock]::Create static method.

When the script block is in the right sessionstate without the extra curly braces the test runs as expected:

Pester v5.5.0

Starting discovery in 1 files.
BeforeDiscovery PARAM1: param1 test
Discovery found 1 tests in 45ms.
Running tests.
BeforeAll PARAM1: param1 test
Describing param1 test attempt
  [+] param1 test attempt 8ms (2ms|6ms)
Tests completed in 124ms
Tests Passed: 1, Failed: 0, Skipped: 0 NotRun: 0
fflaten commented 5 months ago

@nohwnd Thoughts on this? See https://github.com/pester/Pester/issues/2411#issuecomment-1847552468

nohwnd commented 5 months ago

The unbound SB will have $null internal session state. We can check that. But I would throw in that case rather than silently assume that we know where the scriptblock is coming from. This will save us some headaches, but won't help OP. I will try to experiment with their code, it seems like they need something like Invoke-Expression rather than recreating scriptblock, but I did not comprehend it fully yet..

$sb = [scriptBlock]::Create({ "aaa" }) ;  & (get-module pester  ) { param($sb) $script:ScriptBlockSessionStateInternalProperty.GetValue($sb, $null)  } $sb
#output: $null
$sb = { "aaa" } ;  & (get-module pester  ) { param($sb) $script:ScriptBlockSessionStateInternalProperty.GetValue($sb, $null)  } $sb
#output: The session state
fflaten commented 5 months ago

Thanks. Was also thinking a potential fix might be a breaking change

nohwnd commented 4 months ago

@GitHubEddie Creating a scriptblock that produces scriptblock and then invoking it in the current context will bind the scriptblock to the current session state. This will give you a scriptblock that is created from text, and bound to current session state so you can safely pass to pester.

# Create a scriptblock, notice the extra {} inside, this will 
# make a scriptblock that outputs a scriptblock when executed
$unboundScriptBlock = [scriptBlock]::Create('{$ExecutionContext.SessionState}')
# Invoke the scriptblock in current scope to bind it.
$boundScriptBlock = & $unboundScriptBlock

# invoking the bound scriptblock in pester inner state will return session context that has empty Module 
# proving that the scriptblock was bound to our current scope
& (get-module pester  ) { param($sb) &$sb } $boundScriptBlock
nohwnd commented 4 months ago

In 6.0.0 prevent providing unbound scriptblocks in places where we take them. Seems like edge case, and there is workaround above.