test(linux): Add unit tests for mcompile by Markus-SWAG · Pull Request #15892 · keymanapp/keyman

Markus-SWAG · 2026-04-28T15:40:47Z

This is a first try to add unit tests to mcompile.
There is a new file converter.cpp containing a minimal main. This allows the tests to have an own main provided by googletest.
Since gsettings are used, there is the need for new include files and new library dependencies.
The separation of converter.cpp required some refactoring of the header files.

keymanapp-test-bot · 2026-04-28T15:40:57Z

User Test Results

Test specification and instructions

User tests are not required

This is temporary, when the transition to xkblibcommon is done, wayland will be available.

ermshiperete

:+1 Looks good.

As we talked in the huddle, I'm not yet approving it so that it doesn't get accidentally merged while the tests won't work on the noble ba.

ermshiperete · 2026-04-30T12:47:36Z

+  include_directories : common_include_dir
 )
+
+gtest = subproject('gtest')


Since gtest is a dependency for the tests, I think it would better to have this line in test/meson.build (which works even if the subprojects folder is in mcompile/keymap).

ermshiperete · 2026-04-30T12:58:33Z

+  }
+
+
+  void get_default_layout() {


The function name get_default_layout suggests that it returns the default layout. A different name might be better (retrieve_default_layout? - not much better)

done, anyway this method will go away.

ermshiperete · 2026-04-30T13:05:04Z

+                     59, 60, 61, 123, 94};
+
+
+TEST_F(KeyboardConversionTest, KMXgetKeyValUnderlyingFromKeyCodeUnderlyingBase) {


We could consider making this (and the others) a parameterized test

good idea, done.

ermshiperete · 2026-04-30T13:05:52Z

+  int main(int argc, char* argv[]) {
+


nit:

Suggested change

int main(int argc, char* argv[]) {

int main(int argc, char* argv[]) {

ermshiperete · 2026-04-30T13:08:14Z

+  LPKMX_KEYBOARD kmxfile;
+
+  if (!KMX_LoadKeyboard(infile, &kmxfile)) {
+    KMX_LogError(L"Failed to load keyboard (%d)\n", errno);


Do we have to delete kmxfile here?

On failure of KMX_LoadKeyboard, kmxfile should still be nullptr.

ermshiperete · 2026-04-30T13:08:29Z

+
+  if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {  // I4552F
+    if(!KMX_SaveKeyboard(kmxfile, outfile)) {
+      KMX_LogError(L"Failed to save keyboard (%d)\n", errno);


Suggested change

KMX_LogError(L"Failed to save keyboard (%d)\n", errno);

KMX_LogError(L"Failed to save keyboard (%d)\n", errno);

delete kmxfile;

ermshiperete · 2026-04-30T13:10:56Z

+    return 3;
+  }
+
+  if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {  // I4552F


I think we can remove the I* numbers - they refer to a no-longer existing bug tracking system from earlier days of Keyman. For new code they don't make sense and should be omitted IMO.

Suggested change

if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) { // I4552F

if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {

Note that those issue numbers can be found at https://github.com/keymanapp/legacy-issues

left the numbers

ermshiperete · 2026-04-30T13:12:52Z

+ * @brief  print (error) messages
+ * @param  fmt text to print
+ */
+void KMX_LogError(const wchar_t* fmt, ...) {


Would it make more sense to use C++ style output in this function?

Definitely, I found a nice one liner, but this is only possible with C++ 20. So I suggest to wait until we are there.

darcywong00 · 2026-05-01T02:04:36Z

We have a convention for pull request titles

Reference

https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes#title

I suggest changing the title to something like test(linux): Add unit tests for mcompile

This way, the (linux) scope ensures this pull request title automatically gets added to the "Keyman for Linux changlog at
https://help.keyman.com/products/linux/version-history/

mcdurdin · 2026-05-04T08:30:30Z

Our filename convention for tests is tests/mcompile.tests.cpp

renamed to keymap.test.cpp, this is what is really tested

Should be keymap.tests.cpp (currently missing "s")

mcdurdin · 2026-05-04T08:31:46Z

@@ -0,0 +1,78 @@
+#include "mcompile.h"


Please add our standard header.

added to all files, where the header was missing.

mcdurdin · 2026-05-04T08:33:04Z

+    return 3;
+  }
+
+  if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {  // I4552F


Note that those issue numbers can be found at https://github.com/keymanapp/legacy-issues

mcdurdin · 2026-05-04T08:33:49Z

+  //
+  // 3. Write the new keyman keyboard file
+
+  LPKMX_KEYBOARD kmxfile;


Suggested change

LPKMX_KEYBOARD kmxfile;

LPKMX_KEYBOARD kmxfile = nullptr;

mcdurdin · 2026-05-04T08:34:00Z

+  LPKMX_KEYBOARD kmxfile;
+
+  if (!KMX_LoadKeyboard(infile, &kmxfile)) {
+    KMX_LogError(L"Failed to load keyboard (%d)\n", errno);


On failure of KMX_LoadKeyboard, kmxfile should still be nullptr.

mcdurdin · 2026-05-04T08:36:24Z

@@ -0,0 +1,299 @@
+#include <gtest/gtest.h>


Please add standard header comment

- adding header files - renaming files and methods - restructuring meson build

ermshiperete · 2026-05-06T10:03:39Z

Should be keymap.tests.cpp (currently missing "s")

ermshiperete · 2026-05-06T10:06:34Z

@@ -1,3 +1,11 @@
+/*
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Org got renamed from "SIL International" to "SIL Global" last year.

Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) SIL Global. MIT License.

According to the style guide, we don't put the year into the header because it just adds noise. Year can be found from the git history.

ermshiperete · 2026-05-06T10:09:01Z

@@ -1,3 +1,9 @@
+/*
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

ermshiperete · 2026-05-06T10:09:13Z

@@ -1,5 +1,5 @@
 /*
- * Keyman is copyright (C) 2004 - 2024 SIL International. MIT License.
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

ermshiperete · 2026-05-06T10:09:33Z

@@ -1,3 +1,11 @@
+/*
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

ermshiperete · 2026-05-06T10:10:32Z

    }
  }

-  delete kmxfile;


I think we still need this.

done, back again

ermshiperete · 2026-05-06T10:10:40Z

@@ -1,3 +1,9 @@
+/*
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

done, years erased, too

ermshiperete · 2026-05-06T10:10:48Z

@@ -1,5 +1,5 @@
 /*
- * Keyman is copyright (C) 2004 - 2024 SIL International. MIT License.
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

done, years erased, too

ermshiperete · 2026-05-06T10:11:19Z

@@ -1,3 +1,9 @@
+/*
+ * Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.


Suggested change

* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

done, years erased, too

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

…-keyboard-support-unit-tests' into feat/linux/mnemonic-keyboard-support-unit-tests

ermshiperete · 2026-05-07T10:08:06Z

+      this->keycode = k;
+      this->expected_char = e;
+      this->layout = l;
+      this->shiftstate = s;


Is this necessary if we have keycode(k) etc above? Seems to be doppelt gemoppelt 😄

ermshiperete · 2026-05-07T10:10:09Z

+    }
+
+    guint get_keycode() {
+        return keycode;


nit: indentation is wrong. Per coding style we indent each level by 2. The .editorconfig file should give you that for free if the extension is installed in vscode.

Suggested change

return keycode;

return keycode;

ermshiperete · 2026-05-07T10:13:26Z

+      expected_keysyms = e;
+      layout = l;
+      shiftstate = s;


see comment above

ermshiperete · 2026-05-07T10:19:19Z

+      EXPECT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";
+      for (guint k = 0; k < keycodes.size() && k < expected_keysyms.size(); k++) {


I think it doesn't make sense to continue if the the two vectors have different size, so we could use ASSERT_EQ here. That would also simplify the for loop slightly. Also, since we're using a std vector it doesn't really make sense to use a Gnome type for k - doesn't really matter but will reduce dependency. (I'm not sure - is there a uint or should it be unsigned int).

Suggested change

EXPECT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";

for (guint k = 0; k < keycodes.size() && k < expected_keysyms.size(); k++) {

ASSERT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";

for (uint k = 0; k < keycodes.size(); k++) {

When ASSERT_* fails, doesn't it prevent all further instantiations from running?

ermshiperete · 2026-05-07T12:50:50Z

+INSTANTIATE_TEST_SUITE_P(AltGrUs,
+                         GetKeyValUnderlyingFromKeyCodeUnderlyingTest,
+                         testing::ValuesIn(KeyboardTestParameters(
+                              {u'a', u'b', u'c', u'd', u'e', u'f', u'g', u'h', u'i', u'j', u'k',


It would be good to break the lines at the same place for all the tests so that it's easier to see the differences in the data between tests.

ermshiperete · 2026-05-07T12:52:39Z

+      keycode = k;
+      expected_char = e;
+      layout = l;
+      shiftstate = s;


ermshiperete · 2026-05-07T12:55:09Z

+
+};
+
+class GetKeyValFromKeyCodeTestParameters {


What's the difference to KeyboardTestParameters ?

There is one more element for caps.

- formatting - improved constructors

ermshiperete

LGTM

ermshiperete · 2026-05-08T08:23:36Z

+    AltGrDe,
+    GetKeyValUnderlyingFromKeyCodeUnderlyingTest,
+    testing::ValuesIn(KeyboardTestParameters(
+                          {u'æ',      u'\xfffe', u'¢',      u'ð',      u'\xfffe', u'\xfffe', u'\xfffe', u'\xfffe', u'\xfffe', u'\xffff',


Why is it that we have u'\xffff' for some values and u'\xfffe' or u'\000' for others?

ermshiperete · 2026-05-08T08:25:01Z

+    expected_keysyms = e;
+    layout           = l;
+    shiftstate       = s;
+    caps             = c;


Suggested change

expected_keysyms = e;

layout = l;

shiftstate = s;

caps = c;

ermshiperete

Oops, forgot to check devin.ai! That found a few issues that might cause problems in the future.

ermshiperete · 2026-05-08T08:44:28Z

+giomm_dep   = dependency('giomm-2.4')
+glibmm_dep  = dependency('glibmm-2.4')


Since these are only used in tests we should probably move them to test/meson.build - unless you'll need them to be here in a later PR.

Flagged by devin.ai:

giomm/glibmm dependencies fetched unconditionally despite only being needed for tests

In linux/mcompile/keymap/meson.build:12-13, giomm-2.4 and glibmm-2.4 are declared as unconditional dependencies at configure time, even though they are only used in the test executable (test/meson.build:9). This means the main mcompile build will fail at configure time if these packages are not installed, even though the main binary doesn't need them. Consider wrapping these in a required: false or conditionalizing on whether tests are enabled, or moving the dependency declarations into test/meson.build.

ermshiperete · 2026-05-08T08:56:33Z

@@ -582,3 +586,28 @@ FILE* Open_File(const KMX_CHAR* filename, const KMX_CHAR* mode) {
  return fopen(cpath.c_str(), cmode.c_str());


Flagged by devin.ai:

Suggested change

return fopen(cpath.c_str(), cmode.c_str());

return fopen(filename, mode);

Hmm, looks GH has a problem displaying this change. What I try to suggest is only keeping the first line (line 582, return fopen(filename, mode);) and deleting the next 4 lines that are unreachable (lines 583-586).

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

adding unit tests for Linux mcompile

99baacb

Markus-SWAG requested review from SabineSIL, ermshiperete and mcdurdin April 28, 2026 15:40

keymanapp-test-bot Bot added the user-test-missing User tests have not yet been defined for the PR label Apr 28, 2026

keymanapp-test-bot Bot added this to the A19S28 milestone Apr 28, 2026

github-actions Bot added the linux/ label Apr 28, 2026

github-project-automation Bot added this to Keyman Apr 28, 2026

github-project-automation Bot moved this to Todo in Keyman Apr 28, 2026

Markus-SWAG added 3 commits April 28, 2026 18:50

adding dependency to libglibmm-2.4-dev

77acb6f

adding dependency to libglibmm-2.4-dev typo

c040314

Prevent running unit tests on Wayland

0ed8eb1

This is temporary, when the transition to xkblibcommon is done, wayland will be available.

keymanapp-test-bot Bot removed the user-test-missing User tests have not yet been defined for the PR label Apr 28, 2026

ermshiperete reviewed Apr 30, 2026

View reviewed changes

mcdurdin reviewed May 4, 2026

View reviewed changes

incorporating review results for adding unit tests

f309d90

- adding header files - renaming files and methods - restructuring meson build

Markus-SWAG changed the title ~~adding unit tests for Linux mcompile~~ test(linux): Add unit tests for mcompile May 5, 2026

ermshiperete reviewed May 6, 2026

View reviewed changes

parametrized unit tests for mcompile

8bd47e4

github-actions Bot added the test Any acceptance test issue label May 6, 2026

Markus-SWAG and others added 8 commits May 6, 2026 17:51

Update linux/mcompile/keymap/deadkey.h

d5accca

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

renamed files according test standards

6e38fc0

Merge remote-tracking branch 'refs/remotes/origin/feat/linux/mnemonic…

c05bba9

…-keyboard-support-unit-tests' into feat/linux/mnemonic-keyboard-support-unit-tests

substituting SIL International by SIL Global in all file headers

bb87e1d

erasing year from the file headers

0646808

added delete kmxfile as before

e3dd7f1

added 1 more file header

8ba6a13

fixing typo

9cf4e78

adding test suite for KMX_get_KeyVal_From_KeyCode

471c6c4

ermshiperete reviewed May 7, 2026

View reviewed changes

Markus-SWAG added 2 commits May 7, 2026 17:37

incorporating review results

61f1a2b

- formatting - improved constructors

reformatting test parameters

8803089

ermshiperete approved these changes May 8, 2026

View reviewed changes

ermshiperete requested changes May 8, 2026

View reviewed changes

keyman-server modified the milestones: A19S28, A19S29 May 11, 2026

Markus-SWAG and others added 6 commits May 11, 2026 16:15

German Unit test now alos successfully finishing

c9c45a0

Update linux/mcompile/keymap/test/meson.build

0ccc9af

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

Update linux/mcompile/keymap/mc_kmxfile.cpp

8cf0c41

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

Update linux/mcompile/keymap/deadkey.h

cc9e0be

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

Update linux/mcompile/keymap/main.cpp

7b367e9

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

changes after review

28b0c96

		59, 60, 61, 123, 94};


		TEST_F(KeyboardConversionTest, KMXgetKeyValUnderlyingFromKeyCodeUnderlyingBase) {

	int main(int argc, char* argv[]) {
	int main(int argc, char* argv[]) {

	KMX_LogError(L"Failed to save keyboard (%d)\n", errno);
	KMX_LogError(L"Failed to save keyboard (%d)\n", errno);
	delete kmxfile;

	if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) { // I4552F
	if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {

		@@ -1,3 +1,11 @@
		/*
		* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

	* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
	* Keyman is copyright (C) SIL Global. MIT License.

		@@ -1,3 +1,9 @@
		/*
		* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.

		EXPECT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";
		for (guint k = 0; k < keycodes.size() && k < expected_keysyms.size(); k++) {

		giomm_dep = dependency('giomm-2.4')
		glibmm_dep = dependency('glibmm-2.4')

		@@ -582,3 +586,28 @@ FILE* Open_File(const KMX_CHAR* filename, const KMX_CHAR* mode) {
		return fopen(cpath.c_str(), cmode.c_str());

	return fopen(cpath.c_str(), cmode.c_str());
	return fopen(filename, mode);

Uh oh!

Conversation

Markus-SWAG commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

User Test Results

Uh oh!

ermshiperete 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darcywong00 commented May 1, 2026

Reference

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Markus-SWAG May 5, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Markus-SWAG commented Apr 28, 2026 •

edited

Loading

keymanapp-test-bot Bot commented Apr 28, 2026 •

edited

Loading

Markus-SWAG May 5, 2026 •

edited

Loading