From 3d9d2e350e762cdfbb3014564893c58b06eac260 Mon Sep 17 00:00:00 2001 From: seem Date: Sat, 9 Nov 2024 13:51:15 +0200 Subject: [PATCH 01/45] throw an error when starting a notebook session without a notebook uri --- .../services/runtimeSession/common/runtimeSession.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index c5b6e4a6358..cbd1db83f66 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -445,6 +445,11 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession `for the ${metadata.languageName} language.`); } } + } else if (sessionMode === LanguageRuntimeSessionMode.Notebook) { + // If no notebook URI is provided, throw an error. + if (!notebookUri) { + throw new Error(`A notebook URI must be provided when starting a notebook session.`); + } } // If the workspace is not trusted, defer starting the runtime until the From 3a354a33a763095bc63c8280c4f1008689b66fb6 Mon Sep 17 00:00:00 2001 From: seem Date: Sat, 9 Nov 2024 13:52:14 +0200 Subject: [PATCH 02/45] throw an error when starting a runtime for a notebook while another runtime is already starting --- .../runtimeSession/common/runtimeSession.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index cbd1db83f66..3d053a01769 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -450,6 +450,17 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession if (!notebookUri) { throw new Error(`A notebook URI must be provided when starting a notebook session.`); } + + // If there is already a runtime starting for the notebook, throw an error. + const startingLanguageRuntime = this._startingNotebooksByNotebookUri.get(notebookUri); + if (startingLanguageRuntime) { + throw new Error(`Session for language runtime ` + + `${formatLanguageRuntimeMetadata(languageRuntime)} ` + + `cannot be started because language runtime ` + + `${formatLanguageRuntimeMetadata(startingLanguageRuntime)} ` + + `is already starting for the notebook ${notebookUri.toString()}. ` + + `Request source: ${source}`); + } } // If the workspace is not trusted, defer starting the runtime until the @@ -792,6 +803,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // Add the runtime to the starting runtimes. if (sessionMode === LanguageRuntimeSessionMode.Console) { this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); + } else if (sessionMode === LanguageRuntimeSessionMode.Notebook && notebookUri) { + this._startingNotebooksByNotebookUri.set(notebookUri, runtimeMetadata); } // Create a promise that resolves when the runtime is ready to use. @@ -821,6 +834,9 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession startPromise.error(err); this._startingSessionsBySessionMapKey.delete(sessionMapKey); this._startingConsolesByLanguageId.delete(runtimeMetadata.languageId); + if (notebookUri) { + this._startingNotebooksByNotebookUri.delete(notebookUri); + } // Re-throw the error. throw err; From d58c28ccffcdb479818eec42e75a3b969e7c8e1d Mon Sep 17 00:00:00 2001 From: seem Date: Sat, 9 Nov 2024 13:55:19 +0200 Subject: [PATCH 03/45] throw an error when starting a runtime for a notebook while another runtime is already running --- .../runtimeSession/common/runtimeSession.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 3d053a01769..9e8f2b16a2f 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -461,6 +461,24 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession `is already starting for the notebook ${notebookUri.toString()}. ` + `Request source: ${source}`); } + + // If there is already a runtime running for the notebook, throw an error. + const runningLanguageRuntime = this._notebookSessionsByNotebookUri.get(notebookUri); + if (runningLanguageRuntime) { + const metadata = runningLanguageRuntime.runtimeMetadata; + if (metadata.runtimeId === languageRuntime.runtimeId) { + // If the runtime that is running is the one we were just asked + // to start, we're technically in good shape since the runtime + // is already running! + return runningLanguageRuntime.sessionId; + } else { + throw new Error(`A notebook for ` + + `${formatLanguageRuntimeMetadata(languageRuntime)} ` + + `cannot be started because a notebook for ` + + `${formatLanguageRuntimeMetadata(metadata)} is already running ` + + `for the URI ${notebookUri.toString()}.`); + } + } } // If the workspace is not trusted, defer starting the runtime until the From 19368eb028513c943f116b602fe4b1450f29d7ec Mon Sep 17 00:00:00 2001 From: seem Date: Sat, 9 Nov 2024 15:58:06 +0200 Subject: [PATCH 04/45] update restoreRuntimeSession: add validation; coalesce concurrent requests --- .../runtimeSession/common/runtimeSession.ts | 176 +++++++++++------- 1 file changed, 113 insertions(+), 63 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 9e8f2b16a2f..86f6709908d 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -419,66 +419,9 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession throw new Error(`No language runtime with id '${runtimeId}' was found.`); } - // If there is already a runtime starting for the language, throw an error. - if (sessionMode === LanguageRuntimeSessionMode.Console) { - const startingLanguageRuntime = this._startingConsolesByLanguageId.get( - languageRuntime.languageId); - if (startingLanguageRuntime) { - throw new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(languageRuntime)} cannot be started because language runtime ${formatLanguageRuntimeMetadata(startingLanguageRuntime)} is already starting for the language. Request source: ${source}`); - } - - // If there is already a runtime running for the language, throw an error. - const runningLanguageRuntime = - this._consoleSessionsByLanguageId.get(languageRuntime.languageId); - if (runningLanguageRuntime) { - const metadata = runningLanguageRuntime.runtimeMetadata; - if (metadata.runtimeId === runtimeId) { - // If the runtime that is running is the one we were just asked - // to start, we're technically in good shape since the runtime - // is already running! - return runningLanguageRuntime.sessionId; - } else { - throw new Error(`A console for ` + - `${formatLanguageRuntimeMetadata(languageRuntime)} ` + - `cannot be started because a console for ` + - `${formatLanguageRuntimeMetadata(metadata)} is already running ` + - `for the ${metadata.languageName} language.`); - } - } - } else if (sessionMode === LanguageRuntimeSessionMode.Notebook) { - // If no notebook URI is provided, throw an error. - if (!notebookUri) { - throw new Error(`A notebook URI must be provided when starting a notebook session.`); - } - - // If there is already a runtime starting for the notebook, throw an error. - const startingLanguageRuntime = this._startingNotebooksByNotebookUri.get(notebookUri); - if (startingLanguageRuntime) { - throw new Error(`Session for language runtime ` + - `${formatLanguageRuntimeMetadata(languageRuntime)} ` + - `cannot be started because language runtime ` + - `${formatLanguageRuntimeMetadata(startingLanguageRuntime)} ` + - `is already starting for the notebook ${notebookUri.toString()}. ` + - `Request source: ${source}`); - } - - // If there is already a runtime running for the notebook, throw an error. - const runningLanguageRuntime = this._notebookSessionsByNotebookUri.get(notebookUri); - if (runningLanguageRuntime) { - const metadata = runningLanguageRuntime.runtimeMetadata; - if (metadata.runtimeId === languageRuntime.runtimeId) { - // If the runtime that is running is the one we were just asked - // to start, we're technically in good shape since the runtime - // is already running! - return runningLanguageRuntime.sessionId; - } else { - throw new Error(`A notebook for ` + - `${formatLanguageRuntimeMetadata(languageRuntime)} ` + - `cannot be started because a notebook for ` + - `${formatLanguageRuntimeMetadata(metadata)} is already running ` + - `for the URI ${notebookUri.toString()}.`); - } - } + const runningSessionId = this.validateRuntimeSessionStart(sessionMode, languageRuntime, notebookUri, source); + if (runningSessionId) { + return runningSessionId; } // If the workspace is not trusted, defer starting the runtime until the @@ -505,6 +448,16 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession runtimeMetadata: ILanguageRuntimeMetadata, sessionMetadata: IRuntimeSessionMetadata): Promise { + // See if we are already starting the requested session. If we + // are, return the promise that resolves when the session is ready to + // use. This makes it possible for multiple requests to start the same + // session to be coalesced. + const startingRuntimePromise = this._startingSessionsBySessionMapKey.get( + getSessionMapKey(sessionMetadata.sessionMode, runtimeMetadata.runtimeId, sessionMetadata.notebookUri)); + if (startingRuntimePromise && !startingRuntimePromise.isSettled) { + return startingRuntimePromise.p.then(() => { }); + } + // Ensure that the runtime is registered. const languageRuntime = this._languageRuntimeService.getRegisteredRuntime( runtimeMetadata.runtimeId); @@ -514,9 +467,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession this._languageRuntimeService.registerRuntime(runtimeMetadata); } - // Add the runtime to the starting runtimes, if it's a console session. - if (sessionMetadata.sessionMode === LanguageRuntimeSessionMode.Console) { - this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); + const runningSessionId = this.validateRuntimeSessionStart( + sessionMetadata.sessionMode, runtimeMetadata, sessionMetadata.notebookUri); + if (runningSessionId) { + return; } // Create a promise that resolves when the runtime is ready to use. @@ -525,6 +479,14 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession sessionMetadata.sessionMode, runtimeMetadata.runtimeId, sessionMetadata.notebookUri); this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + // Add the runtime to the starting runtimes. + if (sessionMetadata.sessionMode === LanguageRuntimeSessionMode.Console) { + this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); + } else if (sessionMetadata.sessionMode === LanguageRuntimeSessionMode.Notebook && + sessionMetadata.notebookUri) { + this._startingNotebooksByNotebookUri.set(sessionMetadata.notebookUri, runtimeMetadata); + } + // We should already have a session manager registered, since we can't // get here until the extension host has been activated. if (this._sessionManagers.length === 0) { @@ -1081,6 +1043,94 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession })); } + /** + * Validate whether a runtime session can be started. + * + * @param sessionMode The mode of the new session. + * @param languageRuntime The metadata of the runtime to start. + * @param notebookUri The notebook URI to attach to the session, if any. + * @param source The source of the request to start the runtime, if known. + * @throws An error if the session cannot be started. + * @returns A session ID if a session is already running that matches the request, or undefined. + */ + private validateRuntimeSessionStart( + sessionMode: LanguageRuntimeSessionMode, + languageRuntime: ILanguageRuntimeMetadata, + notebookUri: URI | undefined, + source?: string, + ): string | undefined { + // If there is already a runtime starting for the language, throw an error. + if (sessionMode === LanguageRuntimeSessionMode.Console) { + const startingLanguageRuntime = this._startingConsolesByLanguageId.get( + languageRuntime.languageId); + if (startingLanguageRuntime) { + throw new Error(`Session for language runtime ` + + `${formatLanguageRuntimeMetadata(languageRuntime)} ` + + `cannot be started because language runtime ` + + `${formatLanguageRuntimeMetadata(startingLanguageRuntime)} ` + + `is already starting for the language.` + + (source ? ` Request source: ${source}` : ``)); + } + + // If there is already a runtime running for the language, throw an error. + const runningLanguageRuntime = + this._consoleSessionsByLanguageId.get(languageRuntime.languageId); + if (runningLanguageRuntime) { + const metadata = runningLanguageRuntime.runtimeMetadata; + if (metadata.runtimeId === languageRuntime.runtimeId) { + // If the runtime that is running is the one we were just asked + // to start, we're technically in good shape since the runtime + // is already running! + return runningLanguageRuntime.sessionId; + } else { + throw new Error(`A console for ` + + `${formatLanguageRuntimeMetadata(languageRuntime)} ` + + `cannot be started because a console for ` + + `${formatLanguageRuntimeMetadata(metadata)} is already running ` + + `for the ${metadata.languageName} language.` + + (source ? ` Request source: ${source}` : ``)); + } + } + } else if (sessionMode === LanguageRuntimeSessionMode.Notebook) { + // If no notebook URI is provided, throw an error. + if (!notebookUri) { + throw new Error(`A notebook URI must be provided when starting a notebook session.`); + } + + // If there is already a runtime starting for the notebook, throw an error. + const startingLanguageRuntime = this._startingNotebooksByNotebookUri.get(notebookUri); + if (startingLanguageRuntime) { + throw new Error(`Session for language runtime ` + + `${formatLanguageRuntimeMetadata(languageRuntime)} ` + + `cannot be started because language runtime ` + + `${formatLanguageRuntimeMetadata(startingLanguageRuntime)} ` + + `is already starting for the notebook ${notebookUri.toString()}.` + + (source ? ` Request source: ${source}` : ``)); + } + + // If there is already a runtime running for the notebook, throw an error. + const runningLanguageRuntime = this._notebookSessionsByNotebookUri.get(notebookUri); + if (runningLanguageRuntime) { + const metadata = runningLanguageRuntime.runtimeMetadata; + if (metadata.runtimeId === languageRuntime.runtimeId) { + // If the runtime that is running is the one we were just asked + // to start, we're technically in good shape since the runtime + // is already running! + return runningLanguageRuntime.sessionId; + } else { + throw new Error(`A notebook for ` + + `${formatLanguageRuntimeMetadata(languageRuntime)} ` + + `cannot be started because a notebook for ` + + `${formatLanguageRuntimeMetadata(metadata)} is already running ` + + `for the URI ${notebookUri.toString()}.` + + (source ? ` Request source: ${source}` : ``)); + } + } + } + + return undefined; + } + /** * Updates the session maps (for active consoles, notebooks, etc.), after a * session exits. From bb7df675e82ca92a3973215009bd8b88e674ac40 Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 15:19:27 +0200 Subject: [PATCH 05/45] fix leaked disposable --- .../services/runtimeSession/common/runtimeSession.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 86f6709908d..0f4b49f842b 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -627,18 +627,19 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession `${formatLanguageRuntimeMetadata(metadata)} (Source: ${source}) ` + `because workspace trust has not been granted. ` + `The runtime will be started when workspace trust is granted.`); - this._workspaceTrustManagementService.onDidChangeTrust((trusted) => { + const disposable = this._register(this._workspaceTrustManagementService.onDidChangeTrust((trusted) => { if (!trusted) { // If the workspace is still not trusted, do nothing. - return ''; + return; } // If the workspace is trusted, start the runtime. + disposable.dispose(); this._logService.info(`Language runtime ` + `${formatLanguageRuntimeMetadata(metadata)} ` + `automatically starting after workspace trust was granted. ` + `Source: ${source}`); - return this.doAutoStartRuntime(metadata, source); - }); + this.doAutoStartRuntime(metadata, source); + })); } return ''; From 974aa05117515dabd5c330c4fc904a145f197b4e Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 15:19:44 +0200 Subject: [PATCH 06/45] log uncaught exception --- .../services/runtimeSession/common/runtimeSession.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 0f4b49f842b..3a852db9167 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -831,6 +831,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession startPromise.error(err); } + // It's possible that startPromise is never awaited, so we log any errors here + // at the debug level since we still expect the error to be handled/logged elsewhere. + startPromise.p.catch(err => this._logService.debug(`Error starting runtime session: ${err}`)); + return sessionId; } From a7197bdae8361cfc6096665492399e790f66b8c0 Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 15:20:35 +0200 Subject: [PATCH 07/45] update test session manager for tests --- .../test/common/testRuntimeSessionService.ts | 2 +- .../test/common/positronWorkbenchTestServices.ts | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts b/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts index 9ffc0958465..d6e6c187b0f 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts @@ -76,7 +76,7 @@ export function createTestLanguageRuntimeMetadata( disposables.add(languageRuntimeService.registerRuntime(runtime)); // Register the test runtime manager. - const manager = new TestRuntimeSessionManager(); + const manager = TestRuntimeSessionManager.instance; disposables.add(runtimeSessionService.registerSessionManager(manager)); return runtime; diff --git a/src/vs/workbench/test/common/positronWorkbenchTestServices.ts b/src/vs/workbench/test/common/positronWorkbenchTestServices.ts index 32923d7e7d3..028f06ec31e 100644 --- a/src/vs/workbench/test/common/positronWorkbenchTestServices.ts +++ b/src/vs/workbench/test/common/positronWorkbenchTestServices.ts @@ -107,6 +107,10 @@ export class TestPositronModalDialogService implements IPositronModalDialogsServ } } export class TestRuntimeSessionManager implements ILanguageRuntimeSessionManager { + public static readonly instance = new TestRuntimeSessionManager(); + + private _validateMetadata?: (metadata: ILanguageRuntimeMetadata) => Promise; + async managesRuntime(runtime: ILanguageRuntimeMetadata): Promise { return true; } @@ -119,7 +123,14 @@ export class TestRuntimeSessionManager implements ILanguageRuntimeSessionManager return new TestLanguageRuntimeSession(sessionMetadata, runtimeMetadata); } - validateMetadata(metadata: ILanguageRuntimeMetadata): Promise { - throw new Error('Method not implemented'); + async validateMetadata(metadata: ILanguageRuntimeMetadata): Promise { + if (this._validateMetadata) { + return this._validateMetadata(metadata); + } + return metadata; + } + + setValidateMetadata(handler: (metadata: ILanguageRuntimeMetadata) => Promise): void { + this._validateMetadata = handler; } } From 5f1a33f7f195c629d7c84e12ef6f21d18307dbad Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 15:53:50 +0200 Subject: [PATCH 08/45] checkpoint tests --- .../test/common/runtimeSession.test.ts | 995 ++++++++++++++++++ 1 file changed, 995 insertions(+) create mode 100644 src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts new file mode 100644 index 00000000000..430b0e79ab5 --- /dev/null +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -0,0 +1,995 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2024 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +import { strict as assert } from 'assert'; +import * as sinon from 'sinon'; +import { CancellationError } from 'vs/base/common/errors'; +import { URI } from 'vs/base/common/uri'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; +import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; +import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService'; +import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock'; +import { IWorkspaceTrustManagementService } from 'vs/platform/workspace/common/workspaceTrust'; +// import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, LanguageRuntimeSessionMode, RuntimeExitReason, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; +import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, ILanguageRuntimeService, LanguageRuntimeSessionMode, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; +import { ILanguageRuntimeSession, IRuntimeSessionMetadata, IRuntimeSessionService, IRuntimeSessionWillStartEvent } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; +import { TestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession'; +import { createRuntimeServices, createTestLanguageRuntimeMetadata, startTestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService'; +import { TestRuntimeSessionManager } from 'vs/workbench/test/common/positronWorkbenchTestServices'; +import { TestWorkspaceTrustManagementService } from 'vs/workbench/test/common/workbenchTestServices'; + +type IStartSessionTask = (runtimeMetadata?: ILanguageRuntimeMetadata) => Promise; +type IValidator = (obj: T) => void; + +suite('Positron - RuntimeSessionService', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + const sessionName = 'Test session'; + const startReason = 'Test requested to start a runtime session'; + const notebookUri = URI.file('/path/to/notebook'); + let instantiationService: TestInstantiationService; + let languageRuntimeService: ILanguageRuntimeService; + let runtimeSessionService: IRuntimeSessionService; + let runtime: ILanguageRuntimeMetadata; + let anotherRuntime: ILanguageRuntimeMetadata; + let unregisteredRuntime: ILanguageRuntimeMetadata; + + setup(() => { + instantiationService = disposables.add(new TestInstantiationService()); + createRuntimeServices(instantiationService, disposables); + languageRuntimeService = instantiationService.get(ILanguageRuntimeService); + runtimeSessionService = instantiationService.get(IRuntimeSessionService); + runtime = createTestLanguageRuntimeMetadata(instantiationService, disposables); + anotherRuntime = createTestLanguageRuntimeMetadata(instantiationService, disposables); + unregisteredRuntime = { runtimeId: 'unregistered-runtime-id' } as ILanguageRuntimeMetadata; + }); + + function startSession( + runtimeMetadata = runtime, + sessionMode: LanguageRuntimeSessionMode, + notebookUri?: URI, + ) { + return startTestLanguageRuntimeSession( + instantiationService, + disposables, + { + runtime: runtimeMetadata, + sessionName, + startReason, + sessionMode, + notebookUri, + }, + ); + } + + function startConsole(runtimeMetadata?: ILanguageRuntimeMetadata) { + return startSession(runtimeMetadata, LanguageRuntimeSessionMode.Console); + } + + function startNotebook(runtimeMetadata?: ILanguageRuntimeMetadata, notebookUri_ = notebookUri) { + return startSession(runtimeMetadata, LanguageRuntimeSessionMode.Notebook, notebookUri_); + } + + interface IServiceState { + hasStartingOrRunningConsole?: boolean; + consoleSession?: ILanguageRuntimeSession; + notebookSession?: ILanguageRuntimeSession; + notebookSessionForNotebookUri?: ILanguageRuntimeSession; + activeSessions?: ILanguageRuntimeSession[]; + } + + function assertServiceState(expectedState?: IServiceState): void { + // Check the console session state. + assert.equal( + runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), + expectedState?.hasStartingOrRunningConsole ?? false, + ); + assert.equal( + runtimeSessionService.getConsoleSessionForLanguage(runtime.languageId)?.sessionId, + expectedState?.consoleSession?.sessionId, + ); + assert.equal( + runtimeSessionService.getConsoleSessionForRuntime(runtime.runtimeId)?.sessionId, + expectedState?.consoleSession?.sessionId, + ); + assert.equal( + runtimeSessionService.getSession(expectedState?.consoleSession?.sessionId ?? '')?.sessionId, + expectedState?.consoleSession?.sessionId, + ); + + // Check the notebook session state. + assert.equal( + runtimeSessionService.getSession(expectedState?.notebookSession?.sessionId ?? '')?.sessionId, + expectedState?.notebookSession?.sessionId, + ); + assert.equal( + runtimeSessionService.getNotebookSessionForNotebookUri(notebookUri)?.sessionId, + expectedState?.notebookSessionForNotebookUri?.sessionId, + ); + + // Check the global state. + assert.deepEqual( + runtimeSessionService.activeSessions?.map(session => session.sessionId), + expectedState?.activeSessions?.map(session => session?.sessionId) ?? + [expectedState?.consoleSession?.sessionId, expectedState?.notebookSession?.sessionId].filter(session => Boolean(session)), + ); + } + + async function testCallSuccessively(task: () => Promise, validate: IValidator) { + const result1 = await task(); + const result2 = await task(); + const result3 = await task(); + + assert.equal(result1, result2); + assert.equal(result2, result3); + + validate(result1); + } + + async function testCallConcurrently(task: () => Promise, validate: (obj: T) => void) { + const [result1, result2, result3] = await Promise.all([task(), task(), task()]); + + assert.equal(result1, result2); + assert.equal(result2, result3); + + validate(result1); + } + + async function testStartSuccessively(start: IStartSessionTask) { + await testCallSuccessively(start, (session) => assertSessionIsStarting(session)); + } + + async function testStartConcurrently(start: IStartSessionTask) { + await testCallConcurrently(start, (session) => assertSessionIsStarting(session)); + } + + async function testStartConsoleWhileAnotherIsStarting(start: IStartSessionTask, source?: string) { + await assert.rejects( + Promise.all([ + start(), + start(anotherRuntime), + ]), + new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} ` + + `cannot be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already starting for the language.` + + (source ? ` Request source: ${startReason}` : '')), + ); + } + + async function testStartConsoleWhileAnotherIsRunning(start: IStartSessionTask, source?: string) { + await start(); + await assert.rejects( + start(anotherRuntime), + new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the ${runtime.languageName} language.` + + (source ? ` Request source: ${startReason}` : '')), + ); + } + + async function testStartNotebookWhileAnotherIsStarting(start: IStartSessionTask, source?: string) { + await assert.rejects( + Promise.all([ + start(), + start(anotherRuntime), + ]), + new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already starting for the notebook ${notebookUri.toString()}.` + + (source ? ` Request source: ${startReason}` : '')) + ); + } + + async function testStartNotebookWhileAnotherIsRunning(start: IStartSessionTask, source?: string) { + await start(); + await assert.rejects( + start(anotherRuntime), + new Error(`A notebook for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a notebook for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the URI ${notebookUri.toString()}.` + + (source ? ` Request source: ${startReason}` : '')), + ); + } + + async function testStartReturnsExpectedSession(start: IStartSessionTask) { + const session = await start(); + + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assert.equal(session.metadata.sessionName, sessionName); + assert.equal(session.metadata.startReason, startReason); + assert.equal(session.runtimeMetadata, runtime); + + if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { + assert.equal(session.metadata.notebookUri, undefined); + } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { + assert.equal(session.metadata.notebookUri, notebookUri); + } else { + throw new Error(`Unexpected session mode: ${session.metadata.sessionMode}`); + } + } + + async function testStartSetsExpectedServiceState( + start: IStartSessionTask, + verifyWhileStarting: () => void, + verifyAfterStarted: (session: ILanguageRuntimeSession) => void, + ) { + // Check the initial state. + assertServiceState(); + + const promise = start(); + + // Check the state while starting. + verifyWhileStarting(); + + const session = await promise; + + // Check the state after starting. + verifyAfterStarted(session); + } + + function testStartConsoleSetsExpectedServiceState(start: IStartSessionTask) { + return testStartSetsExpectedServiceState( + start, + () => assertServiceState({ hasStartingOrRunningConsole: true }), + session => assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }), + ); + } + + function testStartNotebookSetsExpectedServiceState(start: IStartSessionTask) { + return testStartSetsExpectedServiceState( + start, + () => assertServiceState(), + session => assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }), + ); + } + + async function testStartFiresOnWillStartSession(start: IStartSessionTask, verify: () => void) { + let error: Error | undefined; + const target = sinon.spy(({ session }: IRuntimeSessionWillStartEvent) => { + try { + // TODO: Should onWillStartSession only fire once? + if (target.callCount > 1) { + return; + } + assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); + verify(); + } catch (e) { + error = e; + } + }); + disposables.add(runtimeSessionService.onWillStartSession(target)); + const session = await start(); + + // TODO: Should onWillStartSession only fire once? + sinon.assert.calledTwice(target); + sinon.assert.alwaysCalledWithExactly(target, { isNew: true, session }); + assert.ifError(error); + } + + async function testStartFiresOnDidStartRuntime( + start: IStartSessionTask, + verify: (session: ILanguageRuntimeSession) => void, + ) { + let error: Error | undefined; + const target = sinon.stub<[e: ILanguageRuntimeSession]>().callsFake(session => { + try { + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + verify(session); + } catch (e) { + error = e; + } + }); + disposables.add(runtimeSessionService.onDidStartRuntime(target)); + + const session = await start(); + + sinon.assert.calledOnceWithExactly(target, session); + assert.ifError(error); + } + + async function testEncountersSessionStartError( + start: IStartSessionTask, + verify: (session: ILanguageRuntimeSession) => void, + ) { + // Listen to the onWillStartSession event and stub session.start() to throw an error. + const willStartSession = sinon.spy((e: IRuntimeSessionWillStartEvent) => { + sinon.stub(e.session, 'start').rejects(new Error('Session failed to start')); + }); + disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); + + const didFailStartRuntime = sinon.spy(); + disposables.add(runtimeSessionService.onDidFailStartRuntime(didFailStartRuntime)); + + const didStartRuntime = sinon.spy(); + disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); + + const session = await start(); + + assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); + + verify(session); + + sinon.assert.calledOnceWithExactly(didFailStartRuntime, session); + sinon.assert.callOrder(willStartSession, didFailStartRuntime); + sinon.assert.notCalled(didStartRuntime); + } + + async function testStartFiresEventsInOrder(start: IStartSessionTask) { + const willStartSession = sinon.spy(); + disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); + + const didStartRuntime = sinon.spy(); + disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); + + await start(); + + sinon.assert.callOrder(willStartSession, didStartRuntime); + } + + async function testStartUnknownRuntime(start: IStartSessionTask = startNotebook) { + const runtimeId = 'unknown-runtime-id'; + await assert.rejects( + start({ runtimeId } as ILanguageRuntimeMetadata,), + new Error(`No language runtime with id '${runtimeId}' was found.`), + ); + } + + function assertSessionIsStarting(session: ILanguageRuntimeSession) { + if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { + assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }); + } + } + + suite('startNewRuntimeSession', () => { + test('start console returns the expected session', async () => { + await testStartReturnsExpectedSession(startConsole); + }); + + test('start notebook returns the expected session', async () => { + await testStartReturnsExpectedSession(startNotebook); + }); + + test('start console sets the expected service state', async () => { + await testStartConsoleSetsExpectedServiceState(startConsole); + }); + + test('start notebook sets the expected service state', async () => { + await testStartNotebookSetsExpectedServiceState(startNotebook); + }); + + test('start console fires onWillStartSession', async () => { + await testStartFiresOnWillStartSession( + startConsole, + () => assertServiceState({ hasStartingOrRunningConsole: true }), + ); + }); + + test('start notebook fires onWillStartSession', async () => { + await testStartFiresOnWillStartSession( + startNotebook, + () => assertServiceState(), + ); + }); + + test('start console fires onDidStartRuntime', async () => { + await testStartFiresOnDidStartRuntime( + startConsole, + session => assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }), + ); + }); + + test('start notebook fires onDidStartRuntime', async () => { + await testStartFiresOnDidStartRuntime( + startNotebook, + session => assertSessionIsStarting(session), + ); + }); + + test('start console fires events in order', async () => { + await testStartFiresEventsInOrder(startConsole); + }); + + test('start notebook fires events in order', async () => { + await testStartFiresEventsInOrder(startNotebook); + }); + + test('start console sets foregroundSession', async () => { + const target = sinon.spy(); + disposables.add(runtimeSessionService.onDidChangeForegroundSession(target)); + + const session = await startConsole(); + + assert.equal(runtimeSessionService.foregroundSession, session); + + await waitForRuntimeState(session, RuntimeState.Ready); + + // TODO: Feels a bit surprising that this isn't fired. It's because we set the private + // _foregroundSession property instead of the setter. When the 'ready' state is + // entered, we skip setting foregroundSession because it already matches the session. + sinon.assert.notCalled(target); + }); + + test('start console for unknown runtime', async () => { + await testStartUnknownRuntime(startConsole); + }); + + test('start notebook for unknown runtime', async () => { + await testStartUnknownRuntime(startNotebook); + }); + + test('start notebook without notebook uri', async () => { + await assert.rejects( + startSession(undefined, LanguageRuntimeSessionMode.Notebook, undefined), + new Error('A notebook URI must be provided when starting a notebook session.'), + ); + }); + + test('start console encounters session.start() error', async () => { + await testEncountersSessionStartError( + startConsole, + session => { + // TODO: Seems unexpected that some of these are defined and others not. + assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); + assert.equal(runtimeSessionService.getConsoleSessionForLanguage(runtime.languageId), undefined); + assert.equal(runtimeSessionService.getConsoleSessionForRuntime(runtime.runtimeId), session); + assert.equal(runtimeSessionService.getSession(session.sessionId), session); + assert.deepEqual(runtimeSessionService.activeSessions, [session]); + } + ); + }); + + test('start notebook encounters session.start() error', async () => { + await testEncountersSessionStartError( + startNotebook, + session => { + // TODO: Should failed notebook sessions be included in activeSessions? + assertServiceState({ activeSessions: [session] }); + }, + ); + }); + + test('start console and notebook from the same runtime concurrently', async () => { + // Consoles and notebooks shouldn't interfere with each other, even for the same runtime. + const [consoleSession, notebookSession] = await Promise.all([ + startConsole(), + startNotebook(), + ]); + + assert.equal(consoleSession.getRuntimeState(), RuntimeState.Starting); + assert.equal(notebookSession.getRuntimeState(), RuntimeState.Starting); + + assertServiceState({ + hasStartingOrRunningConsole: true, + consoleSession, + notebookSession, + notebookSessionForNotebookUri: notebookSession, + activeSessions: [consoleSession, notebookSession], + }); + }); + + test('start console while another runtime is starting for the language', async () => { + await testStartConsoleWhileAnotherIsStarting(startConsole, startReason); + }); + + test('start notebook while another runtime is starting for the notebook', async () => { + await testStartNotebookWhileAnotherIsStarting(startNotebook, startReason); + }); + + test('start console while another runtime is running for the language', async () => { + await testStartConsoleWhileAnotherIsRunning(startConsole, startReason); + }); + + test('start notebook while another runtime is running for the notebook', async () => { + await testStartNotebookWhileAnotherIsRunning(startNotebook, startReason); + }); + + test('start console successively', async () => { + await testStartSuccessively(startConsole); + }); + + test('start notebook successively', async () => { + await testStartSuccessively(startNotebook); + }); + + test('start console concurrently', async () => { + await testStartConcurrently(startConsole); + }); + + test('start notebook concurrently', async () => { + await testStartConcurrently(startNotebook); + }); + }); + + async function restoreSession( + sessionMetadata: IRuntimeSessionMetadata, runtimeMetadata = runtime, + ) { + await runtimeSessionService.restoreRuntimeSession(runtimeMetadata, sessionMetadata); + + // Ensure that the session gets disposed after the test. + const session = runtimeSessionService.getSession(sessionMetadata.sessionId); + assert.ok(session instanceof TestLanguageRuntimeSession); + disposables.add(session); + + return session; + } + + function restoreConsole(runtimeMetadata?: ILanguageRuntimeMetadata) { + return restoreSession(consoleSessionMetadata, runtimeMetadata); + } + + function restoreNotebook(runtimeMetadata?: ILanguageRuntimeMetadata) { + return restoreSession(notebookSessionMetadata, runtimeMetadata); + } + + const consoleSessionMetadata = { + sessionId: 'test-session-id', + sessionName: 'Test session', + sessionMode: LanguageRuntimeSessionMode.Console, + createdTimestamp: Date.now(), + startReason, + } as IRuntimeSessionMetadata; + + const notebookSessionMetadata = { + ...consoleSessionMetadata, + sessionMode: LanguageRuntimeSessionMode.Notebook, + notebookUri, + }; + + suite('restoreRuntimeSession', () => { + test('restore console returns the expected session', async () => { + await testStartReturnsExpectedSession(restoreConsole); + }); + + test('restore notebook returns the expected session', async () => { + await testStartReturnsExpectedSession(restoreNotebook); + }); + + test('restore console sets the expected service state', async () => { + await testStartConsoleSetsExpectedServiceState(restoreConsole); + }); + + test('restore notebook sets the expected service state', async () => { + await testStartNotebookSetsExpectedServiceState(restoreNotebook); + }); + + test('restore console registers runtime if unregistered', async () => { + // The runtime should not yet be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + + await restoreConsole(unregisteredRuntime); + + // The runtime should now be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); + }); + + test('restore notebook without notebook URI', async () => { + await assert.rejects( + restoreSession({ ...consoleSessionMetadata, sessionMode: LanguageRuntimeSessionMode.Notebook }), + new Error('A notebook URI must be provided when starting a notebook session.'), + ); + }); + + test('restore console while another runtime is starting for the language', async () => { + await testStartConsoleWhileAnotherIsStarting(restoreConsole); + }); + + test('restore notebook while another runtime is starting for the notebook', async () => { + await testStartNotebookWhileAnotherIsStarting(restoreNotebook); + }); + + test('restore console while another runtime is running for the language', async () => { + await testStartConsoleWhileAnotherIsRunning(restoreConsole); + }); + + test('restore notebook while another runtime is running for the notebook', async () => { + await testStartNotebookWhileAnotherIsRunning(restoreNotebook); + }); + + test('restore console successively', async () => { + await testStartSuccessively(restoreConsole); + }); + + test('restore notebook successively', async () => { + await testStartSuccessively(restoreNotebook); + }); + + test('restore console concurrently', async () => { + await testStartConcurrently(restoreConsole); + }); + + test('restore notebook concurrently', async () => { + await testStartConcurrently(restoreNotebook); + }); + }); + + suite('autoStartRuntime', () => { + async function autoStartSession(runtimeMetadata = runtime) { + const sessionId = await runtimeSessionService.autoStartRuntime(runtimeMetadata, 'Test requested to auto-start a runtime'); + if (!sessionId) { + return undefined; + } + const session = runtimeSessionService.getSession(sessionId); + assert.ok(session); + disposables.add(session); + return session; + } + + let configService: TestConfigurationService; + let workspaceTrustManagementService: TestWorkspaceTrustManagementService; + let manager: TestRuntimeSessionManager; + + setup(() => { + configService = instantiationService.get(IConfigurationService) as TestConfigurationService; + workspaceTrustManagementService = instantiationService.get(IWorkspaceTrustManagementService) as TestWorkspaceTrustManagementService; + manager = TestRuntimeSessionManager.instance; + + // Enable automatic startup. + configService.setUserConfiguration('positron.interpreters.automaticStartup', true); + + // Trust the workspace. + workspaceTrustManagementService.setWorkspaceTrust(true); + }); + + test('auto start console in a trusted workspace', async () => { + const promise = autoStartSession(); + + assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); + + const session = await promise; + + // TODO: Do we need this? It's the same as starting a session. + assert.ok(session); + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assert.equal(session.metadata.sessionName, runtime.runtimeName); + assert.equal(session.metadata.sessionMode, LanguageRuntimeSessionMode.Console); + assert.equal(session.metadata.startReason, 'Test requested to auto-start a runtime'); + assert.equal(session.runtimeMetadata, runtime); + }); + + // TODO: Should auto starting a notebook error? + + test('auto start validates runtime if unregistered', async () => { + // The runtime should not yet be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + + manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + return { ...metadata, extraRuntimeData: { someNewKey: 'someNewValue' } }; + }); + + await autoStartSession(unregisteredRuntime); + + // The *validated* runtime should now be registered. + assert.deepEqual( + languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), + { ...unregisteredRuntime, extraRuntimeData: { someNewKey: 'someNewValue' } } + ); + }); + + test('auto start encounters runtime validation error', async () => { + // The runtime should not yet be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + + const error = new Error('Failed to validate runtime metadata'); + manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + throw error; + }); + + await assert.rejects(autoStartSession(unregisteredRuntime), error); + + // The runtime should still not be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + }); + + test('auto start console does nothing if automatic startup is disabled', async () => { + configService.setUserConfiguration('positron.interpreters.automaticStartup', false); + + const session = await autoStartSession(); + assert.equal(session, undefined); + + // TODO: Do we also need to check the session state? + }); + + test('auto start console in an untrusted workspace starts when trust is granted', async () => { + workspaceTrustManagementService.setWorkspaceTrust(false); + + const sessionId = await runtimeSessionService.autoStartRuntime(runtime, 'Test requested to auto-start a runtime'); + assert.equal(sessionId, ''); + + workspaceTrustManagementService.setWorkspaceTrust(true); + + await new Promise(resolve => { + disposables.add(runtimeSessionService.onDidStartRuntime(session => { + if (session.runtimeMetadata === runtime) { + disposables.add(session); + resolve(); + } + })); + }); + + // TODO: We should probably check more things here? + }); + + // TODO: Should we handle this? + test.skip('auto start console while another runtime is starting for the language', async () => { + await assert.rejects( + Promise.all([ + autoStartSession(), + autoStartSession(anotherRuntime), + ]), + new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} ` + + `cannot be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already starting for the language.`), + ); + }); + + // TODO: Should we handle this? + test.skip('auto start console while another runtime is running for the language', async () => { + await autoStartSession(); + await assert.rejects( + autoStartSession(anotherRuntime), + new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the ${runtime.languageName} language.`), + ); + }); + + // TODO: Should we handle this? + test.skip('auto start console successively', async () => { + const session1 = await autoStartSession(); + const session2 = await autoStartSession(); + const session3 = await autoStartSession(); + + // Check that the same session was returned each time. + assert.equal(session1, session2); + assert.equal(session2, session3); + + // Check that only one session was started. + assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session1 }); + }); + + // TODO: Should we handle this? + test.skip('auto start console concurrently', async () => { + const [session1, session2, session3] = await Promise.all([ + autoStartSession(), + autoStartSession(), + autoStartSession(), + ]); + + // Check that the same session was returned. + assert.equal(session1, session2); + assert.equal(session2, session3); + + // Check that only one session was started. + assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session1 }); + }); + }); + + suite('selectRuntime', () => { + async function selectRuntime(runtimeMetadata = runtime) { + await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, 'Test requested to select a runtime'); + const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); + assert.ok(session, 'Expected a session to be started'); + disposables.add(session); + return session; + } + + test('select runtime', async () => { + await testStartConsoleSetsExpectedServiceState(selectRuntime); + }); + }); + + // TODO: Check sessionManager validation... + + // TODO: If workspace is trusted, + + // suite('shutdownNotebookSession', () => { + // test('shutdown notebook', async () => { + // const session = await startNotebook(); + + // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); + + // assert.equal(session.getRuntimeState(), RuntimeState.Exited); + // // TODO: The session is in activeSessions and returned by getSession but not by + // // getNotebookSessionForNotebookUri. Is that correct? This is also the only reason + // // we need a notebookForNotebookUri parameter in assertServiceState. + // assertServiceState({ notebookSession: session }); + // }); + + // test('shutdown notebook without running runtime', async () => { + // // It should not error, since it's already in the desired state. + // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); + // assertServiceState(); + // }); + + // test('shutdown notebook concurrently', async () => { + // const session = await startNotebook(); + + // await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // ]); + + // assert.equal(session.getRuntimeState(), RuntimeState.Exited); + // }); + + // test('shutdown notebook while starting', async () => { + // const [session,] = await Promise.all([ + // startNotebook(), + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // ]); + + // assert.equal(session.getRuntimeState(), RuntimeState.Exited); + // assertServiceState({ notebookSession: session }); + // }); + // }); + + + // function restartSession(sessionId: string) { + // return runtimeSessionService.restartSession( + // sessionId, 'Test requested to restart a runtime session' + // ); + // } + + // suite('restartSession', () => { + // test('restart console in "ready" state', async () => { + // const session = await startConsole(); + // await waitForRuntimeState(session, RuntimeState.Ready); + + // await restartSession(session.sessionId); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // }); + + // test('restart console in "starting" state', async () => { + // const session = await startConsole(); + + // await restartSession(session.sessionId); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // }); + + // test('restart console in "exited" state', async () => { + // const session = await startConsole(); + // await session.shutdown(RuntimeExitReason.Shutdown); + // await waitForRuntimeState(session, RuntimeState.Exited); + + // await restartSession(session.sessionId); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // }); + + // test('restart session with unknown session id', async () => { + // const sessionId = 'unknown-session-id'; + // assert.rejects( + // restartSession(sessionId), + // new Error(`No session with ID '${sessionId}' was found.`), + // ); + // }); + + // test('restart console concurrently', async () => { + // const session = await startConsole(); + // await waitForRuntimeState(session, RuntimeState.Ready); + + // const target = sinon.spy(session, 'restart'); + + // await Promise.all([ + // restartSession(session.sessionId), + // restartSession(session.sessionId), + // restartSession(session.sessionId), + // ]); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + + // sinon.assert.calledOnce(target); + // }); + + // test('restart console successively', async () => { + // const session = await startConsole(); + + // const target = sinon.spy(session, 'restart'); + + // await waitForRuntimeState(session, RuntimeState.Ready); + // await restartSession(session.sessionId); + // await waitForRuntimeState(session, RuntimeState.Ready); + // await restartSession(session.sessionId); + // await waitForRuntimeState(session, RuntimeState.Ready); + // await restartSession(session.sessionId); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + + // sinon.assert.calledThrice(target); + // }); + + // }); + + // suite('queuing', () => { + // test('start notebook while shutting down', async () => { + // const session1 = await startNotebook(); + + // const [, session2,] = await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // startNotebook(), + // ]); + + // assert.equal(session1.getRuntimeState(), RuntimeState.Exited); + // assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ + // notebookSession: session2, + // notebookSessionForNotebookUri: session2, + // activeSessions: [session1, session2], + // }); + // }); + + // test('start notebook while restarting and in "exited" state', async () => { + // const session = await startNotebook(); + // await waitForRuntimeState(session, RuntimeState.Ready); + + // const target = sinon.spy(session, 'restart'); + + // const startPromise = new Promise(resolve => { + // const disposable = session.onDidChangeRuntimeState(state => { + // if (state === RuntimeState.Exited) { + // disposable.dispose(); + // resolve(startNotebook()); + // } + // }); + // }); + + // const [, session2,] = await Promise.all([ + // restartSession(session.sessionId), + // startPromise, + // // startNotebook(), + // ]); + + // assert.equal(session, session2); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ + // notebookSession: session, + // notebookSessionForNotebookUri: session, + // }); + + // sinon.assert.calledOnce(target); + // }); + + // test('restart notebook while shutting down', async () => { + // const session = await startNotebook(); + + // await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // restartSession(session.sessionId), + // ]); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertNotebookSessionIsStarted(session); + // }); + + // }); +}); + +async function waitForRuntimeState( + session: ILanguageRuntimeSession, + state: RuntimeState, + timeout = 10_000, +) { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + disposable.dispose(); + reject(new CancellationError()); + }, timeout); + + const disposable = session.onDidChangeRuntimeState(newState => { + if (newState === state) { + clearTimeout(timer); + disposable.dispose(); + resolve(); + } + }); + }); +} From 5883cd8a856892604c791f3fd3f242f7e51cdf7e Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 16:08:14 +0200 Subject: [PATCH 09/45] checkpoint 2 tests --- .../test/common/runtimeSession.test.ts | 348 +++++++----------- 1 file changed, 132 insertions(+), 216 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 430b0e79ab5..b5419da0693 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -263,6 +263,7 @@ suite('Positron - RuntimeSessionService', () => { // TODO: Should onWillStartSession only fire once? sinon.assert.calledTwice(target); + // TODO: isNew is false for restored sessions... sinon.assert.alwaysCalledWithExactly(target, { isNew: true, session }); assert.ifError(error); } @@ -343,60 +344,78 @@ suite('Positron - RuntimeSessionService', () => { } } - suite('startNewRuntimeSession', () => { - test('start console returns the expected session', async () => { - await testStartReturnsExpectedSession(startConsole); - }); + async function restoreSession( + sessionMetadata: IRuntimeSessionMetadata, runtimeMetadata = runtime, + ) { + await runtimeSessionService.restoreRuntimeSession(runtimeMetadata, sessionMetadata); - test('start notebook returns the expected session', async () => { - await testStartReturnsExpectedSession(startNotebook); - }); + // Ensure that the session gets disposed after the test. + const session = runtimeSessionService.getSession(sessionMetadata.sessionId); + assert.ok(session instanceof TestLanguageRuntimeSession); + disposables.add(session); - test('start console sets the expected service state', async () => { - await testStartConsoleSetsExpectedServiceState(startConsole); + return session; + } + + function restoreConsole(runtimeMetadata?: ILanguageRuntimeMetadata) { + return restoreSession(consoleSessionMetadata, runtimeMetadata); + } + + function restoreNotebook(runtimeMetadata?: ILanguageRuntimeMetadata) { + return restoreSession(notebookSessionMetadata, runtimeMetadata); + } + + const consoleSessionMetadata: IRuntimeSessionMetadata = { + sessionId: 'test-console-session-id', + sessionName: 'Test console session', + sessionMode: LanguageRuntimeSessionMode.Console, + createdTimestamp: Date.now(), + notebookUri: undefined, + startReason, + }; + + const notebookSessionMetadata: IRuntimeSessionMetadata = { + sessionId: 'test-notebook-session-id', + sessionName: 'Test notebook session', + sessionMode: LanguageRuntimeSessionMode.Notebook, + createdTimestamp: Date.now(), + notebookUri, + startReason, + }; + + function createStartTests( + startConsole: IStartSessionTask, + startNotebook: IStartSessionTask | undefined, + name: string, + startReason?: string, + ) { + test(`${name} console returns the expected session`, async () => { + await testStartReturnsExpectedSession(startConsole); }); - test('start notebook sets the expected service state', async () => { - await testStartNotebookSetsExpectedServiceState(startNotebook); + test(`${name} console sets the expected service state`, async () => { + await testStartConsoleSetsExpectedServiceState(startConsole); }); - test('start console fires onWillStartSession', async () => { + test(`${name} console fires onWillStartSession`, async () => { await testStartFiresOnWillStartSession( startConsole, () => assertServiceState({ hasStartingOrRunningConsole: true }), ); }); - test('start notebook fires onWillStartSession', async () => { - await testStartFiresOnWillStartSession( - startNotebook, - () => assertServiceState(), - ); - }); - - test('start console fires onDidStartRuntime', async () => { + test(`${name} console fires onDidStartRuntime`, async () => { await testStartFiresOnDidStartRuntime( startConsole, session => assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }), ); }); - test('start notebook fires onDidStartRuntime', async () => { - await testStartFiresOnDidStartRuntime( - startNotebook, - session => assertSessionIsStarting(session), - ); - }); - - test('start console fires events in order', async () => { + test(`${name} console fires events in order`, async () => { await testStartFiresEventsInOrder(startConsole); }); - test('start notebook fires events in order', async () => { - await testStartFiresEventsInOrder(startNotebook); - }); - - test('start console sets foregroundSession', async () => { + test(`${name} console sets foregroundSession`, async () => { const target = sinon.spy(); disposables.add(runtimeSessionService.onDidChangeForegroundSession(target)); @@ -412,22 +431,7 @@ suite('Positron - RuntimeSessionService', () => { sinon.assert.notCalled(target); }); - test('start console for unknown runtime', async () => { - await testStartUnknownRuntime(startConsole); - }); - - test('start notebook for unknown runtime', async () => { - await testStartUnknownRuntime(startNotebook); - }); - - test('start notebook without notebook uri', async () => { - await assert.rejects( - startSession(undefined, LanguageRuntimeSessionMode.Notebook, undefined), - new Error('A notebook URI must be provided when starting a notebook session.'), - ); - }); - - test('start console encounters session.start() error', async () => { + test(`${name} console encounters session.start() error`, async () => { await testEncountersSessionStartError( startConsole, session => { @@ -441,7 +445,60 @@ suite('Positron - RuntimeSessionService', () => { ); }); - test('start notebook encounters session.start() error', async () => { + test(`${name} console while another runtime is starting for the language`, async () => { + await testStartConsoleWhileAnotherIsStarting(startConsole, startReason); + }); + + test(`${name} console while another runtime is running for the language`, async () => { + await testStartConsoleWhileAnotherIsRunning(startConsole, startReason); + }); + + test(`${name} console successively`, async () => { + await testStartSuccessively(startConsole); + }); + + test(`${name} console concurrently`, async () => { + await testStartConcurrently(startConsole); + }); + + if (!startNotebook) { + return; + } + + test(`${name} notebook returns the expected session`, async () => { + await testStartReturnsExpectedSession(startNotebook); + }); + + test(`${name} notebook sets the expected service state`, async () => { + await testStartNotebookSetsExpectedServiceState(startNotebook); + }); + + test.skip(`${name} notebook fires onWillStartSession`, async () => { + await testStartFiresOnWillStartSession( + startNotebook, + () => assertServiceState(), + ); + }); + + test(`${name} notebook fires onDidStartRuntime`, async () => { + await testStartFiresOnDidStartRuntime( + startNotebook, + session => assertSessionIsStarting(session), + ); + }); + + test(`${name} notebook fires events in order`, async () => { + await testStartFiresEventsInOrder(startNotebook); + }); + + test(`${name} notebook without notebook uri`, async () => { + await assert.rejects( + startSession(undefined, LanguageRuntimeSessionMode.Notebook, undefined), + new Error('A notebook URI must be provided when starting a notebook session.'), + ); + }); + + test(`${name} notebook encounters session.start() error`, async () => { await testEncountersSessionStartError( startNotebook, session => { @@ -451,7 +508,7 @@ suite('Positron - RuntimeSessionService', () => { ); }); - test('start console and notebook from the same runtime concurrently', async () => { + test(`${name} console and notebook from the same runtime concurrently`, async () => { // Consoles and notebooks shouldn't interfere with each other, even for the same runtime. const [consoleSession, notebookSession] = await Promise.all([ startConsole(), @@ -470,90 +527,38 @@ suite('Positron - RuntimeSessionService', () => { }); }); - test('start console while another runtime is starting for the language', async () => { - await testStartConsoleWhileAnotherIsStarting(startConsole, startReason); - }); - - test('start notebook while another runtime is starting for the notebook', async () => { + test(`${name} notebook while another runtime is starting for the notebook`, async () => { await testStartNotebookWhileAnotherIsStarting(startNotebook, startReason); }); - test('start console while another runtime is running for the language', async () => { - await testStartConsoleWhileAnotherIsRunning(startConsole, startReason); - }); - - test('start notebook while another runtime is running for the notebook', async () => { + test(`${name} notebook while another runtime is running for the notebook`, async () => { await testStartNotebookWhileAnotherIsRunning(startNotebook, startReason); }); - test('start console successively', async () => { - await testStartSuccessively(startConsole); - }); - - test('start notebook successively', async () => { + test(`${name} notebook successively`, async () => { await testStartSuccessively(startNotebook); }); - test('start console concurrently', async () => { - await testStartConcurrently(startConsole); - }); - - test('start notebook concurrently', async () => { + test(`${name} notebook concurrently`, async () => { await testStartConcurrently(startNotebook); }); - }); - - async function restoreSession( - sessionMetadata: IRuntimeSessionMetadata, runtimeMetadata = runtime, - ) { - await runtimeSessionService.restoreRuntimeSession(runtimeMetadata, sessionMetadata); - - // Ensure that the session gets disposed after the test. - const session = runtimeSessionService.getSession(sessionMetadata.sessionId); - assert.ok(session instanceof TestLanguageRuntimeSession); - disposables.add(session); - - return session; - } - - function restoreConsole(runtimeMetadata?: ILanguageRuntimeMetadata) { - return restoreSession(consoleSessionMetadata, runtimeMetadata); } - function restoreNotebook(runtimeMetadata?: ILanguageRuntimeMetadata) { - return restoreSession(notebookSessionMetadata, runtimeMetadata); - } - - const consoleSessionMetadata = { - sessionId: 'test-session-id', - sessionName: 'Test session', - sessionMode: LanguageRuntimeSessionMode.Console, - createdTimestamp: Date.now(), - startReason, - } as IRuntimeSessionMetadata; - - const notebookSessionMetadata = { - ...consoleSessionMetadata, - sessionMode: LanguageRuntimeSessionMode.Notebook, - notebookUri, - }; + suite('startNewRuntimeSession', () => { + createStartTests(startConsole, startNotebook, 'start', startReason); - suite('restoreRuntimeSession', () => { - test('restore console returns the expected session', async () => { - await testStartReturnsExpectedSession(restoreConsole); + test('start console for unknown runtime', async () => { + await testStartUnknownRuntime(startConsole); }); - test('restore notebook returns the expected session', async () => { - await testStartReturnsExpectedSession(restoreNotebook); + test('start notebook for unknown runtime', async () => { + await testStartUnknownRuntime(startNotebook); }); - test('restore console sets the expected service state', async () => { - await testStartConsoleSetsExpectedServiceState(restoreConsole); - }); + }); - test('restore notebook sets the expected service state', async () => { - await testStartNotebookSetsExpectedServiceState(restoreNotebook); - }); + suite('restoreRuntimeSession', () => { + createStartTests(restoreConsole, restoreNotebook, 'restore', undefined); test('restore console registers runtime if unregistered', async () => { // The runtime should not yet be registered. @@ -564,45 +569,6 @@ suite('Positron - RuntimeSessionService', () => { // The runtime should now be registered. assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); }); - - test('restore notebook without notebook URI', async () => { - await assert.rejects( - restoreSession({ ...consoleSessionMetadata, sessionMode: LanguageRuntimeSessionMode.Notebook }), - new Error('A notebook URI must be provided when starting a notebook session.'), - ); - }); - - test('restore console while another runtime is starting for the language', async () => { - await testStartConsoleWhileAnotherIsStarting(restoreConsole); - }); - - test('restore notebook while another runtime is starting for the notebook', async () => { - await testStartNotebookWhileAnotherIsStarting(restoreNotebook); - }); - - test('restore console while another runtime is running for the language', async () => { - await testStartConsoleWhileAnotherIsRunning(restoreConsole); - }); - - test('restore notebook while another runtime is running for the notebook', async () => { - await testStartNotebookWhileAnotherIsRunning(restoreNotebook); - }); - - test('restore console successively', async () => { - await testStartSuccessively(restoreConsole); - }); - - test('restore notebook successively', async () => { - await testStartSuccessively(restoreNotebook); - }); - - test('restore console concurrently', async () => { - await testStartConcurrently(restoreConsole); - }); - - test('restore notebook concurrently', async () => { - await testStartConcurrently(restoreNotebook); - }); }); suite('autoStartRuntime', () => { @@ -711,74 +677,24 @@ suite('Positron - RuntimeSessionService', () => { // TODO: We should probably check more things here? }); - - // TODO: Should we handle this? - test.skip('auto start console while another runtime is starting for the language', async () => { - await assert.rejects( - Promise.all([ - autoStartSession(), - autoStartSession(anotherRuntime), - ]), - new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} ` + - `cannot be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already starting for the language.`), - ); - }); - - // TODO: Should we handle this? - test.skip('auto start console while another runtime is running for the language', async () => { - await autoStartSession(); - await assert.rejects( - autoStartSession(anotherRuntime), - new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + - `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already running for the ${runtime.languageName} language.`), - ); - }); - - // TODO: Should we handle this? - test.skip('auto start console successively', async () => { - const session1 = await autoStartSession(); - const session2 = await autoStartSession(); - const session3 = await autoStartSession(); - - // Check that the same session was returned each time. - assert.equal(session1, session2); - assert.equal(session2, session3); - - // Check that only one session was started. - assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session1 }); - }); - - // TODO: Should we handle this? - test.skip('auto start console concurrently', async () => { - const [session1, session2, session3] = await Promise.all([ - autoStartSession(), - autoStartSession(), - autoStartSession(), - ]); - - // Check that the same session was returned. - assert.equal(session1, session2); - assert.equal(session2, session3); - - // Check that only one session was started. - assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session1 }); - }); }); suite('selectRuntime', () => { + const startReason = 'Test requested to select a runtime'; + async function selectRuntime(runtimeMetadata = runtime) { - await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, 'Test requested to select a runtime'); + await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); - assert.ok(session, 'Expected a session to be started'); + assert.ok(session instanceof TestLanguageRuntimeSession); disposables.add(session); return session; } - test('select runtime', async () => { - await testStartConsoleSetsExpectedServiceState(selectRuntime); - }); + createStartTests(selectRuntime, undefined, 'select runtime', startReason); + + // test('select runtime', async () => { + // await testStartConsoleSetsExpectedServiceState(selectRuntime); + // }); }); // TODO: Check sessionManager validation... @@ -909,7 +825,7 @@ suite('Positron - RuntimeSessionService', () => { // }); // suite('queuing', () => { - // test('start notebook while shutting down', async () => { + // test(`${name} notebook while shutting down`, async () => { // const session1 = await startNotebook(); // const [, session2,] = await Promise.all([ @@ -926,7 +842,7 @@ suite('Positron - RuntimeSessionService', () => { // }); // }); - // test('start notebook while restarting and in "exited" state', async () => { + // test(`${name} notebook while restarting and in "exited" state`, async () => { // const session = await startNotebook(); // await waitForRuntimeState(session, RuntimeState.Ready); From 7069706d68917751dea8305414b8797fa145af91 Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 16:44:58 +0200 Subject: [PATCH 10/45] refactor to generate tests --- .../test/common/runtimeSession.test.ts | 1093 ++++++++--------- 1 file changed, 483 insertions(+), 610 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index b5419da0693..6da44440a61 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -21,7 +21,6 @@ import { TestRuntimeSessionManager } from 'vs/workbench/test/common/positronWork import { TestWorkspaceTrustManagementService } from 'vs/workbench/test/common/workbenchTestServices'; type IStartSessionTask = (runtimeMetadata?: ILanguageRuntimeMetadata) => Promise; -type IValidator = (obj: T) => void; suite('Positron - RuntimeSessionService', () => { const disposables = ensureNoDisposablesAreLeakedInTestSuite(); @@ -116,226 +115,6 @@ suite('Positron - RuntimeSessionService', () => { ); } - async function testCallSuccessively(task: () => Promise, validate: IValidator) { - const result1 = await task(); - const result2 = await task(); - const result3 = await task(); - - assert.equal(result1, result2); - assert.equal(result2, result3); - - validate(result1); - } - - async function testCallConcurrently(task: () => Promise, validate: (obj: T) => void) { - const [result1, result2, result3] = await Promise.all([task(), task(), task()]); - - assert.equal(result1, result2); - assert.equal(result2, result3); - - validate(result1); - } - - async function testStartSuccessively(start: IStartSessionTask) { - await testCallSuccessively(start, (session) => assertSessionIsStarting(session)); - } - - async function testStartConcurrently(start: IStartSessionTask) { - await testCallConcurrently(start, (session) => assertSessionIsStarting(session)); - } - - async function testStartConsoleWhileAnotherIsStarting(start: IStartSessionTask, source?: string) { - await assert.rejects( - Promise.all([ - start(), - start(anotherRuntime), - ]), - new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} ` + - `cannot be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already starting for the language.` - + (source ? ` Request source: ${startReason}` : '')), - ); - } - - async function testStartConsoleWhileAnotherIsRunning(start: IStartSessionTask, source?: string) { - await start(); - await assert.rejects( - start(anotherRuntime), - new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + - `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already running for the ${runtime.languageName} language.` + - (source ? ` Request source: ${startReason}` : '')), - ); - } - - async function testStartNotebookWhileAnotherIsStarting(start: IStartSessionTask, source?: string) { - await assert.rejects( - Promise.all([ - start(), - start(anotherRuntime), - ]), - new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + - `be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already starting for the notebook ${notebookUri.toString()}.` - + (source ? ` Request source: ${startReason}` : '')) - ); - } - - async function testStartNotebookWhileAnotherIsRunning(start: IStartSessionTask, source?: string) { - await start(); - await assert.rejects( - start(anotherRuntime), - new Error(`A notebook for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + - `be started because a notebook for ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already running for the URI ${notebookUri.toString()}.` + - (source ? ` Request source: ${startReason}` : '')), - ); - } - - async function testStartReturnsExpectedSession(start: IStartSessionTask) { - const session = await start(); - - assert.equal(session.getRuntimeState(), RuntimeState.Starting); - assert.equal(session.metadata.sessionName, sessionName); - assert.equal(session.metadata.startReason, startReason); - assert.equal(session.runtimeMetadata, runtime); - - if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { - assert.equal(session.metadata.notebookUri, undefined); - } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { - assert.equal(session.metadata.notebookUri, notebookUri); - } else { - throw new Error(`Unexpected session mode: ${session.metadata.sessionMode}`); - } - } - - async function testStartSetsExpectedServiceState( - start: IStartSessionTask, - verifyWhileStarting: () => void, - verifyAfterStarted: (session: ILanguageRuntimeSession) => void, - ) { - // Check the initial state. - assertServiceState(); - - const promise = start(); - - // Check the state while starting. - verifyWhileStarting(); - - const session = await promise; - - // Check the state after starting. - verifyAfterStarted(session); - } - - function testStartConsoleSetsExpectedServiceState(start: IStartSessionTask) { - return testStartSetsExpectedServiceState( - start, - () => assertServiceState({ hasStartingOrRunningConsole: true }), - session => assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }), - ); - } - - function testStartNotebookSetsExpectedServiceState(start: IStartSessionTask) { - return testStartSetsExpectedServiceState( - start, - () => assertServiceState(), - session => assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }), - ); - } - - async function testStartFiresOnWillStartSession(start: IStartSessionTask, verify: () => void) { - let error: Error | undefined; - const target = sinon.spy(({ session }: IRuntimeSessionWillStartEvent) => { - try { - // TODO: Should onWillStartSession only fire once? - if (target.callCount > 1) { - return; - } - assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); - verify(); - } catch (e) { - error = e; - } - }); - disposables.add(runtimeSessionService.onWillStartSession(target)); - const session = await start(); - - // TODO: Should onWillStartSession only fire once? - sinon.assert.calledTwice(target); - // TODO: isNew is false for restored sessions... - sinon.assert.alwaysCalledWithExactly(target, { isNew: true, session }); - assert.ifError(error); - } - - async function testStartFiresOnDidStartRuntime( - start: IStartSessionTask, - verify: (session: ILanguageRuntimeSession) => void, - ) { - let error: Error | undefined; - const target = sinon.stub<[e: ILanguageRuntimeSession]>().callsFake(session => { - try { - assert.equal(session.getRuntimeState(), RuntimeState.Starting); - verify(session); - } catch (e) { - error = e; - } - }); - disposables.add(runtimeSessionService.onDidStartRuntime(target)); - - const session = await start(); - - sinon.assert.calledOnceWithExactly(target, session); - assert.ifError(error); - } - - async function testEncountersSessionStartError( - start: IStartSessionTask, - verify: (session: ILanguageRuntimeSession) => void, - ) { - // Listen to the onWillStartSession event and stub session.start() to throw an error. - const willStartSession = sinon.spy((e: IRuntimeSessionWillStartEvent) => { - sinon.stub(e.session, 'start').rejects(new Error('Session failed to start')); - }); - disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); - - const didFailStartRuntime = sinon.spy(); - disposables.add(runtimeSessionService.onDidFailStartRuntime(didFailStartRuntime)); - - const didStartRuntime = sinon.spy(); - disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); - - const session = await start(); - - assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); - - verify(session); - - sinon.assert.calledOnceWithExactly(didFailStartRuntime, session); - sinon.assert.callOrder(willStartSession, didFailStartRuntime); - sinon.assert.notCalled(didStartRuntime); - } - - async function testStartFiresEventsInOrder(start: IStartSessionTask) { - const willStartSession = sinon.spy(); - disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); - - const didStartRuntime = sinon.spy(); - disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); - - await start(); - - sinon.assert.callOrder(willStartSession, didStartRuntime); - } - - async function testStartUnknownRuntime(start: IStartSessionTask = startNotebook) { - const runtimeId = 'unknown-runtime-id'; - await assert.rejects( - start({ runtimeId } as ILanguageRuntimeMetadata,), - new Error(`No language runtime with id '${runtimeId}' was found.`), - ); - } - function assertSessionIsStarting(session: ILanguageRuntimeSession) { if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); @@ -367,7 +146,7 @@ suite('Positron - RuntimeSessionService', () => { const consoleSessionMetadata: IRuntimeSessionMetadata = { sessionId: 'test-console-session-id', - sessionName: 'Test console session', + sessionName, sessionMode: LanguageRuntimeSessionMode.Console, createdTimestamp: Date.now(), notebookUri: undefined, @@ -376,139 +155,259 @@ suite('Positron - RuntimeSessionService', () => { const notebookSessionMetadata: IRuntimeSessionMetadata = { sessionId: 'test-notebook-session-id', - sessionName: 'Test notebook session', + sessionName, sessionMode: LanguageRuntimeSessionMode.Notebook, createdTimestamp: Date.now(), notebookUri, startReason, }; - function createStartTests( - startConsole: IStartSessionTask, - startNotebook: IStartSessionTask | undefined, - name: string, - startReason?: string, - ) { - test(`${name} console returns the expected session`, async () => { - await testStartReturnsExpectedSession(startConsole); - }); + const data: { action: string; startConsole: IStartSessionTask; startNotebook: IStartSessionTask }[] = [ + { action: 'start', startConsole: startConsole, startNotebook: startNotebook }, + { action: 'restore', startConsole: restoreConsole, startNotebook: restoreNotebook }, + ]; + for (const { action, startConsole, startNotebook } of data) { - test(`${name} console sets the expected service state`, async () => { - await testStartConsoleSetsExpectedServiceState(startConsole); - }); + for (const mode of [LanguageRuntimeSessionMode.Console, LanguageRuntimeSessionMode.Notebook]) { + const start = mode === LanguageRuntimeSessionMode.Console ? startConsole : startNotebook; - test(`${name} console fires onWillStartSession`, async () => { - await testStartFiresOnWillStartSession( - startConsole, - () => assertServiceState({ hasStartingOrRunningConsole: true }), - ); - }); + test(`${action} ${mode} returns the expected session`, async () => { + const session = await start(); - test(`${name} console fires onDidStartRuntime`, async () => { - await testStartFiresOnDidStartRuntime( - startConsole, - session => assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }), - ); - }); + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assert.equal(session.metadata.sessionName, sessionName); + assert.equal(session.metadata.sessionMode, mode); + assert.equal(session.metadata.startReason, startReason); + assert.equal(session.runtimeMetadata, runtime); + + if (mode === LanguageRuntimeSessionMode.Console) { + assert.equal(session.metadata.notebookUri, undefined); + } else { + assert.equal(session.metadata.notebookUri, notebookUri); + } + }); - test(`${name} console fires events in order`, async () => { - await testStartFiresEventsInOrder(startConsole); - }); + test(`${action} ${mode} sets the expected service state`, async () => { + // Check the initial state. + assertServiceState(); + + const promise = start(); - test(`${name} console sets foregroundSession`, async () => { - const target = sinon.spy(); - disposables.add(runtimeSessionService.onDidChangeForegroundSession(target)); + // Check the state while starting. + if (mode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ hasStartingOrRunningConsole: true }); + } else { + assertServiceState(); + } - const session = await startConsole(); + const session = await promise; - assert.equal(runtimeSessionService.foregroundSession, session); + // Check the state after starting. + if (mode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + } else { + assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }); + } + }); - await waitForRuntimeState(session, RuntimeState.Ready); + test(`${action} ${mode} fires onWillStartSession`, async () => { + if (action === 'restore') { + // TODO: I'm not sure if we should be emitting a 'starting' runtime state event + // when reconnecting to a TestLanguageRuntimeSession. That's firing another + // event with isNew = true. + return; + } - // TODO: Feels a bit surprising that this isn't fired. It's because we set the private - // _foregroundSession property instead of the setter. When the 'ready' state is - // entered, we skip setting foregroundSession because it already matches the session. - sinon.assert.notCalled(target); - }); + let error: Error | undefined; + const target = sinon.spy(({ session }: IRuntimeSessionWillStartEvent) => { + try { + // TODO: Should onWillStartSession only fire once? + if (target.callCount > 1) { + return; + } + assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); + + if (mode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ hasStartingOrRunningConsole: true }); + } else { + assertServiceState(); + } + } catch (e) { + error = e; + } + }); + disposables.add(runtimeSessionService.onWillStartSession(target)); + const session = await start(); + + // TODO: Should onWillStartSession only fire once? + sinon.assert.calledTwice(target); + const isNew = action !== 'restore'; + sinon.assert.alwaysCalledWithExactly(target, { isNew, session }); + assert.ifError(error); + }); + + test(`${action} ${mode} fires onDidStartRuntime`, async () => { + let error: Error | undefined; + const target = sinon.stub<[e: ILanguageRuntimeSession]>().callsFake(session => { + try { + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + + if (mode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + } else { + assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }); + } + } catch (e) { + error = e; + } + }); + disposables.add(runtimeSessionService.onDidStartRuntime(target)); + + const session = await start(); + + sinon.assert.calledOnceWithExactly(target, session); + assert.ifError(error); + }); + + test(`${action} ${mode} fires events in order`, async () => { + const willStartSession = sinon.spy(); + disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); + + const didStartRuntime = sinon.spy(); + disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); + + await start(); + + sinon.assert.callOrder(willStartSession, didStartRuntime); + }); + + if (mode === LanguageRuntimeSessionMode.Console) { + test(`${action} ${mode} sets foregroundSession`, async () => { + const target = sinon.spy(); + disposables.add(runtimeSessionService.onDidChangeForegroundSession(target)); + + const session = await start(); + + assert.equal(runtimeSessionService.foregroundSession, session); + + await waitForRuntimeState(session, RuntimeState.Ready); + + // TODO: Feels a bit surprising that this isn't fired. It's because we set the private + // _foregroundSession property instead of the setter. When the 'ready' state is + // entered, we skip setting foregroundSession because it already matches the session. + sinon.assert.notCalled(target); + }); + } + + if (action === 'start') { + test(`${action} ${mode} for unknown runtime`, async () => { + const runtimeId = 'unknown-runtime-id'; + await assert.rejects( + start({ runtimeId } as ILanguageRuntimeMetadata,), + new Error(`No language runtime with id '${runtimeId}' was found.`), + ); + }); + } + + test(`${action} ${mode} encounters session.start() error`, async () => { + // Listen to the onWillStartSession event and stub session.start() to throw an error. + const willStartSession = sinon.spy((e: IRuntimeSessionWillStartEvent) => { + sinon.stub(e.session, 'start').rejects(new Error('Session failed to start')); + }); + disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); + + const didFailStartRuntime = sinon.spy(); + disposables.add(runtimeSessionService.onDidFailStartRuntime(didFailStartRuntime)); + + const didStartRuntime = sinon.spy(); + disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); - test(`${name} console encounters session.start() error`, async () => { - await testEncountersSessionStartError( - startConsole, - session => { + const session = await start(); + + assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); + + if (mode === LanguageRuntimeSessionMode.Console) { // TODO: Seems unexpected that some of these are defined and others not. assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); assert.equal(runtimeSessionService.getConsoleSessionForLanguage(runtime.languageId), undefined); assert.equal(runtimeSessionService.getConsoleSessionForRuntime(runtime.runtimeId), session); assert.equal(runtimeSessionService.getSession(session.sessionId), session); assert.deepEqual(runtimeSessionService.activeSessions, [session]); + } else { + // TODO: Should failed notebook sessions be included in activeSessions? + assertServiceState({ activeSessions: [session] }); } - ); - }); - test(`${name} console while another runtime is starting for the language`, async () => { - await testStartConsoleWhileAnotherIsStarting(startConsole, startReason); - }); - - test(`${name} console while another runtime is running for the language`, async () => { - await testStartConsoleWhileAnotherIsRunning(startConsole, startReason); - }); + sinon.assert.calledOnceWithExactly(didFailStartRuntime, session); + sinon.assert.callOrder(willStartSession, didFailStartRuntime); + sinon.assert.notCalled(didStartRuntime); + }); - test(`${name} console successively`, async () => { - await testStartSuccessively(startConsole); - }); + test(`${action} ${mode} while another runtime is starting for the language`, async () => { + let error: Error; + if (mode === LanguageRuntimeSessionMode.Console) { + error = new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} ` + + `cannot be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already starting for the language.` + + (action !== 'restore' ? ` Request source: ${startReason}` : '')); + } else { + error = new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because language runtime ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already starting for the notebook ${notebookUri.toString()}.` + + (action !== 'restore' ? ` Request source: ${startReason}` : '')); + } - test(`${name} console concurrently`, async () => { - await testStartConcurrently(startConsole); - }); + await assert.rejects( + Promise.all([ + start(), + start(anotherRuntime), + ]), + error); + }); - if (!startNotebook) { - return; - } + test(`${action} ${mode} while another runtime is running for the language`, async () => { + let error: Error; + if (mode === LanguageRuntimeSessionMode.Console) { + error = new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the ${runtime.languageName} language.` + + (action !== 'restore' ? ` Request source: ${startReason}` : '')); + } else { + error = new Error(`A notebook for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a notebook for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the URI ${notebookUri.toString()}.` + + (action !== 'restore' ? ` Request source: ${startReason}` : '')); + } - test(`${name} notebook returns the expected session`, async () => { - await testStartReturnsExpectedSession(startNotebook); - }); + await start(); + await assert.rejects( + start(anotherRuntime), + error, + ); + }); - test(`${name} notebook sets the expected service state`, async () => { - await testStartNotebookSetsExpectedServiceState(startNotebook); - }); + test(`${action} ${mode} successively`, async () => { + const result1 = await start(); + const result2 = await start(); + const result3 = await start(); - test.skip(`${name} notebook fires onWillStartSession`, async () => { - await testStartFiresOnWillStartSession( - startNotebook, - () => assertServiceState(), - ); - }); + assert.equal(result1, result2); + assert.equal(result2, result3); - test(`${name} notebook fires onDidStartRuntime`, async () => { - await testStartFiresOnDidStartRuntime( - startNotebook, - session => assertSessionIsStarting(session), - ); - }); + assertSessionIsStarting(result1); + }); - test(`${name} notebook fires events in order`, async () => { - await testStartFiresEventsInOrder(startNotebook); - }); + test(`${action} ${mode} concurrently`, async () => { + const [result1, result2, result3] = await Promise.all([start(), start(), start()]); - test(`${name} notebook without notebook uri`, async () => { - await assert.rejects( - startSession(undefined, LanguageRuntimeSessionMode.Notebook, undefined), - new Error('A notebook URI must be provided when starting a notebook session.'), - ); - }); + assert.equal(result1, result2); + assert.equal(result2, result3); - test(`${name} notebook encounters session.start() error`, async () => { - await testEncountersSessionStartError( - startNotebook, - session => { - // TODO: Should failed notebook sessions be included in activeSessions? - assertServiceState({ activeSessions: [session] }); - }, - ); - }); + assertSessionIsStarting(result1); + }); + } - test(`${name} console and notebook from the same runtime concurrently`, async () => { + test(`${action} console and notebook from the same runtime concurrently`, async () => { // Consoles and notebooks shouldn't interfere with each other, even for the same runtime. const [consoleSession, notebookSession] = await Promise.all([ startConsole(), @@ -527,366 +426,340 @@ suite('Positron - RuntimeSessionService', () => { }); }); - test(`${name} notebook while another runtime is starting for the notebook`, async () => { - await testStartNotebookWhileAnotherIsStarting(startNotebook, startReason); - }); + // suite('autoStartRuntime', () => { + // async function autoStartSession(runtimeMetadata = runtime) { + // const sessionId = await runtimeSessionService.autoStartRuntime(runtimeMetadata, 'Test requested to auto-start a runtime'); + // if (!sessionId) { + // return undefined; + // } + // const session = runtimeSessionService.getSession(sessionId); + // assert.ok(session); + // disposables.add(session); + // return session; + // } - test(`${name} notebook while another runtime is running for the notebook`, async () => { - await testStartNotebookWhileAnotherIsRunning(startNotebook, startReason); - }); + // let configService: TestConfigurationService; + // let workspaceTrustManagementService: TestWorkspaceTrustManagementService; + // let manager: TestRuntimeSessionManager; - test(`${name} notebook successively`, async () => { - await testStartSuccessively(startNotebook); - }); + // setup(() => { + // configService = instantiationService.get(IConfigurationService) as TestConfigurationService; + // workspaceTrustManagementService = instantiationService.get(IWorkspaceTrustManagementService) as TestWorkspaceTrustManagementService; + // manager = TestRuntimeSessionManager.instance; - test(`${name} notebook concurrently`, async () => { - await testStartConcurrently(startNotebook); - }); - } + // // Enable automatic startup. + // configService.setUserConfiguration('positron.interpreters.automaticStartup', true); - suite('startNewRuntimeSession', () => { - createStartTests(startConsole, startNotebook, 'start', startReason); + // // Trust the workspace. + // workspaceTrustManagementService.setWorkspaceTrust(true); + // }); - test('start console for unknown runtime', async () => { - await testStartUnknownRuntime(startConsole); - }); + // test('auto start console in a trusted workspace', async () => { + // const promise = autoStartSession(); - test('start notebook for unknown runtime', async () => { - await testStartUnknownRuntime(startNotebook); - }); + // assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); - }); + // const session = await promise; - suite('restoreRuntimeSession', () => { - createStartTests(restoreConsole, restoreNotebook, 'restore', undefined); + // // TODO: Do we need this? It's the same as starting a session. + // assert.ok(session); + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assert.equal(session.metadata.sessionName, runtime.runtimeName); + // assert.equal(session.metadata.sessionMode, LanguageRuntimeSessionMode.Console); + // assert.equal(session.metadata.startReason, 'Test requested to auto-start a runtime'); + // assert.equal(session.runtimeMetadata, runtime); + // }); - test('restore console registers runtime if unregistered', async () => { - // The runtime should not yet be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + // // TODO: Should auto starting a notebook error? - await restoreConsole(unregisteredRuntime); + // test('auto start validates runtime if unregistered', async () => { + // // The runtime should not yet be registered. + // assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - // The runtime should now be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); - }); - }); + // manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + // return { ...metadata, extraRuntimeData: { someNewKey: 'someNewValue' } }; + // }); - suite('autoStartRuntime', () => { - async function autoStartSession(runtimeMetadata = runtime) { - const sessionId = await runtimeSessionService.autoStartRuntime(runtimeMetadata, 'Test requested to auto-start a runtime'); - if (!sessionId) { - return undefined; - } - const session = runtimeSessionService.getSession(sessionId); - assert.ok(session); - disposables.add(session); - return session; - } + // await autoStartSession(unregisteredRuntime); - let configService: TestConfigurationService; - let workspaceTrustManagementService: TestWorkspaceTrustManagementService; - let manager: TestRuntimeSessionManager; + // // The *validated* runtime should now be registered. + // assert.deepEqual( + // languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), + // { ...unregisteredRuntime, extraRuntimeData: { someNewKey: 'someNewValue' } } + // ); + // }); - setup(() => { - configService = instantiationService.get(IConfigurationService) as TestConfigurationService; - workspaceTrustManagementService = instantiationService.get(IWorkspaceTrustManagementService) as TestWorkspaceTrustManagementService; - manager = TestRuntimeSessionManager.instance; + // test('auto start encounters runtime validation error', async () => { + // // The runtime should not yet be registered. + // assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - // Enable automatic startup. - configService.setUserConfiguration('positron.interpreters.automaticStartup', true); + // const error = new Error('Failed to validate runtime metadata'); + // manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + // throw error; + // }); - // Trust the workspace. - workspaceTrustManagementService.setWorkspaceTrust(true); - }); + // await assert.rejects(autoStartSession(unregisteredRuntime), error); - test('auto start console in a trusted workspace', async () => { - const promise = autoStartSession(); + // // The runtime should still not be registered. + // assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + // }); - assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); + // test('auto start console does nothing if automatic startup is disabled', async () => { + // configService.setUserConfiguration('positron.interpreters.automaticStartup', false); - const session = await promise; + // const session = await autoStartSession(); + // assert.equal(session, undefined); - // TODO: Do we need this? It's the same as starting a session. - assert.ok(session); - assert.equal(session.getRuntimeState(), RuntimeState.Starting); - assert.equal(session.metadata.sessionName, runtime.runtimeName); - assert.equal(session.metadata.sessionMode, LanguageRuntimeSessionMode.Console); - assert.equal(session.metadata.startReason, 'Test requested to auto-start a runtime'); - assert.equal(session.runtimeMetadata, runtime); - }); + // // TODO: Do we also need to check the session state? + // }); - // TODO: Should auto starting a notebook error? + // test('auto start console in an untrusted workspace starts when trust is granted', async () => { + // workspaceTrustManagementService.setWorkspaceTrust(false); - test('auto start validates runtime if unregistered', async () => { - // The runtime should not yet be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + // const sessionId = await runtimeSessionService.autoStartRuntime(runtime, 'Test requested to auto-start a runtime'); + // assert.equal(sessionId, ''); - manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { - return { ...metadata, extraRuntimeData: { someNewKey: 'someNewValue' } }; - }); + // workspaceTrustManagementService.setWorkspaceTrust(true); - await autoStartSession(unregisteredRuntime); + // await new Promise(resolve => { + // disposables.add(runtimeSessionService.onDidStartRuntime(session => { + // if (session.runtimeMetadata === runtime) { + // disposables.add(session); + // resolve(); + // } + // })); + // }); - // The *validated* runtime should now be registered. - assert.deepEqual( - languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), - { ...unregisteredRuntime, extraRuntimeData: { someNewKey: 'someNewValue' } } - ); - }); + // // TODO: We should probably check more things here? + // }); + // }); - test('auto start encounters runtime validation error', async () => { - // The runtime should not yet be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + // suite('selectRuntime', () => { + // const startReason = 'Test requested to select a runtime'; - const error = new Error('Failed to validate runtime metadata'); - manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { - throw error; - }); + // async function selectRuntime(runtimeMetadata = runtime) { + // await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); + // const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); + // assert.ok(session instanceof TestLanguageRuntimeSession); + // disposables.add(session); + // return session; + // } - await assert.rejects(autoStartSession(unregisteredRuntime), error); + // createStartTests(selectRuntime, undefined, 'select runtime', startReason); - // The runtime should still not be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - }); + // // test('select runtime', async () => { + // // await testStartConsoleSetsExpectedServiceState(selectRuntime); + // // }); + // }); - test('auto start console does nothing if automatic startup is disabled', async () => { - configService.setUserConfiguration('positron.interpreters.automaticStartup', false); + // TODO: Check sessionManager validation... - const session = await autoStartSession(); - assert.equal(session, undefined); + // TODO: If workspace is trusted, - // TODO: Do we also need to check the session state? - }); + // suite('shutdownNotebookSession', () => { + // test('shutdown notebook', async () => { + // const session = await startNotebook(); - test('auto start console in an untrusted workspace starts when trust is granted', async () => { - workspaceTrustManagementService.setWorkspaceTrust(false); + // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); - const sessionId = await runtimeSessionService.autoStartRuntime(runtime, 'Test requested to auto-start a runtime'); - assert.equal(sessionId, ''); + // assert.equal(session.getRuntimeState(), RuntimeState.Exited); + // // TODO: The session is in activeSessions and returned by getSession but not by + // // getNotebookSessionForNotebookUri. Is that correct? This is also the only reason + // // we need a notebookForNotebookUri parameter in assertServiceState. + // assertServiceState({ notebookSession: session }); + // }); - workspaceTrustManagementService.setWorkspaceTrust(true); + // test('shutdown notebook without running runtime', async () => { + // // It should not error, since it's already in the desired state. + // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); + // assertServiceState(); + // }); - await new Promise(resolve => { - disposables.add(runtimeSessionService.onDidStartRuntime(session => { - if (session.runtimeMetadata === runtime) { - disposables.add(session); - resolve(); - } - })); - }); - - // TODO: We should probably check more things here? - }); - }); + // test('shutdown notebook concurrently', async () => { + // const session = await startNotebook(); - suite('selectRuntime', () => { - const startReason = 'Test requested to select a runtime'; + // await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // ]); - async function selectRuntime(runtimeMetadata = runtime) { - await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); - const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); - assert.ok(session instanceof TestLanguageRuntimeSession); - disposables.add(session); - return session; - } + // assert.equal(session.getRuntimeState(), RuntimeState.Exited); + // }); - createStartTests(selectRuntime, undefined, 'select runtime', startReason); + // test('shutdown notebook while starting', async () => { + // const [session,] = await Promise.all([ + // startNotebook(), + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // ]); - // test('select runtime', async () => { - // await testStartConsoleSetsExpectedServiceState(selectRuntime); + // assert.equal(session.getRuntimeState(), RuntimeState.Exited); + // assertServiceState({ notebookSession: session }); + // }); // }); - }); - - // TODO: Check sessionManager validation... - - // TODO: If workspace is trusted, - - // suite('shutdownNotebookSession', () => { - // test('shutdown notebook', async () => { - // const session = await startNotebook(); - - // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); - - // assert.equal(session.getRuntimeState(), RuntimeState.Exited); - // // TODO: The session is in activeSessions and returned by getSession but not by - // // getNotebookSessionForNotebookUri. Is that correct? This is also the only reason - // // we need a notebookForNotebookUri parameter in assertServiceState. - // assertServiceState({ notebookSession: session }); - // }); - // test('shutdown notebook without running runtime', async () => { - // // It should not error, since it's already in the desired state. - // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); - // assertServiceState(); - // }); - // test('shutdown notebook concurrently', async () => { - // const session = await startNotebook(); + // function restartSession(sessionId: string) { + // return runtimeSessionService.restartSession( + // sessionId, 'Test requested to restart a runtime session' + // ); + // } - // await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // ]); + // suite('restartSession', () => { + // test('restart console in "ready" state', async () => { + // const session = await startConsole(); + // await waitForRuntimeState(session, RuntimeState.Ready); - // assert.equal(session.getRuntimeState(), RuntimeState.Exited); - // }); + // await restartSession(session.sessionId); - // test('shutdown notebook while starting', async () => { - // const [session,] = await Promise.all([ - // startNotebook(), - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // ]); + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // }); - // assert.equal(session.getRuntimeState(), RuntimeState.Exited); - // assertServiceState({ notebookSession: session }); - // }); - // }); + // test('restart console in "starting" state', async () => { + // const session = await startConsole(); + // await restartSession(session.sessionId); - // function restartSession(sessionId: string) { - // return runtimeSessionService.restartSession( - // sessionId, 'Test requested to restart a runtime session' - // ); - // } + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // }); - // suite('restartSession', () => { - // test('restart console in "ready" state', async () => { - // const session = await startConsole(); - // await waitForRuntimeState(session, RuntimeState.Ready); + // test('restart console in "exited" state', async () => { + // const session = await startConsole(); + // await session.shutdown(RuntimeExitReason.Shutdown); + // await waitForRuntimeState(session, RuntimeState.Exited); - // await restartSession(session.sessionId); + // await restartSession(session.sessionId); - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // }); + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // }); - // test('restart console in "starting" state', async () => { - // const session = await startConsole(); + // test('restart session with unknown session id', async () => { + // const sessionId = 'unknown-session-id'; + // assert.rejects( + // restartSession(sessionId), + // new Error(`No session with ID '${sessionId}' was found.`), + // ); + // }); - // await restartSession(session.sessionId); + // test('restart console concurrently', async () => { + // const session = await startConsole(); + // await waitForRuntimeState(session, RuntimeState.Ready); - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // }); + // const target = sinon.spy(session, 'restart'); - // test('restart console in "exited" state', async () => { - // const session = await startConsole(); - // await session.shutdown(RuntimeExitReason.Shutdown); - // await waitForRuntimeState(session, RuntimeState.Exited); + // await Promise.all([ + // restartSession(session.sessionId), + // restartSession(session.sessionId), + // restartSession(session.sessionId), + // ]); - // await restartSession(session.sessionId); + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // }); + // sinon.assert.calledOnce(target); + // }); - // test('restart session with unknown session id', async () => { - // const sessionId = 'unknown-session-id'; - // assert.rejects( - // restartSession(sessionId), - // new Error(`No session with ID '${sessionId}' was found.`), - // ); - // }); + // test('restart console successively', async () => { + // const session = await startConsole(); - // test('restart console concurrently', async () => { - // const session = await startConsole(); - // await waitForRuntimeState(session, RuntimeState.Ready); + // const target = sinon.spy(session, 'restart'); - // const target = sinon.spy(session, 'restart'); + // await waitForRuntimeState(session, RuntimeState.Ready); + // await restartSession(session.sessionId); + // await waitForRuntimeState(session, RuntimeState.Ready); + // await restartSession(session.sessionId); + // await waitForRuntimeState(session, RuntimeState.Ready); + // await restartSession(session.sessionId); - // await Promise.all([ - // restartSession(session.sessionId), - // restartSession(session.sessionId), - // restartSession(session.sessionId), - // ]); + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + // sinon.assert.calledThrice(target); + // }); - // sinon.assert.calledOnce(target); - // }); - - // test('restart console successively', async () => { - // const session = await startConsole(); - - // const target = sinon.spy(session, 'restart'); - - // await waitForRuntimeState(session, RuntimeState.Ready); - // await restartSession(session.sessionId); - // await waitForRuntimeState(session, RuntimeState.Ready); - // await restartSession(session.sessionId); - // await waitForRuntimeState(session, RuntimeState.Ready); - // await restartSession(session.sessionId); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - - // sinon.assert.calledThrice(target); - // }); - - // }); - - // suite('queuing', () => { - // test(`${name} notebook while shutting down`, async () => { - // const session1 = await startNotebook(); - - // const [, session2,] = await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // startNotebook(), - // ]); - - // assert.equal(session1.getRuntimeState(), RuntimeState.Exited); - // assert.equal(session2.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ - // notebookSession: session2, - // notebookSessionForNotebookUri: session2, - // activeSessions: [session1, session2], - // }); - // }); - - // test(`${name} notebook while restarting and in "exited" state`, async () => { - // const session = await startNotebook(); - // await waitForRuntimeState(session, RuntimeState.Ready); + // }); - // const target = sinon.spy(session, 'restart'); + // suite('queuing', () => { + // test(`${name} notebook while shutting down`, async () => { + // const session1 = await startNotebook(); + + // const [, session2,] = await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // startNotebook(), + // ]); + + // assert.equal(session1.getRuntimeState(), RuntimeState.Exited); + // assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ + // notebookSession: session2, + // notebookSessionForNotebookUri: session2, + // activeSessions: [session1, session2], + // }); + // }); + + // test(`${name} notebook while restarting and in "exited" state`, async () => { + // const session = await startNotebook(); + // await waitForRuntimeState(session, RuntimeState.Ready); + + // const target = sinon.spy(session, 'restart'); + + // const startPromise = new Promise(resolve => { + // const disposable = session.onDidChangeRuntimeState(state => { + // if (state === RuntimeState.Exited) { + // disposable.dispose(); + // resolve(startNotebook()); + // } + // }); + // }); + + // const [, session2,] = await Promise.all([ + // restartSession(session.sessionId), + // startPromise, + // // startNotebook(), + // ]); + + // assert.equal(session, session2); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ + // notebookSession: session, + // notebookSessionForNotebookUri: session, + // }); + + // sinon.assert.calledOnce(target); + // }); + + // test('restart notebook while shutting down', async () => { + // const session = await startNotebook(); + + // await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // restartSession(session.sessionId), + // ]); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertNotebookSessionIsStarted(session); + // }); - // const startPromise = new Promise(resolve => { - // const disposable = session.onDidChangeRuntimeState(state => { - // if (state === RuntimeState.Exited) { - // disposable.dispose(); - // resolve(startNotebook()); - // } - // }); - // }); + // }); + } - // const [, session2,] = await Promise.all([ - // restartSession(session.sessionId), - // startPromise, - // // startNotebook(), - // ]); + test(`start notebook without notebook uri`, async () => { + await assert.rejects( + startSession(undefined, LanguageRuntimeSessionMode.Notebook, undefined), + new Error('A notebook URI must be provided when starting a notebook session.'), + ); + }); - // assert.equal(session, session2); + test('restore console registers runtime if unregistered', async () => { + // The runtime should not yet be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ - // notebookSession: session, - // notebookSessionForNotebookUri: session, - // }); - - // sinon.assert.calledOnce(target); - // }); - - // test('restart notebook while shutting down', async () => { - // const session = await startNotebook(); - - // await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // restartSession(session.sessionId), - // ]); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertNotebookSessionIsStarted(session); - // }); + await restoreConsole(unregisteredRuntime); - // }); + // The runtime should now be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); + }); }); async function waitForRuntimeState( From ad4a9623afaf64f775063e1d3c75d9e72b5c2074 Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 16:49:22 +0200 Subject: [PATCH 11/45] add selectRuntime to start tests --- .../test/common/runtimeSession.test.ts | 115 +++++------------- 1 file changed, 32 insertions(+), 83 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 6da44440a61..67c8cb2de1a 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -162,14 +162,26 @@ suite('Positron - RuntimeSessionService', () => { startReason, }; - const data: { action: string; startConsole: IStartSessionTask; startNotebook: IStartSessionTask }[] = [ + async function selectRuntime(runtimeMetadata = runtime) { + await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); + const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); + assert.ok(session instanceof TestLanguageRuntimeSession); + disposables.add(session); + return session; + } + + const data: { action: string; startConsole: IStartSessionTask; startNotebook?: IStartSessionTask }[] = [ { action: 'start', startConsole: startConsole, startNotebook: startNotebook }, { action: 'restore', startConsole: restoreConsole, startNotebook: restoreNotebook }, + { action: 'select', startConsole: selectRuntime, startNotebook: undefined }, ]; for (const { action, startConsole, startNotebook } of data) { for (const mode of [LanguageRuntimeSessionMode.Console, LanguageRuntimeSessionMode.Notebook]) { const start = mode === LanguageRuntimeSessionMode.Console ? startConsole : startNotebook; + if (!start) { + return; + } test(`${action} ${mode} returns the expected session`, async () => { const session = await start(); @@ -407,24 +419,26 @@ suite('Positron - RuntimeSessionService', () => { }); } - test(`${action} console and notebook from the same runtime concurrently`, async () => { - // Consoles and notebooks shouldn't interfere with each other, even for the same runtime. - const [consoleSession, notebookSession] = await Promise.all([ - startConsole(), - startNotebook(), - ]); - - assert.equal(consoleSession.getRuntimeState(), RuntimeState.Starting); - assert.equal(notebookSession.getRuntimeState(), RuntimeState.Starting); - - assertServiceState({ - hasStartingOrRunningConsole: true, - consoleSession, - notebookSession, - notebookSessionForNotebookUri: notebookSession, - activeSessions: [consoleSession, notebookSession], + if (startNotebook) { + test(`${action} console and notebook from the same runtime concurrently`, async () => { + // Consoles and notebooks shouldn't interfere with each other, even for the same runtime. + const [consoleSession, notebookSession] = await Promise.all([ + startConsole(), + startNotebook(), + ]); + + assert.equal(consoleSession.getRuntimeState(), RuntimeState.Starting); + assert.equal(notebookSession.getRuntimeState(), RuntimeState.Starting); + + assertServiceState({ + hasStartingOrRunningConsole: true, + consoleSession, + notebookSession, + notebookSessionForNotebookUri: notebookSession, + activeSessions: [consoleSession, notebookSession], + }); }); - }); + } // suite('autoStartRuntime', () => { // async function autoStartSession(runtimeMetadata = runtime) { @@ -534,71 +548,6 @@ suite('Positron - RuntimeSessionService', () => { // }); // }); - // suite('selectRuntime', () => { - // const startReason = 'Test requested to select a runtime'; - - // async function selectRuntime(runtimeMetadata = runtime) { - // await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); - // const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); - // assert.ok(session instanceof TestLanguageRuntimeSession); - // disposables.add(session); - // return session; - // } - - // createStartTests(selectRuntime, undefined, 'select runtime', startReason); - - // // test('select runtime', async () => { - // // await testStartConsoleSetsExpectedServiceState(selectRuntime); - // // }); - // }); - - // TODO: Check sessionManager validation... - - // TODO: If workspace is trusted, - - // suite('shutdownNotebookSession', () => { - // test('shutdown notebook', async () => { - // const session = await startNotebook(); - - // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); - - // assert.equal(session.getRuntimeState(), RuntimeState.Exited); - // // TODO: The session is in activeSessions and returned by getSession but not by - // // getNotebookSessionForNotebookUri. Is that correct? This is also the only reason - // // we need a notebookForNotebookUri parameter in assertServiceState. - // assertServiceState({ notebookSession: session }); - // }); - - // test('shutdown notebook without running runtime', async () => { - // // It should not error, since it's already in the desired state. - // await runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown); - // assertServiceState(); - // }); - - // test('shutdown notebook concurrently', async () => { - // const session = await startNotebook(); - - // await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // ]); - - // assert.equal(session.getRuntimeState(), RuntimeState.Exited); - // }); - - // test('shutdown notebook while starting', async () => { - // const [session,] = await Promise.all([ - // startNotebook(), - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // ]); - - // assert.equal(session.getRuntimeState(), RuntimeState.Exited); - // assertServiceState({ notebookSession: session }); - // }); - // }); - - // function restartSession(sessionId: string) { // return runtimeSessionService.restartSession( // sessionId, 'Test requested to restart a runtime session' From 4187cf5c07b89eeba41cd941024254ce1370c739 Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 14 Nov 2024 16:51:31 +0200 Subject: [PATCH 12/45] log uncaught promise rejection --- .../workbench/services/runtimeSession/common/runtimeSession.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 3a852db9167..394ae66b1e0 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -517,6 +517,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession } catch (err) { startPromise.error(err); } + + startPromise.p.catch((err) => this._logService.debug(`Error starting session: ${err}`)); } /** From fe6a5f94fb8fc6b883c581d4545f88fbfb0d30eb Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 10:12:57 +0200 Subject: [PATCH 13/45] all current tests passing --- .../test/common/runtimeSession.test.ts | 98 +++++++++---------- 1 file changed, 47 insertions(+), 51 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 67c8cb2de1a..cb6dde471af 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -24,15 +24,17 @@ type IStartSessionTask = (runtimeMetadata?: ILanguageRuntimeMetadata) => Promise suite('Positron - RuntimeSessionService', () => { const disposables = ensureNoDisposablesAreLeakedInTestSuite(); - const sessionName = 'Test session'; const startReason = 'Test requested to start a runtime session'; const notebookUri = URI.file('/path/to/notebook'); let instantiationService: TestInstantiationService; let languageRuntimeService: ILanguageRuntimeService; let runtimeSessionService: IRuntimeSessionService; let runtime: ILanguageRuntimeMetadata; + let sessionName: string; let anotherRuntime: ILanguageRuntimeMetadata; let unregisteredRuntime: ILanguageRuntimeMetadata; + let consoleSessionMetadata: IRuntimeSessionMetadata; + let notebookSessionMetadata: IRuntimeSessionMetadata; setup(() => { instantiationService = disposables.add(new TestInstantiationService()); @@ -40,8 +42,25 @@ suite('Positron - RuntimeSessionService', () => { languageRuntimeService = instantiationService.get(ILanguageRuntimeService); runtimeSessionService = instantiationService.get(IRuntimeSessionService); runtime = createTestLanguageRuntimeMetadata(instantiationService, disposables); + sessionName = runtime.runtimeName; anotherRuntime = createTestLanguageRuntimeMetadata(instantiationService, disposables); unregisteredRuntime = { runtimeId: 'unregistered-runtime-id' } as ILanguageRuntimeMetadata; + consoleSessionMetadata = { + sessionId: 'test-console-session-id', + sessionName, + sessionMode: LanguageRuntimeSessionMode.Console, + createdTimestamp: Date.now(), + notebookUri: undefined, + startReason, + }; + notebookSessionMetadata = { + sessionId: 'test-notebook-session-id', + sessionName, + sessionMode: LanguageRuntimeSessionMode.Notebook, + createdTimestamp: Date.now(), + notebookUri, + startReason, + }; }); function startSession( @@ -144,24 +163,6 @@ suite('Positron - RuntimeSessionService', () => { return restoreSession(notebookSessionMetadata, runtimeMetadata); } - const consoleSessionMetadata: IRuntimeSessionMetadata = { - sessionId: 'test-console-session-id', - sessionName, - sessionMode: LanguageRuntimeSessionMode.Console, - createdTimestamp: Date.now(), - notebookUri: undefined, - startReason, - }; - - const notebookSessionMetadata: IRuntimeSessionMetadata = { - sessionId: 'test-notebook-session-id', - sessionName, - sessionMode: LanguageRuntimeSessionMode.Notebook, - createdTimestamp: Date.now(), - notebookUri, - startReason, - }; - async function selectRuntime(runtimeMetadata = runtime) { await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); @@ -180,7 +181,7 @@ suite('Positron - RuntimeSessionService', () => { for (const mode of [LanguageRuntimeSessionMode.Console, LanguageRuntimeSessionMode.Notebook]) { const start = mode === LanguageRuntimeSessionMode.Console ? startConsole : startNotebook; if (!start) { - return; + continue; } test(`${action} ${mode} returns the expected session`, async () => { @@ -215,11 +216,7 @@ suite('Positron - RuntimeSessionService', () => { const session = await promise; // Check the state after starting. - if (mode === LanguageRuntimeSessionMode.Console) { - assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - } else { - assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }); - } + assertSessionIsStarting(session); }); test(`${action} ${mode} fires onWillStartSession`, async () => { @@ -264,11 +261,7 @@ suite('Positron - RuntimeSessionService', () => { try { assert.equal(session.getRuntimeState(), RuntimeState.Starting); - if (mode === LanguageRuntimeSessionMode.Console) { - assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - } else { - assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }); - } + assertSessionIsStarting(session); } catch (e) { error = e; } @@ -312,7 +305,7 @@ suite('Positron - RuntimeSessionService', () => { } if (action === 'start') { - test(`${action} ${mode} for unknown runtime`, async () => { + test(`${action} ${mode} throws for unknown runtime`, async () => { const runtimeId = 'unknown-runtime-id'; await assert.rejects( start({ runtimeId } as ILanguageRuntimeMetadata,), @@ -355,7 +348,7 @@ suite('Positron - RuntimeSessionService', () => { sinon.assert.notCalled(didStartRuntime); }); - test(`${action} ${mode} while another runtime is starting for the language`, async () => { + test(`${action} ${mode} throws if another runtime is starting for the language`, async () => { let error: Error; if (mode === LanguageRuntimeSessionMode.Console) { error = new Error(`Session for language runtime ${formatLanguageRuntimeMetadata(anotherRuntime)} ` + @@ -377,26 +370,29 @@ suite('Positron - RuntimeSessionService', () => { error); }); - test(`${action} ${mode} while another runtime is running for the language`, async () => { - let error: Error; - if (mode === LanguageRuntimeSessionMode.Console) { - error = new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + - `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already running for the ${runtime.languageName} language.` + - (action !== 'restore' ? ` Request source: ${startReason}` : '')); - } else { - error = new Error(`A notebook for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + - `be started because a notebook for ${formatLanguageRuntimeMetadata(runtime)} ` + - `is already running for the URI ${notebookUri.toString()}.` + - (action !== 'restore' ? ` Request source: ${startReason}` : '')); - } + // Skip for 'select' since selecting another runtime is expected. + if (action !== 'select') { + test(`${action} ${mode} throws if another runtime is running for the language`, async () => { + let error: Error; + if (mode === LanguageRuntimeSessionMode.Console) { + error = new Error(`A console for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a console for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the ${runtime.languageName} language.` + + (action !== 'restore' ? ` Request source: ${startReason}` : '')); + } else { + error = new Error(`A notebook for ${formatLanguageRuntimeMetadata(anotherRuntime)} cannot ` + + `be started because a notebook for ${formatLanguageRuntimeMetadata(runtime)} ` + + `is already running for the URI ${notebookUri.toString()}.` + + (action !== 'restore' ? ` Request source: ${startReason}` : '')); + } - await start(); - await assert.rejects( - start(anotherRuntime), - error, - ); - }); + await start(); + await assert.rejects( + start(anotherRuntime), + error, + ); + }); + } test(`${action} ${mode} successively`, async () => { const result1 = await start(); From 34a37c30a28234989cd56a36767d67e2af9d4249 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 15:20:27 +0200 Subject: [PATCH 14/45] add auto start and more select tests; --- .../test/common/runtimeSession.test.ts | 273 ++++++++++-------- 1 file changed, 154 insertions(+), 119 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index cb6dde471af..2c2e67ce6cc 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -29,9 +29,12 @@ suite('Positron - RuntimeSessionService', () => { let instantiationService: TestInstantiationService; let languageRuntimeService: ILanguageRuntimeService; let runtimeSessionService: IRuntimeSessionService; + let configService: TestConfigurationService; + let workspaceTrustManagementService: TestWorkspaceTrustManagementService; + let manager: TestRuntimeSessionManager; let runtime: ILanguageRuntimeMetadata; - let sessionName: string; let anotherRuntime: ILanguageRuntimeMetadata; + let sessionName: string; let unregisteredRuntime: ILanguageRuntimeMetadata; let consoleSessionMetadata: IRuntimeSessionMetadata; let notebookSessionMetadata: IRuntimeSessionMetadata; @@ -41,10 +44,15 @@ suite('Positron - RuntimeSessionService', () => { createRuntimeServices(instantiationService, disposables); languageRuntimeService = instantiationService.get(ILanguageRuntimeService); runtimeSessionService = instantiationService.get(IRuntimeSessionService); + configService = instantiationService.get(IConfigurationService) as TestConfigurationService; + workspaceTrustManagementService = instantiationService.get(IWorkspaceTrustManagementService) as TestWorkspaceTrustManagementService; + manager = TestRuntimeSessionManager.instance; + runtime = createTestLanguageRuntimeMetadata(instantiationService, disposables); - sessionName = runtime.runtimeName; anotherRuntime = createTestLanguageRuntimeMetadata(instantiationService, disposables); unregisteredRuntime = { runtimeId: 'unregistered-runtime-id' } as ILanguageRuntimeMetadata; + + sessionName = runtime.runtimeName; consoleSessionMetadata = { sessionId: 'test-console-session-id', sessionName, @@ -61,6 +69,12 @@ suite('Positron - RuntimeSessionService', () => { notebookUri, startReason, }; + + // Enable automatic startup. + configService.setUserConfiguration('positron.interpreters.automaticStartup', true); + + // Trust the workspace. + workspaceTrustManagementService.setWorkspaceTrust(true); }); function startSession( @@ -97,18 +111,18 @@ suite('Positron - RuntimeSessionService', () => { activeSessions?: ILanguageRuntimeSession[]; } - function assertServiceState(expectedState?: IServiceState): void { + function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { // Check the console session state. assert.equal( - runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), + runtimeSessionService.hasStartingOrRunningConsole(runtimeMetadata.languageId), expectedState?.hasStartingOrRunningConsole ?? false, ); assert.equal( - runtimeSessionService.getConsoleSessionForLanguage(runtime.languageId)?.sessionId, + runtimeSessionService.getConsoleSessionForLanguage(runtimeMetadata.languageId)?.sessionId, expectedState?.consoleSession?.sessionId, ); assert.equal( - runtimeSessionService.getConsoleSessionForRuntime(runtime.runtimeId)?.sessionId, + runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId)?.sessionId, expectedState?.consoleSession?.sessionId, ); assert.equal( @@ -135,10 +149,12 @@ suite('Positron - RuntimeSessionService', () => { } function assertSessionIsStarting(session: ILanguageRuntimeSession) { + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { - assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); + assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }, session.runtimeMetadata); } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { - assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }); + assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }, session.runtimeMetadata); } } @@ -163,6 +179,16 @@ suite('Positron - RuntimeSessionService', () => { return restoreSession(notebookSessionMetadata, runtimeMetadata); } + async function autoStartSession(runtimeMetadata = runtime) { + // TODO: Maybe all of these functions can return a sessionId? + const sessionId = await runtimeSessionService.autoStartRuntime(runtimeMetadata, startReason); + assert.ok(sessionId); + const session = runtimeSessionService.getSession(sessionId); + assert.ok(session instanceof TestLanguageRuntimeSession); + disposables.add(session); + return session; + } + async function selectRuntime(runtimeMetadata = runtime) { await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); @@ -174,7 +200,8 @@ suite('Positron - RuntimeSessionService', () => { const data: { action: string; startConsole: IStartSessionTask; startNotebook?: IStartSessionTask }[] = [ { action: 'start', startConsole: startConsole, startNotebook: startNotebook }, { action: 'restore', startConsole: restoreConsole, startNotebook: restoreNotebook }, - { action: 'select', startConsole: selectRuntime, startNotebook: undefined }, + { action: 'auto start', startConsole: autoStartSession }, + { action: 'select', startConsole: selectRuntime }, ]; for (const { action, startConsole, startNotebook } of data) { @@ -304,7 +331,7 @@ suite('Positron - RuntimeSessionService', () => { }); } - if (action === 'start') { + if (action === 'start' || action === 'select') { test(`${action} ${mode} throws for unknown runtime`, async () => { const runtimeId = 'unknown-runtime-id'; await assert.rejects( @@ -370,7 +397,7 @@ suite('Positron - RuntimeSessionService', () => { error); }); - // Skip for 'select' since selecting another runtime is expected. + // Skip for 'select' since selecting another runtime is expected in that case. if (action !== 'select') { test(`${action} ${mode} throws if another runtime is running for the language`, async () => { let error: Error; @@ -436,114 +463,6 @@ suite('Positron - RuntimeSessionService', () => { }); } - // suite('autoStartRuntime', () => { - // async function autoStartSession(runtimeMetadata = runtime) { - // const sessionId = await runtimeSessionService.autoStartRuntime(runtimeMetadata, 'Test requested to auto-start a runtime'); - // if (!sessionId) { - // return undefined; - // } - // const session = runtimeSessionService.getSession(sessionId); - // assert.ok(session); - // disposables.add(session); - // return session; - // } - - // let configService: TestConfigurationService; - // let workspaceTrustManagementService: TestWorkspaceTrustManagementService; - // let manager: TestRuntimeSessionManager; - - // setup(() => { - // configService = instantiationService.get(IConfigurationService) as TestConfigurationService; - // workspaceTrustManagementService = instantiationService.get(IWorkspaceTrustManagementService) as TestWorkspaceTrustManagementService; - // manager = TestRuntimeSessionManager.instance; - - // // Enable automatic startup. - // configService.setUserConfiguration('positron.interpreters.automaticStartup', true); - - // // Trust the workspace. - // workspaceTrustManagementService.setWorkspaceTrust(true); - // }); - - // test('auto start console in a trusted workspace', async () => { - // const promise = autoStartSession(); - - // assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); - - // const session = await promise; - - // // TODO: Do we need this? It's the same as starting a session. - // assert.ok(session); - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assert.equal(session.metadata.sessionName, runtime.runtimeName); - // assert.equal(session.metadata.sessionMode, LanguageRuntimeSessionMode.Console); - // assert.equal(session.metadata.startReason, 'Test requested to auto-start a runtime'); - // assert.equal(session.runtimeMetadata, runtime); - // }); - - // // TODO: Should auto starting a notebook error? - - // test('auto start validates runtime if unregistered', async () => { - // // The runtime should not yet be registered. - // assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - - // manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { - // return { ...metadata, extraRuntimeData: { someNewKey: 'someNewValue' } }; - // }); - - // await autoStartSession(unregisteredRuntime); - - // // The *validated* runtime should now be registered. - // assert.deepEqual( - // languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), - // { ...unregisteredRuntime, extraRuntimeData: { someNewKey: 'someNewValue' } } - // ); - // }); - - // test('auto start encounters runtime validation error', async () => { - // // The runtime should not yet be registered. - // assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - - // const error = new Error('Failed to validate runtime metadata'); - // manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { - // throw error; - // }); - - // await assert.rejects(autoStartSession(unregisteredRuntime), error); - - // // The runtime should still not be registered. - // assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); - // }); - - // test('auto start console does nothing if automatic startup is disabled', async () => { - // configService.setUserConfiguration('positron.interpreters.automaticStartup', false); - - // const session = await autoStartSession(); - // assert.equal(session, undefined); - - // // TODO: Do we also need to check the session state? - // }); - - // test('auto start console in an untrusted workspace starts when trust is granted', async () => { - // workspaceTrustManagementService.setWorkspaceTrust(false); - - // const sessionId = await runtimeSessionService.autoStartRuntime(runtime, 'Test requested to auto-start a runtime'); - // assert.equal(sessionId, ''); - - // workspaceTrustManagementService.setWorkspaceTrust(true); - - // await new Promise(resolve => { - // disposables.add(runtimeSessionService.onDidStartRuntime(session => { - // if (session.runtimeMetadata === runtime) { - // disposables.add(session); - // resolve(); - // } - // })); - // }); - - // // TODO: We should probably check more things here? - // }); - // }); - // function restartSession(sessionId: string) { // return runtimeSessionService.restartSession( // sessionId, 'Test requested to restart a runtime session' @@ -705,6 +624,122 @@ suite('Positron - RuntimeSessionService', () => { // The runtime should now be registered. assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); }); + + // TODO: Should auto starting a notebook error? + + test('auto start validates runtime if unregistered', async () => { + // The runtime should not yet be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + + manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + return { ...metadata, extraRuntimeData: { someNewKey: 'someNewValue' } }; + }); + + await autoStartSession(unregisteredRuntime); + + // The *validated* runtime should now be registered. + assert.deepEqual( + languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), + { ...unregisteredRuntime, extraRuntimeData: { someNewKey: 'someNewValue' } } + ); + }); + + test('auto start encounters runtime validation error', async () => { + // The runtime should not yet be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + + const error = new Error('Failed to validate runtime metadata'); + manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + throw error; + }); + + await assert.rejects(autoStartSession(unregisteredRuntime), error); + + // The runtime should still not be registered. + assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + }); + + test('auto start console does nothing if automatic startup is disabled', async () => { + configService.setUserConfiguration('positron.interpreters.automaticStartup', false); + + const sessionId = await runtimeSessionService.autoStartRuntime(runtime, startReason); + assert.equal(sessionId, ''); + + // TODO: Do we also need to check the session state? + }); + + // TODO: Reuse code in below two tests. + test('auto start console in an untrusted workspace defers until trust is granted', async () => { + workspaceTrustManagementService.setWorkspaceTrust(false); + + const sessionId = await runtimeSessionService.autoStartRuntime(runtime, 'Test requested to auto-start a runtime'); + assert.equal(sessionId, ''); + + workspaceTrustManagementService.setWorkspaceTrust(true); + + await new Promise(resolve => { + disposables.add(runtimeSessionService.onDidStartRuntime(session => { + if (session.runtimeMetadata === runtime) { + disposables.add(session); + resolve(); + } + })); + }); + + // TODO: We should probably check more things here? + }); + + test('start console in an untrusted workspace defers until trust is granted', async () => { + workspaceTrustManagementService.setWorkspaceTrust(false); + + const sessionId = await runtimeSessionService.startNewRuntimeSession(runtime.runtimeId, sessionName, LanguageRuntimeSessionMode.Console, undefined, startReason); + assert.equal(sessionId, ''); + + workspaceTrustManagementService.setWorkspaceTrust(true); + + await new Promise(resolve => { + disposables.add(runtimeSessionService.onDidStartRuntime(session => { + if (session.runtimeMetadata === runtime) { + disposables.add(session); + resolve(); + } + })); + }); + + // TODO: We should probably check more things here? + }); + + test('start notebook in an untrusted workspace throws', async () => { + workspaceTrustManagementService.setWorkspaceTrust(false); + + await assert.rejects(startNotebook(), new Error('Cannot start a notebook session in an untrusted workspace.')); + }); + + test('select console while another runtime is running for the language', async () => { + const session1 = await startConsole(anotherRuntime); + const session2 = await selectRuntime(); + + assert.equal(session1.getRuntimeState(), RuntimeState.Exited); + assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + + assertServiceState({ + hasStartingOrRunningConsole: true, + consoleSession: session2, + activeSessions: [session1, session2], + }); + }); + + test('select console to the same runtime sets the foreground session', async () => { + const session1 = await startConsole(); + + runtimeSessionService.foregroundSession = undefined; + + const session2 = await selectRuntime(); + + assert.equal(session1, session2); + assert.equal(runtimeSessionService.foregroundSession, session1); + + }); }); async function waitForRuntimeState( From fe5a76e98c5d10b668744d03a12a053221945552 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 15:20:47 +0200 Subject: [PATCH 15/45] change error message when selecting a non-existing runtime --- .../workbench/services/runtimeSession/common/runtimeSession.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 394ae66b1e0..abc665034ad 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -311,8 +311,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession async selectRuntime(runtimeId: string, source: string): Promise { const runtime = this._languageRuntimeService.getRegisteredRuntime(runtimeId); if (!runtime) { - return Promise.reject(new Error(`Language runtime ID '${runtimeId}' ` + - `is not registered.`)); + throw new Error(`No language runtime with id '${runtimeId}' was found.`); } // Shut down any other runtime consoles for the language. From 6487dea33ecee61ab04ff03be84f583ec9779300 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 15:21:01 +0200 Subject: [PATCH 16/45] throw when starting a notebook in an untrusted workspace --- .../services/runtimeSession/common/runtimeSession.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index abc665034ad..289b7a103f0 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -426,7 +426,11 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // If the workspace is not trusted, defer starting the runtime until the // workspace is trusted. if (!this._workspaceTrustManagementService.isWorkspaceTrusted()) { - return this.autoStartRuntime(languageRuntime, source); + if (sessionMode === LanguageRuntimeSessionMode.Console) { + return this.autoStartRuntime(languageRuntime, source); + } else { + throw new Error(`Cannot start a ${sessionMode} session in an untrusted workspace.`); + } } // Start the runtime. From 987af008c1ccf8294c0f30ca96eeead309337d37 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 15:22:00 +0200 Subject: [PATCH 17/45] coalesce concurrent auto start requests; validate before auto start --- .../runtimeSession/common/runtimeSession.ts | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 289b7a103f0..9d2cf1d0530 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -700,6 +700,34 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession metadata: ILanguageRuntimeMetadata, source: string): Promise { + // Auto-started runtimes are (currently) always console sessions. + const sessionMode = LanguageRuntimeSessionMode.Console; + const notebookUri = undefined; + + // See if we are already starting the requested session. If we + // are, return the promise that resolves when the session is ready to + // use. This makes it possible for multiple requests to start the same + // session to be coalesced. + const startingRuntimePromise = this._startingSessionsBySessionMapKey.get( + getSessionMapKey(sessionMode, metadata.runtimeId, notebookUri)); + if (startingRuntimePromise && !startingRuntimePromise.isSettled) { + return startingRuntimePromise.p; + } + + const runningSessionId = this.validateRuntimeSessionStart(sessionMode, metadata, notebookUri, source); + if (runningSessionId) { + return runningSessionId; + } + + // Before attempting to validate the runtime, add it to the set of + // starting consoles. + this._startingConsolesByLanguageId.set(metadata.languageId, metadata); + + // Create a promise that resolves when the runtime is ready to use. + const startPromise = new DeferredPromise(); + const sessionMapKey = getSessionMapKey(sessionMode, metadata.runtimeId, notebookUri); + this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + // Check to see if the runtime has already been registered with the // language runtime service. const languageRuntime = @@ -709,10 +737,6 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // If it has not been registered, validate the metadata. if (!languageRuntime) { try { - // Before attempting to validate the runtime, add it to the set of - // starting consoles. - this._startingConsolesByLanguageId.set(metadata.languageId, metadata); - // Attempt to validate the metadata. Note that this can throw if the metadata // is invalid! const validated = await sessionManager.validateMetadata(metadata); @@ -764,9 +788,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession } } - // Auto-started runtimes are (currently) always console sessions. - return this.doCreateRuntimeSession(metadata, metadata.runtimeName, - LanguageRuntimeSessionMode.Console, source); + return this.doCreateRuntimeSession(metadata, metadata.runtimeName, sessionMode, source, notebookUri); } /** @@ -793,10 +815,14 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession this._startingNotebooksByNotebookUri.set(notebookUri, runtimeMetadata); } - // Create a promise that resolves when the runtime is ready to use. - const startPromise = new DeferredPromise(); - const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); - this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + // Create a promise that resolves when the runtime is ready to use, if there isn't already one. + let startPromise = this._startingSessionsBySessionMapKey.get( + getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri)); + if (!startPromise || startPromise.isSettled) { + startPromise = new DeferredPromise(); + const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); + this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + } const sessionManager = await this.getManagerForRuntime(runtimeMetadata); const sessionId = this.generateNewSessionId(runtimeMetadata); From d6eb11cb9602938dbdaeb402ae361ea53e896941 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 15:45:13 +0200 Subject: [PATCH 18/45] finalize auto start tests --- .../test/common/runtimeSession.test.ts | 104 ++++++++---------- 1 file changed, 46 insertions(+), 58 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 2c2e67ce6cc..c87a1bba948 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -6,6 +6,7 @@ import { strict as assert } from 'assert'; import * as sinon from 'sinon'; import { CancellationError } from 'vs/base/common/errors'; +import { Event } from 'vs/base/common/event'; import { URI } from 'vs/base/common/uri'; import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; @@ -148,6 +149,14 @@ suite('Positron - RuntimeSessionService', () => { ); } + function assertSessionWillStart(sessionMode: LanguageRuntimeSessionMode) { + if (sessionMode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ hasStartingOrRunningConsole: true }); + } else if (sessionMode === LanguageRuntimeSessionMode.Notebook) { + assertServiceState(); + } + } + function assertSessionIsStarting(session: ILanguageRuntimeSession) { assert.equal(session.getRuntimeState(), RuntimeState.Starting); @@ -233,16 +242,12 @@ suite('Positron - RuntimeSessionService', () => { const promise = start(); - // Check the state while starting. - if (mode === LanguageRuntimeSessionMode.Console) { - assertServiceState({ hasStartingOrRunningConsole: true }); - } else { - assertServiceState(); - } + // Check the state before awaiting the promise. + assertSessionWillStart(mode); const session = await promise; - // Check the state after starting. + // Check the state after awaiting the promise. assertSessionIsStarting(session); }); @@ -263,11 +268,7 @@ suite('Positron - RuntimeSessionService', () => { } assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); - if (mode === LanguageRuntimeSessionMode.Console) { - assertServiceState({ hasStartingOrRunningConsole: true }); - } else { - assertServiceState(); - } + assertSessionWillStart(mode); } catch (e) { error = e; } @@ -341,7 +342,7 @@ suite('Positron - RuntimeSessionService', () => { }); } - test(`${action} ${mode} encounters session.start() error`, async () => { + test(`${action} ${mode} fires onDidFailStartRuntime if session.start() errors`, async () => { // Listen to the onWillStartSession event and stub session.start() to throw an error. const willStartSession = sinon.spy((e: IRuntimeSessionWillStartEvent) => { sinon.stub(e.session, 'start').rejects(new Error('Session failed to start')); @@ -366,7 +367,7 @@ suite('Positron - RuntimeSessionService', () => { assert.equal(runtimeSessionService.getSession(session.sessionId), session); assert.deepEqual(runtimeSessionService.activeSessions, [session]); } else { - // TODO: Should failed notebook sessions be included in activeSessions? + // TODO: Should failed sessions be included in activeSessions? assertServiceState({ activeSessions: [session] }); } @@ -625,37 +626,40 @@ suite('Positron - RuntimeSessionService', () => { assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); }); - // TODO: Should auto starting a notebook error? - test('auto start validates runtime if unregistered', async () => { // The runtime should not yet be registered. assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + // Update the validator to add extra runtime data. + const validatedMetadata: Partial = { + extraRuntimeData: { someNewKey: 'someNewValue' } + }; manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { - return { ...metadata, extraRuntimeData: { someNewKey: 'someNewValue' } }; + return { ...metadata, ...validatedMetadata }; }); await autoStartSession(unregisteredRuntime); - // The *validated* runtime should now be registered. + // The validated metadata should now be registered. assert.deepEqual( languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), - { ...unregisteredRuntime, extraRuntimeData: { someNewKey: 'someNewValue' } } + { ...unregisteredRuntime, ...validatedMetadata } ); }); - test('auto start encounters runtime validation error', async () => { + test('auto start throws if runtime validation errors', async () => { // The runtime should not yet be registered. assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + // Update the validator to throw. const error = new Error('Failed to validate runtime metadata'); - manager.setValidateMetadata(async (metadata: ILanguageRuntimeMetadata) => { + manager.setValidateMetadata(async (_metadata: ILanguageRuntimeMetadata) => { throw error; }); await assert.rejects(autoStartSession(unregisteredRuntime), error); - // The runtime should still not be registered. + // The runtime should remain unregistered. assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); }); @@ -663,51 +667,35 @@ suite('Positron - RuntimeSessionService', () => { configService.setUserConfiguration('positron.interpreters.automaticStartup', false); const sessionId = await runtimeSessionService.autoStartRuntime(runtime, startReason); - assert.equal(sessionId, ''); - - // TODO: Do we also need to check the session state? - }); - - // TODO: Reuse code in below two tests. - test('auto start console in an untrusted workspace defers until trust is granted', async () => { - workspaceTrustManagementService.setWorkspaceTrust(false); - const sessionId = await runtimeSessionService.autoStartRuntime(runtime, 'Test requested to auto-start a runtime'); assert.equal(sessionId, ''); + assertServiceState(); + }); - workspaceTrustManagementService.setWorkspaceTrust(true); + for (const action of ['auto start', 'start']) { + test(`${action} console in an untrusted workspace defers until trust is granted`, async () => { + workspaceTrustManagementService.setWorkspaceTrust(false); - await new Promise(resolve => { - disposables.add(runtimeSessionService.onDidStartRuntime(session => { - if (session.runtimeMetadata === runtime) { - disposables.add(session); - resolve(); - } - })); - }); - - // TODO: We should probably check more things here? - }); + let sessionId: string; + if (action === 'auto start') { + sessionId = await runtimeSessionService.autoStartRuntime(runtime, startReason); + } else { + sessionId = await runtimeSessionService.startNewRuntimeSession( + runtime.runtimeId, sessionName, LanguageRuntimeSessionMode.Console, undefined, startReason); + } - test('start console in an untrusted workspace defers until trust is granted', async () => { - workspaceTrustManagementService.setWorkspaceTrust(false); + assert.equal(sessionId, ''); + assertServiceState(); - const sessionId = await runtimeSessionService.startNewRuntimeSession(runtime.runtimeId, sessionName, LanguageRuntimeSessionMode.Console, undefined, startReason); - assert.equal(sessionId, ''); + workspaceTrustManagementService.setWorkspaceTrust(true); - workspaceTrustManagementService.setWorkspaceTrust(true); + // The session should eventually start. + const session = await Event.toPromise(runtimeSessionService.onDidStartRuntime); + disposables.add(session); - await new Promise(resolve => { - disposables.add(runtimeSessionService.onDidStartRuntime(session => { - if (session.runtimeMetadata === runtime) { - disposables.add(session); - resolve(); - } - })); + assertSessionIsStarting(session); }); - - // TODO: We should probably check more things here? - }); + } test('start notebook in an untrusted workspace throws', async () => { workspaceTrustManagementService.setWorkspaceTrust(false); From fa91bf62baa9314206491ce5d45defe37df83b3b Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 16:35:49 +0200 Subject: [PATCH 19/45] fix typo --- .../services/runtimeSession/common/runtimeSession.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 9d2cf1d0530..281a359f27a 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -816,11 +816,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession } // Create a promise that resolves when the runtime is ready to use, if there isn't already one. - let startPromise = this._startingSessionsBySessionMapKey.get( - getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri)); + const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); + let startPromise = this._startingSessionsBySessionMapKey.get(sessionMapKey); if (!startPromise || startPromise.isSettled) { startPromise = new DeferredPromise(); - const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); } From 79cad9e985bb5d9db4631382525df3bac0fb410d Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 16:35:58 +0200 Subject: [PATCH 20/45] add restart tests --- .../test/common/runtimeSession.test.ts | 346 ++++++++++-------- 1 file changed, 199 insertions(+), 147 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index c87a1bba948..6f014646397 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -13,8 +13,7 @@ import { IConfigurationService } from 'vs/platform/configuration/common/configur import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService'; import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock'; import { IWorkspaceTrustManagementService } from 'vs/platform/workspace/common/workspaceTrust'; -// import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, LanguageRuntimeSessionMode, RuntimeExitReason, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; -import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, ILanguageRuntimeService, LanguageRuntimeSessionMode, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; +import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, ILanguageRuntimeService, LanguageRuntimeSessionMode, RuntimeExitReason, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; import { ILanguageRuntimeSession, IRuntimeSessionMetadata, IRuntimeSessionService, IRuntimeSessionWillStartEvent } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; import { TestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession'; import { createRuntimeServices, createTestLanguageRuntimeMetadata, startTestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService'; @@ -463,150 +462,6 @@ suite('Positron - RuntimeSessionService', () => { }); }); } - - // function restartSession(sessionId: string) { - // return runtimeSessionService.restartSession( - // sessionId, 'Test requested to restart a runtime session' - // ); - // } - - // suite('restartSession', () => { - // test('restart console in "ready" state', async () => { - // const session = await startConsole(); - // await waitForRuntimeState(session, RuntimeState.Ready); - - // await restartSession(session.sessionId); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // }); - - // test('restart console in "starting" state', async () => { - // const session = await startConsole(); - - // await restartSession(session.sessionId); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // }); - - // test('restart console in "exited" state', async () => { - // const session = await startConsole(); - // await session.shutdown(RuntimeExitReason.Shutdown); - // await waitForRuntimeState(session, RuntimeState.Exited); - - // await restartSession(session.sessionId); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // }); - - // test('restart session with unknown session id', async () => { - // const sessionId = 'unknown-session-id'; - // assert.rejects( - // restartSession(sessionId), - // new Error(`No session with ID '${sessionId}' was found.`), - // ); - // }); - - // test('restart console concurrently', async () => { - // const session = await startConsole(); - // await waitForRuntimeState(session, RuntimeState.Ready); - - // const target = sinon.spy(session, 'restart'); - - // await Promise.all([ - // restartSession(session.sessionId), - // restartSession(session.sessionId), - // restartSession(session.sessionId), - // ]); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - - // sinon.assert.calledOnce(target); - // }); - - // test('restart console successively', async () => { - // const session = await startConsole(); - - // const target = sinon.spy(session, 'restart'); - - // await waitForRuntimeState(session, RuntimeState.Ready); - // await restartSession(session.sessionId); - // await waitForRuntimeState(session, RuntimeState.Ready); - // await restartSession(session.sessionId); - // await waitForRuntimeState(session, RuntimeState.Ready); - // await restartSession(session.sessionId); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }); - - // sinon.assert.calledThrice(target); - // }); - - // }); - - // suite('queuing', () => { - // test(`${name} notebook while shutting down`, async () => { - // const session1 = await startNotebook(); - - // const [, session2,] = await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // startNotebook(), - // ]); - - // assert.equal(session1.getRuntimeState(), RuntimeState.Exited); - // assert.equal(session2.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ - // notebookSession: session2, - // notebookSessionForNotebookUri: session2, - // activeSessions: [session1, session2], - // }); - // }); - - // test(`${name} notebook while restarting and in "exited" state`, async () => { - // const session = await startNotebook(); - // await waitForRuntimeState(session, RuntimeState.Ready); - - // const target = sinon.spy(session, 'restart'); - - // const startPromise = new Promise(resolve => { - // const disposable = session.onDidChangeRuntimeState(state => { - // if (state === RuntimeState.Exited) { - // disposable.dispose(); - // resolve(startNotebook()); - // } - // }); - // }); - - // const [, session2,] = await Promise.all([ - // restartSession(session.sessionId), - // startPromise, - // // startNotebook(), - // ]); - - // assert.equal(session, session2); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ - // notebookSession: session, - // notebookSessionForNotebookUri: session, - // }); - - // sinon.assert.calledOnce(target); - // }); - - // test('restart notebook while shutting down', async () => { - // const session = await startNotebook(); - - // await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // restartSession(session.sessionId), - // ]); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertNotebookSessionIsStarted(session); - // }); - - // }); } test(`start notebook without notebook uri`, async () => { @@ -726,8 +581,205 @@ suite('Positron - RuntimeSessionService', () => { assert.equal(session1, session2); assert.equal(runtimeSessionService.foregroundSession, session1); - }); + + function restartSession(sessionId: string) { + return runtimeSessionService.restartSession(sessionId, startReason); + } + + for (const { mode, start } of [ + { mode: LanguageRuntimeSessionMode.Console, start: startConsole }, + { mode: LanguageRuntimeSessionMode.Notebook, start: startNotebook }, + ]) { + test(`restart ${mode} throws if session not found`, async () => { + const sessionId = 'unknown-session-id'; + assert.rejects( + restartSession(sessionId), + new Error(`No session with ID '${sessionId}' was found.`), + ); + }); + + for (const state of [RuntimeState.Busy, RuntimeState.Idle, RuntimeState.Ready]) { + test(`restart ${mode} in '${state}' state`, async () => { + // Start the session and wait for it to be ready. + const session = await start(); + await waitForRuntimeState(session, RuntimeState.Ready); + + // Set the state to the desired state. + if (session.getRuntimeState() !== state) { + session.setRuntimeState(state); + } + + await restartSession(session.sessionId); + + assertSessionIsStarting(session); + }); + } + + for (const state of [RuntimeState.Uninitialized, RuntimeState.Exited]) { + test(`restart ${mode} in '${state}' state`, async () => { + // Get a session to the exited state. + const session = await start(); + await session.shutdown(RuntimeExitReason.Shutdown); + await waitForRuntimeState(session, RuntimeState.Exited); + + await restartSession(session.sessionId); + + // The existing sessino should remain exited. + assert.equal(session.getRuntimeState(), RuntimeState.Exited); + + // A new session should be starting. + let newSession: ILanguageRuntimeSession | undefined; + if (mode === LanguageRuntimeSessionMode.Console) { + newSession = runtimeSessionService.getConsoleSessionForRuntime(runtime.runtimeId); + } else { + newSession = runtimeSessionService.getNotebookSessionForNotebookUri(notebookUri); + } + assert.ok(newSession); + disposables.add(newSession); + + assert.equal(newSession.getRuntimeState(), RuntimeState.Starting); + assert.equal(newSession.metadata.sessionName, session.metadata.sessionName); + assert.equal(newSession.metadata.sessionMode, session.metadata.sessionMode); + assert.equal(newSession.metadata.notebookUri, session.metadata.notebookUri); + assert.equal(newSession.runtimeMetadata, session.runtimeMetadata); + + if (mode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ + hasStartingOrRunningConsole: true, + consoleSession: newSession, + activeSessions: [session, newSession], + }) + } else { + assertServiceState({ + notebookSession: newSession, + notebookSessionForNotebookUri: newSession, + activeSessions: [session, newSession], + }) + } + }); + } + + test(`restart ${mode} in 'starting' state`, async () => { + const session = await start(); + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + + await restartSession(session.sessionId); + + assertSessionIsStarting(session); + }); + + test(`restart ${mode} in 'restarting' state`, async () => { + const session = await start(); + await waitForRuntimeState(session, RuntimeState.Ready); + + session.restart(); + assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + + const target = sinon.spy(session, 'restart'); + + await restartSession(session.sessionId); + + assertSessionIsStarting(session); + + sinon.assert.notCalled(target); + }); + + test(`restart ${mode} concurrently`, async () => { + const session = await start(); + await waitForRuntimeState(session, RuntimeState.Ready); + + const target = sinon.spy(session, 'restart'); + + await Promise.all([ + restartSession(session.sessionId), + restartSession(session.sessionId), + restartSession(session.sessionId), + ]); + + assertSessionIsStarting(session); + + sinon.assert.calledOnce(target); + }); + + test(`restart ${mode} successively`, async () => { + const session = await start(); + + const target = sinon.spy(session, 'restart'); + + await waitForRuntimeState(session, RuntimeState.Ready); + await restartSession(session.sessionId); + await waitForRuntimeState(session, RuntimeState.Ready); + await restartSession(session.sessionId); + await waitForRuntimeState(session, RuntimeState.Ready); + await restartSession(session.sessionId); + + assertSessionIsStarting(session); + + sinon.assert.calledThrice(target); + }); + } + + // test(`${name} notebook while shutting down`, async () => { + // const session1 = await startNotebook(); + + // const [, session2,] = await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // startNotebook(), + // ]); + + // assert.equal(session1.getRuntimeState(), RuntimeState.Exited); + // assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ + // notebookSession: session2, + // notebookSessionForNotebookUri: session2, + // activeSessions: [session1, session2], + // }); + // }); + + // test(`${name} notebook while restarting and in "exited" state`, async () => { + // const session = await startNotebook(); + // await waitForRuntimeState(session, RuntimeState.Ready); + + // const target = sinon.spy(session, 'restart'); + + // const startPromise = new Promise(resolve => { + // const disposable = session.onDidChangeRuntimeState(state => { + // if (state === RuntimeState.Exited) { + // disposable.dispose(); + // resolve(startNotebook()); + // } + // }); + // }); + + // const [, session2,] = await Promise.all([ + // restartSession(session.sessionId), + // startPromise, + // // startNotebook(), + // ]); + + // assert.equal(session, session2); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertServiceState({ + // notebookSession: session, + // notebookSessionForNotebookUri: session, + // }); + + // sinon.assert.calledOnce(target); + // }); + + // test('restart notebook while shutting down', async () => { + // const session = await startNotebook(); + + // await Promise.all([ + // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), + // restartSession(session.sessionId), + // ]); + + // assert.equal(session.getRuntimeState(), RuntimeState.Starting); + // assertNotebookSessionIsStarted(session); + // }); }); async function waitForRuntimeState( From ed52e44e8f04b2df5529642b38bdcf9b0a6db432 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 16:36:27 +0200 Subject: [PATCH 21/45] fix restarting notebooks --- .../services/runtimeSession/common/runtimeSession.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 281a359f27a..5b2554f7526 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -996,14 +996,15 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession this.foregroundSession = session; } - // If this is a console session and there isn't already a console session - // for this language, set this one as the console session. - // (This restores the console session in the case of a - // restart) + // Restore the session in the case of a restart. if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && !this._consoleSessionsByLanguageId.has(session.runtimeMetadata.languageId)) { this._consoleSessionsByLanguageId.set(session.runtimeMetadata.languageId, session); + } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook && + session.metadata.notebookUri && + !this._notebookSessionsByNotebookUri.has(session.metadata.notebookUri)) { + this._notebookSessionsByNotebookUri.set(session.metadata.notebookUri, session); } From 6044bda1e033160a4e0b7aee9acce5f282d73c25 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 16:50:02 +0200 Subject: [PATCH 22/45] test encountering createSession/restoreSession error; catch uncaught exceptions --- .../runtimeSession/common/runtimeSession.ts | 18 ++++++++++++------ .../test/common/runtimeSession.test.ts | 8 ++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 5b2554f7526..c1d3f80e7d7 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -482,6 +482,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession sessionMetadata.sessionMode, runtimeMetadata.runtimeId, sessionMetadata.notebookUri); this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + // It's possible that startPromise is never awaited, so we log any errors here + // at the debug level since we still expect the error to be handled/logged elsewhere. + startPromise.p.catch((err) => this._logService.debug(`Error starting session: ${err}`)); + // Add the runtime to the starting runtimes. if (sessionMetadata.sessionMode === LanguageRuntimeSessionMode.Console) { this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); @@ -520,8 +524,6 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession } catch (err) { startPromise.error(err); } - - startPromise.p.catch((err) => this._logService.debug(`Error starting session: ${err}`)); } /** @@ -728,6 +730,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession const sessionMapKey = getSessionMapKey(sessionMode, metadata.runtimeId, notebookUri); this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + // It's possible that startPromise is never awaited, so we log any errors here + // at the debug level since we still expect the error to be handled/logged elsewhere. + startPromise.p.catch(err => this._logService.debug(`Error starting runtime session: ${err}`)); + // Check to see if the runtime has already been registered with the // language runtime service. const languageRuntime = @@ -821,6 +827,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession if (!startPromise || startPromise.isSettled) { startPromise = new DeferredPromise(); this._startingSessionsBySessionMapKey.set(sessionMapKey, startPromise); + + // It's possible that startPromise is never awaited, so we log any errors here + // at the debug level since we still expect the error to be handled/logged elsewhere. + startPromise.p.catch(err => this._logService.debug(`Error starting runtime session: ${err}`)); } const sessionManager = await this.getManagerForRuntime(runtimeMetadata); @@ -861,10 +871,6 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession startPromise.error(err); } - // It's possible that startPromise is never awaited, so we log any errors here - // at the debug level since we still expect the error to be handled/logged elsewhere. - startPromise.p.catch(err => this._logService.debug(`Error starting runtime session: ${err}`)); - return sessionId; } diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 6f014646397..65ca7fd854c 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -341,6 +341,14 @@ suite('Positron - RuntimeSessionService', () => { }); } + const createOrRestoreMethod = action === 'restore' ? 'restoreSession' : 'createSession'; + test(`${action} ${mode} encounters ${createOrRestoreMethod}() error`, async () => { + const error = new Error('Failed to create session'); + sinon.stub(manager, createOrRestoreMethod).rejects(error); + + await assert.rejects(start(), error); + }); + test(`${action} ${mode} fires onDidFailStartRuntime if session.start() errors`, async () => { // Listen to the onWillStartSession event and stub session.start() to throw an error. const willStartSession = sinon.spy((e: IRuntimeSessionWillStartEvent) => { From 1b549782fa4a619375ce6131c2ad6e6fb3e18345 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 18:05:19 +0200 Subject: [PATCH 23/45] refine test session shutdown & restart behavior; fix related tests; start queuing tests --- .../test/common/runtimeSession.test.ts | 205 ++++++++++-------- .../test/common/testLanguageRuntimeSession.ts | 31 ++- 2 files changed, 147 insertions(+), 89 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 65ca7fd854c..2882517f0fb 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -5,7 +5,6 @@ import { strict as assert } from 'assert'; import * as sinon from 'sinon'; -import { CancellationError } from 'vs/base/common/errors'; import { Event } from 'vs/base/common/event'; import { URI } from 'vs/base/common/uri'; import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; @@ -15,7 +14,7 @@ import { TestInstantiationService } from 'vs/platform/instantiation/test/common/ import { IWorkspaceTrustManagementService } from 'vs/platform/workspace/common/workspaceTrust'; import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, ILanguageRuntimeService, LanguageRuntimeSessionMode, RuntimeExitReason, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; import { ILanguageRuntimeSession, IRuntimeSessionMetadata, IRuntimeSessionService, IRuntimeSessionWillStartEvent } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; -import { TestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession'; +import { TestLanguageRuntimeSession, waitForRuntimeState } from 'vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession'; import { createRuntimeServices, createTestLanguageRuntimeMetadata, startTestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService'; import { TestRuntimeSessionManager } from 'vs/workbench/test/common/positronWorkbenchTestServices'; import { TestWorkspaceTrustManagementService } from 'vs/workbench/test/common/workbenchTestServices'; @@ -156,9 +155,7 @@ suite('Positron - RuntimeSessionService', () => { } } - function assertSessionIsStarting(session: ILanguageRuntimeSession) { - assert.equal(session.getRuntimeState(), RuntimeState.Starting); - + function assertSingleSession(session: ILanguageRuntimeSession) { if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }, session.runtimeMetadata); } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { @@ -166,6 +163,21 @@ suite('Positron - RuntimeSessionService', () => { } } + function assertSessionIsStarting(session: ILanguageRuntimeSession) { + assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assertSingleSession(session); + } + + function assertSessionIsRestarting(session: ILanguageRuntimeSession) { + assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + assertSingleSession(session); + } + + function assertSessionIsStarted(session: ILanguageRuntimeSession) { + assert.equal(session.getRuntimeState(), RuntimeState.Ready); + assertSingleSession(session); + } + async function restoreSession( sessionMetadata: IRuntimeSessionMetadata, runtimeMetadata = runtime, ) { @@ -568,6 +580,7 @@ suite('Positron - RuntimeSessionService', () => { test('select console while another runtime is running for the language', async () => { const session1 = await startConsole(anotherRuntime); + await waitForRuntimeState(session1, RuntimeState.Ready); const session2 = await selectRuntime(); assert.equal(session1.getRuntimeState(), RuntimeState.Exited); @@ -580,6 +593,14 @@ suite('Positron - RuntimeSessionService', () => { }); }); + test('select console throws if session is still starting', async () => { + await startConsole(anotherRuntime); + await assert.rejects( + selectRuntime(), + new Error('Cannot shut down kernel; it is not (yet) running. (state = starting)'), + ); + }); + test('select console to the same runtime sets the foreground session', async () => { const session1 = await startConsole(); @@ -620,7 +641,11 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); - assertSessionIsStarting(session); + assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + assertSessionIsRestarting(session); + + await waitForRuntimeState(session, RuntimeState.Ready); + assertSessionIsStarted(session); }); } @@ -628,6 +653,7 @@ suite('Positron - RuntimeSessionService', () => { test(`restart ${mode} in '${state}' state`, async () => { // Get a session to the exited state. const session = await start(); + await waitForRuntimeState(session, RuntimeState.Ready); await session.shutdown(RuntimeExitReason.Shutdown); await waitForRuntimeState(session, RuntimeState.Exited); @@ -688,7 +714,7 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); - assertSessionIsStarting(session); + assertSessionIsRestarting(session); sinon.assert.notCalled(target); }); @@ -705,7 +731,7 @@ suite('Positron - RuntimeSessionService', () => { restartSession(session.sessionId), ]); - assertSessionIsStarting(session); + assertSessionIsRestarting(session); sinon.assert.calledOnce(target); }); @@ -722,91 +748,94 @@ suite('Positron - RuntimeSessionService', () => { await waitForRuntimeState(session, RuntimeState.Ready); await restartSession(session.sessionId); - assertSessionIsStarting(session); + assertSessionIsRestarting(session); sinon.assert.calledThrice(target); }); } - // test(`${name} notebook while shutting down`, async () => { - // const session1 = await startNotebook(); - - // const [, session2,] = await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // startNotebook(), - // ]); - - // assert.equal(session1.getRuntimeState(), RuntimeState.Exited); - // assert.equal(session2.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ - // notebookSession: session2, - // notebookSessionForNotebookUri: session2, - // activeSessions: [session1, session2], - // }); - // }); - - // test(`${name} notebook while restarting and in "exited" state`, async () => { - // const session = await startNotebook(); - // await waitForRuntimeState(session, RuntimeState.Ready); - - // const target = sinon.spy(session, 'restart'); - - // const startPromise = new Promise(resolve => { - // const disposable = session.onDidChangeRuntimeState(state => { - // if (state === RuntimeState.Exited) { - // disposable.dispose(); - // resolve(startNotebook()); - // } - // }); - // }); - - // const [, session2,] = await Promise.all([ - // restartSession(session.sessionId), - // startPromise, - // // startNotebook(), - // ]); - - // assert.equal(session, session2); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertServiceState({ - // notebookSession: session, - // notebookSessionForNotebookUri: session, - // }); - - // sinon.assert.calledOnce(target); - // }); - - // test('restart notebook while shutting down', async () => { - // const session = await startNotebook(); - - // await Promise.all([ - // runtimeSessionService.shutdownNotebookSession(notebookUri, RuntimeExitReason.Shutdown), - // restartSession(session.sessionId), - // ]); - - // assert.equal(session.getRuntimeState(), RuntimeState.Starting); - // assertNotebookSessionIsStarted(session); - // }); -}); + suite('queueing', () => { + // Consider the following sequence of actions: + // start -> restart == should do both -- not possible since restart requires session ID + // start -> select == select should noop + // restart -> start == start should noop + // restart -> select == select should noop + // select -> start == start should noop + // select -> restart == restart should noop -- not possible since restart requires session ID + test(`start console -> select console`, async () => { + const [session,] = await Promise.all([ + startConsole(), + selectRuntime(), + ]); -async function waitForRuntimeState( - session: ILanguageRuntimeSession, - state: RuntimeState, - timeout = 10_000, -) { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - disposable.dispose(); - reject(new CancellationError()); - }, timeout); - - const disposable = session.onDidChangeRuntimeState(newState => { - if (newState === state) { - clearTimeout(timer); - disposable.dispose(); - resolve(); - } + assertSessionIsStarting(session); + }); + + // TODO: I think this behavior is undefined at the moment... We don't have any safety checks + // in start based on restart behavior. + test(`restart console -> start console`, async () => { + const session = await startConsole(); + await waitForRuntimeState(session, RuntimeState.Ready); + + const didChangeRuntimeState = sinon.spy((_state: RuntimeState) => { }); + disposables.add(session.onDidChangeRuntimeState(didChangeRuntimeState)); + + const promise2 = new Promise(resolve => { + disposables.add(session.onDidChangeRuntimeState(state => { + if (state === RuntimeState.Exited) { + const result = startConsole(); + resolve(result); + } + })); + }); + + const promise = restartSession(session.sessionId); + + await Promise.all([promise, promise2]); + + await waitForRuntimeState(session, RuntimeState.Ready); + + console.log(didChangeRuntimeState.getCalls().map(call => call.args)); + sinon.assert.callCount(didChangeRuntimeState, 4); + sinon.assert.callOrder( + didChangeRuntimeState.withArgs(RuntimeState.Restarting), + didChangeRuntimeState.withArgs(RuntimeState.Exited), + didChangeRuntimeState.withArgs(RuntimeState.Starting), + didChangeRuntimeState.withArgs(RuntimeState.Ready), + ); + sinon.assert.calledOnce(didChangeRuntimeState.withArgs(RuntimeState.Exited)); + + // await Promise.all([ + // restartSession(session.sessionId), + // // TODO: This returns the existing sessionId because it's still considered 'running'. + // // Shouldn't the session going to 'exited' remove it from console sessions? + // startConsole(), + // ]); + + // TODO: Is it possible to start a session immediately when the session goes to 'exited'? + + assertSessionIsRestarting(session); }); + + // test(`restart console -> select console`, async () => { + // const session1 = await startConsole(); + // await waitForRuntimeState(session1, RuntimeState.Ready); + + // const target = sinon.spy((_state: RuntimeState) => { }); + // disposables.add(session1.onDidChangeRuntimeState(target)); + + // const [, session2] = await Promise.all([ + // restartSession(session1.sessionId), + // selectRuntime(), + // ]); + + // assert.equal(session1.sessionId, session2.sessionId); + + // assertSessionIsStarting(session1); + + // sinon.assert.called(target); + // sinon.assert.calledOnce(target.withArgs(RuntimeState.Starting)); + // console.log(target.getCalls().map(call => call.args)); + // }); }); -} +}); diff --git a/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts b/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts index 22e8f89ac4b..e7d07654603 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts @@ -11,6 +11,7 @@ import { ILanguageRuntimeSession, IRuntimeClientInstance, IRuntimeSessionMetadat import { ILanguageRuntimeClientCreatedEvent, ILanguageRuntimeExit, ILanguageRuntimeInfo, ILanguageRuntimeMessage, ILanguageRuntimeMessageClearOutput, ILanguageRuntimeMessageError, ILanguageRuntimeMessageInput, ILanguageRuntimeMessageIPyWidget, ILanguageRuntimeMessageOutput, ILanguageRuntimeMessagePrompt, ILanguageRuntimeMessageResult, ILanguageRuntimeMessageState, ILanguageRuntimeMessageStream, ILanguageRuntimeMetadata, ILanguageRuntimeStartupFailure, LanguageRuntimeMessageType, RuntimeCodeExecutionMode, RuntimeCodeFragmentStatus, RuntimeErrorBehavior, RuntimeExitReason, RuntimeOnlineState, RuntimeOutputKind, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; import { IRuntimeClientEvent } from 'vs/workbench/services/languageRuntime/common/languageRuntimeUiClient'; import { TestRuntimeClientInstance } from 'vs/workbench/services/languageRuntime/test/common/testRuntimeClientInstance'; +import { CancellationError } from 'vs/base/common/errors'; export class TestLanguageRuntimeSession extends Disposable implements ILanguageRuntimeSession { private readonly _onDidChangeRuntimeState = this._register(new Emitter()); @@ -162,10 +163,17 @@ export class TestLanguageRuntimeSession extends Disposable implements ILanguageR async restart(): Promise { await this.shutdown(RuntimeExitReason.Restart); - await this.start(); + waitForRuntimeState(this, RuntimeState.Exited).then(() => this.start()); } async shutdown(exitReason: RuntimeExitReason): Promise { + if (this._currentState !== RuntimeState.Idle && + this._currentState !== RuntimeState.Busy && + this._currentState !== RuntimeState.Ready) { + throw new Error('Cannot shut down kernel; it is not (yet) running.' + + ` (state = ${this._currentState})`); + } + if (exitReason === RuntimeExitReason.Restart) { this._onDidChangeRuntimeState.fire(RuntimeState.Restarting); } else { @@ -362,3 +370,24 @@ export class TestLanguageRuntimeSession extends Disposable implements ILanguageR }); } } + +export async function waitForRuntimeState( + session: ILanguageRuntimeSession, + state: RuntimeState, + timeout = 10_000, +) { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + disposable.dispose(); + reject(new CancellationError()); + }, timeout); + + const disposable = session.onDidChangeRuntimeState(newState => { + if (newState === state) { + clearTimeout(timer); + disposable.dispose(); + resolve(); + } + }); + }); +} From b1eb0c4ed52257a79f934565643c9373d52a3a92 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 18:09:20 +0200 Subject: [PATCH 24/45] skip failing test for now --- .../runtimeSession/test/common/runtimeSession.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 2882517f0fb..19db2a6384a 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -683,13 +683,13 @@ suite('Positron - RuntimeSessionService', () => { hasStartingOrRunningConsole: true, consoleSession: newSession, activeSessions: [session, newSession], - }) + }); } else { assertServiceState({ notebookSession: newSession, notebookSessionForNotebookUri: newSession, activeSessions: [session, newSession], - }) + }); } }); } @@ -773,7 +773,7 @@ suite('Positron - RuntimeSessionService', () => { // TODO: I think this behavior is undefined at the moment... We don't have any safety checks // in start based on restart behavior. - test(`restart console -> start console`, async () => { + test.skip(`restart console -> start console`, async () => { const session = await startConsole(); await waitForRuntimeState(session, RuntimeState.Ready); From 0a89f1e2ac74c747f871f09dbb431e363bf443c4 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 15 Nov 2024 18:12:05 +0200 Subject: [PATCH 25/45] fix leaked disposable --- .../runtimeSession/common/runtimeSession.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index c1d3f80e7d7..751bbdb5fd7 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -366,22 +366,29 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession session: ILanguageRuntimeSession, exitReason: RuntimeExitReason): Promise { // We wait for `onDidEndSession()` rather than `RuntimeState.Exited`, because the former // generates some Console output that must finish before starting up a new runtime: + let disposable: IDisposable | undefined; const promise = new Promise(resolve => { - const disposable = session.onDidEndSession((exit) => { + disposable = session.onDidEndSession((exit) => { resolve(); - disposable.dispose(); + disposable?.dispose(); }); }); const timeout = new Promise((_, reject) => { setTimeout(() => { + disposable?.dispose(); reject(new Error(`Timed out waiting for runtime ` + `${formatLanguageRuntimeSession(session)} to finish exiting.`)); }, 5000); }); // Ask the runtime to shut down. - await session.shutdown(exitReason); + try { + await session.shutdown(exitReason); + } catch (err) { + disposable?.dispose(); + throw err; + } // Wait for the runtime onDidEndSession to resolve, or for the timeout to expire // (whichever comes first) From d15995bcc27608d422349d055bb5a972113ab1d9 Mon Sep 17 00:00:00 2001 From: seem Date: Tue, 19 Nov 2024 20:42:20 +0200 Subject: [PATCH 26/45] wip fixing restart queueing issue --- .../runtimeSession/common/runtimeSession.ts | 48 ++++-- .../test/common/runtimeSession.test.ts | 163 ++++++++---------- 2 files changed, 103 insertions(+), 108 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 751bbdb5fd7..4c3c17c9082 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -493,13 +493,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // at the debug level since we still expect the error to be handled/logged elsewhere. startPromise.p.catch((err) => this._logService.debug(`Error starting session: ${err}`)); - // Add the runtime to the starting runtimes. - if (sessionMetadata.sessionMode === LanguageRuntimeSessionMode.Console) { - this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); - } else if (sessionMetadata.sessionMode === LanguageRuntimeSessionMode.Notebook && - sessionMetadata.notebookUri) { - this._startingNotebooksByNotebookUri.set(sessionMetadata.notebookUri, runtimeMetadata); - } + this.updateSessionMapsBeforeStart( + sessionMetadata.sessionMode, runtimeMetadata, sessionMetadata.notebookUri); // We should already have a session manager registered, since we can't // get here until the extension host has been activated. @@ -821,12 +816,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession sessionMode: LanguageRuntimeSessionMode, source: string, notebookUri?: URI): Promise { - // Add the runtime to the starting runtimes. - if (sessionMode === LanguageRuntimeSessionMode.Console) { - this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); - } else if (sessionMode === LanguageRuntimeSessionMode.Notebook && notebookUri) { - this._startingNotebooksByNotebookUri.set(notebookUri, runtimeMetadata); - } + this.updateSessionMapsBeforeStart(sessionMode, runtimeMetadata, notebookUri); // Create a promise that resolves when the runtime is ready to use, if there isn't already one. const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); @@ -1020,6 +1010,15 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession this._notebookSessionsByNotebookUri.set(session.metadata.notebookUri, session); } + // Remove the session from the starting runtimes. + if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && + this._startingConsolesByLanguageId.get(session.runtimeMetadata.languageId) === session.runtimeMetadata) { + this._startingConsolesByLanguageId.delete(session.runtimeMetadata.languageId); + } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook && + session.metadata.notebookUri && + this._startingNotebooksByNotebookUri.get(session.metadata.notebookUri) === session.runtimeMetadata) { + this._startingNotebooksByNotebookUri.delete(session.metadata.notebookUri); + } // Start the UI client instance once the runtime is fully online. this.startUiClient(session); @@ -1180,6 +1179,25 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession return undefined; } + /** + * Updates the session maps (for starting consoles, notebooks, etc.), before a + * session starts. + * + * @param sessionMode The mode of the session. + * @param runtimeMetadata The metadata of the session's runtime. + * @param notebookUri The notebook URI attached to the session, if any. + */ + private updateSessionMapsBeforeStart( + sessionMode: LanguageRuntimeSessionMode, + runtimeMetadata: ILanguageRuntimeMetadata, + notebookUri?: URI) { + if (sessionMode === LanguageRuntimeSessionMode.Console) { + this._startingConsolesByLanguageId.set(runtimeMetadata.languageId, runtimeMetadata); + } else if (sessionMode === LanguageRuntimeSessionMode.Notebook && notebookUri) { + this._startingNotebooksByNotebookUri.set(notebookUri, runtimeMetadata); + } + } + /** * Updates the session maps (for active consoles, notebooks, etc.), after a * session exits. @@ -1217,6 +1235,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession state === RuntimeState.Ready) { // The runtime looks like it could handle a restart request, so send // one over. + + this.updateSessionMapsBeforeStart( + session.metadata.sessionMode, session.runtimeMetadata, session.metadata.notebookUri); + return session.restart(); } else if (state === RuntimeState.Uninitialized || state === RuntimeState.Exited) { diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 19db2a6384a..02cbf4b55fe 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -111,10 +111,21 @@ suite('Positron - RuntimeSessionService', () => { } function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { + // Check the active sessions. + assert.deepEqual( + runtimeSessionService.activeSessions?.map(session => session.sessionId), + expectedState?.activeSessions?.map(session => session?.sessionId) ?? + [expectedState?.consoleSession?.sessionId, expectedState?.notebookSession?.sessionId].filter(session => Boolean(session)), + 'Unexpected activeSessions', + ); + // Check the console session state. assert.equal( runtimeSessionService.hasStartingOrRunningConsole(runtimeMetadata.languageId), expectedState?.hasStartingOrRunningConsole ?? false, + expectedState?.hasStartingOrRunningConsole ? + 'Expected a starting or running console session' : + 'Expected no starting or running console session', ); assert.equal( runtimeSessionService.getConsoleSessionForLanguage(runtimeMetadata.languageId)?.sessionId, @@ -138,16 +149,9 @@ suite('Positron - RuntimeSessionService', () => { runtimeSessionService.getNotebookSessionForNotebookUri(notebookUri)?.sessionId, expectedState?.notebookSessionForNotebookUri?.sessionId, ); - - // Check the global state. - assert.deepEqual( - runtimeSessionService.activeSessions?.map(session => session.sessionId), - expectedState?.activeSessions?.map(session => session?.sessionId) ?? - [expectedState?.consoleSession?.sessionId, expectedState?.notebookSession?.sessionId].filter(session => Boolean(session)), - ); } - function assertSessionWillStart(sessionMode: LanguageRuntimeSessionMode) { + function assertSingleSessionWillStart(sessionMode: LanguageRuntimeSessionMode) { if (sessionMode === LanguageRuntimeSessionMode.Console) { assertServiceState({ hasStartingOrRunningConsole: true }); } else if (sessionMode === LanguageRuntimeSessionMode.Notebook) { @@ -163,19 +167,19 @@ suite('Positron - RuntimeSessionService', () => { } } - function assertSessionIsStarting(session: ILanguageRuntimeSession) { - assert.equal(session.getRuntimeState(), RuntimeState.Starting); + function assertSingleSessionIsStarting(session: ILanguageRuntimeSession) { assertSingleSession(session); + assert.equal(session.getRuntimeState(), RuntimeState.Starting); } - function assertSessionIsRestarting(session: ILanguageRuntimeSession) { - assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + function assertSingleSessionIsRestarting(session: ILanguageRuntimeSession) { assertSingleSession(session); + assert.equal(session.getRuntimeState(), RuntimeState.Restarting); } - function assertSessionIsStarted(session: ILanguageRuntimeSession) { - assert.equal(session.getRuntimeState(), RuntimeState.Ready); + function assertSingleSessionIsStarted(session: ILanguageRuntimeSession) { assertSingleSession(session); + assert.equal(session.getRuntimeState(), RuntimeState.Ready); } async function restoreSession( @@ -254,12 +258,12 @@ suite('Positron - RuntimeSessionService', () => { const promise = start(); // Check the state before awaiting the promise. - assertSessionWillStart(mode); + assertSingleSessionWillStart(mode); const session = await promise; // Check the state after awaiting the promise. - assertSessionIsStarting(session); + assertSingleSessionIsStarting(session); }); test(`${action} ${mode} fires onWillStartSession`, async () => { @@ -279,7 +283,7 @@ suite('Positron - RuntimeSessionService', () => { } assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); - assertSessionWillStart(mode); + assertSingleSessionWillStart(mode); } catch (e) { error = e; } @@ -300,7 +304,7 @@ suite('Positron - RuntimeSessionService', () => { try { assert.equal(session.getRuntimeState(), RuntimeState.Starting); - assertSessionIsStarting(session); + assertSingleSessionIsStarting(session); } catch (e) { error = e; } @@ -449,7 +453,7 @@ suite('Positron - RuntimeSessionService', () => { assert.equal(result1, result2); assert.equal(result2, result3); - assertSessionIsStarting(result1); + assertSingleSessionIsStarting(result1); }); test(`${action} ${mode} concurrently`, async () => { @@ -458,7 +462,7 @@ suite('Positron - RuntimeSessionService', () => { assert.equal(result1, result2); assert.equal(result2, result3); - assertSessionIsStarting(result1); + assertSingleSessionIsStarting(result1); }); } @@ -568,7 +572,7 @@ suite('Positron - RuntimeSessionService', () => { const session = await Event.toPromise(runtimeSessionService.onDidStartRuntime); disposables.add(session); - assertSessionIsStarting(session); + assertSingleSessionIsStarting(session); }); } @@ -642,10 +646,10 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); assert.equal(session.getRuntimeState(), RuntimeState.Restarting); - assertSessionIsRestarting(session); + assertSingleSessionIsRestarting(session); await waitForRuntimeState(session, RuntimeState.Ready); - assertSessionIsStarted(session); + assertSingleSessionIsStarted(session); }); } @@ -700,7 +704,7 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); - assertSessionIsStarting(session); + assertSingleSessionIsStarting(session); }); test(`restart ${mode} in 'restarting' state`, async () => { @@ -714,7 +718,7 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); - assertSessionIsRestarting(session); + assertSingleSessionIsRestarting(session); sinon.assert.notCalled(target); }); @@ -731,7 +735,7 @@ suite('Positron - RuntimeSessionService', () => { restartSession(session.sessionId), ]); - assertSessionIsRestarting(session); + assertSingleSessionIsRestarting(session); sinon.assert.calledOnce(target); }); @@ -748,94 +752,63 @@ suite('Positron - RuntimeSessionService', () => { await waitForRuntimeState(session, RuntimeState.Ready); await restartSession(session.sessionId); - assertSessionIsRestarting(session); + assertSingleSessionIsRestarting(session); sinon.assert.calledThrice(target); }); } suite('queueing', () => { - // Consider the following sequence of actions: - // start -> restart == should do both -- not possible since restart requires session ID - // start -> select == select should noop - // restart -> start == start should noop - // restart -> select == select should noop - // select -> start == start should noop - // select -> restart == restart should noop -- not possible since restart requires session ID - test(`start console -> select console`, async () => { - const [session,] = await Promise.all([ - startConsole(), - selectRuntime(), - ]); + // test(`start console -> select console`, async () => { + // const [session,] = await Promise.all([ + // startConsole(), + // // Selecting the same runtime should do nothing. + // selectRuntime(), + // ]); - assertSessionIsStarting(session); - }); + // assertSessionIsStarting(session); + // }); - // TODO: I think this behavior is undefined at the moment... We don't have any safety checks - // in start based on restart behavior. - test.skip(`restart console -> start console`, async () => { + // test(`select console -> start console`, async () => { + // const [session,] = await Promise.all([ + // selectRuntime(), + // // Starting the same runtime should do nothing. + // startConsole(), + // ]); + + // assertSessionIsStarting(session); + // }); + + // test(`select another runtime for console -> start console`, async () => { + // const session = await startConsole(); + + // const [session2,] = await Promise.all([ + // selectRuntime(anotherRuntime), + // // Starting the same runtime should do nothing. + // startConsole(anotherRuntime), + // ]); + + // assertSessionIsStarting(session2); + // }); + + test.skip(`restart console -> start console when exited`, async () => { const session = await startConsole(); await waitForRuntimeState(session, RuntimeState.Ready); - const didChangeRuntimeState = sinon.spy((_state: RuntimeState) => { }); - disposables.add(session.onDidChangeRuntimeState(didChangeRuntimeState)); - - const promise2 = new Promise(resolve => { + // Send another start request once the session exits. + const startPromise = new Promise(resolve => { disposables.add(session.onDidChangeRuntimeState(state => { if (state === RuntimeState.Exited) { - const result = startConsole(); - resolve(result); + resolve(startConsole()); } })); }); - const promise = restartSession(session.sessionId); - - await Promise.all([promise, promise2]); - - await waitForRuntimeState(session, RuntimeState.Ready); - - console.log(didChangeRuntimeState.getCalls().map(call => call.args)); - sinon.assert.callCount(didChangeRuntimeState, 4); - sinon.assert.callOrder( - didChangeRuntimeState.withArgs(RuntimeState.Restarting), - didChangeRuntimeState.withArgs(RuntimeState.Exited), - didChangeRuntimeState.withArgs(RuntimeState.Starting), - didChangeRuntimeState.withArgs(RuntimeState.Ready), - ); - sinon.assert.calledOnce(didChangeRuntimeState.withArgs(RuntimeState.Exited)); - - // await Promise.all([ - // restartSession(session.sessionId), - // // TODO: This returns the existing sessionId because it's still considered 'running'. - // // Shouldn't the session going to 'exited' remove it from console sessions? - // startConsole(), - // ]); + restartSession(session.sessionId); - // TODO: Is it possible to start a session immediately when the session goes to 'exited'? + await startPromise; - assertSessionIsRestarting(session); + assertSingleSessionIsStarting(session); }); - - // test(`restart console -> select console`, async () => { - // const session1 = await startConsole(); - // await waitForRuntimeState(session1, RuntimeState.Ready); - - // const target = sinon.spy((_state: RuntimeState) => { }); - // disposables.add(session1.onDidChangeRuntimeState(target)); - - // const [, session2] = await Promise.all([ - // restartSession(session1.sessionId), - // selectRuntime(), - // ]); - - // assert.equal(session1.sessionId, session2.sessionId); - - // assertSessionIsStarting(session1); - - // sinon.assert.called(target); - // sinon.assert.calledOnce(target.withArgs(RuntimeState.Starting)); - // console.log(target.getCalls().map(call => call.args)); - // }); }); }); From 8ae2a4a7a422be1eb523035b0557af2fc6d4674c Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:24:08 +0200 Subject: [PATCH 27/45] make console session for language/runtime checks explicit --- .../test/common/runtimeSession.test.ts | 51 +++++++++++++------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 02cbf4b55fe..d868154cac2 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -105,19 +105,24 @@ suite('Positron - RuntimeSessionService', () => { interface IServiceState { hasStartingOrRunningConsole?: boolean; consoleSession?: ILanguageRuntimeSession; + consoleSessionForLanguage?: ILanguageRuntimeSession; + consoleSessionForRuntime?: ILanguageRuntimeSession; notebookSession?: ILanguageRuntimeSession; notebookSessionForNotebookUri?: ILanguageRuntimeSession; activeSessions?: ILanguageRuntimeSession[]; } - function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { - // Check the active sessions. + function assertActiveSessions(expectedSessions: ILanguageRuntimeSession[]) { assert.deepEqual( - runtimeSessionService.activeSessions?.map(session => session.sessionId), - expectedState?.activeSessions?.map(session => session?.sessionId) ?? - [expectedState?.consoleSession?.sessionId, expectedState?.notebookSession?.sessionId].filter(session => Boolean(session)), - 'Unexpected activeSessions', + runtimeSessionService.activeSessions.map(session => session.sessionId), + expectedSessions.map(session => session.sessionId), ); + } + + function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { + // Check the active sessions. + assertActiveSessions(expectedState?.activeSessions ?? + [expectedState?.consoleSession, expectedState?.notebookSession].filter(session => !!session)); // Check the console session state. assert.equal( @@ -129,11 +134,11 @@ suite('Positron - RuntimeSessionService', () => { ); assert.equal( runtimeSessionService.getConsoleSessionForLanguage(runtimeMetadata.languageId)?.sessionId, - expectedState?.consoleSession?.sessionId, + expectedState?.consoleSessionForLanguage?.sessionId, ); assert.equal( runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId)?.sessionId, - expectedState?.consoleSession?.sessionId, + expectedState?.consoleSessionForRuntime?.sessionId, ); assert.equal( runtimeSessionService.getSession(expectedState?.consoleSession?.sessionId ?? '')?.sessionId, @@ -159,26 +164,34 @@ suite('Positron - RuntimeSessionService', () => { } } - function assertSingleSession(session: ILanguageRuntimeSession) { + function assertHasSingleSession(session: ILanguageRuntimeSession) { if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { - assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session }, session.runtimeMetadata); + assertServiceState({ + hasStartingOrRunningConsole: true, + consoleSession: session, + consoleSessionForLanguage: session, + consoleSessionForRuntime: session, + }, session.runtimeMetadata); } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { - assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session }, session.runtimeMetadata); + assertServiceState({ + notebookSession: session, + notebookSessionForNotebookUri: session, + }, session.runtimeMetadata); } } function assertSingleSessionIsStarting(session: ILanguageRuntimeSession) { - assertSingleSession(session); + assertHasSingleSession(session); assert.equal(session.getRuntimeState(), RuntimeState.Starting); } function assertSingleSessionIsRestarting(session: ILanguageRuntimeSession) { - assertSingleSession(session); + assertHasSingleSession(session); assert.equal(session.getRuntimeState(), RuntimeState.Restarting); } - function assertSingleSessionIsStarted(session: ILanguageRuntimeSession) { - assertSingleSession(session); + function assertSingleSessionIsReady(session: ILanguageRuntimeSession) { + assertHasSingleSession(session); assert.equal(session.getRuntimeState(), RuntimeState.Ready); } @@ -480,6 +493,8 @@ suite('Positron - RuntimeSessionService', () => { assertServiceState({ hasStartingOrRunningConsole: true, consoleSession, + consoleSessionForLanguage: consoleSession, + consoleSessionForRuntime: consoleSession, notebookSession, notebookSessionForNotebookUri: notebookSession, activeSessions: [consoleSession, notebookSession], @@ -593,6 +608,8 @@ suite('Positron - RuntimeSessionService', () => { assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: session2, + consoleSessionForLanguage: session2, + consoleSessionForRuntime: session2, activeSessions: [session1, session2], }); }); @@ -649,7 +666,7 @@ suite('Positron - RuntimeSessionService', () => { assertSingleSessionIsRestarting(session); await waitForRuntimeState(session, RuntimeState.Ready); - assertSingleSessionIsStarted(session); + assertSingleSessionIsReady(session); }); } @@ -686,6 +703,8 @@ suite('Positron - RuntimeSessionService', () => { assertServiceState({ hasStartingOrRunningConsole: true, consoleSession: newSession, + consoleSessionForLanguage: newSession, + consoleSessionForRuntime: newSession, activeSessions: [session, newSession], }); } else { From 0558d3d7b778faa0babe53380d8dd93211ede338 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:25:31 +0200 Subject: [PATCH 28/45] return the correct session for selectRuntime test helper --- .../runtimeSession/test/common/runtimeSession.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index d868154cac2..fc1aa55eb8e 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -228,7 +228,13 @@ suite('Positron - RuntimeSessionService', () => { async function selectRuntime(runtimeMetadata = runtime) { await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); - const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); + // Get the last active session matching the runtime. + const sessionId = runtimeSessionService.activeSessions.reverse() + .find(session => session.runtimeMetadata.runtimeId === runtimeMetadata.runtimeId && + session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) + ?.sessionId; + assert.ok(sessionId); + const session = runtimeSessionService.getSession(sessionId); assert.ok(session instanceof TestLanguageRuntimeSession); disposables.add(session); return session; From 84c37aab770ca57192daa80ce27dcdb78a0ac906 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:27:12 +0200 Subject: [PATCH 29/45] make activeSessions check explicit --- .../runtimeSession/test/common/runtimeSession.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index fc1aa55eb8e..a3247a180a4 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -121,8 +121,7 @@ suite('Positron - RuntimeSessionService', () => { function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { // Check the active sessions. - assertActiveSessions(expectedState?.activeSessions ?? - [expectedState?.consoleSession, expectedState?.notebookSession].filter(session => !!session)); + assertActiveSessions(expectedState?.activeSessions ?? []); // Check the console session state. assert.equal( @@ -171,11 +170,13 @@ suite('Positron - RuntimeSessionService', () => { consoleSession: session, consoleSessionForLanguage: session, consoleSessionForRuntime: session, + activeSessions: [session], }, session.runtimeMetadata); } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { assertServiceState({ notebookSession: session, notebookSessionForNotebookUri: session, + activeSessions: [session], }, session.runtimeMetadata); } } From b67b6f9e1e08cdafadd81cb633b46a2a624ad0f8 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:38:23 +0200 Subject: [PATCH 30/45] test starting after an error was encountered previously --- .../test/common/runtimeSession.test.ts | 63 ++++++++++++++----- 1 file changed, 49 insertions(+), 14 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index a3247a180a4..b56f2bd7f61 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -380,17 +380,23 @@ suite('Positron - RuntimeSessionService', () => { const createOrRestoreMethod = action === 'restore' ? 'restoreSession' : 'createSession'; test(`${action} ${mode} encounters ${createOrRestoreMethod}() error`, async () => { const error = new Error('Failed to create session'); - sinon.stub(manager, createOrRestoreMethod).rejects(error); + const stub = sinon.stub(manager, createOrRestoreMethod).rejects(error); await assert.rejects(start(), error); + + // If we start now, without createOrRestoreMethod rejecting, it should work. + stub.restore(); + const session = await start(); + + assertSingleSessionIsStarting(session); }); - test(`${action} ${mode} fires onDidFailStartRuntime if session.start() errors`, async () => { + test(`${action} ${mode} encounters session.start() error`, async () => { // Listen to the onWillStartSession event and stub session.start() to throw an error. const willStartSession = sinon.spy((e: IRuntimeSessionWillStartEvent) => { sinon.stub(e.session, 'start').rejects(new Error('Session failed to start')); }); - disposables.add(runtimeSessionService.onWillStartSession(willStartSession)); + const willStartSessionDisposable = runtimeSessionService.onWillStartSession(willStartSession); const didFailStartRuntime = sinon.spy(); disposables.add(runtimeSessionService.onDidFailStartRuntime(didFailStartRuntime)); @@ -398,25 +404,54 @@ suite('Positron - RuntimeSessionService', () => { const didStartRuntime = sinon.spy(); disposables.add(runtimeSessionService.onDidStartRuntime(didStartRuntime)); - const session = await start(); + const session1 = await start(); - assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); + assert.equal(session1.getRuntimeState(), RuntimeState.Uninitialized); if (mode === LanguageRuntimeSessionMode.Console) { - // TODO: Seems unexpected that some of these are defined and others not. - assert.equal(runtimeSessionService.hasStartingOrRunningConsole(runtime.languageId), false); - assert.equal(runtimeSessionService.getConsoleSessionForLanguage(runtime.languageId), undefined); - assert.equal(runtimeSessionService.getConsoleSessionForRuntime(runtime.runtimeId), session); - assert.equal(runtimeSessionService.getSession(session.sessionId), session); - assert.deepEqual(runtimeSessionService.activeSessions, [session]); + assertServiceState({ + hasStartingOrRunningConsole: false, + // Note that getConsoleSessionForRuntime includes uninitialized sessions + // but getConsoleSessionForLanguage does not. + consoleSessionForLanguage: undefined, + consoleSessionForRuntime: session1, + activeSessions: [session1], + }); } else { - // TODO: Should failed sessions be included in activeSessions? - assertServiceState({ activeSessions: [session] }); + assertServiceState({ activeSessions: [session1] }); } - sinon.assert.calledOnceWithExactly(didFailStartRuntime, session); + sinon.assert.calledOnceWithExactly(didFailStartRuntime, session1); sinon.assert.callOrder(willStartSession, didFailStartRuntime); sinon.assert.notCalled(didStartRuntime); + + // If we start now, without session.start() rejecting, it should work. + willStartSessionDisposable.dispose(); + const session2 = await start(); + + assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + + const expectedActiveSessions = action === 'restore' ? + // Restoring a session twice overwrites the previous session in activeSessions. + [session2] : + // Other actions create a new session in activeSessions. + [session1, session2]; + + if (mode === LanguageRuntimeSessionMode.Console) { + assertServiceState({ + hasStartingOrRunningConsole: true, + consoleSession: session2, + consoleSessionForLanguage: session2, + consoleSessionForRuntime: session2, + activeSessions: expectedActiveSessions, + }); + } else { + assertServiceState({ + notebookSession: session2, + notebookSessionForNotebookUri: session2, + activeSessions: expectedActiveSessions, + }); + } }); test(`${action} ${mode} throws if another runtime is starting for the language`, async () => { From 4e7efaf69ff693496863e0b2bf9506403f789518 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:39:21 +0200 Subject: [PATCH 31/45] test actual objects since there could be multiple sessions with the same ID in the case of restores --- .../test/common/runtimeSession.test.ts | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index b56f2bd7f61..e369671c015 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -112,16 +112,9 @@ suite('Positron - RuntimeSessionService', () => { activeSessions?: ILanguageRuntimeSession[]; } - function assertActiveSessions(expectedSessions: ILanguageRuntimeSession[]) { - assert.deepEqual( - runtimeSessionService.activeSessions.map(session => session.sessionId), - expectedSessions.map(session => session.sessionId), - ); - } - function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { // Check the active sessions. - assertActiveSessions(expectedState?.activeSessions ?? []); + assert.deepEqual(runtimeSessionService.activeSessions, expectedState?.activeSessions ?? []); // Check the console session state. assert.equal( @@ -132,26 +125,26 @@ suite('Positron - RuntimeSessionService', () => { 'Expected no starting or running console session', ); assert.equal( - runtimeSessionService.getConsoleSessionForLanguage(runtimeMetadata.languageId)?.sessionId, - expectedState?.consoleSessionForLanguage?.sessionId, + runtimeSessionService.getConsoleSessionForLanguage(runtimeMetadata.languageId), + expectedState?.consoleSessionForLanguage, ); assert.equal( - runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId)?.sessionId, - expectedState?.consoleSessionForRuntime?.sessionId, + runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId), + expectedState?.consoleSessionForRuntime, ); assert.equal( - runtimeSessionService.getSession(expectedState?.consoleSession?.sessionId ?? '')?.sessionId, - expectedState?.consoleSession?.sessionId, + runtimeSessionService.getSession(expectedState?.consoleSession?.sessionId ?? ''), + expectedState?.consoleSession, ); // Check the notebook session state. assert.equal( - runtimeSessionService.getSession(expectedState?.notebookSession?.sessionId ?? '')?.sessionId, - expectedState?.notebookSession?.sessionId, + runtimeSessionService.getSession(expectedState?.notebookSession?.sessionId ?? ''), + expectedState?.notebookSession, ); assert.equal( - runtimeSessionService.getNotebookSessionForNotebookUri(notebookUri)?.sessionId, - expectedState?.notebookSessionForNotebookUri?.sessionId, + runtimeSessionService.getNotebookSessionForNotebookUri(notebookUri), + expectedState?.notebookSessionForNotebookUri, ); } From 9acb4b09c55b727dc7a3e3d19b7dd9b63fedb2f6 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:40:04 +0200 Subject: [PATCH 32/45] remove unneeded roundtrip --- .../runtimeSession/test/common/runtimeSession.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index e369671c015..71c59f8d3b9 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -223,12 +223,9 @@ suite('Positron - RuntimeSessionService', () => { async function selectRuntime(runtimeMetadata = runtime) { await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); // Get the last active session matching the runtime. - const sessionId = runtimeSessionService.activeSessions.reverse() + const session = runtimeSessionService.activeSessions.reverse() .find(session => session.runtimeMetadata.runtimeId === runtimeMetadata.runtimeId && - session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) - ?.sessionId; - assert.ok(sessionId); - const session = runtimeSessionService.getSession(sessionId); + session.metadata.sessionMode === LanguageRuntimeSessionMode.Console); assert.ok(session instanceof TestLanguageRuntimeSession); disposables.add(session); return session; From 801d0d7652405461c6420bcea2f23893af210916 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:41:29 +0200 Subject: [PATCH 33/45] we can actually use `getConsoleSessionForRuntime` after the fix --- .../runtimeSession/test/common/runtimeSession.test.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 71c59f8d3b9..efd681c2fa0 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -222,10 +222,7 @@ suite('Positron - RuntimeSessionService', () => { async function selectRuntime(runtimeMetadata = runtime) { await runtimeSessionService.selectRuntime(runtimeMetadata.runtimeId, startReason); - // Get the last active session matching the runtime. - const session = runtimeSessionService.activeSessions.reverse() - .find(session => session.runtimeMetadata.runtimeId === runtimeMetadata.runtimeId && - session.metadata.sessionMode === LanguageRuntimeSessionMode.Console); + const session = runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId); assert.ok(session instanceof TestLanguageRuntimeSession); disposables.add(session); return session; From 275b6119279b74f04f89f6e8202db1f4ee81cbc9 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:43:03 +0200 Subject: [PATCH 34/45] fix: return the latest matching console in `getConsoleSessionForRuntime` --- .../services/runtimeSession/common/runtimeSession.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 4c3c17c9082..cd5b9da05cb 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -264,7 +264,10 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession * no console session with the given runtime identifier exists. */ getConsoleSessionForRuntime(runtimeId: string): ILanguageRuntimeSession | undefined { - const session = Array.from(this._activeSessionsBySessionId.values()).find(session => + // It's possible that there are multiple consoles for the same runtime, + // for example, if one failed to start and is uninitialized. In that case, + // we return the last. + const session = Array.from(this._activeSessionsBySessionId.values()).reverse().find(session => session.session.runtimeMetadata.runtimeId === runtimeId && session.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && session.state !== RuntimeState.Exited); From c9077df0910fc11a456cfaf54ac6a254c4f0b902 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 15:56:27 +0200 Subject: [PATCH 35/45] fix: notebook sessions not being set/cleared from starting starting session maps --- .../runtimeSession/common/runtimeSession.ts | 56 +++++++++++-------- .../test/common/runtimeSession.test.ts | 1 - 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index cd5b9da05cb..1778a35b1fa 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -496,7 +496,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // at the debug level since we still expect the error to be handled/logged elsewhere. startPromise.p.catch((err) => this._logService.debug(`Error starting session: ${err}`)); - this.updateSessionMapsBeforeStart( + this.setStartingSessionMaps( sessionMetadata.sessionMode, runtimeMetadata, sessionMetadata.notebookUri); // We should already have a session manager registered, since we can't @@ -517,8 +517,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession `Reconnecting to session '${sessionMetadata.sessionId}' for language runtime ` + `${formatLanguageRuntimeMetadata(runtimeMetadata)} failed. Reason: ${err}`); startPromise.error(err); - this._startingSessionsBySessionMapKey.delete(sessionMapKey); - this._startingConsolesByLanguageId.delete(runtimeMetadata.languageId); + this.clearStartingSessionMaps( + sessionMetadata.sessionMode, runtimeMetadata, sessionMetadata.notebookUri); throw err; } @@ -819,7 +819,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession sessionMode: LanguageRuntimeSessionMode, source: string, notebookUri?: URI): Promise { - this.updateSessionMapsBeforeStart(sessionMode, runtimeMetadata, notebookUri); + this.setStartingSessionMaps(sessionMode, runtimeMetadata, notebookUri); // Create a promise that resolves when the runtime is ready to use, if there isn't already one. const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); @@ -853,11 +853,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession `Creating session for language runtime ` + `${formatLanguageRuntimeMetadata(runtimeMetadata)} failed. Reason: ${err}`); startPromise.error(err); - this._startingSessionsBySessionMapKey.delete(sessionMapKey); - this._startingConsolesByLanguageId.delete(runtimeMetadata.languageId); - if (notebookUri) { - this._startingNotebooksByNotebookUri.delete(notebookUri); - } + this.clearStartingSessionMaps(sessionMode, runtimeMetadata, notebookUri); // Re-throw the error. throw err; @@ -896,21 +892,18 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // Attach event handlers to the newly provisioned session. this.attachToSession(session, manager); - const sessionMapKey = getSessionMapKey( - session.metadata.sessionMode, session.runtimeMetadata.runtimeId, session.metadata.notebookUri); try { // Attempt to start, or reconnect to, the session. await session.start(); // The runtime started. Move it from the starting runtimes to the // running runtimes. - this._startingSessionsBySessionMapKey.delete(sessionMapKey); + this.clearStartingSessionMaps( + session.metadata.sessionMode, session.runtimeMetadata, session.metadata.notebookUri); if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console) { - this._startingConsolesByLanguageId.delete(session.runtimeMetadata.languageId); this._consoleSessionsByLanguageId.set(session.runtimeMetadata.languageId, session); } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook) { if (session.metadata.notebookUri) { - this._startingNotebooksByNotebookUri.delete(session.metadata.notebookUri); this._logService.info(`Notebook session for ${session.metadata.notebookUri} started: ${session.metadata.sessionId}`); this._notebookSessionsByNotebookUri.set(session.metadata.notebookUri, session); } else { @@ -927,10 +920,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession this._foregroundSession = session; } } catch (reason) { - - // Remove the runtime from the starting runtimes. - this._startingConsolesByLanguageId.delete(session.runtimeMetadata.languageId); - this._startingSessionsBySessionMapKey.delete(sessionMapKey); + this.clearStartingSessionMaps( + session.metadata.sessionMode, session.runtimeMetadata, session.metadata.notebookUri); // Fire the onDidFailStartRuntime event. this._onDidFailStartRuntimeEmitter.fire(session); @@ -1183,14 +1174,13 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession } /** - * Updates the session maps (for starting consoles, notebooks, etc.), before a - * session starts. + * Sets the session maps for a starting session. * * @param sessionMode The mode of the session. * @param runtimeMetadata The metadata of the session's runtime. * @param notebookUri The notebook URI attached to the session, if any. */ - private updateSessionMapsBeforeStart( + private setStartingSessionMaps( sessionMode: LanguageRuntimeSessionMode, runtimeMetadata: ILanguageRuntimeMetadata, notebookUri?: URI) { @@ -1201,6 +1191,26 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession } } + /** + * Clears the session maps for a starting session. + * + * @param sessionMode The mode of the session. + * @param runtimeMetadata The metadata of the session's runtime. + * @param notebookUri The notebook URI attached to the session, if any. + */ + private clearStartingSessionMaps( + sessionMode: LanguageRuntimeSessionMode, + runtimeMetadata: ILanguageRuntimeMetadata, + notebookUri?: URI) { + const sessionMapKey = getSessionMapKey(sessionMode, runtimeMetadata.runtimeId, notebookUri); + this._startingSessionsBySessionMapKey.delete(sessionMapKey); + if (sessionMode === LanguageRuntimeSessionMode.Console) { + this._startingConsolesByLanguageId.delete(runtimeMetadata.languageId); + } else if (sessionMode === LanguageRuntimeSessionMode.Notebook && notebookUri) { + this._startingNotebooksByNotebookUri.delete(notebookUri); + } + } + /** * Updates the session maps (for active consoles, notebooks, etc.), after a * session exits. @@ -1238,10 +1248,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession state === RuntimeState.Ready) { // The runtime looks like it could handle a restart request, so send // one over. - - this.updateSessionMapsBeforeStart( + this.setStartingSessionMaps( session.metadata.sessionMode, session.runtimeMetadata, session.metadata.notebookUri); - return session.restart(); } else if (state === RuntimeState.Uninitialized || state === RuntimeState.Exited) { diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index efd681c2fa0..85469576a14 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -211,7 +211,6 @@ suite('Positron - RuntimeSessionService', () => { } async function autoStartSession(runtimeMetadata = runtime) { - // TODO: Maybe all of these functions can return a sessionId? const sessionId = await runtimeSessionService.autoStartRuntime(runtimeMetadata, startReason); assert.ok(sessionId); const session = runtimeSessionService.getSession(sessionId); From 7b84fda3d7a793ed81fdec6c182be89e419d2156 Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 16:14:31 +0200 Subject: [PATCH 36/45] re-add restore fires onWillStartSession test --- .../test/common/runtimeSession.test.ts | 71 ++----------------- 1 file changed, 6 insertions(+), 65 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 85469576a14..b28fce08458 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -272,18 +272,13 @@ suite('Positron - RuntimeSessionService', () => { assertSingleSessionIsStarting(session); }); + // TODO: Should onWillStartSession only fire once? + // It currently fires twice. Before the session is started and when the session + // enters the ready state. test(`${action} ${mode} fires onWillStartSession`, async () => { - if (action === 'restore') { - // TODO: I'm not sure if we should be emitting a 'starting' runtime state event - // when reconnecting to a TestLanguageRuntimeSession. That's firing another - // event with isNew = true. - return; - } - let error: Error | undefined; const target = sinon.spy(({ session }: IRuntimeSessionWillStartEvent) => { try { - // TODO: Should onWillStartSession only fire once? if (target.callCount > 1) { return; } @@ -297,10 +292,10 @@ suite('Positron - RuntimeSessionService', () => { disposables.add(runtimeSessionService.onWillStartSession(target)); const session = await start(); - // TODO: Should onWillStartSession only fire once? sinon.assert.calledTwice(target); - const isNew = action !== 'restore'; - sinon.assert.alwaysCalledWithExactly(target, { isNew, session }); + // When restoring a session, the first event is fired with isNew: false. + sinon.assert.calledWith(target.getCall(0), { isNew: action !== 'restore', session }); + sinon.assert.calledWith(target.getCall(1), { isNew: true, session }); assert.ifError(error); }); @@ -804,58 +799,4 @@ suite('Positron - RuntimeSessionService', () => { sinon.assert.calledThrice(target); }); } - - suite('queueing', () => { - // test(`start console -> select console`, async () => { - // const [session,] = await Promise.all([ - // startConsole(), - // // Selecting the same runtime should do nothing. - // selectRuntime(), - // ]); - - // assertSessionIsStarting(session); - // }); - - // test(`select console -> start console`, async () => { - // const [session,] = await Promise.all([ - // selectRuntime(), - // // Starting the same runtime should do nothing. - // startConsole(), - // ]); - - // assertSessionIsStarting(session); - // }); - - // test(`select another runtime for console -> start console`, async () => { - // const session = await startConsole(); - - // const [session2,] = await Promise.all([ - // selectRuntime(anotherRuntime), - // // Starting the same runtime should do nothing. - // startConsole(anotherRuntime), - // ]); - - // assertSessionIsStarting(session2); - // }); - - test.skip(`restart console -> start console when exited`, async () => { - const session = await startConsole(); - await waitForRuntimeState(session, RuntimeState.Ready); - - // Send another start request once the session exits. - const startPromise = new Promise(resolve => { - disposables.add(session.onDidChangeRuntimeState(state => { - if (state === RuntimeState.Exited) { - resolve(startConsole()); - } - })); - }); - - restartSession(session.sessionId); - - await startPromise; - - assertSingleSessionIsStarting(session); - }); - }); }); From f9bee543efb5a286958c42099cf932019cfd6cfa Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 16:53:47 +0200 Subject: [PATCH 37/45] fix import across environment types --- .../positronPreview/browser/positronPreview.contribution.ts | 3 +-- .../services/languageRuntime/common/languageRuntimeUiClient.ts | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/positronPreview/browser/positronPreview.contribution.ts b/src/vs/workbench/contrib/positronPreview/browser/positronPreview.contribution.ts index 3de51e015b1..d24db8db586 100644 --- a/src/vs/workbench/contrib/positronPreview/browser/positronPreview.contribution.ts +++ b/src/vs/workbench/contrib/positronPreview/browser/positronPreview.contribution.ts @@ -20,12 +20,11 @@ import { ViewContainer, IViewContainersRegistry, ViewContainerLocation, Extensio import { registerAction2 } from 'vs/platform/actions/common/actions'; import { PositronOpenUrlInViewerAction } from 'vs/workbench/contrib/positronPreview/browser/positronPreviewActions'; import { IConfigurationRegistry, Extensions as ConfigurationExtensions, ConfigurationScope, } from 'vs/platform/configuration/common/configurationRegistry'; +import { POSITRON_PREVIEW_PLOTS_IN_VIEWER } from 'vs/workbench/services/languageRuntime/common/languageRuntimeUiClient'; // The Positron preview view icon. const positronPreviewViewIcon = registerIcon('positron-preview-view-icon', Codicon.positronPreviewView, nls.localize('positronPreviewViewIcon', 'View icon of the Positron preview view.')); -export const POSITRON_PREVIEW_PLOTS_IN_VIEWER = 'positron.viewer.interactivePlotsInViewer'; - // Register the Positron preview container. const VIEW_CONTAINER: ViewContainer = Registry.as(ViewContainerExtensions.ViewContainersRegistry).registerViewContainer({ id: POSITRON_PREVIEW_VIEW_ID, diff --git a/src/vs/workbench/services/languageRuntime/common/languageRuntimeUiClient.ts b/src/vs/workbench/services/languageRuntime/common/languageRuntimeUiClient.ts index d9fd4ca94a6..83ec0da31ce 100644 --- a/src/vs/workbench/services/languageRuntime/common/languageRuntimeUiClient.ts +++ b/src/vs/workbench/services/languageRuntime/common/languageRuntimeUiClient.ts @@ -13,8 +13,8 @@ import { URI } from 'vs/base/common/uri'; import { ICommandService } from 'vs/platform/commands/common/commands'; import { ILogService } from 'vs/platform/log/common/log'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; -import { POSITRON_PREVIEW_PLOTS_IN_VIEWER } from 'vs/workbench/contrib/positronPreview/browser/positronPreview.contribution'; +export const POSITRON_PREVIEW_PLOTS_IN_VIEWER = 'positron.viewer.interactivePlotsInViewer'; /** * The types of messages that can be sent to the backend. From 688322d2289e328586bf3de0e40a02270e56a4fa Mon Sep 17 00:00:00 2001 From: seem Date: Fri, 22 Nov 2024 16:59:18 +0200 Subject: [PATCH 38/45] fix leaked disposable --- .../test/common/testLanguageRuntimeSession.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts b/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts index e7d07654603..1412e5eb4fd 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession.ts @@ -163,7 +163,14 @@ export class TestLanguageRuntimeSession extends Disposable implements ILanguageR async restart(): Promise { await this.shutdown(RuntimeExitReason.Restart); - waitForRuntimeState(this, RuntimeState.Exited).then(() => this.start()); + + // Wait for the session to exit, then start it again. + const disposable = this._register(this.onDidChangeRuntimeState(state => { + if (state === RuntimeState.Exited) { + disposable.dispose(); + this.start(); + } + })); } async shutdown(exitReason: RuntimeExitReason): Promise { @@ -204,10 +211,6 @@ export class TestLanguageRuntimeSession extends Disposable implements ILanguageR throw new Error('Not implemented.'); } - override dispose() { - super.dispose(); - } - // Test helpers setRuntimeState(state: RuntimeState) { From 2d5092a17058e4c5eb9114ae403fe519edf5f848 Mon Sep 17 00:00:00 2001 From: seem Date: Sat, 23 Nov 2024 15:07:49 +0200 Subject: [PATCH 39/45] stop notebook runtime error messages from showing up in the console --- .../src/notebookController.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/extensions/positron-notebook-controllers/src/notebookController.ts b/extensions/positron-notebook-controllers/src/notebookController.ts index aaac1770755..04af82b2549 100644 --- a/extensions/positron-notebook-controllers/src/notebookController.ts +++ b/extensions/positron-notebook-controllers/src/notebookController.ts @@ -331,10 +331,19 @@ function executeCode( } } }).catch(error => { + // Stop listening for replies. handler.dispose(); + + // Reject the outer execution promise since we've encountered an error. reject(error); + + // Rethrow the error to stop any replies that are chained to this promise. throw error; }); + + // Avoid unhandled rejections being logged to the console. + // The actual error-handling is in the catch block above. + currentMessagePromise.catch(() => { }); }); // Execute the cell. From 586b7e1a4511cfc920893dd64e0f28c3b0f1ccf2 Mon Sep 17 00:00:00 2001 From: seem Date: Mon, 25 Nov 2024 17:24:04 +0200 Subject: [PATCH 40/45] sort by createdTimestamp instead of insert order --- .../services/runtimeSession/common/runtimeSession.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 1778a35b1fa..3c03a970259 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -266,11 +266,13 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession getConsoleSessionForRuntime(runtimeId: string): ILanguageRuntimeSession | undefined { // It's possible that there are multiple consoles for the same runtime, // for example, if one failed to start and is uninitialized. In that case, - // we return the last. - const session = Array.from(this._activeSessionsBySessionId.values()).reverse().find(session => - session.session.runtimeMetadata.runtimeId === runtimeId && - session.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && - session.state !== RuntimeState.Exited); + // we return the most recently created. + const session = Array.from(this._activeSessionsBySessionId.values()) + .sort((a, b) => b.session.metadata.createdTimestamp - a.session.metadata.createdTimestamp) + .find(session => + session.session.runtimeMetadata.runtimeId === runtimeId && + session.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && + session.state !== RuntimeState.Exited); if (session) { return session.session; } else { From 0259c3dba8ffbf2b2797b8f45dec0eca73fc0867 Mon Sep 17 00:00:00 2001 From: seem Date: Tue, 26 Nov 2024 14:21:05 +0200 Subject: [PATCH 41/45] fix multiple sessions having the same `createdTimestamp` --- .../runtimeSession/common/runtimeSession.ts | 32 +++++-------- .../test/common/runtimeSession.test.ts | 45 +++++++++---------- 2 files changed, 32 insertions(+), 45 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 3c03a970259..95992903748 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -267,17 +267,17 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // It's possible that there are multiple consoles for the same runtime, // for example, if one failed to start and is uninitialized. In that case, // we return the most recently created. - const session = Array.from(this._activeSessionsBySessionId.values()) - .sort((a, b) => b.session.metadata.createdTimestamp - a.session.metadata.createdTimestamp) - .find(session => - session.session.runtimeMetadata.runtimeId === runtimeId && - session.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && - session.state !== RuntimeState.Exited); - if (session) { - return session.session; - } else { - return undefined; - } + return Array.from(this._activeSessionsBySessionId.values()) + .map((info, index) => ({ info, index })) + .sort((a, b) => + b.info.session.metadata.createdTimestamp - a.info.session.metadata.createdTimestamp + // If the timestamps are the same, prefer the session that was inserted last. + || b.index - a.index) + .find(({ info }) => + info.session.runtimeMetadata.runtimeId === runtimeId && + info.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && + info.state !== RuntimeState.Exited) + ?.info.session; } /** @@ -1006,16 +1006,6 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession this._notebookSessionsByNotebookUri.set(session.metadata.notebookUri, session); } - // Remove the session from the starting runtimes. - if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console && - this._startingConsolesByLanguageId.get(session.runtimeMetadata.languageId) === session.runtimeMetadata) { - this._startingConsolesByLanguageId.delete(session.runtimeMetadata.languageId); - } else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook && - session.metadata.notebookUri && - this._startingNotebooksByNotebookUri.get(session.metadata.notebookUri) === session.runtimeMetadata) { - this._startingNotebooksByNotebookUri.delete(session.metadata.notebookUri); - } - // Start the UI client instance once the runtime is fully online. this.startUiClient(session); break; diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index b28fce08458..da0c5880cfe 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -35,8 +35,6 @@ suite('Positron - RuntimeSessionService', () => { let anotherRuntime: ILanguageRuntimeMetadata; let sessionName: string; let unregisteredRuntime: ILanguageRuntimeMetadata; - let consoleSessionMetadata: IRuntimeSessionMetadata; - let notebookSessionMetadata: IRuntimeSessionMetadata; setup(() => { instantiationService = disposables.add(new TestInstantiationService()); @@ -49,25 +47,8 @@ suite('Positron - RuntimeSessionService', () => { runtime = createTestLanguageRuntimeMetadata(instantiationService, disposables); anotherRuntime = createTestLanguageRuntimeMetadata(instantiationService, disposables); - unregisteredRuntime = { runtimeId: 'unregistered-runtime-id' } as ILanguageRuntimeMetadata; - sessionName = runtime.runtimeName; - consoleSessionMetadata = { - sessionId: 'test-console-session-id', - sessionName, - sessionMode: LanguageRuntimeSessionMode.Console, - createdTimestamp: Date.now(), - notebookUri: undefined, - startReason, - }; - notebookSessionMetadata = { - sessionId: 'test-notebook-session-id', - sessionName, - sessionMode: LanguageRuntimeSessionMode.Notebook, - createdTimestamp: Date.now(), - notebookUri, - startReason, - }; + unregisteredRuntime = { runtimeId: 'unregistered-runtime-id' } as ILanguageRuntimeMetadata; // Enable automatic startup. configService.setUserConfiguration('positron.interpreters.automaticStartup', true); @@ -202,12 +183,28 @@ suite('Positron - RuntimeSessionService', () => { return session; } - function restoreConsole(runtimeMetadata?: ILanguageRuntimeMetadata) { - return restoreSession(consoleSessionMetadata, runtimeMetadata); + function restoreConsole(runtimeMetadata = runtime) { + const sessionMetadata: IRuntimeSessionMetadata = { + sessionId: 'test-console-session-id', + sessionName, + sessionMode: LanguageRuntimeSessionMode.Console, + createdTimestamp: Date.now(), + notebookUri: undefined, + startReason, + }; + return restoreSession(sessionMetadata, runtimeMetadata); } - function restoreNotebook(runtimeMetadata?: ILanguageRuntimeMetadata) { - return restoreSession(notebookSessionMetadata, runtimeMetadata); + function restoreNotebook(runtimeMetadata = runtime) { + const sessionMetadata: IRuntimeSessionMetadata = { + sessionId: 'test-notebook-session-id', + sessionName, + sessionMode: LanguageRuntimeSessionMode.Notebook, + createdTimestamp: Date.now(), + notebookUri, + startReason, + }; + return restoreSession(sessionMetadata, runtimeMetadata); } async function autoStartSession(runtimeMetadata = runtime) { From 0e3cf4f4b9f1263a0cbd56ac36afc3a1251be3a3 Mon Sep 17 00:00:00 2001 From: seem Date: Tue, 26 Nov 2024 19:24:48 +0200 Subject: [PATCH 42/45] fix restart while ready not clearing the starting maps --- .../services/runtimeSession/common/runtimeSession.ts | 11 +++++++++++ .../test/common/runtimeSession.test.ts | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 95992903748..9fed7c92f46 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -1240,8 +1240,19 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession state === RuntimeState.Ready) { // The runtime looks like it could handle a restart request, so send // one over. + + // Mark the session as starting until the restart sequence completes. this.setStartingSessionMaps( session.metadata.sessionMode, session.runtimeMetadata, session.metadata.notebookUri); + const disposable = this._register(session.onDidChangeRuntimeState((state) => { + if (state === RuntimeState.Ready) { + disposable.dispose(); + this.clearStartingSessionMaps( + session.metadata.sessionMode, session.runtimeMetadata, session.metadata.notebookUri); + } + })); + + // Restart the session. return session.restart(); } else if (state === RuntimeState.Uninitialized || state === RuntimeState.Exited) { diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index da0c5880cfe..482473a2077 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -795,5 +795,17 @@ suite('Positron - RuntimeSessionService', () => { sinon.assert.calledThrice(target); }); + + test(`restart ${mode} while ready -> start`, async () => { + const session = await start(); + await waitForRuntimeState(session, RuntimeState.Ready); + + await restartSession(session.sessionId); + await waitForRuntimeState(session, RuntimeState.Ready); + + const newSession = await start(); + + assertSingleSessionIsReady(newSession); + }); } }); From 065e6e7cb455c8fd7aeb807693773ca5217e8999 Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 28 Nov 2024 12:40:53 +0200 Subject: [PATCH 43/45] add `onDidEndSession` disposable to the session's disposables --- .../services/runtimeSession/common/runtimeSession.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 9fed7c92f46..2af31f2a661 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -369,14 +369,20 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession private async doShutdownRuntimeSession( session: ILanguageRuntimeSession, exitReason: RuntimeExitReason): Promise { + + const sessionDisposables = this._sessionDisposables.get(session.sessionId); + if (!sessionDisposables) { + throw new Error(`No disposables found for session ${session.sessionId}`); + } + // We wait for `onDidEndSession()` rather than `RuntimeState.Exited`, because the former // generates some Console output that must finish before starting up a new runtime: let disposable: IDisposable | undefined; const promise = new Promise(resolve => { - disposable = session.onDidEndSession((exit) => { + disposable = sessionDisposables.add(session.onDidEndSession((exit) => { resolve(); disposable?.dispose(); - }); + })); }); const timeout = new Promise((_, reject) => { From 811cc2f81046a01f67e5f4c02fcded93a6e6588e Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 28 Nov 2024 13:53:31 +0200 Subject: [PATCH 44/45] fix uncaught error --- .../runtimeSession/common/runtimeSession.ts | 27 +++++++--------- .../test/common/runtimeSession.test.ts | 31 ++++++++++++++++++- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts index 2af31f2a661..ec9d2016eab 100644 --- a/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts +++ b/src/vs/workbench/services/runtimeSession/common/runtimeSession.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as nls from 'vs/nls'; -import { DeferredPromise } from 'vs/base/common/async'; +import { DeferredPromise, disposableTimeout } from 'vs/base/common/async'; import { Emitter } from 'vs/base/common/event'; import { Disposable, DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; @@ -377,33 +377,30 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession // We wait for `onDidEndSession()` rather than `RuntimeState.Exited`, because the former // generates some Console output that must finish before starting up a new runtime: - let disposable: IDisposable | undefined; - const promise = new Promise(resolve => { - disposable = sessionDisposables.add(session.onDidEndSession((exit) => { + const disposables = sessionDisposables.add(new DisposableStore()); + const promise = new Promise((resolve, reject) => { + disposables.add(session.onDidEndSession((exit) => { + disposables.dispose(); resolve(); - disposable?.dispose(); })); - }); - - const timeout = new Promise((_, reject) => { - setTimeout(() => { - disposable?.dispose(); + disposables.add(disposableTimeout(() => { + disposables.dispose(); reject(new Error(`Timed out waiting for runtime ` + `${formatLanguageRuntimeSession(session)} to finish exiting.`)); - }, 5000); + }, 5000)); }); // Ask the runtime to shut down. try { await session.shutdown(exitReason); - } catch (err) { - disposable?.dispose(); - throw err; + } catch (error) { + disposables.dispose(); + throw error; } // Wait for the runtime onDidEndSession to resolve, or for the timeout to expire // (whichever comes first) - await Promise.race([promise, timeout]); + await promise; } /** diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 482473a2077..85aa3b003ee 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -12,7 +12,7 @@ import { IConfigurationService } from 'vs/platform/configuration/common/configur import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService'; import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock'; import { IWorkspaceTrustManagementService } from 'vs/platform/workspace/common/workspaceTrust'; -import { formatLanguageRuntimeMetadata, ILanguageRuntimeMetadata, ILanguageRuntimeService, LanguageRuntimeSessionMode, RuntimeExitReason, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; +import { formatLanguageRuntimeMetadata, formatLanguageRuntimeSession, ILanguageRuntimeMetadata, ILanguageRuntimeService, LanguageRuntimeSessionMode, RuntimeExitReason, RuntimeState } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; import { ILanguageRuntimeSession, IRuntimeSessionMetadata, IRuntimeSessionService, IRuntimeSessionWillStartEvent } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; import { TestLanguageRuntimeSession, waitForRuntimeState } from 'vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession'; import { createRuntimeServices, createTestLanguageRuntimeMetadata, startTestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService'; @@ -653,6 +653,35 @@ suite('Positron - RuntimeSessionService', () => { assert.equal(runtimeSessionService.foregroundSession, session1); }); + test(`select console to another runtime and first session never fires onDidEndSession`, async () => { + const session = await startConsole(); + await waitForRuntimeState(session, RuntimeState.Ready); + + // Stub onDidEndSession to never fire, causing the shutdown to time out. + sinon.stub(session, 'onDidEndSession').returns({ dispose: () => { } }); + + // Use a fake timer to avoid actually having to wait for the timeout. + const clock = sinon.useFakeTimers(); + const promise = assert.rejects(selectRuntime(anotherRuntime), new Error(`Timed out waiting for runtime ` + + `${formatLanguageRuntimeSession(session)} to finish exiting.`)); + await clock.tickAsync(10_000); + await promise; + }); + + test(`select console to another runtime encounters session.shutdown() error`, async () => { + const session = await startConsole(); + + // Stub session.shutdown() to throw an error. + const error = new Error('Session failed to shut down'); + sinon.stub(session, 'shutdown').rejects(error); + + // We also want to ensure that the timeout is not hit in this case but don't want to + // actually wait, so we use a fake timer. + const clock = sinon.useFakeTimers(); + await assert.rejects(selectRuntime(anotherRuntime), error); + await clock.tickAsync(10_000); + }); + function restartSession(sessionId: string) { return runtimeSessionService.restartSession(sessionId, startReason); } From 25cea7f56fcac0e0c346206c8fd040658f2026fc Mon Sep 17 00:00:00 2001 From: seem Date: Thu, 28 Nov 2024 14:50:25 +0200 Subject: [PATCH 45/45] dont use assert.strict since it doesnt seem to work in the browser environment --- .../test/common/runtimeSession.test.ts | 100 +++++++++--------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts index 85aa3b003ee..ea178cedeea 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts @@ -3,7 +3,7 @@ * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. *--------------------------------------------------------------------------------------------*/ -import { strict as assert } from 'assert'; +import * as assert from 'assert'; import * as sinon from 'sinon'; import { Event } from 'vs/base/common/event'; import { URI } from 'vs/base/common/uri'; @@ -95,35 +95,35 @@ suite('Positron - RuntimeSessionService', () => { function assertServiceState(expectedState?: IServiceState, runtimeMetadata = runtime): void { // Check the active sessions. - assert.deepEqual(runtimeSessionService.activeSessions, expectedState?.activeSessions ?? []); + assert.deepStrictEqual(runtimeSessionService.activeSessions, expectedState?.activeSessions ?? []); // Check the console session state. - assert.equal( + assert.strictEqual( runtimeSessionService.hasStartingOrRunningConsole(runtimeMetadata.languageId), expectedState?.hasStartingOrRunningConsole ?? false, expectedState?.hasStartingOrRunningConsole ? 'Expected a starting or running console session' : 'Expected no starting or running console session', ); - assert.equal( + assert.strictEqual( runtimeSessionService.getConsoleSessionForLanguage(runtimeMetadata.languageId), expectedState?.consoleSessionForLanguage, ); - assert.equal( + assert.strictEqual( runtimeSessionService.getConsoleSessionForRuntime(runtimeMetadata.runtimeId), expectedState?.consoleSessionForRuntime, ); - assert.equal( + assert.strictEqual( runtimeSessionService.getSession(expectedState?.consoleSession?.sessionId ?? ''), expectedState?.consoleSession, ); // Check the notebook session state. - assert.equal( + assert.strictEqual( runtimeSessionService.getSession(expectedState?.notebookSession?.sessionId ?? ''), expectedState?.notebookSession, ); - assert.equal( + assert.strictEqual( runtimeSessionService.getNotebookSessionForNotebookUri(notebookUri), expectedState?.notebookSessionForNotebookUri, ); @@ -157,17 +157,17 @@ suite('Positron - RuntimeSessionService', () => { function assertSingleSessionIsStarting(session: ILanguageRuntimeSession) { assertHasSingleSession(session); - assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Starting); } function assertSingleSessionIsRestarting(session: ILanguageRuntimeSession) { assertHasSingleSession(session); - assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Restarting); } function assertSingleSessionIsReady(session: ILanguageRuntimeSession) { assertHasSingleSession(session); - assert.equal(session.getRuntimeState(), RuntimeState.Ready); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Ready); } async function restoreSession( @@ -241,16 +241,16 @@ suite('Positron - RuntimeSessionService', () => { test(`${action} ${mode} returns the expected session`, async () => { const session = await start(); - assert.equal(session.getRuntimeState(), RuntimeState.Starting); - assert.equal(session.metadata.sessionName, sessionName); - assert.equal(session.metadata.sessionMode, mode); - assert.equal(session.metadata.startReason, startReason); - assert.equal(session.runtimeMetadata, runtime); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(session.metadata.sessionName, sessionName); + assert.strictEqual(session.metadata.sessionMode, mode); + assert.strictEqual(session.metadata.startReason, startReason); + assert.strictEqual(session.runtimeMetadata, runtime); if (mode === LanguageRuntimeSessionMode.Console) { - assert.equal(session.metadata.notebookUri, undefined); + assert.strictEqual(session.metadata.notebookUri, undefined); } else { - assert.equal(session.metadata.notebookUri, notebookUri); + assert.strictEqual(session.metadata.notebookUri, notebookUri); } }); @@ -279,7 +279,7 @@ suite('Positron - RuntimeSessionService', () => { if (target.callCount > 1) { return; } - assert.equal(session.getRuntimeState(), RuntimeState.Uninitialized); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Uninitialized); assertSingleSessionWillStart(mode); } catch (e) { @@ -300,7 +300,7 @@ suite('Positron - RuntimeSessionService', () => { let error: Error | undefined; const target = sinon.stub<[e: ILanguageRuntimeSession]>().callsFake(session => { try { - assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Starting); assertSingleSessionIsStarting(session); } catch (e) { @@ -334,7 +334,7 @@ suite('Positron - RuntimeSessionService', () => { const session = await start(); - assert.equal(runtimeSessionService.foregroundSession, session); + assert.strictEqual(runtimeSessionService.foregroundSession, session); await waitForRuntimeState(session, RuntimeState.Ready); @@ -384,7 +384,7 @@ suite('Positron - RuntimeSessionService', () => { const session1 = await start(); - assert.equal(session1.getRuntimeState(), RuntimeState.Uninitialized); + assert.strictEqual(session1.getRuntimeState(), RuntimeState.Uninitialized); if (mode === LanguageRuntimeSessionMode.Console) { assertServiceState({ @@ -407,7 +407,7 @@ suite('Positron - RuntimeSessionService', () => { willStartSessionDisposable.dispose(); const session2 = await start(); - assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(session2.getRuntimeState(), RuntimeState.Starting); const expectedActiveSessions = action === 'restore' ? // Restoring a session twice overwrites the previous session in activeSessions. @@ -483,8 +483,8 @@ suite('Positron - RuntimeSessionService', () => { const result2 = await start(); const result3 = await start(); - assert.equal(result1, result2); - assert.equal(result2, result3); + assert.strictEqual(result1, result2); + assert.strictEqual(result2, result3); assertSingleSessionIsStarting(result1); }); @@ -492,8 +492,8 @@ suite('Positron - RuntimeSessionService', () => { test(`${action} ${mode} concurrently`, async () => { const [result1, result2, result3] = await Promise.all([start(), start(), start()]); - assert.equal(result1, result2); - assert.equal(result2, result3); + assert.strictEqual(result1, result2); + assert.strictEqual(result2, result3); assertSingleSessionIsStarting(result1); }); @@ -507,8 +507,8 @@ suite('Positron - RuntimeSessionService', () => { startNotebook(), ]); - assert.equal(consoleSession.getRuntimeState(), RuntimeState.Starting); - assert.equal(notebookSession.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(consoleSession.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(notebookSession.getRuntimeState(), RuntimeState.Starting); assertServiceState({ hasStartingOrRunningConsole: true, @@ -532,17 +532,17 @@ suite('Positron - RuntimeSessionService', () => { test('restore console registers runtime if unregistered', async () => { // The runtime should not yet be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + assert.strictEqual(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); await restoreConsole(unregisteredRuntime); // The runtime should now be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); + assert.strictEqual(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), unregisteredRuntime); }); test('auto start validates runtime if unregistered', async () => { // The runtime should not yet be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + assert.strictEqual(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); // Update the validator to add extra runtime data. const validatedMetadata: Partial = { @@ -555,7 +555,7 @@ suite('Positron - RuntimeSessionService', () => { await autoStartSession(unregisteredRuntime); // The validated metadata should now be registered. - assert.deepEqual( + assert.deepStrictEqual( languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), { ...unregisteredRuntime, ...validatedMetadata } ); @@ -563,7 +563,7 @@ suite('Positron - RuntimeSessionService', () => { test('auto start throws if runtime validation errors', async () => { // The runtime should not yet be registered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + assert.strictEqual(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); // Update the validator to throw. const error = new Error('Failed to validate runtime metadata'); @@ -574,7 +574,7 @@ suite('Positron - RuntimeSessionService', () => { await assert.rejects(autoStartSession(unregisteredRuntime), error); // The runtime should remain unregistered. - assert.equal(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); + assert.strictEqual(languageRuntimeService.getRegisteredRuntime(unregisteredRuntime.runtimeId), undefined); }); test('auto start console does nothing if automatic startup is disabled', async () => { @@ -582,7 +582,7 @@ suite('Positron - RuntimeSessionService', () => { const sessionId = await runtimeSessionService.autoStartRuntime(runtime, startReason); - assert.equal(sessionId, ''); + assert.strictEqual(sessionId, ''); assertServiceState(); }); @@ -598,7 +598,7 @@ suite('Positron - RuntimeSessionService', () => { runtime.runtimeId, sessionName, LanguageRuntimeSessionMode.Console, undefined, startReason); } - assert.equal(sessionId, ''); + assert.strictEqual(sessionId, ''); assertServiceState(); workspaceTrustManagementService.setWorkspaceTrust(true); @@ -622,8 +622,8 @@ suite('Positron - RuntimeSessionService', () => { await waitForRuntimeState(session1, RuntimeState.Ready); const session2 = await selectRuntime(); - assert.equal(session1.getRuntimeState(), RuntimeState.Exited); - assert.equal(session2.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(session1.getRuntimeState(), RuntimeState.Exited); + assert.strictEqual(session2.getRuntimeState(), RuntimeState.Starting); assertServiceState({ hasStartingOrRunningConsole: true, @@ -649,8 +649,8 @@ suite('Positron - RuntimeSessionService', () => { const session2 = await selectRuntime(); - assert.equal(session1, session2); - assert.equal(runtimeSessionService.foregroundSession, session1); + assert.strictEqual(session1, session2); + assert.strictEqual(runtimeSessionService.foregroundSession, session1); }); test(`select console to another runtime and first session never fires onDidEndSession`, async () => { @@ -711,7 +711,7 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); - assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Restarting); assertSingleSessionIsRestarting(session); await waitForRuntimeState(session, RuntimeState.Ready); @@ -730,7 +730,7 @@ suite('Positron - RuntimeSessionService', () => { await restartSession(session.sessionId); // The existing sessino should remain exited. - assert.equal(session.getRuntimeState(), RuntimeState.Exited); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Exited); // A new session should be starting. let newSession: ILanguageRuntimeSession | undefined; @@ -742,11 +742,11 @@ suite('Positron - RuntimeSessionService', () => { assert.ok(newSession); disposables.add(newSession); - assert.equal(newSession.getRuntimeState(), RuntimeState.Starting); - assert.equal(newSession.metadata.sessionName, session.metadata.sessionName); - assert.equal(newSession.metadata.sessionMode, session.metadata.sessionMode); - assert.equal(newSession.metadata.notebookUri, session.metadata.notebookUri); - assert.equal(newSession.runtimeMetadata, session.runtimeMetadata); + assert.strictEqual(newSession.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(newSession.metadata.sessionName, session.metadata.sessionName); + assert.strictEqual(newSession.metadata.sessionMode, session.metadata.sessionMode); + assert.strictEqual(newSession.metadata.notebookUri, session.metadata.notebookUri); + assert.strictEqual(newSession.runtimeMetadata, session.runtimeMetadata); if (mode === LanguageRuntimeSessionMode.Console) { assertServiceState({ @@ -768,7 +768,7 @@ suite('Positron - RuntimeSessionService', () => { test(`restart ${mode} in 'starting' state`, async () => { const session = await start(); - assert.equal(session.getRuntimeState(), RuntimeState.Starting); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Starting); await restartSession(session.sessionId); @@ -780,7 +780,7 @@ suite('Positron - RuntimeSessionService', () => { await waitForRuntimeState(session, RuntimeState.Ready); session.restart(); - assert.equal(session.getRuntimeState(), RuntimeState.Restarting); + assert.strictEqual(session.getRuntimeState(), RuntimeState.Restarting); const target = sinon.spy(session, 'restart');