From e53651d75114cc272df532d5eab9f4ccdf1e0e92 Mon Sep 17 00:00:00 2001 From: Anthony Kim <62267334+anthonykim1@users.noreply.github.com> Date: Fri, 16 Feb 2024 13:40:45 -0800 Subject: [PATCH] Prevent first Python command being lost (#22902) 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. --- .../codeExecution/djangoShellCodeExecution.ts | 9 +++- src/client/terminals/codeExecution/repl.ts | 4 +- .../codeExecution/terminalCodeExecution.ts | 35 +++++++++++-- .../djangoShellCodeExect.unit.test.ts | 10 +++- .../terminalCodeExec.unit.test.ts | 52 ++++++++----------- 5 files changed, 73 insertions(+), 37 deletions(-) diff --git a/src/client/terminals/codeExecution/djangoShellCodeExecution.ts b/src/client/terminals/codeExecution/djangoShellCodeExecution.ts index 67b877429a2e..05a1470b5727 100644 --- a/src/client/terminals/codeExecution/djangoShellCodeExecution.ts +++ b/src/client/terminals/codeExecution/djangoShellCodeExecution.ts @@ -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'; @@ -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, @@ -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)); diff --git a/src/client/terminals/codeExecution/repl.ts b/src/client/terminals/codeExecution/repl.ts index 45f19798c3d8..bc9a30af1fac 100644 --- a/src/client/terminals/codeExecution/repl.ts +++ b/src/client/terminals/codeExecution/repl.ts @@ -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'; @@ -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, @@ -31,6 +32,7 @@ export class ReplProvider extends TerminalCodeExecutionProvider { platformService, interpreterService, commandManager, + applicationShell, ); this.terminalTitle = 'REPL'; } diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts index a257fff20dbf..4d775dbf6f97 100644 --- a/src/client/terminals/codeExecution/terminalCodeExecution.ts +++ b/src/client/terminals/codeExecution/terminalCodeExecution.ts @@ -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'; @@ -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 }) { @@ -65,10 +66,34 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { } this.replActive = new Promise(async (resolve) => { const replCommandArgs = await this.getExecutableInfo(resource); + let listener: IDisposable; + Promise.race([ + new Promise((resolve) => setTimeout(() => resolve(true), 3000)), + new Promise((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(() => { diff --git a/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts b/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts index 6c6cf5baec76..749d94672765 100644 --- a/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts +++ b/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts @@ -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'; @@ -32,12 +37,14 @@ suite('Terminal - Django Shell Code Execution', () => { let settings: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; let pythonExecutionFactory: TypeMoq.IMock; + let applicationShell: TypeMoq.IMock; let disposables: Disposable[] = []; setup(() => { const terminalFactory = TypeMoq.Mock.ofType(); terminalSettings = TypeMoq.Mock.ofType(); terminalService = TypeMoq.Mock.ofType(); const configService = TypeMoq.Mock.ofType(); + applicationShell = TypeMoq.Mock.ofType(); workspace = TypeMoq.Mock.ofType(); workspace .setup((c) => c.onDidChangeWorkspaceFolders(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) @@ -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); diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts index 93845e6189eb..4f60adb3b931 100644 --- a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts +++ b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts @@ -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'; @@ -47,6 +52,7 @@ suite('Terminal - Code Execution', () => { let pythonExecutionFactory: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; let isDjangoRepl: boolean; + let applicationShell: TypeMoq.IMock; teardown(() => { disposables.forEach((disposable) => { @@ -71,6 +77,7 @@ suite('Terminal - Code Execution', () => { fileSystem = TypeMoq.Mock.ofType(); pythonExecutionFactory = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); + applicationShell = TypeMoq.Mock.ofType(); settings = TypeMoq.Mock.ofType(); settings.setup((s) => s.terminal).returns(() => terminalSettings.object); configService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); @@ -85,6 +92,7 @@ suite('Terminal - Code Execution', () => { platform.object, interpreterService.object, commandManager.object, + applicationShell.object, ); break; } @@ -97,6 +105,7 @@ suite('Terminal - Code Execution', () => { platform.object, interpreterService.object, commandManager.object, + applicationShell.object, ); expectedTerminalTitle = 'REPL'; break; @@ -120,6 +129,7 @@ suite('Terminal - Code Execution', () => { fileSystem.object, disposables, interpreterService.object, + applicationShell.object, ); expectedTerminalTitle = 'Django Shell'; break; @@ -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); @@ -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(), ); });