From 1d22e23c2dcbb2df12a43e984ab1d54892bf770d Mon Sep 17 00:00:00 2001 From: Chris LaFreniere <40371649+chlafreniere@users.noreply.github.com> Date: Mon, 6 Apr 2020 16:39:02 -0700 Subject: [PATCH] Add a few more notebook model unit tests (#9859) * Add a few more notebook model tests * Add tests for nb managers, active cell * Add assert messages --- .../electron-browser/notebookModel.test.ts | 298 ++++++++++++++++-- .../notebook/browser/models/notebookModel.ts | 7 +- 2 files changed, 270 insertions(+), 35 deletions(-) diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts index 137537e9f2..0e62f033a0 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts @@ -14,7 +14,7 @@ import { URI } from 'vs/base/common/uri'; import { NotebookManagerStub } from 'sql/workbench/contrib/notebook/test/stubs'; import { NotebookModel } from 'sql/workbench/services/notebook/browser/models/notebookModel'; import { ModelFactory } from 'sql/workbench/services/notebook/browser/models/modelFactory'; -import { IClientSession, INotebookModelOptions, NotebookContentChange, IClientSessionOptions } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; +import { IClientSession, INotebookModelOptions, NotebookContentChange, IClientSessionOptions, ICellModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces'; import { ClientSession } from 'sql/workbench/services/notebook/browser/models/clientSession'; import { CellTypes, NotebookChangeType } from 'sql/workbench/services/notebook/common/contracts'; import { Deferred } from 'sql/base/common/promise'; @@ -32,6 +32,9 @@ import { isUndefinedOrNull } from 'vs/base/common/types'; import { assign } from 'vs/base/common/objects'; import { NotebookEditorContentManager } from 'sql/workbench/contrib/notebook/browser/models/notebookInput'; import { SessionManager } from 'sql/workbench/services/notebook/browser/sessionManager'; +import { mssqlProviderName } from 'sql/platform/connection/common/constants'; +import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile'; +import { uriPrefixes } from 'sql/platform/connection/common/utils'; let expectedNotebookContent: nb.INotebookContents = { cells: [{ @@ -205,6 +208,163 @@ suite('notebook model', function (): void { assert.deepEqual(model.cells[1].source, expectedNotebookContent.cells[1].source); }); + test('Should handle multiple notebook managers', async function (): Promise { + // Given a notebook with 2 cells + let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); + mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); + defaultModelOptions.contentManager = mockContentManager.object; + + let defaultNotebookManager = new NotebookManagerStub(); + defaultNotebookManager.providerId = 'SQL'; + + let jupyterNotebookManager = new NotebookManagerStub(); + jupyterNotebookManager.providerId = 'jupyter'; + + // Setup 2 notebook managers + defaultModelOptions.notebookManagers = [defaultNotebookManager, jupyterNotebookManager]; + + // Change default notebook provider id to jupyter + defaultModelOptions.providerId = 'jupyter'; + + // When I initalize the model + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + await model.loadContents(); + + // I expect the default provider to be jupyter + assert.equal(model.notebookManager.providerId, 'jupyter', 'Notebook manager provider id incorrect'); + + // Similarly, change default notebook provider id to SQL + defaultModelOptions.providerId = 'SQL'; + + // When I initalize the model + model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + await model.loadContents(); + + // I expect the default provider to be SQL + assert.equal(model.notebookManager.providerId, 'SQL', 'Notebook manager provider id incorrect after 2nd model load'); + + // Check that the getters return the correct values + assert.equal(model.notebookManagers.length, 2, 'There should be 2 notebook managers'); + assert(!isUndefinedOrNull(model.getNotebookManager('SQL')), 'SQL notebook manager is not defined'); + assert(!isUndefinedOrNull(model.getNotebookManager('jupyter')), 'Jupyter notebook manager is not defined'); + assert(isUndefinedOrNull(model.getNotebookManager('foo')), 'foo notebook manager is incorrectly defined'); + + // Check other properties to ensure that they're returning as expected + // No server manager was passed into the notebook manager stub, so expect hasServerManager to return false + assert.equal(model.hasServerManager, false, 'Notebook model should not have a server manager'); + assert.equal(model.notebookUri, defaultModelOptions.notebookUri, 'Notebook model has incorrect URI'); + }); + + test('Should set active cell correctly', async function (): Promise { + // Given a notebook with 2 cells + let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); + mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); + defaultModelOptions.contentManager = mockContentManager.object; + + // When I initalize the model + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + await model.loadContents(); + + let activeCellChangeCount = 0; + let activeCellFromEvent: ICellModel = undefined; + + model.onActiveCellChanged(c => { + activeCellChangeCount++; + activeCellFromEvent = c; + }); + + let notebookContentChange: NotebookContentChange; + model.contentChanged(c => notebookContentChange = c); + + // Then I expect all cells to be in the model + assert.equal(model.cells.length, 2, 'Cell count in notebook model is not correct'); + + // Set the first cell as active + model.updateActiveCell(model.cells[0]); + assert.deepEqual(model.activeCell, model.cells[0], 'Active cell does not match the first cell'); + assert.deepEqual(model.activeCell, activeCellFromEvent, 'Active cell returned from the event does not match'); + assert.equal(activeCellChangeCount, 1, 'Active cell change count is incorrect'); + assert(isUndefinedOrNull(notebookContentChange), 'Content change should be undefined'); + + + // Set the second cell as active + model.updateActiveCell(model.cells[1]); + assert.deepEqual(model.activeCell, model.cells[1], 'Active cell does not match expected value'); + assert.deepEqual(model.activeCell, activeCellFromEvent, 'Active cell returned from the event does not match (2nd)'); + assert.equal(activeCellChangeCount, 2, 'Active cell change count is incorrect; should be 2'); + + // Delete the active cell + model.deleteCell(model.cells[1]); + assert(isUndefinedOrNull(model.activeCell), 'Active cell should be undefined after active cell is deleted'); + assert.deepEqual(model.activeCell, activeCellFromEvent, 'Active cell should match value from event'); + assert.equal(activeCellChangeCount, 3, 'Active cell change count is incorrect; should be 3'); + + // Set the remaining cell as active + model.updateActiveCell(model.cells[0]); + assert.deepEqual(model.activeCell, activeCellFromEvent, 'Active cell should match value from event'); + assert.equal(activeCellChangeCount, 4, 'Active cell change count is incorrect; should be 4'); + + // Add new cell + let newCell = model.addCell(CellTypes.Code, 0); + + // Ensure new cell is active cell + assert.deepEqual(model.activeCell, newCell, 'Active cell does not match newly created cell'); + assert.equal(activeCellChangeCount, 5, 'Active cell change count is incorrect; should be 5'); + }); + + test('Should delete cells correctly', async function (): Promise { + // Given a notebook with 2 cells + let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); + mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); + defaultModelOptions.contentManager = mockContentManager.object; + + // When I initalize the model + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + await model.loadContents(); + + // Count number of times onError event is fired + let errorCount = 0; + let notebookContentChange: NotebookContentChange; + model.onError(() => errorCount++); + model.contentChanged(c => notebookContentChange = c); + + // Then I expect all cells to be in the model + assert.equal(model.cells.length, 2, 'Cell count in model is incorrect'); + + assert.equal(model.findCellIndex(model.cells[0]), 0, 'findCellIndex returned wrong cell info for first cell'); + assert.equal(model.findCellIndex(model.cells[1]), 1, 'findCellIndex returned wrong cell info for second cell'); + // Delete the first cell + model.deleteCell(model.cells[0]); + assert.equal(model.cells.length, 1, 'Cell model length should be 1 after cell deletion'); + assert.deepEqual(model.cells[0].source, expectedNotebookContent.cells[1].source, 'Expected cell source is incorrect'); + assert.equal(model.findCellIndex(model.cells[0]), 0, 'findCellIndex returned wrong cell info for only remaining cell'); + assert.equal(notebookContentChange.changeType, NotebookChangeType.CellsModified, 'notebookContentChange changeType is incorrect'); + assert.equal(notebookContentChange.isDirty, true, 'notebookContentChange should set dirty flag'); + assert.equal(model.activeCell, undefined, 'active cell is not undefined'); + + // Delete the remaining cell + notebookContentChange = undefined; + model.deleteCell(model.cells[0]); + assert.equal(model.cells.length, 0, 'There should be no cells tracked in the notebook model'); + assert.equal(model.findCellIndex(model.cells[0]), -1, 'findCellIndex is incorrectly finding a deleted cell'); + assert.equal(errorCount, 0, 'There should be no errors after deleting a cell that exists'); + assert.equal(notebookContentChange.changeType, NotebookChangeType.CellsModified, 'notebookContentChange changeType should indicate CellsModified'); + assert.equal(model.activeCell, undefined, 'Active cell should be undefined'); + + // Try deleting the cell again + notebookContentChange = undefined; + model.deleteCell(model.cells[0]); + assert.equal(errorCount, 1, 'The model should record an error after trying to delete a cell that does not exist'); + assert(isUndefinedOrNull(notebookContentChange), 'There should be no content change after an error is recorded'); + + // Try deleting as notebook model is in error state + notebookContentChange = undefined; + model.deleteCell(model.cells[0]); + assert.equal(errorCount, 2, 'Error count should be 2 after trying to delete a cell that does not exist a second time'); + assert(isUndefinedOrNull(notebookContentChange), 'There still should be no content change after an error is recorded'); + + }); + test('Should load contents but then go to error state if client session startup fails', async function (): Promise { let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContentOneCell)); @@ -234,38 +394,10 @@ suite('notebook model', function (): void { }); test('Should not be in error state if client session initialization succeeds', async function (): Promise { - let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); - mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); - defaultModelOptions.contentManager = mockContentManager.object; - let kernelChangedEmitter: Emitter = new Emitter(); - let statusChangedEmitter: Emitter = new Emitter(); + let model = await loadModelAndStartClientSession(); - mockClientSession.setup(c => c.isInErrorState).returns(() => false); - mockClientSession.setup(c => c.isReady).returns(() => true); - mockClientSession.setup(c => c.kernelChanged).returns(() => kernelChangedEmitter.event); - mockClientSession.setup(c => c.statusChanged).returns(() => statusChangedEmitter.event); - - queryConnectionService.setup(c => c.getActiveConnections(TypeMoq.It.isAny())).returns(() => null); - - sessionReady.resolve(); - let actualSession: IClientSession = undefined; - - let options: INotebookModelOptions = assign({}, defaultModelOptions, >{ - factory: mockModelFactory.object - }); - let model = new NotebookModel(options, undefined, logService, undefined, undefined); - model.onClientSessionReady((session) => actualSession = session); - await model.requestModelLoad(); - - await model.startSession(notebookManagers[0]); - - // Then I expect load to succeed - assert(!isUndefinedOrNull(model.clientSession)); - // but on server load completion I expect error state to be set - // Note: do not expect serverLoad event to throw even if failed - await model.sessionLoadFinished; assert.equal(model.inErrorState, false); - assert.deepEqual(actualSession, mockClientSession.object); + assert.equal(model.notebookManagers.length, 1); assert.deepEqual(model.clientSession, mockClientSession.object); }); @@ -301,4 +433,108 @@ suite('notebook model', function (): void { assert(!isUndefinedOrNull(actualChanged)); assert.equal(actualChanged.changeType, NotebookChangeType.TrustChanged); }); + + test('Should close active session when closed', async function () { + let model = await loadModelAndStartClientSession(); + // After client session is started, ensure session is ready + assert(model.isSessionReady); + + // After closing the notebook + await model.handleClosed(); + // Ensure client session is cleaned up + assert(isUndefinedOrNull(model.clientSession), 'clientSession is not cleaned up properly'); + // Ensure session is no longer ready + assert.equal(model.isSessionReady, false, 'session is incorrectly showing as ready'); + }); + + test('Should disconnect when connection profile created by notebook', async function () { + let model = await loadModelAndStartClientSession(); + // Ensure notebook prefix is present in the connection URI + queryConnectionService.setup(c => c.getConnectionUri(TypeMoq.It.isAny())).returns(() => `${uriPrefixes.notebook}some/path`); + await changeContextWithConnectionProfile(model); + + // After client session is started, ensure context isn't null/undefined + assert(!isUndefinedOrNull(model.context), 'context should exist after call to change context'); + + // After closing the notebook + await model.handleClosed(); + + // Ensure disconnect is called once + queryConnectionService.verify((c) => c.disconnect(TypeMoq.It.isAny()), TypeMoq.Times.once()); + }); + + test('Should not disconnect when connection profile not created by notebook', async function () { + let model = await loadModelAndStartClientSession(); + // Ensure notebook prefix isn't present in connection URI + queryConnectionService.setup(c => c.getConnectionUri(TypeMoq.It.isAny())).returns(() => `${uriPrefixes.default}some/path`); + await changeContextWithConnectionProfile(model); + + // After client session is started, ensure context isn't null/undefined + assert(!isUndefinedOrNull(model.context), 'context should exist after call to change context'); + + // After closing the notebook + await model.handleClosed(); + + // Ensure disconnect is never called + queryConnectionService.verify((c) => c.disconnect(TypeMoq.It.isAny()), TypeMoq.Times.never()); + + }); + + async function loadModelAndStartClientSession(): Promise { + let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); + mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); + defaultModelOptions.contentManager = mockContentManager.object; + let kernelChangedEmitter: Emitter = new Emitter(); + let statusChangedEmitter: Emitter = new Emitter(); + + mockClientSession.setup(c => c.isInErrorState).returns(() => false); + mockClientSession.setup(c => c.isReady).returns(() => true); + mockClientSession.setup(c => c.kernelChanged).returns(() => kernelChangedEmitter.event); + mockClientSession.setup(c => c.statusChanged).returns(() => statusChangedEmitter.event); + + queryConnectionService.setup(c => c.getActiveConnections(TypeMoq.It.isAny())).returns(() => null); + + sessionReady.resolve(); + let actualSession: IClientSession = undefined; + + let options: INotebookModelOptions = assign({}, defaultModelOptions, >{ + factory: mockModelFactory.object + }); + let model = new NotebookModel(options, undefined, logService, undefined, undefined); + model.onClientSessionReady((session) => actualSession = session); + await model.requestModelLoad(); + + await model.startSession(notebookManagers[0]); + + // Then I expect load to succeed + assert(!isUndefinedOrNull(model.clientSession), 'clientSession should exist after session is started'); + + assert.deepEqual(actualSession, mockClientSession.object, 'session returned is not the expected object'); + + // but on server load completion I expect error state to be set + // Note: do not expect serverLoad event to throw even if failed + await model.sessionLoadFinished; + return model; + } + + async function changeContextWithConnectionProfile(model: NotebookModel) { + let connection = new ConnectionProfile(capabilitiesService.object, { + connectionName: 'newName', + savePassword: false, + groupFullName: 'testGroup', + serverName: 'testServerName', + databaseName: 'testDatabaseName', + authenticationType: 'integrated', + password: 'test', + userName: 'testUsername', + groupId: undefined, + providerName: mssqlProviderName, + options: {}, + saveProfile: true, + id: 'testID' + }); + + await model.changeContext(connection.connectionName, connection); + } }); + diff --git a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts index cbfb07f560..ab4df12cc4 100644 --- a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts +++ b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts @@ -396,6 +396,9 @@ export class NotebookModel extends Disposable implements INotebookModel { let index = firstIndex(this._cells, (cell) => cell.equals(cellModel)); if (index > -1) { this._cells.splice(index, 1); + if (this._activeCell === cellModel) { + this.updateActiveCell(undefined); + } this._contentChangedEmitter.fire({ changeType: NotebookChangeType.CellsModified, cells: [cellModel], @@ -434,10 +437,6 @@ export class NotebookModel extends Disposable implements INotebookModel { return this._activeCell; } - public set activeCell(cell: ICellModel) { - this._activeCell = cell; - } - private notifyError(error: string): void { this._onErrorEmitter.fire({ message: error, severity: Severity.Error }); }