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

Unbound container scriptblock unexpectedly runs in Pester scope #2411

Open
3 tasks done
GitHubEddie opened this issue Dec 8, 2023 · 10 comments
Open
3 tasks done

Unbound container scriptblock unexpectedly runs in Pester scope #2411

GitHubEddie opened this issue Dec 8, 2023 · 10 comments
Labels
Milestone

Comments

@GitHubEddie
Copy link

GitHubEddie commented Dec 8, 2023

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

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

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

@fflaten
Copy link
Collaborator

fflaten commented Dec 8, 2023

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

fflaten commented Dec 8, 2023

@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.

@fflaten fflaten added the Bug label Dec 8, 2023
@GitHubEddie
Copy link
Author

GitHubEddie commented Dec 10, 2023

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

fflaten commented Apr 14, 2024

@nohwnd Thoughts on this? See #2411 (comment)

@nohwnd
Copy link
Member

nohwnd commented Apr 14, 2024

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

fflaten commented Apr 14, 2024

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

@nohwnd
Copy link
Member

nohwnd commented Apr 16, 2024

@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 nohwnd added this to the 6.0.0 milestone Apr 18, 2024
@nohwnd
Copy link
Member

nohwnd commented Apr 18, 2024

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

@nohwnd nohwnd changed the title Script block creation method discards parameters during run phase Unbound container scriptblock unexpectedly runs in Pester scope Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants