Add error handling for interactive #requires #6469
Conversation
|
I've opened an RFC on updating the behaviour of |
|
@rjmholt - I meant can you add a test for the bug that was fixed - the fix isn't obvious I guess - so maybe check first/last tokens that were supposed to be set but caused the exception. |
|
(Clicked the wrong button just then). Ok will do. The test I wrote traverses the code path I fixed -- just doesn't check the tokenizer state specifically. I assume the thing to do is to add a tokenizer test somewhere in Parser.Tests.ps1? Is there a straightforward way to access the tokenizer's state? I've been assuming the contract on |
|
@rjmholt - First/Last token are exposed through PowerShell variables
|
| param($script, $firstToken, $lastToken) | ||
|
|
||
| $ps.AddScript($script) | ||
| $ps.AddScript("(`$^,`$`$)") |
There was a problem hiding this comment.
I'm a little surprised this works, 2 calls to AddScript is I think like piping the first call to the second. That said, it neatly drops any results from the first call.
To be slightly more like the interactive scenario, I think you would call $ps.Invoke twice, clearing the commands between calls.
There was a problem hiding this comment.
Yeah, I tried that and it didn't seem to do what I wanted. To be honest, I wasn't entirely certain how I should be using the embedded PowerShell session to recreate the input. But I spent some time playing with it, tried the "script, invoke, tokens, invoke" way and seemed to get $null for everything.
ex.ps1
Outdated
| @@ -0,0 +1,4 @@ | |||
| Write-Host "Hello" | |||
There was a problem hiding this comment.
Did you run git commit -a? It seems like this file was accidentally added.
There was a problem hiding this comment.
Gah, I thought I checked that first. One sec
|
@daxian-dbw I think this is all addressed. I just tried |
| $settings = [System.Management.Automation.PSInvocationSettings]::new() | ||
| $settings.AddToHistory = $true | ||
|
|
||
| $ps = [powershell]::Create() |
There was a problem hiding this comment.
Please have a AfterAll block to dispose $ps.
| $ps = [powershell]::Create() | ||
| $ps.AddScript("#requires`n10") | ||
| $ps.Invoke(@(), $settings) | Should -BeExactly 10 |
There was a problem hiding this comment.
Please dispose $ps after you done with it.
Also, I think it's not necessary to use PSInvocationSettings in this test. Just $ps = [powershell]::Create(); $ps.AddScript("#requiresn10").Invoke() | Should -BeExactly 10` will do.
There was a problem hiding this comment.
I think we need to set AddToHistory so we hit this line and test it. At least, that's why I wrote it.
There was a problem hiding this comment.
I see, we need to hit the
PR Summary
Fix for #3803. Restores the tokenizer state more fully to prevent the null dereference causing the original error. Also changed the name of a variable to express that it doesn't actually mean that the PowerShell session is interactive or not.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests