Don't wrap stderr as ErrorRecord #5190
Conversation
SteveL-MSFT
requested review from
BrucePay,
TravisEz13,
adityapatwardhan and
vors
as code owners
.vscode/launch.json
Outdated
There was a problem hiding this comment.
This is being done in #5189. It shouldn't also be in this PR, I don't think.
There was a problem hiding this comment.
I'll remove it, had to change it locally to debug this.
SteveL-MSFT
changed the title
updated tests
What exactly do you mean? Would any |
| this.OutputPipe.Add(sendToPipeline); | ||
| } | ||
|
|
||
| internal void _WriteErrorSkipAllowCheck(object sendToPipeline) |
| Dbg.Assert(record != null, "ProcessReader should ensure that data is ErrorRecord"); | ||
| record.SetInvocationInfo(this.Command.MyInvocation); | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(record, isNativeError: true); | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(outputValue.Data); |
There was a problem hiding this comment.
Please add a comment stating that it is the intent to not wrap native stderror in an ErrorRecord.
| { | ||
| $ErrorActionPreference = $origEA | ||
| } | ||
| It "Verify Validate Dollar Error Populated should be in stderr" { |
There was a problem hiding this comment.
Should we also have a test for stream redirection to output?
$err = & $powershell -noprofile -command { wgwg-wrwrhqwrhrh35h3h3} 2>&1
| /// The Cmdlet should generally just allow PipelineStoppedException | ||
| /// to percolate up to the caller of ProcessRecord etc. | ||
| /// </exception> | ||
| internal void _WriteErrorSkipAllowCheck(object sendToPipeline) |
There was a problem hiding this comment.
Maybe name this WriteNativeCommandErrorSkipAllowCheck instead?
| record.SetInvocationInfo(this.Command.MyInvocation); | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(record, isNativeError: true); | ||
| // write stderr as string instead of as ErrorRecord | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(outputValue.Data); |
There was a problem hiding this comment.
outputValue.Data is still an ErrorRecord - right?
There was a problem hiding this comment.
You're right, outputValue.Data is still an ErrorRecord. I'll need to rework this.
5 participants
Keep stderr as string and don't wrap as ErrorRecord.
This also means that errors from running pwsh.exe within pwsh are now just stderr strings even though they visually look like ErrorRecords so tests had to be changed.
Most customers are likely not running powershell within powershell and expecting errors, but it does mean that try{}catch{} won't work (until we add the enhancement to throw on any stderr).
Fix #3813