diff --git a/extension/src/cli/dvc/runner.ts b/extension/src/cli/dvc/runner.ts index 40236a8b18..e3344c442b 100644 --- a/extension/src/cli/dvc/runner.ts +++ b/extension/src/cli/dvc/runner.ts @@ -12,10 +12,14 @@ import { Config } from '../../config' import { PseudoTerminal } from '../../vscode/pseudoTerminal' import { createProcess, Process } from '../../process/execution' import { StopWatch } from '../../util/time' -import { sendErrorTelemetryEvent, sendTelemetryEvent } from '../../telemetry' -import { EventName } from '../../telemetry/constants' import { Toast } from '../../vscode/toast' import { Disposable } from '../../class/dispose' +import { + captureStdErr, + notifyCompleted, + notifyOutput, + notifyStarted +} from '../util' export const autoRegisteredCommands = { EXPERIMENT_RESET_AND_RUN: 'runExperimentReset', @@ -167,25 +171,23 @@ export class DvcRunner extends Disposable implements ICli { const process = this.dispose.track(createProcess(options)) const baseEvent = { command, cwd, pid: process.pid } - this.processStarted.fire(baseEvent) + notifyStarted(baseEvent, this.processStarted) - this.notifyOutput(process) + notifyOutput(process, this.processOutput) - let stderr = '' - process.stderr?.on( - 'data', - chunk => (stderr += (chunk as Buffer).toString()) - ) + const stderr = captureStdErr(process) void process.on('close', exitCode => { void this.dispose.untrack(process) - this.notifyCompleted({ - ...baseEvent, - duration: stopWatch.getElapsedTime(), - exitCode, - killed: process.killed, - stderr - }) + notifyCompleted( + { + ...baseEvent, + duration: stopWatch.getElapsedTime(), + exitCode, + stderr + }, + this.processCompleted + ) }) return process @@ -208,69 +210,4 @@ export class DvcRunner extends Disposable implements ICli { cwd }) } - - private notifyOutput(process: Process) { - process.all?.on('data', chunk => - this.processOutput.fire( - (chunk as Buffer) - .toString() - .split(/(\r?\n)/g) - .join('\r') - ) - ) - } - - private notifyCompleted({ - command, - pid, - cwd, - duration, - exitCode, - killed, - stderr - }: CliResult & { - killed: boolean - }) { - this.processCompleted.fire({ - command, - cwd, - duration, - exitCode, - pid, - stderr: stderr?.replace(/\n+/g, '\n') - }) - - this.sendTelemetryEvent({ command, duration, exitCode, killed, stderr }) - } - - private sendTelemetryEvent({ - command, - exitCode, - stderr, - duration, - killed - }: { - command: string - exitCode: number | null - stderr?: string - duration: number - killed: boolean - }) { - const properties = { command, exitCode } - - if (!killed && exitCode && stderr) { - return sendErrorTelemetryEvent( - EventName.EXPERIMENTS_RUNNER_COMPLETED, - new Error(stderr), - duration, - properties - ) - } - - return sendTelemetryEvent( - EventName.EXPERIMENTS_RUNNER_COMPLETED, - { ...properties, wasStopped: killed }, - { duration } - ) - } } diff --git a/extension/src/cli/index.ts b/extension/src/cli/index.ts index 5d41ec26ba..2f4a816870 100644 --- a/extension/src/cli/index.ts +++ b/extension/src/cli/index.ts @@ -1,6 +1,7 @@ import { Event, EventEmitter } from 'vscode' -import { CliError, MaybeConsoleError } from './error' import { getCommandString } from './command' +import { CliError, MaybeConsoleError } from './error' +import { notifyCompleted, notifyStarted } from './util' import { createProcess, Process, ProcessOptions } from '../process/execution' import { StopWatch } from '../util/time' import { Disposable } from '../class/dispose' @@ -80,11 +81,15 @@ export class Cli extends Disposable implements ICli { const { stdout, exitCode } = await process - this.processCompleted.fire({ - ...baseEvent, - duration: stopWatch.getElapsedTime(), - exitCode - }) + notifyCompleted( + { + ...baseEvent, + duration: stopWatch.getElapsedTime(), + exitCode + }, + this.processCompleted + ) + return stdout } catch (error: unknown) { throw this.processCliError( @@ -121,7 +126,7 @@ export class Cli extends Disposable implements ICli { private createProcess(baseEvent: CliEvent, options: ProcessOptions) { const createdProcess = createProcess(options) baseEvent.pid = createdProcess.pid - this.processStarted.fire(baseEvent) + notifyStarted(baseEvent, this.processStarted) return createdProcess } @@ -171,12 +176,15 @@ export class Cli extends Disposable implements ICli { baseError: error, options }) - this.processCompleted.fire({ - ...baseEvent, - duration, - exitCode: cliError.exitCode, - stderr: cliError.stderr - }) + notifyCompleted( + { + ...baseEvent, + duration, + exitCode: cliError.exitCode, + stderr: cliError.stderr + }, + this.processCompleted + ) return cliError } } diff --git a/extension/src/cli/util.ts b/extension/src/cli/util.ts new file mode 100644 index 0000000000..4be5a6a3df --- /dev/null +++ b/extension/src/cli/util.ts @@ -0,0 +1,41 @@ +import { EventEmitter } from 'vscode' +import { CliEvent, CliResult } from '.' +import { Process } from '../process/execution' + +export const notifyStarted = ( + baseEvent: CliEvent, + processStarted: EventEmitter +): void => processStarted.fire(baseEvent) + +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') + ) + ) +} + +export const notifyCompleted = ( + { command, pid, cwd, duration, exitCode, stderr }: CliResult, + processCompleted: EventEmitter +): void => + processCompleted.fire({ + command, + cwd, + duration, + exitCode, + pid, + stderr: stderr?.replace(/\n+/g, '\n') + }) + +export const captureStdErr = (process: Process): string => { + let stderr = '' + process.stderr?.on('data', chunk => (stderr += (chunk as Buffer).toString())) + return stderr +} diff --git a/extension/src/cli/viewable.ts b/extension/src/cli/viewable.ts new file mode 100644 index 0000000000..662baefb9f --- /dev/null +++ b/extension/src/cli/viewable.ts @@ -0,0 +1,88 @@ +import { Event, EventEmitter } from 'vscode' +import { CliEvent, CliResult, CliStarted } from '.' +import { + captureStdErr, + notifyCompleted, + notifyOutput, + notifyStarted +} from './util' +import { getCommandString } from './command' +import { ProcessOptions, createProcess } from '../process/execution' +import { StopWatch } from '../util/time' +import { PseudoTerminal } from '../vscode/pseudoTerminal' +import { DeferredDisposable } from '../class/deferred' + +export class ViewableCliProcess extends DeferredDisposable { + public readonly onDidDispose: Event + + private readonly pseudoTerminal: PseudoTerminal + + constructor( + termName: string, + options: ProcessOptions, + processStarted: EventEmitter, + processCompleted: EventEmitter + ) { + super() + const processOutput = this.dispose.track(new EventEmitter()) + const terminalClosed = this.dispose.track(new EventEmitter()) + const onDidCloseTerminal = terminalClosed.event + + this.pseudoTerminal = this.dispose.track( + new PseudoTerminal(processOutput, terminalClosed, termName) + ) + + this.pseudoTerminal.setBlocked(true) + void this.show().then(() => this.deferred.resolve()) + + this.createProcess(options, processStarted, processOutput, processCompleted) + + const disposed = this.dispose.track(new EventEmitter()) + this.onDidDispose = disposed.event + + this.dispose.track( + onDidCloseTerminal(() => { + disposed.fire() + this.dispose() + }) + ) + } + + public show() { + return this.pseudoTerminal.openCurrentInstance() + } + + private createProcess( + options: ProcessOptions, + processStarted: EventEmitter, + processOutput: EventEmitter, + processCompleted: EventEmitter + ) { + const stopWatch = new StopWatch() + const command = getCommandString(options) + processOutput.fire(`Running: ${command}\r\n\n`) + const process = this.dispose.track(createProcess(options)) + + const baseEvent = { command, cwd: options.cwd, pid: process.pid } + + notifyStarted(baseEvent, processStarted) + + notifyOutput(process, processOutput) + + const stderr = captureStdErr(process) + + void process.on('close', exitCode => { + void this.dispose.untrack(process) + notifyCompleted( + { + ...baseEvent, + duration: stopWatch.getElapsedTime(), + exitCode, + stderr + }, + processCompleted + ) + processOutput.fire('\r\nPress any key to close\r\n\n') + }) + } +} diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index 0a36317f38..cf346e051f 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -22,8 +22,6 @@ export type ViewOpenedEventName = export const EventName = Object.assign( { - EXPERIMENTS_RUNNER_COMPLETED: 'experiments.runner.completed', - EXTENSION_EXECUTION_DETAILS_CHANGED: 'extension.executionDetails.changed', EXTENSION_LOAD: 'extension.load', @@ -118,12 +116,6 @@ export interface IEventNamePropertyMapping { [EventName.EXTENSION_EXECUTION_DETAILS_CHANGED]: ExtensionProperties [EventName.EXTENSION_LOAD]: ExtensionProperties - [EventName.EXPERIMENTS_RUNNER_COMPLETED]: { - command: string - exitCode: number | null - wasStopped?: boolean - } - [EventName.EXPERIMENT_AND_PLOTS_SHOW]: undefined [EventName.EXPERIMENT_APPLY]: undefined [EventName.EXPERIMENT_BRANCH]: undefined diff --git a/extension/src/telemetry/index.ts b/extension/src/telemetry/index.ts index 9ad48af560..e5775e7185 100644 --- a/extension/src/telemetry/index.ts +++ b/extension/src/telemetry/index.ts @@ -62,7 +62,12 @@ const sanitizeProperties = ( if (value === undefined || value === null) { continue } - sanitizeProperty(eventName as string, sanitizedProperties, key, value) + sanitizeProperty( + eventName, + sanitizedProperties, + key, + value as string | number | boolean + ) } return sanitizedProperties } diff --git a/extension/src/test/suite/cli/dvc/runner.test.ts b/extension/src/test/suite/cli/dvc/runner.test.ts index dfeebbe47f..f72d64cc78 100644 --- a/extension/src/test/suite/cli/dvc/runner.test.ts +++ b/extension/src/test/suite/cli/dvc/runner.test.ts @@ -1,13 +1,11 @@ import { afterEach, beforeEach, describe, it, suite } from 'mocha' import { expect } from 'chai' -import { restore, spy, stub } from 'sinon' +import { restore, spy } from 'sinon' import { window, Event, EventEmitter } from 'vscode' import { Disposable, Disposer } from '../../../../extension' import { Config } from '../../../../config' import { DvcRunner } from '../../../../cli/dvc/runner' import { CliResult, CliStarted } from '../../../../cli' -import * as Telemetry from '../../../../telemetry' -import { EventName } from '../../../../telemetry/constants' import { WEBVIEW_TEST_TIMEOUT } from '../../timeouts' import { spyOnPrivateMemberMethod } from '../../util' @@ -125,38 +123,5 @@ suite('DVC Runner Test Suite', () => { expect(output.includes(text)).to.be.true return completed }).timeout(WEBVIEW_TEST_TIMEOUT) - - it('should send an error event if the command fails with an exit code and stderr', async () => { - const mockSendTelemetryEvent = stub(Telemetry, 'sendErrorTelemetryEvent') - - const mockConfig = { getPythonBinPath: () => undefined } as Config - const dvcRunner = disposable.track(new DvcRunner(mockConfig, 'sleep')) - - const cwd = __dirname - - await dvcRunner.run(cwd, '1', '&&', 'then', 'die') - const process = dvcRunner.getRunningProcess() - - const processCompleted = new Promise(resolve => { - void process?.on('close', () => resolve(undefined)) - }) - - await expect(process).to.eventually.be.rejectedWith(Error) - - await processCompleted - - const [eventName, error, , properties] = - mockSendTelemetryEvent.getCall(0).args - - const { command, exitCode } = properties as { - command: string - exitCode?: number | undefined - } - - expect(eventName).to.equal(EventName.EXPERIMENTS_RUNNER_COMPLETED) - expect(error.message).to.have.length.greaterThan(0) - expect(command).to.equal('sleep 1 && then die') - expect(exitCode).to.be.greaterThan(0) - }).timeout(WEBVIEW_TEST_TIMEOUT) }) }) diff --git a/extension/src/test/suite/cli/viewable.test.ts b/extension/src/test/suite/cli/viewable.test.ts new file mode 100644 index 0000000000..658a837298 --- /dev/null +++ b/extension/src/test/suite/cli/viewable.test.ts @@ -0,0 +1,93 @@ +import { afterEach, beforeEach, describe, it, suite } from 'mocha' +import { restore, stub } from 'sinon' +import { EventEmitter, Terminal, window } from 'vscode' +import { expect } from 'chai' +import { Disposable } from '../../../extension' +import { CliResult, CliStarted } from '../../../cli' +import { ProcessOptions } from '../../../process/execution' +import { ViewableCliProcess } from '../../../cli/viewable' +import { dvcDemoPath } from '../../util' +import { closeAllTerminals } from '../util' + +suite('Viewable CLI Process Test Suite', () => { + const disposable = Disposable.fn() + + beforeEach(() => { + restore() + }) + + afterEach(() => { + disposable.dispose() + }) + + describe('ViewableCliProcess', () => { + it('should open a pseudoTerminal when created', async () => { + const termName = 'I open' + const options: ProcessOptions = { + args: ['100'], + cwd: dvcDemoPath, + executable: 'sleep' + } + const processStarted = disposable.track(new EventEmitter()) + const processCompleted = disposable.track(new EventEmitter()) + + const onDidStartProcess = processStarted.event + + const processStartedEvent = new Promise(resolve => + onDidStartProcess(() => resolve(undefined)) + ) + + const mockCreateTerminal = stub(window, 'createTerminal').returns({ + dispose: () => undefined + } as Terminal) + + const viewableCliProcess = disposable.track( + new ViewableCliProcess( + termName, + options, + processStarted, + processCompleted + ) + ) + + await processStartedEvent + expect(mockCreateTerminal).to.be.called + + viewableCliProcess.dispose() + }).timeout(8000) + + it('should call dispose when the pseudoTerminal is closed', async () => { + const termName = 'Close Me' + const options: ProcessOptions = { + args: ['10000000'], + cwd: dvcDemoPath, + executable: 'sleep' + } + const processStarted = disposable.track(new EventEmitter()) + const processCompleted = disposable.track(new EventEmitter()) + + const viewableCliProcess = disposable.track( + new ViewableCliProcess( + termName, + options, + processStarted, + processCompleted + ) + ) + + const disposedEvent = new Promise(resolve => + disposable.track( + viewableCliProcess.onDidDispose(() => { + resolve(undefined) + }) + ) + ) + + await viewableCliProcess.isReady() + + await closeAllTerminals() + + await disposedEvent + }).timeout(8000) + }) +}) diff --git a/extension/src/test/suite/util.ts b/extension/src/test/suite/util.ts index fb4c1feb67..f93ef57993 100644 --- a/extension/src/test/suite/util.ts +++ b/extension/src/test/suite/util.ts @@ -113,6 +113,9 @@ export const closeAllEditors = async () => { } } +export const closeAllTerminals = () => + commands.executeCommand('workbench.action.terminal.killAll') + export const mockDuration = (duration: number) => stub(Time, 'getCurrentEpoch') .onFirstCall()