From 9285eec8adb51d70d0f8580721dc7aab10bfcf85 Mon Sep 17 00:00:00 2001 From: dvirtz Date: Tue, 29 Aug 2023 23:23:43 +0100 Subject: [PATCH] fix: fix infinite load on error now code won't wait for user to close error message and will display the error in the json doc fixed #85 --- src/parquet-document-provider.ts | 7 ++- src/parquet-document.ts | 28 ++++++------ .../parquet-editor-provider.test.ts | 43 ++++++++++++------ test/unit/backend.test.ts | 5 +- test/unit/parquets.test.ts | 13 ++++++ test/unit/workspace.ts | 4 ++ test/workspace/.vscode/settings.json | 3 +- test/workspace/version_2.json | 3 ++ test/workspace/version_2.parquet | Bin 0 -> 243 bytes 9 files changed, 71 insertions(+), 35 deletions(-) create mode 100644 test/unit/parquets.test.ts create mode 100644 test/unit/workspace.ts create mode 100644 test/workspace/version_2.json create mode 100644 test/workspace/version_2.parquet diff --git a/src/parquet-document-provider.ts b/src/parquet-document-provider.ts index 0c32929..f9d93eb 100644 --- a/src/parquet-document-provider.ts +++ b/src/parquet-document-provider.ts @@ -26,12 +26,11 @@ export class ParquetTextDocumentContentProvider implements vscode.TextDocumentCo async provideTextDocumentContent(uri: vscode.Uri): Promise { // already loaded? - const document = this._documents.get(uri) || (await (async _ => { + if (!this._documents.has(uri)) { const document = await ParquetDocument.create(uri, this._onDidChange); this._documents.set(uri, document); - return document; - })()); + } - return document.value; + return this._documents.get(uri)?.value; } } diff --git a/src/parquet-document.ts b/src/parquet-document.ts index fc7a5d6..7361be3 100644 --- a/src/parquet-document.ts +++ b/src/parquet-document.ts @@ -1,7 +1,6 @@ import * as os from 'os'; import * as path from 'path'; import * as vscode from 'vscode'; -import { assert } from 'console'; import { promises } from 'fs'; import { getLogger } from './logger'; import { createParquetBackend } from './parquet-backend-factory'; @@ -34,26 +33,25 @@ export default class ParquetDocument implements vscode.Disposable { } } - public static async create(uri: vscode.Uri, emitter: vscode.EventEmitter): Promise { - try { - const parquet = new ParquetDocument(uri, emitter); - await parquet.populate(); - return parquet; - } catch (error) { - const message = `while reading ${uri}: ${error}`; - getLogger().error(message); - await vscode.window.showErrorMessage(`${error}`); - throw Error(message); - } + public static async create(uri: vscode.Uri, emitter: vscode.EventEmitter) { + const parquet = new ParquetDocument(uri, emitter); + await parquet.tryPopulate(); + return parquet; } get value() { return this._lines.join(os.EOL) + os.EOL; } - private tryPopulate(uri: vscode.Uri) { - assert(uri.fsPath == this._parquetPath); - this.populate().catch(error => getLogger().warn(`failed to populate ${this._parquetPath}: ${error}`)) + private async tryPopulate() { + try { + await this.populate(); + } catch (error) { + const message = `while reading ${this._parquetPath}: ${error}`; + getLogger().error(message); + void vscode.window.showErrorMessage(message); + this._lines.push(JSON.stringify({error: message})); + } } private async populate() { diff --git a/test/integration/parquet-editor-provider.test.ts b/test/integration/parquet-editor-provider.test.ts index 5980eda..4ce9d42 100644 --- a/test/integration/parquet-editor-provider.test.ts +++ b/test/integration/parquet-editor-provider.test.ts @@ -58,18 +58,18 @@ describe('ParquetEditorProvider', function () { ['small', 'parquet-tools'], ['small', 'parquets'], ['small', 'arrow'], - ['large', 'parquets'] + ['large', 'parquets'], + ['version_2', 'arrow'] ])('shows %p using %p', async function (name, backend) { const parquet = await getUri(`${name}.parquet`); testFile = await copyTo(`${name}-${backend}.parquet`, parquet); - const checkChanged = new Promise(resolve => { + const checkChanged = new Promise((resolve, reject) => { const callback = jest.fn(async function (editor?: vscode.TextEditor) { expect(editor?.document.fileName).toBe(`${testFile.fsPath}.as.json`); const expected = await readFile(`${name}.json`); expect(editor?.document.getText()).toEqual(expected); - resolve(callback); }); - disposables.push(vscode.window.onDidChangeActiveTextEditor(callback)); + disposables.push(vscode.window.onDidChangeActiveTextEditor(editor => callback(editor).then(_ => resolve(callback)).catch(reject))); }); jest.mocked(settings.backend).mockReturnValue(backend); const document = await editorProvider.openCustomDocument(testFile); @@ -89,23 +89,40 @@ describe('ParquetEditorProvider', function () { expect(document.getText()).toEqual(expected); }); - const checkSmall = new Promise(resolve => - disposables.push(workspace.onDidOpenTextDocument(async document => { - await checkChanged(document, 'small'); - resolve(checkChanged); - }))); + const checkSmall = new Promise((resolve, reject) => + disposables.push(workspace.onDidOpenTextDocument(document => + checkChanged(document, 'small').then(_ => resolve(checkChanged)).catch(reject) + )) + ); await (await editorProvider.openCustomDocument(testFile)).open(); await expect(checkSmall).resolves.toBeCalledWith(expect.anything(), 'small'); // make sure checkSmall is not called again disposables.pop()?.dispose(); - const checkLarge = new Promise(resolve => disposables.push(workspace.onDidChangeTextDocument(async event => { - await checkChanged(event.document, 'large'); - resolve(checkChanged); - }))); + const checkLarge = new Promise((resolve, reject) => + disposables.push(workspace.onDidChangeTextDocument(event => + checkChanged(event.document, 'large').then(_ => resolve(checkChanged)).catch(reject) + )) + ); await fs.copy(await getUri('large.parquet'), testFile, { overwrite: true }); await expect(checkLarge).resolves.toHaveBeenLastCalledWith(expect.anything(), 'large'); }); + + test('handles error', async function () { + const parquet = await getUri(`version_2.parquet`); + testFile = await copyTo(`version_2-parquets.parquet`, parquet); + const checkChanged = new Promise((resolve, reject) => { + const callback = jest.fn(async function (editor?: vscode.TextEditor) { + expect(editor?.document.fileName).toBe(`${testFile.fsPath}.as.json`); + expect(editor?.document.getText()).toMatch(new RegExp(`{\\"error\\":\\"while reading ${testFile.fsPath.replace(/\\/g, '\\\\\\\\')}: .*\\"}`)); + }); + disposables.push(vscode.window.onDidChangeActiveTextEditor(editor => callback(editor).then(_ => resolve(callback)).catch(reject))); + }); + jest.mocked(settings.backend).mockReturnValue('parquets'); + const document = await editorProvider.openCustomDocument(testFile); + await document.open(); + await expect(checkChanged).resolves.toBeCalled(); + }); }); diff --git a/test/unit/backend.test.ts b/test/unit/backend.test.ts index ca412a5..ba76d8d 100644 --- a/test/unit/backend.test.ts +++ b/test/unit/backend.test.ts @@ -7,17 +7,17 @@ import { CancellationToken } from 'vscode'; import { BackendNames } from '../../src/backend-name'; import { createParquetBackend } from '../../src/parquet-backend-factory'; import * as vscode from 'vscode'; +import { workspace } from './workspace'; -const rootDir = path.join(__dirname, '..', '..'); describe.each(BackendNames)("%s backend tests", (backendName) => { const backend = createParquetBackend(backendName); - const workspace = path.join(rootDir, 'test', 'workspace'); const testFiles = { 'parquet-tools': [ ['small', 'small'], ['large', 'large'], + ['version_2', 'version_2'] ], 'parquets': [ ['small', 'small'], @@ -26,6 +26,7 @@ describe.each(BackendNames)("%s backend tests", (backendName) => { 'arrow': [ ['small', 'small'], ['large', 'large.arrow'], + ['version_2', 'version_2'] ] }; diff --git a/test/unit/parquets.test.ts b/test/unit/parquets.test.ts new file mode 100644 index 0000000..2ca1ea3 --- /dev/null +++ b/test/unit/parquets.test.ts @@ -0,0 +1,13 @@ +import toArray from '@async-generators/to-array'; +import { describe, expect, test } from '@jest/globals'; +import * as path from 'path'; +import { ParquetsBackend } from "../../src/parquets-backend"; +import { workspace } from './workspace'; + +describe("ParquetsBackend tests", () => { + const backend = new ParquetsBackend(); + + test("throws on invalid parquet version", async function () { + await expect(toArray(backend.toJson(path.join(workspace, 'version_2.parquet')))).rejects.toThrow('invalid parquet version'); + }); +}); diff --git a/test/unit/workspace.ts b/test/unit/workspace.ts new file mode 100644 index 0000000..056d27f --- /dev/null +++ b/test/unit/workspace.ts @@ -0,0 +1,4 @@ +import * as path from 'path'; + +const rootDir = path.join(__dirname, '..', '..'); +export const workspace = path.join(rootDir, 'test', 'workspace'); diff --git a/test/workspace/.vscode/settings.json b/test/workspace/.vscode/settings.json index effccbf..9707b58 100644 --- a/test/workspace/.vscode/settings.json +++ b/test/workspace/.vscode/settings.json @@ -1,4 +1,5 @@ { "parquet-viewer.parquetToolsPath": "parquet-tools-1.12.0-SNAPSHOT.jar", - "parquet-viewer.logging.level": "debug" + "parquet-viewer.logging.level": "debug", + "parquet-viewer.logging.panel": true } \ No newline at end of file diff --git a/test/workspace/version_2.json b/test/workspace/version_2.json new file mode 100644 index 0000000..1e63657 --- /dev/null +++ b/test/workspace/version_2.json @@ -0,0 +1,3 @@ +{"a":1} +{"a":2} +{"a":3} diff --git a/test/workspace/version_2.parquet b/test/workspace/version_2.parquet new file mode 100644 index 0000000000000000000000000000000000000000..8bef9b2be788e4b4af293ed8f0e38f2f80e80f46 GIT binary patch literal 243 zcmaJ+u?oUK49#7Sqhk?ME^tE!JsfIjhi)$J4&omaDe9orR{fZceyOjUlO&IjmzVIe z-ySM9?AYztGR=q-5osU-m^kJ*$0Px)cV=RpnKUWlw+E%81+kI5HqH*mb9-%A6YC{8 z)lh21iw_^=SxcRe6|f9C9ThTcLj3=}eJldA*;7O-hUj4~%$$-U_xulBh