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

dont automatically inject PYTHONSTARTUP #24346

Merged
merged 46 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
154a3e2
dont automatically inject pystartup
anthonykim1 Oct 28, 2024
377e700
create test to make sure PYTHONSTARTUP not auto injected
anthonykim1 Oct 28, 2024
d60f310
make compiler happy
anthonykim1 Oct 28, 2024
6d8ba24
compile
anthonykim1 Oct 28, 2024
37cd45b
make sure to be safe when checking for property potentitally empty
anthonykim1 Oct 28, 2024
fbcebd2
what is going on with timeout
anthonykim1 Oct 29, 2024
1bbd0bc
try with done() ?
anthonykim1 Oct 29, 2024
3aa86bf
is it because of experiment value
anthonykim1 Oct 29, 2024
5f53706
try adding async and see if that helps
anthonykim1 Oct 29, 2024
d327903
try to add more test
anthonykim1 Oct 29, 2024
2db9394
adjust timeout to see if it helps with potential race condition
anthonykim1 Oct 29, 2024
8b5eeb0
parenthesis
anthonykim1 Oct 29, 2024
7c0b405
add condition in executeCommand in case onDidEndTerminalShellExecutio…
anthonykim1 Oct 29, 2024
0ccb053
remove wrong test
anthonykim1 Oct 29, 2024
40ff863
env should be undefined if not explicitly passed in
anthonykim1 Oct 29, 2024
7dd3f83
add comment
anthonykim1 Oct 29, 2024
3c3bfc9
allow smoke test on windows
anthonykim1 Oct 29, 2024
c1a51ad
reduce to 5 second
anthonykim1 Oct 29, 2024
4ea5c67
fix after feedback
anthonykim1 Oct 29, 2024
8fb2e24
fix test, {} to undefined
anthonykim1 Oct 30, 2024
83dbb90
set default shell to pwsh
anthonykim1 Oct 30, 2024
2f3e787
passing on local, fail on CI - try longer time
anthonykim1 Oct 30, 2024
eaa78f2
try increasing time
anthonykim1 Oct 30, 2024
d12ae35
make sure shell integration is disabled on smart send smoke test
anthonykim1 Oct 30, 2024
f7f8ef6
setting to make sure to enable REPLSmartSend
anthonykim1 Oct 30, 2024
6b4b097
add log to debug in CI
anthonykim1 Oct 30, 2024
edb91ba
try different way in service,ts
anthonykim1 Oct 30, 2024
eaaf338
more descriptive comment, ensure env is only passed in when setting e…
anthonykim1 Oct 30, 2024
de99824
clean up
anthonykim1 Oct 30, 2024
dbb2e2a
clean up
anthonykim1 Oct 30, 2024
038c98e
add TODO for future
anthonykim1 Oct 30, 2024
aa07144
improve comment
anthonykim1 Oct 30, 2024
490ef8b
update comment
anthonykim1 Oct 30, 2024
c896ce7
reset to original timeout number
anthonykim1 Oct 30, 2024
59a3116
remove unncessary exitcode promise
anthonykim1 Oct 30, 2024
4fd5369
add _shellIntegrationEnabled
anthonykim1 Oct 30, 2024
b40f53f
add _terminalFirstLaunched field to not call onDidChangeTerminalShell…
anthonykim1 Oct 30, 2024
e527be7
add log for executeCommand
anthonykim1 Oct 30, 2024
446e95c
remove unused
anthonykim1 Oct 30, 2024
1435329
remove unnecessary comment
anthonykim1 Oct 31, 2024
12f7b3e
make smoke test use pwsh
anthonykim1 Oct 31, 2024
4a2a5d0
stop complaining about unknown import
anthonykim1 Oct 31, 2024
c56fb61
append to standardTest to update setting
anthonykim1 Oct 31, 2024
d4b0227
no more importing vscode
anthonykim1 Oct 31, 2024
9e0d33d
dont add setting programmatically for smoke test
anthonykim1 Oct 31, 2024
6fba2a8
clean up
anthonykim1 Oct 31, 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
43 changes: 15 additions & 28 deletions src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import * as path from 'path';
import { CancellationToken, Disposable, Event, EventEmitter, Terminal } from 'vscode';
import { CancellationToken, Disposable, Event, EventEmitter, Terminal, TerminalShellExecution } from 'vscode';
import '../../common/extensions';
import { IInterpreterService } from '../../interpreter/contracts';
import { IServiceContainer } from '../../ioc/types';
import { captureTelemetry } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { ITerminalAutoActivation } from '../../terminals/types';
import { ITerminalManager } from '../application/types';
import { EXTENSION_ROOT_DIR } from '../constants';
import { _SCRIPTS_DIR } from '../process/internal/scripts/constants';
import { IConfigurationService, IDisposableRegistry } from '../types';
import {
Expand All @@ -20,7 +18,6 @@ import {
ITerminalService,
TerminalCreationOptions,
TerminalShellType,
ITerminalExecutedCommand,
} from './types';
import { traceVerbose } from '../../logging';

Expand All @@ -33,11 +30,12 @@ export class TerminalService implements ITerminalService, Disposable {
private terminalHelper: ITerminalHelper;
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();
private _terminalFirstLaunched: boolean = true;
public get onDidCloseTerminal(): Event<void> {
return this.terminalClosed.event.bind(this.terminalClosed);
}

constructor(
@inject(IServiceContainer) private serviceContainer: IServiceContainer,
private readonly options?: TerminalCreationOptions,
Expand Down Expand Up @@ -76,24 +74,24 @@ export class TerminalService implements ITerminalService, Disposable {
}
this.terminal!.sendText(text);
}
public async executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
public async executeCommand(commandLine: string): Promise<TerminalShellExecution | 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) {
if (!terminal.shellIntegration && this._terminalFirstLaunched) {
this._terminalFirstLaunched = false;
const promise = new Promise<boolean>((resolve) => {
const shellIntegrationChangeEventListener = this.terminalManager.onDidChangeTerminalShellIntegration(
() => {
this.executeCommandListeners.delete(shellIntegrationChangeEventListener);
resolve(true);
},
);
const disposable = this.terminalManager.onDidChangeTerminalShellIntegration(() => {
clearTimeout(timer);
disposable.dispose();
resolve(true);
});
const TIMEOUT_DURATION = 500;
setTimeout(() => {
this.executeCommandListeners.add(shellIntegrationChangeEventListener);
const timer = setTimeout(() => {
disposable.dispose();
resolve(true);
}, TIMEOUT_DURATION);
});
Expand All @@ -102,18 +100,8 @@ export class TerminalService implements ITerminalService, Disposable {

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);
}
traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`);
});
traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`);
return execution;
} else {
terminal.sendText(commandLine);
traceVerbose(`Shell Integration is disabled, sendText: ${commandLine}`);
Expand All @@ -136,7 +124,6 @@ export class TerminalService implements ITerminalService, Disposable {
this.terminalShellType = this.terminalHelper.identifyTerminalShell(this.terminal);
this.terminal = this.terminalManager.createTerminal({
name: this.options?.title || 'Python',
env: { PYTHONSTARTUP: this.envVarScript },
hideFromUser: this.options?.hideFromUser,
});
this.terminalAutoActivator.disableAutoActivation(this.terminal);
Expand Down
6 changes: 3 additions & 3 deletions src/client/common/terminal/syncTerminalService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
'use strict';

import { inject } from 'inversify';
import { CancellationToken, Disposable, Event } from 'vscode';
import { CancellationToken, Disposable, Event, TerminalShellExecution } from 'vscode';
import { IInterpreterService } from '../../interpreter/contracts';
import { traceVerbose } from '../../logging';
import { PythonEnvironment } from '../../pythonEnvironments/info';
Expand All @@ -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, ITerminalExecutedCommand } from './types';
import { ITerminalService } from './types';

enum State {
notStarted = 0,
Expand Down Expand Up @@ -145,7 +145,7 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
public sendText(text: string): Promise<void> {
return this.terminalService.sendText(text);
}
public executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
public executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
return this.terminalService.executeCommand(commandLine);
}
public show(preserveFocus?: boolean | undefined): Promise<void> {
Expand Down
7 changes: 1 addition & 6 deletions src/client/common/terminal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,10 @@ export interface ITerminalService extends IDisposable {
): Promise<void>;
/** @deprecated */
sendText(text: string): Promise<void>;
executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined>;
executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined>;
show(preserveFocus?: boolean): Promise<void>;
}

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

export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory');

export type TerminalCreationOptions = {
Expand Down
24 changes: 24 additions & 0 deletions src/test/common/terminals/helper.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,30 @@ suite('Terminal Service helpers', () => {
teardown(() => shellDetectorIdentifyTerminalShell.restore());
suite('Misc', () => {
setup(doSetup);
test('Creating terminal should not automatically contain PYTHONSTARTUP', () => {
const theTitle = 'Hello';
const terminal = 'Terminal Created';
when(terminalManager.createTerminal(anything())).thenReturn(terminal as any);
const term = helper.createTerminal(theTitle);
const args = capture(terminalManager.createTerminal).first()[0];
expect(term).to.be.deep.equal(terminal);
const terminalOptions = args.env;
const safeTerminalOptions = terminalOptions || {};
expect(safeTerminalOptions).to.not.have.property('PYTHONSTARTUP');
});

test('Env should be undefined if not explicitly passed in ', () => {
const theTitle = 'Hello';
const terminal = 'Terminal Created';
when(terminalManager.createTerminal(anything())).thenReturn(terminal as any);

const term = helper.createTerminal(theTitle);

verify(terminalManager.createTerminal(anything())).once();
const args = capture(terminalManager.createTerminal).first()[0];
expect(term).to.be.deep.equal(terminal);
expect(args.env).to.be.deep.equal(undefined);
});

test('Create terminal without a title', () => {
const terminal = 'Terminal Created';
Expand Down
131 changes: 64 additions & 67 deletions src/test/smoke/smartSend.smoke.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,81 +6,78 @@ import { EXTENSION_ROOT_DIR_FOR_TESTS, IS_SMOKE_TEST } from '../constants';
import { closeActiveWindows, initialize, initializeTest } from '../initialize';
import { openFile, waitForCondition } from '../common';

// TODO: This test is being flaky for windows, need to investigate why only fails on windows
if (process.platform !== 'win32') {
suite('Smoke Test: Run Smart Selection and Advance Cursor', () => {
suiteSetup(async function () {
if (!IS_SMOKE_TEST) {
return this.skip();
}
await initialize();
return undefined;
});
suite('Smoke Test: Run Smart Selection and Advance Cursor', async () => {
suiteSetup(async function () {
if (!IS_SMOKE_TEST) {
return this.skip();
}
await initialize();
return undefined;
});

setup(initializeTest);
suiteTeardown(closeActiveWindows);
teardown(closeActiveWindows);
setup(initializeTest);
suiteTeardown(closeActiveWindows);
teardown(closeActiveWindows);

test('Smart Send', async () => {
const file = path.join(
EXTENSION_ROOT_DIR_FOR_TESTS,
'src',
'testMultiRootWkspc',
'smokeTests',
'create_delete_file.py',
);
const outputFile = path.join(
EXTENSION_ROOT_DIR_FOR_TESTS,
'src',
'testMultiRootWkspc',
'smokeTests',
'smart_send_smoke.txt',
);
test('Smart Send', async () => {
const file = path.join(
EXTENSION_ROOT_DIR_FOR_TESTS,
'src',
'testMultiRootWkspc',
'smokeTests',
'create_delete_file.py',
);
const outputFile = path.join(
EXTENSION_ROOT_DIR_FOR_TESTS,
'src',
'testMultiRootWkspc',
'smokeTests',
'smart_send_smoke.txt',
);

await fs.remove(outputFile);
await fs.remove(outputFile);

const textDocument = await openFile(file);
const textDocument = await openFile(file);

if (vscode.window.activeTextEditor) {
const myPos = new vscode.Position(0, 0);
vscode.window.activeTextEditor!.selections = [new vscode.Selection(myPos, myPos)];
}
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
if (vscode.window.activeTextEditor) {
const myPos = new vscode.Position(0, 0);
vscode.window.activeTextEditor!.selections = [new vscode.Selection(myPos, myPos)];
}
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});

const checkIfFileHasBeenCreated = () => fs.pathExists(outputFile);
await waitForCondition(checkIfFileHasBeenCreated, 10_000, `"${outputFile}" file not created`);
const checkIfFileHasBeenCreated = () => fs.pathExists(outputFile);
await waitForCondition(checkIfFileHasBeenCreated, 20_000, `"${outputFile}" file not created`);

await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});

async function wait() {
return new Promise<void>((resolve) => {
setTimeout(() => {
resolve();
}, 10000);
});
}
async function wait() {
return new Promise<void>((resolve) => {
setTimeout(() => {
resolve();
}, 10000);
});
}

await wait();
await wait();

const deletedFile = !(await fs.pathExists(outputFile));
if (deletedFile) {
assert.ok(true, `"${outputFile}" file has been deleted`);
} else {
assert.fail(`"${outputFile}" file still exists`);
}
});
const deletedFile = !(await fs.pathExists(outputFile));
if (deletedFile) {
assert.ok(true, `"${outputFile}" file has been deleted`);
} else {
assert.fail(`"${outputFile}" file still exists`);
}
});
}
});
1 change: 0 additions & 1 deletion src/test/smokeTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

// Must always be on top to setup expected env.
process.env.VSC_PYTHON_SMOKE_TEST = '1';

import { spawn } from 'child_process';
import * as fs from '../client/common/platform/fs-paths';
import * as glob from 'glob';
Expand Down
Loading