Fix pressing two keys with the same keycode but different modifiers (#2710)

* Fix extra keyboard report during test_fixture teardown

* Add tests for pressing two keys with only different modifers

* Fix #1708

When two keys that use the same keycode, but different modifiers were
pressed at the same time, the second keypress wasn't registered. This is
fixed by forcing a key release when we detect a new press for the same
keycode.

* Fix the NKRO version of is_key_pressed

* Fix uninitalized loop variable

Co-authored-by: Jack Humbert <jack.humb@gmail.com>

3 files changed, 123 insertions, 5 deletions

diff --git a/tests/basic/keymap.c b/tests/basic/keymap.c
index 02b3cc961a..951ce8e0cc 100644
--- a/tests/basic/keymap.c
+++ b/tests/basic/keymap.c
@@ -28,7 +28,7 @@ const uint16_t PROGMEM
                 {
                     // 0    1      2      3        4        5        6       7            8      9
                     {KC_A, KC_B, KC_NO, KC_LSFT, KC_RSFT, KC_LCTL, COMBO1, SFT_T(KC_P), M(0), KC_NO},
-                    {KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
+                    {KC_EQL, KC_PLUS, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
                     {KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
                     {KC_C, KC_D, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
                 },
@@ -43,3 +43,4 @@ const macro_t *action_get_macro(keyrecord_t *record, uint8_t id, uint8_t opt) {
     }
     return MACRO_NONE;
 };
+
diff --git a/tests/basic/test_keypress.cpp b/tests/basic/test_keypress.cpp
index e5247911cb..551458a0d9 100644
--- a/tests/basic/test_keypress.cpp
+++ b/tests/basic/test_keypress.cpp
@@ -18,6 +18,7 @@
 
 using testing::_;
 using testing::Return;
+using testing::InSequence;
 
 class KeyPress : public TestFixture {};
 
@@ -121,4 +122,119 @@ TEST_F(KeyPress, RightShiftLeftControlAndCharWithTheSameKey) {
     EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_RSFT, KC_RCTRL)));
     EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
     keyboard_task();
-}
\ No newline at end of file
+}
+
+TEST_F(KeyPress, PressPlusEqualReleaseBeforePress) {
+  TestDriver driver;
+  InSequence s;
+
+  press_key(1, 1); // KC_PLUS
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(1, 1); // KC_PLUS
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  press_key(0, 1); // KC_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(0, 1); // KC_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
+TEST_F(KeyPress, PressPlusEqualDontReleaseBeforePress) {
+  TestDriver driver;
+  InSequence s;
+
+  press_key(1, 1); // KC_PLUS
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  press_key(0, 1); // KC_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(1, 1); //KC_PLS
+  // BUG: Should really still return KC_EQL, but this is fine too
+  // It's also called twice for some reason
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(2);
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(0, 1); // KC_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
+TEST_F(KeyPress, PressEqualPlusReleaseBeforePress) {
+  TestDriver driver;
+  InSequence s;
+
+  press_key(0, 1); // KC_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(0, 1); // KQ_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  press_key(1, 1); // KC_PLUS
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(1, 1); // KC_PLUS
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
+TEST_F(KeyPress, PressEqualPlusDontReleaseBeforePress) {
+  TestDriver driver;
+  InSequence s;
+
+  press_key(0, 1); // KC_EQL
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  press_key(1, 1); // KC_PLUS
+  // BUG: The sequence is a bit strange, but it works, the end result is that
+  // KC_PLUS is sent
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(0, 1); //KC_EQL
+  // I guess it's fine to still report shift here
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+
+  release_key(1, 1); // KC_PLUS
+  // This report is not needed
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+  EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+  run_one_scan_loop();
+  testing::Mock::VerifyAndClearExpectations(&driver);
+}
diff --git a/tests/test_common/test_fixture.cpp b/tests/test_common/test_fixture.cpp
index 8caf1fca4a..20ed838eb6 100644
--- a/tests/test_common/test_fixture.cpp
+++ b/tests/test_common/test_fixture.cpp
@@ -32,14 +32,15 @@ TestFixture::TestFixture() {}
 
 TestFixture::~TestFixture() {
     TestDriver driver;
-    layer_clear();
-    clear_all_keys();
     // Run for a while to make sure all keys are completely released
     EXPECT_CALL(driver, send_keyboard_mock(_)).Times(AnyNumber());
+    layer_clear();
+    clear_all_keys();
     idle_for(TAPPING_TERM + 10);
     testing::Mock::VerifyAndClearExpectations(&driver);
     // Verify that the matrix really is cleared
-    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(Between(0, 1));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0);
+    idle_for(TAPPING_TERM + 10);
 }
 
 void TestFixture::run_one_scan_loop() {