maint(web): re-add engine .js tests 🎼 by ermshiperete · Pull Request #15497 · keymanapp/keyman

ermshiperete · 2026-01-26T15:40:47Z

PR #15093 merged multiple engine modules into one. However, in doing so we lost the .js tests and ran only the .ts ones. This change re-adds the .js tests and fixes them where the code diverged since then.

Other changes:

rename queryEngine.ts to cloudQueryEngine.ts to match the class name
remove unused keyboard-storage/cloud/index.ts
expose CLOUD_TIMEOUT_ERR and CLOUD_STUB_REGISTRATION_ERR as unitTestEndpoints
add test/resources to exports in web/package.json. This was necessary because the compiled .ts test files are under web/build whereas the .js test files are under web/src and so the relative paths in imports no longer work. Also fix the imports in test files.
remove node-keyboard-loader.ts which was just a wrapper around NodeKeyboardLoader.
some cleanup and removal of default exports and aliases.
moved specialized-backspace.tests.js to specialized-backspace.tests.ts and fixed resulting issues/warnings.
apply fix from fix(web): fix problem with tests if KEYMAN_ROOT is not set 🎼 #15477 if KEYMAN_ROOT is not set
inline some consts so that we don't have to export them for unit testing
create getKeymanRoot and getWebTestResourcesPath helper functions to DRY out the code
run languageProcessortests.js without coverage to prevent a failure creating the coverage report

I'm not happy to put the compiled files for test/auto/resources under web/src/test/auto/resources/build, but that's the only way I got it to work. If I put the compiled files in web/build/test/resources it couldn't find the types for promise-status-async.

Follows: #15093
Test-bot: skip

