From aa8334c0612dca5c068c209a398f08827bc3af57 Mon Sep 17 00:00:00 2001 From: Tomas Kislan Date: Fri, 31 Oct 2025 13:43:14 +0000 Subject: [PATCH] feat: Delete python virtual environment directory when removing deepnote environment Signed-off-by: Tomas Kislan --- .../deepnoteEnvironmentManager.node.ts | 15 ++++- .../deepnoteEnvironmentManager.unit.test.ts | 57 ++++++++++++++++++- 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts b/src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts index cf383d16a8..c001aef5f0 100644 --- a/src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts +++ b/src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts @@ -4,6 +4,7 @@ import { generateUuid as uuid } from '../../../platform/common/uuid'; import { IExtensionContext, IOutputChannel } from '../../../platform/common/types'; import { IExtensionSyncActivationService } from '../../../platform/activation/types'; import { logger } from '../../../platform/logging'; +import { IFileSystem } from '../../../platform/common/platform/types'; import { DeepnoteEnvironmentStorage } from './deepnoteEnvironmentStorage.node'; import { CreateDeepnoteEnvironmentOptions, @@ -31,7 +32,8 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi @inject(DeepnoteEnvironmentStorage) private readonly storage: DeepnoteEnvironmentStorage, @inject(IDeepnoteToolkitInstaller) private readonly toolkitInstaller: IDeepnoteToolkitInstaller, @inject(IDeepnoteServerStarter) private readonly serverStarter: IDeepnoteServerStarter, - @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel + @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel, + @inject(IFileSystem) private readonly fileSystem: IFileSystem ) {} /** @@ -191,6 +193,17 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi Cancellation.throwIfCanceled(token); + // Delete the virtual environment directory from disk + try { + await this.fileSystem.delete(config.venvPath); + logger.info(`Deleted virtual environment directory: ${config.venvPath.fsPath}`); + } catch (error) { + // Log but don't fail - the directory might not exist or might already be deleted + logger.warn(`Failed to delete virtual environment directory: ${config.venvPath.fsPath}`, error); + } + + Cancellation.throwIfCanceled(token); + this.environments.delete(id); await this.persistEnvironments(); this._onDidChangeEnvironments.fire(); diff --git a/src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts b/src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts index d4bf451043..f5893a7558 100644 --- a/src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts +++ b/src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts @@ -2,9 +2,12 @@ import { assert, use } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import { anything, instance, mock, when, verify, deepEqual } from 'ts-mockito'; import { Uri } from 'vscode'; +import * as fs from 'fs'; +import * as os from 'os'; import { DeepnoteEnvironmentManager } from './deepnoteEnvironmentManager.node'; import { DeepnoteEnvironmentStorage } from './deepnoteEnvironmentStorage.node'; import { IExtensionContext, IOutputChannel } from '../../../platform/common/types'; +import { IFileSystem } from '../../../platform/common/platform/types'; import { IDeepnoteServerStarter, IDeepnoteToolkitInstaller, @@ -24,6 +27,8 @@ suite('DeepnoteEnvironmentManager', () => { let mockToolkitInstaller: IDeepnoteToolkitInstaller; let mockServerStarter: IDeepnoteServerStarter; let mockOutputChannel: IOutputChannel; + let mockFileSystem: IFileSystem; + let testGlobalStoragePath: string; const testInterpreter: PythonEnvironment = { id: 'test-python-id', @@ -49,19 +54,40 @@ suite('DeepnoteEnvironmentManager', () => { mockToolkitInstaller = mock(); mockServerStarter = mock(); mockOutputChannel = mock(); + mockFileSystem = mock(); - when(mockContext.globalStorageUri).thenReturn(Uri.file('/global/storage')); + // Create a temporary directory for test storage + testGlobalStoragePath = fs.mkdtempSync(`${os.tmpdir()}/deepnote-test-`); + + when(mockContext.globalStorageUri).thenReturn(Uri.file(testGlobalStoragePath)); when(mockStorage.loadEnvironments()).thenResolve([]); + // Configure mockFileSystem to actually delete directories for testing + when(mockFileSystem.delete(anything())).thenCall((uri: Uri) => { + const dirPath = uri.fsPath; + if (fs.existsSync(dirPath)) { + fs.rmSync(dirPath, { recursive: true, force: true }); + } + return Promise.resolve(); + }); + manager = new DeepnoteEnvironmentManager( instance(mockContext), instance(mockStorage), instance(mockToolkitInstaller), instance(mockServerStarter), - instance(mockOutputChannel) + instance(mockOutputChannel), + instance(mockFileSystem) ); }); + teardown(() => { + // Clean up the temporary directory after each test + if (testGlobalStoragePath && fs.existsSync(testGlobalStoragePath)) { + fs.rmSync(testGlobalStoragePath, { recursive: true, force: true }); + } + }); + suite('activate', () => { test('should load environments on activation', async () => { const existingConfigs = [ @@ -312,6 +338,33 @@ suite('DeepnoteEnvironmentManager', () => { test('should throw error for non-existent environment', async () => { await assert.isRejected(manager.deleteEnvironment('non-existent'), 'Environment not found: non-existent'); }); + + test('should delete virtual environment directory from disk', async () => { + when(mockStorage.saveEnvironments(anything())).thenResolve(); + + const config = await manager.createEnvironment({ + name: 'Test', + pythonInterpreter: testInterpreter + }); + + // Create the virtual environment directory to simulate it existing + const venvDirPath = config.venvPath.fsPath; + fs.mkdirSync(venvDirPath, { recursive: true }); + + // Create a dummy file inside to make it a "real" directory + fs.writeFileSync(`${venvDirPath}/test.txt`, 'test content'); + + // Verify directory and file exist before deletion + assert.isTrue(fs.existsSync(venvDirPath), 'Directory should exist before deletion'); + assert.isTrue(fs.existsSync(`${venvDirPath}/test.txt`), 'File should exist before deletion'); + + // Delete the environment + await manager.deleteEnvironment(config.id); + + // Verify directory no longer exists (with a small delay to allow async operation) + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.isFalse(fs.existsSync(venvDirPath), 'Directory should not exist after deletion'); + }); }); suite('startServer', () => {