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

Switch over to executeCommand from sendText #24078

Merged
merged 37 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2413118
vscode engine to 1.93
anthonykim1 Sep 9, 2024
5de9ca5
use executeCommand
anthonykim1 Sep 9, 2024
6d1f522
codeaction mock
anthonykim1 Sep 9, 2024
c9fc4f1
compile
anthonykim1 Sep 9, 2024
0d78806
switching version allows debugging - magically
anthonykim1 Sep 10, 2024
a16a102
pin version to equal to or above 1.93
anthonykim1 Sep 10, 2024
ea24d50
try to fix unit test
anthonykim1 Sep 10, 2024
2cb1eb3
Debt: switch Promise<void> in ensureTerminal to Promise<Terminal>
anthonykim1 Sep 10, 2024
88b4e9f
passing test when shell integration disabled
anthonykim1 Sep 10, 2024
6ecf4fc
please
anthonykim1 Sep 10, 2024
831f893
// @ts-ignore: TS6133
anthonykim1 Sep 10, 2024
f3aa28e
a
anthonykim1 Sep 10, 2024
342587a
stop
anthonykim1 Sep 11, 2024
3efaf9a
try idisposable
anthonykim1 Sep 11, 2024
d3b2238
more trials
anthonykim1 Sep 11, 2024
28ef3c1
lint
anthonykim1 Sep 11, 2024
2375490
slowly migrate test from sendText to executeCommand
anthonykim1 Sep 11, 2024
a75656b
TODO: test for when shellIntegration is active, mock and fire onDidEn…
anthonykim1 Sep 11, 2024
a51b6b4
onDidEndTerminalShellExecution never gets fired
anthonykim1 Sep 11, 2024
7c01537
switch up the order
anthonykim1 Sep 11, 2024
923df5c
add test onDidEndTerminalShellExecutionEmitter.fire(event)
anthonykim1 Sep 11, 2024
16839c9
attach callback to executeCommand so onDidEndTerminalShellExecutionEm…
anthonykim1 Sep 11, 2024
e30bb77
switch up the order abit + comment
anthonykim1 Sep 11, 2024
e69c490
wow
anthonykim1 Sep 11, 2024
e2e71d0
remove junk
anthonykim1 Sep 11, 2024
3d31d1a
remove unused
anthonykim1 Sep 11, 2024
46d28d2
remove comment
anthonykim1 Sep 11, 2024
5d3878f
TODO: smart send smoke test are flaky on windows
anthonykim1 Sep 11, 2024
dfe61de
a
anthonykim1 Sep 11, 2024
8f9ab0d
take recommended feedbacks
anthonykim1 Sep 12, 2024
6d095c8
remove leftover comments - done
anthonykim1 Sep 12, 2024
563fb12
final
anthonykim1 Sep 12, 2024
145e2fd
remove weird import
anthonykim1 Sep 12, 2024
8bc3951
last missed comment
anthonykim1 Sep 12, 2024
610d0fb
upgrade actions/download-artifact@v4
anthonykim1 Sep 12, 2024
c6a4713
why
anthonykim1 Sep 12, 2024
2b4c2bf
try with actions/download-artifact@v3
anthonykim1 Sep 12, 2024
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: 8 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"theme": "dark"
},
"engines": {
"vscode": "^1.91.0"
"vscode": "^1.93.0"
},
"enableTelemetry": false,
"keywords": [
Expand Down Expand Up @@ -1570,7 +1570,7 @@
"@types/sinon": "^17.0.3",
"@types/stack-trace": "0.0.29",
"@types/tmp": "^0.0.33",
"@types/vscode": "^1.81.0",
"@types/vscode": "^1.93.0",
"@types/which": "^2.0.1",
"@types/winreg": "^1.2.30",
"@types/xml2js": "^0.4.2",
Expand Down
17 changes: 16 additions & 1 deletion src/client/common/application/terminalManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,16 @@
// Licensed under the MIT License.

import { injectable } from 'inversify';
import { Event, EventEmitter, Terminal, TerminalOptions, window } from 'vscode';
import {
Disposable,
Event,
EventEmitter,
Terminal,
TerminalOptions,
TerminalShellExecutionEndEvent,
TerminalShellIntegrationChangeEvent,
window,
} from 'vscode';
import { traceLog } from '../../logging';
import { ITerminalManager } from './types';

Expand All @@ -23,6 +32,12 @@ export class TerminalManager implements ITerminalManager {
public createTerminal(options: TerminalOptions): Terminal {
return monkeyPatchTerminal(window.createTerminal(options));
}
public onDidChangeTerminalShellIntegration(handler: (e: TerminalShellIntegrationChangeEvent) => void): Disposable {
return window.onDidChangeTerminalShellIntegration(handler);
}
public onDidEndTerminalShellExecution(handler: (e: TerminalShellExecutionEndEvent) => void): Disposable {
return window.onDidEndTerminalShellExecution(handler);
}
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/client/common/application/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import {
StatusBarItem,
Terminal,
TerminalOptions,
TerminalShellExecutionEndEvent,
TerminalShellIntegrationChangeEvent,
TextDocument,
TextDocumentChangeEvent,
TextDocumentShowOptions,
Expand Down Expand Up @@ -936,6 +938,10 @@ export interface ITerminalManager {
* @return A new Terminal.
*/
createTerminal(options: TerminalOptions): Terminal;

onDidChangeTerminalShellIntegration(handler: (e: TerminalShellIntegrationChangeEvent) => void): Disposable;

onDidEndTerminalShellExecution(handler: (e: TerminalShellExecutionEndEvent) => void): Disposable;
}

export const IDebugService = Symbol('IDebugManager');
Expand Down
63 changes: 58 additions & 5 deletions src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
ITerminalService,
TerminalCreationOptions,
TerminalShellType,
ITerminalExecutedCommand,
} from './types';

@injectable()
Expand All @@ -32,6 +33,7 @@ export class TerminalService implements ITerminalService, Disposable {
private terminalActivator: ITerminalActivator;
private terminalAutoActivator: ITerminalAutoActivation;
private readonly envVarScript = path.join(EXTENSION_ROOT_DIR, 'python_files', 'pythonrc.py');
private readonly executeCommandListeners: Set<Disposable> = new Set();
public get onDidCloseTerminal(): Event<void> {
return this.terminalClosed.event.bind(this.terminalClosed);
}
Expand All @@ -48,8 +50,12 @@ export class TerminalService implements ITerminalService, Disposable {
this.terminalActivator = this.serviceContainer.get<ITerminalActivator>(ITerminalActivator);
}
public dispose() {
if (this.terminal) {
this.terminal.dispose();
this.terminal?.dispose();

if (this.executeCommandListeners && this.executeCommandListeners.size > 0) {
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
this.executeCommandListeners.forEach((d) => {
d?.dispose();
});
}
}
public async sendCommand(command: string, args: string[], _?: CancellationToken): Promise<void> {
Expand All @@ -59,21 +65,67 @@ export class TerminalService implements ITerminalService, Disposable {
this.terminal!.show(true);
}

this.terminal!.sendText(text, true);
await this.executeCommand(text);
}
/** @deprecated */
public async sendText(text: string): Promise<void> {
await this.ensureTerminal();
if (!this.options?.hideFromUser) {
this.terminal!.show(true);
}
this.terminal!.sendText(text);
}
public async executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
const terminal = this.terminal!;
if (!this.options?.hideFromUser) {
terminal.show(true);
}

// If terminal was just launched, wait some time for shell integration to onDidChangeShellIntegration.
if (!terminal.shellIntegration) {
const promise = new Promise<boolean>((resolve) => {
const shellIntegrationChangeEventListener = this.terminalManager.onDidChangeTerminalShellIntegration(
() => {
this.executeCommandListeners.delete(shellIntegrationChangeEventListener);
resolve(true);
},
);
const TIMEOUT_DURATION = 3000;
setTimeout(() => {
this.executeCommandListeners.add(shellIntegrationChangeEventListener);
resolve(true);
}, TIMEOUT_DURATION);
});
await promise;
}

if (terminal.shellIntegration) {
const execution = terminal.shellIntegration.executeCommand(commandLine);
return await new Promise((resolve) => {
const listener = this.terminalManager.onDidEndTerminalShellExecution((e) => {
if (e.execution === execution) {
this.executeCommandListeners.delete(listener);
resolve({ execution, exitCode: e.exitCode });
}
});
if (listener) {
this.executeCommandListeners.add(listener);
}
});
} else {
terminal.sendText(commandLine);
}

return undefined;
}

public async show(preserveFocus: boolean = true): Promise<void> {
await this.ensureTerminal(preserveFocus);
if (!this.options?.hideFromUser) {
this.terminal!.show(preserveFocus);
}
}
// TODO: Debt switch to Promise<Terminal> ---> breaks 20 tests
public async ensureTerminal(preserveFocus: boolean = true): Promise<void> {
if (this.terminal) {
return;
Expand All @@ -89,18 +141,19 @@ export class TerminalService implements ITerminalService, Disposable {
// Sometimes the terminal takes some time to start up before it can start accepting input.
await new Promise((resolve) => setTimeout(resolve, 100));

await this.terminalActivator.activateEnvironmentInTerminal(this.terminal!, {
await this.terminalActivator.activateEnvironmentInTerminal(this.terminal, {
resource: this.options?.resource,
preserveFocus,
interpreter: this.options?.interpreter,
hideFromUser: this.options?.hideFromUser,
});

if (!this.options?.hideFromUser) {
this.terminal!.show(preserveFocus);
this.terminal.show(preserveFocus);
}

this.sendTelemetry().ignoreErrors();
return;
}
private terminalCloseHandler(terminal: Terminal) {
if (terminal === this.terminal) {
Expand Down
6 changes: 5 additions & 1 deletion src/client/common/terminal/syncTerminalService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import * as internalScripts from '../process/internal/scripts';
import { createDeferred, Deferred } from '../utils/async';
import { noop } from '../utils/misc';
import { TerminalService } from './service';
import { ITerminalService } from './types';
import { ITerminalService, ITerminalExecutedCommand } from './types';

enum State {
notStarted = 0,
Expand Down Expand Up @@ -146,9 +146,13 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
lockFile.dispose();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use less of these services when possible.

/** @deprecated */
public sendText(text: string): Promise<void> {
return this.terminalService.sendText(text);
}
public executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
return this.terminalService.executeCommand(commandLine);
}
public show(preserveFocus?: boolean | undefined): Promise<void> {
return this.terminalService.show(preserveFocus);
}
Expand Down
9 changes: 8 additions & 1 deletion src/client/common/terminal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

'use strict';

import { CancellationToken, Event, Terminal, Uri } from 'vscode';
import { CancellationToken, Event, Terminal, Uri, TerminalShellExecution } from 'vscode';
import { PythonEnvironment } from '../../pythonEnvironments/info';
import { IEventNamePropertyMapping } from '../../telemetry/index';
import { IDisposable, Resource } from '../types';
Expand Down Expand Up @@ -52,10 +52,17 @@ export interface ITerminalService extends IDisposable {
cancel?: CancellationToken,
swallowExceptions?: boolean,
): Promise<void>;
/** @deprecated */
sendText(text: string): Promise<void>;
executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined>;
show(preserveFocus?: boolean): Promise<void>;
}

export interface ITerminalExecutedCommand {
execution: TerminalShellExecution;
exitCode: number | undefined;
}

export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory');

export type TerminalCreationOptions = {
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).sendText(code);
await this.getTerminalService(resource).executeCommand(code);
}
}

Expand Down
59 changes: 55 additions & 4 deletions src/test/common/terminals/service.unit.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

// @ts-ignore: TS6133
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
import { expect } from 'chai';
import * as path from 'path';
import * as TypeMoq from 'typemoq';
import { Disposable, Terminal as VSCodeTerminal, WorkspaceConfiguration } from 'vscode';
import {
Disposable,
EventEmitter,
TerminalShellExecution,
TerminalShellExecutionEndEvent,
TerminalShellIntegration,
Terminal as VSCodeTerminal,
WorkspaceConfiguration,
} from 'vscode';
import { ITerminalManager, IWorkspaceService } from '../../../client/common/application/types';
import { EXTENSION_ROOT_DIR } from '../../../client/common/constants';
import { IPlatformService } from '../../../client/common/platform/types';
Expand All @@ -26,9 +35,44 @@ suite('Terminal Service', () => {
let disposables: Disposable[] = [];
let mockServiceContainer: TypeMoq.IMock<IServiceContainer>;
let terminalAutoActivator: TypeMoq.IMock<ITerminalAutoActivation>;
let terminalShellIntegration: TypeMoq.IMock<TerminalShellIntegration>;
let onDidEndTerminalShellExecutionEmitter: EventEmitter<TerminalShellExecutionEndEvent>;
let event: TerminalShellExecutionEndEvent;

setup(() => {
terminal = TypeMoq.Mock.ofType<VSCodeTerminal>();
terminalShellIntegration = TypeMoq.Mock.ofType<TerminalShellIntegration>();
terminal.setup((t) => t.shellIntegration).returns(() => terminalShellIntegration.object);

onDidEndTerminalShellExecutionEmitter = new EventEmitter<TerminalShellExecutionEndEvent>();
terminalManager = TypeMoq.Mock.ofType<ITerminalManager>();
const execution: TerminalShellExecution = {
commandLine: {
value: 'dummy text',
isTrusted: true,
confidence: 2,
},
cwd: undefined,
read: function (): AsyncIterable<string> {
throw new Error('Function not implemented.');
},
};

event = {
execution,
exitCode: 0,
terminal: terminal.object,
shellIntegration: terminalShellIntegration.object,
};

terminalShellIntegration.setup((t) => t.executeCommand(TypeMoq.It.isAny())).returns(() => execution);

terminalManager
.setup((t) => t.onDidEndTerminalShellExecution)
.returns(() => {
setTimeout(() => onDidEndTerminalShellExecutionEmitter.fire(event), 100);
return onDidEndTerminalShellExecutionEmitter.event;
});
platformService = TypeMoq.Mock.ofType<IPlatformService>();
workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
terminalHelper = TypeMoq.Mock.ofType<ITerminalHelper>();
Expand All @@ -37,6 +81,7 @@ suite('Terminal Service', () => {
disposables = [];

mockServiceContainer = TypeMoq.Mock.ofType<IServiceContainer>();

mockServiceContainer.setup((c) => c.get(ITerminalManager)).returns(() => terminalManager.object);
mockServiceContainer.setup((c) => c.get(ITerminalHelper)).returns(() => terminalHelper.object);
mockServiceContainer.setup((c) => c.get(IPlatformService)).returns(() => platformService.object);
Expand Down Expand Up @@ -75,10 +120,16 @@ suite('Terminal Service', () => {
.setup((h) => h.buildCommandForTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
.returns(() => 'dummy text');

terminalManager
.setup((t) => t.onDidEndTerminalShellExecution)
.returns(() => {
setTimeout(() => onDidEndTerminalShellExecutionEmitter.fire(event), 100);
return onDidEndTerminalShellExecutionEmitter.event;
});
// Sending a command will cause the terminal to be created
await service.sendCommand('', []);

terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(2));
terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.atLeastOnce());
service.dispose();
terminal.verify((t) => t.dispose(), TypeMoq.Times.exactly(1));
});
Expand All @@ -99,10 +150,10 @@ suite('Terminal Service', () => {

await service.sendCommand(commandToSend, args);

terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(2));
terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.atLeastOnce());
terminal.verify(
(t) => t.sendText(TypeMoq.It.isValue(commandToExpect), TypeMoq.It.isValue(true)),
TypeMoq.Times.exactly(1),
TypeMoq.Times.never(),
);
});

Expand Down
Loading
Loading