Skip to content

Commit 7e32a8b

Browse files
tkislantkislan
committed
fix: Enhance error messages in Deepnote environments view and improve cancellation handling
Signed-off-by: Tomas Kislan <[email protected]>
1 parent a96c0aa commit 7e32a8b

File tree

6 files changed

+83
-44
lines changed

6 files changed

+83
-44
lines changed

‎src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,7 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
283283
public async restartServer(id: string, token?: CancellationToken): Promise<void> {
284284
logger.info(`Restarting server for environment: ${id}`);
285285
await this.stopServer(id, token);
286+
Cancellation.throwIfCanceled(token);
286287
await this.startServer(id, token);
287288
}
288289

‎src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ suite('DeepnoteEnvironmentManager', () => {
227227

228228
const updated = manager.getEnvironment(config.id);
229229
assert.strictEqual(updated?.name, 'Updated Name');
230+
verify(mockStorage.saveEnvironments(anything())).atLeast(1);
230231
});
231232

232233
test('should update packages', async () => {
@@ -242,6 +243,7 @@ suite('DeepnoteEnvironmentManager', () => {
242243

243244
const updated = manager.getEnvironment(config.id);
244245
assert.deepStrictEqual(updated?.packages, ['numpy', 'pandas']);
246+
verify(mockStorage.saveEnvironments(anything())).atLeast(1);
245247
});
246248

247249
test('should throw error for non-existent environment', async () => {
@@ -283,6 +285,7 @@ suite('DeepnoteEnvironmentManager', () => {
283285

284286
const deleted = manager.getEnvironment(config.id);
285287
assert.isUndefined(deleted);
288+
verify(mockStorage.saveEnvironments(anything())).atLeast(1);
286289
});
287290

288291
test('should stop server before deleting if running', async () => {
@@ -443,12 +446,10 @@ suite('DeepnoteEnvironmentManager', () => {
443446
pythonInterpreter: testInterpreter
444447
});
445448

446-
const originalLastUsed = config.lastUsedAt;
447-
await new Promise((resolve) => setTimeout(resolve, 10));
449+
const originalLastUsed = config.lastUsedAt.getTime();
448450
await manager.startServer(config.id);
449-
450-
const updated = manager.getEnvironment(config.id);
451-
assert.isTrue(updated!.lastUsedAt > originalLastUsed);
451+
const updated = manager.getEnvironment(config.id)!;
452+
assert.isAtLeast(updated.lastUsedAt.getTime(), originalLastUsed);
452453
});
453454

454455
test('should throw error for non-existent environment', async () => {

‎src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,19 @@ export class DeepnoteEnvironmentTreeItem extends TreeItem {
3434
}
3535
}
3636

37+
/**
38+
* Create an info item to display under an environment
39+
*/
40+
public static createInfoItem(label: string, icon?: string): DeepnoteEnvironmentTreeItem {
41+
const item = new DeepnoteEnvironmentTreeItem(EnvironmentTreeItemType.InfoItem, undefined, undefined, label);
42+
43+
if (icon) {
44+
item.iconPath = new ThemeIcon(icon);
45+
}
46+
47+
return item;
48+
}
49+
3750
private setupEnvironmentItem(): void {
3851
if (!this.environment || !this.status) {
3952
return;
@@ -81,21 +94,21 @@ export class DeepnoteEnvironmentTreeItem extends TreeItem {
8194
const lines: string[] = [];
8295
lines.push(`**${this.environment.name}**`);
8396
lines.push('');
84-
lines.push(`Status: ${this.status}`);
85-
lines.push(`Python: ${this.environment.pythonInterpreter.uri.toString(true)}`);
86-
lines.push(`Venv: ${this.environment.venvPath.toString(true)}`);
97+
lines.push(l10n.t('Status: {0}', this.status ?? l10n.t('Unknown')));
98+
lines.push(l10n.t('Python: {0}', this.environment.pythonInterpreter.uri.toString(true)));
99+
lines.push(l10n.t('Venv: {0}', this.environment.venvPath.toString(true)));
87100

88101
if (this.environment.packages && this.environment.packages.length > 0) {
89-
lines.push(`Packages: ${this.environment.packages.join(', ')}`);
102+
lines.push(l10n.t('Packages: {0}', this.environment.packages.join(', ')));
90103
}
91104

92105
if (this.environment.toolkitVersion) {
93-
lines.push(`Toolkit: ${this.environment.toolkitVersion}`);
106+
lines.push(l10n.t('Toolkit: {0}', this.environment.toolkitVersion));
94107
}
95108

96109
lines.push('');
97-
lines.push(`Created: ${this.environment.createdAt.toLocaleString()}`);
98-
lines.push(`Last used: ${this.environment.lastUsedAt.toLocaleString()}`);
110+
lines.push(l10n.t('Created: {0}', this.environment.createdAt.toLocaleString()));
111+
lines.push(l10n.t('Last used: {0}', this.environment.lastUsedAt.toLocaleString()));
99112

100113
return lines.join('\n');
101114
}
@@ -120,17 +133,4 @@ export class DeepnoteEnvironmentTreeItem extends TreeItem {
120133
return date.toLocaleDateString();
121134
}
122135
}
123-
124-
/**
125-
* Create an info item to display under an environment
126-
*/
127-
public static createInfoItem(label: string, icon?: string): DeepnoteEnvironmentTreeItem {
128-
const item = new DeepnoteEnvironmentTreeItem(EnvironmentTreeItemType.InfoItem, undefined, undefined, label);
129-
130-
if (icon) {
131-
item.iconPath = new ThemeIcon(icon);
132-
}
133-
134-
return item;
135-
}
136136
}

‎src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
265265
}
266266
);
267267
} catch (error) {
268-
void window.showErrorMessage(l10n.t('Failed to create environment: {0}', error));
268+
void window.showErrorMessage(l10n.t('Failed to create environment. See output for details.'));
269269
}
270270
}
271271

@@ -291,7 +291,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
291291
void window.showInformationMessage(l10n.t('Server started for "{0}"', config.name));
292292
} catch (error) {
293293
logger.error('Failed to start server', error);
294-
void window.showErrorMessage(l10n.t('Failed to start server: {0}', error));
294+
void window.showErrorMessage(l10n.t('Failed to start server. See output for details.'));
295295
}
296296
}
297297

