Unit Test Plan: 6. Testing key_mapping.gd #352 - #374
Conversation
Introduce a new GUT test suite test/gut/test_key_mapping_menu_device_aware.gd that verifies key mapping menu behavior (KM-01..KM-13). Covers device toggling (keyboard/gamepad), mutual exclusivity, UI label sync, reset semantics (per-device and defaults), persistence via user config, invalid input handling during remap, signal connections, logging sanity, and rapid-toggle stress. Tests instantiate key_mapping_menu.tscn, drive CheckButtons and InputRemapButton instances, and include setup/teardown to preserve/cleanup a temporary user config.
ikostan
added this to the
Milestone 11: Polish & Platform-wide Enhancements (Menus, Testing & Configuration) milestone
Reviewer's GuideAdds a new GUT test suite that instantiates the key mapping menu scene and exhaustively tests device-aware behavior (keyboard vs gamepad), remapping, reset and persistence semantics, invalid input handling, and signal wiring for key mapping actions. 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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
before_all/after_allyou recomputebackup_pathas a string literal instead of using a shared constant; consider extractinguser://test_backup_device_aware.cfginto aconstto avoid drift between setup/teardown paths. - In
_remap_buttonyou directly call the button’s_on_pressed()and_input()methods; using the public signals (pressed.emit()and feeding events via the scene tree) would make the tests less tightly coupled toInputRemapButtoninternals and more resilient to refactors. - In
test_km_11_rapid_toggle_stressthe final assertion uses19 % 2 == 1inline; since the loop bounds are fixed you can simplify this by asserting the explicitly expected device (GAMEPAD) or deriving it from the final button state to avoid a magic number in the test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `before_all`/`after_all` you recompute `backup_path` as a string literal instead of using a shared constant; consider extracting `user://test_backup_device_aware.cfg` into a `const` to avoid drift between setup/teardown paths.
- In `_remap_button` you directly call the button’s `_on_pressed()` and `_input()` methods; using the public signals (`pressed.emit()` and feeding events via the scene tree) would make the tests less tightly coupled to `InputRemapButton` internals and more resilient to refactors.
- In `test_km_11_rapid_toggle_stress` the final assertion uses `19 % 2 == 1` inline; since the loop bounds are fixed you can simplify this by asserting the explicitly expected device (GAMEPAD) or deriving it from the final button state to avoid a magic number in the test.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new GUT test suite file implementing 13 device-aware Key Mapping Menu tests covering device toggles, remapping, resets, persistence, UI validation, signals, logging, invalid inputs, and stress scenarios for keyboard and gamepad modes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/gut/test_key_mapping_menu_device_aware.gd`:
- Around line 224-244: test_km_09_persistence sets keyboard_btn.button_pressed
and gamepad_btn.button_pressed but never emits their toggled signals before
calling _remap_button, so device context isn't switched; after setting
keyboard_btn.button_pressed = true call keyboard_btn.toggled.emit(true) before
the first _remap_button call, and likewise after setting
gamepad_btn.button_pressed = true call gamepad_btn.toggled.emit(true) before the
second _remap_button call so the remaps apply to the intended devices
(references: test_km_09_persistence, keyboard_btn, gamepad_btn, _remap_button,
toggled.emit).
- Around line 179-195: The test fails to update the menu's device state because
you set keyboard_btn.button_pressed/gamepad_btn.button_pressed without emitting
their toggled signals; update test_km_05_reset_current_device to emit the
corresponding toggled signal immediately after each button_pressed change so the
menu handlers run: after the first keyboard_btn.button_pressed = true call emit
keyboard_btn.toggled.emit(false) (to select keyboard) before calling
_remap_button, after the gamepad_btn.button_pressed = true emit
gamepad_btn.toggled.emit(true) before the D-Pad remap, and before reset emit
keyboard_btn.toggled.emit(false) so _on_reset_pressed reads the correct keyboard
state; this keeps _remap_button, update_all_remap_buttons,
_on_keyboard_toggled/_on_gamepad_toggled and input_remap_button.gd::_input in
sync.
🧹 Nitpick comments (2)
test/gut/test_key_mapping_menu_device_aware.gd (2)
214-221: Replace always‑true assertion with log capture.
Consider using GUT’s log capture/spy utilities to assert expected debug output instead ofassert_true(true).
263-275: Make expected device derive from loop, not a constant.
19 % 2is fixed; if the iteration count changes the assertion silently drifts. Track the expected device inside the loop.♻️ Suggested tweak
- for i in range(20): + var expected_device = InputRemapButton.DeviceType.KEYBOARD + for i in range(20): if i % 2 == 0: keyboard_btn.button_pressed = true keyboard_btn.toggled.emit(true) + expected_device = InputRemapButton.DeviceType.KEYBOARD else: gamepad_btn.button_pressed = true gamepad_btn.toggled.emit(true) + expected_device = InputRemapButton.DeviceType.GAMEPAD menu.update_all_remap_buttons() assert_true(keyboard_btn.button_pressed or gamepad_btn.button_pressed) assert_false(keyboard_btn.button_pressed and gamepad_btn.button_pressed) - assert_eq(speed_up_btn.current_device, InputRemapButton.DeviceType.GAMEPAD if 19 % 2 == 1 else InputRemapButton.DeviceType.KEYBOARD) + assert_eq(speed_up_btn.current_device, expected_device)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_key_mapping_menu_device_aware.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 (14)
test/gut/test_key_mapping_menu_device_aware.gd (14)
25-31: LGTM for config backup setup.
Clean and minimal; preserves existing user settings.
83-89: LGTM for per-test cleanup.
Queue-free + frame wait is appropriate for teardown.
92-104: LGTM for per-suite restore.
Restores backups safely and cleans temporary files.
107-123: LGTM for remap helper.
Clear, deterministic input simulation.
126-135: LGTM for KM‑01.
Toggle test is clear and covers device propagation.
138-147: LGTM for KM‑02.
Covers expected gamepad activation behavior.
150-162: LGTM for KM‑03.
Mutual exclusivity is correctly exercised both ways.
165-176: LGTM for KM‑04.
UI label updates are asserted for both devices.
198-204: LGTM for KM‑06.
Basic node existence coverage is good.
207-211: LGTM for KM‑07.
Signal wiring checks are straightforward.
247-260: LGTM for KM‑10.
Invalid input path is asserted cleanly.
278-287: LGTM for KM‑12.
Label sync checks are crisp and readable.
290-302: LGTM for KM‑13.
Default reset path is well covered.
34-81: No action needed. In Godot 4.x,add_child()is synchronous when the parent is already in the SceneTree (as it is here in a GUT test). The children's_ready()callbacks, includingInputRemapButton._ready()which adds nodes to the "remap_buttons" group, complete beforeadd_child()returns. The test code safely accesses the group and callsupdate_all_remap_buttons()immediately after.
✏️ 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>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/gut/test_key_mapping_menu_device_aware.gd`:
- Around line 293-305: The test_km_13_reset_with_defaults has a missing keyboard
device toggle before calling reset: emit keyboard_btn.toggled(true) (same
pattern used for gamepad_btn) so the reset operates on the current device state;
update the setup so after setting keyboard_btn.button_pressed = true you call
keyboard_btn.toggled.emit(true) before reset_btn.pressed.emit(), ensuring
keyboard_btn, gamepad_btn and reset_btn interactions are consistent.
🧹 Nitpick comments (3)
test/gut/test_key_mapping_menu_device_aware.gd (3)
26-31: Consider checking return values from file operations.
DirAccess.copy_absolute()returns anErrorcode that is currently ignored. If the copy fails, the test suite would proceed without a valid backup, potentially corrupting user settings during cleanup.♻️ Proposed fix to validate copy operations
func before_all() -> void: var backup_path: String = "user://test_backup_device_aware.cfg" if FileAccess.file_exists(TEST_CONFIG_PATH): - DirAccess.copy_absolute(TEST_CONFIG_PATH, backup_path) + var err := DirAccess.copy_absolute(TEST_CONFIG_PATH, backup_path) + assert_eq(err, OK, "Failed to backup test config") if FileAccess.file_exists(Settings.CONFIG_PATH): - DirAccess.copy_absolute(Settings.CONFIG_PATH, DEFAULT_CONFIG_BACKUP) + var err := DirAccess.copy_absolute(Settings.CONFIG_PATH, DEFAULT_CONFIG_BACKUP) + assert_eq(err, OK, "Failed to backup production config")
69-74: Potential scope issue:get_nodes_in_groupreturns all nodes in the SceneTree.
menu.get_tree().get_nodes_in_group("remap_buttons")retrieves all nodes in the group across the entire scene tree, not just descendants ofmenu. If other tests or scenes add nodes to this group, the array may contain unintended elements.♻️ Proposed fix to filter by menu descendants
- var nodes: Array[Node] = menu.get_tree().get_nodes_in_group("remap_buttons") - remap_buttons = [] - for node: Node in nodes: - if node is InputRemapButton: - remap_buttons.append(node as InputRemapButton) + var all_nodes: Array[Node] = menu.get_tree().get_nodes_in_group("remap_buttons") + remap_buttons = [] + for node: Node in all_nodes: + if node is InputRemapButton and menu.is_ancestor_of(node): + remap_buttons.append(node as InputRemapButton)
266-278: Consider simplifying the expected device assertion.Line 278 uses
19 % 2 == 1which is alwaystrue(19 is odd), making the ternary evaluate toGAMEPAD. While correct, this is confusing to read. Consider using a constant or direct value for clarity.♻️ Proposed simplification
- assert_eq(speed_up_btn.current_device, InputRemapButton.DeviceType.GAMEPAD if 19 % 2 == 1 else InputRemapButton.DeviceType.KEYBOARD) + # After 20 iterations (0-19), last iteration is i=19 (odd), so gamepad is active + assert_eq(speed_up_btn.current_device, InputRemapButton.DeviceType.GAMEPAD)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_key_mapping_menu_device_aware.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 (4)
test/gut/test_key_mapping_menu_device_aware.gd (4)
1-23: LGTM!Constants and instance variables are well-defined with proper type hints. The test configuration paths are appropriately namespaced to avoid conflicts with production settings.
83-105: LGTM!The cleanup and restore logic properly handles test isolation and preserves user settings. The
await get_tree().process_frameensures deferred cleanup completes before the next test.
107-124: LGTM!The helper correctly simulates remapping for different input event types. The direct
_input()call is appropriate for unit testing, and the assertion at line 123 validates the remap completed successfully.
126-199: LGTM!Tests KM-01 through KM-05 correctly emit
toggledsignals after settingbutton_pressed, ensuring the menu's device state handlers are triggered. The fix for KM-05 (previously flagged) is properly applied withtoggled.emit()calls at lines 183, 187, and 192.
✏️ 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>
… device switches). The first two remaps in this test set button_pressed without emitting toggled, but later in the same test (lines after reload) properly emit the signal. All other tests consistently emit toggled after setting button_pressed before remapping. Without the signal, device context handlers may not be triggered, causing remaps to apply to the previous device.
DirAccess.copy_absolute() returns an Error code that is currently ignored. If the copy fails, the test suite would proceed without a valid backup, potentially corrupting user settings during cleanup.
… the SceneTree.
menu.get_tree().get_nodes_in_group("remap_buttons") retrieves all nodes in the group across the entire scene tree, not just descendants of menu. If other tests or scenes add nodes to this group, the array may contain unintended elements.
Line 278 uses 19 % 2 == 1 which is always true (19 is odd), making the ternary evaluate to GAMEPAD. While correct, this is confusing to read. Consider using a constant or direct value for clarity.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Several tests assert against hard-coded UI text values like "W", "Right Trigger", and "D-Pad Left"; consider deriving these from the same label/translation helpers used in production (or asserting on underlying InputMap events instead) so that tests are resilient to localization or label changes.
- The tests interact with remap buttons via private methods (
_on_pressed,_input) and direct property twiddling; where possible, prefer going through public APIs or emitting the appropriate signals to better mirror real usage and reduce coupling to internal implementation details. - The backup path string for
user://test_backup_device_aware.cfgand the config backup/restore logic is duplicated betweenbefore_allandafter_all; consider centralizing this into a small helper or constant to avoid divergence if the backup behavior needs to change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests assert against hard-coded UI text values like "W", "Right Trigger", and "D-Pad Left"; consider deriving these from the same label/translation helpers used in production (or asserting on underlying InputMap events instead) so that tests are resilient to localization or label changes.
- The tests interact with remap buttons via private methods (`_on_pressed`, `_input`) and direct property twiddling; where possible, prefer going through public APIs or emitting the appropriate signals to better mirror real usage and reduce coupling to internal implementation details.
- The backup path string for `user://test_backup_device_aware.cfg` and the config backup/restore logic is duplicated between `before_all` and `after_all`; consider centralizing this into a small helper or constant to avoid divergence if the backup behavior needs to change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The backup path string for user://test_backup_device_aware.cfg and the config backup/restore logic is duplicated between before_all and after_all; consider centralizing this into a small helper or constant to avoid divergence if the backup behavior needs to change.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
1 participant
Introduce a new GUT test suite test/gut/test_key_mapping_menu_device_aware.gd that verifies key mapping menu behavior (KM-01..KM-13). Covers device toggling (keyboard/gamepad), mutual exclusivity, UI label sync, reset semantics (per-device and defaults), persistence via user config, invalid input handling during remap, signal connections, logging sanity, and rapid-toggle stress. Tests instantiate key_mapping_menu.tscn, drive CheckButtons and InputRemapButton instances, and include setup/teardown to preserve/cleanup a temporary user config.
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
Tests:
Summary by CodeRabbit