From 041f34bedaa4f08ba5e7b2a28dc4bb0be90a2462 Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Tue, 7 Jan 2020 17:32:51 -0800 Subject: [PATCH] Fix book provider load to not throw on startup (#8835) * Fix book provider load to not throw on startup * Move tests to stable * Rename method * Fix floating promises and broken test --- extensions/notebook/src/book/bookModel.ts | 37 ++-- extensions/notebook/src/book/bookTreeView.ts | 36 ++-- .../notebook/src/common/localizedConstants.ts | 2 +- .../notebook/src/test/book/book.test.ts | 193 +++++++++++------- 4 files changed, 153 insertions(+), 115 deletions(-) diff --git a/extensions/notebook/src/book/bookModel.ts b/extensions/notebook/src/book/bookModel.ts index 88e9b78b52..65c6d91d1f 100644 --- a/extensions/notebook/src/book/bookModel.ts +++ b/extensions/notebook/src/book/bookModel.ts @@ -41,28 +41,23 @@ export class BookModel implements azdata.nb.NavigationProvider { return this._allNotebooks; } - public async getTableOfContentFiles(workspacePath: string): Promise { - try { - let notebookConfig = vscode.workspace.getConfiguration(notebookConfigKey); - let maxDepth = notebookConfig[maxBookSearchDepth]; - // Use default value if user enters an invalid value - if (isNullOrUndefined(maxDepth) || maxDepth < 0) { - maxDepth = 5; - } else if (maxDepth === 0) { // No limit of search depth if user enters 0 - maxDepth = undefined; - } + public async getTableOfContentFiles(folderPath: string): Promise { + let notebookConfig = vscode.workspace.getConfiguration(notebookConfigKey); + let maxDepth = notebookConfig[maxBookSearchDepth]; + // Use default value if user enters an invalid value + if (isNullOrUndefined(maxDepth) || maxDepth < 0) { + maxDepth = 5; + } else if (maxDepth === 0) { // No limit of search depth if user enters 0 + maxDepth = undefined; + } - let p = path.join(workspacePath, '**', '_data', 'toc.yml').replace(/\\/g, '/'); - let tableOfContentPaths = await glob(p, { deep: maxDepth }); - if (tableOfContentPaths.length > 0) { - this._tableOfContentPaths = this._tableOfContentPaths.concat(tableOfContentPaths); - vscode.commands.executeCommand('setContext', 'bookOpened', true); - } else { - vscode.window.showErrorMessage(loc.errBookInitialize); - } - - } catch (error) { - console.log(error); + let p = path.join(folderPath, '**', '_data', 'toc.yml').replace(/\\/g, '/'); + let tableOfContentPaths = await glob(p, { deep: maxDepth }); + if (tableOfContentPaths.length > 0) { + this._tableOfContentPaths = this._tableOfContentPaths.concat(tableOfContentPaths); + vscode.commands.executeCommand('setContext', 'bookOpened', true); + } else { + throw new Error(loc.missingTocError); } } diff --git a/extensions/notebook/src/book/bookTreeView.ts b/extensions/notebook/src/book/bookTreeView.ts index e0a7db5b2e..eddce4b4e1 100644 --- a/extensions/notebook/src/book/bookTreeView.ts +++ b/extensions/notebook/src/book/bookTreeView.ts @@ -27,7 +27,6 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider = new vscode.EventEmitter(); private _openAsUntitled: boolean; public viewId: string; public books: BookModel[]; @@ -37,29 +36,24 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider a.uri.fsPath)).catch(e => console.error(e)); + this.initialize(workspaceFolders).catch(e => console.error(e)); this.viewId = view; this.prompter = new CodeAdapter(); } - private async initialize(bookPaths: string[]): Promise { + private async initialize(workspaceFolders: vscode.WorkspaceFolder[]): Promise { await vscode.commands.executeCommand('setContext', 'unsavedBooks', this._openAsUntitled); - await Promise.all(bookPaths.map(async (bookPath) => { - let book: BookModel = new BookModel(bookPath, this._openAsUntitled, this._extensionContext); - await book.initializeContents(); - this.books.push(book); - if (!this.currentBook) { - this.currentBook = book; + await Promise.all(workspaceFolders.map(async (workspaceFolder) => { + try { + await this.createAndAddBookModel(workspaceFolder.uri.fsPath); + } catch { + // no-op, not all workspace folders are going to be valid books } })); this._initializeDeferred.resolve(); } - public get onReadAllTOCFiles(): vscode.Event { - return this._onReadAllTOCFiles.event; - } - public get initialized(): Promise { return this._initializeDeferred.promise; } @@ -73,7 +67,7 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider book.bookPath === bookPath)[0]; bookViewer.reveal(this.currentBook.bookItems[0], { expand: vscode.TreeItemCollapsibleState.Expanded, focus: true, select: true }); @@ -84,6 +78,20 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { + const book: BookModel = new BookModel(bookPath, this._openAsUntitled, this._extensionContext); + await book.initializeContents(); + this.books.push(book); + if (!this.currentBook) { + this.currentBook = book; + } + } + async showPreviewFile(urlToOpen?: string): Promise { if (this.currentBook) { const bookRoot = this.currentBook.bookItems[0]; diff --git a/extensions/notebook/src/common/localizedConstants.ts b/extensions/notebook/src/common/localizedConstants.ts index 3d7f688b1f..9e463f7271 100644 --- a/extensions/notebook/src/common/localizedConstants.ts +++ b/extensions/notebook/src/common/localizedConstants.ts @@ -24,7 +24,7 @@ export const openNotebookCommand = localize('openNotebookCommand', "Open Noteboo export const openMarkdownCommand = localize('openMarkdownCommand', "Open Markdown"); export const openExternalLinkCommand = localize('openExternalLinkCommand', "Open External Link"); -export const errBookInitialize = localize('bookInitializeFailed', "Book initialize failed: Failed to recognize the structure."); +export const missingTocError = localize('bookInitializeFailed', "Failed to find a toc.yml."); export function missingFileError(title: string): string { return localize('missingFileError', "Missing file : {0}", title); } export function invalidTocFileError(error: string): string { return localize('InvalidError.tocFile', "{0}", error); } export function invalidTocError(title: string): string { return localize('Invalid toc.yml', "Error: {0} has an incorrect toc.yml file", title); } diff --git a/extensions/notebook/src/test/book/book.test.ts b/extensions/notebook/src/test/book/book.test.ts index 204780ef96..f11936e081 100644 --- a/extensions/notebook/src/test/book/book.test.ts +++ b/extensions/notebook/src/test/book/book.test.ts @@ -5,7 +5,6 @@ import * as vscode from 'vscode'; import * as should from 'should'; -import * as TypeMoq from 'typemoq'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as rimraf from 'rimraf'; @@ -13,6 +12,7 @@ import * as os from 'os'; import * as uuid from 'uuid'; import { BookTreeViewProvider } from '../../book/bookTreeView'; import { BookTreeItem } from '../../book/bookTreeItem'; +import { MockExtensionContext } from '../common/stubs'; export interface ExpectedBookItem { title: string; @@ -34,9 +34,13 @@ export function equalBookItems(book: BookTreeItem, expectedBook: ExpectedBookIte } } -describe('BookTreeViewProviderTests @UNSTABLE@', function() { +describe('BookTreeViewProviderTests', function() { - describe('BookTreeViewProvider.getChildren', function (): void { + describe('BookTreeViewProvider', () => { + + let mockExtensionContext: vscode.ExtensionContext; + let nonBookFolderPath: string; + let bookFolderPath: string; let rootFolderPath: string; let expectedNotebook1: ExpectedBookItem; let expectedNotebook2: ExpectedBookItem; @@ -45,18 +49,14 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { let expectedExternalLink: ExpectedBookItem; let expectedBook: ExpectedBookItem; - let mockExtensionContext: TypeMoq.IMock; - let bookTreeViewProvider: BookTreeViewProvider; - let book: BookTreeItem; - let notebook1: BookTreeItem; - this.beforeAll(async () => { - console.log('Generating random rootFolderPath...'); + mockExtensionContext = new MockExtensionContext(); rootFolderPath = path.join(os.tmpdir(), `BookTestData_${uuid.v4()}`); - console.log('Random rootFolderPath generated.'); - let dataFolderPath = path.join(rootFolderPath, '_data'); - let contentFolderPath = path.join(rootFolderPath, 'content'); - let configFile = path.join(rootFolderPath, '_config.yml'); + nonBookFolderPath = path.join(rootFolderPath, `NonBook`); + bookFolderPath = path.join(rootFolderPath, `Book`); + let dataFolderPath = path.join(bookFolderPath, '_data'); + let contentFolderPath = path.join(bookFolderPath, 'content'); + let configFile = path.join(bookFolderPath, '_config.yml'); let tableOfContentsFile = path.join(dataFolderPath, 'toc.yml'); let notebook1File = path.join(contentFolderPath, 'notebook1.ipynb'); let notebook2File = path.join(contentFolderPath, 'notebook2.ipynb'); @@ -93,8 +93,9 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { sections: [expectedNotebook1, expectedMarkdown, expectedExternalLink], title: 'Test Book' }; - console.log('Creating temporary folders and files...'); await fs.mkdir(rootFolderPath); + await fs.mkdir(bookFolderPath); + await fs.mkdir(nonBookFolderPath); await fs.mkdir(dataFolderPath); await fs.mkdir(contentFolderPath); await fs.writeFile(configFile, 'title: Test Book'); @@ -103,58 +104,94 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { await fs.writeFile(notebook2File, ''); await fs.writeFile(notebook3File, ''); await fs.writeFile(markdownFile, ''); - console.log('Temporary folders and files created.'); - mockExtensionContext = TypeMoq.Mock.ofType(); + }); + + it('should initialize correctly with empty workspace array', async () => { + const bookTreeViewProvider = new BookTreeViewProvider([], mockExtensionContext, false, 'bookTreeView'); + await bookTreeViewProvider.initialized; + }); + + it('should initialize correctly with workspace containing non-book path', async () => { let folder: vscode.WorkspaceFolder = { - uri: vscode.Uri.file(rootFolderPath), + uri: vscode.Uri.file(nonBookFolderPath), name: '', index: 0 }; - console.log('Creating BookTreeViewProvider...'); - bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext.object, false, 'bookTreeView'); - let tocRead = new Promise((resolve, reject) => bookTreeViewProvider.onReadAllTOCFiles(() => resolve())); - let errorCase = new Promise((resolve, reject) => setTimeout(() => resolve(), 5000)); - await Promise.race([tocRead, errorCase.then(() => { throw new Error('Table of Contents were not ready in time'); })]); - console.log('BookTreeViewProvider successfully created.'); + const bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext, false, 'bookTreeView'); + await bookTreeViewProvider.initialized; }); - it('should return all book nodes when element is undefined', async function (): Promise { - const children = await bookTreeViewProvider.getChildren(); - should(children).be.Array(); - should(children.length).equal(1); - book = children[0]; - should(book.title).equal(expectedBook.title); + it('should initialize correctly with workspace containing both book and non-book paths', async () => { + const book: vscode.WorkspaceFolder = { + uri: vscode.Uri.file(bookFolderPath), + name: '', + index: 0 + }; + const nonBook: vscode.WorkspaceFolder = { + uri: vscode.Uri.file(nonBookFolderPath), + name: '', + index: 0 + }; + const bookTreeViewProvider = new BookTreeViewProvider([book, nonBook], mockExtensionContext, false, 'bookTreeView'); + await bookTreeViewProvider.initialized; + should(bookTreeViewProvider.books.length).equal(1, 'Expected book was not initialized'); }); - it('should return all page nodes when element is a book', async function (): Promise { - const children = await bookTreeViewProvider.getChildren(book); - should(children).be.Array(); - should(children.length).equal(3); - notebook1 = children[0]; - const markdown = children[1]; - const externalLink = children[2]; - equalBookItems(notebook1, expectedNotebook1); - equalBookItems(markdown, expectedMarkdown); - equalBookItems(externalLink, expectedExternalLink); + describe('BookTreeViewProvider.getChildren', function (): void { + let bookTreeViewProvider: BookTreeViewProvider; + let book: BookTreeItem; + let notebook1: BookTreeItem; + + this.beforeAll(async () => { + let folder: vscode.WorkspaceFolder = { + uri: vscode.Uri.file(rootFolderPath), + name: '', + index: 0 + }; + bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext, false, 'bookTreeView'); + let errorCase = new Promise((resolve, reject) => setTimeout(() => resolve(), 5000)); + await Promise.race([bookTreeViewProvider.initialized, errorCase.then(() => { throw new Error('BookTreeViewProvider did not initialize in time'); })]); + }); + + it('should return all book nodes when element is undefined', async function (): Promise { + const children = await bookTreeViewProvider.getChildren(); + should(children).be.Array(); + should(children.length).equal(1); + book = children[0]; + should(book.title).equal(expectedBook.title); + }); + + it('should return all page nodes when element is a book', async function (): Promise { + const children = await bookTreeViewProvider.getChildren(book); + should(children).be.Array(); + should(children.length).equal(3); + notebook1 = children[0]; + const markdown = children[1]; + const externalLink = children[2]; + equalBookItems(notebook1, expectedNotebook1); + equalBookItems(markdown, expectedMarkdown); + equalBookItems(externalLink, expectedExternalLink); + }); + + it('should return all sections when element is a notebook', async function (): Promise { + const children = await bookTreeViewProvider.getChildren(notebook1); + should(children).be.Array(); + should(children.length).equal(2); + const notebook2 = children[0]; + const notebook3 = children[1]; + equalBookItems(notebook2, expectedNotebook2); + equalBookItems(notebook3, expectedNotebook3); + }); + + this.afterAll(async function () { + console.log('Removing temporary files...'); + if (fs.existsSync(rootFolderPath)) { + rimraf.sync(rootFolderPath); + } + console.log('Successfully removed temporary files.'); + }); }); - it('should return all sections when element is a notebook', async function (): Promise { - const children = await bookTreeViewProvider.getChildren(notebook1); - should(children).be.Array(); - should(children.length).equal(2); - const notebook2 = children[0]; - const notebook3 = children[1]; - equalBookItems(notebook2, expectedNotebook2); - equalBookItems(notebook3, expectedNotebook3); - }); - - this.afterAll(async function () { - console.log('Removing temporary files...'); - if (fs.existsSync(rootFolderPath)) { - rimraf.sync(rootFolderPath); - } - console.log('Successfully removed temporary files.'); - }); }); describe('BookTreeViewProvider.getTableOfContentFiles', function (): void { @@ -170,22 +207,21 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { let tableOfContentsFileIgnore = path.join(rootFolderPath, 'toc.yml'); await fs.mkdir(rootFolderPath); await fs.mkdir(dataFolderPath); - await fs.writeFile(tableOfContentsFile, ''); + await fs.writeFile(tableOfContentsFile, '- title: Notebook1\n url: /notebook1\n sections:\n - title: Notebook2\n url: /notebook2\n - title: Notebook3\n url: /notebook3\n- title: Markdown\n url: /markdown\n- title: GitHub\n url: https://github.com/\n external: true'); await fs.writeFile(tableOfContentsFileIgnore, ''); - let mockExtensionContext = TypeMoq.Mock.ofType(); + const mockExtensionContext = new MockExtensionContext(); folder = { uri: vscode.Uri.file(rootFolderPath), name: '', index: 0 }; - bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext.object, false, 'bookTreeView'); - let tocRead = new Promise((resolve, reject) => bookTreeViewProvider.onReadAllTOCFiles(() => resolve())); + bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext, false, 'bookTreeView'); let errorCase = new Promise((resolve, reject) => setTimeout(() => resolve(), 5000)); - await Promise.race([tocRead, errorCase.then(() => { throw new Error('Table of Contents were not ready in time'); })]); + await Promise.race([bookTreeViewProvider.initialized, errorCase.then(() => { throw new Error('BookTreeViewProvider did not initialize in time'); })]); }); - it('should ignore toc.yml files not in _data folder', function(): void { - bookTreeViewProvider.currentBook.getTableOfContentFiles(folder.uri.toString()); + it('should ignore toc.yml files not in _data folder', async () => { + await bookTreeViewProvider.currentBook.getTableOfContentFiles(rootFolderPath); for (let p of bookTreeViewProvider.currentBook.tableOfContentPaths) { should(p.toLocaleLowerCase()).equal(tableOfContentsFile.replace(/\\/g, '/').toLocaleLowerCase()); } @@ -199,40 +235,40 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { }); - describe('BookTreeViewProvider.getBooks', function (): void { + describe('BookTreeViewProvider.getBooks @UNSTABLE@', function (): void { let rootFolderPath: string; let configFile: string; let folder: vscode.WorkspaceFolder; let bookTreeViewProvider: BookTreeViewProvider; - let mockExtensionContext: TypeMoq.IMock; + let tocFile: string; this.beforeAll(async () => { rootFolderPath = path.join(os.tmpdir(), `BookTestData_${uuid.v4()}`); let dataFolderPath = path.join(rootFolderPath, '_data'); configFile = path.join(rootFolderPath, '_config.yml'); - let tableOfContentsFile = path.join(dataFolderPath, 'toc.yml'); + tocFile = path.join(dataFolderPath, 'toc.yml'); await fs.mkdir(rootFolderPath); await fs.mkdir(dataFolderPath); - await fs.writeFile(tableOfContentsFile, 'title: Test'); - mockExtensionContext = TypeMoq.Mock.ofType(); + await fs.writeFile(tocFile, 'title: Test'); + const mockExtensionContext = new MockExtensionContext(); folder = { uri: vscode.Uri.file(rootFolderPath), name: '', index: 0 }; - bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext.object, false, 'bookTreeView'); - let tocRead = new Promise((resolve, reject) => bookTreeViewProvider.onReadAllTOCFiles(() => resolve())); + bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext, false, 'bookTreeView'); let errorCase = new Promise((resolve, reject) => setTimeout(() => resolve(), 5000)); - await Promise.race([tocRead, errorCase.then(() => { throw new Error('Table of Contents were not ready in time'); })]); + await Promise.race([bookTreeViewProvider.initialized, errorCase.then(() => { throw new Error('BookTreeViewProvider did not initialize in time'); })]); }); - it('should show error message if config.yml file not found', function(): void { - bookTreeViewProvider.currentBook.readBooks(); + it('should show error message if config.yml file not found', async () => { + await bookTreeViewProvider.currentBook.readBooks(); should(bookTreeViewProvider.errorMessage.toLocaleLowerCase()).equal(('ENOENT: no such file or directory, open \'' + configFile + '\'').toLocaleLowerCase()); }); + it('should show error if toc.yml file format is invalid', async function(): Promise { await fs.writeFile(configFile, 'title: Test Book'); - bookTreeViewProvider.currentBook.readBooks(); + await bookTreeViewProvider.currentBook.readBooks(); should(bookTreeViewProvider.errorMessage).equal('Error: Test Book has an incorrect toc.yml file'); }); @@ -244,7 +280,7 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { }); - describe('BookTreeViewProvider.getSections', function (): void { + describe('BookTreeViewProvider.getSections @UNSTABLE@', function (): void { let rootFolderPath: string; let tableOfContentsFile: string; let bookTreeViewProvider: BookTreeViewProvider; @@ -271,16 +307,15 @@ describe('BookTreeViewProviderTests @UNSTABLE@', function() { await fs.writeFile(tableOfContentsFile, '- title: Notebook1\n url: /notebook1\n- title: Notebook2\n url: /notebook2'); await fs.writeFile(notebook2File, ''); - let mockExtensionContext = TypeMoq.Mock.ofType(); + const mockExtensionContext = new MockExtensionContext(); folder = { uri: vscode.Uri.file(rootFolderPath), name: '', index: 0 }; - bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext.object, false, 'bookTreeView'); - let tocRead = new Promise((resolve, reject) => bookTreeViewProvider.onReadAllTOCFiles(() => resolve())); + bookTreeViewProvider = new BookTreeViewProvider([folder], mockExtensionContext, false, 'bookTreeView'); let errorCase = new Promise((resolve, reject) => setTimeout(() => resolve(), 5000)); - await Promise.race([tocRead, errorCase.then(() => { throw new Error('Table of Contents were not ready in time'); })]); + await Promise.race([bookTreeViewProvider.initialized, errorCase.then(() => { throw new Error('BookTreeViewProvider did not initialize in time'); })]); }); it('should show error if notebook or markdown file is missing', async function(): Promise {