From d7f954d6910c6a0edb6d3ac032dddeb5ba98a535 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 16 Aug 2023 16:42:25 -0700 Subject: [PATCH 1/7] Support for Create Env command to re-create env --- src/client/common/utils/localize.ts | 7 + .../creation/common/commonUtils.ts | 11 ++ .../creation/provider/venvCreationProvider.ts | 13 +- .../creation/provider/venvDeleteUtils.ts | 99 ++++++++++++ .../creation/provider/venvSwitchPython.ts | 28 ++++ .../creation/provider/venvUtils.ts | 80 +++++++++- .../venvCreationProvider.unit.test.ts | 4 + .../provider/venvDeleteUtils.unit.test.ts | 142 ++++++++++++++++++ .../creation/provider/venvUtils.unit.test.ts | 105 ++++++++++++- 9 files changed, 482 insertions(+), 7 deletions(-) create mode 100644 src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts create mode 100644 src/client/pythonEnvironments/creation/provider/venvSwitchPython.ts create mode 100644 src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 3ec1829201f8..4401cab56176 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -464,6 +464,13 @@ export namespace CreateEnv { export const error = l10n.t('Creating virtual environment failed with error.'); export const tomlExtrasQuickPickTitle = l10n.t('Select optional dependencies to install from pyproject.toml'); export const requirementsQuickPickTitle = l10n.t('Select dependencies to install'); + export const recreate = l10n.t('Recreate'); + export const recreateDescription = l10n.t('Delete existing ".venv" environment and create a new one'); + export const useExisting = l10n.t('Use Existing'); + export const useExistingDescription = l10n.t('Use existing ".venv" environment with no changes to it'); + export const existingVenvQuickPickPlaceholder = l10n.t('Use or Recreate existing environment?'); + export const deletingEnvironmentProgress = l10n.t('Deleting existing ".venv" environment...'); + export const errorDeletingEnvironment = l10n.t('Error while deleting existing ".venv" environment.'); } export namespace Conda { diff --git a/src/client/pythonEnvironments/creation/common/commonUtils.ts b/src/client/pythonEnvironments/creation/common/commonUtils.ts index 0e303e70138e..583642d32b94 100644 --- a/src/client/pythonEnvironments/creation/common/commonUtils.ts +++ b/src/client/pythonEnvironments/creation/common/commonUtils.ts @@ -1,5 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import * as fs from 'fs-extra'; +import * as path from 'path'; +import { WorkspaceFolder } from 'vscode'; import { Commands } from '../../../common/constants'; import { Common } from '../../../common/utils/localize'; import { executeCommand } from '../../../common/vscodeApis/commandApis'; @@ -13,3 +16,11 @@ export async function showErrorMessageWithLogs(message: string): Promise { await executeCommand(Commands.Set_Interpreter); } } + +export function getVenvPath(workspaceFolder: WorkspaceFolder): string { + return path.join(workspaceFolder.uri.fsPath, '.venv'); +} + +export async function hasVenv(workspaceFolder: WorkspaceFolder): Promise { + return fs.pathExists(getVenvPath(workspaceFolder)); +} diff --git a/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts b/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts index 6c310fd68331..c86914015cb0 100644 --- a/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts +++ b/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts @@ -18,7 +18,7 @@ import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { VenvProgressAndTelemetry, VENV_CREATED_MARKER, VENV_EXISTING_MARKER } from './venvProgressAndTelemetry'; import { showErrorMessageWithLogs } from '../common/commonUtils'; -import { IPackageInstallSelection, pickPackagesToInstall } from './venvUtils'; +import { IPackageInstallSelection, pickExistingVenvAction, pickPackagesToInstall } from './venvUtils'; import { InputFlowAction } from '../../../common/utils/multiStepInput'; import { CreateEnvironmentProvider, @@ -191,6 +191,13 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { ); workspaceStep.next = interpreterStep; + const existingEnvStep = new MultiStepNode( + interpreterStep, + async (context?: MultiStepAction) => pickExistingVenvAction(workspace, interpreter, context), + undefined, + ); + interpreterStep.next = existingEnvStep; + let addGitIgnore = true; let installPackages = true; if (options) { @@ -199,7 +206,7 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { } let installInfo: IPackageInstallSelection[] | undefined; const packagesStep = new MultiStepNode( - interpreterStep, + existingEnvStep, async () => { if (workspace && installPackages) { try { @@ -220,7 +227,7 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { }, undefined, ); - interpreterStep.next = packagesStep; + existingEnvStep.next = packagesStep; const action = await MultiStepNode.run(workspaceStep); if (action === MultiStepAction.Back || action === MultiStepAction.Cancel) { diff --git a/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts b/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts new file mode 100644 index 000000000000..7c154b71fc9b --- /dev/null +++ b/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts @@ -0,0 +1,99 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as fs from 'fs-extra'; +import * as path from 'path'; +import { WorkspaceFolder } from 'vscode'; +import { traceError, traceInfo } from '../../../logging'; +import { getVenvPath, showErrorMessageWithLogs } from '../common/commonUtils'; +import { CreateEnv } from '../../../common/utils/localize'; +import { sleep } from '../../../common/utils/async'; +import { switchSelectedPython } from './venvSwitchPython'; + +async function tryDeleteFile(file: string): Promise { + try { + if (!(await fs.pathExists(file))) { + return true; + } + await fs.unlink(file); + return true; + } catch (err) { + traceError(`Failed to delete file [${file}]:`, err); + return false; + } +} + +async function tryDeleteDir(dir: string): Promise { + try { + if (!(await fs.pathExists(dir))) { + return true; + } + await fs.rmdir(dir, { + recursive: true, + maxRetries: 10, + retryDelay: 200, + }); + return true; + } catch (err) { + traceError(`Failed to delete directory [${dir}]:`, err); + return false; + } +} + +export async function deleteEnvironmentNonWindows(workspaceFolder: WorkspaceFolder): Promise { + const venvPath = getVenvPath(workspaceFolder); + if (await tryDeleteDir(venvPath)) { + traceInfo(`Deleted venv dir: ${venvPath}`); + return true; + } + showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); + return false; +} + +export async function deleteEnvironmentWindows( + workspaceFolder: WorkspaceFolder, + interpreter: string | undefined, +): Promise { + const venvPath = getVenvPath(workspaceFolder); + const venvPythonPath = path.join(venvPath, 'Scripts', 'python.exe'); + + if (await tryDeleteFile(venvPythonPath)) { + traceInfo(`Deleted python executable: ${venvPythonPath}`); + if (await tryDeleteDir(venvPath)) { + traceInfo(`Deleted ".venv" dir: ${venvPath}`); + return true; + } + + traceError(`Failed to delete ".venv" dir: ${venvPath}`); + traceError( + 'This happens if the virtual environment is still in use, or some binary in the venv is still running.', + ); + traceError(`Please delete the ".venv" manually: [${venvPath}]`); + showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); + return false; + } + traceError(`Failed to delete python executable: ${venvPythonPath}`); + traceError('This happens if the virtual environment is still in use.'); + + if (interpreter) { + traceError('We will attempt to switch python temporarily to delete the ".venv"'); + + await switchSelectedPython(interpreter, workspaceFolder.uri, 'temporarily to delete the ".venv"'); + + traceInfo(`Attempting to delete ".venv" again: ${venvPath}`); + const ms = 500; + for (let i = 0; i < 5; i = i + 1) { + traceInfo(`Waiting for ${ms}ms to let processes exit, before a delete attempt.`); + await sleep(ms); + if (await tryDeleteDir(venvPath)) { + traceInfo(`Deleted ".venv" dir: ${venvPath}`); + return true; + } + traceError(`Failed to delete ".venv" dir [${venvPath}] (attempt ${i + 1}/5).`); + } + } else { + traceError(`Please delete the ".venv" dir manually: [${venvPath}] manually.`); + } + showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); + return false; +} diff --git a/src/client/pythonEnvironments/creation/provider/venvSwitchPython.ts b/src/client/pythonEnvironments/creation/provider/venvSwitchPython.ts new file mode 100644 index 000000000000..e2567dfd114b --- /dev/null +++ b/src/client/pythonEnvironments/creation/provider/venvSwitchPython.ts @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as path from 'path'; +import { Disposable, Uri } from 'vscode'; +import { createDeferred } from '../../../common/utils/async'; +import { getExtension } from '../../../common/vscodeApis/extensionsApi'; +import { PVSC_EXTENSION_ID, PythonExtension } from '../../../api/types'; +import { traceInfo } from '../../../logging'; + +export async function switchSelectedPython(interpreter: string, uri: Uri, purpose: string): Promise { + let dispose: Disposable | undefined; + try { + const deferred = createDeferred(); + const api: PythonExtension = getExtension(PVSC_EXTENSION_ID)?.exports as PythonExtension; + dispose = api.environments.onDidChangeActiveEnvironmentPath(async (e) => { + if (path.normalize(e.path) === path.normalize(interpreter)) { + traceInfo(`Switched to interpreter ${purpose}: ${interpreter}`); + deferred.resolve(); + } + }); + api.environments.updateActiveEnvironmentPath(interpreter, uri); + traceInfo(`Switching interpreter ${purpose}: ${interpreter}`); + await deferred.promise; + } finally { + dispose?.dispose(); + } +} diff --git a/src/client/pythonEnvironments/creation/provider/venvUtils.ts b/src/client/pythonEnvironments/creation/provider/venvUtils.ts index 7c6505082fbc..ec8285165af4 100644 --- a/src/client/pythonEnvironments/creation/provider/venvUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvUtils.ts @@ -5,11 +5,20 @@ import * as tomljs from '@iarna/toml'; import * as fs from 'fs-extra'; import { flatten, isArray } from 'lodash'; import * as path from 'path'; -import { CancellationToken, QuickPickItem, RelativePattern, WorkspaceFolder } from 'vscode'; -import { CreateEnv } from '../../../common/utils/localize'; -import { MultiStepAction, MultiStepNode, showQuickPickWithBack } from '../../../common/vscodeApis/windowApis'; +import { CancellationToken, ProgressLocation, QuickPickItem, RelativePattern, WorkspaceFolder } from 'vscode'; +import { Common, CreateEnv } from '../../../common/utils/localize'; +import { + MultiStepAction, + MultiStepNode, + showQuickPickWithBack, + withProgress, +} from '../../../common/vscodeApis/windowApis'; import { findFiles } from '../../../common/vscodeApis/workspaceApis'; import { traceError, traceVerbose } from '../../../logging'; +import { Commands } from '../../../common/constants'; +import { isWindows } from '../../../common/platform/platformService'; +import { getVenvPath, hasVenv } from '../common/commonUtils'; +import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './venvDeleteUtils'; const exclude = '**/{.venv*,.git,.nox,.tox,.conda,site-packages,__pypackages__}/**'; async function getPipRequirementsFiles( @@ -226,3 +235,68 @@ export async function pickPackagesToInstall( return packages; } + +async function deleteEnvironment(workspaceFolder: WorkspaceFolder, interpreter: string | undefined): Promise { + const venvPath = getVenvPath(workspaceFolder); + return withProgress( + { + location: ProgressLocation.Notification, + title: `${CreateEnv.Venv.deletingEnvironmentProgress} ([${Common.showLogs}](command:${Commands.ViewOutput})): ${venvPath}`, + cancellable: false, + }, + async () => { + if (isWindows()) { + return deleteEnvironmentWindows(workspaceFolder, interpreter); + } + return deleteEnvironmentNonWindows(workspaceFolder); + }, + ); +} + +export async function pickExistingVenvAction( + workspaceFolder: WorkspaceFolder | undefined, + interpreter: string | undefined, + context?: MultiStepAction, +): Promise { + if (workspaceFolder && (await hasVenv(workspaceFolder)) && context === MultiStepAction.Continue) { + const items: QuickPickItem[] = [ + { label: CreateEnv.Venv.recreate, description: CreateEnv.Venv.recreateDescription }, + { + label: CreateEnv.Venv.useExisting, + description: CreateEnv.Venv.useExistingDescription, + }, + ]; + + let selection: QuickPickItem | undefined; + try { + selection = (await showQuickPickWithBack( + items, + { + placeHolder: CreateEnv.Venv.existingVenvQuickPickPlaceholder, + ignoreFocusOut: true, + }, + undefined, + )) as QuickPickItem | undefined; + } catch (ex) { + return MultiStepAction.Back; + } + + if (selection === undefined) { + return MultiStepAction.Cancel; + } + + if (selection.label === CreateEnv.Venv.recreate) { + if (await deleteEnvironment(workspaceFolder, interpreter)) { + return MultiStepAction.Continue; + } + return MultiStepAction.Cancel; + } + + if (selection.label === CreateEnv.Venv.useExisting) { + return MultiStepAction.Continue; + } + } else if (context === MultiStepAction.Back) { + return MultiStepAction.Back; + } + return MultiStepAction.Continue; +} diff --git a/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts index 5bd325c51a0f..9a387823c5a7 100644 --- a/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts @@ -35,6 +35,7 @@ suite('venv Creation provider tests', () => { let withProgressStub: sinon.SinonStub; let showErrorMessageWithLogsStub: sinon.SinonStub; let pickPackagesToInstallStub: sinon.SinonStub; + let pickExistingVenvActionStub: sinon.SinonStub; const workspace1 = { uri: Uri.file(path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'testMultiRootWkspc', 'workspace1')), @@ -43,6 +44,7 @@ suite('venv Creation provider tests', () => { }; setup(() => { + pickExistingVenvActionStub = sinon.stub(venvUtils, 'pickExistingVenvAction'); pickWorkspaceFolderStub = sinon.stub(wsSelect, 'pickWorkspaceFolder'); execObservableStub = sinon.stub(rawProcessApis, 'execObservable'); interpreterQuickPick = typemoq.Mock.ofType(); @@ -54,6 +56,8 @@ suite('venv Creation provider tests', () => { progressMock = typemoq.Mock.ofType(); venvProvider = new VenvCreationProvider(interpreterQuickPick.object); + + pickExistingVenvActionStub.resolves(windowApis.MultiStepAction.Continue); }); teardown(() => { diff --git a/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts new file mode 100644 index 000000000000..d075979b70b1 --- /dev/null +++ b/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts @@ -0,0 +1,142 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +import * as fs from 'fs-extra'; +import * as sinon from 'sinon'; +import { Uri, WorkspaceFolder } from 'vscode'; +import { assert } from 'chai'; +import * as path from 'path'; +import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../../../constants'; +import * as commonUtils from '../../../../client/pythonEnvironments/creation/common/commonUtils'; +import { + deleteEnvironmentNonWindows, + deleteEnvironmentWindows, +} from '../../../../client/pythonEnvironments/creation/provider/venvDeleteUtils'; +import * as switchPython from '../../../../client/pythonEnvironments/creation/provider/venvSwitchPython'; +import * as asyncApi from '../../../../client/common/utils/async'; + +suite('Test Delete environments (windows)', () => { + let pathExistsStub: sinon.SinonStub; + let rmdirStub: sinon.SinonStub; + let unlinkStub: sinon.SinonStub; + let showErrorMessageWithLogsStub: sinon.SinonStub; + let switchPythonStub: sinon.SinonStub; + let sleepStub: sinon.SinonStub; + + const workspace1: WorkspaceFolder = { + uri: Uri.file(path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'testMultiRootWkspc', 'workspace1')), + name: 'workspace1', + index: 0, + }; + + setup(() => { + pathExistsStub = sinon.stub(fs, 'pathExists'); + pathExistsStub.resolves(true); + + rmdirStub = sinon.stub(fs, 'rmdir'); + unlinkStub = sinon.stub(fs, 'unlink'); + + sleepStub = sinon.stub(asyncApi, 'sleep'); + sleepStub.resolves(); + + showErrorMessageWithLogsStub = sinon.stub(commonUtils, 'showErrorMessageWithLogs'); + showErrorMessageWithLogsStub.resolves(); + + switchPythonStub = sinon.stub(switchPython, 'switchSelectedPython'); + switchPythonStub.resolves(); + }); + + teardown(() => { + sinon.restore(); + }); + + test('Delete venv folder succeeded', async () => { + rmdirStub.resolves(); + unlinkStub.resolves(); + assert.ok(await deleteEnvironmentWindows(workspace1, 'python.exe')); + + assert.ok(rmdirStub.calledOnce); + assert.ok(unlinkStub.calledOnce); + assert.ok(showErrorMessageWithLogsStub.notCalled); + }); + + test('Delete python.exe succeeded but venv dir failed', async () => { + rmdirStub.rejects(); + unlinkStub.resolves(); + assert.notOk(await deleteEnvironmentWindows(workspace1, 'python.exe')); + + assert.ok(rmdirStub.calledOnce); + assert.ok(unlinkStub.calledOnce); + assert.ok(showErrorMessageWithLogsStub.calledOnce); + }); + + test('Delete python.exe failed first attempt', async () => { + unlinkStub.rejects(); + rmdirStub.resolves(); + assert.ok(await deleteEnvironmentWindows(workspace1, 'python.exe')); + + assert.ok(rmdirStub.calledOnce); + assert.ok(switchPythonStub.calledOnce); + assert.ok(showErrorMessageWithLogsStub.notCalled); + }); + + test('Delete python.exe failed all attempts', async () => { + unlinkStub.rejects(); + rmdirStub.rejects(); + assert.notOk(await deleteEnvironmentWindows(workspace1, 'python.exe')); + assert.ok(switchPythonStub.calledOnce); + assert.ok(showErrorMessageWithLogsStub.calledOnce); + }); + + test('Delete python.exe failed no interpreter', async () => { + unlinkStub.rejects(); + rmdirStub.rejects(); + assert.notOk(await deleteEnvironmentWindows(workspace1, undefined)); + assert.ok(switchPythonStub.notCalled); + assert.ok(showErrorMessageWithLogsStub.calledOnce); + }); +}); + +suite('Test Delete environments (linux/mac)', () => { + let pathExistsStub: sinon.SinonStub; + let rmdirStub: sinon.SinonStub; + let showErrorMessageWithLogsStub: sinon.SinonStub; + + const workspace1: WorkspaceFolder = { + uri: Uri.file(path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'testMultiRootWkspc', 'workspace1')), + name: 'workspace1', + index: 0, + }; + + setup(() => { + pathExistsStub = sinon.stub(fs, 'pathExists'); + rmdirStub = sinon.stub(fs, 'rmdir'); + + showErrorMessageWithLogsStub = sinon.stub(commonUtils, 'showErrorMessageWithLogs'); + showErrorMessageWithLogsStub.resolves(); + }); + + teardown(() => { + sinon.restore(); + }); + + test('Delete venv folder succeeded', async () => { + pathExistsStub.resolves(true); + rmdirStub.resolves(); + + assert.ok(await deleteEnvironmentNonWindows(workspace1)); + + assert.ok(pathExistsStub.calledOnce); + assert.ok(rmdirStub.calledOnce); + assert.ok(showErrorMessageWithLogsStub.notCalled); + }); + + test('Delete venv folder failed', async () => { + pathExistsStub.resolves(true); + rmdirStub.rejects(); + assert.notOk(await deleteEnvironmentNonWindows(workspace1)); + + assert.ok(pathExistsStub.calledOnce); + assert.ok(rmdirStub.calledOnce); + assert.ok(showErrorMessageWithLogsStub.calledOnce); + }); +}); diff --git a/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts index 360bb43fad4b..dcdecfaeb776 100644 --- a/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts @@ -8,7 +8,10 @@ import { Uri } from 'vscode'; import * as path from 'path'; import * as windowApis from '../../../../client/common/vscodeApis/windowApis'; import * as workspaceApis from '../../../../client/common/vscodeApis/workspaceApis'; -import { pickPackagesToInstall } from '../../../../client/pythonEnvironments/creation/provider/venvUtils'; +import { + pickExistingVenvAction, + pickPackagesToInstall, +} from '../../../../client/pythonEnvironments/creation/provider/venvUtils'; import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../../../constants'; import { CreateEnv } from '../../../../client/common/utils/localize'; @@ -346,3 +349,103 @@ suite('Venv Utils test', () => { assert.isTrue(readFileStub.notCalled); }); }); + +suite('Test pick existing venv action', () => { + let withProgressStub: sinon.SinonStub; + let showQuickPickWithBackStub: sinon.SinonStub; + let pathExistsStub: sinon.SinonStub; + + const workspace1 = { + uri: Uri.file(path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'testMultiRootWkspc', 'workspace1')), + name: 'workspace1', + index: 0, + }; + + setup(() => { + pathExistsStub = sinon.stub(fs, 'pathExists'); + withProgressStub = sinon.stub(windowApis, 'withProgress'); + showQuickPickWithBackStub = sinon.stub(windowApis, 'showQuickPickWithBack'); + }); + teardown(() => { + sinon.restore(); + }); + + test('User selects existing venv', async () => { + pathExistsStub.resolves(true); + showQuickPickWithBackStub.resolves({ + label: CreateEnv.Venv.useExisting, + description: CreateEnv.Venv.useExistingDescription, + }); + const actual = await pickExistingVenvAction( + workspace1, + '/path/to/global/interpreter', + windowApis.MultiStepAction.Continue, + ); + assert.deepStrictEqual(actual, windowApis.MultiStepAction.Continue); + }); + + test('User presses escape', async () => { + pathExistsStub.resolves(true); + showQuickPickWithBackStub.resolves(undefined); + const actual = await pickExistingVenvAction( + workspace1, + '/path/to/global/interpreter', + windowApis.MultiStepAction.Continue, + ); + assert.deepStrictEqual(actual, windowApis.MultiStepAction.Cancel); + }); + + test('User selects delete venv and it succeeds', async () => { + pathExistsStub.resolves(true); + showQuickPickWithBackStub.resolves({ + label: CreateEnv.Venv.recreate, + description: CreateEnv.Venv.recreateDescription, + }); + withProgressStub.resolves(true); + const actual = await pickExistingVenvAction( + workspace1, + '/path/to/global/interpreter', + windowApis.MultiStepAction.Continue, + ); + assert.deepStrictEqual(actual, windowApis.MultiStepAction.Continue); + }); + + test('User selects delete venv and it fails', async () => { + pathExistsStub.resolves(true); + showQuickPickWithBackStub.resolves({ + label: CreateEnv.Venv.recreate, + description: CreateEnv.Venv.recreateDescription, + }); + withProgressStub.resolves(false); + const actual = await pickExistingVenvAction( + workspace1, + '/path/to/global/interpreter', + windowApis.MultiStepAction.Continue, + ); + assert.deepStrictEqual(actual, windowApis.MultiStepAction.Cancel); + }); + test('User clicks on back', async () => { + pathExistsStub.resolves(true); + // We use reject with "Back" to simulate the user clicking on back. + showQuickPickWithBackStub.rejects(windowApis.MultiStepAction.Back); + withProgressStub.resolves(false); + const actual = await pickExistingVenvAction( + workspace1, + '/path/to/global/interpreter', + windowApis.MultiStepAction.Continue, + ); + assert.deepStrictEqual(actual, windowApis.MultiStepAction.Back); + }); + test('User clicks on back from the next quick pick', async () => { + pathExistsStub.resolves(true); + showQuickPickWithBackStub.resolves(undefined); + withProgressStub.resolves(false); + const actual = await pickExistingVenvAction( + workspace1, + '/path/to/global/interpreter', + windowApis.MultiStepAction.Back, + ); + assert.deepStrictEqual(actual, windowApis.MultiStepAction.Back); + assert.ok(showQuickPickWithBackStub.notCalled); + }); +}); From 94f04b389f1e6f0ed17c9b0d00a7aaaff169e0f4 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 16 Aug 2023 18:06:25 -0700 Subject: [PATCH 2/7] Telemetry --- .../creation/provider/venvUtils.ts | 13 ++++++++++ src/client/telemetry/constants.ts | 2 ++ src/client/telemetry/index.ts | 24 +++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/src/client/pythonEnvironments/creation/provider/venvUtils.ts b/src/client/pythonEnvironments/creation/provider/venvUtils.ts index ec8285165af4..c0e2ec7b5cb3 100644 --- a/src/client/pythonEnvironments/creation/provider/venvUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvUtils.ts @@ -19,6 +19,8 @@ import { Commands } from '../../../common/constants'; import { isWindows } from '../../../common/platform/platformService'; import { getVenvPath, hasVenv } from '../common/commonUtils'; import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './venvDeleteUtils'; +import { sendTelemetryEvent } from '../../../telemetry'; +import { EventName } from '../../../telemetry/constants'; const exclude = '**/{.venv*,.git,.nox,.tox,.conda,site-packages,__pypackages__}/**'; async function getPipRequirementsFiles( @@ -287,12 +289,23 @@ export async function pickExistingVenvAction( if (selection.label === CreateEnv.Venv.recreate) { if (await deleteEnvironment(workspaceFolder, interpreter)) { + sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { + environmentType: 'venv', + status: 'deleted', + }); return MultiStepAction.Continue; } + sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { + environmentType: 'venv', + status: 'failed', + }); return MultiStepAction.Cancel; } if (selection.label === CreateEnv.Venv.useExisting) { + sendTelemetryEvent(EventName.ENVIRONMENT_REUSE, undefined, { + environmentType: 'venv', + }); return MultiStepAction.Continue; } } else if (context === MultiStepAction.Back) { diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 159f5690e5c5..a729b3d491e8 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -112,6 +112,8 @@ export enum EventName { ENVIRONMENT_INSTALLED_PACKAGES = 'ENVIRONMENT.INSTALLED_PACKAGES', ENVIRONMENT_INSTALLING_PACKAGES_FAILED = 'ENVIRONMENT.INSTALLING_PACKAGES_FAILED', ENVIRONMENT_BUTTON = 'ENVIRONMENT.BUTTON', + ENVIRONMENT_DELETE = 'ENVIRONMENT.DELETE', + ENVIRONMENT_REUSE = 'ENVIRONMENT.REUSE', TOOLS_EXTENSIONS_ALREADY_INSTALLED = 'TOOLS_EXTENSIONS.ALREADY_INSTALLED', TOOLS_EXTENSIONS_PROMPT_SHOWN = 'TOOLS_EXTENSIONS.PROMPT_SHOWN', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index a068f21d2327..e8e7d682788c 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -2120,6 +2120,30 @@ export interface IEventNamePropertyMapping { "environment.button" : {"owner": "karthiknadig" } */ [EventName.ENVIRONMENT_BUTTON]: never | undefined; + /** + * Telemetry event if user selected to delete the existing environment. + */ + /* __GDPR__ + "environment.delete" : { + "environmentType" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "owner": "karthiknadig" }, + "status" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "owner": "karthiknadig" } + } + */ + [EventName.ENVIRONMENT_DELETE]: { + environmentType: 'venv' | 'conda'; + status: 'deleted' | 'failed'; + }; + /** + * Telemetry event if user selected to re-use the existing environment. + */ + /* __GDPR__ + "environment.reuse" : { + "environmentType" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "owner": "karthiknadig" } + } + */ + [EventName.ENVIRONMENT_REUSE]: { + environmentType: 'venv' | 'conda'; + }; /** * Telemetry event sent when a linter or formatter extension is already installed. */ From eb9569a0b69b2e25f3e4ae0a4424d2f2af2fc02b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 16 Aug 2023 18:22:30 -0700 Subject: [PATCH 3/7] Refactor --- .../{venvDeleteUtils.ts => envDeleteUtils.ts} | 46 +++++++++---------- .../creation/provider/venvUtils.ts | 7 +-- .../provider/venvDeleteUtils.unit.test.ts | 19 ++++---- 3 files changed, 38 insertions(+), 34 deletions(-) rename src/client/pythonEnvironments/creation/provider/{venvDeleteUtils.ts => envDeleteUtils.ts} (63%) diff --git a/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts b/src/client/pythonEnvironments/creation/provider/envDeleteUtils.ts similarity index 63% rename from src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts rename to src/client/pythonEnvironments/creation/provider/envDeleteUtils.ts index 7c154b71fc9b..a87ccabdfb2d 100644 --- a/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/envDeleteUtils.ts @@ -5,7 +5,7 @@ import * as fs from 'fs-extra'; import * as path from 'path'; import { WorkspaceFolder } from 'vscode'; import { traceError, traceInfo } from '../../../logging'; -import { getVenvPath, showErrorMessageWithLogs } from '../common/commonUtils'; +import { showErrorMessageWithLogs } from '../common/commonUtils'; import { CreateEnv } from '../../../common/utils/localize'; import { sleep } from '../../../common/utils/async'; import { switchSelectedPython } from './venvSwitchPython'; @@ -40,10 +40,10 @@ async function tryDeleteDir(dir: string): Promise { } } -export async function deleteEnvironmentNonWindows(workspaceFolder: WorkspaceFolder): Promise { - const venvPath = getVenvPath(workspaceFolder); - if (await tryDeleteDir(venvPath)) { - traceInfo(`Deleted venv dir: ${venvPath}`); +export async function deleteEnvironmentNonWindows(envDir: string): Promise { + const name = path.basename(envDir); + if (await tryDeleteDir(envDir)) { + traceInfo(`Deleted "${name}" dir: ${envDir}`); return true; } showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); @@ -51,48 +51,48 @@ export async function deleteEnvironmentNonWindows(workspaceFolder: WorkspaceFold } export async function deleteEnvironmentWindows( + envDir: string, + envPythonBin: string, workspaceFolder: WorkspaceFolder, interpreter: string | undefined, ): Promise { - const venvPath = getVenvPath(workspaceFolder); - const venvPythonPath = path.join(venvPath, 'Scripts', 'python.exe'); - - if (await tryDeleteFile(venvPythonPath)) { - traceInfo(`Deleted python executable: ${venvPythonPath}`); - if (await tryDeleteDir(venvPath)) { - traceInfo(`Deleted ".venv" dir: ${venvPath}`); + const name = path.basename(envDir); + if (await tryDeleteFile(envPythonBin)) { + traceInfo(`Deleted python executable: ${envPythonBin}`); + if (await tryDeleteDir(envDir)) { + traceInfo(`Deleted "${name}" dir: ${envDir}`); return true; } - traceError(`Failed to delete ".venv" dir: ${venvPath}`); + traceError(`Failed to delete "${name}" dir: ${envDir}`); traceError( - 'This happens if the virtual environment is still in use, or some binary in the venv is still running.', + 'This happens if the virtual environment is still in use, or some binary in the "${name}" is still running.', ); - traceError(`Please delete the ".venv" manually: [${venvPath}]`); + traceError(`Please delete the "${name}" manually: [${envDir}]`); showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); return false; } - traceError(`Failed to delete python executable: ${venvPythonPath}`); + traceError(`Failed to delete python executable: ${envPythonBin}`); traceError('This happens if the virtual environment is still in use.'); if (interpreter) { - traceError('We will attempt to switch python temporarily to delete the ".venv"'); + traceError('We will attempt to switch python temporarily to delete the "${name}"'); - await switchSelectedPython(interpreter, workspaceFolder.uri, 'temporarily to delete the ".venv"'); + await switchSelectedPython(interpreter, workspaceFolder.uri, 'temporarily to delete the "${name}"'); - traceInfo(`Attempting to delete ".venv" again: ${venvPath}`); + traceInfo(`Attempting to delete "${name}" again: ${envDir}`); const ms = 500; for (let i = 0; i < 5; i = i + 1) { traceInfo(`Waiting for ${ms}ms to let processes exit, before a delete attempt.`); await sleep(ms); - if (await tryDeleteDir(venvPath)) { - traceInfo(`Deleted ".venv" dir: ${venvPath}`); + if (await tryDeleteDir(envDir)) { + traceInfo(`Deleted "${name}" dir: ${envDir}`); return true; } - traceError(`Failed to delete ".venv" dir [${venvPath}] (attempt ${i + 1}/5).`); + traceError(`Failed to delete "${name}" dir [${envDir}] (attempt ${i + 1}/5).`); } } else { - traceError(`Please delete the ".venv" dir manually: [${venvPath}] manually.`); + traceError(`Please delete the "${name}" dir manually: [${envDir}] manually.`); } showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); return false; diff --git a/src/client/pythonEnvironments/creation/provider/venvUtils.ts b/src/client/pythonEnvironments/creation/provider/venvUtils.ts index c0e2ec7b5cb3..c7364a16dcb5 100644 --- a/src/client/pythonEnvironments/creation/provider/venvUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvUtils.ts @@ -18,7 +18,7 @@ import { traceError, traceVerbose } from '../../../logging'; import { Commands } from '../../../common/constants'; import { isWindows } from '../../../common/platform/platformService'; import { getVenvPath, hasVenv } from '../common/commonUtils'; -import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './venvDeleteUtils'; +import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './envDeleteUtils'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; @@ -248,9 +248,10 @@ async function deleteEnvironment(workspaceFolder: WorkspaceFolder, interpreter: }, async () => { if (isWindows()) { - return deleteEnvironmentWindows(workspaceFolder, interpreter); + const venvPythonPath = path.join(venvPath, 'Scripts', 'python.exe'); + return deleteEnvironmentWindows(venvPath, venvPythonPath, workspaceFolder, interpreter); } - return deleteEnvironmentNonWindows(workspaceFolder); + return deleteEnvironmentNonWindows(venvPath); }, ); } diff --git a/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts index d075979b70b1..d9ada1268b8b 100644 --- a/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts @@ -10,7 +10,7 @@ import * as commonUtils from '../../../../client/pythonEnvironments/creation/com import { deleteEnvironmentNonWindows, deleteEnvironmentWindows, -} from '../../../../client/pythonEnvironments/creation/provider/venvDeleteUtils'; +} from '../../../../client/pythonEnvironments/creation/provider/envDeleteUtils'; import * as switchPython from '../../../../client/pythonEnvironments/creation/provider/venvSwitchPython'; import * as asyncApi from '../../../../client/common/utils/async'; @@ -27,6 +27,8 @@ suite('Test Delete environments (windows)', () => { name: 'workspace1', index: 0, }; + const venvPath = path.join(workspace1.uri.fsPath, 'venv'); + const venvPythonPath = path.join(venvPath, 'scripts', 'python.exe'); setup(() => { pathExistsStub = sinon.stub(fs, 'pathExists'); @@ -52,7 +54,7 @@ suite('Test Delete environments (windows)', () => { test('Delete venv folder succeeded', async () => { rmdirStub.resolves(); unlinkStub.resolves(); - assert.ok(await deleteEnvironmentWindows(workspace1, 'python.exe')); + assert.ok(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); assert.ok(rmdirStub.calledOnce); assert.ok(unlinkStub.calledOnce); @@ -62,7 +64,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe succeeded but venv dir failed', async () => { rmdirStub.rejects(); unlinkStub.resolves(); - assert.notOk(await deleteEnvironmentWindows(workspace1, 'python.exe')); + assert.notOk(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); assert.ok(rmdirStub.calledOnce); assert.ok(unlinkStub.calledOnce); @@ -72,7 +74,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe failed first attempt', async () => { unlinkStub.rejects(); rmdirStub.resolves(); - assert.ok(await deleteEnvironmentWindows(workspace1, 'python.exe')); + assert.ok(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); assert.ok(rmdirStub.calledOnce); assert.ok(switchPythonStub.calledOnce); @@ -82,7 +84,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe failed all attempts', async () => { unlinkStub.rejects(); rmdirStub.rejects(); - assert.notOk(await deleteEnvironmentWindows(workspace1, 'python.exe')); + assert.notOk(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); assert.ok(switchPythonStub.calledOnce); assert.ok(showErrorMessageWithLogsStub.calledOnce); }); @@ -90,7 +92,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe failed no interpreter', async () => { unlinkStub.rejects(); rmdirStub.rejects(); - assert.notOk(await deleteEnvironmentWindows(workspace1, undefined)); + assert.notOk(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, undefined)); assert.ok(switchPythonStub.notCalled); assert.ok(showErrorMessageWithLogsStub.calledOnce); }); @@ -106,6 +108,7 @@ suite('Test Delete environments (linux/mac)', () => { name: 'workspace1', index: 0, }; + const venvPath = path.join(workspace1.uri.fsPath, 'venv'); setup(() => { pathExistsStub = sinon.stub(fs, 'pathExists'); @@ -123,7 +126,7 @@ suite('Test Delete environments (linux/mac)', () => { pathExistsStub.resolves(true); rmdirStub.resolves(); - assert.ok(await deleteEnvironmentNonWindows(workspace1)); + assert.ok(await deleteEnvironmentNonWindows(venvPath)); assert.ok(pathExistsStub.calledOnce); assert.ok(rmdirStub.calledOnce); @@ -133,7 +136,7 @@ suite('Test Delete environments (linux/mac)', () => { test('Delete venv folder failed', async () => { pathExistsStub.resolves(true); rmdirStub.rejects(); - assert.notOk(await deleteEnvironmentNonWindows(workspace1)); + assert.notOk(await deleteEnvironmentNonWindows(venvPath)); assert.ok(pathExistsStub.calledOnce); assert.ok(rmdirStub.calledOnce); From 206c37e7cd20ec4cc602e43d2fad10cf7429f0f2 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 16 Aug 2023 19:19:08 -0700 Subject: [PATCH 4/7] Revert "Refactor" This reverts commit eb9569a0b69b2e25f3e4ae0a4424d2f2af2fc02b. --- .../{envDeleteUtils.ts => venvDeleteUtils.ts} | 46 +++++++++---------- .../creation/provider/venvUtils.ts | 7 ++- .../provider/venvDeleteUtils.unit.test.ts | 19 ++++---- 3 files changed, 34 insertions(+), 38 deletions(-) rename src/client/pythonEnvironments/creation/provider/{envDeleteUtils.ts => venvDeleteUtils.ts} (63%) diff --git a/src/client/pythonEnvironments/creation/provider/envDeleteUtils.ts b/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts similarity index 63% rename from src/client/pythonEnvironments/creation/provider/envDeleteUtils.ts rename to src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts index a87ccabdfb2d..7c154b71fc9b 100644 --- a/src/client/pythonEnvironments/creation/provider/envDeleteUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts @@ -5,7 +5,7 @@ import * as fs from 'fs-extra'; import * as path from 'path'; import { WorkspaceFolder } from 'vscode'; import { traceError, traceInfo } from '../../../logging'; -import { showErrorMessageWithLogs } from '../common/commonUtils'; +import { getVenvPath, showErrorMessageWithLogs } from '../common/commonUtils'; import { CreateEnv } from '../../../common/utils/localize'; import { sleep } from '../../../common/utils/async'; import { switchSelectedPython } from './venvSwitchPython'; @@ -40,10 +40,10 @@ async function tryDeleteDir(dir: string): Promise { } } -export async function deleteEnvironmentNonWindows(envDir: string): Promise { - const name = path.basename(envDir); - if (await tryDeleteDir(envDir)) { - traceInfo(`Deleted "${name}" dir: ${envDir}`); +export async function deleteEnvironmentNonWindows(workspaceFolder: WorkspaceFolder): Promise { + const venvPath = getVenvPath(workspaceFolder); + if (await tryDeleteDir(venvPath)) { + traceInfo(`Deleted venv dir: ${venvPath}`); return true; } showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); @@ -51,48 +51,48 @@ export async function deleteEnvironmentNonWindows(envDir: string): Promise { - const name = path.basename(envDir); - if (await tryDeleteFile(envPythonBin)) { - traceInfo(`Deleted python executable: ${envPythonBin}`); - if (await tryDeleteDir(envDir)) { - traceInfo(`Deleted "${name}" dir: ${envDir}`); + const venvPath = getVenvPath(workspaceFolder); + const venvPythonPath = path.join(venvPath, 'Scripts', 'python.exe'); + + if (await tryDeleteFile(venvPythonPath)) { + traceInfo(`Deleted python executable: ${venvPythonPath}`); + if (await tryDeleteDir(venvPath)) { + traceInfo(`Deleted ".venv" dir: ${venvPath}`); return true; } - traceError(`Failed to delete "${name}" dir: ${envDir}`); + traceError(`Failed to delete ".venv" dir: ${venvPath}`); traceError( - 'This happens if the virtual environment is still in use, or some binary in the "${name}" is still running.', + 'This happens if the virtual environment is still in use, or some binary in the venv is still running.', ); - traceError(`Please delete the "${name}" manually: [${envDir}]`); + traceError(`Please delete the ".venv" manually: [${venvPath}]`); showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); return false; } - traceError(`Failed to delete python executable: ${envPythonBin}`); + traceError(`Failed to delete python executable: ${venvPythonPath}`); traceError('This happens if the virtual environment is still in use.'); if (interpreter) { - traceError('We will attempt to switch python temporarily to delete the "${name}"'); + traceError('We will attempt to switch python temporarily to delete the ".venv"'); - await switchSelectedPython(interpreter, workspaceFolder.uri, 'temporarily to delete the "${name}"'); + await switchSelectedPython(interpreter, workspaceFolder.uri, 'temporarily to delete the ".venv"'); - traceInfo(`Attempting to delete "${name}" again: ${envDir}`); + traceInfo(`Attempting to delete ".venv" again: ${venvPath}`); const ms = 500; for (let i = 0; i < 5; i = i + 1) { traceInfo(`Waiting for ${ms}ms to let processes exit, before a delete attempt.`); await sleep(ms); - if (await tryDeleteDir(envDir)) { - traceInfo(`Deleted "${name}" dir: ${envDir}`); + if (await tryDeleteDir(venvPath)) { + traceInfo(`Deleted ".venv" dir: ${venvPath}`); return true; } - traceError(`Failed to delete "${name}" dir [${envDir}] (attempt ${i + 1}/5).`); + traceError(`Failed to delete ".venv" dir [${venvPath}] (attempt ${i + 1}/5).`); } } else { - traceError(`Please delete the "${name}" dir manually: [${envDir}] manually.`); + traceError(`Please delete the ".venv" dir manually: [${venvPath}] manually.`); } showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); return false; diff --git a/src/client/pythonEnvironments/creation/provider/venvUtils.ts b/src/client/pythonEnvironments/creation/provider/venvUtils.ts index c7364a16dcb5..c0e2ec7b5cb3 100644 --- a/src/client/pythonEnvironments/creation/provider/venvUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvUtils.ts @@ -18,7 +18,7 @@ import { traceError, traceVerbose } from '../../../logging'; import { Commands } from '../../../common/constants'; import { isWindows } from '../../../common/platform/platformService'; import { getVenvPath, hasVenv } from '../common/commonUtils'; -import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './envDeleteUtils'; +import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './venvDeleteUtils'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; @@ -248,10 +248,9 @@ async function deleteEnvironment(workspaceFolder: WorkspaceFolder, interpreter: }, async () => { if (isWindows()) { - const venvPythonPath = path.join(venvPath, 'Scripts', 'python.exe'); - return deleteEnvironmentWindows(venvPath, venvPythonPath, workspaceFolder, interpreter); + return deleteEnvironmentWindows(workspaceFolder, interpreter); } - return deleteEnvironmentNonWindows(venvPath); + return deleteEnvironmentNonWindows(workspaceFolder); }, ); } diff --git a/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts index d9ada1268b8b..d075979b70b1 100644 --- a/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvDeleteUtils.unit.test.ts @@ -10,7 +10,7 @@ import * as commonUtils from '../../../../client/pythonEnvironments/creation/com import { deleteEnvironmentNonWindows, deleteEnvironmentWindows, -} from '../../../../client/pythonEnvironments/creation/provider/envDeleteUtils'; +} from '../../../../client/pythonEnvironments/creation/provider/venvDeleteUtils'; import * as switchPython from '../../../../client/pythonEnvironments/creation/provider/venvSwitchPython'; import * as asyncApi from '../../../../client/common/utils/async'; @@ -27,8 +27,6 @@ suite('Test Delete environments (windows)', () => { name: 'workspace1', index: 0, }; - const venvPath = path.join(workspace1.uri.fsPath, 'venv'); - const venvPythonPath = path.join(venvPath, 'scripts', 'python.exe'); setup(() => { pathExistsStub = sinon.stub(fs, 'pathExists'); @@ -54,7 +52,7 @@ suite('Test Delete environments (windows)', () => { test('Delete venv folder succeeded', async () => { rmdirStub.resolves(); unlinkStub.resolves(); - assert.ok(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); + assert.ok(await deleteEnvironmentWindows(workspace1, 'python.exe')); assert.ok(rmdirStub.calledOnce); assert.ok(unlinkStub.calledOnce); @@ -64,7 +62,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe succeeded but venv dir failed', async () => { rmdirStub.rejects(); unlinkStub.resolves(); - assert.notOk(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); + assert.notOk(await deleteEnvironmentWindows(workspace1, 'python.exe')); assert.ok(rmdirStub.calledOnce); assert.ok(unlinkStub.calledOnce); @@ -74,7 +72,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe failed first attempt', async () => { unlinkStub.rejects(); rmdirStub.resolves(); - assert.ok(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); + assert.ok(await deleteEnvironmentWindows(workspace1, 'python.exe')); assert.ok(rmdirStub.calledOnce); assert.ok(switchPythonStub.calledOnce); @@ -84,7 +82,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe failed all attempts', async () => { unlinkStub.rejects(); rmdirStub.rejects(); - assert.notOk(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, 'python.exe')); + assert.notOk(await deleteEnvironmentWindows(workspace1, 'python.exe')); assert.ok(switchPythonStub.calledOnce); assert.ok(showErrorMessageWithLogsStub.calledOnce); }); @@ -92,7 +90,7 @@ suite('Test Delete environments (windows)', () => { test('Delete python.exe failed no interpreter', async () => { unlinkStub.rejects(); rmdirStub.rejects(); - assert.notOk(await deleteEnvironmentWindows(venvPath, venvPythonPath, workspace1, undefined)); + assert.notOk(await deleteEnvironmentWindows(workspace1, undefined)); assert.ok(switchPythonStub.notCalled); assert.ok(showErrorMessageWithLogsStub.calledOnce); }); @@ -108,7 +106,6 @@ suite('Test Delete environments (linux/mac)', () => { name: 'workspace1', index: 0, }; - const venvPath = path.join(workspace1.uri.fsPath, 'venv'); setup(() => { pathExistsStub = sinon.stub(fs, 'pathExists'); @@ -126,7 +123,7 @@ suite('Test Delete environments (linux/mac)', () => { pathExistsStub.resolves(true); rmdirStub.resolves(); - assert.ok(await deleteEnvironmentNonWindows(venvPath)); + assert.ok(await deleteEnvironmentNonWindows(workspace1)); assert.ok(pathExistsStub.calledOnce); assert.ok(rmdirStub.calledOnce); @@ -136,7 +133,7 @@ suite('Test Delete environments (linux/mac)', () => { test('Delete venv folder failed', async () => { pathExistsStub.resolves(true); rmdirStub.rejects(); - assert.notOk(await deleteEnvironmentNonWindows(venvPath)); + assert.notOk(await deleteEnvironmentNonWindows(workspace1)); assert.ok(pathExistsStub.calledOnce); assert.ok(rmdirStub.calledOnce); From 5d9b448d3177049f932a8b33199f84b73b1f264d Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 17 Aug 2023 16:12:11 -0700 Subject: [PATCH 5/7] Fix typo --- .../pythonEnvironments/creation/provider/venvDeleteUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts b/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts index 7c154b71fc9b..46a0adf0f228 100644 --- a/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvDeleteUtils.ts @@ -92,7 +92,7 @@ export async function deleteEnvironmentWindows( traceError(`Failed to delete ".venv" dir [${venvPath}] (attempt ${i + 1}/5).`); } } else { - traceError(`Please delete the ".venv" dir manually: [${venvPath}] manually.`); + traceError(`Please delete the ".venv" dir manually: [${venvPath}]`); } showErrorMessageWithLogs(CreateEnv.Venv.errorDeletingEnvironment); return false; From 3a127fb27595f339dade639a7ae5d1ec4187f8e8 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 23 Aug 2023 21:05:58 -0700 Subject: [PATCH 6/7] Update flow of various quickpicks --- .../creation/provider/venvCreationProvider.ts | 71 ++++++-- .../creation/provider/venvUtils.ts | 77 ++++----- src/client/telemetry/index.ts | 2 +- .../venvCreationProvider.unit.test.ts | 162 +++++++++++++++++- .../creation/provider/venvUtils.unit.test.ts | 63 ++----- 5 files changed, 265 insertions(+), 110 deletions(-) diff --git a/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts b/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts index c86914015cb0..1f29e1e9fa08 100644 --- a/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts +++ b/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts @@ -18,7 +18,13 @@ import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { VenvProgressAndTelemetry, VENV_CREATED_MARKER, VENV_EXISTING_MARKER } from './venvProgressAndTelemetry'; import { showErrorMessageWithLogs } from '../common/commonUtils'; -import { IPackageInstallSelection, pickExistingVenvAction, pickPackagesToInstall } from './venvUtils'; +import { + ExistingVenvAction, + IPackageInstallSelection, + deleteEnvironment, + pickExistingVenvAction, + pickPackagesToInstall, +} from './venvUtils'; import { InputFlowAction } from '../../../common/utils/multiStepInput'; import { CreateEnvironmentProvider, @@ -150,9 +156,32 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { undefined, ); + let existingVenvAction: ExistingVenvAction | undefined; + const existingEnvStep = new MultiStepNode( + workspaceStep, + async (context?: MultiStepAction) => { + if (workspace && context === MultiStepAction.Continue) { + try { + existingVenvAction = await pickExistingVenvAction(workspace); + return MultiStepAction.Continue; + } catch (ex) { + if (ex === MultiStepAction.Back || ex === MultiStepAction.Cancel) { + return ex; + } + throw ex; + } + } else if (context === MultiStepAction.Back) { + return MultiStepAction.Back; + } + return MultiStepAction.Continue; + }, + undefined, + ); + workspaceStep.next = existingEnvStep; + let interpreter: string | undefined; const interpreterStep = new MultiStepNode( - workspaceStep, + existingEnvStep, async () => { if (workspace) { try { @@ -189,14 +218,7 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { }, undefined, ); - workspaceStep.next = interpreterStep; - - const existingEnvStep = new MultiStepNode( - interpreterStep, - async (context?: MultiStepAction) => pickExistingVenvAction(workspace, interpreter, context), - undefined, - ); - interpreterStep.next = existingEnvStep; + existingEnvStep.next = interpreterStep; let addGitIgnore = true; let installPackages = true; @@ -206,7 +228,7 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { } let installInfo: IPackageInstallSelection[] | undefined; const packagesStep = new MultiStepNode( - existingEnvStep, + interpreterStep, async () => { if (workspace && installPackages) { try { @@ -227,13 +249,38 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { }, undefined, ); - existingEnvStep.next = packagesStep; + interpreterStep.next = packagesStep; const action = await MultiStepNode.run(workspaceStep); if (action === MultiStepAction.Back || action === MultiStepAction.Cancel) { throw action; } + if (workspace) { + if (existingVenvAction === ExistingVenvAction.Recreate) { + sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { + environmentType: 'venv', + status: 'triggered', + }); + if (await deleteEnvironment(workspace, interpreter)) { + sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { + environmentType: 'venv', + status: 'deleted', + }); + } else { + sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { + environmentType: 'venv', + status: 'failed', + }); + throw MultiStepAction.Cancel; + } + } else if (existingVenvAction === ExistingVenvAction.UseExisting) { + sendTelemetryEvent(EventName.ENVIRONMENT_REUSE, undefined, { + environmentType: 'venv', + }); + } + } + const args = generateCommandArgs(installInfo, addGitIgnore); return withProgress( diff --git a/src/client/pythonEnvironments/creation/provider/venvUtils.ts b/src/client/pythonEnvironments/creation/provider/venvUtils.ts index c0e2ec7b5cb3..d7a0be170f99 100644 --- a/src/client/pythonEnvironments/creation/provider/venvUtils.ts +++ b/src/client/pythonEnvironments/creation/provider/venvUtils.ts @@ -19,8 +19,6 @@ import { Commands } from '../../../common/constants'; import { isWindows } from '../../../common/platform/platformService'; import { getVenvPath, hasVenv } from '../common/commonUtils'; import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './venvDeleteUtils'; -import { sendTelemetryEvent } from '../../../telemetry'; -import { EventName } from '../../../telemetry/constants'; const exclude = '**/{.venv*,.git,.nox,.tox,.conda,site-packages,__pypackages__}/**'; async function getPipRequirementsFiles( @@ -238,7 +236,10 @@ export async function pickPackagesToInstall( return packages; } -async function deleteEnvironment(workspaceFolder: WorkspaceFolder, interpreter: string | undefined): Promise { +export async function deleteEnvironment( + workspaceFolder: WorkspaceFolder, + interpreter: string | undefined, +): Promise { const venvPath = getVenvPath(workspaceFolder); return withProgress( { @@ -255,23 +256,26 @@ async function deleteEnvironment(workspaceFolder: WorkspaceFolder, interpreter: ); } +export enum ExistingVenvAction { + Recreate, + UseExisting, + Create, +} + export async function pickExistingVenvAction( workspaceFolder: WorkspaceFolder | undefined, - interpreter: string | undefined, - context?: MultiStepAction, -): Promise { - if (workspaceFolder && (await hasVenv(workspaceFolder)) && context === MultiStepAction.Continue) { - const items: QuickPickItem[] = [ - { label: CreateEnv.Venv.recreate, description: CreateEnv.Venv.recreateDescription }, - { - label: CreateEnv.Venv.useExisting, - description: CreateEnv.Venv.useExistingDescription, - }, - ]; - - let selection: QuickPickItem | undefined; - try { - selection = (await showQuickPickWithBack( +): Promise { + if (workspaceFolder) { + if (await hasVenv(workspaceFolder)) { + const items: QuickPickItem[] = [ + { label: CreateEnv.Venv.recreate, description: CreateEnv.Venv.recreateDescription }, + { + label: CreateEnv.Venv.useExisting, + description: CreateEnv.Venv.useExistingDescription, + }, + ]; + + const selection = (await showQuickPickWithBack( items, { placeHolder: CreateEnv.Venv.existingVenvQuickPickPlaceholder, @@ -279,37 +283,18 @@ export async function pickExistingVenvAction( }, undefined, )) as QuickPickItem | undefined; - } catch (ex) { - return MultiStepAction.Back; - } - - if (selection === undefined) { - return MultiStepAction.Cancel; - } - if (selection.label === CreateEnv.Venv.recreate) { - if (await deleteEnvironment(workspaceFolder, interpreter)) { - sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { - environmentType: 'venv', - status: 'deleted', - }); - return MultiStepAction.Continue; + if (selection?.label === CreateEnv.Venv.recreate) { + return ExistingVenvAction.Recreate; } - sendTelemetryEvent(EventName.ENVIRONMENT_DELETE, undefined, { - environmentType: 'venv', - status: 'failed', - }); - return MultiStepAction.Cancel; - } - if (selection.label === CreateEnv.Venv.useExisting) { - sendTelemetryEvent(EventName.ENVIRONMENT_REUSE, undefined, { - environmentType: 'venv', - }); - return MultiStepAction.Continue; + if (selection?.label === CreateEnv.Venv.useExisting) { + return ExistingVenvAction.UseExisting; + } + } else { + return ExistingVenvAction.Create; } - } else if (context === MultiStepAction.Back) { - return MultiStepAction.Back; } - return MultiStepAction.Continue; + + throw MultiStepAction.Cancel; } diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index e8e7d682788c..f4947cd73f05 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -2131,7 +2131,7 @@ export interface IEventNamePropertyMapping { */ [EventName.ENVIRONMENT_DELETE]: { environmentType: 'venv' | 'conda'; - status: 'deleted' | 'failed'; + status: 'triggered' | 'deleted' | 'failed'; }; /** * Telemetry event if user selected to re-use the existing environment. diff --git a/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts index 9a387823c5a7..19e191a8b0d9 100644 --- a/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts @@ -36,6 +36,7 @@ suite('venv Creation provider tests', () => { let showErrorMessageWithLogsStub: sinon.SinonStub; let pickPackagesToInstallStub: sinon.SinonStub; let pickExistingVenvActionStub: sinon.SinonStub; + let deleteEnvironmentStub: sinon.SinonStub; const workspace1 = { uri: Uri.file(path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'testMultiRootWkspc', 'workspace1')), @@ -45,6 +46,7 @@ suite('venv Creation provider tests', () => { setup(() => { pickExistingVenvActionStub = sinon.stub(venvUtils, 'pickExistingVenvAction'); + deleteEnvironmentStub = sinon.stub(venvUtils, 'deleteEnvironment'); pickWorkspaceFolderStub = sinon.stub(wsSelect, 'pickWorkspaceFolder'); execObservableStub = sinon.stub(rawProcessApis, 'execObservable'); interpreterQuickPick = typemoq.Mock.ofType(); @@ -57,7 +59,8 @@ suite('venv Creation provider tests', () => { progressMock = typemoq.Mock.ofType(); venvProvider = new VenvCreationProvider(interpreterQuickPick.object); - pickExistingVenvActionStub.resolves(windowApis.MultiStepAction.Continue); + pickExistingVenvActionStub.resolves(venvUtils.ExistingVenvAction.Create); + deleteEnvironmentStub.resolves(true); }); teardown(() => { @@ -74,6 +77,8 @@ suite('venv Creation provider tests', () => { assert.isTrue(pickWorkspaceFolderStub.calledOnce); interpreterQuickPick.verifyAll(); assert.isTrue(pickPackagesToInstallStub.notCalled); + assert.isTrue(pickExistingVenvActionStub.notCalled); + assert.isTrue(deleteEnvironmentStub.notCalled); }); test('No Python selected', async () => { @@ -89,6 +94,7 @@ suite('venv Creation provider tests', () => { assert.isTrue(pickWorkspaceFolderStub.calledOnce); interpreterQuickPick.verifyAll(); assert.isTrue(pickPackagesToInstallStub.notCalled); + assert.isTrue(deleteEnvironmentStub.notCalled); }); test('User pressed Esc while selecting dependencies', async () => { @@ -103,6 +109,7 @@ suite('venv Creation provider tests', () => { await assert.isRejected(venvProvider.createEnvironment()); assert.isTrue(pickPackagesToInstallStub.calledOnce); + assert.isTrue(deleteEnvironmentStub.notCalled); }); test('Create venv with python selected by user no packages selected', async () => { @@ -166,6 +173,7 @@ suite('venv Creation provider tests', () => { interpreterQuickPick.verifyAll(); progressMock.verifyAll(); assert.isTrue(showErrorMessageWithLogsStub.notCalled); + assert.isTrue(deleteEnvironmentStub.notCalled); }); test('Create venv failed', async () => { @@ -220,6 +228,7 @@ suite('venv Creation provider tests', () => { _complete!(); await assert.isRejected(promise); assert.isTrue(showErrorMessageWithLogsStub.calledOnce); + assert.isTrue(deleteEnvironmentStub.notCalled); }); test('Create venv failed (non-zero exit code)', async () => { @@ -278,5 +287,156 @@ suite('venv Creation provider tests', () => { interpreterQuickPick.verifyAll(); progressMock.verifyAll(); assert.isTrue(showErrorMessageWithLogsStub.calledOnce); + assert.isTrue(deleteEnvironmentStub.notCalled); + }); + + test('Create venv with pre-existing .venv, user selects re-create', async () => { + pickExistingVenvActionStub.resolves(venvUtils.ExistingVenvAction.Recreate); + pickWorkspaceFolderStub.resolves(workspace1); + + interpreterQuickPick + .setup((i) => i.getInterpreterViaQuickPick(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.resolve('/usr/bin/python')) + .verifiable(typemoq.Times.once()); + + pickPackagesToInstallStub.resolves([]); + + const deferred = createDeferred(); + let _next: undefined | ((value: Output) => void); + let _complete: undefined | (() => void); + execObservableStub.callsFake(() => { + deferred.resolve(); + return { + proc: { + exitCode: 0, + }, + out: { + subscribe: ( + next?: (value: Output) => void, + _error?: (error: unknown) => void, + complete?: () => void, + ) => { + _next = next; + _complete = complete; + }, + }, + dispose: () => undefined, + }; + }); + + progressMock.setup((p) => p.report({ message: CreateEnv.statusStarting })).verifiable(typemoq.Times.once()); + + withProgressStub.callsFake( + ( + _options: ProgressOptions, + task: ( + progress: CreateEnvironmentProgress, + token?: CancellationToken, + ) => Thenable, + ) => task(progressMock.object), + ); + + const promise = venvProvider.createEnvironment(); + await deferred.promise; + assert.isDefined(_next); + assert.isDefined(_complete); + + _next!({ out: `${VENV_CREATED_MARKER}new_environment`, source: 'stdout' }); + _complete!(); + + const actual = await promise; + assert.deepStrictEqual(actual, { + path: 'new_environment', + workspaceFolder: workspace1, + }); + interpreterQuickPick.verifyAll(); + progressMock.verifyAll(); + assert.isTrue(showErrorMessageWithLogsStub.notCalled); + assert.isTrue(deleteEnvironmentStub.calledOnce); + }); + + test('Create venv with pre-existing .venv, user selects re-create, delete env failed', async () => { + pickExistingVenvActionStub.resolves(venvUtils.ExistingVenvAction.Recreate); + pickWorkspaceFolderStub.resolves(workspace1); + deleteEnvironmentStub.resolves(false); + + interpreterQuickPick + .setup((i) => i.getInterpreterViaQuickPick(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.resolve('/usr/bin/python')) + .verifiable(typemoq.Times.once()); + + pickPackagesToInstallStub.resolves([]); + + await assert.isRejected(venvProvider.createEnvironment()); + + interpreterQuickPick.verifyAll(); + assert.isTrue(withProgressStub.notCalled); + assert.isTrue(showErrorMessageWithLogsStub.notCalled); + assert.isTrue(deleteEnvironmentStub.calledOnce); + }); + + test('Create venv with pre-existing .venv, user selects use existing', async () => { + pickExistingVenvActionStub.resolves(venvUtils.ExistingVenvAction.UseExisting); + pickWorkspaceFolderStub.resolves(workspace1); + + interpreterQuickPick + .setup((i) => i.getInterpreterViaQuickPick(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.resolve('/usr/bin/python')) + .verifiable(typemoq.Times.once()); + + pickPackagesToInstallStub.resolves([]); + + const deferred = createDeferred(); + let _next: undefined | ((value: Output) => void); + let _complete: undefined | (() => void); + execObservableStub.callsFake(() => { + deferred.resolve(); + return { + proc: { + exitCode: 0, + }, + out: { + subscribe: ( + next?: (value: Output) => void, + _error?: (error: unknown) => void, + complete?: () => void, + ) => { + _next = next; + _complete = complete; + }, + }, + dispose: () => undefined, + }; + }); + + progressMock.setup((p) => p.report({ message: CreateEnv.statusStarting })).verifiable(typemoq.Times.once()); + + withProgressStub.callsFake( + ( + _options: ProgressOptions, + task: ( + progress: CreateEnvironmentProgress, + token?: CancellationToken, + ) => Thenable, + ) => task(progressMock.object), + ); + + const promise = venvProvider.createEnvironment(); + await deferred.promise; + assert.isDefined(_next); + assert.isDefined(_complete); + + _next!({ out: `${VENV_CREATED_MARKER}new_environment`, source: 'stdout' }); + _complete!(); + + const actual = await promise; + assert.deepStrictEqual(actual, { + path: 'new_environment', + workspaceFolder: workspace1, + }); + interpreterQuickPick.verifyAll(); + progressMock.verifyAll(); + assert.isTrue(showErrorMessageWithLogsStub.notCalled); + assert.isTrue(deleteEnvironmentStub.notCalled); }); }); diff --git a/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts index dcdecfaeb776..ae4f43a0296c 100644 --- a/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvUtils.unit.test.ts @@ -9,6 +9,7 @@ import * as path from 'path'; import * as windowApis from '../../../../client/common/vscodeApis/windowApis'; import * as workspaceApis from '../../../../client/common/vscodeApis/workspaceApis'; import { + ExistingVenvAction, pickExistingVenvAction, pickPackagesToInstall, } from '../../../../client/pythonEnvironments/creation/provider/venvUtils'; @@ -376,76 +377,38 @@ suite('Test pick existing venv action', () => { label: CreateEnv.Venv.useExisting, description: CreateEnv.Venv.useExistingDescription, }); - const actual = await pickExistingVenvAction( - workspace1, - '/path/to/global/interpreter', - windowApis.MultiStepAction.Continue, - ); - assert.deepStrictEqual(actual, windowApis.MultiStepAction.Continue); + const actual = await pickExistingVenvAction(workspace1); + assert.deepStrictEqual(actual, ExistingVenvAction.UseExisting); }); test('User presses escape', async () => { pathExistsStub.resolves(true); showQuickPickWithBackStub.resolves(undefined); - const actual = await pickExistingVenvAction( - workspace1, - '/path/to/global/interpreter', - windowApis.MultiStepAction.Continue, - ); - assert.deepStrictEqual(actual, windowApis.MultiStepAction.Cancel); + await assert.isRejected(pickExistingVenvAction(workspace1)); }); - test('User selects delete venv and it succeeds', async () => { + test('User selects delete venv', async () => { pathExistsStub.resolves(true); showQuickPickWithBackStub.resolves({ label: CreateEnv.Venv.recreate, description: CreateEnv.Venv.recreateDescription, }); withProgressStub.resolves(true); - const actual = await pickExistingVenvAction( - workspace1, - '/path/to/global/interpreter', - windowApis.MultiStepAction.Continue, - ); - assert.deepStrictEqual(actual, windowApis.MultiStepAction.Continue); + const actual = await pickExistingVenvAction(workspace1); + assert.deepStrictEqual(actual, ExistingVenvAction.Recreate); }); - test('User selects delete venv and it fails', async () => { - pathExistsStub.resolves(true); - showQuickPickWithBackStub.resolves({ - label: CreateEnv.Venv.recreate, - description: CreateEnv.Venv.recreateDescription, - }); - withProgressStub.resolves(false); - const actual = await pickExistingVenvAction( - workspace1, - '/path/to/global/interpreter', - windowApis.MultiStepAction.Continue, - ); - assert.deepStrictEqual(actual, windowApis.MultiStepAction.Cancel); - }); test('User clicks on back', async () => { pathExistsStub.resolves(true); // We use reject with "Back" to simulate the user clicking on back. showQuickPickWithBackStub.rejects(windowApis.MultiStepAction.Back); withProgressStub.resolves(false); - const actual = await pickExistingVenvAction( - workspace1, - '/path/to/global/interpreter', - windowApis.MultiStepAction.Continue, - ); - assert.deepStrictEqual(actual, windowApis.MultiStepAction.Back); + await assert.isRejected(pickExistingVenvAction(workspace1)); }); - test('User clicks on back from the next quick pick', async () => { - pathExistsStub.resolves(true); - showQuickPickWithBackStub.resolves(undefined); - withProgressStub.resolves(false); - const actual = await pickExistingVenvAction( - workspace1, - '/path/to/global/interpreter', - windowApis.MultiStepAction.Back, - ); - assert.deepStrictEqual(actual, windowApis.MultiStepAction.Back); - assert.ok(showQuickPickWithBackStub.notCalled); + + test('No venv found', async () => { + pathExistsStub.resolves(false); + const actual = await pickExistingVenvAction(workspace1); + assert.deepStrictEqual(actual, ExistingVenvAction.Create); }); }); From 34adf51840f14bda2d33dcb7c05933934b56fd94 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 24 Aug 2023 12:44:34 -0700 Subject: [PATCH 7/7] Tweak use existing flow --- .../creation/common/commonUtils.ts | 8 ++ .../creation/provider/venvCreationProvider.ts | 81 +++++++++++-------- .../venvCreationProvider.unit.test.ts | 53 +----------- 3 files changed, 59 insertions(+), 83 deletions(-) diff --git a/src/client/pythonEnvironments/creation/common/commonUtils.ts b/src/client/pythonEnvironments/creation/common/commonUtils.ts index 583642d32b94..b4d4a37eae9b 100644 --- a/src/client/pythonEnvironments/creation/common/commonUtils.ts +++ b/src/client/pythonEnvironments/creation/common/commonUtils.ts @@ -7,6 +7,7 @@ import { Commands } from '../../../common/constants'; import { Common } from '../../../common/utils/localize'; import { executeCommand } from '../../../common/vscodeApis/commandApis'; import { showErrorMessage } from '../../../common/vscodeApis/windowApis'; +import { isWindows } from '../../../common/platform/platformService'; export async function showErrorMessageWithLogs(message: string): Promise { const result = await showErrorMessage(message, Common.openOutputPanel, Common.selectPythonInterpreter); @@ -24,3 +25,10 @@ export function getVenvPath(workspaceFolder: WorkspaceFolder): string { export async function hasVenv(workspaceFolder: WorkspaceFolder): Promise { return fs.pathExists(getVenvPath(workspaceFolder)); } + +export function getVenvExecutable(workspaceFolder: WorkspaceFolder): string { + if (isWindows()) { + return path.join(getVenvPath(workspaceFolder), 'Scripts', 'python.exe'); + } + return path.join(getVenvPath(workspaceFolder), 'bin', 'python'); +} diff --git a/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts b/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts index 1f29e1e9fa08..ba47edcf6f30 100644 --- a/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts +++ b/src/client/pythonEnvironments/creation/provider/venvCreationProvider.ts @@ -17,7 +17,7 @@ import { MultiStepAction, MultiStepNode, withProgress } from '../../../common/vs import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { VenvProgressAndTelemetry, VENV_CREATED_MARKER, VENV_EXISTING_MARKER } from './venvProgressAndTelemetry'; -import { showErrorMessageWithLogs } from '../common/commonUtils'; +import { getVenvExecutable, showErrorMessageWithLogs } from '../common/commonUtils'; import { ExistingVenvAction, IPackageInstallSelection, @@ -182,30 +182,40 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { let interpreter: string | undefined; const interpreterStep = new MultiStepNode( existingEnvStep, - async () => { + async (context?: MultiStepAction) => { if (workspace) { - try { - interpreter = await this.interpreterQuickPick.getInterpreterViaQuickPick( - workspace.uri, - (i: PythonEnvironment) => - [ - EnvironmentType.System, - EnvironmentType.MicrosoftStore, - EnvironmentType.Global, - EnvironmentType.Pyenv, - ].includes(i.envType) && i.type === undefined, // only global intepreters - { - skipRecommended: true, - showBackButton: true, - placeholder: CreateEnv.Venv.selectPythonPlaceHolder, - title: null, - }, - ); - } catch (ex) { - if (ex === InputFlowAction.back) { + if ( + existingVenvAction === ExistingVenvAction.Recreate || + existingVenvAction === ExistingVenvAction.Create + ) { + try { + interpreter = await this.interpreterQuickPick.getInterpreterViaQuickPick( + workspace.uri, + (i: PythonEnvironment) => + [ + EnvironmentType.System, + EnvironmentType.MicrosoftStore, + EnvironmentType.Global, + EnvironmentType.Pyenv, + ].includes(i.envType) && i.type === undefined, // only global intepreters + { + skipRecommended: true, + showBackButton: true, + placeholder: CreateEnv.Venv.selectPythonPlaceHolder, + title: null, + }, + ); + } catch (ex) { + if (ex === InputFlowAction.back) { + return MultiStepAction.Back; + } + interpreter = undefined; + } + } else if (existingVenvAction === ExistingVenvAction.UseExisting) { + if (context === MultiStepAction.Back) { return MultiStepAction.Back; } - interpreter = undefined; + interpreter = getVenvExecutable(workspace); } } @@ -229,19 +239,23 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { let installInfo: IPackageInstallSelection[] | undefined; const packagesStep = new MultiStepNode( interpreterStep, - async () => { + async (context?: MultiStepAction) => { if (workspace && installPackages) { - try { - installInfo = await pickPackagesToInstall(workspace); - } catch (ex) { - if (ex === MultiStepAction.Back || ex === MultiStepAction.Cancel) { - return ex; + if (existingVenvAction !== ExistingVenvAction.UseExisting) { + try { + installInfo = await pickPackagesToInstall(workspace); + } catch (ex) { + if (ex === MultiStepAction.Back || ex === MultiStepAction.Cancel) { + return ex; + } + throw ex; } - throw ex; - } - if (!installInfo) { - traceVerbose('Virtual env creation exited during dependencies selection.'); - return MultiStepAction.Cancel; + if (!installInfo) { + traceVerbose('Virtual env creation exited during dependencies selection.'); + return MultiStepAction.Cancel; + } + } else if (context === MultiStepAction.Back) { + return MultiStepAction.Back; } } @@ -278,6 +292,7 @@ export class VenvCreationProvider implements CreateEnvironmentProvider { sendTelemetryEvent(EventName.ENVIRONMENT_REUSE, undefined, { environmentType: 'venv', }); + return { path: getVenvExecutable(workspace), workspaceFolder: workspace }; } } diff --git a/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts b/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts index 19e191a8b0d9..280d05bf5935 100644 --- a/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts +++ b/src/test/pythonEnvironments/creation/provider/venvCreationProvider.unit.test.ts @@ -382,60 +382,13 @@ suite('venv Creation provider tests', () => { interpreterQuickPick .setup((i) => i.getInterpreterViaQuickPick(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) .returns(() => Promise.resolve('/usr/bin/python')) - .verifiable(typemoq.Times.once()); + .verifiable(typemoq.Times.never()); pickPackagesToInstallStub.resolves([]); - const deferred = createDeferred(); - let _next: undefined | ((value: Output) => void); - let _complete: undefined | (() => void); - execObservableStub.callsFake(() => { - deferred.resolve(); - return { - proc: { - exitCode: 0, - }, - out: { - subscribe: ( - next?: (value: Output) => void, - _error?: (error: unknown) => void, - complete?: () => void, - ) => { - _next = next; - _complete = complete; - }, - }, - dispose: () => undefined, - }; - }); - - progressMock.setup((p) => p.report({ message: CreateEnv.statusStarting })).verifiable(typemoq.Times.once()); - - withProgressStub.callsFake( - ( - _options: ProgressOptions, - task: ( - progress: CreateEnvironmentProgress, - token?: CancellationToken, - ) => Thenable, - ) => task(progressMock.object), - ); - - const promise = venvProvider.createEnvironment(); - await deferred.promise; - assert.isDefined(_next); - assert.isDefined(_complete); - - _next!({ out: `${VENV_CREATED_MARKER}new_environment`, source: 'stdout' }); - _complete!(); - - const actual = await promise; - assert.deepStrictEqual(actual, { - path: 'new_environment', - workspaceFolder: workspace1, - }); interpreterQuickPick.verifyAll(); - progressMock.verifyAll(); + assert.isTrue(withProgressStub.notCalled); + assert.isTrue(pickPackagesToInstallStub.notCalled); assert.isTrue(showErrorMessageWithLogsStub.notCalled); assert.isTrue(deleteEnvironmentStub.notCalled); });