diff --git a/extensions/notebook/src/jupyter/serverInstance.ts b/extensions/notebook/src/jupyter/serverInstance.ts index e2865010ed..9204224fa5 100644 --- a/extensions/notebook/src/jupyter/serverInstance.ts +++ b/extensions/notebook/src/jupyter/serverInstance.ts @@ -8,7 +8,7 @@ import * as vscode from 'vscode'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as os from 'os'; -import { spawn, ExecOptions, SpawnOptions, ChildProcess } from 'child_process'; +import { spawn, SpawnOptions, ChildProcess } from 'child_process'; import * as nls from 'vscode-nls'; const localize = nls.loadMessageBundle(); @@ -25,6 +25,26 @@ const defaultPort = 8888; type MessageListener = (data: string | Buffer) => void; type ErrorListener = (err: any) => void; +/** + * Helper function ensures server instance process stops + */ +export function ensureProcessEnded(childProcess: ChildProcess): void { + if (!childProcess) { + return; + } + // Wait 5 seconds and then force kill. Jupyter stop is slow so this seems a reasonable time limit + setTimeout(() => { + // Test if the process is still alive. Throws an exception if not + try { + process.kill(childProcess.pid, 'SIGKILL'); + } catch (error) { + if (!error || !error.code || (typeof error.code === 'string' && error.code !== 'ESRCH')) { + console.log(error); + } + } + }, 5000); +} + export interface IInstanceOptions { /** * The path to the initial document we want to start this server for @@ -43,60 +63,6 @@ export interface IInstanceOptions { notebookDirectory?: string; } -/** - * Helper class to enable testing without calling into file system or - * commandline shell APIs - */ -export class ServerInstanceUtils { - public mkDir(dirPath: string, outputChannel?: vscode.OutputChannel): Promise { - return utils.mkDir(dirPath, outputChannel); - } - public removeDir(dirPath: string): Promise { - return fs.remove(dirPath); - } - public pathExists(dirPath: string): Promise { - return fs.pathExists(dirPath); - } - public copy(src: string, dest: string): Promise { - return fs.copy(src, dest); - } - public async exists(path: string): Promise { - try { - await fs.access(path); - return true; - } catch (e) { - return false; - } - } - public generateUuid(): string { - return UUID.generateUuid(); - } - public executeBufferedCommand(cmd: string, options: ExecOptions, outputChannel?: vscode.OutputChannel): Thenable { - return utils.executeBufferedCommand(cmd, options, outputChannel); - } - - public spawn(command: string, args?: ReadonlyArray, options?: SpawnOptions): ChildProcess { - return spawn(command, args, options); - } - - public ensureProcessEnded(childProcess: ChildProcess): void { - if (!childProcess) { - return; - } - // Wait 5 seconds and then force kill. Jupyter stop is slow so this seems a reasonable time limit - setTimeout(() => { - // Test if the process is still alive. Throws an exception if not - try { - process.kill(childProcess.pid, 'SIGKILL'); - } catch (error) { - if (!error || !error.code || (typeof error.code === 'string' && error.code !== 'ESRCH')) { - console.log(error); - } - } - }, 5000); - } -} - export class PerFolderServerInstance implements IServerInstance { /** @@ -122,14 +88,12 @@ export class PerFolderServerInstance implements IServerInstance { private _uri: vscode.Uri; private _isStarted: boolean = false; private _isStopping: boolean = false; - private utils: ServerInstanceUtils; private childProcess: ChildProcess; private errorHandler: ErrorHandler = new ErrorHandler(); private readonly notebookScriptPath: string; - constructor(private options: IInstanceOptions, fsUtils?: ServerInstanceUtils) { - this.utils = fsUtils || new ServerInstanceUtils(); + constructor(private options: IInstanceOptions) { this.notebookScriptPath = path.join(this.options.install.extensionPath, 'resources', 'pythonScripts', 'startNotebook.py'); } @@ -157,28 +121,27 @@ export class PerFolderServerInstance implements IServerInstance { try { this._isStopping = true; if (this.baseDir) { - let exists = await this.utils.pathExists(this.baseDir); + let exists = await fs.pathExists(this.baseDir); if (exists) { - await this.utils.removeDir(this.baseDir); + await fs.remove(this.baseDir); } } if (this._isStarted) { let install = this.options.install; let stopCommand = `"${install.pythonExecutable}" "${this.notebookScriptPath}" stop ${this._port}`; - await this.utils.executeBufferedCommand(stopCommand, install.execOptions, install.outputChannel); + await utils.executeBufferedCommand(stopCommand, install.execOptions, install.outputChannel); } } catch (error) { // For now, we don't care as this is non-critical this.notify(this.options.install, localize('serverStopError', "Error stopping Notebook Server: {0}", utils.getErrorMessage(error))); } finally { this._isStarted = false; - this.utils.ensureProcessEnded(this.childProcess); + ensureProcessEnded(this.childProcess); this.handleConnectionClosed(); } } - private async configureJupyter(): Promise { await this.createInstanceFolders(); let resourcesFolder = path.join(this.options.install.extensionPath, 'resources', constants.jupyterConfigRootFolder); @@ -188,35 +151,35 @@ export class PerFolderServerInstance implements IServerInstance { } private async createInstanceFolders(): Promise { - this.baseDir = path.join(this.getSystemJupyterHomeDir(), 'instances', `${this.utils.generateUuid()}`); + this.baseDir = path.join(this.getSystemJupyterHomeDir(), 'instances', `${UUID.generateUuid()}`); this.instanceConfigRoot = path.join(this.baseDir, 'config'); this.instanceDataRoot = path.join(this.baseDir, 'data'); - await this.utils.mkDir(this.baseDir, this.options.install.outputChannel); - await this.utils.mkDir(this.instanceConfigRoot, this.options.install.outputChannel); - await this.utils.mkDir(this.instanceDataRoot, this.options.install.outputChannel); + await utils.mkDir(this.baseDir, this.options.install.outputChannel); + await utils.mkDir(this.instanceConfigRoot, this.options.install.outputChannel); + await utils.mkDir(this.instanceDataRoot, this.options.install.outputChannel); } private async copyInstanceConfig(resourcesFolder: string): Promise { let configSource = path.join(resourcesFolder, NotebookConfigFilename); let configDest = path.join(this.instanceConfigRoot, NotebookConfigFilename); - await this.utils.copy(configSource, configDest); + await fs.copy(configSource, configDest); } private async CopyCustomJs(resourcesFolder: string): Promise { let customPath = path.join(this.instanceConfigRoot, 'custom'); - await this.utils.mkDir(customPath, this.options.install.outputChannel); + await utils.mkDir(customPath, this.options.install.outputChannel); let customSource = path.join(resourcesFolder, CustomJsFilename); let customDest = path.join(customPath, CustomJsFilename); - await this.utils.copy(customSource, customDest); + await fs.copy(customSource, customDest); } private async copyKernelsToSystemJupyterDirs(): Promise { let kernelsExtensionSource = path.join(this.options.install.extensionPath, 'kernels'); this._systemJupyterDir = path.join(this.getSystemJupyterHomeDir(), 'kernels'); - if (!(await this.utils.exists(this._systemJupyterDir))) { - await this.utils.mkDir(this._systemJupyterDir, this.options.install.outputChannel); + if (!(await utils.exists(this._systemJupyterDir))) { + await utils.mkDir(this._systemJupyterDir, this.options.install.outputChannel); } - await this.utils.copy(kernelsExtensionSource, this._systemJupyterDir); + await fs.copy(kernelsExtensionSource, this._systemJupyterDir); } private getSystemJupyterHomeDir(): string { @@ -384,7 +347,7 @@ export class PerFolderServerInstance implements IServerInstance { env: env, detached: false }; - let childProcess = this.utils.spawn(startCommand, [], options); + let childProcess = spawn(startCommand, [], options); return childProcess; } diff --git a/extensions/notebook/src/test/model/serverInstance.test.ts b/extensions/notebook/src/test/model/serverInstance.test.ts index 72eb800a3b..e3c7e6b58f 100644 --- a/extensions/notebook/src/test/model/serverInstance.test.ts +++ b/extensions/notebook/src/test/model/serverInstance.test.ts @@ -6,11 +6,14 @@ import * as should from 'should'; import * as TypeMoq from 'typemoq'; import * as stream from 'stream'; -import { ChildProcess } from 'child_process'; +import * as cp from 'child_process'; +import * as si from '../../jupyter/serverInstance' import 'mocha'; +import * as sinon from 'sinon'; +import * as utils from '../../common/utils'; +import * as fs from 'fs-extra'; import { JupyterServerInstallation } from '../../jupyter/jupyterServerInstallation'; -import { PerFolderServerInstance, ServerInstanceUtils } from '../../jupyter/serverInstance'; import { MockOutputChannel } from '../common/stubs'; import * as testUtils from '../common/testUtils'; import { LocalJupyterServerManager } from '../../jupyter/jupyterServerManager'; @@ -24,8 +27,7 @@ describe('Jupyter server instance', function (): void { let expectedPath = 'mydir/notebook.ipynb'; let mockInstall: TypeMoq.IMock; let mockOutputChannel: TypeMoq.IMock; - let mockUtils: TypeMoq.IMock; - let serverInstance: PerFolderServerInstance; + let serverInstance: si.PerFolderServerInstance; beforeEach(() => { mockInstall = TypeMoq.Mock.ofType(JupyterServerInstallation, undefined, undefined, '/root'); @@ -33,14 +35,16 @@ describe('Jupyter server instance', function (): void { mockInstall.setup(i => i.outputChannel).returns(() => mockOutputChannel.object); mockInstall.setup(i => i.pythonExecutable).returns(() => 'python3'); mockInstall.object.execOptions = { env: Object.assign({}, process.env) }; - mockUtils = TypeMoq.Mock.ofType(ServerInstanceUtils); - mockUtils.setup(u => u.ensureProcessEnded(TypeMoq.It.isAny())).returns(() => undefined); - serverInstance = new PerFolderServerInstance({ + serverInstance = new si.PerFolderServerInstance({ documentPath: expectedPath, install: mockInstall.object - }, mockUtils.object); + }); + sinon.stub(si,'ensureProcessEnded').returns(undefined); }); + this.afterEach(function () { + sinon.restore(); + }); it('Should not be started initially', function (): void { // Given a new instance It should not be started @@ -50,17 +54,17 @@ describe('Jupyter server instance', function (): void { it('Should create config and data directories on configure', async function (): Promise { // Given a server instance - mockUtils.setup(u => u.mkDir(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())).returns(() => Promise.resolve()); - mockUtils.setup(u => u.copy(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())).returns(() => Promise.resolve()); - mockUtils.setup(u => u.exists(TypeMoq.It.isAnyString())).returns(() => Promise.resolve(false)); + let mkdirStub = sinon.stub(utils,'mkDir').withArgs(sinon.match.any,sinon.match.any).returns(Promise.resolve()); + let copyStub = sinon.stub(fs,'copy').returns(); + let pathStub = sinon.stub(utils,'exists').withArgs(sinon.match.any).returns(Promise.resolve(false)); // When I run configure await serverInstance.configure(); // Then I expect a folder to have been created with config and data subdirs - mockUtils.verify(u => u.mkDir(TypeMoq.It.isAnyString(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(5)); - mockUtils.verify(u => u.copy(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString()), TypeMoq.Times.exactly(3)); - mockUtils.verify(u => u.exists(TypeMoq.It.isAnyString()), TypeMoq.Times.exactly(1)); + sinon.assert.callCount(mkdirStub,5); + sinon.assert.callCount(copyStub,3); + sinon.assert.callCount(pathStub,1); }); it('Should have URI info after start', async function (): Promise { @@ -69,11 +73,12 @@ describe('Jupyter server instance', function (): void { sdtout: (listener: (msg: string) => void) => { }, stderr: (listener: (msg: string) => void) => listener(successMessage) }); - mockUtils.setup(u => u.spawn(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => process.object); + let spawnStub = sinon.stub(cp,'spawn').returns(process.object); + should(spawnStub.calledOnce).be.false(); // When I call start await serverInstance.start(); + should(spawnStub.calledOnce).be.true(); // Then I expect all parts of the URI to be defined should(serverInstance.uri).not.be.undefined(); @@ -100,8 +105,7 @@ describe('Jupyter server instance', function (): void { stderr: (listener: (msg: string) => void) => listener(successMessage), error: (listener: (msg: string | Error) => void) => setTimeout(() => listener(new Error(error)), 10) }); - mockUtils.setup(u => u.spawn(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => process.object); + sinon.stub(cp,'spawn').returns(process.object); // When I call start then I expect it to pass await serverInstance.start(); @@ -112,8 +116,7 @@ describe('Jupyter server instance', function (): void { let process = setupSpawn({ exit: (listener: (msg: string | number) => void) => listener(code) }); - mockUtils.setup(u => u.spawn(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => process.object); + sinon.stub(cp,'spawn').returns(process.object); // When I call start then I expect the error to be thrown await testUtils.assertThrowsAsync(() => serverInstance.start(), undefined); @@ -126,41 +129,44 @@ describe('Jupyter server instance', function (): void { sdtout: (listener: (msg: string) => void) => { }, stderr: (listener: (msg: string) => void) => listener(successMessage) }); - mockUtils.setup(u => u.spawn(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => process.object); + sinon.stub(cp,'spawn').returns(process.object); let actualCommand: string = undefined; - mockUtils.setup(u => u.executeBufferedCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns((cmd) => { - actualCommand = cmd; - return Promise.resolve(undefined); - }); - mockUtils.setup(u => u.pathExists(TypeMoq.It.isAny())).returns(() => Promise.resolve(false)); - mockUtils.setup(u => u.removeDir(TypeMoq.It.isAny())).returns(() => Promise.resolve()); + let commandStub = sinon.stub(utils, 'executeBufferedCommand').withArgs(sinon.match.any, sinon.match.any,sinon.match.any) + .returns(Promise.resolve(undefined)); + + sinon.stub(fs,'pathExists').withArgs(sinon.match.any,sinon.match.any).returns(); + let removeStub = sinon.stub(fs,'remove').withArgs(sinon.match.any,sinon.match.any).returns(); + // When I call start and then stop await serverInstance.start(); await serverInstance.stop(); // Then I expect stop to be called on the child process + actualCommand = commandStub.args[0][0] as string; should(actualCommand.includes(`stop ${serverInstance.port}`)).be.true('Command did not contain specified port.'); - mockUtils.verify(u => u.removeDir(TypeMoq.It.isAny()), TypeMoq.Times.never()); + sinon.assert.callCount(removeStub,0); }); it('Should remove directory on close', async function (): Promise { // Given configure and startup are done - mockUtils.setup(u => u.mkDir(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())).returns(() => Promise.resolve()); - mockUtils.setup(u => u.copy(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())).returns(() => Promise.resolve()); + sinon.stub(utils,'mkDir').withArgs(sinon.match.any,sinon.match.any).returns(Promise.resolve()); + sinon.stub(fs,'copy').returns(); let process = setupSpawn({ sdtout: (listener: (msg: string) => void) => { }, stderr: (listener: (msg: string) => void) => listener(successMessage) }); - mockUtils.setup(u => u.spawn(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => process.object); - mockUtils.setup(u => u.executeBufferedCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns((cmd) => Promise.resolve(undefined)); - mockUtils.setup(u => u.pathExists(TypeMoq.It.isAny())).returns(() => Promise.resolve(true)); - mockUtils.setup(u => u.removeDir(TypeMoq.It.isAny())).returns(() => Promise.resolve()); + + sinon.stub(cp,'spawn').returns(process.object); + + sinon.stub(utils, 'executeBufferedCommand').withArgs(sinon.match.any, sinon.match.any,sinon.match.any) + .returns(Promise.resolve(undefined)); + + let pathStub = sinon.stub(fs,'pathExists'); + pathStub.resolves(true); + + let removeStub = sinon.stub(fs,'remove').returns(); await serverInstance.configure(); await serverInstance.start(); @@ -169,7 +175,7 @@ describe('Jupyter server instance', function (): void { await serverInstance.stop(); // Then I expect the directory to be cleaned up - mockUtils.verify(u => u.removeDir(TypeMoq.It.isAny()), TypeMoq.Times.once()); + sinon.assert.callCount(removeStub,1); }); function setupSpawn(callbacks: IProcessCallbacks): TypeMoq.IMock {