Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not use shell integration for Python subshell if Python setting is disabled #24418

Merged
merged 9 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
TerminalShellType,
} from './types';
import { traceVerbose } from '../../logging';
import { getConfiguration } from '../vscodeApis/workspaceApis';

@injectable()
export class TerminalService implements ITerminalService, Disposable {
Expand Down Expand Up @@ -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<void> {
Expand All @@ -74,7 +75,10 @@ export class TerminalService implements ITerminalService, Disposable {
}
this.terminal!.sendText(text);
}
public async executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
public async executeCommand(
commandLine: string,
isPythonShell: boolean,
): Promise<TerminalShellExecution | undefined> {
const terminal = this.terminal!;
if (!this.options?.hideFromUser) {
terminal.show(true);
Expand All @@ -98,7 +102,13 @@ export class TerminalService implements ITerminalService, Disposable {
await promise;
}

if (terminal.shellIntegration) {
const config = getConfiguration('python');
const pythonrcSetting = config.get<boolean>('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;
Expand Down
4 changes: 2 additions & 2 deletions src/client/common/terminal/syncTerminalService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
public sendText(text: string): Promise<void> {
return this.terminalService.sendText(text);
}
public executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
return this.terminalService.executeCommand(commandLine);
public executeCommand(commandLine: string, isPythonShell: boolean): Promise<TerminalShellExecution | undefined> {
return this.terminalService.executeCommand(commandLine, isPythonShell);
}
public show(preserveFocus?: boolean | undefined): Promise<void> {
return this.terminalService.show(preserveFocus);
Expand Down
2 changes: 1 addition & 1 deletion src/client/common/terminal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export interface ITerminalService extends IDisposable {
): Promise<void>;
/** @deprecated */
sendText(text: string): Promise<void>;
executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined>;
executeCommand(commandLine: string, isPythonShell: boolean): Promise<TerminalShellExecution | undefined>;
show(preserveFocus?: boolean): Promise<void>;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
81 changes: 81 additions & 0 deletions src/test/common/terminals/service.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -37,6 +39,9 @@ suite('Terminal Service', () => {
let terminalShellIntegration: TypeMoq.IMock<TerminalShellIntegration>;
let onDidEndTerminalShellExecutionEmitter: EventEmitter<TerminalShellExecutionEndEvent>;
let event: TerminalShellExecutionEndEvent;
let getConfigurationStub: sinon.SinonStub;
let pythonConfig: TypeMoq.IMock<WorkspaceConfiguration>;
let editorConfig: TypeMoq.IMock<WorkspaceConfiguration>;

setup(() => {
terminal = TypeMoq.Mock.ofType<VSCodeTerminal>();
Expand Down Expand Up @@ -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<WorkspaceConfiguration>();
editorConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
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 () => {
Expand All @@ -103,13 +118,15 @@ 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(() => {
const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
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');
Expand All @@ -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));
Expand Down Expand Up @@ -171,6 +189,69 @@ suite('Terminal Service', () => {
terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1));
});

test('Ensure sendText is used 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);

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));
});

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));
});

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 () => {
terminalHelper
.setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', true), TypeMoq.Times.once());

await executor.execute('cmd2');
terminalService.verify(async (t) => t.executeCommand('cmd2'), 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 () => {
Expand All @@ -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', true), 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', true), TypeMoq.Times.once());
});
});
});
Expand Down
Loading