From 7afffd3dee879dffc6ad0b23d0b42371825ff293 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 16:18:30 -0800 Subject: [PATCH 1/9] do not use shell integration for Python subshell if Python setting is disabled --- src/client/common/terminal/service.ts | 16 +++++++++++++--- .../common/terminal/syncTerminalService.ts | 4 ++-- src/client/common/terminal/types.ts | 2 +- .../codeExecution/terminalCodeExecution.ts | 2 +- .../codeExecution/terminalCodeExec.unit.test.ts | 8 ++++---- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/client/common/terminal/service.ts b/src/client/common/terminal/service.ts index 45ce9afac47e..d3a9652acb1f 100644 --- a/src/client/common/terminal/service.ts +++ b/src/client/common/terminal/service.ts @@ -20,6 +20,7 @@ import { TerminalShellType, } from './types'; import { traceVerbose } from '../../logging'; +import { getConfiguration } from '../vscodeApis/workspaceApis'; @injectable() export class TerminalService implements ITerminalService, Disposable { @@ -64,7 +65,7 @@ export class TerminalService implements ITerminalService, Disposable { this.terminal!.show(true); } - await this.executeCommand(text); + await this.executeCommand(text, false); } /** @deprecated */ public async sendText(text: string): Promise { @@ -74,7 +75,10 @@ export class TerminalService implements ITerminalService, Disposable { } this.terminal!.sendText(text); } - public async executeCommand(commandLine: string): Promise { + public async executeCommand( + commandLine: string, + isPythonShell: boolean, + ): Promise { const terminal = this.terminal!; if (!this.options?.hideFromUser) { terminal.show(true); @@ -98,7 +102,13 @@ export class TerminalService implements ITerminalService, Disposable { await promise; } - if (terminal.shellIntegration) { + const config = getConfiguration('python'); + const pythonrcSetting = config.get('terminal.shellIntegration.enabled'); + if (isPythonShell && !pythonrcSetting) { + // If user has explicitly disabled SI for Python, use sendText for inside Terminal REPL. + terminal.sendText(commandLine); + return undefined; + } else if (terminal.shellIntegration) { const execution = terminal.shellIntegration.executeCommand(commandLine); traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`); return execution; diff --git a/src/client/common/terminal/syncTerminalService.ts b/src/client/common/terminal/syncTerminalService.ts index 60f8ed7a6847..0b46a86ee51e 100644 --- a/src/client/common/terminal/syncTerminalService.ts +++ b/src/client/common/terminal/syncTerminalService.ts @@ -145,8 +145,8 @@ export class SynchronousTerminalService implements ITerminalService, Disposable public sendText(text: string): Promise { return this.terminalService.sendText(text); } - public executeCommand(commandLine: string): Promise { - return this.terminalService.executeCommand(commandLine); + public executeCommand(commandLine: string, isPythonShell: boolean): Promise { + return this.terminalService.executeCommand(commandLine, isPythonShell); } public show(preserveFocus?: boolean | undefined): Promise { return this.terminalService.show(preserveFocus); diff --git a/src/client/common/terminal/types.ts b/src/client/common/terminal/types.ts index db2b7f80e4b1..3e54458a57fd 100644 --- a/src/client/common/terminal/types.ts +++ b/src/client/common/terminal/types.ts @@ -54,7 +54,7 @@ export interface ITerminalService extends IDisposable { ): Promise; /** @deprecated */ sendText(text: string): Promise; - executeCommand(commandLine: string): Promise; + executeCommand(commandLine: string, isPythonShell: boolean): Promise; show(preserveFocus?: boolean): Promise; } diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts index 3cba6141763b..ea444af4d89e 100644 --- a/src/client/terminals/codeExecution/terminalCodeExecution.ts +++ b/src/client/terminals/codeExecution/terminalCodeExecution.ts @@ -59,7 +59,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { this.configurationService.updateSetting('REPL.enableREPLSmartSend', false, resource); } } else { - await this.getTerminalService(resource).executeCommand(code); + await this.getTerminalService(resource).executeCommand(code, true); } } diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts index 4b5537f515d2..8cd19992aa17 100644 --- a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts +++ b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts @@ -643,10 +643,10 @@ suite('Terminal - Code Execution', () => { terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); await executor.execute('cmd1'); - terminalService.verify(async (t) => t.executeCommand('cmd1'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd1', false), TypeMoq.Times.once()); await executor.execute('cmd2'); - terminalService.verify(async (t) => t.executeCommand('cmd2'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd2', false), TypeMoq.Times.once()); }); test('Ensure code is sent to the same terminal for a particular resource', async () => { @@ -668,10 +668,10 @@ suite('Terminal - Code Execution', () => { terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); await executor.execute('cmd1', resource); - terminalService.verify(async (t) => t.executeCommand('cmd1'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd1', false), TypeMoq.Times.once()); await executor.execute('cmd2', resource); - terminalService.verify(async (t) => t.executeCommand('cmd2'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd2', false), TypeMoq.Times.once()); }); }); }); From f15928ce600e9eaf6edfa041ee9de8dab1250713 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 16:29:02 -0800 Subject: [PATCH 2/9] test correction --- .../terminals/codeExecution/terminalCodeExec.unit.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts index 8cd19992aa17..b5bcecd971ea 100644 --- a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts +++ b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts @@ -643,10 +643,10 @@ suite('Terminal - Code Execution', () => { terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); await executor.execute('cmd1'); - terminalService.verify(async (t) => t.executeCommand('cmd1', false), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd1', true), TypeMoq.Times.once()); await executor.execute('cmd2'); - terminalService.verify(async (t) => t.executeCommand('cmd2', false), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd2', true), TypeMoq.Times.once()); }); test('Ensure code is sent to the same terminal for a particular resource', async () => { @@ -668,10 +668,10 @@ suite('Terminal - Code Execution', () => { terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); await executor.execute('cmd1', resource); - terminalService.verify(async (t) => t.executeCommand('cmd1', false), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd1', true), TypeMoq.Times.once()); await executor.execute('cmd2', resource); - terminalService.verify(async (t) => t.executeCommand('cmd2', false), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd2', true), TypeMoq.Times.once()); }); }); }); From 48de3b3cc8faa0c392d119fcf84045ba81af840a Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 18:30:40 -0800 Subject: [PATCH 3/9] fix test --- src/client/common/terminal/service.ts | 2 +- src/test/common/terminals/service.unit.test.ts | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/client/common/terminal/service.ts b/src/client/common/terminal/service.ts index d3a9652acb1f..c6709dc5ad0c 100644 --- a/src/client/common/terminal/service.ts +++ b/src/client/common/terminal/service.ts @@ -101,7 +101,7 @@ export class TerminalService implements ITerminalService, Disposable { }); await promise; } - + //breaking test const config = getConfiguration('python'); const pythonrcSetting = config.get('terminal.shellIntegration.enabled'); if (isPythonShell && !pythonrcSetting) { diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index f0754948a233..b52d7907608f 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import * as path from 'path'; +import * as sinon from 'sinon'; import * as TypeMoq from 'typemoq'; import { Disposable, @@ -22,6 +23,7 @@ import { IDisposableRegistry } from '../../../client/common/types'; import { IServiceContainer } from '../../../client/ioc/types'; import { ITerminalAutoActivation } from '../../../client/terminals/types'; import { createPythonInterpreter } from '../../utils/interpreters'; +import * as workspaceApis from '../../../client/common/vscodeApis/workspaceApis'; suite('Terminal Service', () => { let service: TerminalService; @@ -37,6 +39,9 @@ suite('Terminal Service', () => { let terminalShellIntegration: TypeMoq.IMock; let onDidEndTerminalShellExecutionEmitter: EventEmitter; let event: TerminalShellExecutionEndEvent; + let getConfigurationStub: sinon.SinonStub; + let pythonConfig: TypeMoq.IMock; + let editorConfig: TypeMoq.IMock; setup(() => { terminal = TypeMoq.Mock.ofType(); @@ -88,12 +93,22 @@ suite('Terminal Service', () => { mockServiceContainer.setup((c) => c.get(IWorkspaceService)).returns(() => workspaceService.object); mockServiceContainer.setup((c) => c.get(ITerminalActivator)).returns(() => terminalActivator.object); mockServiceContainer.setup((c) => c.get(ITerminalAutoActivation)).returns(() => terminalAutoActivator.object); + getConfigurationStub = sinon.stub(workspaceApis, 'getConfiguration'); + pythonConfig = TypeMoq.Mock.ofType(); + editorConfig = TypeMoq.Mock.ofType(); + getConfigurationStub.callsFake((section: string) => { + if (section === 'python') { + return pythonConfig.object; + } + return editorConfig.object; + }); }); teardown(() => { if (service) { service.dispose(); } disposables.filter((item) => !!item).forEach((item) => item.dispose()); + sinon.restore(); }); test('Ensure terminal is disposed', async () => { @@ -103,6 +118,7 @@ suite('Terminal Service', () => { const os: string = 'windows'; service = new TerminalService(mockServiceContainer.object); const shellPath = 'powershell.exe'; + // TODO: switch over legacy Terminal code to use workspace getConfiguration from workspaceApis instead of directly from vscode.workspace workspaceService .setup((w) => w.getConfiguration(TypeMoq.It.isValue('terminal.integrated.shell'))) .returns(() => { @@ -110,6 +126,7 @@ suite('Terminal Service', () => { workspaceConfig.setup((c) => c.get(os)).returns(() => shellPath); return workspaceConfig.object; }); + pythonConfig.setup((p) => p.get('terminal.shellIntegration.enabled')).returns(() => false); platformService.setup((p) => p.isWindows).returns(() => os === 'windows'); platformService.setup((p) => p.isLinux).returns(() => os === 'linux'); @@ -134,6 +151,7 @@ suite('Terminal Service', () => { }); test('Ensure command is sent to terminal and it is shown', async () => { + pythonConfig.setup((p) => p.get('terminal.shellIntegration.enabled')).returns(() => false); terminalHelper .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(undefined)); From 5b76152996ee53e048b919eb1077677a33e26ec7 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 18:35:15 -0800 Subject: [PATCH 4/9] remove unused comments --- src/client/common/terminal/service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/common/terminal/service.ts b/src/client/common/terminal/service.ts index c6709dc5ad0c..d3a9652acb1f 100644 --- a/src/client/common/terminal/service.ts +++ b/src/client/common/terminal/service.ts @@ -101,7 +101,7 @@ export class TerminalService implements ITerminalService, Disposable { }); await promise; } - //breaking test + const config = getConfiguration('python'); const pythonrcSetting = config.get('terminal.shellIntegration.enabled'); if (isPythonShell && !pythonrcSetting) { From 9af9d51dad68930d568781e96e2681c6caf566ca Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 18:48:26 -0800 Subject: [PATCH 5/9] add TODO for more test --- src/test/common/terminals/service.unit.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index b52d7907608f..d81103008a3e 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -189,6 +189,10 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); + // Ensure sendText is called when Python shell integration is disabled. + + // Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled + test('Ensure terminal is not shown if `hideFromUser` option is set to `true`', async () => { terminalHelper .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) From 8e7244b302a5f716965e70463b32688ef4a9defa Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 19:01:50 -0800 Subject: [PATCH 6/9] add more test --- .../common/terminals/service.unit.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index d81103008a3e..a90be24fe539 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -190,6 +190,25 @@ suite('Terminal Service', () => { }); // Ensure sendText is called when Python shell integration is disabled. + test('Ensure text is sent to terminal and it is shown when Python shell integration is disabled', async () => { + pythonConfig + .setup((p) => p.get('terminal.shellIntegration.enabled')) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); + + terminalHelper + .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(undefined)); + service = new TerminalService(mockServiceContainer.object); + const textToSend = 'Some Text'; + terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); + terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); + + await service.sendText(textToSend); + + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(2)); + terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); + }); // Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled From f00d4645f87b895868b4c822f19b7a061bd44130 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 20:32:07 -0800 Subject: [PATCH 7/9] test --- src/test/common/terminals/service.unit.test.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index a90be24fe539..90f9ae653f1f 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -204,13 +204,20 @@ suite('Terminal Service', () => { terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); - await service.sendText(textToSend); + service.ensureTerminal(); + service.executeCommand(textToSend, true); - terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(2)); + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1)); terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); // Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled + test('Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled', async () => { + pythonConfig + .setup((p) => p.get('terminal.shellIntegration.enabled')) + .returns(() => true) + .verifiable(TypeMoq.Times.once()); + }); test('Ensure terminal is not shown if `hideFromUser` option is set to `true`', async () => { terminalHelper From 78ae35c5eff7179cb7bb8680f412903fa0900cd3 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 21:00:21 -0800 Subject: [PATCH 8/9] more teest --- .../common/terminals/service.unit.test.ts | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index 90f9ae653f1f..a9be10e6dd0a 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -189,7 +189,7 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); - // Ensure sendText is called when Python shell integration is disabled. + // Ensure sendText is called when Python shell integration are disabled. test('Ensure text is sent to terminal and it is shown when Python shell integration is disabled', async () => { pythonConfig .setup((p) => p.get('terminal.shellIntegration.enabled')) @@ -211,12 +211,48 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); + // Ensure sendText is called when terminal.shellIntegration is enabled but Python shell integration is disabled + test('Ensure sendText is called when terminal.shellIntegration enabled but Python shell integration disabled', async () => { + pythonConfig + .setup((p) => p.get('terminal.shellIntegration.enabled')) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); + + terminalHelper + .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(undefined)); + service = new TerminalService(mockServiceContainer.object); + const textToSend = 'Some Text'; + terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); + terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); + + service.ensureTerminal(); + service.executeCommand(textToSend, true); + + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1)); + terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); + }); + // Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled - test('Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled', async () => { + test('Ensure sendText is not called when Python shell integration and terminal shell integration are both enabled', async () => { pythonConfig .setup((p) => p.get('terminal.shellIntegration.enabled')) .returns(() => true) .verifiable(TypeMoq.Times.once()); + + terminalHelper + .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(undefined)); + service = new TerminalService(mockServiceContainer.object); + const textToSend = 'Some Text'; + terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); + terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); + + service.ensureTerminal(); + service.executeCommand(textToSend, true); + + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1)); + terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.never()); }); test('Ensure terminal is not shown if `hideFromUser` option is set to `true`', async () => { From 032741d4894e8f6ffe27a8a1a7089840f65d0515 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 11 Nov 2024 21:04:52 -0800 Subject: [PATCH 9/9] polish test --- src/test/common/terminals/service.unit.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index a9be10e6dd0a..7859b6d29e49 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -189,8 +189,7 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); - // Ensure sendText is called when Python shell integration are disabled. - test('Ensure text is sent to terminal and it is shown when Python shell integration is disabled', async () => { + test('Ensure sendText is used when Python shell integration is disabled', async () => { pythonConfig .setup((p) => p.get('terminal.shellIntegration.enabled')) .returns(() => false) @@ -211,7 +210,6 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); - // Ensure sendText is called when terminal.shellIntegration is enabled but Python shell integration is disabled test('Ensure sendText is called when terminal.shellIntegration enabled but Python shell integration disabled', async () => { pythonConfig .setup((p) => p.get('terminal.shellIntegration.enabled')) @@ -233,8 +231,7 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); - // Ensure executeCommand is called when Python shell integration and terminal shell integration are both enabled - test('Ensure sendText is not called when Python shell integration and terminal shell integration are both enabled', async () => { + test('Ensure sendText is NOT called when Python shell integration and terminal shell integration are both enabled', async () => { pythonConfig .setup((p) => p.get('terminal.shellIntegration.enabled')) .returns(() => true)