@@ -317,7 +317,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
317317
void window.showInformationMessage(l10n.t('Server stopped for "{0}"', config.name));
318318
} catch (error) {
319319
logger.error('Failed to stop server', error);
320-
void window.showErrorMessage(l10n.t('Failed to stop server: {0}', error));
320+
void window.showErrorMessage(l10n.t('Failed to stop server. See output for details.'));
321321
}
322322
}
323323

@@ -343,7 +343,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
343343
void window.showInformationMessage(l10n.t('Server restarted for "{0}"', config.name));
344344
} catch (error) {
345345
logger.error('Failed to restart server', error);
346-
void window.showErrorMessage(l10n.t('Failed to restart server: {0}', error));
346+
void window.showErrorMessage(l10n.t('Failed to restart server. See output for details.'));
347347
}
348348
}
349349

@@ -389,7 +389,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
389389
void window.showInformationMessage(l10n.t('Environment "{0}" deleted', config.name));
390390
} catch (error) {
391391
logger.error('Failed to delete environment', error);
392-
void window.showErrorMessage(l10n.t('Failed to delete environment: {0}', error));
392+
void window.showErrorMessage(l10n.t('Failed to delete environment. See output for details.'));
393393
}
394394
}
395395

@@ -423,7 +423,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
423423
void window.showInformationMessage(l10n.t('Environment renamed to "{0}"', newName));
424424
} catch (error) {
425425
logger.error('Failed to rename environment', error);
426-
void window.showErrorMessage(l10n.t('Failed to rename environment: {0}', error));
426+
void window.showErrorMessage(l10n.t('Failed to rename environment. See output for details.'));
427427
}
428428
}
429429

@@ -444,7 +444,11 @@ export class DeepnoteEnvironmentsView implements Disposable {
444444
}
445445
const packages = value.split(',').map((p: string) => p.trim());
446446
for (const pkg of packages) {
447-
if (!/^[a-zA-Z0-9_\-\[\]]+$/.test(pkg)) {
447+
const isValid =
448+
/^[A-Za-z0-9._\-]+(\[[A-Za-z0-9_,.\-]+\])?(\s*(==|>=|<=|~=|>|<)\s*[A-Za-z0-9.*+!\-_.]+)?(?:\s*;.+)?$/.test(
449+
pkg
450+
);
451+
if (!isValid) {
448452
return l10n.t('Invalid package name: {0}', pkg);
449453
}
450454
}
@@ -477,7 +481,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
477481
void window.showInformationMessage(l10n.t('Packages updated for "{0}"', config.name));
478482
} catch (error) {
479483
logger.error('Failed to update packages', error);
480-
void window.showErrorMessage(l10n.t('Failed to update packages: {0}', error));
484+
void window.showErrorMessage(l10n.t('Failed to update packages. See output for details.'));
481485
}
482486
}
483487

@@ -616,7 +620,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
616620
void window.showInformationMessage(l10n.t('Environment switched successfully'));
617621
} catch (error) {
618622
logger.error('Failed to switch environment', error);
619-
void window.showErrorMessage(l10n.t('Failed to switch environment: {0}', error));
623+
void window.showErrorMessage(l10n.t('Failed to switch environment. See output for details.'));
620624
}
621625
}
622626

‎src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,12 @@ suite('DeepnoteEnvironmentsView', () => {
3535
mockKernelProvider = mock<IKernelProvider>();
3636

3737
// Mock onDidChangeEnvironments to return a disposable event
38-
when(mockConfigManager.onDidChangeEnvironments).thenReturn(() => {
38+
when(mockConfigManager.onDidChangeEnvironments).thenReturn((_listener: () => void) => {
3939
return {
4040
dispose: () => {
4141
/* noop */
4242
}
43-
} as Disposable;
43+
};
4444
});
4545

4646
view = new DeepnoteEnvironmentsView(
@@ -224,7 +224,6 @@ suite('DeepnoteEnvironmentsView', () => {
224224

225225
assert.ok(capturedOptions, 'Options should be provided');
226226
assert.strictEqual(capturedOptions.value, 'Original Name');
227-
assert.strictEqual(capturedOptions.prompt, 'Enter a new name for this environment');
228227
});
229228
});
230229
});

