-
-
Notifications
You must be signed in to change notification settings - Fork 469
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
Throw on unbound user-provided scriptblocks #2551
base: main
Are you sure you want to change the base?
Conversation
Good or checking too many places? Update link to https://pester.dev/docs/migrations/v5-to-v6 (when available) with workaround? |
$prettySb = $prettySb.Remove($maxLength) + "..." | ||
} | ||
|
||
throw [System.ArgumentException]::new("Unbound scriptblock '$prettySb' is not allowed. See https://github.com/pester/Pester/issues/2411.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw [System.ArgumentException]::new("Unbound scriptblock '$prettySb' is not allowed. See https://github.com/pester/Pester/issues/2411.") | |
throw [System.ArgumentException]::new("Unbound scriptblock is not allowed to be used, because it would run inside of Pester session state, which would produce unexpected results. See https://github.com/pester/Pester/issues/2411 for more details and workarounds. ScriptBlock: '$prettySb'") |
how about this? I think explaining why the scriptblock is not allowed first and then showing it would make it easier to see what the problem is, rather than saying
The scriptblock
then the loooooooooooooong scriptblock
then the loooooooooooooong scriptblock
then the loooooooooooooong scriptblock
then the loooooooooooooong scriptblock
then the loooooooooooooong scriptblock
and then the actual error.
[ScriptBlock] $ScriptBlock | ||
) | ||
$internalSessionState = $script:ScriptBlockSessionStateInternalProperty.GetValue($ScriptBlock, $null) | ||
if ($null -eq $internalSessionState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great that the check is cheap to do.
The PR looks great. There is some merge conflict |
PR Summary
Throws when user provides scriptblocks that are not bound to a session state. They would be executed in Pester's module state which could cause unexpected behaviour.
Examples:
Fix #2411
Checklist:
Add-AssertionOperator -Test
(type:ScriptBlock
)Add-ShouldOperator -Test
(type:ScriptBlock
)AfterAll -Scriptblock
(type:ScriptBlock
)AfterEach -Scriptblock
(type:ScriptBlock
)BeforeAll -Scriptblock
(type:ScriptBlock
)BeforeDiscovery -ScriptBlock
(type:ScriptBlock
)BeforeEach -Scriptblock
(type:ScriptBlock
)Context -Fixture
(type:ScriptBlock
)Describe -Fixture
(type:ScriptBlock
)InModuleScope -ScriptBlock
(type:ScriptBlock
)It -Test
(type:ScriptBlock
)Mock -MockWith
(type:ScriptBlock
)Mock -ParameterFilter
(type:ScriptBlock
)New-PesterContainer -ScriptBlock
(type:ScriptBlock[]
)Should-All -FilterScript
(type:ScriptBlock
)Should-Any -FilterScript
(type:ScriptBlock
)Should-Throw -ScriptBlock
(type:ScriptBlock
)Run.Scriptblock
Run.Container
PR Checklist
Create Pull Request
to mark it as a draft. PR can be markedReady for review
when it's ready.