From 4667af1fb58ef3e362479aeac31b4829a464e9a9 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 18 Nov 2019 16:13:49 -0800 Subject: [PATCH 01/15] Added telemetry and tests --- src/client/providers/terminalProvider.ts | 24 +++++++------ src/client/telemetry/constants.ts | 1 + src/client/telemetry/index.ts | 9 +++++ src/test/providers/terminal.unit.test.ts | 44 +++++++++++++++++++++++- 4 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/client/providers/terminalProvider.ts b/src/client/providers/terminalProvider.ts index a5a68c85bc91..bff03b7a2cbe 100644 --- a/src/client/providers/terminalProvider.ts +++ b/src/client/providers/terminalProvider.ts @@ -6,8 +6,9 @@ import { ICommandManager, IDocumentManager, IWorkspaceService } from '../common/ import { Commands } from '../common/constants'; import { ITerminalActivator, ITerminalServiceFactory } from '../common/terminal/types'; import { IConfigurationService } from '../common/types'; +import { swallowExceptions } from '../common/utils/decorators'; import { IServiceContainer } from '../ioc/types'; -import { captureTelemetry } from '../telemetry'; +import { captureTelemetry, sendTelemetryEvent } from '../telemetry'; import { EventName } from '../telemetry/constants'; export class TerminalProvider implements Disposable { @@ -15,18 +16,29 @@ export class TerminalProvider implements Disposable { constructor(private serviceContainer: IServiceContainer) { this.registerCommands(); } + + @swallowExceptions('Failed to initialize terminal provider') public async initialize(currentTerminal: Terminal | undefined) { const configuration = this.serviceContainer.get(IConfigurationService); - const pythonSettings = configuration.getSettings(); + const pythonSettings = configuration.getSettings(this.getActiveResource()); if (pythonSettings.terminal.activateEnvInCurrentTerminal && currentTerminal) { const terminalActivator = this.serviceContainer.get(ITerminalActivator); await terminalActivator.activateEnvironmentInTerminal(currentTerminal, undefined, true); + sendTelemetryEvent(EventName.ACTIVATE_ENV_IN_CURRENT_TERMINAL, undefined, { terminalName: currentTerminal.name }); } } public dispose() { this.disposables.forEach(disposable => disposable.dispose()); } + public getActiveResource(): Uri | undefined { + const documentManager = this.serviceContainer.get(IDocumentManager); + if (documentManager.activeTextEditor && !documentManager.activeTextEditor.document.isUntitled) { + return documentManager.activeTextEditor.document.uri; + } + const workspace = this.serviceContainer.get(IWorkspaceService); + return Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0 ? workspace.workspaceFolders[0].uri : undefined; + } private registerCommands() { const commandManager = this.serviceContainer.get(ICommandManager); const disposable = commandManager.registerCommand(Commands.Create_Terminal, this.onCreateTerminal, this); @@ -39,12 +51,4 @@ export class TerminalProvider implements Disposable { const activeResource = this.getActiveResource(); await terminalService.createTerminalService(activeResource, 'Python').show(false); } - private getActiveResource(): Uri | undefined { - const documentManager = this.serviceContainer.get(IDocumentManager); - if (documentManager.activeTextEditor && !documentManager.activeTextEditor.document.isUntitled) { - return documentManager.activeTextEditor.document.uri; - } - const workspace = this.serviceContainer.get(IWorkspaceService); - return Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0 ? workspace.workspaceFolders[0].uri : undefined; - } } diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 63cc830fa30f..bed79e1e3334 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -76,6 +76,7 @@ export enum EventName { EXTENSION_SURVEY_PROMPT = 'EXTENSION_SURVEY_PROMPT', TERMINAL_CREATE = 'TERMINAL.CREATE', + ACTIVATE_ENV_IN_CURRENT_TERMINAL = 'ACTIVATE_ENV_IN_CURRENT_TERMINAL', PYTHON_LANGUAGE_SERVER_LIST_BLOB_STORE_PACKAGES = 'PYTHON_LANGUAGE_SERVER.LIST_BLOB_PACKAGES', DIAGNOSTICS_ACTION = 'DIAGNOSTICS.ACTION', DIAGNOSTICS_MESSAGE = 'DIAGNOSTICS.MESSAGE', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 05aacc2953d9..ed249a04c9c5 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1229,6 +1229,15 @@ export interface IEventNamePropertyMapping { */ failed: boolean; }; + /** + * Telemetry event sent when `python.terminal.activateEnvInCurrentTerminal` setting is set to `true` and an active terminal is present + */ + [EventName.ACTIVATE_ENV_IN_CURRENT_TERMINAL]: { + /** + * The name of terminal being activated + */ + terminalName?: string; + }; /** * Telemetry event sent with details when a terminal is created */ diff --git a/src/test/providers/terminal.unit.test.ts b/src/test/providers/terminal.unit.test.ts index 1d24f4c30879..257c4f730091 100644 --- a/src/test/providers/terminal.unit.test.ts +++ b/src/test/providers/terminal.unit.test.ts @@ -1,7 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import * as assert from 'assert'; import { expect } from 'chai'; +import * as sinon from 'sinon'; import * as TypeMoq from 'typemoq'; import { Disposable, Terminal, TextDocument, TextEditor, Uri, WorkspaceFolder } from 'vscode'; import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; @@ -158,12 +160,15 @@ suite('Terminal Provider', () => { let configService: TypeMoq.IMock; let terminalActivator: TypeMoq.IMock; let terminal: TypeMoq.IMock; + // tslint:disable-next-line:no-any + let getActiveResource: sinon.SinonStub; + const resource = Uri.parse('a'); setup(() => { + getActiveResource = sinon.stub(TerminalProvider.prototype, 'getActiveResource'); configService = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IConfigurationService))).returns(() => configService.object); pythonSettings = TypeMoq.Mock.ofType(); - configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); terminalSettings = TypeMoq.Mock.ofType(); pythonSettings.setup(s => s.terminal).returns(() => terminalSettings.object); @@ -172,33 +177,70 @@ suite('Terminal Provider', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalActivator))).returns(() => terminalActivator.object); terminal = TypeMoq.Mock.ofType(); + getActiveResource.callsFake(() => resource); + }); + + teardown(() => { + sinon.restore(); }); test('If terminal.activateCurrentTerminal setting is set, provided terminal should be activated', async () => { terminalSettings.setup(t => t.activateEnvInCurrentTerminal).returns(() => true); + configService + .setup(c => c.getSettings(resource)) + .returns(() => pythonSettings.object) + .verifiable(TypeMoq.Times.once()); terminalProvider = new TerminalProvider(serviceContainer.object); await terminalProvider.initialize(terminal.object); terminalActivator.verify(a => a.activateEnvironmentInTerminal(terminal.object, undefined, true), TypeMoq.Times.once()); + assert.ok(getActiveResource.calledOnce); + configService.verifyAll(); }); test('If terminal.activateCurrentTerminal setting is not set, provided terminal should not be activated', async () => { terminalSettings.setup(t => t.activateEnvInCurrentTerminal).returns(() => false); + configService + .setup(c => c.getSettings(resource)) + .returns(() => pythonSettings.object) + .verifiable(TypeMoq.Times.once()); terminalProvider = new TerminalProvider(serviceContainer.object); await terminalProvider.initialize(terminal.object); terminalActivator.verify(a => a.activateEnvironmentInTerminal(TypeMoq.It.isAny(), undefined, true), TypeMoq.Times.never()); + assert.ok(getActiveResource.calledOnce); + configService.verifyAll(); }); test('terminal.activateCurrentTerminal setting is set but provided terminal is undefined', async () => { terminalSettings.setup(t => t.activateEnvInCurrentTerminal).returns(() => true); + configService + .setup(c => c.getSettings(resource)) + .returns(() => pythonSettings.object) + .verifiable(TypeMoq.Times.once()); terminalProvider = new TerminalProvider(serviceContainer.object); await terminalProvider.initialize(undefined); terminalActivator.verify(a => a.activateEnvironmentInTerminal(TypeMoq.It.isAny(), undefined, true), TypeMoq.Times.never()); + assert.ok(getActiveResource.calledOnce); + configService.verifyAll(); + }); + + test('Exceptions are swallowed if initializing terminal provider fails', async () => { + terminalSettings.setup(t => t.activateEnvInCurrentTerminal).returns(() => true); + configService + .setup(c => c.getSettings(resource)) + .throws(new Error('Kaboom')); + + terminalProvider = new TerminalProvider(serviceContainer.object); + try { + await terminalProvider.initialize(undefined); + } catch (ex) { + assert(false, `No error should be thrown, ${ex}`); + } }); }); }); From 30fd058bdf0d12f9435a137c2ccc66c33a207498 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 18 Nov 2019 16:15:22 -0800 Subject: [PATCH 02/15] News entry --- news/1 Enhancements/8004.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/1 Enhancements/8004.md diff --git a/news/1 Enhancements/8004.md b/news/1 Enhancements/8004.md new file mode 100644 index 000000000000..2ab47b3cbf5c --- /dev/null +++ b/news/1 Enhancements/8004.md @@ -0,0 +1 @@ +Add telemetry for usage of activateEnvInCurrentTerminal setting From 6bb8fc1c40f7acc8b6c50aec7f63c8fdfba8e559 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 19 Nov 2019 10:04:29 -0800 Subject: [PATCH 03/15] Apply suggestions from code review Co-Authored-By: Eric Snow --- news/1 Enhancements/8004.md | 2 +- src/client/telemetry/index.ts | 5 +++-- src/test/providers/terminal.unit.test.ts | 4 +++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/news/1 Enhancements/8004.md b/news/1 Enhancements/8004.md index 2ab47b3cbf5c..ae88a7d62772 100644 --- a/news/1 Enhancements/8004.md +++ b/news/1 Enhancements/8004.md @@ -1 +1 @@ -Add telemetry for usage of activateEnvInCurrentTerminal setting +Add telemetry for usage of activateEnvInCurrentTerminal setting. diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index ed249a04c9c5..979ad21dcac8 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1230,11 +1230,12 @@ export interface IEventNamePropertyMapping { failed: boolean; }; /** - * Telemetry event sent when `python.terminal.activateEnvInCurrentTerminal` setting is set to `true` and an active terminal is present + * Telemetry event sent when the extension is activated, if an active terminal is present and + * the `python.terminal.activateEnvInCurrentTerminal` setting is set to `true`. */ [EventName.ACTIVATE_ENV_IN_CURRENT_TERMINAL]: { /** - * The name of terminal being activated + * The name of terminal in which the Python environment was activated. */ terminalName?: string; }; diff --git a/src/test/providers/terminal.unit.test.ts b/src/test/providers/terminal.unit.test.ts index 257c4f730091..8fda182df022 100644 --- a/src/test/providers/terminal.unit.test.ts +++ b/src/test/providers/terminal.unit.test.ts @@ -230,7 +230,9 @@ suite('Terminal Provider', () => { }); test('Exceptions are swallowed if initializing terminal provider fails', async () => { - terminalSettings.setup(t => t.activateEnvInCurrentTerminal).returns(() => true); + terminalSettings + .setup(t => t.activateEnvInCurrentTerminal) + .returns(() => true); configService .setup(c => c.getSettings(resource)) .throws(new Error('Kaboom')); From d3a448f8b2bb141a17c94f9973f85db50ed97ef1 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 19 Nov 2019 11:54:44 -0800 Subject: [PATCH 04/15] Modified telemetry + code reviews --- src/client/activation/activeResource.ts | 27 +++++++++++++++ src/client/activation/serviceRegistry.ts | 4 ++- src/client/activation/types.ts | 5 +++ src/client/providers/terminalProvider.ts | 29 +++++++--------- src/client/telemetry/index.ts | 6 ++-- .../activation/serviceRegistry.unit.test.ts | 3 ++ src/test/providers/terminal.unit.test.ts | 34 ++++++++++++------- 7 files changed, 76 insertions(+), 32 deletions(-) create mode 100644 src/client/activation/activeResource.ts diff --git a/src/client/activation/activeResource.ts b/src/client/activation/activeResource.ts new file mode 100644 index 000000000000..c2d6c575e28e --- /dev/null +++ b/src/client/activation/activeResource.ts @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { inject, injectable } from 'inversify'; +import { workspace } from 'vscode'; +import { IDocumentManager, IWorkspaceService } from '../common/application/types'; +import { Resource } from '../common/types'; +import { IActiveResourceService } from './types'; + +@injectable() +export class ActiveResourceService implements IActiveResourceService { + constructor( + @inject(IDocumentManager) private readonly documentManager: IDocumentManager, + @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService + ) { } + + public getActiveResource(): Resource { + if (this.documentManager.activeTextEditor && !this.documentManager.activeTextEditor.document.isUntitled) { + return this.documentManager.activeTextEditor.document.uri; + } + return Array.isArray(this.workspaceService.workspaceFolders) && workspace.workspaceFolders!.length > 0 + ? workspace.workspaceFolders![0].uri + : undefined; + } +} diff --git a/src/client/activation/serviceRegistry.ts b/src/client/activation/serviceRegistry.ts index 4b8daee8dc9b..e6e0f0107c5f 100644 --- a/src/client/activation/serviceRegistry.ts +++ b/src/client/activation/serviceRegistry.ts @@ -13,6 +13,7 @@ import { ProposeLanguageServerBanner } from '../languageServices/proposeLanguage import { AATesting } from './aaTesting'; import { ExtensionActivationManager } from './activationManager'; import { LanguageServerExtensionActivationService } from './activationService'; +import { ActiveResourceService } from './activeResource'; import { ExtensionSurveyPrompt } from './extensionSurvey'; import { JediExtensionActivator } from './jedi'; import { LanguageServerExtensionActivator } from './languageServer/activator'; @@ -29,7 +30,7 @@ import { LanguageServerPackageService } from './languageServer/languageServerPac import { LanguageServerManager } from './languageServer/manager'; import { LanguageServerOutputChannel } from './languageServer/outputChannel'; import { PlatformData } from './languageServer/platformData'; -import { IDownloadChannelRule, IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService, ILanguageClientFactory, ILanguageServer, ILanguageServerActivator, ILanguageServerAnalysisOptions, ILanguageServerCompatibilityService as ILanagueServerCompatibilityService, ILanguageServerDownloader, ILanguageServerExtension, ILanguageServerFolderService, ILanguageServerManager, ILanguageServerOutputChannel, ILanguageServerPackageService, IPlatformData, LanguageClientFactory, LanguageServerActivator } from './types'; +import { IActiveResourceService, IDownloadChannelRule, IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService, ILanguageClientFactory, ILanguageServer, ILanguageServerActivator, ILanguageServerAnalysisOptions, ILanguageServerCompatibilityService as ILanagueServerCompatibilityService, ILanguageServerDownloader, ILanguageServerExtension, ILanguageServerFolderService, ILanguageServerManager, ILanguageServerOutputChannel, ILanguageServerPackageService, IPlatformData, LanguageClientFactory, LanguageServerActivator } from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IExtensionActivationService, LanguageServerExtensionActivationService); @@ -60,5 +61,6 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.add(ILanguageServerManager, LanguageServerManager); serviceManager.addSingleton(ILanguageServerOutputChannel, LanguageServerOutputChannel); serviceManager.addSingleton(IExtensionSingleActivationService, ExtensionSurveyPrompt); + serviceManager.addSingleton(IActiveResourceService, ActiveResourceService); serviceManager.addSingleton(IExtensionSingleActivationService, AATesting); } diff --git a/src/client/activation/types.ts b/src/client/activation/types.ts index 078d98760b1a..ea050863429e 100644 --- a/src/client/activation/types.ts +++ b/src/client/activation/types.ts @@ -169,3 +169,8 @@ export const IExtensionSingleActivationService = Symbol('IExtensionSingleActivat export interface IExtensionSingleActivationService { activate(): Promise; } + +export const IActiveResourceService = Symbol('IActiveResourceService'); +export interface IActiveResourceService { + getActiveResource(): Resource; +} diff --git a/src/client/providers/terminalProvider.ts b/src/client/providers/terminalProvider.ts index bff03b7a2cbe..325416fc0e16 100644 --- a/src/client/providers/terminalProvider.ts +++ b/src/client/providers/terminalProvider.ts @@ -1,8 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { Disposable, Terminal, Uri } from 'vscode'; -import { ICommandManager, IDocumentManager, IWorkspaceService } from '../common/application/types'; +import { Disposable, Terminal } from 'vscode'; +import { IActiveResourceService } from '../activation/types'; +import { ICommandManager } from '../common/application/types'; import { Commands } from '../common/constants'; import { ITerminalActivator, ITerminalServiceFactory } from '../common/terminal/types'; import { IConfigurationService } from '../common/types'; @@ -13,32 +14,28 @@ import { EventName } from '../telemetry/constants'; export class TerminalProvider implements Disposable { private disposables: Disposable[] = []; + private activeResourceService: IActiveResourceService; constructor(private serviceContainer: IServiceContainer) { this.registerCommands(); + this.activeResourceService = this.serviceContainer.get(IActiveResourceService); } @swallowExceptions('Failed to initialize terminal provider') public async initialize(currentTerminal: Terminal | undefined) { const configuration = this.serviceContainer.get(IConfigurationService); - const pythonSettings = configuration.getSettings(this.getActiveResource()); + const pythonSettings = configuration.getSettings(this.activeResourceService.getActiveResource()); - if (pythonSettings.terminal.activateEnvInCurrentTerminal && currentTerminal) { - const terminalActivator = this.serviceContainer.get(ITerminalActivator); - await terminalActivator.activateEnvironmentInTerminal(currentTerminal, undefined, true); - sendTelemetryEvent(EventName.ACTIVATE_ENV_IN_CURRENT_TERMINAL, undefined, { terminalName: currentTerminal.name }); + if (pythonSettings.terminal.activateEnvInCurrentTerminal) { + if (currentTerminal) { + const terminalActivator = this.serviceContainer.get(ITerminalActivator); + await terminalActivator.activateEnvironmentInTerminal(currentTerminal, undefined, true); + } + sendTelemetryEvent(EventName.ACTIVATE_ENV_IN_CURRENT_TERMINAL, undefined, { isTerminalVisible: currentTerminal ? true : false }); } } public dispose() { this.disposables.forEach(disposable => disposable.dispose()); } - public getActiveResource(): Uri | undefined { - const documentManager = this.serviceContainer.get(IDocumentManager); - if (documentManager.activeTextEditor && !documentManager.activeTextEditor.document.isUntitled) { - return documentManager.activeTextEditor.document.uri; - } - const workspace = this.serviceContainer.get(IWorkspaceService); - return Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0 ? workspace.workspaceFolders[0].uri : undefined; - } private registerCommands() { const commandManager = this.serviceContainer.get(ICommandManager); const disposable = commandManager.registerCommand(Commands.Create_Terminal, this.onCreateTerminal, this); @@ -48,7 +45,7 @@ export class TerminalProvider implements Disposable { @captureTelemetry(EventName.TERMINAL_CREATE, { triggeredBy: 'commandpalette' }) private async onCreateTerminal() { const terminalService = this.serviceContainer.get(ITerminalServiceFactory); - const activeResource = this.getActiveResource(); + const activeResource = this.activeResourceService.getActiveResource(); await terminalService.createTerminalService(activeResource, 'Python').show(false); } } diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 979ad21dcac8..1ad33338e206 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1230,14 +1230,14 @@ export interface IEventNamePropertyMapping { failed: boolean; }; /** - * Telemetry event sent when the extension is activated, if an active terminal is present and + * Telemetry event sent when the extension is activated, if an active terminal is present and * the `python.terminal.activateEnvInCurrentTerminal` setting is set to `true`. */ [EventName.ACTIVATE_ENV_IN_CURRENT_TERMINAL]: { /** - * The name of terminal in which the Python environment was activated. + * Carries boolean `true` if an active terminal is present (terminal is visible), `false` otherwise */ - terminalName?: string; + isTerminalVisible?: boolean; }; /** * Telemetry event sent with details when a terminal is created diff --git a/src/test/activation/serviceRegistry.unit.test.ts b/src/test/activation/serviceRegistry.unit.test.ts index ae1c95c2e733..6efcb9716305 100644 --- a/src/test/activation/serviceRegistry.unit.test.ts +++ b/src/test/activation/serviceRegistry.unit.test.ts @@ -8,6 +8,7 @@ import { instance, mock, verify } from 'ts-mockito'; import { AATesting } from '../../client/activation/aaTesting'; import { ExtensionActivationManager } from '../../client/activation/activationManager'; import { LanguageServerExtensionActivationService } from '../../client/activation/activationService'; +import { ActiveResourceService } from '../../client/activation/activeResource'; import { ExtensionSurveyPrompt } from '../../client/activation/extensionSurvey'; import { JediExtensionActivator } from '../../client/activation/jedi'; import { LanguageServerExtensionActivator } from '../../client/activation/languageServer/activator'; @@ -31,6 +32,7 @@ import { LanguageServerOutputChannel } from '../../client/activation/languageSer import { PlatformData } from '../../client/activation/languageServer/platformData'; import { registerTypes } from '../../client/activation/serviceRegistry'; import { + IActiveResourceService, IDownloadChannelRule, IExtensionActivationManager, IExtensionActivationService, @@ -97,6 +99,7 @@ suite('Unit Tests - Activation Service Registry', () => { verify(serviceManager.add(ILanguageServerManager, LanguageServerManager)).once(); verify(serviceManager.addSingleton(IExtensionSingleActivationService, AATesting)).once(); verify(serviceManager.addSingleton(ILanguageServerOutputChannel, LanguageServerOutputChannel)).once(); + verify(serviceManager.addSingleton(IActiveResourceService, ActiveResourceService)).once(); verify(serviceManager.addSingleton(IExtensionSingleActivationService, ExtensionSurveyPrompt)).once(); }); }); diff --git a/src/test/providers/terminal.unit.test.ts b/src/test/providers/terminal.unit.test.ts index 8fda182df022..d8682e2715b6 100644 --- a/src/test/providers/terminal.unit.test.ts +++ b/src/test/providers/terminal.unit.test.ts @@ -3,9 +3,9 @@ import * as assert from 'assert'; import { expect } from 'chai'; -import * as sinon from 'sinon'; import * as TypeMoq from 'typemoq'; import { Disposable, Terminal, TextDocument, TextEditor, Uri, WorkspaceFolder } from 'vscode'; +import { IActiveResourceService } from '../../client/activation/types'; import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; import { TerminalService } from '../../client/common/terminal/service'; @@ -153,22 +153,22 @@ suite('Terminal Provider', () => { terminalService.verify(t => t.show(false), TypeMoq.Times.once()); }); + // tslint:disable-next-line: max-func-body-length suite('terminal.activateCurrentTerminal setting', () => { let pythonSettings: TypeMoq.IMock; let terminalSettings: TypeMoq.IMock; let configService: TypeMoq.IMock; + let activeResourceService: TypeMoq.IMock; let terminalActivator: TypeMoq.IMock; let terminal: TypeMoq.IMock; - // tslint:disable-next-line:no-any - let getActiveResource: sinon.SinonStub; const resource = Uri.parse('a'); setup(() => { - getActiveResource = sinon.stub(TerminalProvider.prototype, 'getActiveResource'); configService = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IConfigurationService))).returns(() => configService.object); pythonSettings = TypeMoq.Mock.ofType(); + activeResourceService = TypeMoq.Mock.ofType(); terminalSettings = TypeMoq.Mock.ofType(); pythonSettings.setup(s => s.terminal).returns(() => terminalSettings.object); @@ -177,11 +177,6 @@ suite('Terminal Provider', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalActivator))).returns(() => terminalActivator.object); terminal = TypeMoq.Mock.ofType(); - getActiveResource.callsFake(() => resource); - }); - - teardown(() => { - sinon.restore(); }); test('If terminal.activateCurrentTerminal setting is set, provided terminal should be activated', async () => { @@ -190,13 +185,17 @@ suite('Terminal Provider', () => { .setup(c => c.getSettings(resource)) .returns(() => pythonSettings.object) .verifiable(TypeMoq.Times.once()); + activeResourceService + .setup(a => a.getActiveResource()) + .returns(() => resource) + .verifiable(TypeMoq.Times.once()); terminalProvider = new TerminalProvider(serviceContainer.object); await terminalProvider.initialize(terminal.object); terminalActivator.verify(a => a.activateEnvironmentInTerminal(terminal.object, undefined, true), TypeMoq.Times.once()); - assert.ok(getActiveResource.calledOnce); configService.verifyAll(); + activeResourceService.verifyAll(); }); test('If terminal.activateCurrentTerminal setting is not set, provided terminal should not be activated', async () => { @@ -205,12 +204,16 @@ suite('Terminal Provider', () => { .setup(c => c.getSettings(resource)) .returns(() => pythonSettings.object) .verifiable(TypeMoq.Times.once()); + activeResourceService + .setup(a => a.getActiveResource()) + .returns(() => resource) + .verifiable(TypeMoq.Times.once()); terminalProvider = new TerminalProvider(serviceContainer.object); await terminalProvider.initialize(terminal.object); terminalActivator.verify(a => a.activateEnvironmentInTerminal(TypeMoq.It.isAny(), undefined, true), TypeMoq.Times.never()); - assert.ok(getActiveResource.calledOnce); + activeResourceService.verifyAll(); configService.verifyAll(); }); @@ -220,12 +223,16 @@ suite('Terminal Provider', () => { .setup(c => c.getSettings(resource)) .returns(() => pythonSettings.object) .verifiable(TypeMoq.Times.once()); + activeResourceService + .setup(a => a.getActiveResource()) + .returns(() => resource) + .verifiable(TypeMoq.Times.once()); terminalProvider = new TerminalProvider(serviceContainer.object); await terminalProvider.initialize(undefined); terminalActivator.verify(a => a.activateEnvironmentInTerminal(TypeMoq.It.isAny(), undefined, true), TypeMoq.Times.never()); - assert.ok(getActiveResource.calledOnce); + activeResourceService.verifyAll(); configService.verifyAll(); }); @@ -236,6 +243,9 @@ suite('Terminal Provider', () => { configService .setup(c => c.getSettings(resource)) .throws(new Error('Kaboom')); + activeResourceService + .setup(a => a.getActiveResource()) + .returns(() => resource); terminalProvider = new TerminalProvider(serviceContainer.object); try { From 8e41eea012159188f436db9149b585bce7573646 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 19 Nov 2019 13:05:37 -0800 Subject: [PATCH 05/15] Oops --- src/client/activation/activeResource.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/client/activation/activeResource.ts b/src/client/activation/activeResource.ts index c2d6c575e28e..976cea375881 100644 --- a/src/client/activation/activeResource.ts +++ b/src/client/activation/activeResource.ts @@ -4,7 +4,6 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { workspace } from 'vscode'; import { IDocumentManager, IWorkspaceService } from '../common/application/types'; import { Resource } from '../common/types'; import { IActiveResourceService } from './types'; @@ -20,8 +19,8 @@ export class ActiveResourceService implements IActiveResourceService { if (this.documentManager.activeTextEditor && !this.documentManager.activeTextEditor.document.isUntitled) { return this.documentManager.activeTextEditor.document.uri; } - return Array.isArray(this.workspaceService.workspaceFolders) && workspace.workspaceFolders!.length > 0 - ? workspace.workspaceFolders![0].uri + return Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders!.length > 0 + ? this.workspaceService.workspaceFolders![0].uri : undefined; } } From bc3df8aaa43a365b98da3643c124b796a2412738 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 19 Nov 2019 14:23:36 -0800 Subject: [PATCH 06/15] Added tests --- src/client/activation/activeResource.ts | 4 +- .../activation/activeResource.unit.test.ts | 90 +++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 src/test/activation/activeResource.unit.test.ts diff --git a/src/client/activation/activeResource.ts b/src/client/activation/activeResource.ts index 976cea375881..8c7c5345761d 100644 --- a/src/client/activation/activeResource.ts +++ b/src/client/activation/activeResource.ts @@ -19,8 +19,8 @@ export class ActiveResourceService implements IActiveResourceService { if (this.documentManager.activeTextEditor && !this.documentManager.activeTextEditor.document.isUntitled) { return this.documentManager.activeTextEditor.document.uri; } - return Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders!.length > 0 - ? this.workspaceService.workspaceFolders![0].uri + return Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders.length > 0 + ? this.workspaceService.workspaceFolders[0].uri : undefined; } } diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts new file mode 100644 index 000000000000..8338dc2f25e4 --- /dev/null +++ b/src/test/activation/activeResource.unit.test.ts @@ -0,0 +1,90 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +// tslint:disable: no-any + +import { assert } from 'chai'; +import { instance, mock, verify, when } from 'ts-mockito'; +import { Uri } from 'vscode'; +import { ActiveResourceService } from '../../client/activation/activeResource'; +import { DocumentManager } from '../../client/common/application/documentManager'; +import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import { WorkspaceService } from '../../client/common/application/workspace'; + +// tslint:disable-next-line: max-func-body-length +suite('Active resource service', () => { + let documentManager: IDocumentManager; + let workspaceService: IWorkspaceService; + let activeResourceService: ActiveResourceService; + setup(() => { + documentManager = mock(DocumentManager); + workspaceService = mock(WorkspaceService); + activeResourceService = new ActiveResourceService(instance(documentManager), instance(workspaceService)); + }); + + test('Return document uri if a saved document is currently opened', async () => { + const activeTextEditor = { + document: { + isUntitled: false, + uri: Uri.parse('a') + } + }; + when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); + when(workspaceService.workspaceFolders).thenReturn([]); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, activeTextEditor.document.uri); + verify(documentManager.activeTextEditor).atLeast(1); + verify(workspaceService.workspaceFolders).never(); + }); + + test('Don\'t return document uri if a unsaved document is currently opened', async () => { + const activeTextEditor = { + document: { + isUntitled: true, + uri: Uri.parse('a') + } + }; + when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); + when(workspaceService.workspaceFolders).thenReturn([]); + const activeResource = activeResourceService.getActiveResource(); + assert.notDeepEqual(activeResource, activeTextEditor.document.uri); + verify(documentManager.activeTextEditor).atLeast(1); + verify(workspaceService.workspaceFolders).atLeast(1); + }); + + test('If no document is currently opened & the workspace opened contains workspace folders, return the uri of the first workspace folder', async () => { + const workspaceFolders = [ + { + uri: Uri.parse('a') + }, + { + uri: Uri.parse('b') + }]; + when(documentManager.activeTextEditor).thenReturn(undefined); + when(workspaceService.workspaceFolders).thenReturn(workspaceFolders as any); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, workspaceFolders[0].uri); + verify(documentManager.activeTextEditor).atLeast(1); + verify(workspaceService.workspaceFolders).atLeast(1); + }); + + test('If no document is currently opened & no folder is opened, return undefined', async () => { + when(documentManager.activeTextEditor).thenReturn(undefined); + when(workspaceService.workspaceFolders).thenReturn(undefined); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, undefined); + verify(documentManager.activeTextEditor).atLeast(1); + verify(workspaceService.workspaceFolders).atLeast(1); + }); + + test('If no document is currently opened & workspace contains no workspace folders, return undefined', async () => { + when(documentManager.activeTextEditor).thenReturn(undefined); + when(workspaceService.workspaceFolders).thenReturn([]); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, undefined); + verify(documentManager.activeTextEditor).atLeast(1); + verify(workspaceService.workspaceFolders).atLeast(1); + }); +}); From e8304f36e0a26e051a34f0ce96a02e09dbab240a Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 19 Nov 2019 15:42:19 -0800 Subject: [PATCH 07/15] Move getActiveResource to a common place --- src/client/activation/activationManager.ts | 17 +-- src/client/providers/replProvider.ts | 19 ++-- .../activation/activationManager.unit.test.ts | 8 +- src/test/providers/repl.unit.test.ts | 82 +++----------- src/test/providers/terminal.unit.test.ts | 101 +++--------------- 5 files changed, 42 insertions(+), 185 deletions(-) diff --git a/src/client/activation/activationManager.ts b/src/client/activation/activationManager.ts index 6c2470918647..bf68dd3be956 100644 --- a/src/client/activation/activationManager.ts +++ b/src/client/activation/activationManager.ts @@ -4,7 +4,7 @@ 'use strict'; import { inject, injectable, multiInject } from 'inversify'; -import { TextDocument, workspace } from 'vscode'; +import { TextDocument } from 'vscode'; import { IApplicationDiagnostics } from '../application/types'; import { IDocumentManager, IWorkspaceService } from '../common/application/types'; import { PYTHON_LANGUAGE } from '../common/constants'; @@ -12,7 +12,7 @@ import { traceDecorators } from '../common/logger'; import { IDisposable, Resource } from '../common/types'; import { IInterpreterAutoSelectionService } from '../interpreter/autoSelection/types'; import { IInterpreterService } from '../interpreter/contracts'; -import { IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService } from './types'; +import { IActiveResourceService, IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService } from './types'; @injectable() export class ExtensionActivationManager implements IExtensionActivationManager { @@ -26,7 +26,8 @@ export class ExtensionActivationManager implements IExtensionActivationManager { @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, @inject(IInterpreterAutoSelectionService) private readonly autoSelection: IInterpreterAutoSelectionService, @inject(IApplicationDiagnostics) private readonly appDiagnostics: IApplicationDiagnostics, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService + @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, + @inject(IActiveResourceService) private readonly activeResourceService: IActiveResourceService ) { } public dispose() { @@ -44,7 +45,7 @@ export class ExtensionActivationManager implements IExtensionActivationManager { // Activate all activation services together. await Promise.all([ Promise.all(this.singleActivationServices.map(item => item.activate())), - this.activateWorkspace(this.getActiveResource()) + this.activateWorkspace(this.activeResourceService.getActiveResource()) ]); await this.autoSelection.autoSelectInterpreter(undefined); } @@ -114,12 +115,4 @@ export class ExtensionActivationManager implements IExtensionActivationManager { protected getWorkspaceKey(resource: Resource) { return this.workspaceService.getWorkspaceFolderIdentifier(resource, ''); } - private getActiveResource(): Resource { - if (this.documentManager.activeTextEditor && !this.documentManager.activeTextEditor.document.isUntitled) { - return this.documentManager.activeTextEditor.document.uri; - } - return Array.isArray(this.workspaceService.workspaceFolders) && workspace.workspaceFolders!.length > 0 - ? workspace.workspaceFolders![0].uri - : undefined; - } } diff --git a/src/client/providers/replProvider.ts b/src/client/providers/replProvider.ts index b31d274b9015..82233e9f73c0 100644 --- a/src/client/providers/replProvider.ts +++ b/src/client/providers/replProvider.ts @@ -1,5 +1,6 @@ -import { Disposable, Uri } from 'vscode'; -import { ICommandManager, IDocumentManager, IWorkspaceService } from '../common/application/types'; +import { Disposable } from 'vscode'; +import { IActiveResourceService } from '../activation/types'; +import { ICommandManager } from '../common/application/types'; import { Commands } from '../common/constants'; import { IServiceContainer } from '../ioc/types'; import { captureTelemetry } from '../telemetry'; @@ -8,7 +9,9 @@ import { ICodeExecutionService } from '../terminals/types'; export class ReplProvider implements Disposable { private readonly disposables: Disposable[] = []; + private activeResourceService: IActiveResourceService; constructor(private serviceContainer: IServiceContainer) { + this.activeResourceService = this.serviceContainer.get(IActiveResourceService); this.registerCommand(); } public dispose() { @@ -21,18 +24,8 @@ export class ReplProvider implements Disposable { } @captureTelemetry(EventName.REPL) private async commandHandler() { - const resource = this.getActiveResourceUri(); + const resource = this.activeResourceService.getActiveResource(); const replProvider = this.serviceContainer.get(ICodeExecutionService, 'repl'); await replProvider.initializeRepl(resource); } - private getActiveResourceUri(): Uri | undefined { - const documentManager = this.serviceContainer.get(IDocumentManager); - if (documentManager.activeTextEditor && !documentManager.activeTextEditor!.document.isUntitled) { - return documentManager.activeTextEditor!.document.uri; - } - const workspace = this.serviceContainer.get(IWorkspaceService); - if (Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0) { - return workspace.workspaceFolders[0].uri; - } - } } diff --git a/src/test/activation/activationManager.unit.test.ts b/src/test/activation/activationManager.unit.test.ts index 2d67ef90ff09..9a3ab8391cbf 100644 --- a/src/test/activation/activationManager.unit.test.ts +++ b/src/test/activation/activationManager.unit.test.ts @@ -9,7 +9,8 @@ import * as typemoq from 'typemoq'; import { TextDocument, Uri } from 'vscode'; import { ExtensionActivationManager } from '../../client/activation/activationManager'; import { LanguageServerExtensionActivationService } from '../../client/activation/activationService'; -import { IExtensionActivationService } from '../../client/activation/types'; +import { ActiveResourceService } from '../../client/activation/activeResource'; +import { IActiveResourceService, IExtensionActivationService } from '../../client/activation/types'; import { IApplicationDiagnostics } from '../../client/application/types'; import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import { WorkspaceService } from '../../client/common/application/workspace'; @@ -41,11 +42,13 @@ suite('Activation - ActivationManager', () => { let appDiagnostics: typemoq.IMock; let autoSelection: typemoq.IMock; let interpreterService: IInterpreterService; + let activeResourceService: IActiveResourceService; let documentManager: typemoq.IMock; let activationService1: IExtensionActivationService; let activationService2: IExtensionActivationService; setup(() => { workspaceService = mock(WorkspaceService); + activeResourceService = mock(ActiveResourceService); appDiagnostics = typemoq.Mock.ofType(); autoSelection = typemoq.Mock.ofType(); interpreterService = mock(InterpreterService); @@ -58,7 +61,8 @@ suite('Activation - ActivationManager', () => { instance(interpreterService), autoSelection.object, appDiagnostics.object, - instance(workspaceService) + instance(workspaceService), + instance(activeResourceService) ); }); test('Initialize will add event handlers and will dispose them when running dispose', async () => { diff --git a/src/test/providers/repl.unit.test.ts b/src/test/providers/repl.unit.test.ts index 784a07868d71..330b94d6e43d 100644 --- a/src/test/providers/repl.unit.test.ts +++ b/src/test/providers/repl.unit.test.ts @@ -3,7 +3,8 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; -import { Disposable, TextDocument, TextEditor, Uri, WorkspaceFolder } from 'vscode'; +import { Disposable, Uri } from 'vscode'; +import { IActiveResourceService } from '../../client/activation/types'; import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; import { IServiceContainer } from '../../client/ioc/types'; @@ -17,6 +18,7 @@ suite('REPL Provider', () => { let workspace: TypeMoq.IMock; let codeExecutionService: TypeMoq.IMock; let documentManager: TypeMoq.IMock; + let activeResourceService: TypeMoq.IMock; let replProvider: ReplProvider; setup(() => { serviceContainer = TypeMoq.Mock.ofType(); @@ -24,10 +26,12 @@ suite('REPL Provider', () => { workspace = TypeMoq.Mock.ofType(); codeExecutionService = TypeMoq.Mock.ofType(); documentManager = TypeMoq.Mock.ofType(); + activeResourceService = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(ICommandManager)).returns(() => commandManager.object); serviceContainer.setup(c => c.get(IWorkspaceService)).returns(() => workspace.object); serviceContainer.setup(c => c.get(ICodeExecutionService, TypeMoq.It.isValue('repl'))).returns(() => codeExecutionService.object); serviceContainer.setup(c => c.get(IDocumentManager)).returns(() => documentManager.object); + serviceContainer.setup(c => c.get(IActiveResourceService)).returns(() => activeResourceService.object); }); teardown(() => { try { @@ -51,86 +55,24 @@ suite('REPL Provider', () => { disposable.verify(d => d.dispose(), TypeMoq.Times.once()); }); - test('Ensure resource is \'undefined\' if there\s no active document nor a workspace', () => { + test('Ensure execution is carried smoothly in the handler if there are no errors', () => { + const resource = Uri.parse('a'); const disposable = TypeMoq.Mock.ofType(); let commandHandler: undefined | (() => void); commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Start_REPL), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { commandHandler = callback; return disposable.object; }); - documentManager.setup(d => d.activeTextEditor).returns(() => undefined); + activeResourceService + .setup(a => a.getActiveResource()) + .returns(() => resource) + .verifiable(TypeMoq.Times.once()); replProvider = new ReplProvider(serviceContainer.object); expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); commandHandler!.call(replProvider); serviceContainer.verify(c => c.get(TypeMoq.It.isValue(ICodeExecutionService), TypeMoq.It.isValue('repl')), TypeMoq.Times.once()); - codeExecutionService.verify(c => c.initializeRepl(TypeMoq.It.isValue(undefined)), TypeMoq.Times.once()); - }); - - test('Ensure resource is uri of the active document', () => { - const disposable = TypeMoq.Mock.ofType(); - let commandHandler: undefined | (() => void); - commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Start_REPL), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { - commandHandler = callback; - return disposable.object; - }); - const documentUri = Uri.file('a'); - const editor = TypeMoq.Mock.ofType(); - const document = TypeMoq.Mock.ofType(); - document.setup(d => d.uri).returns(() => documentUri); - document.setup(d => d.isUntitled).returns(() => false); - editor.setup(e => e.document).returns(() => document.object); - documentManager.setup(d => d.activeTextEditor).returns(() => editor.object); - - replProvider = new ReplProvider(serviceContainer.object); - expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); - commandHandler!.call(replProvider); - - serviceContainer.verify(c => c.get(TypeMoq.It.isValue(ICodeExecutionService), TypeMoq.It.isValue('repl')), TypeMoq.Times.once()); - codeExecutionService.verify(c => c.initializeRepl(TypeMoq.It.isValue(documentUri)), TypeMoq.Times.once()); - }); - - test('Ensure resource is \'undefined\' if the active document is not used if it is untitled (new document)', () => { - const disposable = TypeMoq.Mock.ofType(); - let commandHandler: undefined | (() => void); - commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Start_REPL), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { - commandHandler = callback; - return disposable.object; - }); - const editor = TypeMoq.Mock.ofType(); - const document = TypeMoq.Mock.ofType(); - document.setup(d => d.isUntitled).returns(() => true); - editor.setup(e => e.document).returns(() => document.object); - documentManager.setup(d => d.activeTextEditor).returns(() => editor.object); - - replProvider = new ReplProvider(serviceContainer.object); - expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); - commandHandler!.call(replProvider); - - serviceContainer.verify(c => c.get(TypeMoq.It.isValue(ICodeExecutionService), TypeMoq.It.isValue('repl')), TypeMoq.Times.once()); - codeExecutionService.verify(c => c.initializeRepl(TypeMoq.It.isValue(undefined)), TypeMoq.Times.once()); - }); - - test('Ensure first available workspace folder is used if there no document', () => { - const disposable = TypeMoq.Mock.ofType(); - let commandHandler: undefined | (() => void); - commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Start_REPL), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { - commandHandler = callback; - return disposable.object; - }); - documentManager.setup(d => d.activeTextEditor).returns(() => undefined); - - const workspaceUri = Uri.file('a'); - const workspaceFolder = TypeMoq.Mock.ofType(); - workspaceFolder.setup(w => w.uri).returns(() => workspaceUri); - workspace.setup(w => w.workspaceFolders).returns(() => [workspaceFolder.object]); - - replProvider = new ReplProvider(serviceContainer.object); - expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); - commandHandler!.call(replProvider); - - serviceContainer.verify(c => c.get(TypeMoq.It.isValue(ICodeExecutionService), TypeMoq.It.isValue('repl')), TypeMoq.Times.once()); - codeExecutionService.verify(c => c.initializeRepl(TypeMoq.It.isValue(workspaceUri)), TypeMoq.Times.once()); + codeExecutionService.verify(c => c.initializeRepl(TypeMoq.It.isValue(resource)), TypeMoq.Times.once()); }); }); diff --git a/src/test/providers/terminal.unit.test.ts b/src/test/providers/terminal.unit.test.ts index d8682e2715b6..eb04a2070d08 100644 --- a/src/test/providers/terminal.unit.test.ts +++ b/src/test/providers/terminal.unit.test.ts @@ -4,9 +4,9 @@ import * as assert from 'assert'; import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; -import { Disposable, Terminal, TextDocument, TextEditor, Uri, WorkspaceFolder } from 'vscode'; +import { Disposable, Terminal, Uri } from 'vscode'; import { IActiveResourceService } from '../../client/activation/types'; -import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import { ICommandManager, IWorkspaceService } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; import { TerminalService } from '../../client/common/terminal/service'; import { ITerminalActivator, ITerminalServiceFactory } from '../../client/common/terminal/types'; @@ -19,16 +19,17 @@ suite('Terminal Provider', () => { let serviceContainer: TypeMoq.IMock; let commandManager: TypeMoq.IMock; let workspace: TypeMoq.IMock; - let documentManager: TypeMoq.IMock; + let activeResourceService: TypeMoq.IMock; let terminalProvider: TerminalProvider; + const resource = Uri.parse('a'); setup(() => { serviceContainer = TypeMoq.Mock.ofType(); commandManager = TypeMoq.Mock.ofType(); + activeResourceService = TypeMoq.Mock.ofType(); workspace = TypeMoq.Mock.ofType(); - documentManager = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(ICommandManager)).returns(() => commandManager.object); serviceContainer.setup(c => c.get(IWorkspaceService)).returns(() => workspace.object); - serviceContainer.setup(c => c.get(IDocumentManager)).returns(() => documentManager.object); + serviceContainer.setup(c => c.get(IActiveResourceService)).returns(() => activeResourceService.object); }); teardown(() => { try { @@ -59,7 +60,10 @@ suite('Terminal Provider', () => { commandHandler = callback; return disposable.object; }); - documentManager.setup(d => d.activeTextEditor).returns(() => undefined); + activeResourceService + .setup(a => a.getActiveResource()) + .returns(() => resource) + .verifiable(TypeMoq.Times.once()); workspace.setup(w => w.workspaceFolders).returns(() => undefined); terminalProvider = new TerminalProvider(serviceContainer.object); @@ -68,88 +72,10 @@ suite('Terminal Provider', () => { const terminalServiceFactory = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalServiceFactory))).returns(() => terminalServiceFactory.object); const terminalService = TypeMoq.Mock.ofType(); - terminalServiceFactory.setup(t => t.createTerminalService(TypeMoq.It.isValue(undefined), TypeMoq.It.isValue('Python'))).returns(() => terminalService.object); - - commandHandler!.call(terminalProvider); - terminalService.verify(t => t.show(false), TypeMoq.Times.once()); - }); - - test('Ensure terminal creation does not use uri of the active documents which is untitled', () => { - const disposable = TypeMoq.Mock.ofType(); - let commandHandler: undefined | (() => void); - commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Create_Terminal), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { - commandHandler = callback; - return disposable.object; - }); - const editor = TypeMoq.Mock.ofType(); - documentManager.setup(d => d.activeTextEditor).returns(() => editor.object); - const document = TypeMoq.Mock.ofType(); - document.setup(d => d.isUntitled).returns(() => true); - editor.setup(e => e.document).returns(() => document.object); - workspace.setup(w => w.workspaceFolders).returns(() => undefined); - - terminalProvider = new TerminalProvider(serviceContainer.object); - expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); - - const terminalServiceFactory = TypeMoq.Mock.ofType(); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalServiceFactory))).returns(() => terminalServiceFactory.object); - const terminalService = TypeMoq.Mock.ofType(); - terminalServiceFactory.setup(t => t.createTerminalService(TypeMoq.It.isValue(undefined), TypeMoq.It.isValue('Python'))).returns(() => terminalService.object); - - commandHandler!.call(terminalProvider); - terminalService.verify(t => t.show(false), TypeMoq.Times.once()); - }); - - test('Ensure terminal creation uses uri of active document', () => { - const disposable = TypeMoq.Mock.ofType(); - let commandHandler: undefined | (() => void); - commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Create_Terminal), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { - commandHandler = callback; - return disposable.object; - }); - const editor = TypeMoq.Mock.ofType(); - documentManager.setup(d => d.activeTextEditor).returns(() => editor.object); - const document = TypeMoq.Mock.ofType(); - const documentUri = Uri.file('a'); - document.setup(d => d.isUntitled).returns(() => false); - document.setup(d => d.uri).returns(() => documentUri); - editor.setup(e => e.document).returns(() => document.object); - workspace.setup(w => w.workspaceFolders).returns(() => undefined); - - terminalProvider = new TerminalProvider(serviceContainer.object); - expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); - - const terminalServiceFactory = TypeMoq.Mock.ofType(); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalServiceFactory))).returns(() => terminalServiceFactory.object); - const terminalService = TypeMoq.Mock.ofType(); - terminalServiceFactory.setup(t => t.createTerminalService(TypeMoq.It.isValue(documentUri), TypeMoq.It.isValue('Python'))).returns(() => terminalService.object); - - commandHandler!.call(terminalProvider); - terminalService.verify(t => t.show(false), TypeMoq.Times.once()); - }); - - test('Ensure terminal creation uses uri of active workspace', () => { - const disposable = TypeMoq.Mock.ofType(); - let commandHandler: undefined | (() => void); - commandManager.setup(c => c.registerCommand(TypeMoq.It.isValue(Commands.Create_Terminal), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_cmd, callback) => { - commandHandler = callback; - return disposable.object; - }); - documentManager.setup(d => d.activeTextEditor).returns(() => undefined); - const workspaceUri = Uri.file('a'); - const workspaceFolder = TypeMoq.Mock.ofType(); - workspaceFolder.setup(w => w.uri).returns(() => workspaceUri); - workspace.setup(w => w.workspaceFolders).returns(() => [workspaceFolder.object]); - - terminalProvider = new TerminalProvider(serviceContainer.object); - expect(commandHandler).not.to.be.equal(undefined, 'Handler not set'); - - const terminalServiceFactory = TypeMoq.Mock.ofType(); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalServiceFactory))).returns(() => terminalServiceFactory.object); - const terminalService = TypeMoq.Mock.ofType(); - terminalServiceFactory.setup(t => t.createTerminalService(TypeMoq.It.isValue(workspaceUri), TypeMoq.It.isValue('Python'))).returns(() => terminalService.object); + terminalServiceFactory.setup(t => t.createTerminalService(TypeMoq.It.isValue(resource), TypeMoq.It.isValue('Python'))).returns(() => terminalService.object); commandHandler!.call(terminalProvider); + activeResourceService.verifyAll(); terminalService.verify(t => t.show(false), TypeMoq.Times.once()); }); @@ -159,10 +85,8 @@ suite('Terminal Provider', () => { let pythonSettings: TypeMoq.IMock; let terminalSettings: TypeMoq.IMock; let configService: TypeMoq.IMock; - let activeResourceService: TypeMoq.IMock; let terminalActivator: TypeMoq.IMock; let terminal: TypeMoq.IMock; - const resource = Uri.parse('a'); setup(() => { configService = TypeMoq.Mock.ofType(); @@ -175,6 +99,7 @@ suite('Terminal Provider', () => { terminalActivator = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ITerminalActivator))).returns(() => terminalActivator.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IActiveResourceService))).returns(() => activeResourceService.object); terminal = TypeMoq.Mock.ofType(); }); From 1e4b2326e5d98985211502b1ae615d1a8ed8a345 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 19 Nov 2019 16:05:51 -0800 Subject: [PATCH 08/15] Refactor more stuff --- src/client/terminals/activation.ts | 19 +++-------- .../common/terminals/activation.unit.test.ts | 32 +++++++------------ 2 files changed, 16 insertions(+), 35 deletions(-) diff --git a/src/client/terminals/activation.ts b/src/client/terminals/activation.ts index 3218b32cb96a..3de63857b252 100644 --- a/src/client/terminals/activation.ts +++ b/src/client/terminals/activation.ts @@ -4,10 +4,10 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { Terminal, Uri } from 'vscode'; -import { IExtensionSingleActivationService } from '../activation/types'; +import { Terminal } from 'vscode'; +import { IActiveResourceService, IExtensionSingleActivationService } from '../activation/types'; import { - ICommandManager, IDocumentManager, ITerminalManager, IWorkspaceService + ICommandManager, ITerminalManager } from '../common/application/types'; import { CODE_RUNNER_EXTENSION_ID } from '../common/constants'; import { ITerminalActivator } from '../common/terminal/types'; @@ -49,9 +49,8 @@ export class TerminalAutoActivation implements ITerminalAutoActivation { constructor( @inject(ITerminalManager) private readonly terminalManager: ITerminalManager, @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry, - @inject(IDocumentManager) private readonly documentManager: IDocumentManager, @inject(ITerminalActivator) private readonly activator: ITerminalActivator, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService + @inject(IActiveResourceService) private readonly activeResourceService: IActiveResourceService ) { disposableRegistry.push(this); } @@ -70,14 +69,6 @@ export class TerminalAutoActivation implements ITerminalAutoActivation { private async activateTerminal(terminal: Terminal): Promise { // If we have just one workspace, then pass that as the resource. // Until upstream VSC issue is resolved https://github.com/Microsoft/vscode/issues/63052. - await this.activator.activateEnvironmentInTerminal(terminal, this.getActiveResource()); - } - - private getActiveResource(): Uri | undefined { - if (this.documentManager.activeTextEditor && !this.documentManager.activeTextEditor.document.isUntitled) { - return this.documentManager.activeTextEditor.document.uri; - } - - return Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders.length > 0 ? this.workspaceService.workspaceFolders[0].uri : undefined; + await this.activator.activateEnvironmentInTerminal(terminal, this.activeResourceService.getActiveResource()); } } diff --git a/src/test/common/terminals/activation.unit.test.ts b/src/test/common/terminals/activation.unit.test.ts index 4eaee471d4b9..af0a5ebccf0b 100644 --- a/src/test/common/terminals/activation.unit.test.ts +++ b/src/test/common/terminals/activation.unit.test.ts @@ -6,10 +6,10 @@ import { expect } from 'chai'; import { anything, instance, mock, verify, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { Terminal, Uri } from 'vscode'; -import { DocumentManager } from '../../../client/common/application/documentManager'; +import { ActiveResourceService } from '../../../client/activation/activeResource'; +import { IActiveResourceService } from '../../../client/activation/types'; import { TerminalManager } from '../../../client/common/application/terminalManager'; -import { IDocumentManager, ITerminalManager, IWorkspaceService } from '../../../client/common/application/types'; -import { WorkspaceService } from '../../../client/common/application/workspace'; +import { ITerminalManager } from '../../../client/common/application/types'; import { TerminalActivator } from '../../../client/common/terminal/activator'; import { ITerminalActivator } from '../../../client/common/terminal/types'; import { IDisposable } from '../../../client/common/types'; @@ -18,23 +18,21 @@ import { ITerminalAutoActivation } from '../../../client/terminals/types'; suite('Terminal Auto Activation', () => { let activator: ITerminalActivator; - let documentManager: IDocumentManager; let terminalManager: ITerminalManager; let terminalAutoActivation: ITerminalAutoActivation; - let workspaceService: IWorkspaceService; + let activeResourceService: IActiveResourceService; + const resource = Uri.parse('a'); setup(() => { terminalManager = mock(TerminalManager); - documentManager = mock(DocumentManager); activator = mock(TerminalActivator); - workspaceService = mock(WorkspaceService); + activeResourceService = mock(ActiveResourceService); terminalAutoActivation = new TerminalAutoActivation( instance(terminalManager), [], - instance(documentManager), instance(activator), - instance(workspaceService) + instance(activeResourceService) ); }); @@ -47,9 +45,9 @@ suite('Terminal Auto Activation', () => { handler = cb; return handlerDisposable.object; }; + when(activeResourceService.getActiveResource()).thenReturn(resource); when(terminalManager.onDidOpenTerminal).thenReturn(onDidOpenTerminal); when(activator.activateEnvironmentInTerminal(anything(), anything(), anything())).thenResolve(); - when(workspaceService.hasWorkspaceFolders).thenReturn(false); terminalAutoActivation.register(); @@ -57,7 +55,7 @@ suite('Terminal Auto Activation', () => { handler!.bind(terminalAutoActivation)(terminal.object); - verify(activator.activateEnvironmentInTerminal(terminal.object, undefined)).once(); + verify(activator.activateEnvironmentInTerminal(terminal.object, resource)).once(); }); test('New Terminals should be activated with resource of single workspace', async () => { type EventHandler = (e: Terminal) => void; @@ -68,11 +66,9 @@ suite('Terminal Auto Activation', () => { handler = cb; return handlerDisposable.object; }; - const resource = Uri.file(__filename); + when(activeResourceService.getActiveResource()).thenReturn(resource); when(terminalManager.onDidOpenTerminal).thenReturn(onDidOpenTerminal); when(activator.activateEnvironmentInTerminal(anything(), anything(), anything())).thenResolve(); - when(workspaceService.hasWorkspaceFolders).thenReturn(true); - when(workspaceService.workspaceFolders).thenReturn([{ index: 0, name: '', uri: resource }]); terminalAutoActivation.register(); @@ -91,15 +87,9 @@ suite('Terminal Auto Activation', () => { handler = cb; return handlerDisposable.object; }; - const resource = Uri.file(__filename); + when(activeResourceService.getActiveResource()).thenReturn(resource); when(terminalManager.onDidOpenTerminal).thenReturn(onDidOpenTerminal); when(activator.activateEnvironmentInTerminal(anything(), anything(), anything())).thenResolve(); - when(workspaceService.hasWorkspaceFolders).thenReturn(true); - when(workspaceService.workspaceFolders).thenReturn([ - { index: 0, name: '', uri: resource }, - { index: 2, name: '2', uri: Uri.file('1234') } - ]); - terminalAutoActivation.register(); expect(handler).not.to.be.an('undefined', 'event handler not initialized'); From 754d2d81da359ccd48545889da13ca684cebc31d Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 11:57:59 -0800 Subject: [PATCH 09/15] Moved into common/application --- src/client/activation/activationManager.ts | 4 ++-- src/client/activation/serviceRegistry.ts | 5 +++-- src/client/activation/types.ts | 5 ----- .../{activation => common/application}/activeResource.ts | 5 ++--- src/client/common/application/types.ts | 5 +++++ src/client/providers/replProvider.ts | 3 +-- src/client/providers/terminalProvider.ts | 3 +-- src/client/terminals/activation.ts | 4 ++-- src/test/activation/activationManager.unit.test.ts | 6 +++--- src/test/activation/activeResource.unit.test.ts | 2 +- src/test/activation/serviceRegistry.unit.test.ts | 2 +- src/test/common/terminals/activation.unit.test.ts | 5 ++--- src/test/providers/repl.unit.test.ts | 3 +-- src/test/providers/terminal.unit.test.ts | 3 +-- 14 files changed, 25 insertions(+), 30 deletions(-) rename src/client/{activation => common/application}/activeResource.ts (81%) diff --git a/src/client/activation/activationManager.ts b/src/client/activation/activationManager.ts index bf68dd3be956..2e93fd739855 100644 --- a/src/client/activation/activationManager.ts +++ b/src/client/activation/activationManager.ts @@ -6,13 +6,13 @@ import { inject, injectable, multiInject } from 'inversify'; import { TextDocument } from 'vscode'; import { IApplicationDiagnostics } from '../application/types'; -import { IDocumentManager, IWorkspaceService } from '../common/application/types'; +import { IActiveResourceService, IDocumentManager, IWorkspaceService } from '../common/application/types'; import { PYTHON_LANGUAGE } from '../common/constants'; import { traceDecorators } from '../common/logger'; import { IDisposable, Resource } from '../common/types'; import { IInterpreterAutoSelectionService } from '../interpreter/autoSelection/types'; import { IInterpreterService } from '../interpreter/contracts'; -import { IActiveResourceService, IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService } from './types'; +import { IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService } from './types'; @injectable() export class ExtensionActivationManager implements IExtensionActivationManager { diff --git a/src/client/activation/serviceRegistry.ts b/src/client/activation/serviceRegistry.ts index e6e0f0107c5f..89a09d291894 100644 --- a/src/client/activation/serviceRegistry.ts +++ b/src/client/activation/serviceRegistry.ts @@ -3,6 +3,8 @@ 'use strict'; +import { ActiveResourceService } from '../common/application/activeResource'; +import { IActiveResourceService } from '../common/application/types'; import { INugetRepository } from '../common/nuget/types'; import { BANNER_NAME_DS_SURVEY, BANNER_NAME_INTERACTIVE_SHIFTENTER, BANNER_NAME_LS_SURVEY, BANNER_NAME_PROPOSE_LS, IPythonExtensionBanner } from '../common/types'; import { DataScienceSurveyBanner } from '../datascience/dataScienceSurveyBanner'; @@ -13,7 +15,6 @@ import { ProposeLanguageServerBanner } from '../languageServices/proposeLanguage import { AATesting } from './aaTesting'; import { ExtensionActivationManager } from './activationManager'; import { LanguageServerExtensionActivationService } from './activationService'; -import { ActiveResourceService } from './activeResource'; import { ExtensionSurveyPrompt } from './extensionSurvey'; import { JediExtensionActivator } from './jedi'; import { LanguageServerExtensionActivator } from './languageServer/activator'; @@ -30,7 +31,7 @@ import { LanguageServerPackageService } from './languageServer/languageServerPac import { LanguageServerManager } from './languageServer/manager'; import { LanguageServerOutputChannel } from './languageServer/outputChannel'; import { PlatformData } from './languageServer/platformData'; -import { IActiveResourceService, IDownloadChannelRule, IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService, ILanguageClientFactory, ILanguageServer, ILanguageServerActivator, ILanguageServerAnalysisOptions, ILanguageServerCompatibilityService as ILanagueServerCompatibilityService, ILanguageServerDownloader, ILanguageServerExtension, ILanguageServerFolderService, ILanguageServerManager, ILanguageServerOutputChannel, ILanguageServerPackageService, IPlatformData, LanguageClientFactory, LanguageServerActivator } from './types'; +import { IDownloadChannelRule, IExtensionActivationManager, IExtensionActivationService, IExtensionSingleActivationService, ILanguageClientFactory, ILanguageServer, ILanguageServerActivator, ILanguageServerAnalysisOptions, ILanguageServerCompatibilityService as ILanagueServerCompatibilityService, ILanguageServerDownloader, ILanguageServerExtension, ILanguageServerFolderService, ILanguageServerManager, ILanguageServerOutputChannel, ILanguageServerPackageService, IPlatformData, LanguageClientFactory, LanguageServerActivator } from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IExtensionActivationService, LanguageServerExtensionActivationService); diff --git a/src/client/activation/types.ts b/src/client/activation/types.ts index ea050863429e..078d98760b1a 100644 --- a/src/client/activation/types.ts +++ b/src/client/activation/types.ts @@ -169,8 +169,3 @@ export const IExtensionSingleActivationService = Symbol('IExtensionSingleActivat export interface IExtensionSingleActivationService { activate(): Promise; } - -export const IActiveResourceService = Symbol('IActiveResourceService'); -export interface IActiveResourceService { - getActiveResource(): Resource; -} diff --git a/src/client/activation/activeResource.ts b/src/client/common/application/activeResource.ts similarity index 81% rename from src/client/activation/activeResource.ts rename to src/client/common/application/activeResource.ts index 8c7c5345761d..f93a112306ad 100644 --- a/src/client/activation/activeResource.ts +++ b/src/client/common/application/activeResource.ts @@ -4,9 +4,8 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { IDocumentManager, IWorkspaceService } from '../common/application/types'; -import { Resource } from '../common/types'; -import { IActiveResourceService } from './types'; +import { Resource } from '../types'; +import { IActiveResourceService, IDocumentManager, IWorkspaceService } from './types'; @injectable() export class ActiveResourceService implements IActiveResourceService { diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 1b3b25095771..34873993d0b0 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -1017,3 +1017,8 @@ export interface ILanguageService { } export type Channel = 'stable' | 'insiders'; + +export const IActiveResourceService = Symbol('IActiveResourceService'); +export interface IActiveResourceService { + getActiveResource(): Resource; +} diff --git a/src/client/providers/replProvider.ts b/src/client/providers/replProvider.ts index 82233e9f73c0..a7a68b647fe7 100644 --- a/src/client/providers/replProvider.ts +++ b/src/client/providers/replProvider.ts @@ -1,6 +1,5 @@ import { Disposable } from 'vscode'; -import { IActiveResourceService } from '../activation/types'; -import { ICommandManager } from '../common/application/types'; +import { IActiveResourceService, ICommandManager } from '../common/application/types'; import { Commands } from '../common/constants'; import { IServiceContainer } from '../ioc/types'; import { captureTelemetry } from '../telemetry'; diff --git a/src/client/providers/terminalProvider.ts b/src/client/providers/terminalProvider.ts index 325416fc0e16..0307c0799f03 100644 --- a/src/client/providers/terminalProvider.ts +++ b/src/client/providers/terminalProvider.ts @@ -2,8 +2,7 @@ // Licensed under the MIT License. import { Disposable, Terminal } from 'vscode'; -import { IActiveResourceService } from '../activation/types'; -import { ICommandManager } from '../common/application/types'; +import { IActiveResourceService, ICommandManager } from '../common/application/types'; import { Commands } from '../common/constants'; import { ITerminalActivator, ITerminalServiceFactory } from '../common/terminal/types'; import { IConfigurationService } from '../common/types'; diff --git a/src/client/terminals/activation.ts b/src/client/terminals/activation.ts index 3de63857b252..d1307827f205 100644 --- a/src/client/terminals/activation.ts +++ b/src/client/terminals/activation.ts @@ -5,9 +5,9 @@ import { inject, injectable } from 'inversify'; import { Terminal } from 'vscode'; -import { IActiveResourceService, IExtensionSingleActivationService } from '../activation/types'; +import { IExtensionSingleActivationService } from '../activation/types'; import { - ICommandManager, ITerminalManager + IActiveResourceService, ICommandManager, ITerminalManager } from '../common/application/types'; import { CODE_RUNNER_EXTENSION_ID } from '../common/constants'; import { ITerminalActivator } from '../common/terminal/types'; diff --git a/src/test/activation/activationManager.unit.test.ts b/src/test/activation/activationManager.unit.test.ts index 9a3ab8391cbf..4fb64c57d97d 100644 --- a/src/test/activation/activationManager.unit.test.ts +++ b/src/test/activation/activationManager.unit.test.ts @@ -9,10 +9,10 @@ import * as typemoq from 'typemoq'; import { TextDocument, Uri } from 'vscode'; import { ExtensionActivationManager } from '../../client/activation/activationManager'; import { LanguageServerExtensionActivationService } from '../../client/activation/activationService'; -import { ActiveResourceService } from '../../client/activation/activeResource'; -import { IActiveResourceService, IExtensionActivationService } from '../../client/activation/types'; +import { IExtensionActivationService } from '../../client/activation/types'; import { IApplicationDiagnostics } from '../../client/application/types'; -import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import { ActiveResourceService } from '../../client/common/application/activeResource'; +import { IActiveResourceService, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import { WorkspaceService } from '../../client/common/application/workspace'; import { PYTHON_LANGUAGE } from '../../client/common/constants'; import { IDisposable } from '../../client/common/types'; diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts index 8338dc2f25e4..2bedaf52a3dc 100644 --- a/src/test/activation/activeResource.unit.test.ts +++ b/src/test/activation/activeResource.unit.test.ts @@ -8,7 +8,7 @@ import { assert } from 'chai'; import { instance, mock, verify, when } from 'ts-mockito'; import { Uri } from 'vscode'; -import { ActiveResourceService } from '../../client/activation/activeResource'; +import { ActiveResourceService } from '../../client/common/application/activeResource'; import { DocumentManager } from '../../client/common/application/documentManager'; import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import { WorkspaceService } from '../../client/common/application/workspace'; diff --git a/src/test/activation/serviceRegistry.unit.test.ts b/src/test/activation/serviceRegistry.unit.test.ts index 6efcb9716305..b89df55c650b 100644 --- a/src/test/activation/serviceRegistry.unit.test.ts +++ b/src/test/activation/serviceRegistry.unit.test.ts @@ -8,7 +8,7 @@ import { instance, mock, verify } from 'ts-mockito'; import { AATesting } from '../../client/activation/aaTesting'; import { ExtensionActivationManager } from '../../client/activation/activationManager'; import { LanguageServerExtensionActivationService } from '../../client/activation/activationService'; -import { ActiveResourceService } from '../../client/activation/activeResource'; +import { ActiveResourceService } from '../../client/common/application/activeResource'; import { ExtensionSurveyPrompt } from '../../client/activation/extensionSurvey'; import { JediExtensionActivator } from '../../client/activation/jedi'; import { LanguageServerExtensionActivator } from '../../client/activation/languageServer/activator'; diff --git a/src/test/common/terminals/activation.unit.test.ts b/src/test/common/terminals/activation.unit.test.ts index af0a5ebccf0b..ed92ad1487cd 100644 --- a/src/test/common/terminals/activation.unit.test.ts +++ b/src/test/common/terminals/activation.unit.test.ts @@ -6,10 +6,9 @@ import { expect } from 'chai'; import { anything, instance, mock, verify, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { Terminal, Uri } from 'vscode'; -import { ActiveResourceService } from '../../../client/activation/activeResource'; -import { IActiveResourceService } from '../../../client/activation/types'; +import { ActiveResourceService } from '../../../client/common/application/activeResource'; import { TerminalManager } from '../../../client/common/application/terminalManager'; -import { ITerminalManager } from '../../../client/common/application/types'; +import { IActiveResourceService, ITerminalManager } from '../../../client/common/application/types'; import { TerminalActivator } from '../../../client/common/terminal/activator'; import { ITerminalActivator } from '../../../client/common/terminal/types'; import { IDisposable } from '../../../client/common/types'; diff --git a/src/test/providers/repl.unit.test.ts b/src/test/providers/repl.unit.test.ts index 330b94d6e43d..2a744faed7e1 100644 --- a/src/test/providers/repl.unit.test.ts +++ b/src/test/providers/repl.unit.test.ts @@ -4,8 +4,7 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { Disposable, Uri } from 'vscode'; -import { IActiveResourceService } from '../../client/activation/types'; -import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import { IActiveResourceService, ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; import { IServiceContainer } from '../../client/ioc/types'; import { ReplProvider } from '../../client/providers/replProvider'; diff --git a/src/test/providers/terminal.unit.test.ts b/src/test/providers/terminal.unit.test.ts index eb04a2070d08..9ba484f20e1f 100644 --- a/src/test/providers/terminal.unit.test.ts +++ b/src/test/providers/terminal.unit.test.ts @@ -5,8 +5,7 @@ import * as assert from 'assert'; import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { Disposable, Terminal, Uri } from 'vscode'; -import { IActiveResourceService } from '../../client/activation/types'; -import { ICommandManager, IWorkspaceService } from '../../client/common/application/types'; +import { IActiveResourceService, ICommandManager, IWorkspaceService } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; import { TerminalService } from '../../client/common/terminal/service'; import { ITerminalActivator, ITerminalServiceFactory } from '../../client/common/terminal/types'; From 0b3b8028c8efd873382b0c93d73b9cfdd3759f26 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 13:36:27 -0800 Subject: [PATCH 10/15] Apply suggestions from code review Co-Authored-By: Eric Snow --- src/client/providers/terminalProvider.ts | 4 +++- src/test/activation/activeResource.unit.test.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/client/providers/terminalProvider.ts b/src/client/providers/terminalProvider.ts index 0307c0799f03..be2a7f795c6f 100644 --- a/src/client/providers/terminalProvider.ts +++ b/src/client/providers/terminalProvider.ts @@ -22,7 +22,9 @@ export class TerminalProvider implements Disposable { @swallowExceptions('Failed to initialize terminal provider') public async initialize(currentTerminal: Terminal | undefined) { const configuration = this.serviceContainer.get(IConfigurationService); - const pythonSettings = configuration.getSettings(this.activeResourceService.getActiveResource()); + const pythonSettings = configuration.getSettings( + this.activeResourceService.getActiveResource() + ); if (pythonSettings.terminal.activateEnvInCurrentTerminal) { if (currentTerminal) { diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts index 2bedaf52a3dc..d45aef971c52 100644 --- a/src/test/activation/activeResource.unit.test.ts +++ b/src/test/activation/activeResource.unit.test.ts @@ -33,13 +33,15 @@ suite('Active resource service', () => { }; when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); when(workspaceService.workspaceFolders).thenReturn([]); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, activeTextEditor.document.uri); verify(documentManager.activeTextEditor).atLeast(1); verify(workspaceService.workspaceFolders).never(); }); - test('Don\'t return document uri if a unsaved document is currently opened', async () => { + test('Don\'t return document uri if the active document is new (still unsaved)', async () => { const activeTextEditor = { document: { isUntitled: true, From a0dfc5c7aa73258e53a4d6092948995f3387e4df Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 13:38:07 -0800 Subject: [PATCH 11/15] Update src/test/activation/activeResource.unit.test.ts Co-Authored-By: Eric Snow --- src/test/activation/activeResource.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts index d45aef971c52..19974657d8ec 100644 --- a/src/test/activation/activeResource.unit.test.ts +++ b/src/test/activation/activeResource.unit.test.ts @@ -24,7 +24,7 @@ suite('Active resource service', () => { activeResourceService = new ActiveResourceService(instance(documentManager), instance(workspaceService)); }); - test('Return document uri if a saved document is currently opened', async () => { + test('Return document uri if the active document is not new (has been saved)', async () => { const activeTextEditor = { document: { isUntitled: false, From 39509dc96570fe30556e943ece34e94c61528726 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 14:34:58 -0800 Subject: [PATCH 12/15] Code reviews --- src/client/common/application/activeResource.ts | 5 +++-- src/test/activation/activeResource.unit.test.ts | 1 - src/test/activation/serviceRegistry.unit.test.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/client/common/application/activeResource.ts b/src/client/common/application/activeResource.ts index f93a112306ad..00efb7ade3b1 100644 --- a/src/client/common/application/activeResource.ts +++ b/src/client/common/application/activeResource.ts @@ -15,8 +15,9 @@ export class ActiveResourceService implements IActiveResourceService { ) { } public getActiveResource(): Resource { - if (this.documentManager.activeTextEditor && !this.documentManager.activeTextEditor.document.isUntitled) { - return this.documentManager.activeTextEditor.document.uri; + const editor = this.documentManager.activeTextEditor; + if (editor && !editor.document.isUntitled) { + return editor.document.uri; } return Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders.length > 0 ? this.workspaceService.workspaceFolders[0].uri diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts index 19974657d8ec..44ab05ec1ec8 100644 --- a/src/test/activation/activeResource.unit.test.ts +++ b/src/test/activation/activeResource.unit.test.ts @@ -32,7 +32,6 @@ suite('Active resource service', () => { } }; when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); - when(workspaceService.workspaceFolders).thenReturn([]); const activeResource = activeResourceService.getActiveResource(); diff --git a/src/test/activation/serviceRegistry.unit.test.ts b/src/test/activation/serviceRegistry.unit.test.ts index b89df55c650b..fd69a7c50c24 100644 --- a/src/test/activation/serviceRegistry.unit.test.ts +++ b/src/test/activation/serviceRegistry.unit.test.ts @@ -8,7 +8,6 @@ import { instance, mock, verify } from 'ts-mockito'; import { AATesting } from '../../client/activation/aaTesting'; import { ExtensionActivationManager } from '../../client/activation/activationManager'; import { LanguageServerExtensionActivationService } from '../../client/activation/activationService'; -import { ActiveResourceService } from '../../client/common/application/activeResource'; import { ExtensionSurveyPrompt } from '../../client/activation/extensionSurvey'; import { JediExtensionActivator } from '../../client/activation/jedi'; import { LanguageServerExtensionActivator } from '../../client/activation/languageServer/activator'; @@ -32,7 +31,6 @@ import { LanguageServerOutputChannel } from '../../client/activation/languageSer import { PlatformData } from '../../client/activation/languageServer/platformData'; import { registerTypes } from '../../client/activation/serviceRegistry'; import { - IActiveResourceService, IDownloadChannelRule, IExtensionActivationManager, IExtensionActivationService, @@ -52,6 +50,8 @@ import { LanguageClientFactory, LanguageServerActivator } from '../../client/activation/types'; +import { ActiveResourceService } from '../../client/common/application/activeResource'; +import { IActiveResourceService } from '../../client/common/application/types'; import { INugetRepository } from '../../client/common/nuget/types'; import { BANNER_NAME_DS_SURVEY, BANNER_NAME_INTERACTIVE_SHIFTENTER, BANNER_NAME_LS_SURVEY, BANNER_NAME_PROPOSE_LS, IPythonExtensionBanner } from '../../client/common/types'; import { DataScienceSurveyBanner } from '../../client/datascience/dataScienceSurveyBanner'; From 66fd30c7fb203b559bed129f47bd7e56f4f0e4d5 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 14:36:52 -0800 Subject: [PATCH 13/15] Don't overuse tslint any --- src/test/activation/activeResource.unit.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts index 44ab05ec1ec8..2d6c913680da 100644 --- a/src/test/activation/activeResource.unit.test.ts +++ b/src/test/activation/activeResource.unit.test.ts @@ -3,8 +3,6 @@ 'use strict'; -// tslint:disable: no-any - import { assert } from 'chai'; import { instance, mock, verify, when } from 'ts-mockito'; import { Uri } from 'vscode'; @@ -31,6 +29,7 @@ suite('Active resource service', () => { uri: Uri.parse('a') } }; + // tslint:disable-next-line:no-any when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); const activeResource = activeResourceService.getActiveResource(); @@ -47,6 +46,7 @@ suite('Active resource service', () => { uri: Uri.parse('a') } }; + // tslint:disable-next-line:no-any when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); when(workspaceService.workspaceFolders).thenReturn([]); const activeResource = activeResourceService.getActiveResource(); @@ -64,6 +64,7 @@ suite('Active resource service', () => { uri: Uri.parse('b') }]; when(documentManager.activeTextEditor).thenReturn(undefined); + // tslint:disable-next-line:no-any when(workspaceService.workspaceFolders).thenReturn(workspaceFolders as any); const activeResource = activeResourceService.getActiveResource(); assert.deepEqual(activeResource, workspaceFolders[0].uri); From 7b00729e4a05c09731a75f9eaf4deff4e2fc7cb8 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 14:55:50 -0800 Subject: [PATCH 14/15] Add description --- src/client/common/application/types.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 34873993d0b0..11b5547d7dc4 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -1018,6 +1018,9 @@ export interface ILanguageService { export type Channel = 'stable' | 'insiders'; +/** + * Wraps the `ActiveResourceService` API class. Created for injecting and mocking class methods in testing + */ export const IActiveResourceService = Symbol('IActiveResourceService'); export interface IActiveResourceService { getActiveResource(): Resource; From 201342e8e2eba8ae1a18f7ce8a98ac7516103dc2 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 20 Nov 2019 16:07:46 -0800 Subject: [PATCH 15/15] Better format --- src/test/activation/activeResource.unit.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/test/activation/activeResource.unit.test.ts b/src/test/activation/activeResource.unit.test.ts index 2d6c913680da..e29798179af5 100644 --- a/src/test/activation/activeResource.unit.test.ts +++ b/src/test/activation/activeResource.unit.test.ts @@ -49,7 +49,9 @@ suite('Active resource service', () => { // tslint:disable-next-line:no-any when(documentManager.activeTextEditor).thenReturn(activeTextEditor as any); when(workspaceService.workspaceFolders).thenReturn([]); + const activeResource = activeResourceService.getActiveResource(); + assert.notDeepEqual(activeResource, activeTextEditor.document.uri); verify(documentManager.activeTextEditor).atLeast(1); verify(workspaceService.workspaceFolders).atLeast(1); @@ -66,7 +68,9 @@ suite('Active resource service', () => { when(documentManager.activeTextEditor).thenReturn(undefined); // tslint:disable-next-line:no-any when(workspaceService.workspaceFolders).thenReturn(workspaceFolders as any); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, workspaceFolders[0].uri); verify(documentManager.activeTextEditor).atLeast(1); verify(workspaceService.workspaceFolders).atLeast(1); @@ -75,7 +79,9 @@ suite('Active resource service', () => { test('If no document is currently opened & no folder is opened, return undefined', async () => { when(documentManager.activeTextEditor).thenReturn(undefined); when(workspaceService.workspaceFolders).thenReturn(undefined); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, undefined); verify(documentManager.activeTextEditor).atLeast(1); verify(workspaceService.workspaceFolders).atLeast(1); @@ -84,7 +90,9 @@ suite('Active resource service', () => { test('If no document is currently opened & workspace contains no workspace folders, return undefined', async () => { when(documentManager.activeTextEditor).thenReturn(undefined); when(workspaceService.workspaceFolders).thenReturn([]); + const activeResource = activeResourceService.getActiveResource(); + assert.deepEqual(activeResource, undefined); verify(documentManager.activeTextEditor).atLeast(1); verify(workspaceService.workspaceFolders).atLeast(1);