Fixes to detecting shells (microsoft#6627)

* Fixes * Better comments * Enable test

Author: DonJayamanne

src/client/common/terminal/shellDetector.ts

@@ -53,6 +53,7 @@ export class ShellDetector {
 
  for (const detector of shellDetectors) {
  shell = detector.identify(telemetryProperties, terminal);
+ traceVerbose(`${detector}. Shell identified as ${shell} ${terminal ? `(Terminal name is ${terminal.name})` : ''}`);
  if (shell) {
  break;
  }
@@ -66,6 +67,7 @@ export class ShellDetector {
 
  // If we could not identify the shell, use the defaults.
  if (shell === undefined || shell === TerminalShellType.other) {
+ traceVerbose('Using default OS shell');
  shell = defaultOSShells[this.platform.osType];
  }
  return shell;

src/client/common/terminal/shellDetectors/baseShellDetector.ts

@@ -13,6 +13,7 @@ import { IShellDetector, ShellIdentificationTelemetry, TerminalShellType } from
 /*
 When identifying the shell use the following algorithm:
 * 1. Identify shell based on the name of the terminal (if there is one already opened and used).
+* 2. Identify shell based on the api provided by VSC.
 * 2. Identify shell based on the settings in VSC.
 * 3. Identify shell based on users environment variables.
 * 4. Use default shells (bash for mac and linux, cmd for windows).

src/client/common/terminal/shellDetectors/terminalNameShellDetector.ts

@@ -18,12 +18,12 @@ import { BaseShellDetector } from './baseShellDetector';
  */
 @injectable()
 export class TerminalNameShellDetector extends BaseShellDetector {
- constructor() { super(0); }
+ constructor() { super(4); }
  public identify(telemetryProperties: ShellIdentificationTelemetry, terminal?: Terminal): TerminalShellType | undefined {
  if (!terminal) {
  return;
  }
- const shell = this.identifyShellFromShellPath(terminal.name)
+ const shell = this.identifyShellFromShellPath(terminal.name);
  traceVerbose(`Terminal name '${terminal.name}' identified as shell '${shell}'`);
  telemetryProperties.shellIdentificationSource = shell === TerminalShellType.other ? telemetryProperties.shellIdentificationSource : 'terminalName';
  return shell;

src/client/common/terminal/shellDetectors/userEnvironmentShellDetector.ts

@@ -12,7 +12,6 @@ import { OSType } from '../../utils/platform';
 import { ShellIdentificationTelemetry, TerminalShellType } from '../types';
 import { BaseShellDetector } from './baseShellDetector';
 
-
 /**
  * Identifies the shell based on the users environment (env variables).
  *
@@ -24,7 +23,7 @@ import { BaseShellDetector } from './baseShellDetector';
 export class UserEnvironmentShellDetector extends BaseShellDetector {
  constructor(@inject(ICurrentProcess) private readonly currentProcess: ICurrentProcess,
  @inject(IPlatformService) private readonly platform: IPlatformService) {
- super(3);
+ super(1);
  }
  public getDefaultPlatformShell(): string {
  return getDefaultShell(this.platform, this.currentProcess);

src/client/common/terminal/shellDetectors/vscEnvironmentShellDetector.ts

@@ -18,7 +18,7 @@ import { BaseShellDetector } from './baseShellDetector';
  * @extends {BaseShellDetector}
  */
 export class VSCEnvironmentShellDetector extends BaseShellDetector {
- constructor(@inject(IApplicationEnvironment) private readonly appEnv: IApplicationEnvironment) { super(1); }
+ constructor(@inject(IApplicationEnvironment) private readonly appEnv: IApplicationEnvironment) { super(3); }
  public identify(telemetryProperties: ShellIdentificationTelemetry, _terminal?: Terminal): TerminalShellType | undefined {
  if (!this.appEnv.shell) {
  return;

src/test/common/terminals/environmentActivationProviders/terminalActivation.testvirtualenvs.ts

@@ -129,9 +129,9 @@ suite('Activation of Environments in Terminal', () => {
  test('Should activate with virtualenv', async () => {
  await testActivation(envPaths.virtualEnvPath);
  });
- // test('Should activate with conda', async () => {
- //  await terminalSettings.update('integrated.shell.windows', 'C:\\Windows\\System32\\cmd.exe', vscode.ConfigurationTarget.Global);
- //  await pythonSettings.update('condaPath', envPaths.condaExecPath, vscode.ConfigurationTarget.Workspace);
- //  await testActivation(envPaths.condaPath);
- // });
+ test('Should activate with conda', async () => {
+ await terminalSettings.update('integrated.shell.windows', 'C:\\Windows\\System32\\cmd.exe', vscode.ConfigurationTarget.Global);
+ await pythonSettings.update('condaPath', envPaths.condaExecPath, vscode.ConfigurationTarget.Workspace);
+ await testActivation(envPaths.condaPath);
+ });
 });

src/test/common/terminals/shellDetector.unit.test.ts

@@ -4,14 +4,21 @@
 'use strict';
 
 import { expect } from 'chai';
+import * as sinon from 'sinon';
 import { anything, instance, mock, verify, when } from 'ts-mockito';
+import { ApplicationEnvironment } from '../../../client/common/application/applicationEnvironment';
+import { WorkspaceService } from '../../../client/common/application/workspace';
 import { PlatformService } from '../../../client/common/platform/platformService';
 import { IPlatformService } from '../../../client/common/platform/types';
 import { ShellDetector } from '../../../client/common/terminal/shellDetector';
+import { SettingsShellDetector } from '../../../client/common/terminal/shellDetectors/settingsShellDetector';
+import { TerminalNameShellDetector } from '../../../client/common/terminal/shellDetectors/terminalNameShellDetector';
 import { UserEnvironmentShellDetector } from '../../../client/common/terminal/shellDetectors/userEnvironmentShellDetector';
+import { VSCEnvironmentShellDetector } from '../../../client/common/terminal/shellDetectors/vscEnvironmentShellDetector';
 import { TerminalShellType } from '../../../client/common/terminal/types';
 import { getNamesAndValues } from '../../../client/common/utils/enum';
 import { OSType } from '../../../client/common/utils/platform';
+import { MockProcess } from '../../../test/mocks/process';
 
 // tslint:disable:max-func-body-length no-any
 
@@ -23,11 +30,47 @@ suite('Shell Detector', () => {
  [OSType.Windows]: TerminalShellType.commandPrompt,
  [OSType.Unknown]: TerminalShellType.other
  };
-
+ const sandbox = sinon.createSandbox();
  setup(() => platformService = mock(PlatformService));
+ teardown(() => sandbox.restore());
 
  getNamesAndValues<OSType>(OSType).forEach(os => {
  const testSuffix = `(OS ${os.name})`;
+ test('Test identification of Terminal Shells in order of priority', async () => {
+ const callOrder: string[] = [];
+ const nameDetectorIdentify = sandbox.stub(TerminalNameShellDetector.prototype, 'identify');
+ nameDetectorIdentify.callsFake(() => {
+ callOrder.push('calledFirst');
+ return undefined;
+ });
+ const vscEnvDetectorIdentify = sandbox.stub(VSCEnvironmentShellDetector.prototype, 'identify');
+ vscEnvDetectorIdentify.callsFake(() => {
+ callOrder.push('calledSecond');
+ return undefined;
+ });
+ const userEnvDetectorIdentify = sandbox.stub(UserEnvironmentShellDetector.prototype, 'identify');
+ userEnvDetectorIdentify.callsFake(() => {
+ callOrder.push('calledLast');
+ return undefined;
+ });
+ const settingsDetectorIdentify = sandbox.stub(SettingsShellDetector.prototype, 'identify');
+ settingsDetectorIdentify.callsFake(() => {
+ callOrder.push('calledThird');
+ return undefined;
+ });
+
+ when(platformService.osType).thenReturn(os.value);
+ const nameDetector = new TerminalNameShellDetector();
+ const vscEnvDetector = new VSCEnvironmentShellDetector(instance(mock(ApplicationEnvironment)));
+ const userEnvDetector = new UserEnvironmentShellDetector(mock(MockProcess), instance(platformService));
+ const settingsDetector = new SettingsShellDetector(instance(mock(WorkspaceService)), instance(platformService));
+ const detectors = [settingsDetector, userEnvDetector, nameDetector, vscEnvDetector];
+ const shellDetector = new ShellDetector(instance(platformService), detectors);
+
+ shellDetector.identifyTerminalShell();
+
+ expect(callOrder).to.deep.equal(['calledFirst', 'calledSecond', 'calledThird', 'calledLast']);
+ });
  test(`Use default shell based on OS if there are no shell detectors ${testSuffix}`, () => {
  when(platformService.osType).thenReturn(os.value);
  when(platformService.osType).thenReturn(os.value);

src/test/common/terminals/shellDetectors/shellDetectors.unit.test.ts

@@ -63,10 +63,10 @@ suite('Shell Detectors', () => {
  appEnv = mock(ApplicationEnvironment);
  });
  test('Test Priority of detectors', async () => {
- expect(new TerminalNameShellDetector().priority).to.equal(0);
- expect(new VSCEnvironmentShellDetector(instance(appEnv)).priority).to.equal(1);
+ expect(new TerminalNameShellDetector().priority).to.equal(4);
+ expect(new VSCEnvironmentShellDetector(instance(appEnv)).priority).to.equal(3);
  expect(new SettingsShellDetector(instance(workspaceService), instance(platformService)).priority).to.equal(2);
- expect(new UserEnvironmentShellDetector(instance(currentProcess), instance(platformService)).priority).to.equal(3);
+ expect(new UserEnvironmentShellDetector(instance(currentProcess), instance(platformService)).priority).to.equal(1);
  });
  test('Test identification of Terminal Shells (base class method)', async () => {
  const shellDetector = new TerminalNameShellDetector();