From 1dd6989747723967faddf868cf4e0614ae7372ce Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Wed, 11 Sep 2024 18:26:13 +0200 Subject: [PATCH] feat(web): add error handling and tests Addresses code review comments. --- web/src/engine/keyboard/src/index.ts | 4 +- .../src/keyboards/keyboardLoadError.ts | 27 ++++ .../src/keyboards/keyboardLoaderBase.ts | 26 +++- .../keyboards/loaders/domKeyboardLoader.ts | 31 ++-- .../keyboards/loaders/nodeKeyboardLoader.ts | 24 ++-- .../engine/keyboard/keyboard-loading.js | 94 ------------- .../engine/keyboard/keyboard-loading.tests.ts | 132 ++++++++++++++++++ 7 files changed, 221 insertions(+), 117 deletions(-) delete mode 100644 web/src/test/auto/headless/engine/keyboard/keyboard-loading.js create mode 100644 web/src/test/auto/headless/engine/keyboard/keyboard-loading.tests.ts diff --git a/web/src/engine/keyboard/src/index.ts b/web/src/engine/keyboard/src/index.ts index cc0e86db53..e430556ed6 100644 --- a/web/src/engine/keyboard/src/index.ts +++ b/web/src/engine/keyboard/src/index.ts @@ -3,8 +3,8 @@ export * from "./keyboards/defaultLayouts.js"; export { default as Keyboard } from "./keyboards/keyboard.js"; export * from "./keyboards/keyboard.js"; export { KeyboardHarness, KeyboardKeymanGlobal, MinimalCodesInterface, MinimalKeymanGlobal } from "./keyboards/keyboardHarness.js"; -export { default as KeyboardLoaderBase, } from "./keyboards/keyboardLoaderBase.js"; -export { KeyboardLoadErrorBuilder, KeyboardMissingError, KeyboardScriptError } from './keyboards/keyboardLoadError.js' +export { KeyboardLoaderBase } from "./keyboards/keyboardLoaderBase.js"; +export { KeyboardLoadErrorBuilder, KeyboardMissingError, KeyboardScriptError, KeyboardDownloadError } from './keyboards/keyboardLoadError.js' export { CloudKeyboardFont, internalizeFont, diff --git a/web/src/engine/keyboard/src/keyboards/keyboardLoadError.ts b/web/src/engine/keyboard/src/keyboards/keyboardLoadError.ts index 9a95c24ef2..d394be15bf 100644 --- a/web/src/engine/keyboard/src/keyboards/keyboardLoadError.ts +++ b/web/src/engine/keyboard/src/keyboards/keyboardLoadError.ts @@ -3,6 +3,8 @@ import { type KeyboardStub } from './keyboardLoaderBase.js'; export interface KeyboardLoadErrorBuilder { scriptError(err?: Error): void; missingError(err: Error): void; + missingKeyboardError(msg: string, err: Error): void; + keyboardDownloadError(msg: string, err: Error): void; } export class KeyboardScriptError extends Error { @@ -23,6 +25,15 @@ export class KeyboardMissingError extends Error { } } +export class KeyboardDownloadError extends Error { + public readonly cause; + + constructor(message: string, cause?: Error) { + super(message); + this.cause = cause; + } +} + export class UriBasedErrorBuilder implements KeyboardLoadErrorBuilder { readonly uri: string; @@ -35,10 +46,18 @@ export class UriBasedErrorBuilder implements KeyboardLoadErrorBuilder { return new KeyboardMissingError(msg, err); } + missingKeyboardError(msg: string, err: Error) { + return new KeyboardMissingError(msg, err); + } + scriptError(err: Error) { const msg = `Error registering the keyboard script at ${this.uri}; it may contain an error.`; return new KeyboardScriptError(msg, err); } + + keyboardDownloadError(msg: string, err: Error) { + return new KeyboardDownloadError(msg, err); + } } export class StubBasedErrorBuilder implements KeyboardLoadErrorBuilder { @@ -54,10 +73,18 @@ export class StubBasedErrorBuilder implements KeyboardLoadErrorBuilder { return new KeyboardMissingError(msg, err); } + missingKeyboardError(msg: string, err: Error) { + return new KeyboardMissingError(msg, err); + } + scriptError(err: Error) { const stub = this.stub; const msg = `Error registering the ${stub.name} keyboard for ${stub.langName}; keyboard script at ${stub.filename} may contain an error.`; return new KeyboardScriptError(msg, err); } + + keyboardDownloadError(msg: string, err: Error) { + return new KeyboardDownloadError(msg, err); + } } diff --git a/web/src/engine/keyboard/src/keyboards/keyboardLoaderBase.ts b/web/src/engine/keyboard/src/keyboards/keyboardLoaderBase.ts index 8024609c88..4c718d3e88 100644 --- a/web/src/engine/keyboard/src/keyboards/keyboardLoaderBase.ts +++ b/web/src/engine/keyboard/src/keyboards/keyboardLoaderBase.ts @@ -5,7 +5,7 @@ import { KeyboardLoadErrorBuilder, StubBasedErrorBuilder, UriBasedErrorBuilder } export type KeyboardStub = KeyboardProperties & { filename: string }; -export default abstract class KeyboardLoaderBase { +export abstract class KeyboardLoaderBase { private _harness: KeyboardHarness; public get harness(): KeyboardHarness { @@ -16,20 +16,38 @@ export default abstract class KeyboardLoaderBase { this._harness = harness; } + /** + * Load a keyboard from a remote or local URI. + * + * @param uri The URI of the keyboard to load. + * @returns A Promise that resolves to the loaded keyboard. + */ public loadKeyboardFromPath(uri: string): Promise { this.harness.install(); return this.loadKeyboardInternal(uri, new UriBasedErrorBuilder(uri)); } + /** + * Load a keyboard from keyboard stub. + * + * @param stub The stub of the keyboard to load. + * @returns A Promise that resolves to the loaded keyboard. + */ public loadKeyboardFromStub(stub: KeyboardStub) { this.harness.install(); return this.loadKeyboardInternal(stub.filename, new StubBasedErrorBuilder(stub)); } private async loadKeyboardInternal(uri: string, errorBuilder: KeyboardLoadErrorBuilder): Promise { - const blob = await this.loadKeyboardBlob(uri); + const blob = await this.loadKeyboardBlob(uri, errorBuilder); + + let script: string; + try { + script = await blob.text(); + } catch (e) { + throw errorBuilder.missingKeyboardError('The keyboard has an invalid encoding.', e); + } - const script = await blob.text(); if (script.startsWith('KXTS', 0)) { // KMX or LDML (KMX+) keyboard console.error("KMX keyboard loading is not yet implemented!"); @@ -40,7 +58,7 @@ export default abstract class KeyboardLoaderBase { return await this.loadKeyboardFromScript(script, errorBuilder); } - protected abstract loadKeyboardBlob(uri: string): Promise; + protected abstract loadKeyboardBlob(uri: string, errorBuilder: KeyboardLoadErrorBuilder): Promise; protected abstract loadKeyboardFromScript(scriptSrc: string, errorBuilder: KeyboardLoadErrorBuilder): Promise; } \ No newline at end of file diff --git a/web/src/engine/keyboard/src/keyboards/loaders/domKeyboardLoader.ts b/web/src/engine/keyboard/src/keyboards/loaders/domKeyboardLoader.ts index abfea28a91..2fba52a80e 100644 --- a/web/src/engine/keyboard/src/keyboards/loaders/domKeyboardLoader.ts +++ b/web/src/engine/keyboard/src/keyboards/loaders/domKeyboardLoader.ts @@ -4,8 +4,8 @@ import { default as Keyboard } from '../keyboard.js'; import { KeyboardHarness, MinimalKeymanGlobal } from '../keyboardHarness.js'; -import { default as KeyboardLoaderBase } from '../keyboardLoaderBase.js'; -import { KeyboardLoadErrorBuilder, KeyboardMissingError } from '../keyboardLoadError.js'; +import { KeyboardLoaderBase } from '../keyboardLoaderBase.js'; +import { KeyboardLoadErrorBuilder } from '../keyboardLoadError.js'; export class DOMKeyboardLoader extends KeyboardLoaderBase { public readonly element: HTMLIFrameElement; @@ -29,20 +29,35 @@ export class DOMKeyboardLoader extends KeyboardLoaderBase { this.performCacheBusting = cacheBust || false; } - protected async loadKeyboardBlob(uri: string): Promise { + protected async loadKeyboardBlob(uri: string, errorBuilder: KeyboardLoadErrorBuilder): Promise { if (this.performCacheBusting) { uri = this.cacheBust(uri); } - const response = await fetch(uri); - if (!response.ok) { - throw new KeyboardMissingError(`Cannot find the keyboard at ${uri}.`, new Error(`HTTP ${response.status} ${response.statusText}`)); + let response: Response; + try { + response = await fetch(uri); + } catch (e) { + throw errorBuilder.keyboardDownloadError(`Unable to fetch fetch keyboard at ${uri}`, e); + } + + if (!response.ok) { + throw errorBuilder.missingError(new Error(`HTTP ${response.status} ${response.statusText}`)); + } + + try { + return await response.blob(); + } catch (e) { + throw errorBuilder.keyboardDownloadError(`Unable to retrieve blob from keyboard at ${uri}`, e); } - return response.blob(); } protected async loadKeyboardFromScript(script: string, errorBuilder: KeyboardLoadErrorBuilder): Promise { - this.evalScriptInContext(script, this.harness._jsGlobal); + try { + this.evalScriptInContext(script, this.harness._jsGlobal); + } catch (e) { + throw errorBuilder.scriptError(e); + } const keyboard = this.harness.loadedKeyboard; this.harness.loadedKeyboard = null; return keyboard; diff --git a/web/src/engine/keyboard/src/keyboards/loaders/nodeKeyboardLoader.ts b/web/src/engine/keyboard/src/keyboards/loaders/nodeKeyboardLoader.ts index ca0874dfd8..a63181fe8b 100644 --- a/web/src/engine/keyboard/src/keyboards/loaders/nodeKeyboardLoader.ts +++ b/web/src/engine/keyboard/src/keyboards/loaders/nodeKeyboardLoader.ts @@ -1,12 +1,13 @@ +import vm from 'node:vm'; +import { readFile } from 'node:fs/promises'; + +import { globalObject } from '@keymanapp/web-utils'; + import { default as Keyboard } from '../keyboard.js'; import { KeyboardHarness, MinimalKeymanGlobal } from '../keyboardHarness.js'; -import { default as KeyboardLoaderBase } from '../keyboardLoaderBase.js'; +import { KeyboardLoaderBase } from '../keyboardLoaderBase.js'; import { KeyboardLoadErrorBuilder } from '../keyboardLoadError.js'; -import vm from 'vm'; -import { readFile } from 'fs/promises'; -import { globalObject } from '@keymanapp/web-utils'; - export class NodeKeyboardLoader extends KeyboardLoaderBase { constructor() constructor(harness: KeyboardHarness); @@ -25,13 +26,18 @@ export class NodeKeyboardLoader extends KeyboardLoaderBase { } } - protected async loadKeyboardBlob(uri: string): Promise { + protected async loadKeyboardBlob(uri: string, errorBuilder: KeyboardLoadErrorBuilder): Promise { // `fs` does not like 'file:///'; it IS "File System" oriented, after all, and wants a path, not a URI. if (uri.indexOf('file:///') == 0) { uri = uri.substring('file:///'.length); } - const buffer = await readFile(uri); + let buffer: Buffer; + try { + buffer = await readFile(uri); + } catch (err) { + throw errorBuilder.keyboardDownloadError(`Unable to read keyboard file at ${uri}`, err); + } return new Blob([buffer]); } @@ -40,12 +46,12 @@ export class NodeKeyboardLoader extends KeyboardLoaderBase { try { script = new vm.Script(scriptSrc); } catch (err) { - return Promise.reject(errorBuilder.missingError(err)); + throw errorBuilder.missingError(err); } try { script.runInContext(this.harness._jsGlobal); } catch (err) { - return Promise.reject(errorBuilder.scriptError(err)); + throw errorBuilder.scriptError(err); } const keyboard = this.harness.loadedKeyboard; diff --git a/web/src/test/auto/headless/engine/keyboard/keyboard-loading.js b/web/src/test/auto/headless/engine/keyboard/keyboard-loading.js deleted file mode 100644 index 0dd89a3614..0000000000 --- a/web/src/test/auto/headless/engine/keyboard/keyboard-loading.js +++ /dev/null @@ -1,94 +0,0 @@ -import { assert } from 'chai'; - -import { createRequire } from 'module'; -const require = createRequire(import.meta.url); - -import { KeyboardHarness, MinimalKeymanGlobal } from 'keyman/engine/keyboard'; -import { KeyboardInterface, Mock } from 'keyman/engine/js-processor'; -import { NodeKeyboardLoader } from 'keyman/engine/keyboard/node-keyboard-loader'; - -describe('Headless keyboard loading', function() { - const laoPath = require.resolve('@keymanapp/common-test-resources/keyboards/lao_2008_basic.js'); - const khmerPath = require.resolve('@keymanapp/common-test-resources/keyboards/khmer_angkor.js'); - const nonKeyboardPath = require.resolve('@keymanapp/common-test-resources/index.mjs'); - const ipaPath = require.resolve('@keymanapp/common-test-resources/keyboards/sil_ipa.js'); - // Common test suite setup. - - let device = { - formFactor: 'desktop', - OS: 'windows', - browser: 'native' - } - - describe('Minimal harness loading', () => { - it('successfully loads', async function() { - // -- START: Standard Recorder-based unit test loading boilerplate -- - let harness = new KeyboardHarness({}, MinimalKeymanGlobal); - let keyboardLoader = new NodeKeyboardLoader(harness); - let keyboard = await keyboardLoader.loadKeyboardFromPath(laoPath); - // -- END: Standard Recorder-based unit test loading boilerplate -- - - // Asserts that the harness's loading field is cleared once the load is complete. - assert.isNotOk(harness.loadedKeyboard); - - // Asserts that the `activeKeyboard` field was not set by the operation. - assert.isNotOk(harness.activeKeyboard); - - // This part provides assurance that the keyboard properly loaded. - assert.equal(keyboard.id, "Keyboard_lao_2008_basic"); - }); - - it('successfully loads (has variable stores)', async () => { - // -- START: Standard Recorder-based unit test loading boilerplate -- - let harness = new KeyboardHarness({}, MinimalKeymanGlobal); - let keyboardLoader = new NodeKeyboardLoader(harness); - let keyboard = await keyboardLoader.loadKeyboardFromPath(ipaPath); - // -- END: Standard Recorder-based unit test loading boilerplate -- - - // This part provides extra assurance that the keyboard properly loaded. - assert.equal(keyboard.id, "Keyboard_sil_ipa"); - }); - - it('cannot evaluate rules', async function() { - // -- START: Standard Recorder-based unit test loading boilerplate -- - let harness = new KeyboardHarness({}, MinimalKeymanGlobal); - let keyboardLoader = new NodeKeyboardLoader(harness); - let keyboard = await keyboardLoader.loadKeyboardFromPath(laoPath); - // -- END: Standard Recorder-based unit test loading boilerplate -- - - // Runs a blank KeyEvent through the keyboard's rule processing... - // but via separate harness configured with a different captured global. - let ruleHarness = new KeyboardInterface({}, MinimalKeymanGlobal); - ruleHarness.activeKeyboard = keyboard; - try { - ruleHarness.processKeystroke(new Mock(), keyboard.constructNullKeyEvent(device)); - assert.fail(); - } catch (err) { - // Drives home an important detail: the 'global' object is effectively - // closure-captured. (Similar constraints may occur when experimenting with - // 'sandboxed' keyboard loading in the DOM!) - assert.equal(err.message, 'k.KKM is not a function'); - } - }); - - it('accurately determines supported gesture types', async () => { - // -- START: Standard Recorder-based unit test loading boilerplate -- - let harness = new KeyboardHarness({}, MinimalKeymanGlobal); - let keyboardLoader = new NodeKeyboardLoader(harness); - let km_keyboard = await keyboardLoader.loadKeyboardFromPath(khmerPath); - // -- END: Standard Recorder-based unit test loading boilerplate -- - - // `khmer_angkor` - supports longpresses, but not flicks or multitaps. - - const desktopLayout = km_keyboard.layout('desktop'); - assert.isFalse(desktopLayout.hasFlicks); - assert.isFalse(desktopLayout.hasLongpresses); - assert.isFalse(desktopLayout.hasMultitaps); - - const mobileLayout = km_keyboard.layout('phone'); - assert.isFalse(mobileLayout.hasFlicks); - assert.isTrue(mobileLayout.hasLongpresses); - assert.isFalse(mobileLayout.hasMultitaps); - }); - }); -}); diff --git a/web/src/test/auto/headless/engine/keyboard/keyboard-loading.tests.ts b/web/src/test/auto/headless/engine/keyboard/keyboard-loading.tests.ts new file mode 100644 index 0000000000..a01d469e47 --- /dev/null +++ b/web/src/test/auto/headless/engine/keyboard/keyboard-loading.tests.ts @@ -0,0 +1,132 @@ +import { assert } from 'chai'; + +import { createRequire } from 'module'; +const require = createRequire(import.meta.url); + +import { KeyboardHarness, MinimalKeymanGlobal, KeyboardDownloadError, DeviceSpec, KeyboardMissingError } from 'keyman/engine/keyboard'; +import { KeyboardInterface, Mock } from 'keyman/engine/js-processor'; +import { NodeKeyboardLoader } from 'keyman/engine/keyboard/node-keyboard-loader'; + +// async function assertThrowsAsync(fn: () => Promise, message?: string): Promise; +async function assertThrowsAsync(fn: () => Promise, type?: any, message?: string): Promise { + if (typeof (type) === 'string') { + message = type; + type = undefined; + } + try { + await fn(); + assert.fail('Expected function to throw an error, but it did not.'); + } catch (err) { + if (type) { + assert.isTrue(err instanceof type, `Expected error to be of type ${type.name}, but got ${err.constructor.name}`); + } + if (message) { + assert.equal((err as Error).message, message); + } + } +} + +function assertThrows(fn: () => any, message?: string): void; +function assertThrows(fn: () => any, type?: any, message?: string): void { + if (typeof (type) === 'string') { + message = type; + type = undefined; + } + assert.throws(fn, type, message); +} + +describe('Headless keyboard loading', function() { + const laoPath = require.resolve('@keymanapp/common-test-resources/keyboards/lao_2008_basic.js'); + const khmerPath = require.resolve('@keymanapp/common-test-resources/keyboards/khmer_angkor.js'); + const nonKeyboardPath = require.resolve('@keymanapp/common-test-resources/index.mjs'); + const ipaPath = require.resolve('@keymanapp/common-test-resources/keyboards/sil_ipa.js'); + const nonExisting = '/does/not/exist.js'; + // Common test suite setup. + + const device = { + formFactor: DeviceSpec.FormFactor.Desktop, + OS: DeviceSpec.OperatingSystem.Windows, + browser: DeviceSpec.Browser.Native, + touchable: false + } + + describe('Minimal harness loading', () => { + it('successfully loads', async function() { + // -- START: Standard Recorder-based unit test loading boilerplate -- + const harness = new KeyboardInterface({}, MinimalKeymanGlobal); + const keyboardLoader = new NodeKeyboardLoader(harness); + const keyboard = await keyboardLoader.loadKeyboardFromPath(laoPath); + // -- END: Standard Recorder-based unit test loading boilerplate -- + + // Asserts that the harness's loading field is cleared once the load is complete. + assert.isNotOk(harness.loadedKeyboard); + + // Asserts that the `activeKeyboard` field was not set by the operation. + assert.isNotOk(harness.activeKeyboard); + + // This part provides assurance that the keyboard properly loaded. + assert.equal(keyboard.id, "Keyboard_lao_2008_basic"); + }); + + it('throws error when keyboard does not exist', async function () { + const harness = new KeyboardInterface({}, MinimalKeymanGlobal); + const keyboardLoader = new NodeKeyboardLoader(harness); + + await assertThrowsAsync(() => keyboardLoader.loadKeyboardFromPath(nonExisting), + KeyboardDownloadError, `Unable to read keyboard file at ${nonExisting}`); + }); + + it('throws error when keyboard is invalid', async function () { + const harness = new KeyboardInterface({}, MinimalKeymanGlobal); + const keyboardLoader = new NodeKeyboardLoader(harness); + + await assertThrowsAsync(() => keyboardLoader.loadKeyboardFromPath(nonKeyboardPath), + KeyboardMissingError, `Cannot find the keyboard at ${nonKeyboardPath}.`); + }); + + it('successfully loads (has variable stores)', async () => { + // -- START: Standard Recorder-based unit test loading boilerplate -- + const harness = new KeyboardInterface({}, MinimalKeymanGlobal); + const keyboardLoader = new NodeKeyboardLoader(harness); + const keyboard = await keyboardLoader.loadKeyboardFromPath(ipaPath); + // -- END: Standard Recorder-based unit test loading boilerplate -- + + // This part provides extra assurance that the keyboard properly loaded. + assert.equal(keyboard.id, "Keyboard_sil_ipa"); + }); + + it('cannot evaluate rules', async function() { + // -- START: Standard Recorder-based unit test loading boilerplate -- + const harness = new KeyboardHarness({}, MinimalKeymanGlobal); + const keyboardLoader = new NodeKeyboardLoader(harness); + const keyboard = await keyboardLoader.loadKeyboardFromPath(laoPath); + // -- END: Standard Recorder-based unit test loading boilerplate -- + + // Runs a blank KeyEvent through the keyboard's rule processing... + // but via separate harness configured with a different captured global. + const ruleHarness = new KeyboardInterface({}, MinimalKeymanGlobal); + ruleHarness.activeKeyboard = keyboard; + assertThrows(() => ruleHarness.processKeystroke(new Mock(), keyboard.constructNullKeyEvent(device)), 'k.KKM is not a function'); + }); + + it('accurately determines supported gesture types', async () => { + // -- START: Standard Recorder-based unit test loading boilerplate -- + const harness = new KeyboardHarness({}, MinimalKeymanGlobal); + const keyboardLoader = new NodeKeyboardLoader(harness); + const km_keyboard = await keyboardLoader.loadKeyboardFromPath(khmerPath); + // -- END: Standard Recorder-based unit test loading boilerplate -- + + // `khmer_angkor` - supports longpresses, but not flicks or multitaps. + + const desktopLayout = km_keyboard.layout(DeviceSpec.FormFactor.Desktop); + assert.isFalse(desktopLayout.hasFlicks); + assert.isFalse(desktopLayout.hasLongpresses); + assert.isFalse(desktopLayout.hasMultitaps); + + const mobileLayout = km_keyboard.layout(DeviceSpec.FormFactor.Phone); + assert.isFalse(mobileLayout.hasFlicks); + assert.isTrue(mobileLayout.hasLongpresses); + assert.isFalse(mobileLayout.hasMultitaps); + }); + }); +});