Unit test integration settings UI sync - #368
Conversation
Merge from master
Added a new section for current development status and milestones. Reorganized and clarified play instructions. Expanded the contribution section with labeling guidelines and PR instructions for better onboarding.
ikostan
added this to the
🚀 Milestone 10: UI & Input Improvements, System Refactors, Testing & Documentation milestone
Reviewer's GuideAdds GUT integration tests to validate that key-mapping settings correctly load from disk, propagate to InputMap and the key-mapping UI, persist after remapping, and reset to defaults, and refreshes README documentation with current milestone/status and clearer play and contribution guidance. Sequence diagram for key mapping load and UI syncsequenceDiagram
actor Player
participant KeyMappingUI
participant SettingsManager
participant FileSystem
participant InputMap
Player->>KeyMappingUI: open_settings_menu
KeyMappingUI->>SettingsManager: request_key_mappings
SettingsManager->>FileSystem: load_config_file
FileSystem-->>SettingsManager: config_key_bindings
SettingsManager->>InputMap: apply_mappings_from_config
InputMap-->>SettingsManager: mappings_applied
SettingsManager-->>KeyMappingUI: current_key_mappings
KeyMappingUI->>KeyMappingUI: render_key_mapping_list
KeyMappingUI-->>Player: display_current_bindings
Sequence diagram for key remap, save, persistence, and resetsequenceDiagram
participant GutTest
participant KeyMappingUI
participant SettingsManager
participant InputMap
participant FileSystem
GutTest->>KeyMappingUI: open_key_mapping_screen
GutTest->>KeyMappingUI: select_action_to_remap
GutTest->>KeyMappingUI: press_new_key
KeyMappingUI->>InputMap: remap_action_to_new_key
InputMap-->>KeyMappingUI: mapping_updated
KeyMappingUI->>SettingsManager: save_current_key_mappings
SettingsManager->>InputMap: read_runtime_mappings
InputMap-->>SettingsManager: runtime_mappings
SettingsManager->>FileSystem: write_config_key_bindings
FileSystem-->>SettingsManager: config_saved
GutTest->>SettingsManager: reload_key_mappings_from_disk
SettingsManager->>FileSystem: load_config_file
FileSystem-->>SettingsManager: config_key_bindings
SettingsManager->>InputMap: apply_mappings_from_config
SettingsManager-->>GutTest: loaded_key_mappings
GutTest->>KeyMappingUI: refresh_display
KeyMappingUI->>SettingsManager: request_key_mappings
SettingsManager-->>KeyMappingUI: current_key_mappings
KeyMappingUI-->>GutTest: show_updated_bindings
GutTest->>SettingsManager: reset_key_mappings_to_defaults
SettingsManager->>InputMap: apply_default_mappings
SettingsManager->>FileSystem: write_default_key_bindings
FileSystem-->>SettingsManager: defaults_saved
GutTest->>KeyMappingUI: refresh_display
KeyMappingUI->>SettingsManager: request_key_mappings
SettingsManager-->>KeyMappingUI: default_key_mappings
Class diagram for key mapping integration test suite and collaboratorsclassDiagram
class GutTest {
}
class KeyMappingIntegrationTest {
- original_input_map_state
- original_config_path
+ setup()
+ teardown()
+ test_load_key_mappings_from_disk()
+ test_remap_updates_inputmap_and_ui()
+ test_persisted_mappings_reload_correctly()
+ test_reset_restores_default_mappings()
}
class SettingsManager {
+ load_key_mappings()
+ save_key_mappings()
+ reset_key_mappings_to_defaults()
+ get_current_key_mappings()
}
class KeyMappingUI {
- settings_manager
+ open()
+ refresh_display()
+ get_displayed_mappings()
+ remap_action(action_name, new_key)
}
class InputMapWrapper {
+ apply_mappings(mappings)
+ get_current_mappings()
+ reset_to_defaults()
}
class ConfigFileAdapter {
+ load_key_mappings()
+ save_key_mappings(mappings)
+ reset_to_defaults()
}
GutTest <|-- KeyMappingIntegrationTest
KeyMappingIntegrationTest --> SettingsManager : uses
KeyMappingIntegrationTest --> KeyMappingUI : drives
SettingsManager --> InputMapWrapper : updates
SettingsManager --> ConfigFileAdapter : persists
KeyMappingUI --> SettingsManager : queries
KeyMappingUI --> InputMapWrapper : remaps at runtime
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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)
📝 WalkthroughWalkthroughReorganized README to emphasize Current Development Status (Milestone 10) and contribution flow. Added a new GUT integration test Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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:
- The integration tests currently rely on several assumptions about
Settingsinternals (e.g., config path,load_input_mappings/save_input_mappingsformats,reset_to_defaultssemantics) that are only documented in comments; consider adding an explicit way to injectTEST_CONFIG_PATHor otherwise configure these behaviors so the tests are less brittle to internal changes. - The tests hardcode node paths like
Panel/Options/KeyMapContainer/PlayerKeyMap/KeyMappingSpeedUp/SpeedUpInputRemap, which will break easily on UI refactors; consider introducing helper methods or using@onready-style lookup functions in the scene/scripts so tests can resolve controls via more stable identifiers. - Assertions that compare
speed_up_btn.textdirectly to "W"/"Z" and rely on specific keycodes may fail on different keyboard layouts or if labels change; you could instead derive expected labels from theInputEventKeyor a shared key-to-label utility to keep tests resilient to presentation changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The integration tests currently rely on several assumptions about `Settings` internals (e.g., config path, `load_input_mappings`/`save_input_mappings` formats, `reset_to_defaults` semantics) that are only documented in comments; consider adding an explicit way to inject `TEST_CONFIG_PATH` or otherwise configure these behaviors so the tests are less brittle to internal changes.
- The tests hardcode node paths like `Panel/Options/KeyMapContainer/PlayerKeyMap/KeyMappingSpeedUp/SpeedUpInputRemap`, which will break easily on UI refactors; consider introducing helper methods or using `@onready`-style lookup functions in the scene/scripts so tests can resolve controls via more stable identifiers.
- Assertions that compare `speed_up_btn.text` directly to "W"/"Z" and rely on specific keycodes may fail on different keyboard layouts or if labels change; you could instead derive expected labels from the `InputEventKey` or a shared key-to-label utility to keep tests resilient to presentation changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@test/gut/test_integration_key_mapping.gd`:
- Around line 58-84: The test writes a ConfigFile to TEST_CONFIG_PATH but calls
settings_inst.load_input_mappings() which uses Settings' default path, so the
saved config is never loaded; either make settings_inst load the test file or
write the config to the Settings default path. Fix by updating the test to (a)
set Settings (or settings_inst) to use TEST_CONFIG_PATH before calling
settings_inst.load_input_mappings() or (b) save the ConfigFile to the same path
Settings expects; reference TEST_CONFIG_PATH,
settings_inst.load_input_mappings(), ConfigFile, TEST_ACTION and KEY_Z_CODE to
locate the relevant setup and assertions and ensure the loaded mapping matches
the written ["key:"+str(KEY_Z_CODE)].
- Around line 109-112: The test loads ConfigFile from TEST_CONFIG_PATH but the
remap save writes to Settings' default config path, so update the test to load
the config from the same location the save logic uses: replace the hardcoded
TEST_CONFIG_PATH with the runtime path obtained from the Settings singleton
(call the Settings accessor used by the remap save code, e.g.,
Settings.get_singleton().get_default_config_path() or the equivalent method your
save logic uses), then use ConfigFile.load(...) and assert against that loaded
file so the persistence verification targets the file actually written by the
remap button.
- Around line 141-143: The test assumes all events from
InputMap.action_get_events(TEST_ACTION) are InputEventKey but a reset can return
gamepad events causing "Runtime error accessing InputEventJoypadMotion
properties"; update the test (in test_integration_key_mapping.gd) to check each
event's type (use instanceof/InputEventKey) or filter events to only
InputEventKey before asserting, then assert that there is one InputEventKey and
that its physical_keycode equals KEY_W_CODE; reference
InputMap.action_get_events, InputEventKey, InputEventJoypadMotion, events and
physical_keycode when making the change.
- Around line 97-102: Tests call private methods speed_up_btn._on_pressed() and
speed_up_btn._input(...) which bypass Godot's signal/event flow and can miss
side effects like persistence; instead simulate real input and signals: trigger
the button press via the public API or Godot input/signal mechanisms (e.g. call
speed_up_btn.emit_signal("pressed") or use GUT's input simulation utilities to
send the Key event) so the control's internal handlers and save logic run
naturally; update the test to stop calling _on_pressed and _input directly and
use the public pressed signal or input simulation to reproduce the behavior.
- Around line 31-41: The test never uses the test config file because
load_input_mappings() is called without arguments and defaults to CONFIG_PATH;
update calls to Settings.load_input_mappings() in this test (e.g. the call
originating after before_each and any other occurrences) to pass
TEST_CONFIG_PATH explicitly so the test reads the test config, referencing the
Settings class and its load_input_mappings(path) method and the TEST_CONFIG_PATH
constant.
🧹 Nitpick comments (1)
test/gut/test_integration_key_mapping.gd (1)
51-52: Null assignment doesn't properly free the Settings instance.If Settings holds resources or has cleanup logic, setting
settings_inst = nullwithout callingfree()orqueue_free()may leak resources. Consider explicit cleanup if Settings is a RefCounted or Node-derived class.♻️ Proposed fix
if settings_inst: + if settings_inst is Node: + settings_inst.queue_free() + elif settings_inst is RefCounted: + pass # RefCounted auto-frees when no references remain settings_inst = null
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdtest/gut/test_integration_key_mapping.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
test/gut/test_integration_key_mapping.gd
[error] 69-69: GUT test failed: test_int_01_load_to_ui - Should have one event after load.
[error] 71-71: GUT test failed: test_int_01_load_to_ui - Loaded keycode should match config (expected 90, got 87).
[error] 79-79: GUT test failed: test_int_01_load_to_ui - UI label should show loaded custom key 'Z'.
[error] 111-112: GUT test failed: test_int_02_remap_persist - Config should have input section after save; mapping did not persist as expected.
[error] 111-112: GDScript errors: Unexpected errors (Variant NIL and similar) during remap persistence test.
[error] 141-143: GUT test failed: test_int_03_reset_via_ui - Reset did not produce default event; Runtime error accessing InputEventJoypadMotion properties.
🔇 Additional comments (4)
README.md (3)
135-138: LGTM!The development status section clearly communicates the active milestone and current focus areas for contributors.
216-234: LGTM!Play instructions are well-organized with clear steps for different environments (editor, local web testing, itch.io).
236-253: LGTM!Clear contribution guidelines with labeled issue categories help onboard new contributors effectively.
test/gut/test_integration_key_mapping.gd (1)
1-15: Good documentation header with test plan references.The header clearly documents the test plan, assumptions, and references. This helps maintainers understand the test's purpose and constraints.
✏️ 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_integration_key_mapping.gd`:
- Around line 29-37: The assignment to settings_inst inside before_each is
dead/unused and triggers an untyped_declaration warning; remove the line
"settings_inst = Settings.new()" from the before_each function (or if you
actually need it later, change it to a declared variable like "var
settings_inst: Settings = Settings.new()"), leaving existing usage of the
Settings autoload (e.g. Settings.load_input_mappings(),
Settings.reset_to_defaults()) unchanged so tests continue to use the singleton.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
requirements.txttest/gut/test_integration_key_mapping.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_integration_key_mapping.gd (4)
18-23: Clear, readable test constants.The constants make the test intent and expected keycodes easy to follow.
38-46: Cleanup logic looks good.Queue-free + config cleanup + one frame wait is a solid teardown pattern.
49-81: INT‑01 flow is solid and well‑asserted.Good end‑to‑end validation from config → InputMap → UI text.
132-163: INT‑03 coverage is comprehensive.Nice validation across UI, InputMap, and persisted config.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Line 36 assigns Settings.new() to an undeclared variable that is never used. Since Settings is an AutoLoad singleton (accessed directly throughout the test via Settings.load_input_mappings(), Settings.reset_to_defaults(), etc.), this instantiation creates an unused, unreferenced instance. Either remove the line entirely or, if a reference is needed later, declare it explicitly with var settings_inst: Settings = Settings.new(). Currently it triggers an untyped_declaration warning and constitutes dead code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/gut/test_integration_key_mapping.gd`:
- Around line 33-46: Remove the unresolved git conflict markers and accept the
parent version: delete the lines with <<<<<<<, =======, and >>>>>>> and replace
the HEAD block (including Settings.new()) with the loop that iterates over
Settings.ACTIONS, calls InputMap.action_erase_events(action) if the action
exists or InputMap.add_action(action) otherwise, then call
Settings.load_input_mappings(); ensure Settings.new() is removed since it was
dead code and the intended implementation uses Settings.ACTIONS and
Settings.load_input_mappings().
🧹 Nitpick comments (1)
test/gut/test_integration_key_mapping.gd (1)
104-109: Consider using signal-based simulation instead of direct private method calls.While line 14 documents this as intentional due to "input event mocking complexity in GUT," directly calling
_on_pressed()and_input()bypasses the normal signal/event flow. This can cause tests to pass while the actual user-facing flow fails, or vice versa.If GUT's input simulation proves too complex, emitting signals is a more realistic approach that still exercises the connected handlers.
♻️ Suggested alternative using signal emission
# Simulate remap to "Z" (direct calls as in ref tests) - speed_up_btn.button_pressed = true - speed_up_btn._on_pressed() # Start listening + speed_up_btn.pressed.emit() # Trigger connected handlers + await get_tree().process_frame var temp_event := InputEventKey.new() temp_event.physical_keycode = KEY_Z_CODE temp_event.pressed = true - speed_up_btn._input(temp_event) # Triggers erase/add/save + Input.parse_input_event(temp_event) # Route through input system + await get_tree().process_frame
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_integration_key_mapping.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_integration_key_mapping.gd (4)
1-25: LGTM!The header documentation is comprehensive, linking to the test plan and explaining assumptions. Constants are properly typed and named.
48-57: LGTM!The cleanup logic properly validates the instance before freeing, handles file deletion with error checking, and awaits a frame for the scene tree to process.
61-91: LGTM!The test properly filters
InputEventKeyevents (lines 71-74) to avoid type mismatch errors with gamepad events. The test flow correctly validates the config → InputMap → UI synchronization chain.
144-173: LGTM!This test correctly uses
reset_btn.pressed.emit()(line 158) to trigger the reset via signal emission rather than calling private methods directly. The type filtering forInputEventKeyevents is consistent with the other tests, and the verification of both UI and config state is thorough.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- test_integration_key_mapping.gd still contains unresolved merge conflict markers (e.g.,
<<<<<<< HEAD/>>>>>>> parent of db8a3ab), so the tests won’t compile—please resolve the conflicting setup block before merging. - The new GUT tests read/write
user://settings.cfg, which is the actual runtime config, meaning running the suite will delete/overwrite developers’ real settings; please point the tests at an isolated temp path or mock Settings so they don’t touch production config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- test_integration_key_mapping.gd still contains unresolved merge conflict markers (e.g., `<<<<<<< HEAD`/`>>>>>>> parent of db8a3ab`), so the tests won’t compile—please resolve the conflicting setup block before merging.
- The new GUT tests read/write `user://settings.cfg`, which is the actual runtime config, meaning running the suite will delete/overwrite developers’ real settings; please point the tests at an isolated temp path or mock Settings so they don’t touch production config.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The GUT tests currently write to
user://settings.cfg, which risks clobbering real user/editor settings; consider routingSettingsto a test-specific config path or using a temporary file for isolation. - The integration tests depend heavily on concrete scene node paths and calls to private methods like
_on_pressed,_input, andupdate_button_text, which makes them brittle to UI refactors; you might expose higher-level APIs or lookup helpers to reduce this coupling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The GUT tests currently write to `user://settings.cfg`, which risks clobbering real user/editor settings; consider routing `Settings` to a test-specific config path or using a temporary file for isolation.
- The integration tests depend heavily on concrete scene node paths and calls to private methods like `_on_pressed`, `_input`, and `update_button_text`, which makes them brittle to UI refactors; you might expose higher-level APIs or lookup helpers to reduce this coupling.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
188-193:⚠️ Potential issue | 🟡 MinorMilestone numbering looks inconsistent.
The status section says Milestone 10, but the roadmap still labels planned work as Milestone 9. Consider aligning the numbering or clarifying if Milestone 9 is still active.
🤖 Fix all issues with AI agents
In `@test/gut/test_integration_key_mapping.gd`:
- Around line 18-48: The test uses the production config path referenced by
TEST_CONFIG_PATH and wipes it in before_each()/after_each(); change
TEST_CONFIG_PATH to an isolated path (e.g., "user://test_key_mapping.cfg") or
implement the backup/restore pattern used in test_preserve_other_sections.gd: in
before_each() save the existing user://settings.cfg to a temp, operate on the
file, and in after_each() restore the backup and remove the temp; update
references in before_each(), after_each(), and Settings.load_input_mappings()
accordingly so the test no longer mutates the real user settings.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdtest/gut/test_integration_key_mapping.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 (5)
README.md (2)
135-167: Clear current-status section.
Nice summary of the Milestone 10 focus and current feature set.
214-249: Play instructions and contribution guidance read well.
The step-by-step flow and label guidance are clear and easy to follow.test/gut/test_integration_key_mapping.gd (3)
1-15: Test plan header is clear and well-scoped.
Good context for the INT-01..03 coverage.
54-83: Solid load-to-UI verification.
Filtering toInputEventKeykeeps the test resilient to non-key events.
137-166: Reset flow looks good.
Using the reset button signal keeps the test aligned with real UI behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Use an isolated test config path or implement backup/restore. TEST_CONFIG_PATH points to user://settings.cfg—the production path where users' real settings are stored. Deleting it in before_each() and after_each() will wipe a developer's settings when running tests locally. Two solutions exist: Simpler: Use an isolated test path like other tests in the suite (e.g., "user://test_key_mapping.cfg"), matching the pattern used in test_settings.gd, test_audio_manager.gd, etc. Current pattern: Implement backup/restore as shown in test_preserve_other_sections.gd if the production path must be tested directly. The backup/restore approach from test_preserve_other_sections.gd is a proven pattern in the codebase and can be adapted here.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
1 participant
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 integration tests verifying key mapping settings correctly sync between configuration, InputMap, and UI, and update README with current milestone status, play instructions, and contribution labeling guidance.
Documentation:
Tests:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.