Unit Test : Key Mapping Menu — DeviceSettings & UI Behavior - #362
Merged
ikostan merged 8 commits intoJan 29, 2026
Conversation
ikostan
added this to the
🚀 Milestone 10: UI & Input Improvements, System Refactors, Testing & Documentation milestone
Contributor
Reviewer's GuideAdds a shared remap prompt text constant and refactors the input remap button to use it, and introduces a new GUT test suite validating the key mapping menu’s device switching, reset behavior per device, and label updates after remapping for keyboard and gamepad inputs. Sequence diagram for key remap button press using shared prompt textsequenceDiagram
actor Player
participant KeyMappingMenu
participant InputRemapButton
participant Globals
Player->>KeyMappingMenu: select action to remap
KeyMappingMenu->>InputRemapButton: trigger _on_pressed()
InputRemapButton->>InputRemapButton: set listening = true
InputRemapButton->>Globals: access REMAP_PROMPT_TEXT
Globals-->>InputRemapButton: REMAP_PROMPT_TEXT value
InputRemapButton->>InputRemapButton: set text = REMAP_PROMPT_TEXT
Player->>InputRemapButton: press key or controller input
InputRemapButton->>InputRemapButton: set listening = false
InputRemapButton->>InputRemapButton: update_button_text()
Class diagram for Globals and InputRemapButton remap prompt constantclassDiagram
class Globals {
<<Node>>
enum LogLevel
+current_log_level: LogLevel
+enable_debug_logging: bool
+difficulty: float
+options_open: bool
+previous_scene: String
+options_scene: PackedScene
+next_scene: String
+REMAP_PROMPT_TEXT: String
+_ready(): void
}
class InputRemapButton {
<<Button>>
-listening: bool
+_ready(): void
+_on_pressed(): void
+update_button_text(): void
}
Globals <.. InputRemapButton : uses REMAP_PROMPT_TEXT
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- These tests rely heavily on calling internal methods like
_ready(),_on_pressed(), and_input()directly; consider driving the behavior via public signals/inputs where possible to reduce coupling to implementation details. - Assertions that depend on exact or partially descriptive button text (e.g.
"A","D-Pad Left", orcontains("Stick")) may be brittle against localization or label changes; using properties like the boundInputEventor a stable mapping helper would make the tests more robust. - The
remap_buttonsloop types each element asVariant; if the group is guaranteed to containInputRemapButtoninstances, annotate the array and loop variable with that type to catch mismatches at edit-time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- These tests rely heavily on calling internal methods like `_ready()`, `_on_pressed()`, and `_input()` directly; consider driving the behavior via public signals/inputs where possible to reduce coupling to implementation details.
- Assertions that depend on exact or partially descriptive button text (e.g. `"A"`, `"D-Pad Left"`, or `contains("Stick")`) may be brittle against localization or label changes; using properties like the bound `InputEvent` or a stable mapping helper would make the tests more robust.
- The `remap_buttons` loop types each element as `Variant`; if the group is guaranteed to contain `InputRemapButton` instances, annotate the array and loop variable with that type to catch mismatches at edit-time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
📝 WalkthroughWalkthroughAdds a new GUT test file exercising the Key Mapping Menu UI: device switching (keyboard/gamepad), remap button states and labels, reset flows, and remap interaction simulations across five test cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/gut/test_key_mapping_menu.gd`:
- Line 33: Remove the manual call to menu._ready() after add_child() to avoid
duplicate initialization; instead, after calling add_child(menu) wait a frame to
allow Godot to run the node lifecycle (e.g., await get_tree().process_frame or
yield(get_tree(), "idle_frame") depending on engine version) or assert the
specific initialized state you need—update the test in test_key_mapping_menu.gd
to call add_child(menu) and then wait for a process frame rather than invoking
menu._ready() directly.
- Around line 42-51: The test test_ui_01_switch_to_keyboard sets
keyboard_btn.button_pressed directly which does not emit the toggled signal, so
it may not exercise the menu's device-switch logic; update the test to simulate
a real user toggle by emitting the toggled signal or calling the CheckButton’s
pressed action (e.g., call emit_signal("toggled", true) or invoke the same
method the UI uses to handle toggles) on keyboard_btn (and ensure gamepad_btn
reflects the prior state from before_each) and then assert that each
remap_buttons entry has current_device == InputRemapButton.DeviceType.KEYBOARD
to verify the actual switch path is executed.
🧹 Nitpick comments (4)
test/gut/test_key_mapping_menu.gd (4)
64-67: Fragile assertion for gamepad label validation.The condition
text.contains("Stick") or text.contains("Button") or text.length() > 1is overly permissive—any two-character string would pass. This doesn't reliably verify that the label reflects a gamepad-specific input.Consider checking against known gamepad label patterns or verifying the button's
current_deviceproperty instead:Proposed improvement
# Example: SpeedUp button text should reflect gamepad label (e.g. contains "Stick" or button name after update) var speed_up_btn: Button = menu.get_node("Panel/Options/KeyMapContainer/PlayerKeyMap/KeyMappingSpeedUp/SpeedUpInputRemap") - assert_true(speed_up_btn.text.contains("Stick") or speed_up_btn.text.contains("Button") or speed_up_btn.text.length() > 1, - "Gamepad label should be descriptive (not a single key char)") + # Verify it's not showing a keyboard key (single letter) and has meaningful gamepad text + assert_false(speed_up_btn.text.length() == 1 and speed_up_btn.text.to_upper() == speed_up_btn.text, + "Gamepad label should not be a single keyboard key character") + assert_ne(speed_up_btn.text, "Unbound", "Gamepad should have a default binding")
75-82: Calling private methods (_on_pressed,_input) directly couples tests to implementation.Invoking
_on_pressed()and_input()directly tests internal implementation rather than observable behavior. If these method names change, all tests break even if functionality is preserved.This is a common tradeoff in UI testing—simulating real input in GUT is complex. If this approach is intentional, consider adding a comment documenting why direct method calls are used, or extract these into helper methods to isolate the coupling.
118-118: Hardcoded UI string may be fragile.The expected text
"Press a key or controller button/axis..."is hardcoded. If this prompt changes in the implementation, the test will fail even though functionality is correct.Consider extracting this string to a constant shared with the implementation, or using a partial match (e.g.,
assert_true(left_btn.text.contains("Press"))) for more resilient assertions.
126-137: Expected label strings depend on Godot's internal formatting.The assertions expect exact strings
"A"and"D-Pad Left"which depend on how Godot formatsInputEvent.as_text()or similar. These could vary across Godot versions.If this is acceptable for your target Godot version, consider adding a comment noting the version dependency. Otherwise, consider more flexible assertions or extracting label formatting to a testable utility.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_key_mapping_menu.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (2)
test/gut/test_key_mapping_menu.gd (2)
36-38: LGTM!Proper cleanup using
is_instance_valid()check beforequeue_free().
90-107: LGTM!Follows the same pattern as
test_ui_03. The same notes about private method calls apply, but the test logic is consistent and the assertions appropriately verify reset behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
In Godot 4, programmatically setting button_pressed = true on a CheckButton does not trigger the toggled signal. If the menu's device-switching logic relies on that signal, this test may not actually exercise the switching code path. Since keyboard_btn.button_pressed = true is already set in before_each, this test may pass trivially without verifying the switching behavior. Consider emitting the signal explicitly or testing the actual transition from gamepad to keyboard.
The condition text.contains("Stick") or text.contains("Button") or text.length() > 1 is overly permissive—any two-character string would pass. This doesn't reliably verify that the label reflects a gamepad-specific input.
Consider checking against known gamepad label patterns or verifying the button's current_device property instead
…to implementation. Invoking _on_pressed() and _input() directly tests internal implementation rather than observable behavior. If these method names change, all tests break even if functionality is preserved. This is a common tradeoff in UI testing—simulating real input in GUT is complex. If this approach is intentional, consider adding a comment documenting why direct method calls are used, or extract these into helper methods to isolate the coupling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/gut/test_key_mapping_menu.gd`:
- Around line 67-74: Replace the brittle string-heuristic checks on the Button
text (speed_up_btn / SpeedUpInputRemap) with assertions against the actual bound
input event or InputMap entries: retrieve the remap control's bound InputEvent
(or use InputMap.get_action_list with the speed-up action name) and assert the
event is a gamepad type (e.g., InputEventJoypadButton or InputEventJoypadMotion)
and not an InputEventKey, and also assert that an event exists (not unbound).
Use the SpeedUpInputRemap node reference to locate the binding instead of
inspecting speed_up_btn.text.
🧹 Nitpick comments (1)
test/gut/test_key_mapping_menu.gd (1)
28-31: Remove duplicated default keyboard assignment.The second
keyboard_btn.button_pressed = trueis redundant and adds noise.♻️ Proposed cleanup
- # Default to keyboard (as in _ready) - keyboard_btn.button_pressed = true - # Default to keyboard (as in _ready) - keyboard_btn.button_pressed = true + # Default to keyboard (as in _ready) + keyboard_btn.button_pressed = true
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_key_mapping_menu.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The second keyboard_btn.button_pressed = true is redundant and adds noise.
The expected text "Press a key or controller button/axis..." is hardcoded. If this prompt changes in the implementation, the test will fail even though functionality is correct.
Consider extracting this string to a constant shared with the implementation, or using a partial match (e.g., assert_true(left_btn.text.contains("Press"))) for more resilient assertions.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tests reach into private methods like
_on_pressed()and_input()on the remap buttons, which tightly couples them to the internal implementation; consider driving these flows via public signals or higher-level menu handlers so future refactors don’t break tests unnecessarily. - In
test_key_mapping_menu.gd, the use of hard-coded node paths (e.g.Panel/Options/KeyMapContainer/PlayerKeyMap/...) makes the tests brittle against UI layout changes; you might instead rely on groups, exported references, or helper getters from the menu script to locate key remap buttons. - The
remap_buttonsarray is typed asArraywith elements accessed asVariant; you can tighten this toArray[InputRemapButton](and drop theVariantannotation in the loop) to catch type issues at compile time and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests reach into private methods like `_on_pressed()` and `_input()` on the remap buttons, which tightly couples them to the internal implementation; consider driving these flows via public signals or higher-level menu handlers so future refactors don’t break tests unnecessarily.
- In `test_key_mapping_menu.gd`, the use of hard-coded node paths (e.g. `Panel/Options/KeyMapContainer/PlayerKeyMap/...`) makes the tests brittle against UI layout changes; you might instead rely on groups, exported references, or helper getters from the menu script to locate key remap buttons.
- The `remap_buttons` array is typed as `Array` with elements accessed as `Variant`; you can tighten this to `Array[InputRemapButton]` (and drop the `Variant` annotation in the loop) to catch type issues at compile time and improve readability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Won't fix any of it, those are minor issues in my opinion. |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
deleted the
unit-test-3-keymapping-menu-device-tabs-ui-behavior
branch
January 29, 2026 05:36
This was referenced Jan 31, 2026
8 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Add unit tests for the key mapping menu’s device switching, reset behavior, and remap label updates, and centralize the remap prompt text in a global constant.
Enhancements:
Tests:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.