[Tests] Increase QMK test coverage #13789

KarlK90 · 2021-07-30T11:52:58Z

Description

This PR enhances the current ~~unit-testing~~ integration testing facilities and adds additional tests:

All tests use an isolated per-test keymap instead of a global shared keymap in keymap.c the usage goes like this:

/* Define keys like this: layer, column, row, keycode */
auto       key_a      = KeymapKey(0, 0, 0, KC_A);
auto       key_lshift = KeymapKey(0, 1, 1, KC_LSFT);

/* Add keys to keymap for this test */
set_keymap({key_a, key_lshift});

key_a.press();
key_a.release();
key_lshift.press();
key_lshift.release();

Tests that fail provide a lot more context on the events that happened during the execution.
Tests were added:
- basic key-presses
- mod-taps and layer taps with different tap-hold configurations
- one-shot mods and layers

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

KarlK90 · 2021-08-14T14:29:32Z

This is now ready for review and a good starting point to add more test-cases.

I haven't figured out a good way to run the different configurations in one go as this requires a change to the way the tests are compiled and called by the make system. Maybe one of the reviewers has a killer idea? The way QMK currently works would require several differently configured testing binaries to be build and run. For instance one with the default mod-tap behavior, one with mod-tap-interrupt only, one with force-tap-hold etc.

KarlK90 · 2021-08-18T18:36:29Z

Every QMK configuration that needs a different binary is now split out into an individual testing binary, so all tests can be run simultaneously. 💪

drashna · 2021-09-27T18:44:33Z

Can the "in progress" label be removed here?

KarlK90 · 2021-09-27T18:52:02Z

Yes it can!

drashna · 2021-09-27T20:28:39Z

Looks like this is erroring out on:

Unexpected mock function call - returning directly.
    Function call: send_keyboard_mock(@0x5582251ce890 Keyboard Report: Mods (0) Keys ()
)
Google Mock tried the following 2 expectations, but none matched:

tests/basic/test_action_layer.cpp:286: tried expectation #0: EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_RALT)))...
  Expected arg #0: is equal to Keyboard Report: Mods (64) Keys ()

           Actual: Keyboard Report: Mods (0) Keys ()

         Expected: to be called once
           Actual: never called - unsatisfied and active
tests/basic/test_action_layer.cpp:287: tried expectation #1: EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_RALT, KC_9)))...

Running this from the docker container.

KarlK90 · 2021-09-27T21:32:45Z

Yes, this is expected, as the test documents false buggy behavior and was added in d50dcaf.

drashna · 2021-09-27T22:08:52Z

Well, specifically it is causing one of the tests to fail, which is my concern.

[  PASSED  ] 49 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ActionLayer.LayerTapReleasedBeforeKeypressReleaseWithModifiers

KarlK90 · 2021-09-27T22:28:03Z

Yup, that is the one I added in the commit. It is an edge-case which doesnt happen to often on ansi-us layouts and the like. My reasoning was that I rather document the expected behaviour then what QMK currently does. Which is removing the modifier from the report on a layer change, altough it should not be modified because the key adding the modifier is still pressed. This results on a german layout in ((((<layer release>8888 instead of ((((<layer release>((((

tmk_core/native.mk

zvecr · 2021-10-21T03:29:33Z

tests/test_common/test_logger.cpp

+
+void TestLogger::reset() { this->m_log.str(""); };
+
+void TestLogger::print_log() { std::cerr << this->m_log.str(); }


It would be nice if this somehow took advantage of #13556, rather than it being its own entity.

Maybe binding int8_t sendchar(uint8_t c) to print to the same buffer, or using the debug macros instead of piping to std::cerr?

From a quick skim of the PR, the high level is that you want to only dump log on errors? I could potentially see it being none/errors/all, with a nicer interface than the current DEBUG=.

tzarc · 2021-11-01T17:38:30Z