Unit Test Plan: 1. Settings Logic — Default Mappings & Persistence, GitHub workflows maintenance - #354
Conversation
ikostan
added this to the
🚀 Milestone 10: UI & Input Improvements, System Refactors, Testing & Documentation milestone
Reviewer's GuideAdds a focused GUT unit test suite for the InputRemapButton control (covering remap flows, device-specific behavior, event lookup, logging, and edge cases) and bumps several GitHub Actions (CodeQL, upload-sarif, and release-drafter) to newer pinned versions. Sequence diagram for InputRemapButton keyboard remap flow under testsequenceDiagram
actor GutTest
participant InputRemapButton
participant InputMap
participant Logger
GutTest->>InputRemapButton: set_action_name(action_name)
GutTest->>InputRemapButton: start_listening_for_device(device_keyboard)
InputRemapButton->>Logger: log_listening_started(action_name, device_keyboard)
GutTest->>InputRemapButton: provide_input_event(keyboard_event)
InputRemapButton->>InputMap: action_erase_events(action_name)
InputMap-->>InputRemapButton: events_cleared
InputRemapButton->>InputMap: action_add_event(action_name, keyboard_event)
InputMap-->>InputRemapButton: event_added
InputRemapButton->>InputRemapButton: update_label_from_event(keyboard_event)
InputRemapButton-->>GutTest: listening_stopped
InputRemapButton->>Logger: log_remap_success(action_name, keyboard_event)
GutTest->>InputMap: get_action_list(action_name)
InputMap-->>GutTest: [keyboard_event]
GutTest->>GutTest: assert_event_matches(keyboard_event)
Sequence diagram for InputRemapButton wrong-device handling and persistence under testsequenceDiagram
actor GutTest
participant InputRemapButton
participant InputMap
participant Logger
GutTest->>InputRemapButton: set_action_name(action_name)
GutTest->>InputRemapButton: start_listening_for_device(device_gamepad)
InputRemapButton->>Logger: log_listening_started(action_name, device_gamepad)
GutTest->>InputRemapButton: provide_input_event(keyboard_event)
InputRemapButton->>InputRemapButton: is_event_from_expected_device(keyboard_event, device_gamepad)
InputRemapButton-->>GutTest: ignore_event
InputRemapButton->>Logger: log_wrong_device(action_name, keyboard_event, device_gamepad)
GutTest->>InputRemapButton: provide_input_event(gamepad_event)
InputRemapButton->>InputMap: action_erase_events(action_name)
InputMap-->>InputRemapButton: events_cleared
InputRemapButton->>InputMap: action_add_event(action_name, gamepad_event)
InputMap-->>InputRemapButton: event_added
InputRemapButton->>InputRemapButton: update_label_from_event(gamepad_event)
InputRemapButton-->>GutTest: listening_stopped
InputRemapButton->>Logger: log_remap_success(action_name, gamepad_event)
GutTest->>InputMap: get_action_list(action_name)
InputMap-->>GutTest: [gamepad_event]
GutTest->>GutTest: assert_persistence_after_remap()
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 (3)
📝 WalkthroughWalkthroughAdds a rendering configuration entry to Changes
Sequence Diagram(s)(Skipped — changes are configuration, tests, and CI action reference updates without new multi-component runtime control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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:
- Several tests call internal methods like
_pressed()and_input()directly; if possible, consider driving these paths via the public UI/scene API (e.g., simulating button presses or input propagation) so the tests more closely match real usage and are less coupled to internal implementation details. test_irb_07describes verifying logging but currently only asserts that remapping occurred; if you want to truly cover logging behavior, consider using GUT spying or injecting a logging dependency so you can assert on the log output rather than relying on a comment.- The tests assert exact label strings like "Left Button" and
"Gamepad Button " + str(JOY_BUTTON_B); if these labels are likely to change (e.g., via localization or UI tweaks), you may want to centralize expected labels or derive them using the same helper used in production code to reduce brittleness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests call internal methods like `_pressed()` and `_input()` directly; if possible, consider driving these paths via the public UI/scene API (e.g., simulating button presses or input propagation) so the tests more closely match real usage and are less coupled to internal implementation details.
- `test_irb_07` describes verifying logging but currently only asserts that remapping occurred; if you want to truly cover logging behavior, consider using GUT spying or injecting a logging dependency so you can assert on the log output rather than relying on a comment.
- The tests assert exact label strings like "Left Button" and `"Gamepad Button " + str(JOY_BUTTON_B)`; if these labels are likely to change (e.g., via localization or UI tweaks), you may want to centralize expected labels or derive them using the same helper used in production code to reduce brittleness.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: 2
🤖 Fix all issues with AI agents
In `@test/gut/test_input_remap_button.gd`:
- Around line 161-168: In test_irb_06 replace the non-existent call to
button.update_label() with button.update_button_text(), and then either extend
the get_event_label(event) function to handle InputEventMouseButton (map
MOUSE_BUTTON_RIGHT to "Right Button" and return that label for mouse button
events) or adjust the test to avoid asserting mouse-button labels if mouse
support isn't intended; locate and update the get_event_label implementation
(and any mouse-label mapping) so it recognizes InputEventMouseButton alongside
keyboard/joypad handling.
- Around line 37-61: The test calls a non-existent method and uses the wrong
property and event setup: replace the call to button._pressed() with either
button._on_pressed() or button.emit_signal("pressed") to start remapping (the
class exposes _on_pressed/pressed signal), change assertions from
button.is_remapping to button.listening (use assert_true(button.listening) and
assert_false(button.listening)), and mark the simulated InputEventKey as a key
press by setting new_key.pressed = true before calling button._input(new_key) so
the input handler recognizes the event.
♻️ Duplicate comments (5)
test/gut/test_input_remap_button.gd (5)
66-85: Same critical issue:_pressed()and_input()calls will fail.This test has the same issues as IRB-01. Additionally, the expected label format
"Gamepad Button " + str(JOY_BUTTON_B)(line 84) is an assumption that should be verified against the actualInputRemapButtonimplementation.
90-104: Same critical issue with_pressed()and_input()calls.
109-128: Same critical issue with_pressed()and_input()calls.Additionally, line 114 captures
button.textbefore any explicit label initialization. Depending on the implementation,initial_textmay be empty or a default value rather than the expected prior key label. Consider calling an initialization method or setting the label explicitly after adding the prior event.
173-183: Same critical issue with_pressed()and_input()calls; test doesn't verify logging.Beyond the method call issues, this test doesn't actually verify logging—it only confirms the remap succeeded. GUT provides
gut.get_logger()for capturing output, or you could use a spy/mock onGlobals.log_infoif that's the logging mechanism.Consider either implementing proper log verification or renaming this test to reflect what it actually tests.
188-203: Same critical issue with_pressed()and_input()calls.The edge case test logic is sound once the method calls are fixed.
🧹 Nitpick comments (1)
test/gut/test_input_remap_button.gd (1)
9-11: Fix type annotation syntax and usage.Two issues with the variable declarations:
- Line 9: Inconsistent spacing in type annotation (
: =should be:=or: Script =)- Line 11: The type annotation
button: InputRemapButtonwon't work becauseInputRemapButtonis aScriptvariable, not a class type. In GDScript, you cannot use a preloaded script as a type hint directly.♻️ Proposed fix
-var InputRemapButton: = preload("res://scripts/input_remap_button.gd") # Adjust path as needed. +const InputRemapButton := preload("res://scripts/input_remap_button.gd") # Adjust path as needed. -var button: InputRemapButton +var button: Button # Or use dynamic typing: var button
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
project.godottest/gut/test_input_remap_button.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
test/gut/test_input_remap_button.gd
[error] 49-49: SCRIPT ERROR: Invalid call. Nonexistent function '_pressed' in base 'Button (InputRemapButton)'.
[error] 73-73: SCRIPT ERROR: Invalid call. Nonexistent function '_pressed' in base 'Button (InputRemapButton)'.
[error] 92-92: SCRIPT ERROR: Invalid call. Nonexistent function '_pressed' in base 'Button (InputRemapButton)'.
[error] 1-1: SCRIPT ERROR: Invalid call. Nonexistent function '_pressed' in base 'Button (InputRemapButton)'.
[error] 166-166: SCRIPT ERROR: Invalid call. Nonexistent function 'update_label' in base 'Button (InputRemapButton)'.
[error] 176-176: SCRIPT ERROR: Invalid call. Nonexistent function '_pressed' in base 'Button (InputRemapButton)'.
[error] 191-191: SCRIPT ERROR: Invalid call. Nonexistent function '_pressed' in base 'Button (InputRemapButton)'.
🔇 Additional comments (3)
project.godot (1)
115-115: LGTM!Setting
default_texture_filter=0(Nearest) is appropriate for maintaining sharp pixel rendering. This is consistent with the GL Compatibility renderer configuration.test/gut/test_input_remap_button.gd (2)
17-24: LGTM!The test setup correctly initializes the InputMap action and uses
add_child_autofreefor proper cleanup.
29-32: LGTM!Proper cleanup with frame await to ensure clean state between tests.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Refactor tests to use InputEventKey and InputEventJoypadButton instead of mouse events where appropriate. Adjust assertions to account for multiple event types and update expected button text values. This improves test accuracy and better reflects intended input remapping behavior.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_irb_03the test description and intent mention mouse button handling andMOUSE_BUTTON_LABELS, but the implementation remaps a keyboardInputEventKeyand asserts a simple "Left" label—consider updating this test to actually useInputEventMouseButtonor adjusting the description so behavior and coverage align. - The tests call
_on_pressed()and_input()directly onInputRemapButton, which couples them to internal implementation details; if feasible, prefer triggering via the public button press/input pathway (e.g., emitting the pressed signal or using Godot’s input simulation) to keep the tests resilient to internal refactors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_irb_03` the test description and intent mention mouse button handling and `MOUSE_BUTTON_LABELS`, but the implementation remaps a keyboard `InputEventKey` and asserts a simple "Left" label—consider updating this test to actually use `InputEventMouseButton` or adjusting the description so behavior and coverage align.
- The tests call `_on_pressed()` and `_input()` directly on `InputRemapButton`, which couples them to internal implementation details; if feasible, prefer triggering via the public button press/input pathway (e.g., emitting the pressed signal or using Godot’s input simulation) to keep the tests resilient to internal refactors.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
🤖 Fix all issues with AI agents
In `@test/gut/test_input_remap_button.gd`:
- Line 9: Replace the invalid typed preload `var InputRemapButton: =
preload("res://scripts/input_remap_button.gd")` with a const script alias so the
type hint used elsewhere works; change it to `const InputRemapButton =
preload("res://scripts/input_remap_button.gd")` (keep the same identifier so `:
InputRemapButton` type hints on line 11 remain valid) and remove the stray
colon/equal syntax.
🧹 Nitpick comments (3)
test/gut/test_input_remap_button.gd (3)
89-107: Align test descriptions with actual key-based inputs.IRB-03 and IRB-06 describe mouse behavior, but the tests use
InputEventKey. Either update the descriptions to keyboard, or switch inputs/assertions to mouse events if mouse coverage is intended.Also applies to: 163-172
175-189: Add an assertion for logging if IRB-07 is meant to validate it.Currently the test performs a remap but doesn’t verify log output. If logging is part of the requirement, consider using a GUT spy/capture to assert on the log entry.
27-33: Use GUT'swait_idle_frames()helper instead of rawawait get_tree().process_frame.GUT properly awaits async
after_each()functions, so the teardown is not at risk. However, GUT recommends using its own wait helpers (likeawait wait_idle_frames(1)) instead of arbitrary awaits—this makes it explicit to the test runner that it's paused and improves clarity.func after_each() -> void: if InputMap.has_action(TEST_ACTION): InputMap.erase_action(TEST_ACTION) await wait_idle_frames(1)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_input_remap_button.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)
test/gut/test_input_remap_button.gd (5)
35-60: IRB-01 coverage looks solid.Clear setup, remap flow, and assertions for keyboard vs gamepad event retention.
63-86: IRB-02 looks good.Device switch, remap flow, and label expectation are consistent.
110-133: IRB-04 behavior is well covered.Wrong-device input is ignored and listening state is preserved as expected.
135-160: IRB-05 looks good.Covers mixed events, device filtering, and empty action behavior clearly.
192-211: IRB-08 is solid.Good edge-case coverage for empty mappings and remap flow.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Deleted test cases related to remapping mouse button events and updating button labels for mouse inputs. The affected tests include IRB-03 and IRB-06, as well as related assertions in IRB-05.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_irb_02andtest_irb_04the assertions onbutton.textuse hardcoded strings (e.g., "B", "Press a key or controller button/axis...") which makes the tests brittle against localization or label changes; consider deriving the expected text the same way the production code does (e.g., viaOS.get_keycode_stringor a shared constant). - The preload for
InputRemapButtonis annotated with a comment "Adjust path as needed"; it would be more robust to either fix this to the definitive path used in the project or remove the comment to avoid confusion about where the canonical script actually lives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_irb_02` and `test_irb_04` the assertions on `button.text` use hardcoded strings (e.g., "B", "Press a key or controller button/axis...") which makes the tests brittle against localization or label changes; consider deriving the expected text the same way the production code does (e.g., via `OS.get_keycode_string` or a shared constant).
- The preload for `InputRemapButton` is annotated with a comment "Adjust path as needed"; it would be more robust to either fix this to the definitive path used in the project or remove the comment to avoid confusion about where the canonical script actually lives.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
All mouse related test code was removed since it is not relevant (mouse settings was not implemented) |
Bumps [release-drafter/release-drafter](https://github.com/release-drafter/release-drafter) from 6.1.0 to 6.2.0. - [Release notes](https://github.com/release-drafter/release-drafter/releases) - [Commits](release-drafter/release-drafter@b1476f6...6db134d) --- updated-dependencies: - dependency-name: release-drafter/release-drafter dependency-version: 6.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.31.10 to 4.31.11. - [Release notes](https://github.com/github/codeql-action/releases) - [Commits](github/codeql-action@v4.31.10...v4.31.11) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.31.11 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
The syntax var InputRemapButton: = preload(...) is invalid GDScript (the space between : and = breaks the syntax). Line 11 uses InputRemapButton as a type hint, so use a const alias instead:
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/gut/test_input_remap_button.gd`:
- Line 114: Update the stale test comment on the "Get matching event for device"
test to accurately reflect that the test only includes keyboard and gamepad
events (mouse events were removed); change the phrase "mixed keyboard, mouse,
and gamepad events" to something like "mixed keyboard and gamepad events" and
keep the rest of the description referencing
get_matching_event(DeviceType.KEYBOARD) and expected behavior intact so the
comment matches the actual test setup in test_input_remap_button.gd.
🧹 Nitpick comments (1)
test/gut/test_input_remap_button.gd (1)
139-153: Consider enhancing log verification when GUT spy support is available.The test acknowledges it doesn't verify actual log output. GUT's
gut.get_logger()or double/spy capabilities could potentially be used to verifyGlobals.log_infocalls if needed for more rigorous coverage.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_input_remap_button.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 (7)
test/gut/test_input_remap_button.gd (7)
1-12: LGTM!The file header, imports, and constants are well-structured. The
constpreload syntax is correct, and the test action constant provides good test isolation.
15-24: LGTM!Good test isolation with proper InputMap reset and
add_child_autofreefor automatic node cleanup.
27-32: LGTM!The teardown properly cleans up the test action and the
await get_tree().process_frameensures deferred operations complete before the next test.
35-60: LGTM!Test properly verifies keyboard remapping behavior: the old keyboard event is replaced while the gamepad event is preserved. The lambda-based assertions with
.any()provide flexible event verification.
63-86: LGTM!Test correctly verifies gamepad remapping with device-type awareness, ensuring the keyboard binding is preserved while the gamepad binding is updated.
89-111: LGTM!Test correctly verifies that wrong-device events are ignored during remapping, with the button remaining in listening mode awaiting a valid input.
156-175: LGTM!Good edge-case coverage for remapping when no prior events exist, ensuring the implementation handles empty action mappings gracefully.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The comment mentions "mixed keyboard, mouse, and gamepad events" but the test only adds keyboard and gamepad events—no mouse events are included (consistent with the PR note that mouse tests were removed).
The tests call _on_pressed() and _input() directly on InputRemapButton, which couples them to internal implementation details; if feasible, prefer triggering via the public button press/input pathway (e.g., emitting the pressed signal or using Godot’s input simulation) to keep the tests resilient to internal refactors.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_irb_07the logging behavior is only mentioned in comments and not actually asserted; either wire it up to GUT's spying/override mechanism or narrow the test’s scope to only what you can verify to avoid a misleading test name/description. - There is a fair bit of repeated boilerplate for creating and adding
InputEventKey/InputEventJoypadButtoninstances across tests; consider extracting small helper functions (e.g.,make_key_event(code)/make_joypad_event(button)) to keep the intent of each test focused on behavior rather than setup.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_irb_07` the logging behavior is only mentioned in comments and not actually asserted; either wire it up to GUT's spying/override mechanism or narrow the test’s scope to only what you can verify to avoid a misleading test name/description.
- There is a fair bit of repeated boilerplate for creating and adding `InputEventKey`/`InputEventJoypadButton` instances across tests; consider extracting small helper functions (e.g., `make_key_event(code)` / `make_joypad_event(button)`) to keep the intent of each test focused on behavior rather than setup.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
🤖 Fix all issues with AI agents
In `@test/gut/test_input_remap_button.gd`:
- Around line 50-60: Replace the hardcoded label assertion with a call to the
control's label helper: in the test block that creates new_key and checks
button.text, change the assertion to compare button.text with
button.get_event_label(new_key) (use the existing button instance and the
new_key InputEventKey) so the test relies on get_event_label() instead of
OS.get_keycode_string().
🧹 Nitpick comments (1)
test/gut/test_input_remap_button.gd (1)
143-158: Test does not verify its stated purpose (logging).The test description claims to verify log output, but the implementation only confirms the remap occurred—functionality already covered by other tests. Without actual log verification, this test provides limited additional value.
Consider either:
- Using GUT's
double()and spy capabilities to verifyGlobals.log_infowas called with expected arguments- Renaming/removing this test if log verification isn't feasible, to avoid suggesting coverage that doesn't exist
Example: Using GUT spy to verify logging
func test_irb_07() -> void: # Spy on Globals if log_info is mockable var globals_double = double(Globals).new() stub(globals_double, "log_info").to_do_nothing() # ... perform remap ... assert_called(globals_double, "log_info")Note: This requires
Globalsto be injectable or mockable in the button.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/gut/test_input_remap_button.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 (6)
test/gut/test_input_remap_button.gd (6)
1-12: LGTM!Header comments clearly reference the test plan, and the preload pattern using
constis correct. The typed variable declarations are clean and follow GDScript conventions.
15-32: LGTM!Setup and teardown are well-structured. The
before_eachproperly isolates tests by resetting the InputMap, andadd_child_autofreeensures proper cleanup. Theawait get_tree().process_frameinafter_eachcorrectly allows pending operations to complete.
64-88: LGTM!Test correctly validates gamepad remapping while preserving keyboard events. Using
button.get_event_label(new_gamepad)for the text assertion (line 87) is a good approach that avoids hardcoding expected labels.
91-115: LGTM!Test correctly validates that wrong-device events are ignored during remapping. The assertions properly verify that the InputMap remains unchanged, the button text stays at the listening prompt, and the
listeningstate persists.
118-140: LGTM!Test thoroughly covers
get_matching_event()functionality for both device types and the null case when no events exist. The comment accurately reflects the test coverage (updated per past review feedback).
161-181: LGTM - good edge case coverage.Test properly validates the empty-action scenario. The same
physical_keycodevsOS.get_keycode_string()consideration fromtest_irb_01applies here (line 180), but the test logic is sound.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…n the assertion. The test should assert button.text == button.get_event_label(new_key) rather than button.text == OS.get_keycode_string(KEY_B) to match the pattern used in test_irb_02 and ensure the test validates the actual implementation logic. This avoids fragility from duplicating the label derivation logic and makes the test more maintainable if get_event_label() changes.
…/codeql-action-4.31.11 Bump github/codeql-action from 4.31.10 to 4.31.11
…e-drafter/release-drafter-6.2.0 Bump release-drafter/release-drafter from 6.1.0 to 6.2.0
|
@sourcery-ai review |
ikostan
added
tools
github actions
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_irb_07, the comment mentions using a spy forGlobals.log_infobut the test currently doesn’t assert anything about logging; either wire up an actual spy/assert on the logging behavior or tighten the description so the test’s intent matches what’s being verified (i.e., just that remapping succeeds). - The tests rely on
await get_tree().process_frameafterInput.parse_input_event; if the remap logic can be triggered synchronously, consider using direct method calls or signals instead to reduce flakiness and make the tests more deterministic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_irb_07`, the comment mentions using a spy for `Globals.log_info` but the test currently doesn’t assert anything about logging; either wire up an actual spy/assert on the logging behavior or tighten the description so the test’s intent matches what’s being verified (i.e., just that remapping succeeds).
- The tests rely on `await get_tree().process_frame` after `Input.parse_input_event`; if the remap logic can be triggered synchronously, consider using direct method calls or signals instead to reduce flakiness and make the tests more deterministic.
## Individual Comments
### Comment 1
<location> `.github/workflows/snyk.yml:46` </location>
<code_context>
- name: "Upload Snyk Code SARIF to GitHub"
- uses: "github/codeql-action/upload-sarif@v4.31.10"
+ uses: "github/codeql-action/upload-sarif@v4.31.11"
if: "always() && hashFiles('snyk-code.sarif') != ''"
with:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider pinning `github/codeql-action/upload-sarif` by commit SHA instead of a version tag for stronger supply-chain security.
In `trivy.yml` this action is already pinned by commit SHA for stronger integrity guarantees. For consistency and better supply‑chain hardening, please pin these Snyk `upload-sarif` steps to the specific commit SHA that corresponds to `v4.31.11` as well.
Suggested implementation:
```
- name: "Upload Snyk Code SARIF to GitHub"
# github/codeql-action/upload-sarif v4.31.11
uses: "github/codeql-action/upload-sarif@<COMMIT-SHA-FOR-v4.31.11>"
if: "always() && hashFiles('snyk-code.sarif') != ''"
with:
sarif_file: "snyk-code.sarif"
continue-on-error: true
- name: "Upload Snyk Open Source SARIF to GitHub"
# github/codeql-action/upload-sarif v4.31.11
uses: "github/codeql-action/upload-sarif@<COMMIT-SHA-FOR-v4.31.11>"
if: "always() && hashFiles('snyk-os.sarif') != ''"
with:
sarif_file: "snyk-os.sarif"
```
1. Replace `<COMMIT-SHA-FOR-v4.31.11>` with the actual commit SHA that the `v4.31.11` tag of `github/codeql-action/upload-sarif` points to. You can retrieve this from the GitHub UI (`tags` → `v4.31.11` → copy the commit SHA) or via `git ls-remote https://github.com/github/codeql-action v4.31.11`.
2. Ensure the same SHA is used wherever `github/codeql-action/upload-sarif@v4.31.11` appears in the repository, for consistency with how it is pinned in `trivy.yml`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ikostan
changed the title
…arif by commit SHA instead of a version tag for stronger supply-chain security. In trivy.yml this action is already pinned by commit SHA for stronger integrity guarantees. For consistency and better supply‑chain hardening, please pin these Snyk upload-sarif steps to the specific commit SHA that corresponds to v4.31.11 as well.
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
Update security and release GitHub workflows and add unit tests for input remapping UI behavior.
CI:
Tests:
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.