Skip to content

Commit

Permalink
Prevent first Python command being lost (#22902)
Browse files Browse the repository at this point in the history 
Fixes: #22673
Fixes: #22545
Fixes: #22691 

Making best effort to address issue where very first command sent to
REPL via Terminal gets ignored, or gets pasted both in Terminal and in
REPL.

With the fix, we observe whether Python REPL is launched in Terminal via
VS Code's `onDidWriteTerminalData` and send the command, or wait three
seconds as a fallback mechanism.

These two combined together will significantly reduce or resolve
all-together the chance of very first command being swollen up or gets
pasted twice in Terminal and REPL previously where it did not have
context of whether Python REPL instance have started inside the Terminal
or not.
  • Loading branch information
@anthonykim1
anthonykim1 authored Feb 16, 2024
1 parent a60fbd5 commit e53651d
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 37 deletions.
9 changes: 8 additions & 1 deletion src/client/terminals/codeExecution/djangoShellCodeExecution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@
import { inject, injectable } from 'inversify';
import * as path from 'path';
import { Disposable, Uri } from 'vscode';
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../common/application/types';
import {
IApplicationShell,
ICommandManager,
IDocumentManager,
IWorkspaceService,
} from '../../common/application/types';
import '../../common/extensions';
import { IFileSystem, IPlatformService } from '../../common/platform/types';
import { ITerminalServiceFactory } from '../../common/terminal/types';
Expand All @@ -28,6 +33,7 @@ export class DjangoShellCodeExecutionProvider extends TerminalCodeExecutionProvi
@inject(IFileSystem) fileSystem: IFileSystem,
@inject(IDisposableRegistry) disposableRegistry: Disposable[],
@inject(IInterpreterService) interpreterService: IInterpreterService,
@inject(IApplicationShell) applicationShell: IApplicationShell,
) {
super(
terminalServiceFactory,
Expand All @@ -37,6 +43,7 @@ export class DjangoShellCodeExecutionProvider extends TerminalCodeExecutionProvi
platformService,
interpreterService,
commandManager,
applicationShell,
);
this.terminalTitle = 'Django Shell';
disposableRegistry.push(new DjangoContextInitializer(documentManager, workspace, fileSystem, commandManager));
Expand Down
4 changes: 3 additions & 1 deletion src/client/terminals/codeExecution/repl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { inject, injectable } from 'inversify';
import { Disposable } from 'vscode';
import { ICommandManager, IWorkspaceService } from '../../common/application/types';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../common/application/types';
import { IPlatformService } from '../../common/platform/types';
import { ITerminalServiceFactory } from '../../common/terminal/types';
import { IConfigurationService, IDisposableRegistry } from '../../common/types';
Expand All @@ -22,6 +22,7 @@ export class ReplProvider extends TerminalCodeExecutionProvider {
@inject(IPlatformService) platformService: IPlatformService,
@inject(IInterpreterService) interpreterService: IInterpreterService,
@inject(ICommandManager) commandManager: ICommandManager,
@inject(IApplicationShell) applicationShell: IApplicationShell,
) {
super(
terminalServiceFactory,
Expand All @@ -31,6 +32,7 @@ export class ReplProvider extends TerminalCodeExecutionProvider {
platformService,
interpreterService,
commandManager,
applicationShell,
);
this.terminalTitle = 'REPL';
}
Expand Down
35 changes: 30 additions & 5 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
import { inject, injectable } from 'inversify';
import * as path from 'path';
import { Disposable, Uri } from 'vscode';
import { ICommandManager, IWorkspaceService } from '../../common/application/types';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../common/application/types';
import '../../common/extensions';
import { IPlatformService } from '../../common/platform/types';
import { ITerminalService, ITerminalServiceFactory } from '../../common/terminal/types';
import { IConfigurationService, IDisposableRegistry, Resource } from '../../common/types';
import { IConfigurationService, IDisposable, IDisposableRegistry, Resource } from '../../common/types';
import { Diagnostics, Repl } from '../../common/utils/localize';
import { showWarningMessage } from '../../common/vscodeApis/windowApis';
import { IInterpreterService } from '../../interpreter/contracts';
Expand All @@ -30,6 +30,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
@inject(IPlatformService) protected readonly platformService: IPlatformService,
@inject(IInterpreterService) protected readonly interpreterService: IInterpreterService,
@inject(ICommandManager) protected readonly commandManager: ICommandManager,
@inject(IApplicationShell) protected readonly applicationShell: IApplicationShell,
) {}

public async executeFile(file: Uri, options?: { newTerminalPerFile: boolean }) {
Expand Down Expand Up @@ -65,10 +66,34 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
}
this.replActive = new Promise<boolean>(async (resolve) => {
const replCommandArgs = await this.getExecutableInfo(resource);
let listener: IDisposable;
Promise.race([
new Promise<boolean>((resolve) => setTimeout(() => resolve(true), 3000)),
new Promise<boolean>((resolve) => {
let count = 0;
const terminalDataTimeout = setTimeout(() => {
resolve(true); // Fall back for test case scenarios.
}, 3000);
// Watch TerminalData to see if REPL launched.
listener = this.applicationShell.onDidWriteTerminalData((e) => {
for (let i = 0; i < e.data.length; i++) {
if (e.data[i] === '>') {
count++;
if (count === 3) {
clearTimeout(terminalDataTimeout);
resolve(true);
}
}
}
});
}),
]).then(() => {
if (listener) {
listener.dispose();
}
resolve(true);
});
terminalService.sendCommand(replCommandArgs.command, replCommandArgs.args);

// Give python repl time to start before we start sending text.
setTimeout(() => resolve(true), 1000);
});
this.disposables.push(
terminalService.onDidCloseTerminal(() => {
Expand Down
10 changes: 9 additions & 1 deletion src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import * as path from 'path';
import * as TypeMoq from 'typemoq';
import * as sinon from 'sinon';
import { Disposable, Uri, WorkspaceFolder } from 'vscode';
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../../client/common/application/types';
import {
IApplicationShell,
ICommandManager,
IDocumentManager,
IWorkspaceService,
} from '../../../client/common/application/types';
import { IFileSystem, IPlatformService } from '../../../client/common/platform/types';
import { createCondaEnv } from '../../../client/common/process/pythonEnvironment';
import { createPythonProcessService } from '../../../client/common/process/pythonProcess';
Expand All @@ -32,12 +37,14 @@ suite('Terminal - Django Shell Code Execution', () => {
let settings: TypeMoq.IMock<IPythonSettings>;
let interpreterService: TypeMoq.IMock<IInterpreterService>;
let pythonExecutionFactory: TypeMoq.IMock<IPythonExecutionFactory>;
let applicationShell: TypeMoq.IMock<IApplicationShell>;
let disposables: Disposable[] = [];
setup(() => {
const terminalFactory = TypeMoq.Mock.ofType<ITerminalServiceFactory>();
terminalSettings = TypeMoq.Mock.ofType<ITerminalSettings>();
terminalService = TypeMoq.Mock.ofType<ITerminalService>();
const configService = TypeMoq.Mock.ofType<IConfigurationService>();
applicationShell = TypeMoq.Mock.ofType<IApplicationShell>();
workspace = TypeMoq.Mock.ofType<IWorkspaceService>();
workspace
.setup((c) => c.onDidChangeWorkspaceFolders(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
Expand All @@ -62,6 +69,7 @@ suite('Terminal - Django Shell Code Execution', () => {
fileSystem.object,
disposables,
interpreterService.object,
applicationShell.object,
);

terminalFactory.setup((f) => f.getTerminalService(TypeMoq.It.isAny())).returns(() => terminalService.object);
Expand Down
52 changes: 23 additions & 29 deletions src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import * as path from 'path';
import { SemVer } from 'semver';
import * as TypeMoq from 'typemoq';
import { Disposable, Uri, WorkspaceFolder } from 'vscode';
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../../client/common/application/types';
import {
IApplicationShell,
ICommandManager,
IDocumentManager,
IWorkspaceService,
} from '../../../client/common/application/types';
import { IFileSystem, IPlatformService } from '../../../client/common/platform/types';
import { createCondaEnv } from '../../../client/common/process/pythonEnvironment';
import { createPythonProcessService } from '../../../client/common/process/pythonProcess';
Expand Down Expand Up @@ -47,6 +52,7 @@ suite('Terminal - Code Execution', () => {
let pythonExecutionFactory: TypeMoq.IMock<IPythonExecutionFactory>;
let interpreterService: TypeMoq.IMock<IInterpreterService>;
let isDjangoRepl: boolean;
let applicationShell: TypeMoq.IMock<IApplicationShell>;

teardown(() => {
disposables.forEach((disposable) => {
Expand All @@ -71,6 +77,7 @@ suite('Terminal - Code Execution', () => {
fileSystem = TypeMoq.Mock.ofType<IFileSystem>();
pythonExecutionFactory = TypeMoq.Mock.ofType<IPythonExecutionFactory>();
interpreterService = TypeMoq.Mock.ofType<IInterpreterService>();
applicationShell = TypeMoq.Mock.ofType<IApplicationShell>();
settings = TypeMoq.Mock.ofType<IPythonSettings>();
settings.setup((s) => s.terminal).returns(() => terminalSettings.object);
configService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => settings.object);
Expand All @@ -85,6 +92,7 @@ suite('Terminal - Code Execution', () => {
platform.object,
interpreterService.object,
commandManager.object,
applicationShell.object,
);
break;
}
Expand All @@ -97,6 +105,7 @@ suite('Terminal - Code Execution', () => {
platform.object,
interpreterService.object,
commandManager.object,
applicationShell.object,
);
expectedTerminalTitle = 'REPL';
break;
Expand All @@ -120,6 +129,7 @@ suite('Terminal - Code Execution', () => {
fileSystem.object,
disposables,
interpreterService.object,
applicationShell.object,
);
expectedTerminalTitle = 'Django Shell';
break;
Expand Down Expand Up @@ -590,7 +600,7 @@ suite('Terminal - Code Execution', () => {
);
});

test('Ensure repl is re-initialized when terminal is closed', async () => {
test('Ensure REPL launches after reducing risk of command being ignored or duplicated', async () => {
const pythonPath = 'usr/bin/python1234';
const terminalArgs = ['-a', 'b', 'c'];
platform.setup((p) => p.isWindows).returns(() => false);
Expand All @@ -599,43 +609,27 @@ suite('Terminal - Code Execution', () => {
.returns(() => Promise.resolve(({ path: pythonPath } as unknown) as PythonEnvironment));
terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs);

let closeTerminalCallback: undefined | (() => void);
terminalService
.setup((t) => t.onDidCloseTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
.returns((callback) => {
closeTerminalCallback = callback;
return {
dispose: noop,
};
});

await executor.execute('cmd1');
await executor.execute('cmd2');
await executor.execute('cmd3');

const expectedTerminalArgs = isDjangoRepl ? terminalArgs.concat(['manage.py', 'shell']) : terminalArgs;

expect(closeTerminalCallback).not.to.be.an('undefined', 'Callback not initialized');
terminalService.verify(
async (t) =>
t.sendCommand(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isValue(expectedTerminalArgs)),
TypeMoq.Times.once(),
// Now check if sendCommand from the initializeRepl is called atLeastOnce.
// This is due to newly added Promise race and fallback to lower risk of swollen first command.
applicationShell.verify(
async (t) => t.onDidWriteTerminalData(TypeMoq.It.isAny(), TypeMoq.It.isAny()),
TypeMoq.Times.atLeastOnce(),
);

closeTerminalCallback!.call(terminalService.object);
await executor.execute('cmd4');
terminalService.verify(
async (t) =>
t.sendCommand(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isValue(expectedTerminalArgs)),
TypeMoq.Times.exactly(2),
applicationShell.verify(
async (t) => t.onDidWriteTerminalData(TypeMoq.It.isAny(), TypeMoq.It.isAny()),
TypeMoq.Times.atLeastOnce(),
);

closeTerminalCallback!.call(terminalService.object);
await executor.execute('cmd5');
terminalService.verify(
async (t) =>
t.sendCommand(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isValue(expectedTerminalArgs)),
TypeMoq.Times.exactly(3),
applicationShell.verify(
async (t) => t.onDidWriteTerminalData(TypeMoq.It.isAny(), TypeMoq.It.isAny()),
TypeMoq.Times.atLeastOnce(),
);
});

Expand Down

0 comments on commit e53651d

Please sign in to comment.