From 1a505d6f10a473f1e7f0d192bf36eeb664ff6eac Mon Sep 17 00:00:00 2001 From: Paul Donnelly Date: Sat, 1 Mar 2025 11:11:48 -0600 Subject: [PATCH 1/5] Closes #24359 --- src/client/repl/nativeRepl.ts | 6 ++++++ src/client/repl/pythonServer.ts | 1 + 2 files changed, 7 insertions(+) diff --git a/src/client/repl/nativeRepl.ts b/src/client/repl/nativeRepl.ts index 6edd3cbd70a7..65cb04ad8404 100644 --- a/src/client/repl/nativeRepl.ts +++ b/src/client/repl/nativeRepl.ts @@ -74,6 +74,12 @@ export class NativeRepl implements Disposable { this.notebookDocument = undefined; this.newReplSession = true; await updateWorkspaceStateValue(NATIVE_REPL_URI_MEMENTO, undefined); + this.pythonServer.dispose(); + this.pythonServer = createPythonServer([this.interpreter.path as string], this.cwd); + if (this.replController) { + this.replController.dispose(); + } + nativeRepl = undefined; } }), ); diff --git a/src/client/repl/pythonServer.ts b/src/client/repl/pythonServer.ts index 570433714f98..74e2d6ae7251 100644 --- a/src/client/repl/pythonServer.ts +++ b/src/client/repl/pythonServer.ts @@ -104,6 +104,7 @@ class PythonServerImpl implements PythonServer, Disposable { this.connection.sendNotification('exit'); this.disposables.forEach((d) => d.dispose()); this.connection.dispose(); + serverInstance = undefined; } } From 265a273e4201865668c91897aa22893f562ad72e Mon Sep 17 00:00:00 2001 From: Paul Donnelly Date: Fri, 7 Mar 2025 19:06:01 -0600 Subject: [PATCH 2/5] add new pythonServer to disposables array --- src/client/repl/nativeRepl.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/client/repl/nativeRepl.ts b/src/client/repl/nativeRepl.ts index 65cb04ad8404..ae2f1641f658 100644 --- a/src/client/repl/nativeRepl.ts +++ b/src/client/repl/nativeRepl.ts @@ -76,6 +76,7 @@ export class NativeRepl implements Disposable { await updateWorkspaceStateValue(NATIVE_REPL_URI_MEMENTO, undefined); this.pythonServer.dispose(); this.pythonServer = createPythonServer([this.interpreter.path as string], this.cwd); + this.disposables.push(this.pythonServer); if (this.replController) { this.replController.dispose(); } From 1943c5c55a6aa24d29a3339d77c6d28f565139da Mon Sep 17 00:00:00 2001 From: Paul Donnelly Date: Thu, 20 Mar 2025 19:37:23 -0500 Subject: [PATCH 3/5] add test for native repl instance disposal --- src/test/repl/nativeRepl.test.ts | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/test/repl/nativeRepl.test.ts b/src/test/repl/nativeRepl.test.ts index 999bc656c64d..e8f6d495f07e 100644 --- a/src/test/repl/nativeRepl.test.ts +++ b/src/test/repl/nativeRepl.test.ts @@ -95,4 +95,26 @@ suite('REPL - Native REPL', () => { setReplDirectoryStub.restore(); setReplControllerSpy.restore(); }); + + test('Closing REPL should dispose of the previous instance and create a new one on reopening', async () => { + const disposeSpy = sinon.spy(NativeRepl.prototype, 'dispose'); + const createStub = sinon.stub(NativeRepl, 'create').callThrough(); + interpreterService + .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) + .returns(() => Promise.resolve(({ path: 'ps' } as unknown) as PythonEnvironment)); + const interpreter = await interpreterService.object.getActiveInterpreter(); + + const firstRepl = await getNativeRepl(interpreter as PythonEnvironment, disposableArray); + + firstRepl.dispose(); + + const secondRepl = await getNativeRepl(interpreter as PythonEnvironment, disposableArray); + + expect(disposeSpy.calledOnce).to.be.true; + expect(createStub.calledTwice).to.be.true; + expect(firstRepl).to.not.equal(secondRepl); + + disposeSpy.restore(); + createStub.restore(); + }); }); From c0dde023573fc7e512351691ab58b481db9570f8 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Tue, 1 Apr 2025 11:15:01 -0700 Subject: [PATCH 4/5] add test for properly disposing notebook, pythonServer --- src/client/common/vscodeApis/workspaceApis.ts | 4 + src/client/repl/nativeRepl.ts | 8 +- src/test/repl/nativeRepl.test.ts | 129 +++++++++++++----- 3 files changed, 107 insertions(+), 34 deletions(-) diff --git a/src/client/common/vscodeApis/workspaceApis.ts b/src/client/common/vscodeApis/workspaceApis.ts index cb516da73075..1e3a9528078a 100644 --- a/src/client/common/vscodeApis/workspaceApis.ts +++ b/src/client/common/vscodeApis/workspaceApis.ts @@ -60,6 +60,10 @@ export function onDidChangeConfiguration(handler: (e: vscode.ConfigurationChange return vscode.workspace.onDidChangeConfiguration(handler); } +export function onDidCloseNotebookDocument(handler: (e: vscode.NotebookDocument) => void): vscode.Disposable { + return vscode.workspace.onDidCloseNotebookDocument(handler); +} + export function createFileSystemWatcher( globPattern: vscode.GlobPattern, ignoreCreateEvents?: boolean, diff --git a/src/client/repl/nativeRepl.ts b/src/client/repl/nativeRepl.ts index ae2f1641f658..9b002655d714 100644 --- a/src/client/repl/nativeRepl.ts +++ b/src/client/repl/nativeRepl.ts @@ -1,3 +1,6 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + // Native Repl class that holds instance of pythonServer and replController import { @@ -7,13 +10,12 @@ import { QuickPickItem, TextEditor, Uri, - workspace, WorkspaceFolder, } from 'vscode'; import { Disposable } from 'vscode-jsonrpc'; import { PVSC_EXTENSION_ID } from '../common/constants'; import { showQuickPick } from '../common/vscodeApis/windowApis'; -import { getWorkspaceFolders } from '../common/vscodeApis/workspaceApis'; +import { getWorkspaceFolders, onDidCloseNotebookDocument } from '../common/vscodeApis/workspaceApis'; import { PythonEnvironment } from '../pythonEnvironments/info'; import { createPythonServer, PythonServer } from './pythonServer'; import { executeNotebookCell, openInteractiveREPL, selectNotebookKernel } from './replCommandHandler'; @@ -69,7 +71,7 @@ export class NativeRepl implements Disposable { */ private watchNotebookClosed(): void { this.disposables.push( - workspace.onDidCloseNotebookDocument(async (nb) => { + onDidCloseNotebookDocument(async (nb) => { if (this.notebookDocument && nb.uri.toString() === this.notebookDocument.uri.toString()) { this.notebookDocument = undefined; this.newReplSession = true; diff --git a/src/test/repl/nativeRepl.test.ts b/src/test/repl/nativeRepl.test.ts index e8f6d495f07e..c05bb311a839 100644 --- a/src/test/repl/nativeRepl.test.ts +++ b/src/test/repl/nativeRepl.test.ts @@ -1,14 +1,21 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + /* eslint-disable no-unused-expressions */ /* eslint-disable @typescript-eslint/no-explicit-any */ import * as TypeMoq from 'typemoq'; import * as sinon from 'sinon'; -import { Disposable } from 'vscode'; +import { Disposable, EventEmitter, NotebookDocument, Uri } from 'vscode'; import { expect } from 'chai'; import { IInterpreterService } from '../../client/interpreter/contracts'; import { PythonEnvironment } from '../../client/pythonEnvironments/info'; -import { getNativeRepl, NativeRepl } from '../../client/repl/nativeRepl'; +import * as NativeReplModule from '../../client/repl/nativeRepl'; import * as persistentState from '../../client/common/persistentState'; +import * as PythonServer from '../../client/repl/pythonServer'; +import * as vscodeWorkspaceApis from '../../client/common/vscodeApis/workspaceApis'; +import * as replController from '../../client/repl/replController'; +import { executeCommand } from '../../client/common/vscodeApis/commandApis'; suite('REPL - Native REPL', () => { let interpreterService: TypeMoq.IMock; @@ -19,8 +26,20 @@ suite('REPL - Native REPL', () => { let setReplControllerSpy: sinon.SinonSpy; let getWorkspaceStateValueStub: sinon.SinonStub; let updateWorkspaceStateValueStub: sinon.SinonStub; + let createReplControllerStub: sinon.SinonStub; + let mockNotebookController: any; setup(() => { + (NativeReplModule as any).nativeRepl = undefined; + + mockNotebookController = { + id: 'mockController', + dispose: sinon.stub(), + updateNotebookAffinity: sinon.stub(), + createNotebookCellExecution: sinon.stub(), + variableProvider: null, + }; + interpreterService = TypeMoq.Mock.ofType(); interpreterService .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) @@ -28,13 +47,13 @@ suite('REPL - Native REPL', () => { disposable = TypeMoq.Mock.ofType(); disposableArray = [disposable.object]; - setReplDirectoryStub = sinon.stub(NativeRepl.prototype as any, 'setReplDirectory').resolves(); // Stubbing private method - // Use a spy instead of a stub for setReplController - setReplControllerSpy = sinon.spy(NativeRepl.prototype, 'setReplController'); + createReplControllerStub = sinon.stub(replController, 'createReplController').returns(mockNotebookController); + setReplDirectoryStub = sinon.stub(NativeReplModule.NativeRepl.prototype as any, 'setReplDirectory').resolves(); + setReplControllerSpy = sinon.spy(NativeReplModule.NativeRepl.prototype, 'setReplController'); updateWorkspaceStateValueStub = sinon.stub(persistentState, 'updateWorkspaceStateValue').resolves(); }); - teardown(() => { + teardown(async () => { disposableArray.forEach((d) => { if (d) { d.dispose(); @@ -42,15 +61,16 @@ suite('REPL - Native REPL', () => { }); disposableArray = []; sinon.restore(); + executeCommand('workbench.action.closeActiveEditor'); }); test('getNativeRepl should call create constructor', async () => { - const createMethodStub = sinon.stub(NativeRepl, 'create'); + const createMethodStub = sinon.stub(NativeReplModule.NativeRepl, 'create'); interpreterService .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => Promise.resolve(({ path: 'ps' } as unknown) as PythonEnvironment)); const interpreter = await interpreterService.object.getActiveInterpreter(); - await getNativeRepl(interpreter as PythonEnvironment, disposableArray); + await NativeReplModule.getNativeRepl(interpreter as PythonEnvironment, disposableArray); expect(createMethodStub.calledOnce).to.be.true; }); @@ -61,7 +81,7 @@ suite('REPL - Native REPL', () => { .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => Promise.resolve(({ path: 'ps' } as unknown) as PythonEnvironment)); const interpreter = await interpreterService.object.getActiveInterpreter(); - const nativeRepl = await getNativeRepl(interpreter as PythonEnvironment, disposableArray); + const nativeRepl = await NativeReplModule.getNativeRepl(interpreter as PythonEnvironment, disposableArray); nativeRepl.sendToNativeRepl(undefined, false); @@ -74,7 +94,7 @@ suite('REPL - Native REPL', () => { .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => Promise.resolve(({ path: 'ps' } as unknown) as PythonEnvironment)); const interpreter = await interpreterService.object.getActiveInterpreter(); - const nativeRepl = await getNativeRepl(interpreter as PythonEnvironment, disposableArray); + const nativeRepl = await NativeReplModule.getNativeRepl(interpreter as PythonEnvironment, disposableArray); nativeRepl.sendToNativeRepl(undefined, false); @@ -87,34 +107,81 @@ suite('REPL - Native REPL', () => { .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => Promise.resolve(({ path: 'ps' } as unknown) as PythonEnvironment)); - await NativeRepl.create(interpreter as PythonEnvironment); + await NativeReplModule.NativeRepl.create(interpreter as PythonEnvironment); expect(setReplDirectoryStub.calledOnce).to.be.true; expect(setReplControllerSpy.calledOnce).to.be.true; - - setReplDirectoryStub.restore(); - setReplControllerSpy.restore(); + expect(createReplControllerStub.calledOnce).to.be.true; }); - test('Closing REPL should dispose of the previous instance and create a new one on reopening', async () => { - const disposeSpy = sinon.spy(NativeRepl.prototype, 'dispose'); - const createStub = sinon.stub(NativeRepl, 'create').callThrough(); - interpreterService - .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) - .returns(() => Promise.resolve(({ path: 'ps' } as unknown) as PythonEnvironment)); - const interpreter = await interpreterService.object.getActiveInterpreter(); - - const firstRepl = await getNativeRepl(interpreter as PythonEnvironment, disposableArray); - - firstRepl.dispose(); + test('watchNotebookClosed should clean up resources when notebook is closed', async () => { + const notebookCloseEmitter = new EventEmitter(); + sinon.stub(vscodeWorkspaceApis, 'onDidCloseNotebookDocument').callsFake((handler) => { + const disposable = notebookCloseEmitter.event(handler); + return disposable; + }); - const secondRepl = await getNativeRepl(interpreter as PythonEnvironment, disposableArray); + const mockPythonServer = { + onCodeExecuted: new EventEmitter().event, + execute: sinon.stub().resolves({ status: true, output: 'test output' }), + executeSilently: sinon.stub().resolves({ status: true, output: 'test output' }), + interrupt: sinon.stub(), + input: sinon.stub(), + checkValidCommand: sinon.stub().resolves(true), + dispose: sinon.stub(), + }; + + // Track the number of times createPythonServer was called + let createPythonServerCallCount = 0; + sinon.stub(PythonServer, 'createPythonServer').callsFake(() => { + // eslint-disable-next-line no-plusplus + createPythonServerCallCount++; + return mockPythonServer; + }); - expect(disposeSpy.calledOnce).to.be.true; - expect(createStub.calledTwice).to.be.true; - expect(firstRepl).to.not.equal(secondRepl); + const interpreter = await interpreterService.object.getActiveInterpreter(); - disposeSpy.restore(); - createStub.restore(); + // Create NativeRepl directly to have more control over its state, go around private constructor. + const nativeRepl = new (NativeReplModule.NativeRepl as any)(); + nativeRepl.interpreter = interpreter as PythonEnvironment; + nativeRepl.cwd = '/helloJustMockedCwd/cwd'; + nativeRepl.pythonServer = mockPythonServer; + nativeRepl.replController = mockNotebookController; + nativeRepl.disposables = []; + + // Make the singleton point to our instance for testing + // Otherwise, it gets mixed with Native Repl from .create from test above. + (NativeReplModule as any).nativeRepl = nativeRepl; + + // Reset call count after initial setup + createPythonServerCallCount = 0; + + // Set notebookDocument to a mock document + const mockReplUri = Uri.parse('untitled:Untitled-999.ipynb?jupyter-notebook'); + const mockNotebookDocument = ({ + uri: mockReplUri, + toString: () => mockReplUri.toString(), + } as unknown) as NotebookDocument; + + nativeRepl.notebookDocument = mockNotebookDocument; + + // Create a mock notebook document for closing event with same URI + const closingNotebookDocument = ({ + uri: mockReplUri, + toString: () => mockReplUri.toString(), + } as unknown) as NotebookDocument; + + notebookCloseEmitter.fire(closingNotebookDocument); + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect( + updateWorkspaceStateValueStub.calledWith(NativeReplModule.NATIVE_REPL_URI_MEMENTO, undefined), + 'updateWorkspaceStateValue should be called with NATIVE_REPL_URI_MEMENTO and undefined', + ).to.be.true; + expect(mockPythonServer.dispose.calledOnce, 'pythonServer.dispose() should be called once').to.be.true; + expect(createPythonServerCallCount, 'createPythonServer should be called to create a new server').to.equal(1); + expect(nativeRepl.notebookDocument, 'notebookDocument should be undefined after closing').to.be.undefined; + expect(nativeRepl.newReplSession, 'newReplSession should be set to true after closing').to.be.true; + expect(mockNotebookController.dispose.calledOnce, 'replController.dispose() should be called once').to.be.true; }); }); From a887ebe061f5cf8912bc9cc8d69fd3e0c3f344c7 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Tue, 1 Apr 2025 12:03:56 -0700 Subject: [PATCH 5/5] update setup-node, setup-python on smoketest action.yml to match with other action.yml --- .github/actions/smoke-tests/action.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/actions/smoke-tests/action.yml b/.github/actions/smoke-tests/action.yml index 2463f83ee90c..ed760e8b8202 100644 --- a/.github/actions/smoke-tests/action.yml +++ b/.github/actions/smoke-tests/action.yml @@ -13,13 +13,13 @@ runs: using: 'composite' steps: - name: Install Node - uses: actions/setup-node@v2 + uses: actions/setup-node@v4 with: node-version: ${{ inputs.node_version }} cache: 'npm' - name: Install Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: '3.x' cache: 'pip'