keymanapp-test-bot · 2026-01-26T15:40:52Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
- KeyboardHarness apk - build : all tests passed (no artifacts on BuildLevel "build")
- KMSample1 apk - build : all tests passed (no artifacts on BuildLevel "build")
- KMSample2 apk - build : all tests passed (no artifacts on BuildLevel "build")
Developer
- Keyman Developer - build : all tests passed (no artifacts on BuildLevel "build")
- Compiler Regression Tests - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman Developer (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
iOS
- Keyman for iOS (simulator image) - build : all tests passed (no artifacts on BuildLevel "build")
- FirstVoices Keyboards for iOS (simulator image) - build : all tests passed (no artifacts on BuildLevel "build")
- FirstVoices Keyboards for iOS (simulator image) (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman for iOS (simulator image) (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
Keyboards
- Test Keyboards - build : all tests passed (no artifacts on BuildLevel "build")
Web
- KeymanWeb Test Home - build : all tests passed (no artifacts on BuildLevel "build")

PR #15093 merged multiple engine modules into one. However, in doing so we lost the .js tests and ran only the .ts ones. This change re-adds the .js tests and fixes them where the code diverged since then. Other changes: - rename `queryEngine.ts` to `cloudQueryEngine.ts` to match the class name - remove unused `keyboard-storage/cloud/index.ts` - expose `CLOUD_TIMEOUT_ERR` and `CLOUD_STUB_REGISTRATION_ERR` as `unitTestEndpoints` - add `test/resources` to exports in `web/package.json`. This was necessary because the compiled .ts test files are under `web/build` whereas the .js test files are under `web/src` and so the relative paths in imports no longer work. Also fix the imports in test files. - apply fix from #15477 if KEYMAN_ROOT is not set I'm not happy to put the compiled files under `web/src/test/auto/resources/build`, but that's the only way I got it to work. If I put the compiled files in `web/build/test/resources` it couldn't find the types for `promise-status-async`. Also, running the .js tests succeeded but then creating the coverage report failed. I was not able to find the reason or a fix for that. As a hack to work around this problem we check the number of failed tests instead of relying on the exit code of the test. Follows: #15093 Test-bot: skip

mcdurdin

I think we should have another go at sorting the locations of files. And a few more changes requested. Happy to rubber-ducky if that makes it easier.

mcdurdin · 2026-01-29T00:37:03Z

+import { CLOUD_TIMEOUT_ERR, CLOUD_STUB_REGISTRATION_ERR } from './cloud/cloudQueryEngine.js';
+
+export const unitTestEndpoints = {
+  CLOUD_TIMEOUT_ERR,
+  CLOUD_STUB_REGISTRATION_ERR
+};


This doesn't feel right. unitTestEndpoints are intended for targets for unit tests, not for sharing variables. There's no need for these to be exported for their usages in unit tests -- they are both used just one time in nodeCloudeRequester.ts (tests):

keyman/web/src/test/auto/resources/loader/nodeCloudRequester.ts

Lines 35 to 38 in 9930a8c

// Set callback timer

const timeoutObj = setTimeout(() => {

promise.reject(new Error(CLOUD_TIMEOUT_ERR));

}, 10000);

keyman/web/src/test/auto/resources/loader/nodeCloudRequester.ts

Lines 58 to 60 in 9930a8c

if(!promise.isResolved) {

promise.reject(new Error(CLOUD_STUB_REGISTRATION_ERR));

}

IMHO, those errors should just be plain strings in the unit testing code -- it's not deployed, there's no localization need, etc, etc. Just KISS. We can then eliminate the messiness of exporting these two constants.

Suggested change

import { CLOUD_TIMEOUT_ERR, CLOUD_STUB_REGISTRATION_ERR } from './cloud/cloudQueryEngine.js';

export const unitTestEndpoints = {

CLOUD_TIMEOUT_ERR,

CLOUD_STUB_REGISTRATION_ERR

};

ok, inlined those consts.

mcdurdin · 2026-01-29T00:39:43Z

+  # Unfortunately we get an error from the coverage report generation:
+  # "TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file"
+  # The following lines ignore the exit code and instead check the number
+  # of failed tests from the output.
+  set +e
+  OUTPUT_FILE=$(mktemp)
+  test-headless engine "" 2>&1 | tee "${OUTPUT_FILE}"
+  set -e
+
+  FAILURE_COUNT=$(grep ' failing' "${OUTPUT_FILE}" | xargs | cut -f 1 -d' ')
+  rm "${OUTPUT_FILE}"
+  builder_echo "(The 'TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file' is expected)"
+  if ((FAILURE_COUNT > 0)); then
+    builder_die "Headless engine tests failed (.js tests)"
+  fi


This doesn't seem right. We are running code coverage tests elsewhere just fine.

Turns out previously we didn't run the tests with coverage for engine/main. The reason c8 coverage report generation fails is a data:text/javascript URL coming from web/src/engine/predictive-text/worker-main/src/node/mappedWorker.ts. I changed it so that languageProcessor.tests.js which imports mappedWorker runs without test coverage.

mcdurdin · 2026-01-29T00:42:01Z

 import { env } from 'node:process';
-const KEYMAN_ROOT = env.KEYMAN_ROOT;
+import { fileURLToPath } from 'node:url';
+import { dirname } from 'node:path';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
+const KEYMAN_ROOT = env.KEYMAN_ROOT ?? (__dirname + '/../../../../../../../../');


Can we put this into a helper module in keyman/test/resources and avoid repeating it?

- inline consts so that we don't have to export them for unit testing - create `getKeymanRoot` and `getWebTestResourcesPath` helper functions to DRY out the code - run `languageProcessortests.js` without coverage to prevent a failure creating the coverage report.

Requesting re-review

mcdurdin

LGTM. Minor nits only

mcdurdin · 2026-03-23T09:22:52Z

    // Set callback timer
    const timeoutID = window.setTimeout(() => {
-      promise.reject(new Error(CLOUD_TIMEOUT_ERR));
+      promise.reject(new Error('The Cloud API request timed out.'));


Not sure why you are removing the constants?

mcdurdin · 2026-03-23T09:24:09Z

 import { PathConfiguration } from 'keyman/engine/interfaces';
-import NodeCloudRequester from '../../../resources/loader/nodeCloudRequester.js';

 import path from 'path';


Suggested change

import path from 'path';

import path from 'node:path';

Trying to make this more consistent over time

mcdurdin · 2026-03-23T09:24:41Z

@@ -2,20 +2,13 @@ import { assert } from 'chai';
 import sinon from 'sinon';
 import fs from 'fs';


Suggested change

import fs from 'fs';

import fs from 'node:fs';

Also see our import order at https://github.com/keymanapp/keyman/wiki/Keyman-Code-Style-Guide#imports-typescript-nodejs-etc

mcdurdin · 2026-03-23T09:25:35Z

@@ -1,5 +1,6 @@
 import { assert } from 'chai';

+import path from 'path';


Suggested change

import path from 'path';

import path from 'mode:path';

mcdurdin · 2026-03-23T09:26:56Z

+
+export function getKeymanRoot(): string {
+  const __dirname = dirname(fileURLToPath(import.meta.url));
+  return process.env['KEYMAN_ROOT'] ?? (__dirname + '/../../../../../../');


Should we just use:

Suggested change

return process.env['KEYMAN_ROOT'] ?? (__dirname + '/../../../../../../');

return __dirname + '/../../../../../../';

And never reference the KEYMAN_ROOT variable at all? I don't see any benefit to having both methods here.

Co-authored-by: Marc Durdin <marc@durdin.net>

For the files touched in this PR: - fix the order of the imports according to style guide - use `node:` prefix for node modules - add header

github-project-automation Bot added this to Keyman Jan 26, 2026

github-project-automation Bot moved this to Todo in Keyman Jan 26, 2026

keymanapp-test-bot Bot added the epic-web-core label Jan 26, 2026

keymanapp-test-bot Bot changed the title ~~maint(web): re-add engine .js tests~~ maint(web): re-add engine .js tests 🎼 Jan 26, 2026

keymanapp-test-bot Bot added this to the A19S21 milestone Jan 26, 2026

github-actions Bot added web/ maint Maintenance work -- continuous integration, build scripts, infrastructure labels Jan 26, 2026

ermshiperete force-pushed the maint/web/jstests branch from 39763fc to 677ca5f Compare January 26, 2026 16:51

ermshiperete requested a review from mcdurdin January 26, 2026 16:54

ermshiperete marked this pull request as ready for review January 26, 2026 16:54

ermshiperete force-pushed the maint/web/jstests branch from 677ca5f to e2b31c4 Compare January 26, 2026 17:06

mcdurdin previously requested changes Jan 29, 2026

View reviewed changes

keyman-server modified the milestones: A19S21, A19S22 Jan 31, 2026

keyman-server modified the milestones: A19S22, A19S23 Feb 13, 2026

keyman-server modified the milestones: A19S23, A19S24 Feb 27, 2026

keyman-server modified the milestones: A19S24, A19S25 Mar 14, 2026

ermshiperete requested a review from mcdurdin March 20, 2026 15:01

chore(web): Merge branch 'epic/web-core' into maint/web/jstests

745fa36

ermshiperete force-pushed the maint/web/jstests branch 2 times, most recently from 6ac2b79 to 745fa36 Compare March 20, 2026 17:26

mcdurdin approved these changes Mar 23, 2026

View reviewed changes

maint(web): address more code review comments

06f7f22

Co-authored-by: Marc Durdin <marc@durdin.net>

maint(web): cleanup imports

bbad490

For the files touched in this PR: - fix the order of the imports according to style guide - use `node:` prefix for node modules - add header

ermshiperete merged commit 66c5f12 into epic/web-core Mar 23, 2026
15 of 16 checks passed

ermshiperete deleted the maint/web/jstests branch March 23, 2026 17:15

github-project-automation Bot moved this from Todo to Done in Keyman Mar 23, 2026

	// Set callback timer
	const timeoutObj = setTimeout(() => {
	promise.reject(new Error(CLOUD_TIMEOUT_ERR));
	}, 10000);

	if(!promise.isResolved) {
	promise.reject(new Error(CLOUD_STUB_REGISTRATION_ERR));
	}

		@@ -2,20 +2,13 @@ import { assert } from 'chai';
		import sinon from 'sinon';
		import fs from 'fs';

		@@ -1,5 +1,6 @@
		import { assert } from 'chai';

		import path from 'path';

	return process.env['KEYMAN_ROOT'] ?? (__dirname + '/../../../../../../');
	return __dirname + '/../../../../../../';

Uh oh!

Conversation

ermshiperete commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keymanapp-test-bot Bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ermshiperete commented Jan 26, 2026 •

edited

Loading

keymanapp-test-bot Bot commented Jan 26, 2026 •

edited

Loading

ermshiperete Jan 29, 2026 •

edited

Loading