Send all stderr and stdout to the output channel when a DVC process fails #3857
Conversation
…wable) DVC processes
| @@ -164,8 +164,8 @@ export class DvcRunner extends Disposable implements ICli { | |||
| { | |||
| ...baseEvent, | |||
| duration: stopWatch.getElapsedTime(), | |||
| exitCode, | |||
| stderr | |||
| errorOutput: stderr, | |||
There was a problem hiding this comment.
[F] The runner and viewer show processes in the foreground so there is no change for these
| @@ -180,8 +196,8 @@ export class Cli extends Disposable implements ICli { | |||
| { | |||
| ...baseEvent, | |||
| duration, | |||
| exitCode: cliError.exitCode, | |||
| stderr: cliError.stderr | |||
| errorOutput: all || cliError.stderr, | |||
There was a problem hiding this comment.
[F] If we have captured a valid all string (length > 0) then we will use that, otherwise use the stderr from the generated error. I feel like this way there will always be something provided to the output channel
| backgroundProcess.all?.on( | ||
| 'data', | ||
| chunk => (all += transformChunkToString(chunk)) | ||
| ) |
There was a problem hiding this comment.
Should this be moved to a private function, or even a util (just because it's being duplicated from line 83)? That would also make sure this line is covered since line 83 is.
There was a problem hiding this comment.
I did have this in a separate helper function but the string was not captured as expected and passed to the catch block.
|
Thanks for taking this so quickly @mattseddon1 |
|
Code Climate has analyzed commit 3c8c9d1 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 91.6% (85% is the threshold). This pull request will bring the total coverage in the repository to 94.9%. View more on Code Climate. |
As discussed in the cross-team meeting this morning (cc @dberenbaum).
It is useful to send all of the stderr/stdout to the output channel when a DVC process fails. This PR makes the necessary updates.
Demo
Screen.Recording.2023-05-10.at.2.41.28.pm.mov