Improve test invocation, fix Retro Shift bugs, and add Auto+Retro Shi…

…ft test cases (qmk#15889)
christrotter · Nov 28, 2023 · 900ca86 · 900ca86
1 parent 4b81c9f
commit 900ca86
Show file tree

Hide file tree

Showing 48 changed files with 2,584 additions and 89 deletions.
diff --git a/Makefile b/Makefile
@@ -300,17 +300,18 @@ endef
 define BUILD_TEST
     TEST_PATH := $1
     TEST_NAME := $$(notdir $$(TEST_PATH))
+    TEST_FULL_NAME := $$(subst /,_,$$(patsubst $$(ROOT_DIR)tests/%,%,$$(TEST_PATH)))
     MAKE_TARGET := $2
     COMMAND := $1
     MAKE_CMD := $$(MAKE) -r -R -C $(ROOT_DIR) -f $(BUILDDEFS_PATH)/build_test.mk $$(MAKE_TARGET)
-    MAKE_VARS := TEST=$$(TEST_NAME) TEST_PATH=$$(TEST_PATH) FULL_TESTS="$$(FULL_TESTS)"
+    MAKE_VARS := TEST=$$(TEST_NAME) TEST_OUTPUT=$$(TEST_FULL_NAME) TEST_PATH=$$(TEST_PATH) FULL_TESTS="$$(FULL_TESTS)"
     MAKE_MSG := $$(MSG_MAKE_TEST)
     $$(eval $$(call BUILD))
     ifneq ($$(MAKE_TARGET),clean)
-        TEST_EXECUTABLE := $$(TEST_OUTPUT_DIR)/$$(TEST_NAME).elf
-        TESTS += $$(TEST_NAME)
+        TEST_EXECUTABLE := $$(TEST_OUTPUT_DIR)/$$(TEST_FULL_NAME).elf
+        TESTS += $$(TEST_FULL_NAME)
         TEST_MSG := $$(MSG_TEST)
-        $$(TEST_NAME)_COMMAND := \
+        $$(TEST_FULL_NAME)_COMMAND := \
             printf "$$(TEST_MSG)\n"; \
             $$(TEST_EXECUTABLE); \
             if [ $$$$? -gt 0 ]; \
@@ -322,15 +323,22 @@ endef
 
 define PARSE_TEST
     TESTS :=
-    TEST_NAME := $$(firstword $$(subst :, ,$$(RULE)))
-    TEST_TARGET := $$(subst $$(TEST_NAME),,$$(subst $$(TEST_NAME):,,$$(RULE)))
+    # list of possible targets, colon-delimited, to reassign to MAKE_TARGET and remove
+    TARGETS := :clean:
+    ifneq (,$$(findstring :$$(lastword $$(subst :, ,$$(RULE))):, $$(TARGETS)))
+        MAKE_TARGET := $$(lastword $$(subst :, ,$$(RULE)))
+        TEST_SUBPATH := $$(subst $$(eval) ,/,$$(wordlist 2, $$(words $$(subst :, ,$$(RULE))), _ $$(subst :, ,$$(RULE))))
+    else
+        MAKE_TARGET :=
+        TEST_SUBPATH := $$(subst :,/,$$(RULE))
+    endif
     include $(BUILDDEFS_PATH)/testlist.mk
-    ifeq ($$(TEST_NAME),all)
+    ifeq ($$(RULE),all)
         MATCHED_TESTS := $$(TEST_LIST)
     else
-        MATCHED_TESTS := $$(foreach TEST, $$(TEST_LIST),$$(if $$(findstring x$$(TEST_NAME)x, x$$(notdir $$(TEST))x), $$(TEST),))
+        MATCHED_TESTS := $$(foreach TEST, $$(TEST_LIST),$$(if $$(findstring /$$(TEST_SUBPATH)/, $$(patsubst %,%/,$$(TEST))), $$(TEST),))
     endif
-    $$(foreach TEST,$$(MATCHED_TESTS),$$(eval $$(call BUILD_TEST,$$(TEST),$$(TEST_TARGET))))
+    $$(foreach TEST,$$(MATCHED_TESTS),$$(eval $$(call BUILD_TEST,$$(TEST),$$(MAKE_TARGET))))
 endef
 
 

diff --git a/builddefs/build_full_test.mk b/builddefs/build_full_test.mk
@@ -13,10 +13,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-$(TEST)_INC := \
+$(TEST_OUTPUT)_INC := \
 	tests/test_common/common_config.h
 
-$(TEST)_SRC := \
+$(TEST_OUTPUT)_SRC := \
 	$(QUANTUM_SRC) \
 	$(SRC) \
 	$(QUANTUM_PATH)/keymap_introspection.c \
@@ -30,8 +30,8 @@ $(TEST)_SRC := \
 	tests/test_common/test_logger.cpp \
 	$(patsubst $(ROOTDIR)/%,%,$(wildcard $(TEST_PATH)/*.cpp))
 
-$(TEST)_DEFS := $(OPT_DEFS) "-DKEYMAP_C=\"keymap.c\""
+$(TEST_OUTPUT)_DEFS := $(OPT_DEFS) "-DKEYMAP_C=\"keymap.c\""
 
-$(TEST)_CONFIG := $(TEST_PATH)/config.h
+$(TEST_OUTPUT)_CONFIG := $(TEST_PATH)/config.h
 
 VPATH += $(TOP_DIR)/tests/test_common
diff --git a/builddefs/build_test.mk b/builddefs/build_test.mk
@@ -9,13 +9,13 @@ OPT = g
 include paths.mk
 include $(BUILDDEFS_PATH)/message.mk
 
-TARGET=test/$(TEST)
+TARGET=test/$(TEST_OUTPUT)
 
 GTEST_OUTPUT = $(BUILD_DIR)/gtest
 
 TEST_OBJ = $(BUILD_DIR)/test_obj
 
-OUTPUTS := $(TEST_OBJ)/$(TEST) $(GTEST_OUTPUT)
+OUTPUTS := $(TEST_OBJ)/$(TEST_OUTPUT) $(GTEST_OUTPUT)
 
 GTEST_INC := \
 	$(LIB_PATH)/googletest/googletest/include \
@@ -71,18 +71,18 @@ ifneq ($(filter $(FULL_TESTS),$(TEST)),)
 include $(BUILDDEFS_PATH)/build_full_test.mk
 endif
 
-$(TEST)_SRC += \
+$(TEST_OUTPUT)_SRC += \
 	tests/test_common/main.cpp \
 	$(QUANTUM_PATH)/logging/print.c
 
 ifneq ($(strip $(INTROSPECTION_KEYMAP_C)),)
-$(TEST)_DEFS += -DINTROSPECTION_KEYMAP_C=\"$(strip $(INTROSPECTION_KEYMAP_C))\"
+$(TEST_OUTPUT)_DEFS += -DINTROSPECTION_KEYMAP_C=\"$(strip $(INTROSPECTION_KEYMAP_C))\"
 endif
 
-$(TEST_OBJ)/$(TEST)_SRC := $($(TEST)_SRC)
-$(TEST_OBJ)/$(TEST)_INC := $($(TEST)_INC) $(VPATH) $(GTEST_INC)
-$(TEST_OBJ)/$(TEST)_DEFS := $($(TEST)_DEFS)
-$(TEST_OBJ)/$(TEST)_CONFIG := $($(TEST)_CONFIG)
+$(TEST_OBJ)/$(TEST_OUTPUT)_SRC := $($(TEST_OUTPUT)_SRC)
+$(TEST_OBJ)/$(TEST_OUTPUT)_INC := $($(TEST_OUTPUT)_INC) $(VPATH) $(GTEST_INC)
+$(TEST_OBJ)/$(TEST_OUTPUT)_DEFS := $($(TEST_OUTPUT)_DEFS)
+$(TEST_OBJ)/$(TEST_OUTPUT)_CONFIG := $($(TEST_OUTPUT)_CONFIG)
 
 include $(PLATFORM_PATH)/$(PLATFORM_KEY)/platform.mk
 include $(BUILDDEFS_PATH)/common_rules.mk

diff --git a/docs/feature_auto_shift.md b/docs/feature_auto_shift.md
@@ -180,18 +180,18 @@ For more granular control, there is `get_auto_shifted_key`. The default function
 bool get_auto_shifted_key(uint16_t keycode, keyrecord_t *record) {
     switch (keycode) {
 #    ifndef NO_AUTO_SHIFT_ALPHA
-        case KC_A ... KC_Z:
+        case AUTO_SHIFT_ALPHA:
 #    endif
 #    ifndef NO_AUTO_SHIFT_NUMERIC
-        case KC_1 ... KC_0:
+        case AUTO_SHIFT_NUMERIC:
 #    endif
 #    ifndef NO_AUTO_SHIFT_SPECIAL
-#    ifndef NO_AUTO_SHIFT_TAB
+#        ifndef NO_AUTO_SHIFT_TAB
         case KC_TAB:
-#    endif
-#    ifndef NO_AUTO_SHIFT_SYMBOLS
+#        endif
+#        ifndef NO_AUTO_SHIFT_SYMBOLS
         case AUTO_SHIFT_SYMBOLS:
-#    endif
+#        endif
 #    endif
 #    ifdef AUTO_SHIFT_ENTER
         case KC_ENT:
@@ -310,10 +310,16 @@ generating taps on release. For example:
 #define RETRO_SHIFT 500
 ```
 
+Without a value set, holds of any length without an interrupting key will produce the shifted value.
+
 This value (if set) must be greater than one's `TAPPING_TERM`, as the key press
 must be designated as a 'hold' by `process_tapping` before we send the modifier.
+[Per-key tapping terms](tap_hold.md#tapping-term) can be used as a workaround.
 There is no such limitation in regards to `AUTO_SHIFT_TIMEOUT` for normal keys.
 
+**Note:** Tap Holds must be added to Auto Shift, see [here.](feature_auto_shift.md#auto-shift-per-key)
+`IS_RETRO` may be helpful if one wants all Tap Holds retro shifted.
+
 ### Retro Shift and Tap Hold Configurations
 
 Tap Hold Configurations work a little differently when using Retro Shift.

diff --git a/docs/unit_testing.md b/docs/unit_testing.md
@@ -36,7 +36,9 @@ Note how there's several different tests, each mocking out a separate part. Also
 
 ## Running the Tests
 
-To run all the tests in the codebase, type `make test:all`. You can also run test matching a substring by typing `make test:matchingsubstring` Note that the tests are always compiled with the native compiler of your platform, so they are also run like any other program on your computer.
+To run all the tests in the codebase, type `make test:all`. You can also run test matching a substring by typing `make test:matchingsubstring`. `matchingsubstring` can contain colons to be more specific; `make test:tap_hold_configurations` will run the `tap_hold_configurations` tests for all features while `make test:retro_shift:tap_hold_configurations` will run the `tap_hold_configurations` tests for only the Retro Shift feature.
+
+Note that the tests are always compiled with the native compiler of your platform, so they are also run like any other program on your computer.
 
 ## Debugging the Tests
 

diff --git a/quantum/action.c b/quantum/action.c
@@ -497,7 +497,7 @@ void process_action(keyrecord_t *record, action_t action) {
                 default:
                     if (event.pressed) {
                         if (tap_count > 0) {
-#    ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
+#    ifdef HOLD_ON_OTHER_KEY_PRESS
                             if (
 #        ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
                                 get_hold_on_other_key_press(get_event_keycode(record->event, false), record) &&

diff --git a/quantum/action_tapping.c b/quantum/action_tapping.c
@@ -116,25 +116,26 @@ void action_tapping_process(keyrecord_t record) {
  * readable. The conditional definition of tapping_keycode and all the
  * conditional uses of it are hidden inside macros named TAP_...
  */
-#    if (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)) || defined(PERMISSIVE_HOLD_PER_KEY) || defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY)
-#        define TAP_DEFINE_KEYCODE const uint16_t tapping_keycode = get_record_keycode(&tapping_key, false)
-#    else
-#        define TAP_DEFINE_KEYCODE
-#    endif
+#    define TAP_DEFINE_KEYCODE const uint16_t tapping_keycode = get_record_keycode(&tapping_key, false)
 
 #    if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
 #        ifdef RETRO_TAPPING_PER_KEY
-#            define TAP_GET_RETRO_TAPPING get_retro_tapping(tapping_keycode, &tapping_key)
+#            define TAP_GET_RETRO_TAPPING(keyp) get_auto_shifted_key(tapping_keycode, keyp) && get_retro_tapping(tapping_keycode, &tapping_key)
 #        else
-#            define TAP_GET_RETRO_TAPPING true
+#            define TAP_GET_RETRO_TAPPING(keyp) get_auto_shifted_key(tapping_keycode, keyp)
 #        endif
-#        define MAYBE_RETRO_SHIFTING(ev) (TAP_GET_RETRO_TAPPING && (RETRO_SHIFT + 0) != 0 && TIMER_DIFF_16((ev).time, tapping_key.event.time) < (RETRO_SHIFT + 0))
+/* Used to extend TAPPING_TERM:
+ *     indefinitely if RETRO_SHIFT does not have a value
+ *     to RETRO_SHIFT if RETRO_SHIFT is set
+ * for possibly retro shifted keys.
+ */
+#        define MAYBE_RETRO_SHIFTING(ev, keyp) (get_auto_shifted_key(tapping_keycode, keyp) && TAP_GET_RETRO_TAPPING(keyp) && ((RETRO_SHIFT + 0) == 0 || TIMER_DIFF_16((ev).time, tapping_key.event.time) < (RETRO_SHIFT + 0)))
 #        define TAP_IS_LT IS_QK_LAYER_TAP(tapping_keycode)
 #        define TAP_IS_MT IS_QK_MOD_TAP(tapping_keycode)
 #        define TAP_IS_RETRO IS_RETRO(tapping_keycode)
 #    else
-#        define TAP_GET_RETRO_TAPPING false
-#        define MAYBE_RETRO_SHIFTING(ev) false
+#        define TAP_GET_RETRO_TAPPING(keyp) false
+#        define MAYBE_RETRO_SHIFTING(ev, kp) false
 #        define TAP_IS_LT false
 #        define TAP_IS_MT false
 #        define TAP_IS_RETRO false
@@ -187,20 +188,19 @@ bool process_tapping(keyrecord_t *keyp) {
         return true;
     }
 
+#    if (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)) || defined(PERMISSIVE_HOLD_PER_KEY) || defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY)
     TAP_DEFINE_KEYCODE;
+#    endif
 
     // process "pressed" tapping key state
     if (tapping_key.event.pressed) {
-        if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event)) {
+        if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event, keyp)) {
             if (IS_NOEVENT(event)) {
                 // early return for tick events
                 return true;
             }
             if (tapping_key.tap.count == 0) {
                 if (IS_TAPPING_RECORD(keyp) && !event.pressed) {
-#    if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
-                    retroshift_swap_times();
-#    endif
                     // first tap!
                     ac_dprintf("Tapping: First tap(0->1).\n");
                     tapping_key.tap.count = 1;
@@ -218,28 +218,12 @@ bool process_tapping(keyrecord_t *keyp) {
                  */
                 // clang-format off
                 else if (
+                    !event.pressed && waiting_buffer_typed(event) &&
                     (
-                        !event.pressed && waiting_buffer_typed(event) &&
-                        TAP_GET_PERMISSIVE_HOLD
-                    )
-                    // Causes nested taps to not wait past TAPPING_TERM/RETRO_SHIFT
-                    // unnecessarily and fixes them for Layer Taps.
-                    || (TAP_GET_RETRO_TAPPING &&
-                        (
-                            // Rolled over the two keys.
-                            (tapping_key.tap.interrupted == true && (
-                                (TAP_IS_LT && TAP_GET_HOLD_ON_OTHER_KEY_PRESS) ||
-                                (TAP_IS_MT && TAP_GET_HOLD_ON_OTHER_KEY_PRESS)
-                                )
-                            )
-                            // Makes Retro Shift ignore the default behavior of
-                            // MTs and LTs on nested taps below TAPPING_TERM or RETRO_SHIFT
-                            || (
-                                TAP_IS_RETRO
-                                && (event.key.col != tapping_key.event.key.col || event.key.row != tapping_key.event.key.row)
-                                && !event.pressed && waiting_buffer_typed(event)
-                            )
-                        )
+                        TAP_GET_PERMISSIVE_HOLD ||
+                        // Causes nested taps to not wait past TAPPING_TERM/RETRO_SHIFT
+                        // unnecessarily and fixes them for Layer Taps.
+                        TAP_GET_RETRO_TAPPING(keyp)
                     )
                 ) {
                     // clang-format on
@@ -284,10 +268,16 @@ bool process_tapping(keyrecord_t *keyp) {
                     process_record(keyp);
                     return true;
                 } else {
-                    // set interrupted flag when other key preesed during tapping
+                    // set interrupted flag when other key pressed during tapping
                     if (event.pressed) {
                         tapping_key.tap.interrupted = true;
-                        if (TAP_GET_HOLD_ON_OTHER_KEY_PRESS) {
+                        if (TAP_GET_HOLD_ON_OTHER_KEY_PRESS
+#    if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
+                            // Auto Shift cannot evaluate this early
+                            // Retro Shift uses the hold action for all nested taps even without HOLD_ON_OTHER_KEY_PRESS, so this is fine to skip
+                            && !(MAYBE_RETRO_SHIFTING(event, keyp) && get_auto_shifted_key(get_record_keycode(keyp, false), keyp))
+#    endif
+                        ) {
                             ac_dprintf("Tapping: End. No tap. Interfered by pressed key\n");
                             process_record(&tapping_key);
                             tapping_key = (keyrecord_t){0};
@@ -332,6 +322,9 @@ bool process_tapping(keyrecord_t *keyp) {
                     return true;
                 } else {
                     ac_dprintf("Tapping: key event while last tap(>0).\n");
+#    if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
+                    retroshift_swap_times();
+#    endif
                     process_record(keyp);
                     return true;
                 }
@@ -388,7 +381,7 @@ bool process_tapping(keyrecord_t *keyp) {
     }
     // process "released" tapping key state
     else {
-        if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event)) {
+        if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event, keyp)) {
             if (IS_NOEVENT(event)) {
                 // early return for tick events
                 return true;
@@ -506,9 +499,16 @@ void waiting_buffer_scan_tap(void) {
         return;
     }
 
+#    if (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+    TAP_DEFINE_KEYCODE;
+#    endif
     for (uint8_t i = waiting_buffer_tail; i != waiting_buffer_head; i = (i + 1) % WAITING_BUFFER_SIZE) {
         keyrecord_t *candidate = &waiting_buffer[i];
-        if (IS_EVENT(candidate->event) && KEYEQ(candidate->event.key, tapping_key.event.key) && !candidate->event.pressed && WITHIN_TAPPING_TERM(candidate->event)) {
+        // clang-format off
+        if (IS_EVENT(candidate->event) && KEYEQ(candidate->event.key, tapping_key.event.key) && !candidate->event.pressed && (
+            WITHIN_TAPPING_TERM(waiting_buffer[i].event) || MAYBE_RETRO_SHIFTING(waiting_buffer[i].event, &tapping_key)
+        )) {
+            // clang-format on
             tapping_key.tap.count = 1;
             candidate->tap.count  = 1;
             process_record(&tapping_key);

diff --git a/quantum/process_keycode/process_auto_shift.c b/quantum/process_keycode/process_auto_shift.c
@@ -66,7 +66,7 @@ __attribute__((weak)) bool get_custom_auto_shifted_key(uint16_t keycode, keyreco
     return false;
 }
 
-/** \brief Called on physical press, returns whether is Auto Shift key */
+/** \brief Called on physical press, returns whether key is an Auto Shift key */
 __attribute__((weak)) bool get_auto_shifted_key(uint16_t keycode, keyrecord_t *record) {
     switch (keycode) {
 #ifndef NO_AUTO_SHIFT_ALPHA
@@ -178,9 +178,8 @@ static bool autoshift_press(uint16_t keycode, uint16_t now, keyrecord_t *record)
     }
 
     // Store record to be sent to user functions if there's no release record then.
-    autoshift_lastrecord               = *record;
-    autoshift_lastrecord.event.pressed = false;
-    autoshift_lastrecord.event.time    = 0;
+    autoshift_lastrecord            = *record;
+    autoshift_lastrecord.event.time = 0;
     // clang-format off
 #if defined(AUTO_SHIFT_REPEAT) || defined(AUTO_SHIFT_REPEAT_PER_KEY)
     if (keycode == autoshift_lastkey &&
@@ -409,8 +408,12 @@ bool process_auto_shift(uint16_t keycode, keyrecord_t *record) {
             // If Retro Shift is disabled, possible custom actions shouldn't happen.
             // clang-format off
 #if defined(RETRO_SHIFT) && !defined(NO_ACTION_TAPPING)
-#    ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
-            const bool is_hold_on_interrupt = get_hold_on_other_key_press(keycode, record);
+#    ifdef HOLD_ON_OTHER_KEY_PRESS
+            const bool is_hold_on_interrupt = (IS_QK_MOD_TAP(keycode)
+#        ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
+                && get_hold_on_other_key_press(keycode, record)
+#        endif
+            );
 #    else
             const bool is_hold_on_interrupt = false;
 #    endif
@@ -450,8 +453,12 @@ bool process_auto_shift(uint16_t keycode, keyrecord_t *record) {
 #endif
         ) {
             // Fixes modifiers not being applied to rolls with AUTO_SHIFT_MODIFIERS set.
-#ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
-            if (autoshift_flags.in_progress && get_hold_on_other_key_press(keycode, record)) {
+#ifdef HOLD_ON_OTHER_KEY_PRESS
+            if (autoshift_flags.in_progress
+#    ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
+                && get_hold_on_other_key_press(keycode, record)
+#    endif
+            ) {
                 autoshift_end(KC_NO, now, false, &autoshift_lastrecord);
             }
 #endif
@@ -488,10 +495,8 @@ void retroshift_poll_time(keyevent_t *event) {
 }
 // Used to swap the times of Retro Shifted key and Auto Shift key that interrupted it.
 void retroshift_swap_times(void) {
-    if (last_retroshift_time != 0 && autoshift_flags.in_progress) {
-        uint16_t temp        = retroshift_time;
-        retroshift_time      = last_retroshift_time;
-        last_retroshift_time = temp;
+    if (autoshift_flags.in_progress) {
+        autoshift_time = last_retroshift_time;
     }
 }
 #endif
diff --git a/quantum/process_keycode/process_auto_shift.h b/quantum/process_keycode/process_auto_shift.h
@@ -56,4 +56,5 @@ uint16_t (get_autoshift_timeout)(uint16_t keycode, keyrecord_t *record);
 void     set_autoshift_timeout(uint16_t timeout);
 void     autoshift_matrix_scan(void);
 bool     get_custom_auto_shifted_key(uint16_t keycode, keyrecord_t *record);
+bool     get_auto_shifted_key(uint16_t keycode, keyrecord_t *record);
 // clang-format on