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