From 1904ec4a53b49b1c1c79ae86395e38a906e11ed6 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Mon, 24 Jun 2019 09:09:11 -0700 Subject: [PATCH 01/21] News file --- news/3 Code Health/1131.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/3 Code Health/1131.md diff --git a/news/3 Code Health/1131.md b/news/3 Code Health/1131.md new file mode 100644 index 000000000000..a188997d2b96 --- /dev/null +++ b/news/3 Code Health/1131.md @@ -0,0 +1 @@ +Log all extension commands being run behind the scenes in the extension output panel. From b53b0815aa2a161c1f494e6b9582b85efc9bf321 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Mon, 24 Jun 2019 10:40:52 -0700 Subject: [PATCH 02/21] Add logging to ProcessService --- src/client/common/process/proc.ts | 35 ++++++++++++++- src/client/common/process/processFactory.ts | 8 +++- .../common/process/pythonExecutionFactory.ts | 8 +++- src/client/common/utils/localize.ts | 4 ++ .../Common/processServiceFactory.ts | 8 +++- src/test/common.ts | 16 +++---- src/test/common/process/proc.exec.test.ts | 45 +++++++++++-------- .../common/process/proc.observable.test.ts | 42 ++++++++++------- .../pythonProc.simple.multiroot.test.ts | 5 ++- src/test/datascience/executionServiceMock.ts | 10 ++++- src/test/format/extension.format.test.ts | 2 +- .../jedi/autocomplete/base.test.ts | 8 +++- .../jedi/autocomplete/pep526.test.ts | 8 +++- .../extension.refactor.extract.var.test.ts | 4 +- src/test/refactor/rename.test.ts | 10 ++++- .../terminals/codeExecution/helper.test.ts | 24 ++++++---- 16 files changed, 165 insertions(+), 72 deletions(-) diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index f046aac94813..f797428028db 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -1,10 +1,15 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { exec, execSync, spawn } from 'child_process'; +import * as path from 'path'; import { Observable } from 'rxjs/Observable'; +import { window } from 'vscode'; -import { IDisposable } from '../types'; +import { IWorkspaceService } from '../application/types'; +import { traceInfo } from '../logger'; +import { IDisposable, IOutputChannel } from '../types'; import { createDeferred } from '../utils/async'; +import { Logging } from '../utils/localize'; import { EnvironmentVariables } from '../variables/types'; import { DEFAULT_ENCODING } from './constants'; import { @@ -21,7 +26,7 @@ import { // tslint:disable:no-any export class ProcessService implements IProcessService, IDisposable { private processesToKill = new Set(); - constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { } + constructor(private readonly decoder: IBufferDecoder, private readonly output: IOutputChannel, private readonly workspaceService?: IWorkspaceService, private readonly env?: EnvironmentVariables) { } public static isAlive(pid: number): boolean { try { process.kill(pid, 0); @@ -115,6 +120,8 @@ export class ProcessService implements IProcessService, IDisposable { }); }); + this.logProcessExec(file, args, options); + return { proc, out: output, @@ -175,11 +182,16 @@ export class ProcessService implements IProcessService, IDisposable { disposables.forEach(d => d.dispose()); }); + this.logProcessExec(file, args, options); + return deferred.promise; } public shellExec(command: string, options: ShellOptions = {}): Promise> { const shellOptions = this.getDefaultOptions(options); + + this.logProcessExec(command, [], options); + return new Promise((resolve, reject) => { const proc = exec(command, shellOptions, (e, stdout, stderr) => { if (e && e !== null) { @@ -227,4 +239,23 @@ export class ProcessService implements IProcessService, IDisposable { return defaultOptions; } + private logProcessExec(file: string, args: string[], options: SpawnOptions) { + const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); + let currentWorkingDirectory; + + if (this.workspaceService && this.workspaceService.hasWorkspaceFolders) { + currentWorkingDirectory = this.workspaceService.workspaceFolders![0].uri.fsPath; + } else { + const openFile = window.activeTextEditor ? window.activeTextEditor!.document.uri : undefined; + currentWorkingDirectory = openFile ? path.dirname(openFile.fsPath) : options.cwd!; + } + const info = [ + `> ${file} ${formattedArgs}`, + `${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}` + ].join('\n'); + + traceInfo(info); + this.output.appendLine(info); + } + } diff --git a/src/client/common/process/processFactory.ts b/src/client/common/process/processFactory.ts index 17fbf6bbbe18..d8ef2e5e664a 100644 --- a/src/client/common/process/processFactory.ts +++ b/src/client/common/process/processFactory.ts @@ -6,7 +6,9 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; import { IServiceContainer } from '../../ioc/types'; -import { IDisposableRegistry } from '../types'; +import { IWorkspaceService } from '../application/types'; +import { STANDARD_OUTPUT_CHANNEL } from '../constants'; +import { IDisposableRegistry, IOutputChannel } from '../types'; import { IEnvironmentVariablesProvider } from '../variables/types'; import { ProcessService } from './proc'; import { IBufferDecoder, IProcessService, IProcessServiceFactory } from './types'; @@ -21,7 +23,9 @@ export class ProcessServiceFactory implements IProcessServiceFactory { const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource); const decoder = this.serviceContainer.get(IBufferDecoder); const disposableRegistry = this.serviceContainer.get(IDisposableRegistry); - const proc = new ProcessService(decoder, customEnvVars); + const output = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + const workspaceService = this.serviceContainer.get(IWorkspaceService); + const proc = new ProcessService(decoder, output, workspaceService, customEnvVars); disposableRegistry.push(proc); return proc; } diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index 1fe1ee7fb9e2..e495c5a86718 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -6,7 +6,9 @@ import { IEnvironmentActivationService } from '../../interpreter/activation/type import { IServiceContainer } from '../../ioc/types'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; -import { IConfigurationService, IDisposableRegistry } from '../types'; +import { IWorkspaceService } from '../application/types'; +import { STANDARD_OUTPUT_CHANNEL } from '../constants'; +import { IConfigurationService, IDisposableRegistry, IOutputChannel } from '../types'; import { ProcessService } from './proc'; import { PythonExecutionService } from './pythonProcess'; import { @@ -39,7 +41,9 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { return this.create({ resource: options.resource, pythonPath: options.interpreter ? options.interpreter.path : undefined }); } const pythonPath = options.interpreter ? options.interpreter.path : this.configService.getSettings(options.resource).pythonPath; - const processService = new ProcessService(this.decoder, { ...envVars }); + const output = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + const workspaceService = this.serviceContainer.get(IWorkspaceService); + const processService = new ProcessService(this.decoder, output, workspaceService, { ...envVars }); this.serviceContainer.get(IDisposableRegistry).push(processService); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index cc4d5096c446..aea5d78ca465 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -59,6 +59,10 @@ export namespace Interpreters { export const selectInterpreterTip = localize('Interpreters.selectInterpreterTip', 'Tip: you can change the Python interpreter used by the Python extension by clicking on the Python version in the status bar'); } +export namespace Logging { + export const currentWorkingDirectory = localize('Logging.CurrentWorkingDirectory', 'Current working directory:'); +} + export namespace Linters { export const enableLinter = localize('Linter.enableLinter', 'Enable {0}'); export const enablePylint = localize('Linter.enablePylint', 'You have a pylintrc file in your workspace. Do you want to enable pylint?'); diff --git a/src/client/debugger/debugAdapter/Common/processServiceFactory.ts b/src/client/debugger/debugAdapter/Common/processServiceFactory.ts index cc969f52d8fd..8f568cd28a8d 100644 --- a/src/client/debugger/debugAdapter/Common/processServiceFactory.ts +++ b/src/client/debugger/debugAdapter/Common/processServiceFactory.ts @@ -4,16 +4,20 @@ 'use strict'; import { inject, injectable } from 'inversify'; +import { IWorkspaceService } from '../../../common/application/types'; +import { STANDARD_OUTPUT_CHANNEL } from '../../../common/constants'; import { ProcessService } from '../../../common/process/proc'; import { IBufferDecoder, IProcessService, IProcessServiceFactory } from '../../../common/process/types'; -import { IDisposableRegistry } from '../../../common/types'; +import { IDisposableRegistry, IOutputChannel } from '../../../common/types'; import { IServiceContainer } from '../../../ioc/types'; @injectable() export class DebuggerProcessServiceFactory implements IProcessServiceFactory { constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { } public create(): Promise { - const processService = new ProcessService(this.serviceContainer.get(IBufferDecoder), process.env); + const output = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + const workspaceService = this.serviceContainer.get(IWorkspaceService); + const processService = new ProcessService(this.serviceContainer.get(IBufferDecoder), output, workspaceService, process.env); this.serviceContainer.get(IDisposableRegistry).push(processService); return Promise.resolve(processService); } diff --git a/src/test/common.ts b/src/test/common.ts index 1a32f4128104..26210ac55f32 100644 --- a/src/test/common.ts +++ b/src/test/common.ts @@ -11,8 +11,8 @@ import * as path from 'path'; import { coerce, SemVer } from 'semver'; import { ConfigurationTarget, Event, TextDocument, Uri } from 'vscode'; import { IExtensionApi } from '../client/api'; -import { IProcessService } from '../client/common/process/types'; -import { IPythonSettings, Resource } from '../client/common/types'; +import { ExecutionResult, IProcessService } from '../client/common/process/types'; +import { IOutputChannel, IPythonSettings, Resource } from '../client/common/types'; import { PythonInterpreter } from '../client/interpreter/contracts'; import { IServiceContainer, IServiceManager } from '../client/ioc/types'; import { @@ -255,16 +255,16 @@ export function correctPathForOsType(pathToCorrect: string, os?: OSType): string * @param {procService} IProcessService Optionally specify the IProcessService implementation to use to execute with. * @return `SemVer` version of the Python interpreter, or `undefined` if an error occurs. */ -export async function getPythonSemVer(procService?: IProcessService): Promise { +export async function getPythonSemVer(procService?: IProcessService, outputChannel?: IOutputChannel): Promise { const decoder = await import('../client/common/process/decoder'); const proc = await import('../client/common/process/proc'); - const pythonProcRunner = procService ? procService : new proc.ProcessService(new decoder.BufferDecoder()); + const pythonProcRunner = procService ? procService : new proc.ProcessService(new decoder.BufferDecoder(), outputChannel as IOutputChannel); const pyVerArgs = ['-c', 'import sys;print("{0}.{1}.{2}".format(*sys.version_info[:3]))']; return pythonProcRunner.exec(PYTHON_PATH, pyVerArgs) - .then(strVersion => new SemVer(strVersion.stdout.trim())) - .catch((err) => { + .then((strVersion: ExecutionResult) => new SemVer(strVersion.stdout.trim())) + .catch((err: Error) => { // if the call fails this should make it loudly apparent. console.error('Failed to get Python Version in getPythonSemVer', err); return undefined; @@ -376,8 +376,8 @@ export async function isPythonVersionInProcess(procService?: IProcessService, .. * @param {resource} vscode.Uri Current workspace resource Uri or undefined. * @return true if the current Python version matches a version in the skip list, false otherwise. */ -export async function isPythonVersion(...versions: string[]): Promise { - const currentPyVersion = await getPythonSemVer(); +export async function isPythonVersion(outputChannel: IOutputChannel, ...versions: string[]): Promise { + const currentPyVersion = await getPythonSemVer(undefined, outputChannel); if (currentPyVersion) { return isVersionInList(currentPyVersion, ...versions); } else { diff --git a/src/test/common/process/proc.exec.test.ts b/src/test/common/process/proc.exec.test.ts index 0edddb562da6..8990eb5ad7b0 100644 --- a/src/test/common/process/proc.exec.test.ts +++ b/src/test/common/process/proc.exec.test.ts @@ -5,10 +5,13 @@ import { expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; +import * as typeMoq from 'typemoq'; import { CancellationTokenSource } from 'vscode'; +import { IWorkspaceService } from '../../../client/common/application/types'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; import { StdErrError } from '../../../client/common/process/types'; +import { IOutputChannel } from '../../../client/common/types'; import { OSType } from '../../../client/common/utils/platform'; import { getExtensionSettings, isOs, isPythonVersion } from '../../common'; import { initialize } from './../../initialize'; @@ -18,15 +21,19 @@ use(chaiAsPromised); // tslint:disable-next-line:max-func-body-length suite('ProcessService Observable', () => { let pythonPath: string; + let outputChannel: typeMoq.IMock; + let workspaceService: typeMoq.IMock; suiteSetup(() => { pythonPath = getExtensionSettings(undefined).pythonPath; + outputChannel = typeMoq.Mock.ofType(); + workspaceService = typeMoq.Mock.ofType(); return initialize(); }); setup(initialize); teardown(initialize); test('exec should output print statements', async () => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const printOutput = '1234'; const result = await procService.exec(pythonPath, ['-c', `print("${printOutput}")`]); @@ -38,12 +45,12 @@ suite('ProcessService Observable', () => { test('exec should output print unicode characters', async function () { // This test has not been working for many months in Python 2.7 under // Windows. Tracked by #2546. (unicode under Py2.7 is tough!) - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const printOutput = 'öä'; const result = await procService.exec(pythonPath, ['-c', `print("${printOutput}")`]); @@ -55,7 +62,7 @@ suite('ProcessService Observable', () => { test('exec should wait for completion of program with new lines', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(5000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(1)', @@ -64,7 +71,7 @@ suite('ProcessService Observable', () => { const outputs = ['1', '2', '3']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const values = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); + const values = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); expect(values).to.deep.equal(outputs, 'Output values are incorrect'); expect(result.stderr).to.equal(undefined, 'stderr not undefined'); }); @@ -72,7 +79,7 @@ suite('ProcessService Observable', () => { test('exec should wait for completion of program without new lines', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(5000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'sys.stdout.write("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'sys.stdout.write("2")', 'sys.stdout.flush()', 'time.sleep(1)', @@ -81,7 +88,7 @@ suite('ProcessService Observable', () => { const outputs = ['123']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const values = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); + const values = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); expect(values).to.deep.equal(outputs, 'Output values are incorrect'); expect(result.stderr).to.equal(undefined, 'stderr not undefined'); }); @@ -89,7 +96,7 @@ suite('ProcessService Observable', () => { test('exec should end when cancellationToken is cancelled', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(15000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(10)', 'print("2")', 'sys.stdout.flush()']; @@ -99,7 +106,7 @@ suite('ProcessService Observable', () => { const result = await procService.exec(pythonPath, ['-c', pythonCode.join(';')], { token: cancellationToken.token }); expect(result).not.to.be.an('undefined', 'result is undefined'); - const values = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); + const values = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); expect(values).to.deep.equal(['1'], 'Output values are incorrect'); expect(result.stderr).to.equal(undefined, 'stderr not undefined'); }); @@ -107,7 +114,7 @@ suite('ProcessService Observable', () => { test('exec should stream stdout and stderr separately', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(7000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'sys.stderr.write("a")', 'sys.stderr.flush()', 'time.sleep(1)', @@ -120,16 +127,16 @@ suite('ProcessService Observable', () => { const expectedStderr = ['abc']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const stdouts = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); + const stdouts = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); expect(stdouts).to.deep.equal(expectedStdout, 'stdout values are incorrect'); - const stderrs = result.stderr!.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); + const stderrs = result.stderr!.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); expect(stderrs).to.deep.equal(expectedStderr, 'stderr values are incorrect'); }); test('exec should merge stdout and stderr streams', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(7000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'sys.stdout.write("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'sys.stderr.write("a")', 'sys.stderr.flush()', 'time.sleep(1)', @@ -141,12 +148,12 @@ suite('ProcessService Observable', () => { const expectedOutput = ['1a2b3c']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const outputs = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); + const outputs = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); expect(outputs).to.deep.equal(expectedOutput, 'Output values are incorrect'); }); test('exec should throw an error with stderr output', async () => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'sys.stderr.write("a")', 'sys.stderr.flush()']; const result = procService.exec(pythonPath, ['-c', pythonCode.join(';')], { throwOnStdErr: true }); @@ -154,21 +161,21 @@ suite('ProcessService Observable', () => { }); test('exec should throw an error when spawn file not found', async () => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const result = procService.exec(Date.now().toString(), []); await expect(result).to.eventually.be.rejected.and.to.have.property('code', 'ENOENT', 'Invalid error code'); }); test('exec should exit without no output', async () => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const result = await procService.exec(pythonPath, ['-c', 'import sys', 'sys.exit()']); expect(result.stdout).equals('', 'stdout is invalid'); expect(result.stderr).equals(undefined, 'stderr is invalid'); }); test('shellExec should be able to run python too', async () => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const printOutput = '1234'; const result = await procService.shellExec(`"${pythonPath}" -c "print('${printOutput}')"`); @@ -177,7 +184,7 @@ suite('ProcessService Observable', () => { expect(result.stdout.trim()).to.be.equal(printOutput, 'Invalid output'); }); test('shellExec should fail on invalid command', async () => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const result = procService.shellExec('invalid command'); await expect(result).to.eventually.be.rejectedWith(Error, 'a', 'Expected error to be thrown'); }); diff --git a/src/test/common/process/proc.observable.test.ts b/src/test/common/process/proc.observable.test.ts index b6f597ed054e..6d0974874a2e 100644 --- a/src/test/common/process/proc.observable.test.ts +++ b/src/test/common/process/proc.observable.test.ts @@ -3,9 +3,13 @@ import { expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; +import * as typeMoq from 'typemoq'; import { CancellationTokenSource } from 'vscode'; +import { IWorkspaceService } from '../../../client/common/application/types'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; +import { Output } from '../../../client/common/process/types'; +import { IOutputChannel } from '../../../client/common/types'; import { createDeferred } from '../../../client/common/utils/async'; import { getExtensionSettings, isOs, OSType } from '../../common'; import { initialize } from './../../initialize'; @@ -15,8 +19,12 @@ use(chaiAsPromised); // tslint:disable-next-line:max-func-body-length suite('ProcessService', () => { let pythonPath: string; + let outputChannel: typeMoq.IMock; + let workspaceService: typeMoq.IMock; suiteSetup(() => { pythonPath = getExtensionSettings(undefined).pythonPath; + outputChannel = typeMoq.Mock.ofType(); + workspaceService = typeMoq.Mock.ofType(); return initialize(); }); setup(initialize); @@ -25,7 +33,7 @@ suite('ProcessService', () => { test('execObservable should stream output with new lines', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(10000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(2)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(2)', @@ -34,7 +42,7 @@ suite('ProcessService', () => { const outputs = ['1', '2', '3']; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe(output => { + result.out.subscribe((output: Output) => { // Ignore line breaks. if (output.out.trim().length === 0) { return; @@ -52,7 +60,7 @@ suite('ProcessService', () => { test('execObservable should stream output without new lines', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(10000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'sys.stdout.write("1")', 'sys.stdout.flush()', 'time.sleep(2)', 'sys.stdout.write("2")', 'sys.stdout.flush()', 'time.sleep(2)', @@ -61,7 +69,7 @@ suite('ProcessService', () => { const outputs = ['1', '2', '3']; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe(output => { + result.out.subscribe((output: Output) => { // Ignore line breaks. if (output.out.trim().length === 0) { return; @@ -79,7 +87,7 @@ suite('ProcessService', () => { test('execObservable should end when cancellationToken is cancelled', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(15000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(10)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(2)']; @@ -89,7 +97,7 @@ suite('ProcessService', () => { const def = createDeferred(); def.promise.then(done).catch(done); expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe(output => { + result.out.subscribe((output: Output) => { const value = output.out.trim(); if (value === '1') { cancellationToken.cancel(); @@ -113,7 +121,7 @@ suite('ProcessService', () => { test('execObservable should end when process is killed', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(15000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(10)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(2)']; @@ -122,7 +130,7 @@ suite('ProcessService', () => { let procKilled = false; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe(output => { + result.out.subscribe((output: Output) => { const value = output.out.trim(); // Ignore line breaks. if (value.length === 0) { @@ -145,7 +153,7 @@ suite('ProcessService', () => { test('execObservable should stream stdout and stderr separately', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(20000); - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(2)', 'sys.stderr.write("a")', 'sys.stderr.flush()', 'time.sleep(2)', @@ -160,7 +168,7 @@ suite('ProcessService', () => { { out: '3', source: 'stdout' }, { out: 'c', source: 'stderr' }]; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe(output => { + result.out.subscribe((output: Output) => { const value = output.out.trim(); // Ignore line breaks. if (value.length === 0) { @@ -182,12 +190,12 @@ suite('ProcessService', () => { }); test('execObservable should throw an error with stderr output', (done) => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const pythonCode = ['import sys', 'sys.stderr.write("a")', 'sys.stderr.flush()']; const result = procService.execObservable(pythonPath, ['-c', pythonCode.join(';')], { throwOnStdErr: true }); expect(result).not.to.be.an('undefined', 'result is undefined.'); - result.out.subscribe(_output => { + result.out.subscribe((_output: Output) => { done('Output received, when we\'re expecting an error to be thrown.'); }, (ex: Error) => { expect(ex).to.have.property('message', 'a', 'Invalid error thrown'); @@ -198,13 +206,13 @@ suite('ProcessService', () => { }); test('execObservable should throw an error when spawn file not found', (done) => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const result = procService.execObservable(Date.now().toString(), []); expect(result).not.to.be.an('undefined', 'result is undefined.'); - result.out.subscribe(_output => { + result.out.subscribe((_output: Output) => { done('Output received, when we\'re expecting an error to be thrown.'); - }, ex => { + }, (ex: Error) => { expect(ex).to.have.property('code', 'ENOENT', 'Invalid error code'); done(); }, () => { @@ -213,11 +221,11 @@ suite('ProcessService', () => { }); test('execObservable should exit without no output', (done) => { - const procService = new ProcessService(new BufferDecoder()); + const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); const result = procService.execObservable(pythonPath, ['-c', 'import sys', 'sys.exit()']); expect(result).not.to.be.an('undefined', 'result is undefined.'); - result.out.subscribe(output => { + result.out.subscribe((output: Output) => { done(`Output received, when we\'re not expecting any, ${JSON.stringify(output)}`); }, done, done); }); diff --git a/src/test/common/process/pythonProc.simple.multiroot.test.ts b/src/test/common/process/pythonProc.simple.multiroot.test.ts index 4b7a93fbc79b..725444ed3fc7 100644 --- a/src/test/common/process/pythonProc.simple.multiroot.test.ts +++ b/src/test/common/process/pythonProc.simple.multiroot.test.ts @@ -11,7 +11,7 @@ import { Container } from 'inversify'; import { EOL } from 'os'; import * as path from 'path'; import { anything, instance, mock, when } from 'ts-mockito'; -import { ConfigurationTarget, Disposable, Uri } from 'vscode'; +import { ConfigurationTarget, Disposable, Uri, window } from 'vscode'; import { IWorkspaceService } from '../../../client/common/application/types'; import { WorkspaceService } from '../../../client/common/application/workspace'; import { ConfigurationService } from '../../../client/common/configuration/service'; @@ -123,7 +123,8 @@ suite('PythonExecutableService', () => { test('Importing with a valid PYTHONPATH from .env file should succeed', async function () { // This test has not been working for many months in Python 2.7 under // Windows. Tracked by #2547. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + const output = window.createOutputChannel('Tests'); + if (isOs(OSType.Windows) && await isPythonVersion(output, '2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } diff --git a/src/test/datascience/executionServiceMock.ts b/src/test/datascience/executionServiceMock.ts index aefb095b50cb..351e8d88a920 100644 --- a/src/test/datascience/executionServiceMock.ts +++ b/src/test/datascience/executionServiceMock.ts @@ -2,6 +2,8 @@ // Licensed under the MIT License. 'use strict'; import { SemVer } from 'semver'; +import { IWorkspaceService } from '../../client/common/application/types'; +import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants'; import { ErrorUtils } from '../../client/common/errors/errorUtils'; import { ModuleNotInstalledError } from '../../client/common/errors/moduleNotInstalledError'; import { BufferDecoder } from '../../client/common/process/decoder'; @@ -13,15 +15,19 @@ import { ObservableExecutionResult, SpawnOptions } from '../../client/common/process/types'; +import { IOutputChannel } from '../../client/common/types'; import { Architecture } from '../../client/common/utils/platform'; +import { IServiceContainer } from '../../client/ioc/types'; export class MockPythonExecutionService implements IPythonExecutionService { private procService : ProcessService; private pythonPath : string = 'python'; - constructor() { - this.procService = new ProcessService(new BufferDecoder()); + constructor(serviceContainer: IServiceContainer) { + const output = serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + const workspaceService = serviceContainer.get(IWorkspaceService); + this.procService = new ProcessService(new BufferDecoder(), output, workspaceService); } public getInterpreterInformation(): Promise { return Promise.resolve( diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index b8da5f609703..659d0b9a48c3 100644 --- a/src/test/format/extension.format.test.ts +++ b/src/test/format/extension.format.test.ts @@ -18,8 +18,8 @@ import { CondaService } from '../../client/interpreter/locators/services/condaSe import { isPythonVersionInProcess } from '../common'; import { closeActiveWindows, initialize, initializeTest } from '../initialize'; import { MockProcessService } from '../mocks/proc'; -import { compareFiles } from '../textUtils'; import { UnitTestIocContainer } from '../testing/serviceRegistry'; +import { compareFiles } from '../textUtils'; const ch = window.createOutputChannel('Tests'); const formatFilesPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting'); diff --git a/src/test/languageServers/jedi/autocomplete/base.test.ts b/src/test/languageServers/jedi/autocomplete/base.test.ts index 3b9a60e6159b..58d046a8516a 100644 --- a/src/test/languageServers/jedi/autocomplete/base.test.ts +++ b/src/test/languageServers/jedi/autocomplete/base.test.ts @@ -21,6 +21,7 @@ const fileDecorator = path.join(autoCompPath, 'deco.py'); const fileEncoding = path.join(autoCompPath, 'four.py'); const fileEncodingUsed = path.join(autoCompPath, 'five.py'); const fileSuppress = path.join(autoCompPath, 'suppress.py'); +const output = vscode.window.createOutputChannel('Tests'); // tslint:disable-next-line:max-func-body-length suite('Autocomplete Base Tests', function () { @@ -37,7 +38,10 @@ suite('Autocomplete Base Tests', function () { initializeDI(); }); setup(initializeTest); - suiteTeardown(closeActiveWindows); + suiteTeardown(async () => { + output.dispose(); + await closeActiveWindows(); + }); teardown(async () => { await closeActiveWindows(); await ioc.dispose(); @@ -96,7 +100,7 @@ suite('Autocomplete Base Tests', function () { // https://github.com/DonJayamanne/pythonVSCode/issues/265 test('For "lambda"', async function () { - if (await isPythonVersion('2')) { + if (await isPythonVersion(output, '2')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } diff --git a/src/test/languageServers/jedi/autocomplete/pep526.test.ts b/src/test/languageServers/jedi/autocomplete/pep526.test.ts index 0ce3cc50cb7f..80e68aeb0f9d 100644 --- a/src/test/languageServers/jedi/autocomplete/pep526.test.ts +++ b/src/test/languageServers/jedi/autocomplete/pep526.test.ts @@ -16,13 +16,14 @@ import { UnitTestIocContainer } from '../../../testing/serviceRegistry'; const autoCompPath = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'pythonFiles', 'autocomp'); const filePep526 = path.join(autoCompPath, 'pep526.py'); +const output = vscode.window.createOutputChannel('Tests'); // tslint:disable-next-line:max-func-body-length suite('Autocomplete PEP 526', () => { let ioc: UnitTestIocContainer; suiteSetup(async function () { // Pep526 only valid for 3.6+ (#2545) - if (await isPythonVersion('2', '3.4', '3.5')) { + if (await isPythonVersion(output, '2', '3.4', '3.5')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -31,7 +32,10 @@ suite('Autocomplete PEP 526', () => { initializeDI(); }); setup(initializeTest); - suiteTeardown(closeActiveWindows); + suiteTeardown(async () => { + output.dispose(); + await closeActiveWindows(); + }); teardown(async () => { await closeActiveWindows(); await ioc.dispose(); diff --git a/src/test/refactor/extension.refactor.extract.var.test.ts b/src/test/refactor/extension.refactor.extract.var.test.ts index 7652d5398326..4dff81e70864 100644 --- a/src/test/refactor/extension.refactor.extract.var.test.ts +++ b/src/test/refactor/extension.refactor.extract.var.test.ts @@ -15,6 +15,7 @@ import { MockOutputChannel } from './../mockClasses'; const EXTENSION_DIR = path.join(__dirname, '..', '..', '..'); const refactorSourceFile = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'refactoring', 'standAlone', 'refactor.py'); const refactorTargetFileDir = path.join(__dirname, '..', '..', '..', 'out', 'test', 'pythonFiles', 'refactoring', 'standAlone'); +const output = window.createOutputChannel('Tests'); interface RenameResponse { results: [{ diff: string }]; @@ -29,6 +30,7 @@ suite('Variable Extraction', () => { suiteSetup(initialize); suiteTeardown(() => { commands.executeCommand = oldExecuteCommand; + output.dispose(); return closeActiveWindows(); }); setup(async () => { @@ -82,7 +84,7 @@ suite('Variable Extraction', () => { // tslint:disable-next-line:no-function-expression test('Extract Variable', async function () { - if (isPythonVersion('3.7')) { + if (isPythonVersion(output, '3.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } else { diff --git a/src/test/refactor/rename.test.ts b/src/test/refactor/rename.test.ts index 6459701abc8e..a3691ccf7b0d 100644 --- a/src/test/refactor/rename.test.ts +++ b/src/test/refactor/rename.test.ts @@ -8,13 +8,14 @@ import { EOL } from 'os'; import * as path from 'path'; import * as typeMoq from 'typemoq'; import { Range, TextEditorCursorStyle, TextEditorLineNumbersStyle, TextEditorOptions, window, workspace } from 'vscode'; +import { IWorkspaceService } from '../../client/common/application/types'; import { EXTENSION_ROOT_DIR } from '../../client/common/constants'; import '../../client/common/extensions'; import { BufferDecoder } from '../../client/common/process/decoder'; import { ProcessService } from '../../client/common/process/proc'; import { PythonExecutionFactory } from '../../client/common/process/pythonExecutionFactory'; import { IProcessServiceFactory, IPythonExecutionFactory } from '../../client/common/process/types'; -import { IConfigurationService, IPythonSettings } from '../../client/common/types'; +import { IConfigurationService, IOutputChannel, IPythonSettings } from '../../client/common/types'; import { IEnvironmentActivationService } from '../../client/interpreter/activation/types'; import { IServiceContainer } from '../../client/ioc/types'; import { RefactorProxy } from '../../client/refactor/proxy'; @@ -31,14 +32,19 @@ suite('Refactor Rename', () => { const options: TextEditorOptions = { cursorStyle: TextEditorCursorStyle.Line, insertSpaces: true, lineNumbers: TextEditorLineNumbersStyle.Off, tabSize: 4 }; let pythonSettings: typeMoq.IMock; let serviceContainer: typeMoq.IMock; + let outputChannel: typeMoq.IMock; + let workspaceService: typeMoq.IMock; + suiteSetup(initialize); setup(async () => { + outputChannel = typeMoq.Mock.ofType(); + workspaceService = typeMoq.Mock.ofType(); pythonSettings = typeMoq.Mock.ofType(); pythonSettings.setup(p => p.pythonPath).returns(() => PYTHON_PATH); const configService = typeMoq.Mock.ofType(); configService.setup(c => c.getSettings(typeMoq.It.isAny())).returns(() => pythonSettings.object); const processServiceFactory = typeMoq.Mock.ofType(); - processServiceFactory.setup(p => p.create(typeMoq.It.isAny())).returns(() => Promise.resolve(new ProcessService(new BufferDecoder()))); + processServiceFactory.setup(p => p.create(typeMoq.It.isAny())).returns(() => Promise.resolve(new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object))); const envActivationService = typeMoq.Mock.ofType(); envActivationService.setup(e => e.getActivatedEnvironmentVariables(typeMoq.It.isAny())).returns(() => Promise.resolve(undefined)); serviceContainer = typeMoq.Mock.ofType(); diff --git a/src/test/terminals/codeExecution/helper.test.ts b/src/test/terminals/codeExecution/helper.test.ts index be8185f2b238..fb1f68fc7456 100644 --- a/src/test/terminals/codeExecution/helper.test.ts +++ b/src/test/terminals/codeExecution/helper.test.ts @@ -9,13 +9,13 @@ import { EOL } from 'os'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; import { Range, Selection, TextDocument, TextEditor, TextLine, Uri } from 'vscode'; -import { IApplicationShell, IDocumentManager } from '../../../client/common/application/types'; +import { IApplicationShell, IDocumentManager, IWorkspaceService } from '../../../client/common/application/types'; import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../../client/common/constants'; import '../../../client/common/extensions'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; -import { IConfigurationService, IPythonSettings } from '../../../client/common/types'; +import { IConfigurationService, IOutputChannel, IPythonSettings } from '../../../client/common/types'; import { OSType } from '../../../client/common/utils/platform'; import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -29,6 +29,9 @@ const TEST_FILES_PATH = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'pythonFile suite('Terminal - Code Execution Helper', () => { let documentManager: TypeMoq.IMock; let applicationShell: TypeMoq.IMock; + let outputChannel: TypeMoq.IMock; + let workspaceService: TypeMoq.IMock; + let helper: ICodeExecutionHelper; let document: TypeMoq.IMock; let editor: TypeMoq.IMock; @@ -38,6 +41,9 @@ suite('Terminal - Code Execution Helper', () => { const serviceContainer = TypeMoq.Mock.ofType(); documentManager = TypeMoq.Mock.ofType(); applicationShell = TypeMoq.Mock.ofType(); + outputChannel = TypeMoq.Mock.ofType(); + workspaceService = TypeMoq.Mock.ofType(); + const envVariablesProvider = TypeMoq.Mock.ofType(); processService = TypeMoq.Mock.ofType(); configService = TypeMoq.Mock.ofType(); @@ -54,6 +60,8 @@ suite('Terminal - Code Execution Helper', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => applicationShell.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider), TypeMoq.It.isAny())).returns(() => envVariablesProvider.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IConfigurationService), TypeMoq.It.isAny())).returns(() => configService.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProcessServiceFactory), TypeMoq.It.isAny())).returns(() => processServiceFactory.object); + helper = new CodeExecutionHelper(serviceContainer.object); document = TypeMoq.Mock.ofType(); @@ -62,7 +70,7 @@ suite('Terminal - Code Execution Helper', () => { }); async function ensureBlankLinesAreRemoved(source: string, expectedSource: string) { - const actualProcessService = new ProcessService(new BufferDecoder()); + const actualProcessService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns((file, args, options) => { return actualProcessService.exec.apply(actualProcessService, [file, args, options]); @@ -75,7 +83,7 @@ suite('Terminal - Code Execution Helper', () => { test('Ensure blank lines are NOT removed when code is not indented (simple)', async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -86,7 +94,7 @@ suite('Terminal - Code Execution Helper', () => { }); test('Ensure there are no multiple-CR elements in the normalized code.', async () => { const code = ['import sys', '', '', '', 'print(sys.executable)', '', 'print("1234")', '', '', 'print(1)', 'print(2)']; - const actualProcessService = new ProcessService(new BufferDecoder()); + const actualProcessService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns((file, args, options) => { return actualProcessService.exec.apply(actualProcessService, [file, args, options]); @@ -99,7 +107,7 @@ suite('Terminal - Code Execution Helper', () => { test(`Ensure blank lines are removed (Sample${fileNameSuffix})`, async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -111,7 +119,7 @@ suite('Terminal - Code Execution Helper', () => { test(`Ensure last two blank lines are preserved (Sample${fileNameSuffix})`, async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -123,7 +131,7 @@ suite('Terminal - Code Execution Helper', () => { test(`Ensure last two blank lines are preserved even if we have more than 2 trailing blank lines (Sample${fileNameSuffix})`, async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } From 9d5ec6f2bbca2c0ccbdabb6e8e8a4e08e2a898e3 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Mon, 24 Jun 2019 19:08:57 -0700 Subject: [PATCH 03/21] Undo changes, move to a new class --- src/client/common/process/logger.ts | 42 +++++++++++++++++ src/client/common/process/proc.ts | 43 +++++------------- src/client/common/process/processFactory.ts | 13 +++--- .../common/process/pythonExecutionFactory.ts | 13 +++--- src/client/common/process/types.ts | 14 +++++- src/client/common/serviceRegistry.ts | 3 ++ .../Common/processServiceFactory.ts | 8 +--- src/test/common.ts | 16 +++---- src/test/common/process/proc.exec.test.ts | 45 ++++++++----------- .../common/process/proc.observable.test.ts | 42 +++++++---------- .../pythonProc.simple.multiroot.test.ts | 5 +-- src/test/datascience/executionServiceMock.ts | 10 +---- src/test/datascience/mockProcessService.ts | 7 +++ src/test/format/extension.format.test.ts | 2 +- .../jedi/autocomplete/base.test.ts | 8 +--- .../jedi/autocomplete/pep526.test.ts | 8 +--- src/test/mocks/proc.ts | 6 +++ .../extension.refactor.extract.var.test.ts | 4 +- src/test/refactor/rename.test.ts | 10 +---- .../terminals/codeExecution/helper.test.ts | 24 ++++------ 20 files changed, 162 insertions(+), 161 deletions(-) create mode 100644 src/client/common/process/logger.ts diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts new file mode 100644 index 000000000000..c5f4c4465f94 --- /dev/null +++ b/src/client/common/process/logger.ts @@ -0,0 +1,42 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +import { inject, injectable } from 'inversify'; +import * as path from 'path'; +import { window } from 'vscode'; +import { IServiceContainer } from '../../ioc/types'; +import { IWorkspaceService } from '../application/types'; +import { STANDARD_OUTPUT_CHANNEL } from '../constants'; + +import { traceInfo } from '../logger'; +import { IOutputChannel } from '../types'; +import { Logging } from '../utils/localize'; +import { IProcessLogger, ProcessServiceEvent } from './types'; + +@injectable() +export class ProcessLogger implements IProcessLogger { + private outputChannel: IOutputChannel; + private workspaceService: IWorkspaceService; + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.outputChannel = serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + this.workspaceService = serviceContainer.get(IWorkspaceService); + } + + public logProcess({ file, args, options }: ProcessServiceEvent) { + const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); + let currentWorkingDirectory; + + if (this.workspaceService.hasWorkspaceFolders) { + currentWorkingDirectory = this.workspaceService.workspaceFolders![0].uri.fsPath; + } else { + const openFile = window.activeTextEditor !== undefined ? window.activeTextEditor.document.uri : undefined; + currentWorkingDirectory = openFile ? path.dirname(openFile.fsPath) : options.cwd!; + } + const info = [ + `> ${file} ${formattedArgs}`, + `${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}` + ].join('\n'); + + traceInfo(info); + this.outputChannel.appendLine(info); + } +} diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index f797428028db..e47a2f6f258e 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -1,15 +1,11 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { exec, execSync, spawn } from 'child_process'; -import * as path from 'path'; import { Observable } from 'rxjs/Observable'; -import { window } from 'vscode'; +import { Event, EventEmitter } from 'vscode'; -import { IWorkspaceService } from '../application/types'; -import { traceInfo } from '../logger'; -import { IDisposable, IOutputChannel } from '../types'; +import { IDisposable } from '../types'; import { createDeferred } from '../utils/async'; -import { Logging } from '../utils/localize'; import { EnvironmentVariables } from '../variables/types'; import { DEFAULT_ENCODING } from './constants'; import { @@ -18,6 +14,7 @@ import { IProcessService, ObservableExecutionResult, Output, + ProcessServiceEvent, ShellOptions, SpawnOptions, StdErrError @@ -26,7 +23,8 @@ import { // tslint:disable:no-any export class ProcessService implements IProcessService, IDisposable { private processesToKill = new Set(); - constructor(private readonly decoder: IBufferDecoder, private readonly output: IOutputChannel, private readonly workspaceService?: IWorkspaceService, private readonly env?: EnvironmentVariables) { } + private readonly onProcessExecuted = new EventEmitter(); + constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { } public static isAlive(pid: number): boolean { try { process.kill(pid, 0); @@ -48,6 +46,7 @@ export class ProcessService implements IProcessService, IDisposable { } } public dispose() { + this.onProcessExecuted.dispose(); this.processesToKill.forEach(p => { try { p.dispose(); @@ -57,6 +56,10 @@ export class ProcessService implements IProcessService, IDisposable { }); } + public get processExecutedEvent(): Event { + return this.onProcessExecuted.event; + } + public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult { const spawnOptions = this.getDefaultOptions(options); const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; @@ -120,7 +123,7 @@ export class ProcessService implements IProcessService, IDisposable { }); }); - this.logProcessExec(file, args, options); + this.onProcessExecuted.fire({ file, args, options }); return { proc, @@ -182,16 +185,13 @@ export class ProcessService implements IProcessService, IDisposable { disposables.forEach(d => d.dispose()); }); - this.logProcessExec(file, args, options); + this.onProcessExecuted.fire({ file, args, options }); return deferred.promise; } public shellExec(command: string, options: ShellOptions = {}): Promise> { const shellOptions = this.getDefaultOptions(options); - - this.logProcessExec(command, [], options); - return new Promise((resolve, reject) => { const proc = exec(command, shellOptions, (e, stdout, stderr) => { if (e && e !== null) { @@ -239,23 +239,4 @@ export class ProcessService implements IProcessService, IDisposable { return defaultOptions; } - private logProcessExec(file: string, args: string[], options: SpawnOptions) { - const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); - let currentWorkingDirectory; - - if (this.workspaceService && this.workspaceService.hasWorkspaceFolders) { - currentWorkingDirectory = this.workspaceService.workspaceFolders![0].uri.fsPath; - } else { - const openFile = window.activeTextEditor ? window.activeTextEditor!.document.uri : undefined; - currentWorkingDirectory = openFile ? path.dirname(openFile.fsPath) : options.cwd!; - } - const info = [ - `> ${file} ${formattedArgs}`, - `${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}` - ].join('\n'); - - traceInfo(info); - this.output.appendLine(info); - } - } diff --git a/src/client/common/process/processFactory.ts b/src/client/common/process/processFactory.ts index d8ef2e5e664a..d1826b4e0751 100644 --- a/src/client/common/process/processFactory.ts +++ b/src/client/common/process/processFactory.ts @@ -6,27 +6,26 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; import { IServiceContainer } from '../../ioc/types'; -import { IWorkspaceService } from '../application/types'; -import { STANDARD_OUTPUT_CHANNEL } from '../constants'; -import { IDisposableRegistry, IOutputChannel } from '../types'; +import { IDisposableRegistry } from '../types'; import { IEnvironmentVariablesProvider } from '../variables/types'; import { ProcessService } from './proc'; -import { IBufferDecoder, IProcessService, IProcessServiceFactory } from './types'; +import { IBufferDecoder, IProcessLogger, IProcessService, IProcessServiceFactory } from './types'; @injectable() export class ProcessServiceFactory implements IProcessServiceFactory { private envVarsService: IEnvironmentVariablesProvider; + private processLogger: IProcessLogger; constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.envVarsService = serviceContainer.get(IEnvironmentVariablesProvider); + this.processLogger = serviceContainer.get(IProcessLogger); } public async create(resource?: Uri): Promise { const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource); const decoder = this.serviceContainer.get(IBufferDecoder); const disposableRegistry = this.serviceContainer.get(IDisposableRegistry); - const output = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); - const workspaceService = this.serviceContainer.get(IWorkspaceService); - const proc = new ProcessService(decoder, output, workspaceService, customEnvVars); + const proc = new ProcessService(decoder, customEnvVars); disposableRegistry.push(proc); + proc.processExecutedEvent(this.processLogger.logProcess, this.processLogger); return proc; } } diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index e495c5a86718..3045af6a9f46 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -6,15 +6,14 @@ import { IEnvironmentActivationService } from '../../interpreter/activation/type import { IServiceContainer } from '../../ioc/types'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; -import { IWorkspaceService } from '../application/types'; -import { STANDARD_OUTPUT_CHANNEL } from '../constants'; -import { IConfigurationService, IDisposableRegistry, IOutputChannel } from '../types'; +import { IConfigurationService, IDisposableRegistry } from '../types'; import { ProcessService } from './proc'; import { PythonExecutionService } from './pythonProcess'; import { ExecutionFactoryCreateWithEnvironmentOptions, ExecutionFactoryCreationOptions, IBufferDecoder, + IProcessLogger, IProcessServiceFactory, IPythonExecutionFactory, IPythonExecutionService @@ -31,6 +30,8 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { public async create(options: ExecutionFactoryCreationOptions): Promise { const pythonPath = options.pythonPath ? options.pythonPath : this.configService.getSettings(options.resource).pythonPath; const processService = await this.processServiceFactory.create(options.resource); + const processLogger = this.serviceContainer.get(IProcessLogger); + processService.processExecutedEvent(processLogger.logProcess, processLogger); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } public async createActivatedEnvironment(options: ExecutionFactoryCreateWithEnvironmentOptions): Promise { @@ -41,9 +42,9 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { return this.create({ resource: options.resource, pythonPath: options.interpreter ? options.interpreter.path : undefined }); } const pythonPath = options.interpreter ? options.interpreter.path : this.configService.getSettings(options.resource).pythonPath; - const output = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); - const workspaceService = this.serviceContainer.get(IWorkspaceService); - const processService = new ProcessService(this.decoder, output, workspaceService, { ...envVars }); + const processService = new ProcessService(this.decoder, { ...envVars }); + const processLogger = this.serviceContainer.get(IProcessLogger); + processService.processExecutedEvent(processLogger.logProcess, processLogger); this.serviceContainer.get(IDisposableRegistry).push(processService); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index a48e82930b5c..467b3f74943f 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. import { ChildProcess, ExecOptions, SpawnOptions as ChildProcessSpawnOptions } from 'child_process'; import { Observable } from 'rxjs/Observable'; -import { CancellationToken, Uri } from 'vscode'; +import { CancellationToken, Event, Uri } from 'vscode'; import { PythonInterpreter } from '../../interpreter/contracts'; import { ExecutionInfo, Version } from '../types'; @@ -40,7 +40,19 @@ export type ExecutionResult = { stderr?: T; }; +export type ProcessServiceEvent = { + file: string; + args: string[]; + options: SpawnOptions; +}; + +export const IProcessLogger = Symbol('IProcessLogger'); +export interface IProcessLogger { + logProcess(event: ProcessServiceEvent): void; +} + export interface IProcessService { + readonly processExecutedEvent: Event; execObservable(file: string, args: string[], options?: SpawnOptions): ObservableExecutionResult; exec(file: string, args: string[], options?: SpawnOptions): Promise>; shellExec(command: string, options?: ShellOptions): Promise>; diff --git a/src/client/common/serviceRegistry.ts b/src/client/common/serviceRegistry.ts index 3994718fca47..1cf0541319e2 100644 --- a/src/client/common/serviceRegistry.ts +++ b/src/client/common/serviceRegistry.ts @@ -41,6 +41,8 @@ import { PersistentStateFactory } from './persistentState'; import { IS_WINDOWS } from './platform/constants'; import { PathUtils } from './platform/pathUtils'; import { CurrentProcess } from './process/currentProcess'; +import { ProcessLogger } from './process/logger'; +import { IProcessLogger } from './process/types'; import { TerminalActivator } from './terminal/activator'; import { PowershellTerminalActivationFailedHandler } from './terminal/activator/powershellFailedHandler'; import { Bash } from './terminal/environmentActivationProviders/bash'; @@ -93,6 +95,7 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(ICommandManager, CommandManager); serviceManager.addSingleton(IConfigurationService, ConfigurationService); serviceManager.addSingleton(IWorkspaceService, WorkspaceService); + serviceManager.addSingleton(IProcessLogger, ProcessLogger); serviceManager.addSingleton(IDocumentManager, DocumentManager); serviceManager.addSingleton(ITerminalManager, TerminalManager); serviceManager.addSingleton(IDebugService, DebugService); diff --git a/src/client/debugger/debugAdapter/Common/processServiceFactory.ts b/src/client/debugger/debugAdapter/Common/processServiceFactory.ts index 8f568cd28a8d..cc969f52d8fd 100644 --- a/src/client/debugger/debugAdapter/Common/processServiceFactory.ts +++ b/src/client/debugger/debugAdapter/Common/processServiceFactory.ts @@ -4,20 +4,16 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { IWorkspaceService } from '../../../common/application/types'; -import { STANDARD_OUTPUT_CHANNEL } from '../../../common/constants'; import { ProcessService } from '../../../common/process/proc'; import { IBufferDecoder, IProcessService, IProcessServiceFactory } from '../../../common/process/types'; -import { IDisposableRegistry, IOutputChannel } from '../../../common/types'; +import { IDisposableRegistry } from '../../../common/types'; import { IServiceContainer } from '../../../ioc/types'; @injectable() export class DebuggerProcessServiceFactory implements IProcessServiceFactory { constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { } public create(): Promise { - const output = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); - const workspaceService = this.serviceContainer.get(IWorkspaceService); - const processService = new ProcessService(this.serviceContainer.get(IBufferDecoder), output, workspaceService, process.env); + const processService = new ProcessService(this.serviceContainer.get(IBufferDecoder), process.env); this.serviceContainer.get(IDisposableRegistry).push(processService); return Promise.resolve(processService); } diff --git a/src/test/common.ts b/src/test/common.ts index 26210ac55f32..1a32f4128104 100644 --- a/src/test/common.ts +++ b/src/test/common.ts @@ -11,8 +11,8 @@ import * as path from 'path'; import { coerce, SemVer } from 'semver'; import { ConfigurationTarget, Event, TextDocument, Uri } from 'vscode'; import { IExtensionApi } from '../client/api'; -import { ExecutionResult, IProcessService } from '../client/common/process/types'; -import { IOutputChannel, IPythonSettings, Resource } from '../client/common/types'; +import { IProcessService } from '../client/common/process/types'; +import { IPythonSettings, Resource } from '../client/common/types'; import { PythonInterpreter } from '../client/interpreter/contracts'; import { IServiceContainer, IServiceManager } from '../client/ioc/types'; import { @@ -255,16 +255,16 @@ export function correctPathForOsType(pathToCorrect: string, os?: OSType): string * @param {procService} IProcessService Optionally specify the IProcessService implementation to use to execute with. * @return `SemVer` version of the Python interpreter, or `undefined` if an error occurs. */ -export async function getPythonSemVer(procService?: IProcessService, outputChannel?: IOutputChannel): Promise { +export async function getPythonSemVer(procService?: IProcessService): Promise { const decoder = await import('../client/common/process/decoder'); const proc = await import('../client/common/process/proc'); - const pythonProcRunner = procService ? procService : new proc.ProcessService(new decoder.BufferDecoder(), outputChannel as IOutputChannel); + const pythonProcRunner = procService ? procService : new proc.ProcessService(new decoder.BufferDecoder()); const pyVerArgs = ['-c', 'import sys;print("{0}.{1}.{2}".format(*sys.version_info[:3]))']; return pythonProcRunner.exec(PYTHON_PATH, pyVerArgs) - .then((strVersion: ExecutionResult) => new SemVer(strVersion.stdout.trim())) - .catch((err: Error) => { + .then(strVersion => new SemVer(strVersion.stdout.trim())) + .catch((err) => { // if the call fails this should make it loudly apparent. console.error('Failed to get Python Version in getPythonSemVer', err); return undefined; @@ -376,8 +376,8 @@ export async function isPythonVersionInProcess(procService?: IProcessService, .. * @param {resource} vscode.Uri Current workspace resource Uri or undefined. * @return true if the current Python version matches a version in the skip list, false otherwise. */ -export async function isPythonVersion(outputChannel: IOutputChannel, ...versions: string[]): Promise { - const currentPyVersion = await getPythonSemVer(undefined, outputChannel); +export async function isPythonVersion(...versions: string[]): Promise { + const currentPyVersion = await getPythonSemVer(); if (currentPyVersion) { return isVersionInList(currentPyVersion, ...versions); } else { diff --git a/src/test/common/process/proc.exec.test.ts b/src/test/common/process/proc.exec.test.ts index 8990eb5ad7b0..0edddb562da6 100644 --- a/src/test/common/process/proc.exec.test.ts +++ b/src/test/common/process/proc.exec.test.ts @@ -5,13 +5,10 @@ import { expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; -import * as typeMoq from 'typemoq'; import { CancellationTokenSource } from 'vscode'; -import { IWorkspaceService } from '../../../client/common/application/types'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; import { StdErrError } from '../../../client/common/process/types'; -import { IOutputChannel } from '../../../client/common/types'; import { OSType } from '../../../client/common/utils/platform'; import { getExtensionSettings, isOs, isPythonVersion } from '../../common'; import { initialize } from './../../initialize'; @@ -21,19 +18,15 @@ use(chaiAsPromised); // tslint:disable-next-line:max-func-body-length suite('ProcessService Observable', () => { let pythonPath: string; - let outputChannel: typeMoq.IMock; - let workspaceService: typeMoq.IMock; suiteSetup(() => { pythonPath = getExtensionSettings(undefined).pythonPath; - outputChannel = typeMoq.Mock.ofType(); - workspaceService = typeMoq.Mock.ofType(); return initialize(); }); setup(initialize); teardown(initialize); test('exec should output print statements', async () => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const printOutput = '1234'; const result = await procService.exec(pythonPath, ['-c', `print("${printOutput}")`]); @@ -45,12 +38,12 @@ suite('ProcessService Observable', () => { test('exec should output print unicode characters', async function () { // This test has not been working for many months in Python 2.7 under // Windows. Tracked by #2546. (unicode under Py2.7 is tough!) - if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const printOutput = 'öä'; const result = await procService.exec(pythonPath, ['-c', `print("${printOutput}")`]); @@ -62,7 +55,7 @@ suite('ProcessService Observable', () => { test('exec should wait for completion of program with new lines', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(5000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(1)', @@ -71,7 +64,7 @@ suite('ProcessService Observable', () => { const outputs = ['1', '2', '3']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const values = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); + const values = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); expect(values).to.deep.equal(outputs, 'Output values are incorrect'); expect(result.stderr).to.equal(undefined, 'stderr not undefined'); }); @@ -79,7 +72,7 @@ suite('ProcessService Observable', () => { test('exec should wait for completion of program without new lines', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(5000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'sys.stdout.write("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'sys.stdout.write("2")', 'sys.stdout.flush()', 'time.sleep(1)', @@ -88,7 +81,7 @@ suite('ProcessService Observable', () => { const outputs = ['123']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const values = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); + const values = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); expect(values).to.deep.equal(outputs, 'Output values are incorrect'); expect(result.stderr).to.equal(undefined, 'stderr not undefined'); }); @@ -96,7 +89,7 @@ suite('ProcessService Observable', () => { test('exec should end when cancellationToken is cancelled', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(15000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(10)', 'print("2")', 'sys.stdout.flush()']; @@ -106,7 +99,7 @@ suite('ProcessService Observable', () => { const result = await procService.exec(pythonPath, ['-c', pythonCode.join(';')], { token: cancellationToken.token }); expect(result).not.to.be.an('undefined', 'result is undefined'); - const values = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); + const values = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); expect(values).to.deep.equal(['1'], 'Output values are incorrect'); expect(result.stderr).to.equal(undefined, 'stderr not undefined'); }); @@ -114,7 +107,7 @@ suite('ProcessService Observable', () => { test('exec should stream stdout and stderr separately', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(7000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'sys.stderr.write("a")', 'sys.stderr.flush()', 'time.sleep(1)', @@ -127,16 +120,16 @@ suite('ProcessService Observable', () => { const expectedStderr = ['abc']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const stdouts = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); + const stdouts = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); expect(stdouts).to.deep.equal(expectedStdout, 'stdout values are incorrect'); - const stderrs = result.stderr!.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); + const stderrs = result.stderr!.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); expect(stderrs).to.deep.equal(expectedStderr, 'stderr values are incorrect'); }); test('exec should merge stdout and stderr streams', async function () { // tslint:disable-next-line:no-invalid-this this.timeout(7000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'sys.stdout.write("1")', 'sys.stdout.flush()', 'time.sleep(1)', 'sys.stderr.write("a")', 'sys.stderr.flush()', 'time.sleep(1)', @@ -148,12 +141,12 @@ suite('ProcessService Observable', () => { const expectedOutput = ['1a2b3c']; expect(result).not.to.be.an('undefined', 'result is undefined'); - const outputs = result.stdout.split(/\r?\n/g).map((line: string) => line.trim()).filter((line: string) => line.length > 0); + const outputs = result.stdout.split(/\r?\n/g).map(line => line.trim()).filter(line => line.length > 0); expect(outputs).to.deep.equal(expectedOutput, 'Output values are incorrect'); }); test('exec should throw an error with stderr output', async () => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'sys.stderr.write("a")', 'sys.stderr.flush()']; const result = procService.exec(pythonPath, ['-c', pythonCode.join(';')], { throwOnStdErr: true }); @@ -161,21 +154,21 @@ suite('ProcessService Observable', () => { }); test('exec should throw an error when spawn file not found', async () => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const result = procService.exec(Date.now().toString(), []); await expect(result).to.eventually.be.rejected.and.to.have.property('code', 'ENOENT', 'Invalid error code'); }); test('exec should exit without no output', async () => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const result = await procService.exec(pythonPath, ['-c', 'import sys', 'sys.exit()']); expect(result.stdout).equals('', 'stdout is invalid'); expect(result.stderr).equals(undefined, 'stderr is invalid'); }); test('shellExec should be able to run python too', async () => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const printOutput = '1234'; const result = await procService.shellExec(`"${pythonPath}" -c "print('${printOutput}')"`); @@ -184,7 +177,7 @@ suite('ProcessService Observable', () => { expect(result.stdout.trim()).to.be.equal(printOutput, 'Invalid output'); }); test('shellExec should fail on invalid command', async () => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const result = procService.shellExec('invalid command'); await expect(result).to.eventually.be.rejectedWith(Error, 'a', 'Expected error to be thrown'); }); diff --git a/src/test/common/process/proc.observable.test.ts b/src/test/common/process/proc.observable.test.ts index 6d0974874a2e..b6f597ed054e 100644 --- a/src/test/common/process/proc.observable.test.ts +++ b/src/test/common/process/proc.observable.test.ts @@ -3,13 +3,9 @@ import { expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; -import * as typeMoq from 'typemoq'; import { CancellationTokenSource } from 'vscode'; -import { IWorkspaceService } from '../../../client/common/application/types'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; -import { Output } from '../../../client/common/process/types'; -import { IOutputChannel } from '../../../client/common/types'; import { createDeferred } from '../../../client/common/utils/async'; import { getExtensionSettings, isOs, OSType } from '../../common'; import { initialize } from './../../initialize'; @@ -19,12 +15,8 @@ use(chaiAsPromised); // tslint:disable-next-line:max-func-body-length suite('ProcessService', () => { let pythonPath: string; - let outputChannel: typeMoq.IMock; - let workspaceService: typeMoq.IMock; suiteSetup(() => { pythonPath = getExtensionSettings(undefined).pythonPath; - outputChannel = typeMoq.Mock.ofType(); - workspaceService = typeMoq.Mock.ofType(); return initialize(); }); setup(initialize); @@ -33,7 +25,7 @@ suite('ProcessService', () => { test('execObservable should stream output with new lines', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(10000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(2)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(2)', @@ -42,7 +34,7 @@ suite('ProcessService', () => { const outputs = ['1', '2', '3']; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe((output: Output) => { + result.out.subscribe(output => { // Ignore line breaks. if (output.out.trim().length === 0) { return; @@ -60,7 +52,7 @@ suite('ProcessService', () => { test('execObservable should stream output without new lines', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(10000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'sys.stdout.write("1")', 'sys.stdout.flush()', 'time.sleep(2)', 'sys.stdout.write("2")', 'sys.stdout.flush()', 'time.sleep(2)', @@ -69,7 +61,7 @@ suite('ProcessService', () => { const outputs = ['1', '2', '3']; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe((output: Output) => { + result.out.subscribe(output => { // Ignore line breaks. if (output.out.trim().length === 0) { return; @@ -87,7 +79,7 @@ suite('ProcessService', () => { test('execObservable should end when cancellationToken is cancelled', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(15000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(10)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(2)']; @@ -97,7 +89,7 @@ suite('ProcessService', () => { const def = createDeferred(); def.promise.then(done).catch(done); expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe((output: Output) => { + result.out.subscribe(output => { const value = output.out.trim(); if (value === '1') { cancellationToken.cancel(); @@ -121,7 +113,7 @@ suite('ProcessService', () => { test('execObservable should end when process is killed', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(15000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(10)', 'print("2")', 'sys.stdout.flush()', 'time.sleep(2)']; @@ -130,7 +122,7 @@ suite('ProcessService', () => { let procKilled = false; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe((output: Output) => { + result.out.subscribe(output => { const value = output.out.trim(); // Ignore line breaks. if (value.length === 0) { @@ -153,7 +145,7 @@ suite('ProcessService', () => { test('execObservable should stream stdout and stderr separately', function (done) { // tslint:disable-next-line:no-invalid-this this.timeout(20000); - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'import time', 'print("1")', 'sys.stdout.flush()', 'time.sleep(2)', 'sys.stderr.write("a")', 'sys.stderr.flush()', 'time.sleep(2)', @@ -168,7 +160,7 @@ suite('ProcessService', () => { { out: '3', source: 'stdout' }, { out: 'c', source: 'stderr' }]; expect(result).not.to.be.an('undefined', 'result is undefined'); - result.out.subscribe((output: Output) => { + result.out.subscribe(output => { const value = output.out.trim(); // Ignore line breaks. if (value.length === 0) { @@ -190,12 +182,12 @@ suite('ProcessService', () => { }); test('execObservable should throw an error with stderr output', (done) => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const pythonCode = ['import sys', 'sys.stderr.write("a")', 'sys.stderr.flush()']; const result = procService.execObservable(pythonPath, ['-c', pythonCode.join(';')], { throwOnStdErr: true }); expect(result).not.to.be.an('undefined', 'result is undefined.'); - result.out.subscribe((_output: Output) => { + result.out.subscribe(_output => { done('Output received, when we\'re expecting an error to be thrown.'); }, (ex: Error) => { expect(ex).to.have.property('message', 'a', 'Invalid error thrown'); @@ -206,13 +198,13 @@ suite('ProcessService', () => { }); test('execObservable should throw an error when spawn file not found', (done) => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const result = procService.execObservable(Date.now().toString(), []); expect(result).not.to.be.an('undefined', 'result is undefined.'); - result.out.subscribe((_output: Output) => { + result.out.subscribe(_output => { done('Output received, when we\'re expecting an error to be thrown.'); - }, (ex: Error) => { + }, ex => { expect(ex).to.have.property('code', 'ENOENT', 'Invalid error code'); done(); }, () => { @@ -221,11 +213,11 @@ suite('ProcessService', () => { }); test('execObservable should exit without no output', (done) => { - const procService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const procService = new ProcessService(new BufferDecoder()); const result = procService.execObservable(pythonPath, ['-c', 'import sys', 'sys.exit()']); expect(result).not.to.be.an('undefined', 'result is undefined.'); - result.out.subscribe((output: Output) => { + result.out.subscribe(output => { done(`Output received, when we\'re not expecting any, ${JSON.stringify(output)}`); }, done, done); }); diff --git a/src/test/common/process/pythonProc.simple.multiroot.test.ts b/src/test/common/process/pythonProc.simple.multiroot.test.ts index 725444ed3fc7..4b7a93fbc79b 100644 --- a/src/test/common/process/pythonProc.simple.multiroot.test.ts +++ b/src/test/common/process/pythonProc.simple.multiroot.test.ts @@ -11,7 +11,7 @@ import { Container } from 'inversify'; import { EOL } from 'os'; import * as path from 'path'; import { anything, instance, mock, when } from 'ts-mockito'; -import { ConfigurationTarget, Disposable, Uri, window } from 'vscode'; +import { ConfigurationTarget, Disposable, Uri } from 'vscode'; import { IWorkspaceService } from '../../../client/common/application/types'; import { WorkspaceService } from '../../../client/common/application/workspace'; import { ConfigurationService } from '../../../client/common/configuration/service'; @@ -123,8 +123,7 @@ suite('PythonExecutableService', () => { test('Importing with a valid PYTHONPATH from .env file should succeed', async function () { // This test has not been working for many months in Python 2.7 under // Windows. Tracked by #2547. - const output = window.createOutputChannel('Tests'); - if (isOs(OSType.Windows) && await isPythonVersion(output, '2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } diff --git a/src/test/datascience/executionServiceMock.ts b/src/test/datascience/executionServiceMock.ts index 351e8d88a920..aefb095b50cb 100644 --- a/src/test/datascience/executionServiceMock.ts +++ b/src/test/datascience/executionServiceMock.ts @@ -2,8 +2,6 @@ // Licensed under the MIT License. 'use strict'; import { SemVer } from 'semver'; -import { IWorkspaceService } from '../../client/common/application/types'; -import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants'; import { ErrorUtils } from '../../client/common/errors/errorUtils'; import { ModuleNotInstalledError } from '../../client/common/errors/moduleNotInstalledError'; import { BufferDecoder } from '../../client/common/process/decoder'; @@ -15,19 +13,15 @@ import { ObservableExecutionResult, SpawnOptions } from '../../client/common/process/types'; -import { IOutputChannel } from '../../client/common/types'; import { Architecture } from '../../client/common/utils/platform'; -import { IServiceContainer } from '../../client/ioc/types'; export class MockPythonExecutionService implements IPythonExecutionService { private procService : ProcessService; private pythonPath : string = 'python'; - constructor(serviceContainer: IServiceContainer) { - const output = serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); - const workspaceService = serviceContainer.get(IWorkspaceService); - this.procService = new ProcessService(new BufferDecoder(), output, workspaceService); + constructor() { + this.procService = new ProcessService(new BufferDecoder()); } public getInterpreterInformation(): Promise { return Promise.resolve( diff --git a/src/test/datascience/mockProcessService.ts b/src/test/datascience/mockProcessService.ts index 94f41afe3748..c69a6a2c0c8c 100644 --- a/src/test/datascience/mockProcessService.ts +++ b/src/test/datascience/mockProcessService.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. 'use strict'; import { Observable } from 'rxjs/Observable'; +import { Event, EventEmitter } from 'vscode'; import { Cancellation, CancellationError } from '../../client/common/cancellation'; import { @@ -9,12 +10,14 @@ import { IProcessService, ObservableExecutionResult, Output, + ProcessServiceEvent, ShellOptions, SpawnOptions } from '../../client/common/process/types'; import { noop, sleep } from '../core'; export class MockProcessService implements IProcessService { + private readonly onExec = new EventEmitter(); private execResults: {file: string; args: (string | RegExp)[]; result(): Promise> }[] = []; private execObservableResults: {file: string; args: (string | RegExp)[]; result(): ObservableExecutionResult }[] = []; private timeDelay: number | undefined; @@ -28,6 +31,10 @@ export class MockProcessService implements IProcessService { return this.defaultObservable([file, ...args]); } + public get processExecutedEvent(): Event { + return this.onExec.event; + } + public async exec(file: string, args: string[], options: SpawnOptions): Promise> { const match = this.execResults.find(f => this.argsMatch(f.args, args) && f.file === file); if (match) { diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index 659d0b9a48c3..b8da5f609703 100644 --- a/src/test/format/extension.format.test.ts +++ b/src/test/format/extension.format.test.ts @@ -18,8 +18,8 @@ import { CondaService } from '../../client/interpreter/locators/services/condaSe import { isPythonVersionInProcess } from '../common'; import { closeActiveWindows, initialize, initializeTest } from '../initialize'; import { MockProcessService } from '../mocks/proc'; -import { UnitTestIocContainer } from '../testing/serviceRegistry'; import { compareFiles } from '../textUtils'; +import { UnitTestIocContainer } from '../testing/serviceRegistry'; const ch = window.createOutputChannel('Tests'); const formatFilesPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting'); diff --git a/src/test/languageServers/jedi/autocomplete/base.test.ts b/src/test/languageServers/jedi/autocomplete/base.test.ts index 58d046a8516a..3b9a60e6159b 100644 --- a/src/test/languageServers/jedi/autocomplete/base.test.ts +++ b/src/test/languageServers/jedi/autocomplete/base.test.ts @@ -21,7 +21,6 @@ const fileDecorator = path.join(autoCompPath, 'deco.py'); const fileEncoding = path.join(autoCompPath, 'four.py'); const fileEncodingUsed = path.join(autoCompPath, 'five.py'); const fileSuppress = path.join(autoCompPath, 'suppress.py'); -const output = vscode.window.createOutputChannel('Tests'); // tslint:disable-next-line:max-func-body-length suite('Autocomplete Base Tests', function () { @@ -38,10 +37,7 @@ suite('Autocomplete Base Tests', function () { initializeDI(); }); setup(initializeTest); - suiteTeardown(async () => { - output.dispose(); - await closeActiveWindows(); - }); + suiteTeardown(closeActiveWindows); teardown(async () => { await closeActiveWindows(); await ioc.dispose(); @@ -100,7 +96,7 @@ suite('Autocomplete Base Tests', function () { // https://github.com/DonJayamanne/pythonVSCode/issues/265 test('For "lambda"', async function () { - if (await isPythonVersion(output, '2')) { + if (await isPythonVersion('2')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } diff --git a/src/test/languageServers/jedi/autocomplete/pep526.test.ts b/src/test/languageServers/jedi/autocomplete/pep526.test.ts index 80e68aeb0f9d..0ce3cc50cb7f 100644 --- a/src/test/languageServers/jedi/autocomplete/pep526.test.ts +++ b/src/test/languageServers/jedi/autocomplete/pep526.test.ts @@ -16,14 +16,13 @@ import { UnitTestIocContainer } from '../../../testing/serviceRegistry'; const autoCompPath = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'pythonFiles', 'autocomp'); const filePep526 = path.join(autoCompPath, 'pep526.py'); -const output = vscode.window.createOutputChannel('Tests'); // tslint:disable-next-line:max-func-body-length suite('Autocomplete PEP 526', () => { let ioc: UnitTestIocContainer; suiteSetup(async function () { // Pep526 only valid for 3.6+ (#2545) - if (await isPythonVersion(output, '2', '3.4', '3.5')) { + if (await isPythonVersion('2', '3.4', '3.5')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -32,10 +31,7 @@ suite('Autocomplete PEP 526', () => { initializeDI(); }); setup(initializeTest); - suiteTeardown(async () => { - output.dispose(); - await closeActiveWindows(); - }); + suiteTeardown(closeActiveWindows); teardown(async () => { await closeActiveWindows(); await ioc.dispose(); diff --git a/src/test/mocks/proc.ts b/src/test/mocks/proc.ts index 857a9d6c369c..135e3008e296 100644 --- a/src/test/mocks/proc.ts +++ b/src/test/mocks/proc.ts @@ -2,12 +2,14 @@ import 'rxjs/add/observable/of'; import { EventEmitter } from 'events'; import { Observable } from 'rxjs/Observable'; +import { Event, EventEmitter as VSCodeEventEmitter } from 'vscode'; import { ExecutionResult, IProcessService, ObservableExecutionResult, Output, + ProcessServiceEvent, ShellOptions, SpawnOptions } from '../../client/common/process/types'; @@ -19,6 +21,7 @@ type ExecCallback = (result: ExecutionResult) => void; export const IOriginalProcessService = Symbol('IProcessService'); export class MockProcessService extends EventEmitter implements IProcessService { + private readonly onProcessExecuted = new VSCodeEventEmitter(); constructor(private procService: IProcessService) { super(); } @@ -51,6 +54,9 @@ export class MockProcessService extends EventEmitter implements IProcessService return this.procService.execObservable(file, args, options); } } + public get processExecutedEvent(): Event { + return this.onProcessExecuted.event; + } public onExec(handler: (file: string, args: string[], options: SpawnOptions, callback: ExecCallback) => void) { this.on('exec', handler); } diff --git a/src/test/refactor/extension.refactor.extract.var.test.ts b/src/test/refactor/extension.refactor.extract.var.test.ts index 4dff81e70864..7652d5398326 100644 --- a/src/test/refactor/extension.refactor.extract.var.test.ts +++ b/src/test/refactor/extension.refactor.extract.var.test.ts @@ -15,7 +15,6 @@ import { MockOutputChannel } from './../mockClasses'; const EXTENSION_DIR = path.join(__dirname, '..', '..', '..'); const refactorSourceFile = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'refactoring', 'standAlone', 'refactor.py'); const refactorTargetFileDir = path.join(__dirname, '..', '..', '..', 'out', 'test', 'pythonFiles', 'refactoring', 'standAlone'); -const output = window.createOutputChannel('Tests'); interface RenameResponse { results: [{ diff: string }]; @@ -30,7 +29,6 @@ suite('Variable Extraction', () => { suiteSetup(initialize); suiteTeardown(() => { commands.executeCommand = oldExecuteCommand; - output.dispose(); return closeActiveWindows(); }); setup(async () => { @@ -84,7 +82,7 @@ suite('Variable Extraction', () => { // tslint:disable-next-line:no-function-expression test('Extract Variable', async function () { - if (isPythonVersion(output, '3.7')) { + if (isPythonVersion('3.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } else { diff --git a/src/test/refactor/rename.test.ts b/src/test/refactor/rename.test.ts index a3691ccf7b0d..6459701abc8e 100644 --- a/src/test/refactor/rename.test.ts +++ b/src/test/refactor/rename.test.ts @@ -8,14 +8,13 @@ import { EOL } from 'os'; import * as path from 'path'; import * as typeMoq from 'typemoq'; import { Range, TextEditorCursorStyle, TextEditorLineNumbersStyle, TextEditorOptions, window, workspace } from 'vscode'; -import { IWorkspaceService } from '../../client/common/application/types'; import { EXTENSION_ROOT_DIR } from '../../client/common/constants'; import '../../client/common/extensions'; import { BufferDecoder } from '../../client/common/process/decoder'; import { ProcessService } from '../../client/common/process/proc'; import { PythonExecutionFactory } from '../../client/common/process/pythonExecutionFactory'; import { IProcessServiceFactory, IPythonExecutionFactory } from '../../client/common/process/types'; -import { IConfigurationService, IOutputChannel, IPythonSettings } from '../../client/common/types'; +import { IConfigurationService, IPythonSettings } from '../../client/common/types'; import { IEnvironmentActivationService } from '../../client/interpreter/activation/types'; import { IServiceContainer } from '../../client/ioc/types'; import { RefactorProxy } from '../../client/refactor/proxy'; @@ -32,19 +31,14 @@ suite('Refactor Rename', () => { const options: TextEditorOptions = { cursorStyle: TextEditorCursorStyle.Line, insertSpaces: true, lineNumbers: TextEditorLineNumbersStyle.Off, tabSize: 4 }; let pythonSettings: typeMoq.IMock; let serviceContainer: typeMoq.IMock; - let outputChannel: typeMoq.IMock; - let workspaceService: typeMoq.IMock; - suiteSetup(initialize); setup(async () => { - outputChannel = typeMoq.Mock.ofType(); - workspaceService = typeMoq.Mock.ofType(); pythonSettings = typeMoq.Mock.ofType(); pythonSettings.setup(p => p.pythonPath).returns(() => PYTHON_PATH); const configService = typeMoq.Mock.ofType(); configService.setup(c => c.getSettings(typeMoq.It.isAny())).returns(() => pythonSettings.object); const processServiceFactory = typeMoq.Mock.ofType(); - processServiceFactory.setup(p => p.create(typeMoq.It.isAny())).returns(() => Promise.resolve(new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object))); + processServiceFactory.setup(p => p.create(typeMoq.It.isAny())).returns(() => Promise.resolve(new ProcessService(new BufferDecoder()))); const envActivationService = typeMoq.Mock.ofType(); envActivationService.setup(e => e.getActivatedEnvironmentVariables(typeMoq.It.isAny())).returns(() => Promise.resolve(undefined)); serviceContainer = typeMoq.Mock.ofType(); diff --git a/src/test/terminals/codeExecution/helper.test.ts b/src/test/terminals/codeExecution/helper.test.ts index fb1f68fc7456..be8185f2b238 100644 --- a/src/test/terminals/codeExecution/helper.test.ts +++ b/src/test/terminals/codeExecution/helper.test.ts @@ -9,13 +9,13 @@ import { EOL } from 'os'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; import { Range, Selection, TextDocument, TextEditor, TextLine, Uri } from 'vscode'; -import { IApplicationShell, IDocumentManager, IWorkspaceService } from '../../../client/common/application/types'; +import { IApplicationShell, IDocumentManager } from '../../../client/common/application/types'; import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../../client/common/constants'; import '../../../client/common/extensions'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; -import { IConfigurationService, IOutputChannel, IPythonSettings } from '../../../client/common/types'; +import { IConfigurationService, IPythonSettings } from '../../../client/common/types'; import { OSType } from '../../../client/common/utils/platform'; import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -29,9 +29,6 @@ const TEST_FILES_PATH = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'pythonFile suite('Terminal - Code Execution Helper', () => { let documentManager: TypeMoq.IMock; let applicationShell: TypeMoq.IMock; - let outputChannel: TypeMoq.IMock; - let workspaceService: TypeMoq.IMock; - let helper: ICodeExecutionHelper; let document: TypeMoq.IMock; let editor: TypeMoq.IMock; @@ -41,9 +38,6 @@ suite('Terminal - Code Execution Helper', () => { const serviceContainer = TypeMoq.Mock.ofType(); documentManager = TypeMoq.Mock.ofType(); applicationShell = TypeMoq.Mock.ofType(); - outputChannel = TypeMoq.Mock.ofType(); - workspaceService = TypeMoq.Mock.ofType(); - const envVariablesProvider = TypeMoq.Mock.ofType(); processService = TypeMoq.Mock.ofType(); configService = TypeMoq.Mock.ofType(); @@ -60,8 +54,6 @@ suite('Terminal - Code Execution Helper', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => applicationShell.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider), TypeMoq.It.isAny())).returns(() => envVariablesProvider.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IConfigurationService), TypeMoq.It.isAny())).returns(() => configService.object); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProcessServiceFactory), TypeMoq.It.isAny())).returns(() => processServiceFactory.object); - helper = new CodeExecutionHelper(serviceContainer.object); document = TypeMoq.Mock.ofType(); @@ -70,7 +62,7 @@ suite('Terminal - Code Execution Helper', () => { }); async function ensureBlankLinesAreRemoved(source: string, expectedSource: string) { - const actualProcessService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const actualProcessService = new ProcessService(new BufferDecoder()); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns((file, args, options) => { return actualProcessService.exec.apply(actualProcessService, [file, args, options]); @@ -83,7 +75,7 @@ suite('Terminal - Code Execution Helper', () => { test('Ensure blank lines are NOT removed when code is not indented (simple)', async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -94,7 +86,7 @@ suite('Terminal - Code Execution Helper', () => { }); test('Ensure there are no multiple-CR elements in the normalized code.', async () => { const code = ['import sys', '', '', '', 'print(sys.executable)', '', 'print("1234")', '', '', 'print(1)', 'print(2)']; - const actualProcessService = new ProcessService(new BufferDecoder(), outputChannel.object, workspaceService.object); + const actualProcessService = new ProcessService(new BufferDecoder()); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns((file, args, options) => { return actualProcessService.exec.apply(actualProcessService, [file, args, options]); @@ -107,7 +99,7 @@ suite('Terminal - Code Execution Helper', () => { test(`Ensure blank lines are removed (Sample${fileNameSuffix})`, async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -119,7 +111,7 @@ suite('Terminal - Code Execution Helper', () => { test(`Ensure last two blank lines are preserved (Sample${fileNameSuffix})`, async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -131,7 +123,7 @@ suite('Terminal - Code Execution Helper', () => { test(`Ensure last two blank lines are preserved even if we have more than 2 trailing blank lines (Sample${fileNameSuffix})`, async function () { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion(outputChannel.object, '2.7')) { + if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { // tslint:disable-next-line:no-invalid-this return this.skip(); } From 5d7b3ebd9efa14c375299a3aae5f1731eae9b686 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 10:58:08 -0700 Subject: [PATCH 04/21] Fix existing unit tests --- .../process/pythonExecutionFactory.unit.test.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/test/common/process/pythonExecutionFactory.unit.test.ts b/src/test/common/process/pythonExecutionFactory.unit.test.ts index 449d67257aa6..03870c7c02c2 100644 --- a/src/test/common/process/pythonExecutionFactory.unit.test.ts +++ b/src/test/common/process/pythonExecutionFactory.unit.test.ts @@ -5,11 +5,12 @@ import * as assert from 'assert'; import { expect } from 'chai'; import { SemVer } from 'semver'; import { anything, instance, mock, verify, when } from 'ts-mockito'; -import { Uri } from 'vscode'; +import { Disposable, Uri } from 'vscode'; import { PythonSettings } from '../../../client/common/configSettings'; import { ConfigurationService } from '../../../client/common/configuration/service'; import { BufferDecoder } from '../../../client/common/process/decoder'; +import { ProcessLogger } from '../../../client/common/process/logger'; import { ProcessService } from '../../../client/common/process/proc'; import { ProcessServiceFactory } from '../../../client/common/process/processFactory'; import { PythonExecutionFactory } from '../../../client/common/process/pythonExecutionFactory'; @@ -17,6 +18,7 @@ import { PythonExecutionService } from '../../../client/common/process/pythonPro import { ExecutionFactoryCreationOptions, IBufferDecoder, + IProcessLogger, IProcessServiceFactory, IPythonExecutionService } from '../../../client/common/process/types'; @@ -67,14 +69,21 @@ suite('Process - PythonExecutionFactory', () => { let bufferDecoder: IBufferDecoder; let procecssFactory: IProcessServiceFactory; let configService: IConfigurationService; - + let processLogger: IProcessLogger; + let processService: ProcessService; setup(() => { bufferDecoder = mock(BufferDecoder); activationHelper = mock(EnvironmentActivationService); procecssFactory = mock(ProcessServiceFactory); configService = mock(ConfigurationService); + processLogger = mock(ProcessLogger); + when(processLogger.logProcess({file: '', args: [], options: {}})).thenReturn(); + processService = mock(ProcessService); + const disposable = mock(Disposable); + when(processService.processExecutedEvent).thenReturn(() => disposable); const serviceContainer = mock(ServiceContainer); when(serviceContainer.get(IDisposableRegistry)).thenReturn([]); + when(serviceContainer.get(IProcessLogger)).thenReturn(processLogger); factory = new PythonExecutionFactory(instance(serviceContainer), instance(activationHelper), instance(procecssFactory), instance(configService), instance(bufferDecoder)); @@ -82,7 +91,7 @@ suite('Process - PythonExecutionFactory', () => { test('Ensure PythonExecutionService is created', async () => { const pythonSettings = mock(PythonSettings); - when(procecssFactory.create(resource)).thenResolve(instance(mock(ProcessService))); + when(procecssFactory.create(resource)).thenResolve(instance(processService)); when(activationHelper.getActivatedEnvironmentVariables(resource)).thenResolve({ x: '1' }); when(pythonSettings.pythonPath).thenReturn('HELLO'); when(configService.getSettings(resource)).thenReturn(instance(pythonSettings)); From 860803f5760e241ab342996a47f3759d5afb36cc Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 11:41:50 -0700 Subject: [PATCH 05/21] Simplify logger --- src/client/common/process/logger.ts | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index c5f4c4465f94..323a461f28b7 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -1,36 +1,24 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { window } from 'vscode'; import { IServiceContainer } from '../../ioc/types'; -import { IWorkspaceService } from '../application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../constants'; import { traceInfo } from '../logger'; -import { IOutputChannel } from '../types'; +import { IOutputChannel, IPathUtils } from '../types'; import { Logging } from '../utils/localize'; import { IProcessLogger, ProcessServiceEvent } from './types'; @injectable() export class ProcessLogger implements IProcessLogger { private outputChannel: IOutputChannel; - private workspaceService: IWorkspaceService; - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { this.outputChannel = serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); - this.workspaceService = serviceContainer.get(IWorkspaceService); } public logProcess({ file, args, options }: ProcessServiceEvent) { const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); - let currentWorkingDirectory; - - if (this.workspaceService.hasWorkspaceFolders) { - currentWorkingDirectory = this.workspaceService.workspaceFolders![0].uri.fsPath; - } else { - const openFile = window.activeTextEditor !== undefined ? window.activeTextEditor.document.uri : undefined; - currentWorkingDirectory = openFile ? path.dirname(openFile.fsPath) : options.cwd!; - } + const currentWorkingDirectory = this.pathUtils.getDisplayName(options.cwd!); const info = [ `> ${file} ${formattedArgs}`, `${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}` From 56e5b93744010d537b86e20561905c65e3084710 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 11:41:59 -0700 Subject: [PATCH 06/21] Add logger unit tests --- src/test/common/process/logger.unit.test.ts | 61 +++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 src/test/common/process/logger.unit.test.ts diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts new file mode 100644 index 000000000000..ec8baa19c658 --- /dev/null +++ b/src/test/common/process/logger.unit.test.ts @@ -0,0 +1,61 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { expect } from 'chai'; +import * as path from 'path'; +import * as TypeMoq from 'typemoq'; +// tslint:disable-next-line:no-require-imports +import untildify = require('untildify'); + +import { STANDARD_OUTPUT_CHANNEL } from '../../../client/common/constants'; +import { PathUtils } from '../../../client/common/platform/pathUtils'; +import { ProcessLogger } from '../../../client/common/process/logger'; +import { IOutputChannel } from '../../../client/common/types'; +import { Logging } from '../../../client/common/utils/localize'; +import { IServiceContainer } from '../../../client/ioc/types'; +import { getOSType, OSType } from '../../common'; + +suite('ProcessLogger suite', () => { + let outputChannel: TypeMoq.IMock; + let serviceContainer: TypeMoq.IMock; + let pathUtils: PathUtils; + let outputResult: string; + + suiteSetup(() => { + outputChannel = TypeMoq.Mock.ofType(); + serviceContainer = TypeMoq.Mock.ofType(); + pathUtils = new PathUtils(getOSType() === OSType.Windows); + }); + + setup(() => { + outputChannel.setup(o => o.appendLine(TypeMoq.It.isAnyString())).returns((s: string) => outputResult = s); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IOutputChannel), STANDARD_OUTPUT_CHANNEL)).returns(() => outputChannel.object); + }); + + teardown(() => { + outputChannel.reset(); + serviceContainer.reset(); + }); + + test('Logger displays the process command, arguments and current working directory in the output channel', async () => { + const options = { cwd: path.join('debug', 'path')}; + const logger = new ProcessLogger(serviceContainer.object, pathUtils); + logger.logProcess({ file: 'test', args: ['--foo', '--bar'], options }); + + const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect - String built incorrectly'); + + outputChannel.verify(o => o.appendLine(TypeMoq.It.isAnyString()), TypeMoq.Times.once()); + }); + + test('Logger replaces the path/to/home with ~', async () => { + const options = { cwd: path.join(untildify('~'), 'debug', 'path') }; + const logger = new ProcessLogger(serviceContainer.object, pathUtils); + logger.logProcess({ file: 'test', args: ['--foo', '--bar'], options}); + + const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${path.join('~', 'debug', 'path')}`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); + }); +}); From 91e62ceedc52d8ee7c94754cb2b278a80e36dc53 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 11:52:55 -0700 Subject: [PATCH 07/21] PR comment fixes --- src/client/common/process/logger.ts | 11 ++++------- src/client/common/process/proc.ts | 6 +++--- src/client/common/process/types.ts | 6 +++--- src/test/datascience/mockProcessService.ts | 6 +++--- src/test/mocks/proc.ts | 6 +++--- 5 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index 323a461f28b7..13c88558ab41 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -1,22 +1,19 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { inject, injectable } from 'inversify'; -import { IServiceContainer } from '../../ioc/types'; +import { inject, injectable, named } from 'inversify'; import { STANDARD_OUTPUT_CHANNEL } from '../constants'; import { traceInfo } from '../logger'; import { IOutputChannel, IPathUtils } from '../types'; import { Logging } from '../utils/localize'; -import { IProcessLogger, ProcessServiceEvent } from './types'; +import { IProcessLogger, ProcessServiceEventArgs } from './types'; @injectable() export class ProcessLogger implements IProcessLogger { - private outputChannel: IOutputChannel; - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { - this.outputChannel = serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { } - public logProcess({ file, args, options }: ProcessServiceEvent) { + public logProcess({ file, args, options }: ProcessServiceEventArgs) { const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); const currentWorkingDirectory = this.pathUtils.getDisplayName(options.cwd!); const info = [ diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index e47a2f6f258e..851b845defab 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -14,7 +14,7 @@ import { IProcessService, ObservableExecutionResult, Output, - ProcessServiceEvent, + ProcessServiceEventArgs, ShellOptions, SpawnOptions, StdErrError @@ -23,7 +23,7 @@ import { // tslint:disable:no-any export class ProcessService implements IProcessService, IDisposable { private processesToKill = new Set(); - private readonly onProcessExecuted = new EventEmitter(); + private readonly onProcessExecuted = new EventEmitter(); constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { } public static isAlive(pid: number): boolean { try { @@ -56,7 +56,7 @@ export class ProcessService implements IProcessService, IDisposable { }); } - public get processExecutedEvent(): Event { + public get processExecutedEvent(): Event { return this.onProcessExecuted.event; } diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index 467b3f74943f..6a5cd5aa095a 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -40,7 +40,7 @@ export type ExecutionResult = { stderr?: T; }; -export type ProcessServiceEvent = { +export type ProcessServiceEventArgs = { file: string; args: string[]; options: SpawnOptions; @@ -48,11 +48,11 @@ export type ProcessServiceEvent = { export const IProcessLogger = Symbol('IProcessLogger'); export interface IProcessLogger { - logProcess(event: ProcessServiceEvent): void; + logProcess(event: ProcessServiceEventArgs): void; } export interface IProcessService { - readonly processExecutedEvent: Event; + readonly processExecutedEvent: Event; execObservable(file: string, args: string[], options?: SpawnOptions): ObservableExecutionResult; exec(file: string, args: string[], options?: SpawnOptions): Promise>; shellExec(command: string, options?: ShellOptions): Promise>; diff --git a/src/test/datascience/mockProcessService.ts b/src/test/datascience/mockProcessService.ts index c69a6a2c0c8c..6b91d486c258 100644 --- a/src/test/datascience/mockProcessService.ts +++ b/src/test/datascience/mockProcessService.ts @@ -10,14 +10,14 @@ import { IProcessService, ObservableExecutionResult, Output, - ProcessServiceEvent, + ProcessServiceEventArgs, ShellOptions, SpawnOptions } from '../../client/common/process/types'; import { noop, sleep } from '../core'; export class MockProcessService implements IProcessService { - private readonly onExec = new EventEmitter(); + private readonly onExec = new EventEmitter(); private execResults: {file: string; args: (string | RegExp)[]; result(): Promise> }[] = []; private execObservableResults: {file: string; args: (string | RegExp)[]; result(): ObservableExecutionResult }[] = []; private timeDelay: number | undefined; @@ -31,7 +31,7 @@ export class MockProcessService implements IProcessService { return this.defaultObservable([file, ...args]); } - public get processExecutedEvent(): Event { + public get processExecutedEvent(): Event { return this.onExec.event; } diff --git a/src/test/mocks/proc.ts b/src/test/mocks/proc.ts index 135e3008e296..37327e9b7db8 100644 --- a/src/test/mocks/proc.ts +++ b/src/test/mocks/proc.ts @@ -9,7 +9,7 @@ import { IProcessService, ObservableExecutionResult, Output, - ProcessServiceEvent, + ProcessServiceEventArgs, ShellOptions, SpawnOptions } from '../../client/common/process/types'; @@ -21,7 +21,7 @@ type ExecCallback = (result: ExecutionResult) => void; export const IOriginalProcessService = Symbol('IProcessService'); export class MockProcessService extends EventEmitter implements IProcessService { - private readonly onProcessExecuted = new VSCodeEventEmitter(); + private readonly onProcessExecuted = new VSCodeEventEmitter(); constructor(private procService: IProcessService) { super(); } @@ -54,7 +54,7 @@ export class MockProcessService extends EventEmitter implements IProcessService return this.procService.execObservable(file, args, options); } } - public get processExecutedEvent(): Event { + public get processExecutedEvent(): Event { return this.onProcessExecuted.event; } public onExec(handler: (file: string, args: string[], options: SpawnOptions, callback: ExecCallback) => void) { From cb50ff85a1e5b8cca683323ae59166cffea04cc9 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 11:59:06 -0700 Subject: [PATCH 08/21] More PR changes, prettify command, add unit tests --- src/client/common/process/logger.ts | 2 +- src/test/common/process/logger.unit.test.ts | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index 13c88558ab41..14386a19e0f4 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -17,7 +17,7 @@ export class ProcessLogger implements IProcessLogger { const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); const currentWorkingDirectory = this.pathUtils.getDisplayName(options.cwd!); const info = [ - `> ${file} ${formattedArgs}`, + `> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`, `${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}` ].join('\n'); diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts index ec8baa19c658..ee601ed0a443 100644 --- a/src/test/common/process/logger.unit.test.ts +++ b/src/test/common/process/logger.unit.test.ts @@ -9,39 +9,33 @@ import * as TypeMoq from 'typemoq'; // tslint:disable-next-line:no-require-imports import untildify = require('untildify'); -import { STANDARD_OUTPUT_CHANNEL } from '../../../client/common/constants'; import { PathUtils } from '../../../client/common/platform/pathUtils'; import { ProcessLogger } from '../../../client/common/process/logger'; import { IOutputChannel } from '../../../client/common/types'; import { Logging } from '../../../client/common/utils/localize'; -import { IServiceContainer } from '../../../client/ioc/types'; import { getOSType, OSType } from '../../common'; suite('ProcessLogger suite', () => { let outputChannel: TypeMoq.IMock; - let serviceContainer: TypeMoq.IMock; let pathUtils: PathUtils; let outputResult: string; suiteSetup(() => { outputChannel = TypeMoq.Mock.ofType(); - serviceContainer = TypeMoq.Mock.ofType(); pathUtils = new PathUtils(getOSType() === OSType.Windows); }); setup(() => { outputChannel.setup(o => o.appendLine(TypeMoq.It.isAnyString())).returns((s: string) => outputResult = s); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IOutputChannel), STANDARD_OUTPUT_CHANNEL)).returns(() => outputChannel.object); }); teardown(() => { outputChannel.reset(); - serviceContainer.reset(); }); test('Logger displays the process command, arguments and current working directory in the output channel', async () => { const options = { cwd: path.join('debug', 'path')}; - const logger = new ProcessLogger(serviceContainer.object, pathUtils); + const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess({ file: 'test', args: ['--foo', '--bar'], options }); const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; @@ -50,12 +44,21 @@ suite('ProcessLogger suite', () => { outputChannel.verify(o => o.appendLine(TypeMoq.It.isAnyString()), TypeMoq.Times.once()); }); - test('Logger replaces the path/to/home with ~', async () => { + test('Logger replaces the path/to/home with ~ in the current working directory', async () => { const options = { cwd: path.join(untildify('~'), 'debug', 'path') }; - const logger = new ProcessLogger(serviceContainer.object, pathUtils); + const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess({ file: 'test', args: ['--foo', '--bar'], options}); const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${path.join('~', 'debug', 'path')}`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); }); + + test('Logger replaces the path/to/home with ~ in the command path', async () => { + const options = { cwd: path.join('debug', 'path') }; + const logger = new ProcessLogger(outputChannel.object, pathUtils); + logger.logProcess({ file: path.join(untildify('~'), 'test'), args: ['--foo', '--bar'], options}); + + const expectedResult = `> ${path.join('~', 'test')} --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); + }); }); From d57e83cc081dbe316095b8f1048971682e783be6 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 12:35:38 -0700 Subject: [PATCH 09/21] More test fixes --- src/test/common/installer.test.ts | 4 +++- src/test/common/moduleInstaller.test.ts | 4 +++- src/test/refactor/rename.test.ts | 5 ++++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index 8eb649d073c3..e47793d8dea5 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -14,7 +14,8 @@ import { Logger } from '../../client/common/logger'; import { PersistentStateFactory } from '../../client/common/persistentState'; import { PathUtils } from '../../client/common/platform/pathUtils'; import { CurrentProcess } from '../../client/common/process/currentProcess'; -import { IProcessServiceFactory } from '../../client/common/process/types'; +import { ProcessLogger } from '../../client/common/process/logger'; +import { IProcessLogger, IProcessServiceFactory } from '../../client/common/process/types'; import { TerminalHelper } from '../../client/common/terminal/helper'; import { ITerminalHelper } from '../../client/common/terminal/types'; import { IConfigurationService, ICurrentProcess, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, IsWindows, ModuleNamePurpose, Product, ProductType } from '../../client/common/types'; @@ -58,6 +59,7 @@ suite('Installer', () => { ioc.serviceManager.addSingleton(ILogger, Logger); ioc.serviceManager.addSingleton(IInstaller, ProductInstaller); ioc.serviceManager.addSingleton(IPathUtils, PathUtils); + ioc.serviceManager.addSingleton(IProcessLogger, ProcessLogger); ioc.serviceManager.addSingleton(ICurrentProcess, CurrentProcess); ioc.serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); ioc.serviceManager.addSingletonInstance(ICommandManager, TypeMoq.Mock.ofType().object); diff --git a/src/test/common/moduleInstaller.test.ts b/src/test/common/moduleInstaller.test.ts index c8e884fe2d0b..250b895264ef 100644 --- a/src/test/common/moduleInstaller.test.ts +++ b/src/test/common/moduleInstaller.test.ts @@ -21,7 +21,8 @@ import { PathUtils } from '../../client/common/platform/pathUtils'; import { PlatformService } from '../../client/common/platform/platformService'; import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; import { CurrentProcess } from '../../client/common/process/currentProcess'; -import { IProcessServiceFactory, IPythonExecutionFactory } from '../../client/common/process/types'; +import { ProcessLogger } from '../../client/common/process/logger'; +import { IProcessLogger, IProcessServiceFactory, IPythonExecutionFactory } from '../../client/common/process/types'; import { TerminalHelper } from '../../client/common/terminal/helper'; import { ITerminalHelper, ITerminalService, ITerminalServiceFactory } from '../../client/common/terminal/types'; import { IConfigurationService, ICurrentProcess, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, IPythonSettings, IsWindows } from '../../client/common/types'; @@ -82,6 +83,7 @@ suite('Module Installer', () => { ioc.serviceManager.addSingleton(IPersistentStateFactory, PersistentStateFactory); ioc.serviceManager.addSingleton(ILogger, Logger); + ioc.serviceManager.addSingleton(IProcessLogger, ProcessLogger); ioc.serviceManager.addSingleton(IInstaller, ProductInstaller); mockTerminalService = TypeMoq.Mock.ofType(); diff --git a/src/test/refactor/rename.test.ts b/src/test/refactor/rename.test.ts index 6459701abc8e..58f8e30f9b53 100644 --- a/src/test/refactor/rename.test.ts +++ b/src/test/refactor/rename.test.ts @@ -13,7 +13,7 @@ import '../../client/common/extensions'; import { BufferDecoder } from '../../client/common/process/decoder'; import { ProcessService } from '../../client/common/process/proc'; import { PythonExecutionFactory } from '../../client/common/process/pythonExecutionFactory'; -import { IProcessServiceFactory, IPythonExecutionFactory } from '../../client/common/process/types'; +import { IProcessLogger, IProcessServiceFactory, IPythonExecutionFactory } from '../../client/common/process/types'; import { IConfigurationService, IPythonSettings } from '../../client/common/types'; import { IEnvironmentActivationService } from '../../client/interpreter/activation/types'; import { IServiceContainer } from '../../client/ioc/types'; @@ -51,6 +51,9 @@ suite('Refactor Rename', () => { .returns(() => new PythonExecutionFactory(serviceContainer.object, undefined as any, processServiceFactory.object, configService.object, undefined as any)); + const processLogger = typeMoq.Mock.ofType(); + processLogger.setup(p => p.logProcess(typeMoq.It.isAny())).returns(() => { return; }); + serviceContainer.setup(s => s.get(typeMoq.It.isValue(IProcessLogger), typeMoq.It.isAny())).returns(() => processLogger.object); await initializeTest(); }); teardown(closeActiveWindows); From 1915d9d0569d8edf08c29a48028ffc722d8c0b6a Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 13:14:02 -0700 Subject: [PATCH 10/21] Test fixes, again --- package.nls.json | 1 + src/test/linters/lint.functional.test.ts | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/package.nls.json b/package.nls.json index 2bd9ebdb872c..85bf4eeb500e 100644 --- a/package.nls.json +++ b/package.nls.json @@ -105,6 +105,7 @@ "Experiments.inGroup": "User belongs to experiment group '{0}'", "Interpreters.RefreshingInterpreters": "Refreshing Python Interpreters", "Interpreters.LoadingInterpreters": "Loading Python Interpreters", + "Logging.CurrentWorkingDirectory": "Current working directory:", "Common.doNotShowAgain": "Do not show again", "Interpreters.environmentPromptMessage": "We noticed a new virtual environment has been created. Do you want to select it for the workspace folder?", "DataScience.restartKernelMessage": "Do you want to restart the IPython kernel? All variables will be lost.", diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts index cdfb83cf3006..8f501e65114b 100644 --- a/src/test/linters/lint.functional.test.ts +++ b/src/test/linters/lint.functional.test.ts @@ -22,6 +22,7 @@ import { PythonExecutionFactory } from '../../client/common/process/pythonExecut import { PythonToolExecutionService } from '../../client/common/process/pythonToolService'; import { IBufferDecoder, + IProcessLogger, IPythonExecutionFactory, IPythonToolExecutionService } from '../../client/common/process/types'; @@ -189,6 +190,9 @@ class TestFixture extends BaseTestFixture { ) { const serviceContainer = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); const configService = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); + const processLogger = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); + processLogger.setup(p => p.logProcess(TypeMoq.It.isAny())).returns(() => { return; }); + serviceContainer.setup(s => s.get(TypeMoq.It.isValue(IProcessLogger), TypeMoq.It.isAny())).returns(() => processLogger.object); const platformService = new PlatformService(); const filesystem = new FileSystem(platformService); From 95c8771607748db0459733e4ae561e7e183b6982 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 15:37:58 -0700 Subject: [PATCH 11/21] Use nodejs' event emitter instead of vscode's --- src/client/common/process/logger.ts | 18 +++++++-------- src/client/common/process/proc.ts | 20 +++++++--------- src/client/common/process/processFactory.ts | 4 ++-- .../common/process/pythonExecutionFactory.ts | 9 ++++---- src/client/common/process/types.ts | 20 ++++++++-------- src/test/common/process/logger.unit.test.ts | 23 ++++++++++++++++--- .../pythonExecutionFactory.unit.test.ts | 7 +++--- src/test/datascience/mockProcessService.ts | 15 ++++++------ src/test/linters/lint.functional.test.ts | 2 +- src/test/mocks/proc.ts | 10 ++++---- src/test/refactor/rename.test.ts | 2 +- 11 files changed, 71 insertions(+), 59 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index 14386a19e0f4..eeb9eaff01db 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -6,20 +6,20 @@ import { STANDARD_OUTPUT_CHANNEL } from '../constants'; import { traceInfo } from '../logger'; import { IOutputChannel, IPathUtils } from '../types'; import { Logging } from '../utils/localize'; -import { IProcessLogger, ProcessServiceEventArgs } from './types'; +import { IProcessLogger, SpawnOptions } from './types'; @injectable() export class ProcessLogger implements IProcessLogger { - constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { - } + constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { } - public logProcess({ file, args, options }: ProcessServiceEventArgs) { + public logProcess(file: string, args: string[], options?: SpawnOptions) { const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); - const currentWorkingDirectory = this.pathUtils.getDisplayName(options.cwd!); - const info = [ - `> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`, - `${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}` - ].join('\n'); + const currentWorkingDirectory = options && options.cwd ? this.pathUtils.getDisplayName(options.cwd!) : undefined; + let info = `> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`; + + if (currentWorkingDirectory) { + info += `\n${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}`; + } traceInfo(info); this.outputChannel.appendLine(info); diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index 851b845defab..36496795972c 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -1,8 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { exec, execSync, spawn } from 'child_process'; +import { EventEmitter } from 'events'; import { Observable } from 'rxjs/Observable'; -import { Event, EventEmitter } from 'vscode'; import { IDisposable } from '../types'; import { createDeferred } from '../utils/async'; @@ -14,17 +14,17 @@ import { IProcessService, ObservableExecutionResult, Output, - ProcessServiceEventArgs, ShellOptions, SpawnOptions, StdErrError } from './types'; // tslint:disable:no-any -export class ProcessService implements IProcessService, IDisposable { +export class ProcessService extends EventEmitter implements IProcessService { private processesToKill = new Set(); - private readonly onProcessExecuted = new EventEmitter(); - constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { } + constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { + super(); + } public static isAlive(pid: number): boolean { try { process.kill(pid, 0); @@ -46,7 +46,7 @@ export class ProcessService implements IProcessService, IDisposable { } } public dispose() { - this.onProcessExecuted.dispose(); + this.removeAllListeners(); this.processesToKill.forEach(p => { try { p.dispose(); @@ -56,10 +56,6 @@ export class ProcessService implements IProcessService, IDisposable { }); } - public get processExecutedEvent(): Event { - return this.onProcessExecuted.event; - } - public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult { const spawnOptions = this.getDefaultOptions(options); const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; @@ -123,7 +119,7 @@ export class ProcessService implements IProcessService, IDisposable { }); }); - this.onProcessExecuted.fire({ file, args, options }); + this.emit('processExecuted', file, args, options); return { proc, @@ -185,7 +181,7 @@ export class ProcessService implements IProcessService, IDisposable { disposables.forEach(d => d.dispose()); }); - this.onProcessExecuted.fire({ file, args, options }); + this.emit('processExecuted', file, args, options); return deferred.promise; } diff --git a/src/client/common/process/processFactory.ts b/src/client/common/process/processFactory.ts index d1826b4e0751..3a3e75b081ba 100644 --- a/src/client/common/process/processFactory.ts +++ b/src/client/common/process/processFactory.ts @@ -23,9 +23,9 @@ export class ProcessServiceFactory implements IProcessServiceFactory { const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource); const decoder = this.serviceContainer.get(IBufferDecoder); const disposableRegistry = this.serviceContainer.get(IDisposableRegistry); - const proc = new ProcessService(decoder, customEnvVars); + const proc: IProcessService = new ProcessService(decoder, customEnvVars); disposableRegistry.push(proc); - proc.processExecutedEvent(this.processLogger.logProcess, this.processLogger); + proc.on('processExecuted', this.processLogger.logProcess.bind(this.processLogger)); return proc; } } diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index 3045af6a9f46..a2e9d835487c 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -14,6 +14,7 @@ import { ExecutionFactoryCreationOptions, IBufferDecoder, IProcessLogger, + IProcessService, IProcessServiceFactory, IPythonExecutionFactory, IPythonExecutionService @@ -29,9 +30,9 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { } public async create(options: ExecutionFactoryCreationOptions): Promise { const pythonPath = options.pythonPath ? options.pythonPath : this.configService.getSettings(options.resource).pythonPath; - const processService = await this.processServiceFactory.create(options.resource); + const processService: IProcessService = await this.processServiceFactory.create(options.resource); const processLogger = this.serviceContainer.get(IProcessLogger); - processService.processExecutedEvent(processLogger.logProcess, processLogger); + processService.on('processExecuted', processLogger.logProcess.bind(processLogger)); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } public async createActivatedEnvironment(options: ExecutionFactoryCreateWithEnvironmentOptions): Promise { @@ -42,9 +43,9 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { return this.create({ resource: options.resource, pythonPath: options.interpreter ? options.interpreter.path : undefined }); } const pythonPath = options.interpreter ? options.interpreter.path : this.configService.getSettings(options.resource).pythonPath; - const processService = new ProcessService(this.decoder, { ...envVars }); + const processService: IProcessService = new ProcessService(this.decoder, { ...envVars }); const processLogger = this.serviceContainer.get(IProcessLogger); - processService.processExecutedEvent(processLogger.logProcess, processLogger); + processService.on('processExecuted', processLogger.logProcess.bind(processLogger)); this.serviceContainer.get(IDisposableRegistry).push(processService); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index 6a5cd5aa095a..9a36b04839a1 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -2,10 +2,10 @@ // Licensed under the MIT License. import { ChildProcess, ExecOptions, SpawnOptions as ChildProcessSpawnOptions } from 'child_process'; import { Observable } from 'rxjs/Observable'; -import { CancellationToken, Event, Uri } from 'vscode'; +import { CancellationToken, Uri } from 'vscode'; import { PythonInterpreter } from '../../interpreter/contracts'; -import { ExecutionInfo, Version } from '../types'; +import { ExecutionInfo, IDisposable, Version } from '../types'; import { Architecture } from '../utils/platform'; import { EnvironmentVariables } from '../variables/types'; @@ -40,22 +40,22 @@ export type ExecutionResult = { stderr?: T; }; -export type ProcessServiceEventArgs = { - file: string; - args: string[]; - options: SpawnOptions; -}; +// export type ProcessServiceEventArgs = { +// file: string; +// args: string[]; +// options: SpawnOptions; +// }; export const IProcessLogger = Symbol('IProcessLogger'); export interface IProcessLogger { - logProcess(event: ProcessServiceEventArgs): void; + logProcess(file: string, ars: string[], options?: SpawnOptions): void; } -export interface IProcessService { - readonly processExecutedEvent: Event; +export interface IProcessService extends IDisposable { execObservable(file: string, args: string[], options?: SpawnOptions): ObservableExecutionResult; exec(file: string, args: string[], options?: SpawnOptions): Promise>; shellExec(command: string, options?: ShellOptions): Promise>; + on(event: 'processExecuted', listener: (file: string, args: string[], options?: SpawnOptions) => void): this; } export const IProcessServiceFactory = Symbol('IProcessServiceFactory'); diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts index ee601ed0a443..8c87a74740ab 100644 --- a/src/test/common/process/logger.unit.test.ts +++ b/src/test/common/process/logger.unit.test.ts @@ -36,7 +36,7 @@ suite('ProcessLogger suite', () => { test('Logger displays the process command, arguments and current working directory in the output channel', async () => { const options = { cwd: path.join('debug', 'path')}; const logger = new ProcessLogger(outputChannel.object, pathUtils); - logger.logProcess({ file: 'test', args: ['--foo', '--bar'], options }); + logger.logProcess('test', ['--foo', '--bar'], options); const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect - String built incorrectly'); @@ -47,7 +47,7 @@ suite('ProcessLogger suite', () => { test('Logger replaces the path/to/home with ~ in the current working directory', async () => { const options = { cwd: path.join(untildify('~'), 'debug', 'path') }; const logger = new ProcessLogger(outputChannel.object, pathUtils); - logger.logProcess({ file: 'test', args: ['--foo', '--bar'], options}); + logger.logProcess('test', ['--foo', '--bar'], options); const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${path.join('~', 'debug', 'path')}`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); @@ -56,9 +56,26 @@ suite('ProcessLogger suite', () => { test('Logger replaces the path/to/home with ~ in the command path', async () => { const options = { cwd: path.join('debug', 'path') }; const logger = new ProcessLogger(outputChannel.object, pathUtils); - logger.logProcess({ file: path.join(untildify('~'), 'test'), args: ['--foo', '--bar'], options}); + logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar'], options); const expectedResult = `> ${path.join('~', 'test')} --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); }); + + test('Logger doesn\'t display the working directory line if there is no options parameter', async () => { + const logger = new ProcessLogger(outputChannel.object, pathUtils); + logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar']); + + const expectedResult = `> ${path.join('~', 'test')} --foo --bar`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Working directory line should not be displayed'); + }); + + test('Logger doesn\'t display the working directory line if there is no cwd key in the options parameter', async () => { + const options = { }; + const logger = new ProcessLogger(outputChannel.object, pathUtils); + logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar'], options); + + const expectedResult = `> ${path.join('~', 'test')} --foo --bar`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Working directory line should not be displayed'); + }); }); diff --git a/src/test/common/process/pythonExecutionFactory.unit.test.ts b/src/test/common/process/pythonExecutionFactory.unit.test.ts index 03870c7c02c2..a72dee6215f4 100644 --- a/src/test/common/process/pythonExecutionFactory.unit.test.ts +++ b/src/test/common/process/pythonExecutionFactory.unit.test.ts @@ -5,7 +5,7 @@ import * as assert from 'assert'; import { expect } from 'chai'; import { SemVer } from 'semver'; import { anything, instance, mock, verify, when } from 'ts-mockito'; -import { Disposable, Uri } from 'vscode'; +import { Uri } from 'vscode'; import { PythonSettings } from '../../../client/common/configSettings'; import { ConfigurationService } from '../../../client/common/configuration/service'; @@ -77,10 +77,9 @@ suite('Process - PythonExecutionFactory', () => { procecssFactory = mock(ProcessServiceFactory); configService = mock(ConfigurationService); processLogger = mock(ProcessLogger); - when(processLogger.logProcess({file: '', args: [], options: {}})).thenReturn(); + when(processLogger.logProcess('', [], {})).thenReturn(); processService = mock(ProcessService); - const disposable = mock(Disposable); - when(processService.processExecutedEvent).thenReturn(() => disposable); + when(processService.on('processExecuted', () => { return; })).thenReturn(processService); const serviceContainer = mock(ServiceContainer); when(serviceContainer.get(IDisposableRegistry)).thenReturn([]); when(serviceContainer.get(IProcessLogger)).thenReturn(processLogger); diff --git a/src/test/datascience/mockProcessService.ts b/src/test/datascience/mockProcessService.ts index 6b91d486c258..f7021095bb70 100644 --- a/src/test/datascience/mockProcessService.ts +++ b/src/test/datascience/mockProcessService.ts @@ -2,7 +2,6 @@ // Licensed under the MIT License. 'use strict'; import { Observable } from 'rxjs/Observable'; -import { Event, EventEmitter } from 'vscode'; import { Cancellation, CancellationError } from '../../client/common/cancellation'; import { @@ -10,14 +9,12 @@ import { IProcessService, ObservableExecutionResult, Output, - ProcessServiceEventArgs, ShellOptions, SpawnOptions } from '../../client/common/process/types'; import { noop, sleep } from '../core'; export class MockProcessService implements IProcessService { - private readonly onExec = new EventEmitter(); private execResults: {file: string; args: (string | RegExp)[]; result(): Promise> }[] = []; private execObservableResults: {file: string; args: (string | RegExp)[]; result(): ObservableExecutionResult }[] = []; private timeDelay: number | undefined; @@ -31,10 +28,6 @@ export class MockProcessService implements IProcessService { return this.defaultObservable([file, ...args]); } - public get processExecutedEvent(): Event { - return this.onExec.event; - } - public async exec(file: string, args: string[], options: SpawnOptions): Promise> { const match = this.execResults.find(f => this.argsMatch(f.args, args) && f.file === file); if (match) { @@ -72,6 +65,14 @@ export class MockProcessService implements IProcessService { this.timeDelay = timeout; } + public on() { + return this; + } + + public dispose() { + return; + } + private argsMatch(matchers: (string | RegExp)[], args: string[]): boolean { if (matchers.length === args.length) { return args.every((s, i) => { diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts index 8f501e65114b..b9ad0985514c 100644 --- a/src/test/linters/lint.functional.test.ts +++ b/src/test/linters/lint.functional.test.ts @@ -191,7 +191,7 @@ class TestFixture extends BaseTestFixture { const serviceContainer = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); const configService = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); const processLogger = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); - processLogger.setup(p => p.logProcess(TypeMoq.It.isAny())).returns(() => { return; }); + processLogger.setup(p => p.logProcess(TypeMoq.It.isAnyString(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => { return; }); serviceContainer.setup(s => s.get(TypeMoq.It.isValue(IProcessLogger), TypeMoq.It.isAny())).returns(() => processLogger.object); const platformService = new PlatformService(); diff --git a/src/test/mocks/proc.ts b/src/test/mocks/proc.ts index 37327e9b7db8..947f42d6265c 100644 --- a/src/test/mocks/proc.ts +++ b/src/test/mocks/proc.ts @@ -2,14 +2,12 @@ import 'rxjs/add/observable/of'; import { EventEmitter } from 'events'; import { Observable } from 'rxjs/Observable'; -import { Event, EventEmitter as VSCodeEventEmitter } from 'vscode'; import { ExecutionResult, IProcessService, ObservableExecutionResult, Output, - ProcessServiceEventArgs, ShellOptions, SpawnOptions } from '../../client/common/process/types'; @@ -21,7 +19,6 @@ type ExecCallback = (result: ExecutionResult) => void; export const IOriginalProcessService = Symbol('IProcessService'); export class MockProcessService extends EventEmitter implements IProcessService { - private readonly onProcessExecuted = new VSCodeEventEmitter(); constructor(private procService: IProcessService) { super(); } @@ -54,9 +51,6 @@ export class MockProcessService extends EventEmitter implements IProcessService return this.procService.execObservable(file, args, options); } } - public get processExecutedEvent(): Event { - return this.onProcessExecuted.event; - } public onExec(handler: (file: string, args: string[], options: SpawnOptions, callback: ExecCallback) => void) { this.on('exec', handler); } @@ -76,4 +70,8 @@ export class MockProcessService extends EventEmitter implements IProcessService return valueReturned ? value! : this.procService.shellExec(command, options); } + public dispose() { + return; + } + } diff --git a/src/test/refactor/rename.test.ts b/src/test/refactor/rename.test.ts index 58f8e30f9b53..8823d8be62fb 100644 --- a/src/test/refactor/rename.test.ts +++ b/src/test/refactor/rename.test.ts @@ -52,7 +52,7 @@ suite('Refactor Rename', () => { undefined as any, processServiceFactory.object, configService.object, undefined as any)); const processLogger = typeMoq.Mock.ofType(); - processLogger.setup(p => p.logProcess(typeMoq.It.isAny())).returns(() => { return; }); + processLogger.setup(p => p.logProcess(typeMoq.It.isAny(), typeMoq.It.isAny(), typeMoq.It.isAny())).returns(() => { return; }); serviceContainer.setup(s => s.get(typeMoq.It.isValue(IProcessLogger), typeMoq.It.isAny())).returns(() => processLogger.object); await initializeTest(); }); From 0e44650ecfe84b045a353f4a143649a7ad949e58 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 25 Jun 2019 15:39:58 -0700 Subject: [PATCH 12/21] Remove dead code --- src/client/common/process/types.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index 9a36b04839a1..de9994e215bb 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -40,12 +40,6 @@ export type ExecutionResult = { stderr?: T; }; -// export type ProcessServiceEventArgs = { -// file: string; -// args: string[]; -// options: SpawnOptions; -// }; - export const IProcessLogger = Symbol('IProcessLogger'); export interface IProcessLogger { logProcess(file: string, ars: string[], options?: SpawnOptions): void; From 678af2bfe56a401842046b14cb814589eeb3a78f Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 26 Jun 2019 09:03:47 -0700 Subject: [PATCH 13/21] PR comment fixes --- news/3 Code Health/1131.md | 2 +- src/client/common/process/logger.ts | 16 ++++++++-------- src/client/common/process/proc.ts | 4 ++-- src/client/common/process/processFactory.ts | 3 +-- .../common/process/pythonExecutionFactory.ts | 4 ++-- src/client/common/process/types.ts | 2 +- src/test/common/process/logger.unit.test.ts | 15 ++++++++------- .../process/pythonExecutionFactory.unit.test.ts | 2 +- 8 files changed, 24 insertions(+), 24 deletions(-) diff --git a/news/3 Code Health/1131.md b/news/3 Code Health/1131.md index a188997d2b96..42e007a755e2 100644 --- a/news/3 Code Health/1131.md +++ b/news/3 Code Health/1131.md @@ -1 +1 @@ -Log all extension commands being run behind the scenes in the extension output panel. +Log processes executed behind the scenes in the extension output panel. diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index eeb9eaff01db..f2b96980a455 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -13,15 +13,15 @@ export class ProcessLogger implements IProcessLogger { constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { } public logProcess(file: string, args: string[], options?: SpawnOptions) { - const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${current}`, ''); - const currentWorkingDirectory = options && options.cwd ? this.pathUtils.getDisplayName(options.cwd!) : undefined; - let info = `> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`; - - if (currentWorkingDirectory) { - info += `\n${Logging.currentWorkingDirectory()} ${currentWorkingDirectory}`; + const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${this.pathUtils.getDisplayName(current)}`, ''); + const info = [`> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`]; + if (options && options.cwd) { + info.push(`${Logging.currentWorkingDirectory()} ${this.pathUtils.getDisplayName(options.cwd)}`); } - traceInfo(info); - this.outputChannel.appendLine(info); + info.forEach(line => { + traceInfo(line); + this.outputChannel.appendLine(line); + }); } } diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index 36496795972c..6039854930dd 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -119,7 +119,7 @@ export class ProcessService extends EventEmitter implements IProcessService { }); }); - this.emit('processExecuted', file, args, options); + this.emit('exec', file, args, options); return { proc, @@ -181,7 +181,7 @@ export class ProcessService extends EventEmitter implements IProcessService { disposables.forEach(d => d.dispose()); }); - this.emit('processExecuted', file, args, options); + this.emit('exec', file, args, options); return deferred.promise; } diff --git a/src/client/common/process/processFactory.ts b/src/client/common/process/processFactory.ts index 3a3e75b081ba..f217994cd7d7 100644 --- a/src/client/common/process/processFactory.ts +++ b/src/client/common/process/processFactory.ts @@ -25,7 +25,6 @@ export class ProcessServiceFactory implements IProcessServiceFactory { const disposableRegistry = this.serviceContainer.get(IDisposableRegistry); const proc: IProcessService = new ProcessService(decoder, customEnvVars); disposableRegistry.push(proc); - proc.on('processExecuted', this.processLogger.logProcess.bind(this.processLogger)); - return proc; + return proc.on('exec', this.processLogger.logProcess.bind(this.processLogger)); } } diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index a2e9d835487c..784d536130c8 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -32,7 +32,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { const pythonPath = options.pythonPath ? options.pythonPath : this.configService.getSettings(options.resource).pythonPath; const processService: IProcessService = await this.processServiceFactory.create(options.resource); const processLogger = this.serviceContainer.get(IProcessLogger); - processService.on('processExecuted', processLogger.logProcess.bind(processLogger)); + processService.on('exec', processLogger.logProcess.bind(processLogger)); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } public async createActivatedEnvironment(options: ExecutionFactoryCreateWithEnvironmentOptions): Promise { @@ -45,7 +45,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { const pythonPath = options.interpreter ? options.interpreter.path : this.configService.getSettings(options.resource).pythonPath; const processService: IProcessService = new ProcessService(this.decoder, { ...envVars }); const processLogger = this.serviceContainer.get(IProcessLogger); - processService.on('processExecuted', processLogger.logProcess.bind(processLogger)); + processService.on('exec', processLogger.logProcess.bind(processLogger)); this.serviceContainer.get(IDisposableRegistry).push(processService); return new PythonExecutionService(this.serviceContainer, processService, pythonPath); } diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index de9994e215bb..de044693fa2e 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -49,7 +49,7 @@ export interface IProcessService extends IDisposable { execObservable(file: string, args: string[], options?: SpawnOptions): ObservableExecutionResult; exec(file: string, args: string[], options?: SpawnOptions): Promise>; shellExec(command: string, options?: ShellOptions): Promise>; - on(event: 'processExecuted', listener: (file: string, args: string[], options?: SpawnOptions) => void): this; + on(event: 'exec', listener: (file: string, args: string[], options?: SpawnOptions) => void): this; } export const IProcessServiceFactory = Symbol('IProcessServiceFactory'); diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts index 8c87a74740ab..baf517672110 100644 --- a/src/test/common/process/logger.unit.test.ts +++ b/src/test/common/process/logger.unit.test.ts @@ -26,7 +26,8 @@ suite('ProcessLogger suite', () => { }); setup(() => { - outputChannel.setup(o => o.appendLine(TypeMoq.It.isAnyString())).returns((s: string) => outputResult = s); + outputResult = ''; + outputChannel.setup(o => o.appendLine(TypeMoq.It.isAnyString())).returns((s: string) => outputResult += `${s}\n`); }); teardown(() => { @@ -38,10 +39,10 @@ suite('ProcessLogger suite', () => { const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess('test', ['--foo', '--bar'], options); - const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; + const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}\n`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect - String built incorrectly'); - outputChannel.verify(o => o.appendLine(TypeMoq.It.isAnyString()), TypeMoq.Times.once()); + outputChannel.verify(o => o.appendLine(TypeMoq.It.isAnyString()), TypeMoq.Times.exactly(2)); }); test('Logger replaces the path/to/home with ~ in the current working directory', async () => { @@ -49,7 +50,7 @@ suite('ProcessLogger suite', () => { const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess('test', ['--foo', '--bar'], options); - const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${path.join('~', 'debug', 'path')}`; + const expectedResult = `> test --foo --bar\n${Logging.currentWorkingDirectory()} ${path.join('~', 'debug', 'path')}\n`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); }); @@ -58,7 +59,7 @@ suite('ProcessLogger suite', () => { const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar'], options); - const expectedResult = `> ${path.join('~', 'test')} --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}`; + const expectedResult = `> ${path.join('~', 'test')} --foo --bar\n${Logging.currentWorkingDirectory()} ${options.cwd}\n`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); }); @@ -66,7 +67,7 @@ suite('ProcessLogger suite', () => { const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar']); - const expectedResult = `> ${path.join('~', 'test')} --foo --bar`; + const expectedResult = `> ${path.join('~', 'test')} --foo --bar\n`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Working directory line should not be displayed'); }); @@ -75,7 +76,7 @@ suite('ProcessLogger suite', () => { const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar'], options); - const expectedResult = `> ${path.join('~', 'test')} --foo --bar`; + const expectedResult = `> ${path.join('~', 'test')} --foo --bar\n`; expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Working directory line should not be displayed'); }); }); diff --git a/src/test/common/process/pythonExecutionFactory.unit.test.ts b/src/test/common/process/pythonExecutionFactory.unit.test.ts index a72dee6215f4..3f755abf35a4 100644 --- a/src/test/common/process/pythonExecutionFactory.unit.test.ts +++ b/src/test/common/process/pythonExecutionFactory.unit.test.ts @@ -79,7 +79,7 @@ suite('Process - PythonExecutionFactory', () => { processLogger = mock(ProcessLogger); when(processLogger.logProcess('', [], {})).thenReturn(); processService = mock(ProcessService); - when(processService.on('processExecuted', () => { return; })).thenReturn(processService); + when(processService.on('exec', () => { return; })).thenReturn(processService); const serviceContainer = mock(ServiceContainer); when(serviceContainer.get(IDisposableRegistry)).thenReturn([]); when(serviceContainer.get(IProcessLogger)).thenReturn(processLogger); From 9767cee8e2bb7929c00f76cfbbc4a7503ae1a38d Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 26 Jun 2019 10:48:29 -0700 Subject: [PATCH 14/21] Shorten cwd copy, add quotes to args with spaces --- package.nls.json | 2 +- src/client/common/process/logger.ts | 2 +- src/client/common/utils/localize.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.nls.json b/package.nls.json index 980ca388fbb4..ce8ce563362d 100644 --- a/package.nls.json +++ b/package.nls.json @@ -105,7 +105,7 @@ "Experiments.inGroup": "User belongs to experiment group '{0}'", "Interpreters.RefreshingInterpreters": "Refreshing Python Interpreters", "Interpreters.LoadingInterpreters": "Loading Python Interpreters", - "Logging.CurrentWorkingDirectory": "Current working directory:", + "Logging.CurrentWorkingDirectory": "cwd:", "Common.doNotShowAgain": "Do not show again", "Interpreters.environmentPromptMessage": "We noticed a new virtual environment has been created. Do you want to select it for the workspace folder?", "DataScience.restartKernelMessage": "Do you want to restart the IPython kernel? All variables will be lost.", diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index f2b96980a455..6a22d56f10b9 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -13,7 +13,7 @@ export class ProcessLogger implements IProcessLogger { constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { } public logProcess(file: string, args: string[], options?: SpawnOptions) { - const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${this.pathUtils.getDisplayName(current)}`, ''); + const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${this.pathUtils.getDisplayName(current.toCommandArgument())}`, ''); const info = [`> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`]; if (options && options.cwd) { info.push(`${Logging.currentWorkingDirectory()} ${this.pathUtils.getDisplayName(options.cwd)}`); diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 59fec76f50bd..07a8231a92a1 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -64,7 +64,7 @@ export namespace Interpreters { } export namespace Logging { - export const currentWorkingDirectory = localize('Logging.CurrentWorkingDirectory', 'Current working directory:'); + export const currentWorkingDirectory = localize('Logging.CurrentWorkingDirectory', 'cwd:'); } export namespace Linters { From fcbaa83d9a2cd261b836a5db6014318c4eb998e5 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 26 Jun 2019 16:45:52 -0700 Subject: [PATCH 15/21] Don't exclude first arg from being formatted --- src/client/common/process/logger.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index 6a22d56f10b9..bdc4cb0b7c18 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -13,7 +13,10 @@ export class ProcessLogger implements IProcessLogger { constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { } public logProcess(file: string, args: string[], options?: SpawnOptions) { - const formattedArgs = args.reduce((accumulator, current, index) => index === 0 ? current : `${accumulator} ${this.pathUtils.getDisplayName(current.toCommandArgument())}`, ''); + const formattedArgs = args.reduce((accumulator, current, index) => { + const formattedArg = this.pathUtils.getDisplayName(current.toCommandArgument()); + return index === 0 ? formattedArg : `${accumulator} ${formattedArg}`; + }, ''); const info = [`> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`]; if (options && options.cwd) { info.push(`${Logging.currentWorkingDirectory()} ${this.pathUtils.getDisplayName(options.cwd)}`); From 6292dbc19ffeb69923aacff6a50ed16f3465d86f Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 2 Jul 2019 09:20:04 -0700 Subject: [PATCH 16/21] Prettier-ified logger.ts and logger.unit.test.ts --- src/client/common/process/logger.ts | 5 ++++- src/test/common/process/logger.unit.test.ts | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index bdc4cb0b7c18..76470a19e362 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -10,7 +10,10 @@ import { IProcessLogger, SpawnOptions } from './types'; @injectable() export class ProcessLogger implements IProcessLogger { - constructor(@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, @inject(IPathUtils) private readonly pathUtils: IPathUtils) { } + constructor( + @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, + @inject(IPathUtils) private readonly pathUtils: IPathUtils + ) {} public logProcess(file: string, args: string[], options?: SpawnOptions) { const formattedArgs = args.reduce((accumulator, current, index) => { diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts index baf517672110..4bf6bb0ed7dd 100644 --- a/src/test/common/process/logger.unit.test.ts +++ b/src/test/common/process/logger.unit.test.ts @@ -27,7 +27,7 @@ suite('ProcessLogger suite', () => { setup(() => { outputResult = ''; - outputChannel.setup(o => o.appendLine(TypeMoq.It.isAnyString())).returns((s: string) => outputResult += `${s}\n`); + outputChannel.setup(o => o.appendLine(TypeMoq.It.isAnyString())).returns((s: string) => (outputResult += `${s}\n`)); }); teardown(() => { @@ -35,7 +35,7 @@ suite('ProcessLogger suite', () => { }); test('Logger displays the process command, arguments and current working directory in the output channel', async () => { - const options = { cwd: path.join('debug', 'path')}; + const options = { cwd: path.join('debug', 'path') }; const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess('test', ['--foo', '--bar'], options); @@ -72,7 +72,7 @@ suite('ProcessLogger suite', () => { }); test('Logger doesn\'t display the working directory line if there is no cwd key in the options parameter', async () => { - const options = { }; + const options = {}; const logger = new ProcessLogger(outputChannel.object, pathUtils); logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar'], options); From 2e6a1e794b9abcf51e3aadb3fbd9f13c311342b8 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 2 Jul 2019 11:16:37 -0700 Subject: [PATCH 17/21] Address comment --- src/client/common/process/logger.ts | 11 ++++++++--- src/test/common/process/logger.unit.test.ts | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index 76470a19e362..05ab913d2afd 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -16,11 +16,16 @@ export class ProcessLogger implements IProcessLogger { ) {} public logProcess(file: string, args: string[], options?: SpawnOptions) { - const formattedArgs = args.reduce((accumulator, current, index) => { - const formattedArg = this.pathUtils.getDisplayName(current.toCommandArgument()); + const argsList = args.reduce((accumulator, current, index) => { + let formattedArg = this.pathUtils.getDisplayName(current).toCommandArgument(); + if (current[0] === '\'' || current[0] === '"') { + formattedArg = `${current[0]}${this.pathUtils.getDisplayName(current.substr(1))}`; + } + return index === 0 ? formattedArg : `${accumulator} ${formattedArg}`; }, ''); - const info = [`> ${this.pathUtils.getDisplayName(file)} ${formattedArgs}`]; + + const info = [`> ${this.pathUtils.getDisplayName(file)} ${argsList}`]; if (options && options.cwd) { info.push(`${Logging.currentWorkingDirectory()} ${this.pathUtils.getDisplayName(options.cwd)}`); } diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts index 4bf6bb0ed7dd..18a3b1e9626e 100644 --- a/src/test/common/process/logger.unit.test.ts +++ b/src/test/common/process/logger.unit.test.ts @@ -45,6 +45,24 @@ suite('ProcessLogger suite', () => { outputChannel.verify(o => o.appendLine(TypeMoq.It.isAnyString()), TypeMoq.Times.exactly(2)); }); + test('Logger adds quotes around arguments if they contain spaces', async () => { + const options = { cwd: path.join('debug', 'path') }; + const logger = new ProcessLogger(outputChannel.object, pathUtils); + logger.logProcess('test', ['--foo', '--bar', 'import test'], options); + + const expectedResult = `> test --foo --bar "import test"\n${Logging.currentWorkingDirectory()} ${path.join('debug', 'path')}\n`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); + }); + + test('Logger preserves quotes around arguments if they contain spaces', async () => { + const options = { cwd: path.join('debug', 'path') }; + const logger = new ProcessLogger(outputChannel.object, pathUtils); + logger.logProcess('test', ['--foo', '--bar', '\'import test\''], options); + + const expectedResult = `> test --foo --bar \'import test\'\n${Logging.currentWorkingDirectory()} ${path.join('debug', 'path')}\n`; + expect(outputResult).to.equal(expectedResult, 'Output string is incorrect: Home directory is not tildified'); + }); + test('Logger replaces the path/to/home with ~ in the current working directory', async () => { const options = { cwd: path.join(untildify('~'), 'debug', 'path') }; const logger = new ProcessLogger(outputChannel.object, pathUtils); From 94fc7f401b0aaaed2585619d02cd999800486884 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 2 Jul 2019 13:42:59 -0700 Subject: [PATCH 18/21] unit tests --- .../process/processFactory.unit.test.ts | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 src/test/common/process/processFactory.unit.test.ts diff --git a/src/test/common/process/processFactory.unit.test.ts b/src/test/common/process/processFactory.unit.test.ts new file mode 100644 index 000000000000..12a64f00c9b3 --- /dev/null +++ b/src/test/common/process/processFactory.unit.test.ts @@ -0,0 +1,56 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +'use strict'; +import { expect } from 'chai'; +import { instance, mock, verify, when } from 'ts-mockito'; +import { Disposable, Uri } from 'vscode'; + +import { BufferDecoder } from '../../../client/common/process/decoder'; +import { ProcessLogger } from '../../../client/common/process/logger'; +import { ProcessService } from '../../../client/common/process/proc'; +import { ProcessServiceFactory } from '../../../client/common/process/processFactory'; +import { IBufferDecoder, IProcessLogger } from '../../../client/common/process/types'; +import { IDisposableRegistry } from '../../../client/common/types'; +import { EnvironmentVariablesProvider } from '../../../client/common/variables/environmentVariablesProvider'; +import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; + +suite('Process - ProcessServiceFactory', () => { + let factory: ProcessServiceFactory; + let envVariablesProvider: IEnvironmentVariablesProvider; + let bufferDecoder: IBufferDecoder; + let processLogger: IProcessLogger; + let processService: ProcessService; + let disposableRegistry: IDisposableRegistry; + + setup(() => { + bufferDecoder = mock(BufferDecoder); + envVariablesProvider = mock(EnvironmentVariablesProvider); + processLogger = mock(ProcessLogger); + when(processLogger.logProcess('', [], {})).thenReturn(); + processService = mock(ProcessService); + when( + processService.on('exec', () => { + return; + }) + ).thenReturn(processService); + disposableRegistry = []; + factory = new ProcessServiceFactory(instance(envVariablesProvider), instance(processLogger), instance(bufferDecoder), disposableRegistry); + }); + + teardown(() => { + (disposableRegistry as Disposable[]).forEach(d => d.dispose()); + }); + + [Uri.parse('test'), undefined].forEach(resource => { + test(`Ensure ProcessService is created with an ${resource ? 'existing' : 'undefined'} resource`, async () => { + when(envVariablesProvider.getEnvironmentVariables(resource)).thenResolve({ x: 'test' }); + + const proc = await factory.create(resource); + verify(envVariablesProvider.getEnvironmentVariables(resource)).once(); + + const disposables = disposableRegistry as Disposable[]; + expect(disposables.length).equal(1); + expect(proc).instanceOf(ProcessService); + }); + }); +}); From fc9d10a4dd0509f5ffb88f2c5b6c53703ffb272d Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 2 Jul 2019 13:52:06 -0700 Subject: [PATCH 19/21] refactor processFactory to use ioc --- src/client/common/process/processFactory.ts | 19 ++++++++----------- src/test/linters/lint.functional.test.ts | 4 +++- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/client/common/process/processFactory.ts b/src/client/common/process/processFactory.ts index f217994cd7d7..d6da78721138 100644 --- a/src/client/common/process/processFactory.ts +++ b/src/client/common/process/processFactory.ts @@ -5,7 +5,6 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; -import { IServiceContainer } from '../../ioc/types'; import { IDisposableRegistry } from '../types'; import { IEnvironmentVariablesProvider } from '../variables/types'; import { ProcessService } from './proc'; @@ -13,18 +12,16 @@ import { IBufferDecoder, IProcessLogger, IProcessService, IProcessServiceFactory @injectable() export class ProcessServiceFactory implements IProcessServiceFactory { - private envVarsService: IEnvironmentVariablesProvider; - private processLogger: IProcessLogger; - constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { - this.envVarsService = serviceContainer.get(IEnvironmentVariablesProvider); - this.processLogger = serviceContainer.get(IProcessLogger); - } + constructor( + @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider, + @inject(IProcessLogger) private readonly processLogger: IProcessLogger, + @inject(IBufferDecoder) private readonly decoder: IBufferDecoder, + @inject(IDisposableRegistry) private readonly disposableRegistry: IDisposableRegistry + ) {} public async create(resource?: Uri): Promise { const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource); - const decoder = this.serviceContainer.get(IBufferDecoder); - const disposableRegistry = this.serviceContainer.get(IDisposableRegistry); - const proc: IProcessService = new ProcessService(decoder, customEnvVars); - disposableRegistry.push(proc); + const proc: IProcessService = new ProcessService(this.decoder, customEnvVars); + this.disposableRegistry.push(proc); return proc.on('exec', this.processLogger.logProcess.bind(this.processLogger)); } } diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts index b9ad0985514c..a58f6fd823d3 100644 --- a/src/test/linters/lint.functional.test.ts +++ b/src/test/linters/lint.functional.test.ts @@ -246,7 +246,9 @@ class TestFixture extends BaseTestFixture { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IBufferDecoder), TypeMoq.It.isAny())) .returns(() => decoder); - const procServiceFactory = new ProcessServiceFactory(serviceContainer.object); + const processLogger = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); + const disposableRegistry = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); + const procServiceFactory = new ProcessServiceFactory(envVarsService.object, processLogger.object, decoder, disposableRegistry.object); return new PythonExecutionFactory( serviceContainer.object, From 15f0d2c2dc36c7753c339feeb801bdd7e544c8ca Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 2 Jul 2019 15:14:35 -0700 Subject: [PATCH 20/21] update debugAdapter too --- .../debugger/debugAdapter/Common/processServiceFactory.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/client/debugger/debugAdapter/Common/processServiceFactory.ts b/src/client/debugger/debugAdapter/Common/processServiceFactory.ts index cc969f52d8fd..b6facbdf39d6 100644 --- a/src/client/debugger/debugAdapter/Common/processServiceFactory.ts +++ b/src/client/debugger/debugAdapter/Common/processServiceFactory.ts @@ -7,14 +7,13 @@ import { inject, injectable } from 'inversify'; import { ProcessService } from '../../../common/process/proc'; import { IBufferDecoder, IProcessService, IProcessServiceFactory } from '../../../common/process/types'; import { IDisposableRegistry } from '../../../common/types'; -import { IServiceContainer } from '../../../ioc/types'; @injectable() export class DebuggerProcessServiceFactory implements IProcessServiceFactory { - constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { } + constructor(@inject(IBufferDecoder) private readonly decoder: IBufferDecoder, @inject(IDisposableRegistry) private readonly disposableRegistry: IDisposableRegistry) {} public create(): Promise { - const processService = new ProcessService(this.serviceContainer.get(IBufferDecoder), process.env); - this.serviceContainer.get(IDisposableRegistry).push(processService); + const processService = new ProcessService(this.decoder, process.env); + this.disposableRegistry.push(processService); return Promise.resolve(processService); } } From 338cf30376d764b27f49a87f869036b727ae9264 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 2 Jul 2019 15:34:13 -0700 Subject: [PATCH 21/21] Fix linting functional tests --- src/test/linters/lint.functional.test.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts index a58f6fd823d3..f2dc06456ad7 100644 --- a/src/test/linters/lint.functional.test.ts +++ b/src/test/linters/lint.functional.test.ts @@ -233,12 +233,10 @@ class TestFixture extends BaseTestFixture { configService: IConfigurationService ): IPythonExecutionFactory { const envVarsService = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); - envVarsService.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())) - .returns(() => Promise.resolve({})); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider), TypeMoq.It.isAny())) - .returns(() => envVarsService.object); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IDisposableRegistry), TypeMoq.It.isAny())) - .returns(() => []); + envVarsService.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider), TypeMoq.It.isAny())).returns(() => envVarsService.object); + const disposableRegistry: IDisposableRegistry = []; + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IDisposableRegistry), TypeMoq.It.isAny())).returns(() => disposableRegistry); const envActivationService = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); @@ -247,8 +245,12 @@ class TestFixture extends BaseTestFixture { .returns(() => decoder); const processLogger = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); - const disposableRegistry = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); - const procServiceFactory = new ProcessServiceFactory(envVarsService.object, processLogger.object, decoder, disposableRegistry.object); + processLogger + .setup(p => p.logProcess(TypeMoq.It.isAnyString(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => { + return; + }); + const procServiceFactory = new ProcessServiceFactory(envVarsService.object, processLogger.object, decoder, disposableRegistry); return new PythonExecutionFactory( serviceContainer.object,