From 54ef45d0d0557d88dad5df5e7ceb3836a4b400fe Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 3 Jun 2019 12:11:59 -0700 Subject: [PATCH 1/3] Changes to how shell in terminal is identified --- .vscode/launch.json | 2 +- news/2 Fixes/5743.md | 1 + src/client/common/terminal/activator/base.ts | 3 +- .../activator/powershellFailedHandler.ts | 4 +- src/client/common/terminal/helper.ts | 99 ++++++++++++------- src/client/common/terminal/service.ts | 3 +- src/client/common/terminal/types.ts | 3 +- src/client/extension.ts | 2 +- src/client/telemetry/constants.ts | 1 + src/client/telemetry/index.ts | 7 ++ .../terminals/activation.conda.unit.test.ts | 7 +- .../terminals/activator/base.unit.test.ts | 3 - .../powerShellFailedHandler.unit.test.ts | 6 -- src/test/common/terminals/helper.unit.test.ts | 45 ++------- .../common/terminals/service.unit.test.ts | 12 --- 15 files changed, 92 insertions(+), 106 deletions(-) create mode 100644 news/2 Fixes/5743.md diff --git a/.vscode/launch.json b/.vscode/launch.json index 7609fc076e7f..dcbc11b4c024 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -210,4 +210,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/news/2 Fixes/5743.md b/news/2 Fixes/5743.md new file mode 100644 index 000000000000..0906922fee16 --- /dev/null +++ b/news/2 Fixes/5743.md @@ -0,0 +1 @@ +Changes to identificaction of `shell` for the activation of environments in the terminal. diff --git a/src/client/common/terminal/activator/base.ts b/src/client/common/terminal/activator/base.ts index 0c3f23f812af..cae3f4108132 100644 --- a/src/client/common/terminal/activator/base.ts +++ b/src/client/common/terminal/activator/base.ts @@ -16,8 +16,7 @@ export class BaseTerminalActivator implements ITerminalActivator { } const deferred = createDeferred(); this.activatedTerminals.set(terminal, deferred.promise); - const shellPath = this.helper.getTerminalShellPath(); - const terminalShellType = !shellPath || shellPath.length === 0 ? TerminalShellType.other : this.helper.identifyTerminalShell(shellPath); + const terminalShellType = this.helper.identifyTerminalShell(terminal); const activationCommamnds = await this.helper.getEnvironmentActivationCommands(terminalShellType, resource); let activated = false; diff --git a/src/client/common/terminal/activator/powershellFailedHandler.ts b/src/client/common/terminal/activator/powershellFailedHandler.ts index 3196555ca9be..574b0532ae01 100644 --- a/src/client/common/terminal/activator/powershellFailedHandler.ts +++ b/src/client/common/terminal/activator/powershellFailedHandler.ts @@ -17,11 +17,11 @@ export class PowershellTerminalActivationFailedHandler implements ITerminalActiv @inject(IPlatformService) private readonly platformService: IPlatformService, @inject(IDiagnosticsService) @named(PowerShellActivationHackDiagnosticsServiceId) private readonly diagnosticService: IDiagnosticsService) { } - public async handleActivation(_terminal: Terminal, resource: Resource, _preserveFocus: boolean, activated: boolean) { + public async handleActivation(terminal: Terminal, resource: Resource, _preserveFocus: boolean, activated: boolean) { if (activated || !this.platformService.isWindows) { return; } - const shell = this.helper.identifyTerminalShell(this.helper.getTerminalShellPath()); + const shell = this.helper.identifyTerminalShell(terminal); if (shell !== TerminalShellType.powershell && shell !== TerminalShellType.powershellCore) { return; } diff --git a/src/client/common/terminal/helper.ts b/src/client/common/terminal/helper.ts index ec9369501c96..30670185d568 100644 --- a/src/client/common/terminal/helper.ts +++ b/src/client/common/terminal/helper.ts @@ -2,15 +2,16 @@ // Licensed under the MIT License. import { inject, injectable, named } from 'inversify'; +import * as os from 'os'; import { Terminal, Uri } from 'vscode'; import { ICondaService, IInterpreterService, InterpreterType, PythonInterpreter } from '../../interpreter/contracts'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; -import { ITerminalManager, IWorkspaceService } from '../application/types'; +import { ITerminalManager } from '../application/types'; import '../extensions'; import { traceDecorators, traceError } from '../logger'; import { IPlatformService } from '../platform/types'; -import { IConfigurationService, Resource } from '../types'; +import { IConfigurationService, ICurrentProcess, Resource } from '../types'; import { OSType } from '../utils/platform'; import { ITerminalActivationCommandProvider, ITerminalHelper, TerminalActivationProviders, TerminalShellType } from './types'; @@ -21,7 +22,7 @@ const IS_BASH = /(bash.exe$|bash$)/i; const IS_WSL = /(wsl.exe$)/i; const IS_ZSH = /(zsh$)/i; const IS_KSH = /(ksh$)/i; -const IS_COMMAND = /cmd.exe$/i; +const IS_COMMAND = /(cmd.exe$|cmd$)/i; const IS_POWERSHELL = /(powershell.exe$|powershell$)/i; const IS_POWERSHELL_CORE = /(pwsh.exe$|pwsh$)/i; const IS_FISH = /(fish$)/i; @@ -41,7 +42,6 @@ export class TerminalHelper implements ITerminalHelper { private readonly detectableShells: Map; constructor(@inject(IPlatformService) private readonly platform: IPlatformService, @inject(ITerminalManager) private readonly terminalManager: ITerminalManager, - @inject(IWorkspaceService) private readonly workspace: IWorkspaceService, @inject(ICondaService) private readonly condaService: ICondaService, @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, @inject(IConfigurationService) private readonly configurationService: IConfigurationService, @@ -49,7 +49,8 @@ export class TerminalHelper implements ITerminalHelper { @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.bashCShellFish) private readonly bashCShellFish: ITerminalActivationCommandProvider, @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.commandPromptAndPowerShell) private readonly commandPromptAndPowerShell: ITerminalActivationCommandProvider, @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.pyenv) private readonly pyenv: ITerminalActivationCommandProvider, - @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.pipenv) private readonly pipenv: ITerminalActivationCommandProvider + @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.pipenv) private readonly pipenv: ITerminalActivationCommandProvider, + @inject(IConfigurationService) private readonly currentProcess: ICurrentProcess ) { this.detectableShells = new Map(); this.detectableShells.set(TerminalShellType.powershell, IS_POWERSHELL); @@ -68,36 +69,39 @@ export class TerminalHelper implements ITerminalHelper { public createTerminal(title?: string): Terminal { return this.terminalManager.createTerminal({ name: title }); } - public identifyTerminalShell(shellPath: string): TerminalShellType { - return Array.from(this.detectableShells.keys()) - .reduce((matchedShell, shellToDetect) => { - if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(shellPath)) { - return shellToDetect; - } - return matchedShell; - }, TerminalShellType.other); - } - public getTerminalShellPath(): string { - const shellConfig = this.workspace.getConfiguration('terminal.integrated.shell'); - let osSection = ''; - switch (this.platform.osType) { - case OSType.Windows: { - osSection = 'windows'; - break; - } - case OSType.OSX: { - osSection = 'osx'; - break; - } - case OSType.Linux: { - osSection = 'linux'; - break; - } - default: { - return ''; - } + public identifyTerminalShell(terminal?: Terminal): TerminalShellType { + let shell = TerminalShellType.other; + let usingDefaultShell = false; + const terminalProvided = !!terminal; + // Determine shell based on the name of the terminal. + // See solution here https://github.com/microsoft/vscode/issues/74233#issuecomment-497527337 + if (terminal) { + shell = Array.from(this.detectableShells.keys()) + .reduce((matchedShell, shellToDetect) => { + if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(terminal.name)) { + return shellToDetect; + } + return matchedShell; + }, TerminalShellType.other); + } + + // If still unable to identify, then use fall back to determine path to the default shell. + if (shell === TerminalShellType.other) { + const shellPath = getDefaultShell(this.platform.osType, this.currentProcess); + shell = Array.from(this.detectableShells.keys()) + .reduce((matchedShell, shellToDetect) => { + if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(shellPath)) { + return shellToDetect; + } + return matchedShell; + }, TerminalShellType.other); + + // We have restored to using the default shell. + usingDefaultShell = shell !== TerminalShellType.other; } - return shellConfig.get(osSection)!; + const properties = { failed: shell === TerminalShellType.other, usingDefaultShell, terminalProvided }; + sendTelemetryEvent(EventName.TERMINAL_SHELL_IDENTIFICATION, undefined, properties); + return shell; } public buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]) { const isPowershell = terminalShellType === TerminalShellType.powershell || terminalShellType === TerminalShellType.powershellCore; @@ -173,3 +177,30 @@ export class TerminalHelper implements ITerminalHelper { } } } + +/* + The following code is based on VS Code from https://github.com/microsoft/vscode/blob/5c65d9bfa4c56538150d7f3066318e0db2c6151f/src/vs/workbench/contrib/terminal/node/terminal.ts#L12-L55 + This is only a fall back to identify the default shell used by VSC. + On Windows, determine the default shell. + On others, default to bash. +*/ +export function getDefaultShell(osType: OSType, currentProcess: ICurrentProcess): string { + if (osType === OSType.Windows) { + return getTerminalDefaultShellWindows(osType, currentProcess); + } + return '/bin/bash'; +} +let _TERMINAL_DEFAULT_SHELL_WINDOWS: string | null = null; +function getTerminalDefaultShellWindows(osType: OSType, currentProcess: ICurrentProcess): string { + if (!_TERMINAL_DEFAULT_SHELL_WINDOWS) { + const isAtLeastWindows10 = osType === OSType.Windows && parseFloat(os.release()) >= 10; + const is32ProcessOn64Windows = process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432'); + const powerShellPath = `${process.env.windir}\\${is32ProcessOn64Windows ? 'Sysnative' : 'System32'}\\WindowsPowerShell\\v1.0\\powershell.exe`; + _TERMINAL_DEFAULT_SHELL_WINDOWS = isAtLeastWindows10 ? powerShellPath : getWindowsShell(currentProcess); + } + return _TERMINAL_DEFAULT_SHELL_WINDOWS; +} + +function getWindowsShell(currentProcess: ICurrentProcess): string { + return currentProcess.env.comspec || 'cmd.exe'; +} diff --git a/src/client/common/terminal/service.ts b/src/client/common/terminal/service.ts index 2db3b9f5c3c3..ec3710aefa8c 100644 --- a/src/client/common/terminal/service.ts +++ b/src/client/common/terminal/service.ts @@ -58,8 +58,7 @@ export class TerminalService implements ITerminalService, Disposable { if (this.terminal) { return; } - const shellPath = this.terminalHelper.getTerminalShellPath(); - this.terminalShellType = !shellPath || shellPath.length === 0 ? TerminalShellType.other : this.terminalHelper.identifyTerminalShell(shellPath); + this.terminalShellType = this.terminalHelper.identifyTerminalShell(this.terminal); this.terminal = this.terminalManager.createTerminal({ name: this.title }); // Sometimes the terminal takes some time to start up before it can start accepting input. diff --git a/src/client/common/terminal/types.ts b/src/client/common/terminal/types.ts index 193e2e1efbb3..ec92bdc1e7f8 100644 --- a/src/client/common/terminal/types.ts +++ b/src/client/common/terminal/types.ts @@ -54,8 +54,7 @@ export const ITerminalHelper = Symbol('ITerminalHelper'); export interface ITerminalHelper { createTerminal(title?: string): Terminal; - identifyTerminalShell(shellPath: string): TerminalShellType; - getTerminalShellPath(): string; + identifyTerminalShell(terminal?: Terminal): TerminalShellType; buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]): string; getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri): Promise; getEnvironmentActivationShellCommands(resource: Resource, interpreter?: PythonInterpreter): Promise; diff --git a/src/client/extension.ts b/src/client/extension.ts index 3507f53b9ae9..eaccea7e8f52 100644 --- a/src/client/extension.ts +++ b/src/client/extension.ts @@ -354,7 +354,7 @@ async function getActivationTelemetryProps(serviceContainer: IServiceContainer): // be able to partially populate as much as possible instead // (through granular try-catch statements). const terminalHelper = serviceContainer.get(ITerminalHelper); - const terminalShellType = terminalHelper.identifyTerminalShell(terminalHelper.getTerminalShellPath()); + const terminalShellType = terminalHelper.identifyTerminalShell(); const condaLocator = serviceContainer.get(ICondaService); const interpreterService = serviceContainer.get(IInterpreterService); const workspaceService = serviceContainer.get(IWorkspaceService); diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index bb7d55f396b4..0100f24da04e 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -29,6 +29,7 @@ export enum EventName { PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES = 'PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES', PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE = 'PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE', PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL = 'PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL', + TERMINAL_SHELL_IDENTIFICATION = 'TERMINAL_SHELL_IDENTIFICATION', PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT', ENVFILE_VARIABLE_SUBSTITUTION = 'ENVFILE_VARIABLE_SUBSTITUTION', WORKSPACE_SYMBOLS_BUILD = 'WORKSPACE_SYMBOLS.BUILD', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index ddebf55c913b..ecb1f9d3f7da 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -377,4 +377,11 @@ export interface IEventNamePropertyMapping { restart - Whether to restart the Jedi Process (i.e. memory > limit). */ [EventName.JEDI_MEMORY]: { memory: number; limit: number; isUserDefinedLimit: boolean; restart: boolean }; + /* + Telemetry event sent to provide information on whether we have successfully identify the type of shell used. + failed - If true, indicates we have failed to identify the shell. Note this impacts impacts ability to activate environments in the terminal & code. + usingDefaultShell - If true, this indicates we failed to identify the shell and we have reverted to using the default shell on user machine. + terminalProvided - If true, we used the terminal provided to detec the shell. If not provided, we use the default shell on user machine. + */ + [EventName.TERMINAL_SHELL_IDENTIFICATION]: { failed: boolean; usingDefaultShell: boolean; terminalProvided: boolean }; } diff --git a/src/test/common/terminals/activation.conda.unit.test.ts b/src/test/common/terminals/activation.conda.unit.test.ts index 7afbb07574a4..974f1997bf59 100644 --- a/src/test/common/terminals/activation.conda.unit.test.ts +++ b/src/test/common/terminals/activation.conda.unit.test.ts @@ -10,11 +10,11 @@ import { anything, instance, mock, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { Disposable } from 'vscode'; import { TerminalManager } from '../../../client/common/application/terminalManager'; -import { WorkspaceService } from '../../../client/common/application/workspace'; import '../../../client/common/extensions'; import { IFileSystem, IPlatformService } from '../../../client/common/platform/types'; +import { CurrentProcess } from '../../../client/common/process/currentProcess'; import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; @@ -84,7 +84,7 @@ suite('Terminal Environment Activation conda', () => { pythonSettings.setup(s => s.terminal).returns(() => terminalSettings.object); terminalHelper = new TerminalHelper(platformService.object, - instance(mock(TerminalManager)), instance(mock(WorkspaceService)), + instance(mock(TerminalManager)), condaService.object, instance(mock(InterpreterService)), configService.object, @@ -92,7 +92,8 @@ suite('Terminal Environment Activation conda', () => { instance(bash), mock(CommandPromptAndPowerShell), mock(PyEnvActivationCommandProvider), - mock(PipEnvActivationCommandProvider)); + mock(PipEnvActivationCommandProvider), + instance(mock(CurrentProcess))); }); teardown(() => { diff --git a/src/test/common/terminals/activator/base.unit.test.ts b/src/test/common/terminals/activator/base.unit.test.ts index 80584ebbe3ca..54d68bfc6cf8 100644 --- a/src/test/common/terminals/activator/base.unit.test.ts +++ b/src/test/common/terminals/activator/base.unit.test.ts @@ -30,7 +30,6 @@ suite('Terminal Base Activator', () => { const titleSuffix = `(${item.commandCount} activation command, and preserve focus in terminal is ${item.preserveFocus})`; const activationCommands = item.commandCount === 1 ? ['CMD1'] : ['CMD1', 'CMD2']; test(`Terminal is activated ${titleSuffix}`, async () => { - helper.setup(h => h.getTerminalShellPath()).returns(() => ''); helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(activationCommands)); const terminal = TypeMoq.Mock.ofType(); @@ -50,7 +49,6 @@ suite('Terminal Base Activator', () => { terminal.verifyAll(); }); test(`Terminal is activated only once ${titleSuffix}`, async () => { - helper.setup(h => h.getTerminalShellPath()).returns(() => ''); helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(activationCommands)); const terminal = TypeMoq.Mock.ofType(); @@ -72,7 +70,6 @@ suite('Terminal Base Activator', () => { terminal.verifyAll(); }); test(`Terminal is activated only once ${titleSuffix} (even when not waiting)`, async () => { - helper.setup(h => h.getTerminalShellPath()).returns(() => ''); helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(activationCommands)); const terminal = TypeMoq.Mock.ofType(); diff --git a/src/test/common/terminals/activator/powerShellFailedHandler.unit.test.ts b/src/test/common/terminals/activator/powerShellFailedHandler.unit.test.ts index 1711af2988f4..eb2384483090 100644 --- a/src/test/common/terminals/activator/powerShellFailedHandler.unit.test.ts +++ b/src/test/common/terminals/activator/powerShellFailedHandler.unit.test.ts @@ -20,18 +20,12 @@ suite('Terminal Activation Powershell Failed Handler', () => { async function testDiagnostics(mustHandleDiagnostics: boolean, isWindows: boolean, activatedSuccessfully: boolean, shellType: TerminalShellType, cmdPromptHasActivationCommands: boolean) { platform.setup(p => p.isWindows).returns(() => isWindows); - helper - .setup(p => p.getTerminalShellPath()) - .returns(() => ''); - // .verifiable(TypeMoq.Times.atMostOnce()); helper .setup(p => p.identifyTerminalShell(TypeMoq.It.isAny())) .returns(() => shellType); - // .verifiable(TypeMoq.Times.atMostOnce());c const cmdPromptCommands = cmdPromptHasActivationCommands ? ['a'] : []; helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isValue(TerminalShellType.commandPrompt), TypeMoq.It.isAny())) .returns(() => Promise.resolve(cmdPromptCommands)); - // .verifiable(TypeMoq.Times.atMostOnce()); diagnosticService .setup(d => d.handle(TypeMoq.It.isAny())) diff --git a/src/test/common/terminals/helper.unit.test.ts b/src/test/common/terminals/helper.unit.test.ts index ec592448892f..f981df1cd80b 100644 --- a/src/test/common/terminals/helper.unit.test.ts +++ b/src/test/common/terminals/helper.unit.test.ts @@ -3,16 +3,14 @@ import { expect } from 'chai'; import { SemVer } from 'semver'; import { anything, capture, instance, mock, verify, when } from 'ts-mockito'; -import * as TypeMoq from 'typemoq'; -import { Uri, WorkspaceConfiguration } from 'vscode'; - +import { Terminal, Uri } from 'vscode'; import { TerminalManager } from '../../../client/common/application/terminalManager'; -import { ITerminalManager, IWorkspaceService } from '../../../client/common/application/types'; -import { WorkspaceService } from '../../../client/common/application/workspace'; +import { ITerminalManager } from '../../../client/common/application/types'; import { PythonSettings } from '../../../client/common/configSettings'; import { ConfigurationService } from '../../../client/common/configuration/service'; import { PlatformService } from '../../../client/common/platform/platformService'; import { IPlatformService } from '../../../client/common/platform/types'; +import { CurrentProcess } from '../../../client/common/process/currentProcess'; import { Bash } from '../../../client/common/terminal/environmentActivationProviders/bash'; import { CommandPromptAndPowerShell } from '../../../client/common/terminal/environmentActivationProviders/commandPrompt'; import { @@ -43,7 +41,6 @@ suite('Terminal Service helpers', () => { let helper: ITerminalHelper; let terminalManager: ITerminalManager; let platformService: IPlatformService; - let workspaceService: IWorkspaceService; let condaService: ICondaService; let configurationService: IConfigurationService; let condaActivationProvider: ITerminalActivationCommandProvider; @@ -65,7 +62,6 @@ suite('Terminal Service helpers', () => { function doSetup() { terminalManager = mock(TerminalManager); platformService = mock(PlatformService); - workspaceService = mock(WorkspaceService); condaService = mock(CondaService); configurationService = mock(ConfigurationService); condaActivationProvider = mock(CondaActivationCommandProvider); @@ -76,7 +72,6 @@ suite('Terminal Service helpers', () => { pythonSettings = mock(PythonSettings); helper = new TerminalHelper(instance(platformService), instance(terminalManager), - instance(workspaceService), instance(condaService), instance(mock(InterpreterService)), instance(configurationService), @@ -84,7 +79,8 @@ suite('Terminal Service helpers', () => { instance(bashActivationProvider), instance(cmdActivationProvider), instance(pyenvActivationProvider), - instance(pipenvActivationProvider)); + instance(pipenvActivationProvider), + instance(mock(CurrentProcess))); } suite('Misc', () => { setup(doSetup); @@ -140,37 +136,10 @@ suite('Terminal Service helpers', () => { shellPathsAndIdentification.set('/usr/bin/xonshx', TerminalShellType.other); shellPathsAndIdentification.forEach((shellType, shellPath) => { - expect(helper.identifyTerminalShell(shellPath)).to.equal(shellType, `Incorrect Shell Type for path '${shellPath}'`); + const terminal = {name: shellPath} as any as Terminal; + expect(helper.identifyTerminalShell(terminal)).to.equal(shellType, `Incorrect Shell Type for path '${shellPath}'`); }); }); - async function ensurePathForShellIsCorrectlyRetrievedFromSettings(osType: OSType, expectedShellPath: string) { - when(platformService.osType).thenReturn(osType); - const cfgSetting = osType === OSType.Windows ? 'windows' : (osType === OSType.OSX ? 'osx' : 'linux'); - const workspaceConfig = TypeMoq.Mock.ofType(); - const invocationCount = osType === OSType.Unknown ? 0 : 1; - workspaceConfig - .setup(w => w.get(TypeMoq.It.isValue(cfgSetting))) - .returns(() => expectedShellPath) - .verifiable(TypeMoq.Times.exactly(invocationCount)); - when(workspaceService.getConfiguration('terminal.integrated.shell')).thenReturn(workspaceConfig.object); - - const shellPath = helper.getTerminalShellPath(); - - workspaceConfig.verifyAll(); - expect(shellPath).to.equal(expectedShellPath, 'Incorrect path for Osx'); - } - test('Ensure path for shell is correctly retrieved from settings (osx)', async () => { - await ensurePathForShellIsCorrectlyRetrievedFromSettings(OSType.OSX, 'abcd'); - }); - test('Ensure path for shell is correctly retrieved from settings (linux)', async () => { - await ensurePathForShellIsCorrectlyRetrievedFromSettings(OSType.Linux, 'abcd'); - }); - test('Ensure path for shell is correctly retrieved from settings (windows)', async () => { - await ensurePathForShellIsCorrectlyRetrievedFromSettings(OSType.Windows, 'abcd'); - }); - test('Ensure path for shell is correctly retrieved from settings (unknown os)', async () => { - await ensurePathForShellIsCorrectlyRetrievedFromSettings(OSType.Unknown, ''); - }); test('Ensure spaces in command is quoted', async () => { getNamesAndValues(TerminalShellType).forEach(item => { const command = 'c:\\python 3.7.exe'; diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index 24814c12decb..9afc97d5eb53 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -79,7 +79,6 @@ suite('Terminal Service', () => { const args = ['1', '2']; const commandToExpect = [commandToSend].concat(args).join(' '); terminalHelper.setup(h => h.buildCommandForTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => commandToExpect); - terminalHelper.setup(h => h.getTerminalShellPath()).returns(() => ''); terminalHelper.setup(h => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup(t => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); @@ -93,7 +92,6 @@ suite('Terminal Service', () => { terminalHelper.setup(helper => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(undefined)); service = new TerminalService(mockServiceContainer.object); const textToSend = 'Some Text'; - terminalHelper.setup(h => h.getTerminalShellPath()).returns(() => ''); terminalHelper.setup(h => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup(t => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); @@ -106,7 +104,6 @@ suite('Terminal Service', () => { test('Ensure terminal shown', async () => { terminalHelper.setup(helper => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(undefined)); service = new TerminalService(mockServiceContainer.object); - terminalHelper.setup(h => h.getTerminalShellPath()).returns(() => ''); terminalHelper.setup(h => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup(t => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); @@ -118,7 +115,6 @@ suite('Terminal Service', () => { test('Ensure terminal shown and focus is set to the Terminal', async () => { terminalHelper.setup(helper => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(undefined)); service = new TerminalService(mockServiceContainer.object); - terminalHelper.setup(h => h.getTerminalShellPath()).returns(() => ''); terminalHelper.setup(h => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup(t => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); @@ -129,9 +125,6 @@ suite('Terminal Service', () => { test('Ensure terminal is activated once after creation', async () => { service = new TerminalService(mockServiceContainer.object); - terminalHelper - .setup(h => h.getTerminalShellPath()).returns(() => '') - .verifiable(TypeMoq.Times.once()); terminalActivator .setup(h => h.activateEnvironmentInTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(true)) @@ -153,9 +146,6 @@ suite('Terminal Service', () => { test('Ensure terminal is activated once before sending text', async () => { service = new TerminalService(mockServiceContainer.object); const textToSend = 'Some Text'; - terminalHelper - .setup(h => h.getTerminalShellPath()).returns(() => '') - .verifiable(TypeMoq.Times.once()); terminalActivator .setup(h => h.activateEnvironmentInTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(true)) @@ -185,7 +175,6 @@ suite('Terminal Service', () => { }); service = new TerminalService(mockServiceContainer.object); service.onDidCloseTerminal(() => eventFired = true, service); - terminalHelper.setup(h => h.getTerminalShellPath()).returns(() => ''); terminalHelper.setup(h => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup(t => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); @@ -209,7 +198,6 @@ suite('Terminal Service', () => { service = new TerminalService(mockServiceContainer.object); service.onDidCloseTerminal(() => eventFired = true); - terminalHelper.setup(h => h.getTerminalShellPath()).returns(() => ''); terminalHelper.setup(h => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup(t => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); From 5846ac74610629d82f6649f1f4f0fc7f256c6b43 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 3 Jun 2019 13:10:08 -0700 Subject: [PATCH 2/3] Fix tests --- src/client/common/terminal/helper.ts | 18 +++++++++++------- src/test/common/terminals/helper.unit.test.ts | 8 +++----- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/client/common/terminal/helper.ts b/src/client/common/terminal/helper.ts index 30670185d568..45090b8f0699 100644 --- a/src/client/common/terminal/helper.ts +++ b/src/client/common/terminal/helper.ts @@ -76,13 +76,7 @@ export class TerminalHelper implements ITerminalHelper { // Determine shell based on the name of the terminal. // See solution here https://github.com/microsoft/vscode/issues/74233#issuecomment-497527337 if (terminal) { - shell = Array.from(this.detectableShells.keys()) - .reduce((matchedShell, shellToDetect) => { - if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(terminal.name)) { - return shellToDetect; - } - return matchedShell; - }, TerminalShellType.other); + shell = this.identifyTerminalShellByName(terminal.name); } // If still unable to identify, then use fall back to determine path to the default shell. @@ -103,6 +97,16 @@ export class TerminalHelper implements ITerminalHelper { sendTelemetryEvent(EventName.TERMINAL_SHELL_IDENTIFICATION, undefined, properties); return shell; } + public identifyTerminalShellByName(name: string): TerminalShellType { + return Array.from(this.detectableShells.keys()) + .reduce((matchedShell, shellToDetect) => { + if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(name)) { + return shellToDetect; + } + return matchedShell; + }, TerminalShellType.other); + } + public buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]) { const isPowershell = terminalShellType === TerminalShellType.powershell || terminalShellType === TerminalShellType.powershellCore; const commandPrefix = isPowershell ? '& ' : ''; diff --git a/src/test/common/terminals/helper.unit.test.ts b/src/test/common/terminals/helper.unit.test.ts index f981df1cd80b..64c70aaf7b53 100644 --- a/src/test/common/terminals/helper.unit.test.ts +++ b/src/test/common/terminals/helper.unit.test.ts @@ -3,7 +3,7 @@ import { expect } from 'chai'; import { SemVer } from 'semver'; import { anything, capture, instance, mock, verify, when } from 'ts-mockito'; -import { Terminal, Uri } from 'vscode'; +import { Uri } from 'vscode'; import { TerminalManager } from '../../../client/common/application/terminalManager'; import { ITerminalManager } from '../../../client/common/application/types'; import { PythonSettings } from '../../../client/common/configSettings'; @@ -25,7 +25,6 @@ import { import { TerminalHelper } from '../../../client/common/terminal/helper'; import { ITerminalActivationCommandProvider, - ITerminalHelper, TerminalShellType } from '../../../client/common/terminal/types'; import { IConfigurationService } from '../../../client/common/types'; @@ -38,7 +37,7 @@ import { CondaService } from '../../../client/interpreter/locators/services/cond // tslint:disable:max-func-body-length no-any suite('Terminal Service helpers', () => { - let helper: ITerminalHelper; + let helper: TerminalHelper; let terminalManager: ITerminalManager; let platformService: IPlatformService; let condaService: ICondaService; @@ -136,8 +135,7 @@ suite('Terminal Service helpers', () => { shellPathsAndIdentification.set('/usr/bin/xonshx', TerminalShellType.other); shellPathsAndIdentification.forEach((shellType, shellPath) => { - const terminal = {name: shellPath} as any as Terminal; - expect(helper.identifyTerminalShell(terminal)).to.equal(shellType, `Incorrect Shell Type for path '${shellPath}'`); + expect(helper.identifyTerminalShellByName(shellPath)).to.equal(shellType, `Incorrect Shell Type for path '${shellPath}'`); }); }); test('Ensure spaces in command is quoted', async () => { From 8de540072ce68641fc4b29276030d086b3758ad8 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 4 Jun 2019 10:41:02 -0700 Subject: [PATCH 3/3] Add new tests --- src/client/common/terminal/helper.ts | 2 +- src/test/common/terminals/helper.unit.test.ts | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/client/common/terminal/helper.ts b/src/client/common/terminal/helper.ts index 45090b8f0699..ff67db9a74d6 100644 --- a/src/client/common/terminal/helper.ts +++ b/src/client/common/terminal/helper.ts @@ -188,7 +188,7 @@ export class TerminalHelper implements ITerminalHelper { On Windows, determine the default shell. On others, default to bash. */ -export function getDefaultShell(osType: OSType, currentProcess: ICurrentProcess): string { +function getDefaultShell(osType: OSType, currentProcess: ICurrentProcess): string { if (osType === OSType.Windows) { return getTerminalDefaultShellWindows(osType, currentProcess); } diff --git a/src/test/common/terminals/helper.unit.test.ts b/src/test/common/terminals/helper.unit.test.ts index 64c70aaf7b53..28f255278d8a 100644 --- a/src/test/common/terminals/helper.unit.test.ts +++ b/src/test/common/terminals/helper.unit.test.ts @@ -107,6 +107,13 @@ suite('Terminal Service helpers', () => { expect(term).to.be.deep.equal(terminal); expect(args.name).to.be.deep.equal(theTitle); }); + test('Revert to default shell', async () => { + when(platformService.osType).thenReturn(OSType.OSX); + + const shellType = helper.identifyTerminalShell(); + + expect(shellType).to.be.equal(TerminalShellType.bash); + }); test('Test identification of Terminal Shells', async () => { const shellPathsAndIdentification = new Map(); shellPathsAndIdentification.set('c:\\windows\\system32\\cmd.exe', TerminalShellType.commandPrompt);