‎src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts‎

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { assert } from 'chai';
22
import * as sinon from 'sinon';
3-
import { anything, instance, mock, when } from 'ts-mockito';
3+
import { anything, instance, mock, verify, when } from 'ts-mockito';
44
import { DeepnoteKernelAutoSelector } from './deepnoteKernelAutoSelector.node';
55
import {
66
IDeepnoteEnvironmentManager,
@@ -20,7 +20,6 @@ import { IDeepnoteRequirementsHelper } from './deepnoteRequirementsHelper.node';
2020
import { NotebookDocument, Uri, NotebookController, CancellationToken } from 'vscode';
2121
import { DeepnoteEnvironment } from '../../kernels/deepnote/environments/deepnoteEnvironment';
2222
import { PythonEnvironment } from '../../platform/pythonEnvironments/info';
23-
import { STANDARD_OUTPUT_CHANNEL } from '../../platform/common/constants';
2423

2524
suite('DeepnoteKernelAutoSelector - rebuildController', () => {
2625
let selector: DeepnoteKernelAutoSelector;
@@ -60,7 +59,7 @@ suite('DeepnoteKernelAutoSelector - rebuildController', () => {
6059
mockEnvironmentManager = mock<IDeepnoteEnvironmentManager>();
6160
mockEnvironmentPicker = mock<IDeepnoteEnvironmentPicker>();
6261
mockNotebookEnvironmentMapper = mock<IDeepnoteNotebookEnvironmentMapper>();
63-
mockOutputChannel = mock<IOutputChannel>(STANDARD_OUTPUT_CHANNEL);
62+
mockOutputChannel = mock<IOutputChannel>();
6463

6564
// Create mock notebook
6665
mockNotebook = {
@@ -117,9 +116,9 @@ suite('DeepnoteKernelAutoSelector - rebuildController', () => {
117116
});
118117

119118
suite('rebuildController', () => {
120-
test('should log warning when switching with pending cells', async () => {
121-
// This test verifies that rebuildController logs a warning when cells are executing
122-
// but still proceeds with the environment switch
119+
test('should proceed with environment switch despite pending cells', async () => {
120+
// This test verifies that rebuildController continues with the environment switch
121+
// even when cells are currently executing (pending)
123122

124123
// Arrange
125124
const mockKernel = mock<IKernel>();
@@ -208,13 +207,48 @@ suite('DeepnoteKernelAutoSelector - rebuildController', () => {
208207
// Mock kernel provider to return no kernel (no cells executing)
209208
when(mockKernelProvider.get(mockNotebook)).thenReturn(undefined);
210209

210+
// Get the notebook key that will be used internally
211+
const baseFileUri = mockNotebook.uri.with({ query: '', fragment: '' });
212+
const notebookKey = baseFileUri.fsPath;
213+
214+
// Set up initial metadata and server handle to verify they get cleared
215+
const selectorWithPrivateAccess = selector as any;
216+
const mockMetadata = { id: 'test-metadata' };
217+
const mockServerHandle = 'test-server-handle';
218+
selectorWithPrivateAccess.notebookConnectionMetadata.set(notebookKey, mockMetadata);
219+
selectorWithPrivateAccess.notebookServerHandles.set(notebookKey, mockServerHandle);
220+
221+
// Verify metadata is set before rebuild
222+
assert.strictEqual(
223+
selectorWithPrivateAccess.notebookConnectionMetadata.has(notebookKey),
224+
true,
225+
'Metadata should be set before rebuildController'
226+
);
227+
assert.strictEqual(
228+
selectorWithPrivateAccess.notebookServerHandles.has(notebookKey),
229+
true,
230+
'Server handle should be set before rebuildController'
231+
);
232+
211233
// Stub ensureKernelSelected to avoid full execution
212234
const ensureKernelSelectedStub = sandbox.stub(selector, 'ensureKernelSelected').resolves();
213235

214236
// Act
215237
await selector.rebuildController(mockNotebook);
216238

217-
// Assert
239+
// Assert - verify metadata has been cleared
240+
assert.strictEqual(
241+
selectorWithPrivateAccess.notebookConnectionMetadata.has(notebookKey),
242+
false,
243+
'Connection metadata should be cleared to force fresh metadata creation'
244+
);
245+
assert.strictEqual(
246+
selectorWithPrivateAccess.notebookServerHandles.has(notebookKey),
247+
false,
248+
'Server handle should be cleared'
249+
);
250+
251+
// Assert - verify ensureKernelSelected has been called
218252
assert.strictEqual(
219253
ensureKernelSelectedStub.calledOnce,
220254
true,

0 commit comments

Comments
 (0)