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 18 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
38 changes: 23 additions & 15 deletions src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import * as path from 'path';
import { CancellationToken, Disposable, Event, EventEmitter, Terminal } from 'vscode';
import '../../common/extensions';
import { IInterpreterService } from '../../interpreter/contracts';
Expand All @@ -11,7 +10,6 @@ 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 @@ -33,7 +31,6 @@ 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();
public get onDidCloseTerminal(): Event<void> {
return this.terminalClosed.event.bind(this.terminalClosed);
Expand Down Expand Up @@ -102,18 +99,29 @@ 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 });
return await Promise.race([
new Promise<ITerminalExecutedCommand>((resolve) => {
const listener = this.terminalManager.onDidEndTerminalShellExecution((e) => {
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
if (e.execution === execution) {
this.executeCommandListeners.delete(listener);
resolve({ execution, exitCode: e.exitCode });
}
});
if (listener) {
this.executeCommandListeners.add(listener);
}
});
if (listener) {
this.executeCommandListeners.add(listener);
}
traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`);
});
traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`);
}),
// Once shell integration is active, hearing back from onDidEndTerminalShellExecution should not take too long.
new Promise<undefined>((resolve) => {
Copy link
Author

Choose a reason for hiding this comment

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

Found that without this, smoke test would time out.
Havent heard issue from user about execution taking forever, so in real-life I think onDidEndTerminalShellExecution gets back in reasonable amout of time.

Copy link
Author

Choose a reason for hiding this comment

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

still added the promise just in case there is a case where onDidEndTerminalShellExecution never gets fired, we don't want people stuck and waiting for their execution result forever.

Copy link
Member

Choose a reason for hiding this comment

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

You would have already scheduled something to run on the terminal. Doesn't sendText mean that it will now add yet another command? Also, in practice Promise.race returns when one of the two finishes, wouldn't it still sendText. there is nothing stopping the setTimeout function from running,

Are you sure that CI is using the new powershell, and not the old one. Have we ensured to set the default shell type to be Powershell when testing this.

Copy link
Author

Choose a reason for hiding this comment

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

I got the smoke tests to use pwsh now :) 12f7b3e
Screenshot 2024-10-30 at 6 53 33 PM

But Im not sure if it is using the newer pwsh or the slower one

setTimeout(() => {
traceVerbose(`Execution timed out, falling back to sendText: ${commandLine}`);
terminal.sendText(commandLine);
resolve(undefined);
return undefined;
}, 5000);
}),
]);
} else {
terminal.sendText(commandLine);
traceVerbose(`Shell Integration is disabled, sendText: ${commandLine}`);
Expand All @@ -136,7 +144,7 @@ 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 },
env: {},
hideFromUser: this.options?.hideFromUser,
});
this.terminalAutoActivator.disableAutoActivation(this.terminal);
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
134 changes: 67 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,81 @@ 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 configuration = vscode.workspace.getConfiguration('python');
await configuration.update('REPL.sendToNativeREPL', false, vscode.ConfigurationTarget.Global);

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',
);
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, 10_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();
}, 5000);
});
}

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`);
}
});
}
});
Loading