From 4b448364086a6af929949bc9b864cfc5bcdc09c6 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 10 May 2023 14:39:58 +1000 Subject: [PATCH 1/2] send all stdout and stderr to the output channel for failing (non-viewable) DVC processes --- extension/src/cli/dvc/runner.ts | 4 +-- extension/src/cli/index.ts | 30 ++++++++++++++----- extension/src/cli/util.ts | 19 ++++++------ extension/src/cli/viewable.ts | 4 +-- .../test/suite/vscode/outputChannel.test.ts | 6 ++-- extension/src/vscode/outputChannel.ts | 14 +++++---- 6 files changed, 48 insertions(+), 29 deletions(-) diff --git a/extension/src/cli/dvc/runner.ts b/extension/src/cli/dvc/runner.ts index 681e05beb3..3fda4ed662 100644 --- a/extension/src/cli/dvc/runner.ts +++ b/extension/src/cli/dvc/runner.ts @@ -164,8 +164,8 @@ export class DvcRunner extends Disposable implements ICli { { ...baseEvent, duration: stopWatch.getElapsedTime(), - exitCode, - stderr + errorOutput: stderr, + exitCode }, this.processCompleted ) diff --git a/extension/src/cli/index.ts b/extension/src/cli/index.ts index 2f4a816870..1678d17390 100644 --- a/extension/src/cli/index.ts +++ b/extension/src/cli/index.ts @@ -1,7 +1,7 @@ import { Event, EventEmitter } from 'vscode' import { getCommandString } from './command' import { CliError, MaybeConsoleError } from './error' -import { notifyCompleted, notifyStarted } from './util' +import { notifyCompleted, notifyStarted, transformChunkToString } from './util' import { createProcess, Process, ProcessOptions } from '../process/execution' import { StopWatch } from '../util/time' import { Disposable } from '../class/dispose' @@ -13,7 +13,7 @@ export type CliEvent = { } export type CliResult = CliEvent & { - stderr?: string + errorOutput?: string duration: number exitCode: number | null } @@ -72,6 +72,7 @@ export class Cli extends Disposable implements ICli { protected async executeProcess(options: ProcessOptions): Promise { const { baseEvent, stopWatch } = this.getProcessDetails(options) + let all = '' try { const process = this.dispose.track(this.createProcess(baseEvent, options)) @@ -79,6 +80,11 @@ export class Cli extends Disposable implements ICli { void this.dispose.untrack(process) }) + process.all?.on( + 'data', + chunk => (all += transformChunkToString(chunk as Buffer)) + ) + const { stdout, exitCode } = await process notifyCompleted( @@ -96,18 +102,26 @@ export class Cli extends Disposable implements ICli { error as MaybeConsoleError, options, baseEvent, - stopWatch.getElapsedTime() + stopWatch.getElapsedTime(), + all ) } } protected async createBackgroundProcess(options: ProcessOptions) { const { baseEvent, stopWatch } = this.getProcessDetails(options) + let all = '' try { const backgroundProcess = this.createProcess(baseEvent, { detached: true, ...options }) + + backgroundProcess.all?.on( + 'data', + chunk => (all += transformChunkToString(chunk as Buffer)) + ) + return await this.getOutputAndDisconnect( baseEvent, backgroundProcess, @@ -118,7 +132,8 @@ export class Cli extends Disposable implements ICli { error as MaybeConsoleError, options, baseEvent, - stopWatch.getElapsedTime() + stopWatch.getElapsedTime(), + all ) } } @@ -170,7 +185,8 @@ export class Cli extends Disposable implements ICli { error: MaybeConsoleError, options: ProcessOptions, baseEvent: CliEvent, - duration: number + duration: number, + all: string ) { const cliError = new CliError({ baseError: error, @@ -180,8 +196,8 @@ export class Cli extends Disposable implements ICli { { ...baseEvent, duration, - exitCode: cliError.exitCode, - stderr: cliError.stderr + errorOutput: all || cliError.stderr, + exitCode: cliError.exitCode }, this.processCompleted ) diff --git a/extension/src/cli/util.ts b/extension/src/cli/util.ts index 4be5a6a3df..772a82fd97 100644 --- a/extension/src/cli/util.ts +++ b/extension/src/cli/util.ts @@ -7,31 +7,32 @@ export const notifyStarted = ( processStarted: EventEmitter ): void => processStarted.fire(baseEvent) +export const transformChunkToString = (chunk: Buffer): string => + chunk + .toString() + .split(/(\r?\n)/g) + .join('\r') + export const notifyOutput = ( process: Process, processOutput: EventEmitter ): void => { process.all?.on('data', chunk => - processOutput.fire( - (chunk as Buffer) - .toString() - .split(/(\r?\n)/g) - .join('\r') - ) + processOutput.fire(transformChunkToString(chunk as Buffer)) ) } export const notifyCompleted = ( - { command, pid, cwd, duration, exitCode, stderr }: CliResult, + { command, pid, cwd, duration, exitCode, errorOutput: stderr }: CliResult, processCompleted: EventEmitter ): void => processCompleted.fire({ command, cwd, duration, + errorOutput: stderr?.replace(/\n+/g, '\n'), exitCode, - pid, - stderr: stderr?.replace(/\n+/g, '\n') + pid }) export const captureStdErr = (process: Process): string => { diff --git a/extension/src/cli/viewable.ts b/extension/src/cli/viewable.ts index 9e7dcecb02..d7cb85eadb 100644 --- a/extension/src/cli/viewable.ts +++ b/extension/src/cli/viewable.ts @@ -77,8 +77,8 @@ export class ViewableCliProcess extends DeferredDisposable { { ...baseEvent, duration: stopWatch.getElapsedTime(), - exitCode, - stderr + errorOutput: stderr, + exitCode }, processCompleted ) diff --git a/extension/src/test/suite/vscode/outputChannel.test.ts b/extension/src/test/suite/vscode/outputChannel.test.ts index 5a298dfdf5..434f8bcce8 100644 --- a/extension/src/test/suite/vscode/outputChannel.test.ts +++ b/extension/src/test/suite/vscode/outputChannel.test.ts @@ -79,10 +79,10 @@ suite('Output Channel Test Suite', () => { command: 'some command', cwd, duration: 20, + errorOutput: + 'THIS IS AN IMPOSSIBLE ERROR. THIS ERROR CANNOT OCCUR. IF THIS ERROR OCCURS, SEE YOUR IBM REPRESENTATIVE.', exitCode: -9, - pid: 12345, - stderr: - 'THIS IS AN IMPOSSIBLE ERROR. THIS ERROR CANNOT OCCUR. IF THIS ERROR OCCURS, SEE YOUR IBM REPRESENTATIVE.' + pid: 12345 }) expect(mockOutputChannel).to.be.called diff --git a/extension/src/vscode/outputChannel.ts b/extension/src/vscode/outputChannel.ts index d59b611fc7..65db25841e 100644 --- a/extension/src/vscode/outputChannel.ts +++ b/extension/src/vscode/outputChannel.ts @@ -41,15 +41,17 @@ export class OutputChannel extends Disposable { private onDidCompleteProcess(cli: ICli) { this.dispose.track( cli.onDidCompleteProcess( - ({ command, duration, exitCode, pid, stderr }) => { + ({ command, duration, exitCode, pid, errorOutput }) => { const processStatus = - exitCode && stderr ? ProcessStatus.FAILED : ProcessStatus.COMPLETED + exitCode && errorOutput + ? ProcessStatus.FAILED + : ProcessStatus.COMPLETED const baseOutput = this.getBaseOutput(pid, command, processStatus) const completionOutput = this.getCompletionOutput( exitCode, duration, - stderr + errorOutput ) return this.outputChannel.append(`${baseOutput}${completionOutput}\n`) @@ -75,7 +77,7 @@ export class OutputChannel extends Disposable { private getCompletionOutput( exitCode: number | null, duration: number, - stderr?: string + errorOutput?: string ) { let completionOutput = '' if (exitCode) { @@ -84,8 +86,8 @@ export class OutputChannel extends Disposable { completionOutput += ` (${duration}ms)` - if (exitCode && stderr) { - completionOutput += `\n${stderr}` + if (exitCode && errorOutput) { + completionOutput += `\n${errorOutput}` } return completionOutput From 5c77dee4e40783e9441e6cf41867348be0c0e555 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 10 May 2023 15:15:46 +1000 Subject: [PATCH 2/2] add integration test --- extension/src/cli/index.ts | 7 ++--- extension/src/cli/util.ts | 6 ++--- extension/src/test/suite/cli/failed.ts | 7 +++++ extension/src/test/suite/cli/index.test.ts | 31 ++++++++++++++++++++++ extension/src/test/suite/cli/util.ts | 4 ++- 5 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 extension/src/test/suite/cli/failed.ts diff --git a/extension/src/cli/index.ts b/extension/src/cli/index.ts index 1678d17390..c95e1223f3 100644 --- a/extension/src/cli/index.ts +++ b/extension/src/cli/index.ts @@ -80,10 +80,7 @@ export class Cli extends Disposable implements ICli { void this.dispose.untrack(process) }) - process.all?.on( - 'data', - chunk => (all += transformChunkToString(chunk as Buffer)) - ) + process.all?.on('data', chunk => (all += transformChunkToString(chunk))) const { stdout, exitCode } = await process @@ -119,7 +116,7 @@ export class Cli extends Disposable implements ICli { backgroundProcess.all?.on( 'data', - chunk => (all += transformChunkToString(chunk as Buffer)) + chunk => (all += transformChunkToString(chunk)) ) return await this.getOutputAndDisconnect( diff --git a/extension/src/cli/util.ts b/extension/src/cli/util.ts index 772a82fd97..7a14b20a9e 100644 --- a/extension/src/cli/util.ts +++ b/extension/src/cli/util.ts @@ -7,8 +7,8 @@ export const notifyStarted = ( processStarted: EventEmitter ): void => processStarted.fire(baseEvent) -export const transformChunkToString = (chunk: Buffer): string => - chunk +export const transformChunkToString = (chunk: unknown): string => + (chunk as Buffer) .toString() .split(/(\r?\n)/g) .join('\r') @@ -18,7 +18,7 @@ export const notifyOutput = ( processOutput: EventEmitter ): void => { process.all?.on('data', chunk => - processOutput.fire(transformChunkToString(chunk as Buffer)) + processOutput.fire(transformChunkToString(chunk)) ) } diff --git a/extension/src/test/suite/cli/failed.ts b/extension/src/test/suite/cli/failed.ts new file mode 100644 index 0000000000..dadada885e --- /dev/null +++ b/extension/src/test/suite/cli/failed.ts @@ -0,0 +1,7 @@ +import { Logger } from '../../../common/logger' +import { delay } from '../../../util/time' + +void delay(2000).then(() => { + Logger.log('this is some stdout') + throw new Error('and this is some stderr') +}) diff --git a/extension/src/test/suite/cli/index.test.ts b/extension/src/test/suite/cli/index.test.ts index bb38d7af75..3362a30798 100644 --- a/extension/src/test/suite/cli/index.test.ts +++ b/extension/src/test/suite/cli/index.test.ts @@ -8,6 +8,7 @@ import { getTimeSafeDisposer } from '../util' import { createProcess, processExists } from '../../../process/execution' import { createValidInteger } from '../../../util/number' import { Cli, CliEvent, CliResult } from '../../../cli' +import { MaybeConsoleError } from '../../../cli/error' suite('CLI Suite', () => { const disposable = getTimeSafeDisposer() @@ -20,6 +21,7 @@ suite('CLI Suite', () => { disposable.dispose() }) + // eslint-disable-next-line sonarjs/cognitive-complexity describe('Cli', () => { it('should be able to create a background process that does not exit when the parent process exits', async () => { const options = getOptions('child') @@ -100,5 +102,34 @@ suite('CLI Suite', () => { expect(processIsStillExecuting).to.be.false }) + + it('should capture all of the stdout and stderr and send it through the processCompleted event emitter if the process fails', async () => { + const processStarted = disposable.track(new EventEmitter()) + const processCompleted = disposable.track(new EventEmitter()) + + const cli = disposable.track( + new Cli({ processCompleted, processStarted }) + ) + + const processCompletedEvent = new Promise(resolve => + disposable.track(processCompleted.event(event => resolve(event))) + ) + const options = getOptions('failed') + + let maybeConsoleError + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (cli as any).executeProcess(options) + } catch (error: unknown) { + maybeConsoleError = error as MaybeConsoleError + } + + expect(maybeConsoleError?.stderr).not.to.contain('stdout') + expect(maybeConsoleError?.stderr).to.contain('stderr') + + const { errorOutput } = await processCompletedEvent + expect(errorOutput).to.contain('stdout') + expect(errorOutput).to.contain('stderr') + }) }) }) diff --git a/extension/src/test/suite/cli/util.ts b/extension/src/test/suite/cli/util.ts index 9993928cca..679f2522b0 100644 --- a/extension/src/test/suite/cli/util.ts +++ b/extension/src/test/suite/cli/util.ts @@ -1,7 +1,9 @@ import { join } from 'path' import type { ProcessOptions } from '../../../process/execution' -export const getOptions = (file: 'child' | 'background'): ProcessOptions => ({ +export const getOptions = ( + file: 'child' | 'background' | 'failed' +): ProcessOptions => ({ args: [join(__dirname, `${file}.js`)], cwd: __dirname, executable: 'node'