UI audio logic unit tests#746
Merged
Merged
Conversation
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 6.0.1 to 7.0.0. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@e79a696...fb8b358) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…v/codecov-action-7.0.0 Bump codecov/codecov-action from 6.0.1 to 7.0.0
Create a comprehensive GUT test suite to validate that UI interaction and navigation events are correctly intercepted, interpreted, and translated into sound effect playback requests.
ikostan
added this to the
Milestone 20: Resource-Based Settings & UI Acoustic Integration milestone
7 tasks
Contributor
Reviewer's GuideAdds diagnostic AudioManager APIs to inspect active SFX, tightens VolumeSlider value handling, and introduces a comprehensive GUT-based UI interaction test suite for audio settings, plus a minor CI workflow bump and docs for the milestone. Sequence diagram for UI audio tests using diagnostic AudioManager APIssequenceDiagram
actor TestAudioSettings
participant VolumeSlider
participant AudioManager
TestAudioSettings->>VolumeSlider: _on_value_changed(out_of_range_value)
VolumeSlider->>VolumeSlider: clamp(new_value, min_value, max_value)
alt [value changed]
VolumeSlider->>AudioManager: play_sfx(sound_key, position, volume_db, pitch_scale)
end
TestAudioSettings->>AudioManager: is_any_sfx_playing()
TestAudioSettings->>AudioManager: get_active_sfx_stream_path()
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
More reviews will be available in 11 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a comprehensive GUT test suite for audio settings interaction scenarios, introduces AudioManager SFX inspection APIs, validates volume slider input bounds, and updates a GitHub Actions dependency version. ChangesAudio Settings Interaction Test Suite
CI Coverage Action Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tests reach into AudioManager internals (e.g.,
_sfx_pool,get_active_sfx_playback_count) and controller_on_*handlers directly, which makes them tightly coupled to implementation details—consider driving behavior via the public UI/API or signals where possible to keep the tests resilient to refactors. test_back_button_triggers_exitcurrently callsqueue_free()onaudio_instancedirectly instead of exercising the actual back-button handler, so it isn't validating the real interaction path; consider invoking the same method or signal the UI uses for back navigation.- Check whether
test/gut/test_audio_settings_interaction.gd.uidneeds to be versioned, as committing engine-generated UID/metadata files can cause noisy diffs and merge conflicts if they are regenerated frequently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests reach into AudioManager internals (e.g., `_sfx_pool`, `get_active_sfx_playback_count`) and controller `_on_*` handlers directly, which makes them tightly coupled to implementation details—consider driving behavior via the public UI/API or signals where possible to keep the tests resilient to refactors.
- `test_back_button_triggers_exit` currently calls `queue_free()` on `audio_instance` directly instead of exercising the actual back-button handler, so it isn't validating the real interaction path; consider invoking the same method or signal the UI uses for back navigation.
- Check whether `test/gut/test_audio_settings_interaction.gd.uid` needs to be versioned, as committing engine-generated UID/metadata files can cause noisy diffs and merge conflicts if they are regenerated frequently.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jun 11, 2026 11:58p.m. | Review ↗ |
| JavaScript | |
Jun 11, 2026 11:58p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![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: 5
🧹 Nitpick comments (2)
test/gut/test_audio_settings_interaction.gd (2)
12-12: ⚖️ Poor tradeoffMove scene loading to runtime to avoid parse-time dependency.
Loading the scene at class scope (parse time) can fail if the
GamePathsautoload isn't yet available. GUT best practice is to load scenes inbefore_each()to ensure autoloads are initialized.♻️ Proposed fix to defer scene loading
-# Load the scene using the shared GamePaths -var audio_scene: PackedScene = load(GamePaths.AUDIO_SETTINGS_SCENE) +var audio_scene: PackedScene var audio_instance: ControlThen in
before_each():func before_each() -> void: + if not audio_scene: + audio_scene = load(GamePaths.AUDIO_SETTINGS_SCENE) _clear_pool_players() audio_instance = audio_scene.instantiate() as Control🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` at line 12, The test currently loads the scene at parse time via the top-level var audio_scene: PackedScene = load(GamePaths.AUDIO_SETTINGS_SCENE), which can fail if the GamePaths autoload isn't initialized; change this so the declaration only reserves the variable (e.g., var audio_scene: PackedScene) and move the load() call into the test suite setup (implement or update before_each() to assign audio_scene = load(GamePaths.AUDIO_SETTINGS_SCENE)) so the scene is loaded at runtime after autoloads are ready; update any uses of audio_scene in tests accordingly.
42-45: ⚖️ Poor tradeoffAvoid coupling tests to private AudioManager implementation.
Directly accessing
AudioManager._sfx_pool(underscore indicates private) couples these tests to internal pooling implementation. If AudioManager refactors its pooling strategy, these tests will break.Consider adding public test helpers to AudioManager:
clear_sfx_pool_for_testing() -> voidis_any_sfx_playing() -> boolOr use GUT's
partial_double()to spy on the AudioManager if these helpers shouldn't be in production code.Also applies to: 51-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 42 - 45, The test is directly accessing the private AudioManager._sfx_pool which couples tests to internal implementation; update tests to use a public test API or a test spy instead: add/publicize test helpers on AudioManager such as clear_sfx_pool_for_testing() to stop and null out SFX players and is_any_sfx_playing() to check play state, then replace direct references to AudioManager._sfx_pool in test/gut/test_audio_settings_interaction.gd with calls to those helpers, or alternatively use GUT's partial_double() to spy on AudioManager methods rather than touching the private _sfx_pool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Line 151: The tests set slider.value programmatically which does not emit
Godot's value_changed signal, so the connected handlers (the slider's
value_changed callbacks in AudioManager) are never invoked; after assigning
slider.value = X in the tests (for the slider instance named slider) emit the
signal manually using slider.emit_signal("value_changed", X) (or call the same
handler directly) so the AudioManager handlers run and the test exercises the
actual slider interaction path; apply this change for all occurrences where
slider.value is assigned.
- Line 195: The stress test calls audio_instance._on_music_mute_toggled(...) in
a loop but never gives the control focus, so focus-gate logic (has_focus() in
audio_settings.gd) prevents audio from playing; before the loop that exercises
audio_instance._on_music_mute_toggled, explicitly grab focus on the toggle
control used by audio settings (e.g., call grab_focus() on the music mute button
or the same UI node whose has_focus() gate is checked) so the toggles will pass
the focus check and trigger audio during the stress test.
- Line 22: Remove the ineffective focus call: the test calls
audio_instance.grab_focus() which does nothing because audio_instance is typed
as Control and Control nodes are not focusable by default; delete the
audio_instance.grab_focus() invocation and rely on the existing await
process_frame to ensure the UI is ready (keep references to audio_instance and
await process_frame unchanged).
- Around line 86-89: The test currently calls audio_instance.queue_free()
directly which bypasses the UI logic; update test_back_button_triggers_exit to
simulate the actual back-button interaction by invoking the back button handler
(e.g. call audio_instance._on_back_button_pressed() or emit the back button
signal) and then assert audio_instance.is_queued_for_deletion() so the test
verifies the full handler flow rather than forcing deletion directly.
- Line 132: Setting slider.value programmatically won't emit the value_changed
signal, so replace the bare assignment (slider.value = 0.5) with a call that
actually triggers the handler: either emit the signal
(slider.emit_signal("value_changed", 0.5)) or call the control's handler
directly (the test should invoke the same handler used in the UI, e.g.,
_on_<control>_value_changed or whatever handler is wired to this slider),
ensuring the slider instance named "slider" is the one passed to that handler.
---
Nitpick comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Line 12: The test currently loads the scene at parse time via the top-level
var audio_scene: PackedScene = load(GamePaths.AUDIO_SETTINGS_SCENE), which can
fail if the GamePaths autoload isn't initialized; change this so the declaration
only reserves the variable (e.g., var audio_scene: PackedScene) and move the
load() call into the test suite setup (implement or update before_each() to
assign audio_scene = load(GamePaths.AUDIO_SETTINGS_SCENE)) so the scene is
loaded at runtime after autoloads are ready; update any uses of audio_scene in
tests accordingly.
- Around line 42-45: The test is directly accessing the private
AudioManager._sfx_pool which couples tests to internal implementation; update
tests to use a public test API or a test spy instead: add/publicize test helpers
on AudioManager such as clear_sfx_pool_for_testing() to stop and null out SFX
players and is_any_sfx_playing() to check play state, then replace direct
references to AudioManager._sfx_pool in
test/gut/test_audio_settings_interaction.gd with calls to those helpers, or
alternatively use GUT's partial_double() to spy on AudioManager methods rather
than touching the private _sfx_pool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2d079f28-8ab6-4d74-bbde-a5a0a4f76a38
📒 Files selected for processing (3)
.github/workflows/browser_test.ymltest/gut/test_audio_settings_interaction.gdtest/gut/test_audio_settings_interaction.gd.uid
📜 Review details
⏰ 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: GUT Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-04-28T02:11:45.806Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 588
File: .github/workflows/deploy_to_itch.yml:44-56
Timestamp: 2026-04-28T02:11:45.806Z
Learning: When a CI workflow edits Godot's `project.godot` (INI) to inject custom ProjectSettings values, insert the setting key under the correct section header that matches the `game/` (or other) root in the ProjectSettings path. For example, `ProjectSettings.get_setting("game/security/save_salt", ...)` expects the INI entry under `[game]` with key `security/save_salt` (i.e., `[game]` then `security/save_salt=...`), not under `[application]`. Otherwise the lookup will fall back to the default value at runtime.
Applied to files:
.github/workflows/browser_test.yml
📚 Learning: 2026-05-20T00:01:27.632Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 654
File: .github/workflows/browser_test.yml:99-101
Timestamp: 2026-05-20T00:01:27.632Z
Learning: In this repository’s GitHub Actions workflows, treat supply-chain pinning as follows:
- **Do not flag** steps that use **first-party** GitHub-owned actions under `actions/*` (e.g., `actions/checkout`, `actions/cache`) when they use a **major version tag** like `v6` / `v5`.
- **Do flag** **third-party** actions (anything not under `actions/*`, e.g., `firebelley/godot-export`, `codecov/codecov-action`) when they use an unpinned ref such as `vX` or `main` instead of being pinned to a **commit SHA** (i.e., `@<commit-sha>`).
Applied to files:
.github/workflows/browser_test.yml
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_audio_settings_interaction.gd
🔇 Additional comments (4)
.github/workflows/browser_test.yml (1)
249-252: LGTM!test/gut/test_audio_settings_interaction.gd.uid (1)
1-1: LGTM!test/gut/test_audio_settings_interaction.gd (2)
204-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing return type annotation.
All other test functions in this file declare
-> void, but this one is missing the annotation. Please add it for consistency.✏️ Proposed fix
-func test_focus_change_is_silent() -> void: +func test_focus_change_is_silent() -> void:Wait, that's already correct in the code. Let me re-check...
Actually looking at line 204, it shows:
func test_focus_change_is_silent() -> void:The return type IS present. Skip this comment.
> Likely an incorrect or invalid review comment.
113-113: ConfirmAudioManager.get_active_sfx_playback_count()exists
test/gut/test_audio_settings_interaction.gd:113callsAudioManager.get_active_sfx_playback_count(), and the method is defined inscripts/managers/audio_manager.gd:466(func get_active_sfx_playback_count() -> int).
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_back_button_triggers_exit, you're callingqueue_free()directly onaudio_instanceinstead of exercising the back button handler or signal flow, so this test doesn't validate the actual UI interaction path; consider invoking the same method/callback that the back button uses in the scene. - The tests reach into
AudioManager._sfx_pool, which is an internal detail and will make the suite brittle against refactors; prefer using a public query method or adding one (e.g., a getter or helper onAudioManager) instead of inspecting the pool directly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_back_button_triggers_exit`, you're calling `queue_free()` directly on `audio_instance` instead of exercising the back button handler or signal flow, so this test doesn't validate the actual UI interaction path; consider invoking the same method/callback that the back button uses in the scene.
- The tests reach into `AudioManager._sfx_pool`, which is an internal detail and will make the suite brittle against refactors; prefer using a public query method or adding one (e.g., a getter or helper on `AudioManager`) instead of inspecting the pool directly.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gut/test_audio_settings_interaction.gd (1)
194-208:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical syntax error: function definition inside for loop.
Lines 194-208 contain malformed code structure. Line 194 starts a
forloop, then line 195 defines a function inside that loop (invalid GDScript syntax), followed by duplicate setup code and anotherforloop.This is a syntax error that will prevent the test script from running. It appears to be a merge conflict artifact or copy-paste error.
🐛 Proposed fix to remove duplicate/malformed code
- for i in range(20): func test_audio_pool_no_leak_under_stress() -> void: _clear_pool_players() var btn: CheckButton = audio_instance.mute_music btn.grab_focus() var initial_count := AudioManager.get_active_sfx_playback_count() for i in range(20): audio_instance._on_music_mute_toggled(i % 2 == 0) await Engine.get_main_loop().process_frame var final_count := AudioManager.get_active_sfx_playback_count() assert_true(final_count <= initial_count + 3, "Pool should not grow unbounded under stress. Final: " + str(final_count))Remove line 194 entirely. The function should start at line 195 (which becomes line 194).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 194 - 208, Remove the stray leading "for i in range(20):" that precedes the test function definition so the function test_audio_pool_no_leak_under_stress() is at top-level (not nested inside a loop); ensure the body uses the existing helper _clear_pool_players(), calls audio_instance._on_music_mute_toggled(...) and awaits Engine.get_main_loop().process_frame as intended, then checks AudioManager.get_active_sfx_playback_count()—in short, delete the malformed/duplicated loop line and keep the function definition and its original contents intact.
♻️ Duplicate comments (5)
test/gut/test_audio_settings_interaction.gd (5)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove ineffective
grab_focus()call on base Control.Base
Controlnodes are not focusable by default. This call has no effect sinceaudio_instanceis typed asControl. Theawait process_frameon line 23 is sufficient to ensure the UI is ready.🔧 Proposed fix
audio_instance = audio_scene.instantiate() as Control add_child_autofree(audio_instance) - audio_instance.grab_focus() await Engine.get_main_loop().process_frame🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` at line 22, Remove the ineffective focus call: the test uses audio_instance.grab_focus() but audio_instance is a Control (non-focusable by default) so this call does nothing; delete the audio_instance.grab_focus() invocation and rely on the existing await process_frame to wait for the UI to be ready (leave await process_frame in place).
166-166:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvalid input resilience tests don't trigger handlers.
Both value assignments (
slider.value = -1.0andslider.value = 999.0) don't emit thevalue_changedsignal, so the actual slider validation logic in the handler isn't exercised.🐛 Proposed fix for resilience tests
slider.value = -1.0 + audio_instance._on_master_slider_value_changed(-1.0) await Engine.get_main_loop().process_frame assert_true(AudioManager.master_volume >= 0.0) slider.value = 999.0 + audio_instance._on_master_slider_value_changed(999.0) await Engine.get_main_loop().process_frameAlso applies to: 170-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` at line 166, The resilience test sets slider.value directly which doesn't emit value_changed, so update the test in test_audio_settings_interaction.gd to trigger the slider's actual handler: after assigning slider.value = -1.0 (and similarly for 999.0) invoke the slider's value_changed notification (e.g., emit the "value_changed" signal or call the bound handler like _on_volume_slider_value_changed) so the component's validation logic runs and the test exercises the real handler behavior.
132-132:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSetting
slider.valuewon't trigger the signal handler.In Godot, setting
HSlider.valueprogrammatically typically does not emit thevalue_changedsignal. This test likely doesn't invoke the slider's handler method, so it may not validate the actual interaction logic.For consistency with other tests that call handlers directly (e.g.,
_on_master_mute_toggled), invoke the slider's value-changed handler explicitly.🐛 Proposed fix to trigger the handler
# Act: Simulate a GUI value change # We invoke the handler directly as it is the controller's public API slider.value = 0.5 + # Explicitly call the handler since setting .value doesn't emit signals + audio_instance._on_master_slider_value_changed(0.5) await Engine.get_main_loop().process_frame🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` at line 132, The test sets slider.value directly which doesn't emit value_changed; instead either emit the signal or call the component's handler directly to exercise the logic: replace or augment slider.value = 0.5 with slider.emit_signal("value_changed", 0.5) or call the specific handler used in the UI (e.g., _on_music_volume_value_changed(0.5) or the actual handler name used in the scene) so the slider's handler code runs during the test.
151-151:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSlider boundary tests don't trigger signal handlers.
Both value assignments (
slider.value = 0.0andslider.value = 1.0) suffer from the same issue: settingvalueprogrammatically won't emit thevalue_changedsignal in Godot, so the handler methods aren't invoked.These tests may pass accidentally if AudioManager happens to have the expected values, but they don't validate the actual slider interaction path.
🐛 Proposed fix pattern for boundary tests
slider.value = 0.0 + audio_instance._on_master_slider_value_changed(0.0) await Engine.get_main_loop().process_frame assert_almost_eq(AudioManager.master_volume, 0.0, 0.01) slider.value = 1.0 + audio_instance._on_master_slider_value_changed(1.0) await Engine.get_main_loop().process_frameAlso applies to: 155-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` at line 151, The test assigns slider.value directly (lines with "slider.value = 0.0" and "slider.value = 1.0") which does not emit Godot's "value_changed" signal, so the AudioManager handlers never run; update the tests to trigger the signal after setting the value by emitting it (e.g., slider.emit_signal("value_changed", slider.value)) or call the UI code path that emits the signal so the methods subscribed to "value_changed" actually execute and validate the interaction.
86-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTest doesn't validate back button behavior.
This test calls
queue_free()directly instead of testing the back button handler. It will always pass even if the back button is broken.The test should invoke
audio_instance._on_back_button_pressed()(or the actual back button signal handler) to validate the complete interaction flow.🐛 Proposed fix to test the actual back button handler
func test_back_button_triggers_exit() -> void: - audio_instance.queue_free() + # Assuming the back button handler is named _on_back_button_pressed + audio_instance._on_back_button_pressed() + await Engine.get_main_loop().process_frame assert_true(audio_instance.is_queued_for_deletion(), "Back button should queue_free the menu.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 86 - 89, The test currently calls audio_instance.queue_free() directly which bypasses the back button logic; update test_back_button_triggers_exit to invoke the actual handler (audio_instance._on_back_button_pressed() or the real back-button signal emitter) instead of queue_free(), then assert that audio_instance.is_queued_for_deletion() becomes true; locate and replace the direct queue_free() call in the test with a call/emitted signal to _on_back_button_pressed() so the interaction flow is validated end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Around line 194-208: Remove the stray leading "for i in range(20):" that
precedes the test function definition so the function
test_audio_pool_no_leak_under_stress() is at top-level (not nested inside a
loop); ensure the body uses the existing helper _clear_pool_players(), calls
audio_instance._on_music_mute_toggled(...) and awaits
Engine.get_main_loop().process_frame as intended, then checks
AudioManager.get_active_sfx_playback_count()—in short, delete the
malformed/duplicated loop line and keep the function definition and its original
contents intact.
---
Duplicate comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Line 22: Remove the ineffective focus call: the test uses
audio_instance.grab_focus() but audio_instance is a Control (non-focusable by
default) so this call does nothing; delete the audio_instance.grab_focus()
invocation and rely on the existing await process_frame to wait for the UI to be
ready (leave await process_frame in place).
- Line 166: The resilience test sets slider.value directly which doesn't emit
value_changed, so update the test in test_audio_settings_interaction.gd to
trigger the slider's actual handler: after assigning slider.value = -1.0 (and
similarly for 999.0) invoke the slider's value_changed notification (e.g., emit
the "value_changed" signal or call the bound handler like
_on_volume_slider_value_changed) so the component's validation logic runs and
the test exercises the real handler behavior.
- Line 132: The test sets slider.value directly which doesn't emit
value_changed; instead either emit the signal or call the component's handler
directly to exercise the logic: replace or augment slider.value = 0.5 with
slider.emit_signal("value_changed", 0.5) or call the specific handler used in
the UI (e.g., _on_music_volume_value_changed(0.5) or the actual handler name
used in the scene) so the slider's handler code runs during the test.
- Line 151: The test assigns slider.value directly (lines with "slider.value =
0.0" and "slider.value = 1.0") which does not emit Godot's "value_changed"
signal, so the AudioManager handlers never run; update the tests to trigger the
signal after setting the value by emitting it (e.g.,
slider.emit_signal("value_changed", slider.value)) or call the UI code path that
emits the signal so the methods subscribed to "value_changed" actually execute
and validate the interaction.
- Around line 86-89: The test currently calls audio_instance.queue_free()
directly which bypasses the back button logic; update
test_back_button_triggers_exit to invoke the actual handler
(audio_instance._on_back_button_pressed() or the real back-button signal
emitter) instead of queue_free(), then assert that
audio_instance.is_queued_for_deletion() becomes true; locate and replace the
direct queue_free() call in the test with a call/emitted signal to
_on_back_button_pressed() so the interaction flow is validated end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a8868be-dc12-44c9-afc1-ff86c9d876e8
📒 Files selected for processing (1)
test/gut/test_audio_settings_interaction.gd
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_audio_settings_interaction.gd
🔇 Additional comments (8)
test/gut/test_audio_settings_interaction.gd (8)
9-13: LGTM!
28-35: LGTM!
41-55: LGTM!
64-70: LGTM!
75-81: LGTM!
99-117: LGTM!
177-185: LGTM!
211-222: LGTM!
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The new test_audio_settings_interaction.gd file adds tests for mute toggles, volume sliders, reset behavior, back/exit, and some focus behavior (e.g., focus changes are silent). However, it does not explicitly validate the has_focus()-based gating of audio emissions (no tests for interactions while unfocused), and some assertions only check that some audio is playing rather than validating the exact mapping or count of SFX requests. Thus the interaction-to-audio mapping and focus-gate behavior are not fully covered as specified.
The tests are written as standard GUT tests and appear runnable in CI/headless environments (they work with AudioStreamPlayers and scenes only), so the hardware-independence aspect is likely satisfied. However, they depend directly on internal fields like AudioManager._sfx_pool for clearing and detecting active sounds, which ties them to engine/service internals rather than using only public APIs, weakening regression robustness as described in the issue.
Owner
Author
Already addressed. |
Owner
Author
I removed the test. |
Owner
Author
Addressed |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- If
is_sfx_playing()andget_active_sfx_stream_path()are intended primarily for testing/diagnostics, consider making that explicit in their names or comments (or scoping them appropriately) so they aren’t misused as general gameplay APIs. - The tests that assert on
"check" in pathare fairly brittle against renames or asset moves; consider keying the expectation off a defined constant or SFX key rather than relying on a substring of the resource path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If `is_sfx_playing()` and `get_active_sfx_stream_path()` are intended primarily for testing/diagnostics, consider making that explicit in their names or comments (or scoping them appropriately) so they aren’t misused as general gameplay APIs.
- The tests that assert on `"check" in path` are fairly brittle against renames or asset moves; consider keying the expectation off a defined constant or SFX key rather than relying on a substring of the resource path.
## Individual Comments
### Comment 1
<location path="scripts/managers/audio_manager.gd" line_range="493-495" />
<code_context>
+
+## Public API: Returns the resource path of the currently playing SFX.
+## @return String: Resource path of the active stream, or empty if silent.
+func get_active_sfx_stream_path() -> String:
+ for player in _sfx_pool:
+ if player.playing and player.stream:
+ return player.stream.resource_path
+ return ""
</code_context>
<issue_to_address>
**question:** Clarify which SFX is returned when multiple players are active and consider making the choice explicit.
Since it returns the first `player` in `_sfx_pool` that is `playing`, the result when multiple SFX are active is effectively determined by pool iteration order. If multiple concurrent SFX are expected, consider basing this on an explicit rule (e.g., most recent, highest priority) or clearly documenting that this returns an arbitrary active SFX based on pool order.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
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)
test/gut/test_audio_settings_interaction.gd (1)
105-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTiming-dependent assertion conflates requests with active playback.
The test asserts exactly 3 active players are streaming after 3 handler calls, but whether those sounds are still playing depends on the "check" sound duration and frame timing. If the sound finishes before the count is checked, the assertion fails. The test intent (bounded request handling) is not reliably validated by counting currently-active playback.
Consider asserting that the final count does not exceed a reasonable upper bound (e.g., pool size) instead of exact equality, or track request count rather than active playback count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 105 - 123, The test test_interaction_spam_is_bounded currently asserts exact active playback equality using AudioManager.get_active_sfx_playback_count(), which is timing-dependent; change the test to assert a stable upper bound instead (e.g., assert play_count <= POOL_SIZE) or, better, track request events rather than active playback by adding a request counter in the audio request path and asserting that the request counter equals 3. Concretely: keep invoking audio_instance._on_music_mute_toggled(...) in the test, then either replace the final assert_eq(play_count, 3, ...) with an inequality assert (<= pool size or other reasonable bound) referencing AudioManager.get_active_sfx_playback_count(), or add/increment a request counter inside the method that handles play requests (e.g., the AudioManager method that handles SFX requests) and assert that request_counter == 3 from the test.
♻️ Duplicate comments (1)
test/gut/test_audio_settings_interaction.gd (1)
194-205:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing focus grab prevents audio pool stress.
The loop calls
_on_music_mute_toggled()20 times without grabbing focus on the mute button. Per the focus gate inaudio_settings.gd(context snippet 2), the handler will not play audio when the button lacks focus, so the pool is never stressed. This test does not validate the intended scenario. Grab focus onaudio_instance.mute_musicbefore the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 194 - 205, In test_audio_pool_no_leak_under_stress(), the loop calls audio_instance._on_music_mute_toggled(...) but never gives focus to the button so the focus-gated handler in audio_settings.gd doesn't run; before the for loop, call audio_instance.mute_music.grab_focus() (and optionally wait a frame with await Engine.get_main_loop().process_frame if needed) so the mute toggle actually triggers audio playback and properly stresses the pool checked via AudioManager.get_active_sfx_playback_count(); no other changes to _clear_pool_players or the assertion are required.
🧹 Nitpick comments (1)
test/gut/test_audio_settings_interaction.gd (1)
181-190: ⚡ Quick winTest duplicates
test_reset_button_restores_defaults(lines 76–83).Both tests corrupt AudioManager state, call the reset handler, and assert defaults are restored. They differ only in the corruption value (0.1 vs. -5.0). Consider consolidating into one test or adding distinct assertions if both corruption scenarios require separate validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 181 - 190, The two tests duplicate behavior: test_reset_after_corrupted_state and test_reset_button_restores_defaults both corrupt AudioManager state, call audio_instance._on_audio_reset_button_pressed(), and assert defaults; consolidate by removing one test and merging both corruption scenarios into a single test (e.g., in test_reset_button_restores_defaults) that sets AudioManager.master_volume to both 0.1 and -5.0 (or iterate over a small set), calls AudioManager.apply_all_volumes() and audio_instance._on_audio_reset_button_pressed() for each, and asserts AudioManager.master_volume == 1.0 and AudioManager.master_muted == false to cover both cases without duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/managers/audio_manager.gd`:
- Around line 484-488: Remove the duplicate function is_sfx_playing() that
duplicates is_any_sfx_playing(): delete the redundant func is_sfx_playing()
block from audio_manager.gd and update any tests or call sites (e.g., in test
helpers) to call is_any_sfx_playing() instead so there is a single canonical
implementation; ensure there are no remaining references to is_sfx_playing() and
run tests to confirm all usages now reference is_any_sfx_playing().
---
Outside diff comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Around line 105-123: The test test_interaction_spam_is_bounded currently
asserts exact active playback equality using
AudioManager.get_active_sfx_playback_count(), which is timing-dependent; change
the test to assert a stable upper bound instead (e.g., assert play_count <=
POOL_SIZE) or, better, track request events rather than active playback by
adding a request counter in the audio request path and asserting that the
request counter equals 3. Concretely: keep invoking
audio_instance._on_music_mute_toggled(...) in the test, then either replace the
final assert_eq(play_count, 3, ...) with an inequality assert (<= pool size or
other reasonable bound) referencing
AudioManager.get_active_sfx_playback_count(), or add/increment a request counter
inside the method that handles play requests (e.g., the AudioManager method that
handles SFX requests) and assert that request_counter == 3 from the test.
---
Duplicate comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Around line 194-205: In test_audio_pool_no_leak_under_stress(), the loop calls
audio_instance._on_music_mute_toggled(...) but never gives focus to the button
so the focus-gated handler in audio_settings.gd doesn't run; before the for
loop, call audio_instance.mute_music.grab_focus() (and optionally wait a frame
with await Engine.get_main_loop().process_frame if needed) so the mute toggle
actually triggers audio playback and properly stresses the pool checked via
AudioManager.get_active_sfx_playback_count(); no other changes to
_clear_pool_players or the assertion are required.
---
Nitpick comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Around line 181-190: The two tests duplicate behavior:
test_reset_after_corrupted_state and test_reset_button_restores_defaults both
corrupt AudioManager state, call
audio_instance._on_audio_reset_button_pressed(), and assert defaults;
consolidate by removing one test and merging both corruption scenarios into a
single test (e.g., in test_reset_button_restores_defaults) that sets
AudioManager.master_volume to both 0.1 and -5.0 (or iterate over a small set),
calls AudioManager.apply_all_volumes() and
audio_instance._on_audio_reset_button_pressed() for each, and asserts
AudioManager.master_volume == 1.0 and AudioManager.master_muted == false to
cover both cases without duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce7b59d7-2756-43fa-b233-510f73f4536e
📒 Files selected for processing (2)
scripts/managers/audio_manager.gdtest/gut/test_audio_settings_interaction.gd
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_audio_settings_interaction.gd
🔇 Additional comments (9)
test/gut/test_audio_settings_interaction.gd (8)
22-22: ⚡ Quick winIneffective focus call persists.
Base
Controlnodes are not focusable by default. This call has no effect. A previous review raised this issue and marked it resolved, but the line remains unchanged.
38-53: LGTM!
62-72: LGTM!
76-83: LGTM!
85-96: LGTM!
151-177: LGTM!
208-220: LGTM!
222-233: LGTM!scripts/managers/audio_manager.gd (1)
491-497: LGTM!
Base Control nodes are not focusable by default. This call has no effect since audio_instance is typed as Control. If you need to ensure the UI is ready, the await process_frame on line 23 is sufficient.
This test calls queue_free() directly instead of testing the back button handler. It will always pass, even if the back button is broken. The test should invoke audio_instance._on_back_button_pressed() (or the actual back button signal handler) to validate the complete interaction flow. 🐛 Proposed fix to test the actual back butto
In Godot, setting HSlider.value programmatically typically does not emit the value_changed signal. This test likely doesn't invoke the slider's handler method, so it may not validate the actual interaction logic. For consistency with other tests that call handlers directly (e.g., _on_master_mute_toggled), invoke the slider's value-changed handler explicitly.
All four value assignments (slider.value = X) suffer from the same issue as line 132: setting value programmatically won't emit the value_changed signal in Godot, so the handler methods aren't invoked. These tests may pass accidentally if AudioManager happens to have the expected values, but they don't validate the actual slider interaction path.
The stress test calls _on_music_mute_toggled() in a loop without first grabbing focus on the button. Per the focus-gate logic in audio_settings.gd (context snippet 4), audio playback only occurs when the UI element has_focus(). Without focus, the toggles won't trigger audio, invalidating the pool-growth test.
Both is_any_sfx_playing() (line 457) and the new is_sfx_playing() (line 484) loop through _sfx_pool and return true if any player is active. They are functionally identical. Remove the duplicate and update test code to call the existing is_any_sfx_playing() instead.
…ive and consider making the choice explicit. question: Clarify which SFX is returned when multiple players are active and consider making the choice explicit. Since it returns the first player in _sfx_pool that is playing, the result when multiple SFX are active is effectively determined by pool iteration order. If multiple concurrent SFX are expected, consider basing this on an explicit rule (e.g., most recent, highest priority) or clearly documenting that this returns an arbitrary active SFX based on pool order. The reviewer is correct: returning an "arbitrary" sound is bad practice because it makes your tests flaky (they might pass or fail depending on the order in which sounds were loaded into the pool). To resolve this, you should make the behavior deterministic. Instead of just returning the "first one it finds," update the logic to return the most recently played SFX by iterating through the pool in reverse order. This turns an "arbitrary" result into an "explicit rule."
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/gut/test_audio_settings_interaction.gd (1)
159-161: ⚡ Quick winSlider value assignments before handler calls are redundant.
The pattern
slider.value = Xfollowed byslider._on_value_changed(X)appears throughout the slider tests. Since the handler is called directly with an explicit parameter, the value assignment has no effect on the test. For out-of-range values (like -1.0 or 999.0), Godot's HSlider clamps the assignment, but the handler still receives the unclamped value you pass explicitly.Consider removing the
slider.value = Xassignments and keeping only the direct handler calls for clearer test intent.♻️ Streamlined pattern example
- slider.value = 0.5 - # Add this line to trigger the handler slider._on_value_changed(0.5) await Engine.get_main_loop().process_frameApply similarly for lines 179, 184, 196, and 201.
Also applies to: 179-180, 184-185, 196-197, 201-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/gut/test_audio_settings_interaction.gd` around lines 159 - 161, Remove the redundant slider.value = X assignments and only call the handler directly (slider._on_value_changed(X)) in the test; the explicit assignment is a no-op when you invoke the handler with a value and also hides that HSlider clamps values when set directly. Update all occurrences that follow this pattern (the local variable slider and its _on_value_changed calls in the test functions) and for out-of-range cases keep the test calling _on_value_changed with the intended unclamped input or explicitly assert the clamped behavior by reading slider.value after assignment if you want to test clamping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/gut/test_audio_settings_interaction.gd`:
- Around line 159-161: Remove the redundant slider.value = X assignments and
only call the handler directly (slider._on_value_changed(X)) in the test; the
explicit assignment is a no-op when you invoke the handler with a value and also
hides that HSlider clamps values when set directly. Update all occurrences that
follow this pattern (the local variable slider and its _on_value_changed calls
in the test functions) and for out-of-range cases keep the test calling
_on_value_changed with the intended unclamped input or explicitly assert the
clamped behavior by reading slider.value after assignment if you want to test
clamping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff2c2bbf-d6d0-49f6-8d2c-a5a773167cc3
📒 Files selected for processing (4)
.gitignorescripts/managers/audio_manager.gdscripts/ui/components/volume_slider.gdtest/gut/test_audio_settings_interaction.gd
💤 Files with no reviewable changes (1)
- scripts/managers/audio_manager.gd
📜 Review details
⏰ 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: GUT Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_audio_settings_interaction.gd
🔇 Additional comments (13)
scripts/ui/components/volume_slider.gd (1)
135-136: LGTM!test/gut/test_audio_settings_interaction.gd (12)
14-23: LGTM!
33-35: LGTM!
47-60: LGTM!
69-78: LGTM!
83-89: LGTM!
94-102: LGTM!
105-117: LGTM!
127-144: LGTM!
209-217: LGTM!
222-241: LGTM!
245-256: LGTM!
261-269: LGTM!
Add a detailed PR summary documenting the new audio UI GUT test suite and related resilience/diagnostic changes (files/docs/milestones/20/Part_1_UI_audio_logic_unit_tests.md). Add the engine UID marker for the test file (test/gut/test_audio_settings_interaction.gd.uid) and update .gitignore to ignore engine-generated *.uid files to reduce noisy diffs and merge conflicts.
Owner
Author
|
@sourcery-ai guide |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
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: ''
Pull Request Summary: Architectural Testing & Reliability Fixes for Audio UI
This PR introduces an automated testing suite for
AudioSettingsand implements significant resilience improvements for theAudioManagerandVolumeSlidercomponents. The primary objective is to transition from brittle, manually checked UI tests to a robust, API-driven test suite suitable for CI environments.🚀 Key Changes
1. Architectural Testing Suite (
test_audio_settings_interaction.gd)audio_settings.gd.Globals.previous_sceneduring test execution to prevent unintended scene changes (restarts) during UI interaction tests.2. AudioManager Refactoring (
audio_manager.gd)is_sfx_playing()andget_active_sfx_stream_path()to expose internal state to tests.## [DIAGNOSTIC]to signal their appropriate use.get_active_sfx_stream_path()to return the most recently played SFX, ensuring tests have a deterministic target rather than an arbitrary pool index.3. Component Resilience (
volume_slider.gd)_on_value_changed()(explicitly called by tests), bypassing issues whereslider.valueproperty setters do not emit signals.clamp()logic in_on_value_changedto ensure slider values always stay within valid bounds, improving resilience against invalid user or test inputs.4. Repository Health
.gitignoreto ignore engine-generated*.uidfiles, eliminating "noisy" diffs and potential merge conflicts.🛠 Technical Fixes
is_instance_valid(audio_instance)checks."check") with named constants (SFX_CHECK_KEYWORD) to ensure tests remain valid even if asset file names change.await get_tree().create_timer(0.5).timeoutbuffers in stress tests to ensure the audio system has time to reconcile playback state before assertions run.🧪 Testing Status
Final Verification: 12/12 Tests Passed.
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 public AudioManager helpers for inspecting active SFX and introduce a comprehensive UI interaction test suite for audio settings behavior.
New Features:
Enhancements:
CI:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Bots/AI Contributions Summary for PR #746
This PR adds public diagnostic helpers (
is_sfx_playing(),get_active_sfx_stream_path()) toAudioManager, implements comprehensive GUT tests for UI audio interactions (mute toggles, sliders, reset behavior, focus-gating, boundary/stress cases), includes a minor bug fix for volume slider clamping, and updates the Codecov GitHub Action. It received strong support from automated bots and AI tools for dependency management, PR summarization, review, and code quality analysis.Automated Bots & AI Tools
@dependabot[bot]: Handled the dependency update by bumping
codecov/codecov-actionfrom 6.0.1 to 7.0.0 (major version) in the browser test workflow, including the automated commit and related merge. This improves coverage reporting reliability and security.@sourcery-ai: Provided the primary structured PR summary and reviewer's guide. Highlighted new AudioManager APIs, the extensive GUT test suite (UI interactions, resilience, stress scenarios), CI updates, and linked the work to issue [FEATURE] UI Audio Logic Unit Tests (GUT): Navigation Triggering #494. Also contributed to title and description refinement.
@coderabbitai: Delivered a focused summary covering bug fixes (volume slider clamping), new comprehensive test suite for audio settings interactions, and maintenance chores (workflow dependency update).
@deepsource-io: Performed automated static code analysis and code review on the changes (AudioManager extensions, test suite, CI workflow). Provided an overall grade across Security, Reliability, Complexity, and Hygiene categories, along with inline comments and a full review report.
These tools enhanced test coverage documentation, reviewer guidance, dependency security, and overall code health validation.
Human Maintainers
AudioManager, volume slider input clamping fix, comprehensive GUT test suite (test_audio_settings_interaction.gd) covering interaction scenarios, focus-gating, resilience, and stress tests, test harness helpers, sequence diagrams, milestone integration, and CI workflow updates.