Conversation
…tialization #485 Description: Implement the foundation of the gameplay settings test suite, focusing on the GameSettingsResource as the single source of truth and the initial menu setup. Why is this useful?: Ensures that difficulty clamping (0.5 to 2.0) and signal emissions work correctly before the UI layer is involved. Proposed Implementation: Create test_game_settings_resource.gd covering: GS-RES-01 to 07: Validate clamping, boundary values (0.5, 1.0, 2.0), and redundant emission stability. GS-READY-01 to 06: Confirm _ready() correctly syncs the slider and label to Globals.settings.difficulty and connects signals exactly once.
…vity #486 Description: [cite_start]Develop tests for user-driven interactions and the menu's behavior as an observer of external resource changes[cite: 8, 75]. Why is this useful?: [cite_start]Verifies that the UI stays in sync with the global state and that set_value_no_signal prevents infinite feedback loops between the slider and the resource[cite: 10, 11]. Proposed Implementation: Create test_gameplay_settings_ui.gd covering: GS-UI-01 to 06: User slider changes, Reset button functionality, and label updates[cite: 15, 20]. GS-OBS-01 to 05: Verify _on_external_setting_changed correctly updates the UI when the resource is modified by other scripts[cite: 10, 11].
Create test_gameplay_settings_js.gd covering: GS-JS-01 to 05: Validating nested array shapes like [[1.5]]. GS-JS-10 to 25: Critical Defensive Tests for empty arrays [], non-numeric strings [["abc"]], and scalar values [1.5] to ensure no calls to .size() or .is_empty() occur on primitive types. GS-JS-30 to 32: Missing node safety (e.g., if the slider is null during a JS callback).
…#471) This PR implements a comprehensive suite of unit tests and defensive programming patterns for the GameplaySettings module. The primary goal was to resolve Issue #471, which involved engine crashes during JavaScript-to-Godot communication, and to ensure the menu remains stable during complex lifecycle events.
…esn’t guarantee that method exists. In the branch below, first_arg.size() is still called when first_arg is JavaScriptObject. Since JavaScriptObject doesn’t guarantee size() or index access, this is a potential runtime error. Please branch the logic so that .size() and indexing are only used when typeof(first_arg) == TYPE_ARRAY, and handle JavaScriptObject via a separate path (e.g., converting to an array or using a defined API) before accessing its contents.
In _on_change_difficulty_js, the branch that treats first_arg as a container still calls .size() and indexes [0] on values that may be JavaScriptObjects; if the regression you’re fixing was around calling array APIs on scalars, consider restricting the container path to first_arg is Array and handling JavaScriptObject via a safer accessor (e.g., known property) to avoid future engine-level crashes.
The Playwright test failed because of a type mismatch between the browser and Godot. When the Playwright script calls window.changeDifficulty([2.0]), the browser sends a JavaScript Object (a proxy for a JS array) to Godot. However, your console logs show that Godot rejected this: [WARNING] JS difficulty callback received non-convertible value: <JavaScriptObject#-9223371977983523341> This happened because our recent "bulletproof" refactoring added a check that strictly expects TYPE_INT, TYPE_FLOAT, or TYPE_STRING. It doesn't yet know how to "unwrap" a value from a JavaScriptObject.
Line 35, Line 48, and Line 275 assume Globals.settings is always initialized. If Globals._ready() hasn’t run yet (or tests reset it), this can null-deref and crash the menu.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## GS-LIFE-01 | Cleanup on tree exit verifies signals and ALL callbacks
func test_gs_life_01_cleanup_on_exit() -> void:
# 1. Setup: Ensure everything is connected first
assert_true(Globals.settings.setting_changed.is_connected(gameplay_menu._on_external_setting_changed))
assert_true(gameplay_menu.difficulty_slider.value_changed.is_connected(gameplay_menu._on_difficulty_value_changed))
# 2. Act: Trigger exit
gameplay_menu._on_tree_exited()
# 3. Assert Signal Disconnections (The missing piece)
assert_false(Globals.settings.setting_changed.is_connected(gameplay_menu._on_external_setting_changed),
"Global resource signal should be disconnected")
assert_false(gameplay_menu.difficulty_slider.value_changed.is_connected(gameplay_menu._on_difficulty_value_changed),
"Local UI signals should be disconnected")
# 4. Assert ALL Callbacks (Including the missing reset callback)
assert_null(gameplay_menu._change_difficulty_cb, "Difficulty callback nullified")
assert_null(gameplay_menu._gameplay_back_button_pressed_cb, "Back button callback nullified")
assert_null(gameplay_menu._gameplay_reset_cb, "Reset callback nullified")
…thub.com/ikostan/SkyLockAssault into settings-labels-display-unclamped-values
Line 99 (assert_true(true, ...)) can’t detect regressions. This path has a defined fallback behavior (resource update when slider is invalid), so assert that explicitly.
…riptObject can be fragile.
Here we assume every JavaScriptObject is an array-like proxy and use length and [0]. If a plain object is passed (e.g. { value: 3 }), length may be missing or non-numeric and indexing can misbehave. Please either validate that the object is array-like before using length/indexing (e.g. check for a numeric length), or handle generic JavaScriptObject values as scalars instead to avoid runtime issues.
In several places (e.g. _ready() and _on_difficulty_value_changed) you compute a settings_res local but still read from Globals.settings inside the guarded block; for consistency and to avoid any future race with Globals becoming invalid, consider using the local reference exclusively once it’s established.
The _on_change_difficulty_js handler has become quite branch-heavy (array vs JavaScriptObject vs scalar, plus type/bounds checks); extracting the value-normalization into a small helper (e.g. _extract_js_difficulty(args: Array) -> Variant) would make the main callback easier to follow and reason about.
…epetition #552 During gameplay, the parallax background elements (bushes, crates, rocks, barrels) repeat themselves every few seconds. Because the randomly generated background block is currently too short, the exact same clusters of items become easily recognizable as the screen scrolls, breaking the illusion of an infinite environment.
Introduce GUT unit tests (test_main_scene_parallax_chunks.gd) that instantiate MainScene and verify parallax layer behavior for bushes and decor. Each test mocks a 1920x1080 viewport, ensures motion_mirroring.y equals viewport.y * 20, and asserts spawned sprite counts equal 5x the number of preloaded textures prefixed with "bush_" or "decor_". Includes per-test setup/teardown to initialize the scene and clean up instances. A .uid companion file was also added by the test tooling.
…sformations Here is a new GUT test file dedicated specifically to verifying the randomized transformations (rotation, scaling, and flipping) that we added to the decor layer. To prevent the test output from becoming overly cluttered with hundreds of individual assertions, these tests iterate through all spawned sprites and collect any invalid items into an array. They then assert that the array of invalid items is completely empty.
Reduce repeating layer height from 20 screens to 8 for bushes and decor to balance the infinite-scrolling illusion with CPU overhead. Lower the density multipliers from 5x to 2x (num_bushes and num_decors) so perceived density matches the smaller repeated area. Comments updated to explain the rationale; no changes to rotation logic.
The Structural Asserts: test_parallax_chunk_size_is_optimized and test_parallax_sprite_density_is_optimized explicitly enforce the 8-screen limit and the 2x multiplier we just set. They act as automated guardrails against future developers accidentally re-introducing the BVH bloat that caused your FPS drop. The Execution Proxy: test_process_script_execution_time is a neat trick. It fires the _process() function 60 times in a row as fast as possible and measures the raw CPU time in microseconds. While it doesn't measure the GPU rendering the sprites, it does measure the GDScript overhead, ensuring your math and checks inside _process stay incredibly fast.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
The PR does not change the child cleanup semantics to eliminate the orphan window. setup_bushes_layer() still has no explicit clear loop shown in the diff, and setup_decor_layer() continues to clear children using decor_layer.remove_child(child) followed by child.queue_free(), rather than calling free() directly. This preserves the timing window where removed children exist as orphans until the end of the frame, which is the core bug described in the issue.
…gd' of https://github.com/ikostan/SkyLockAssault into orphan-node-leak-from-placeholder-sprites-in-main_scenegd
The test_decor_sprites_have_boolean_flips test only checks that flip_h/flip_v are of type bool (which they always are by default) rather than that random flips are actually being applied; consider asserting that at least some sprites have true and some have false to make this test meaningful.
Rename test/gut/test_main_scene_performance_limits.gd -> test_main_scene_parallax_and_performance.gd (and corresponding .uid) and remove the separate test_main_scene_parallax_chunks.gd file. Add two new GUT tests that verify parallax layers: bushes and decor should mirror at exactly 8 screens tall (motion_mirroring.y == viewport.y * 8) and spawn 2x the number of sprites based on the texture preloader. Also add a brief initialization comment and ensure tests filter out nodes queued for deletion when counting active children.
Add a standardized safe_hard_free helper and use it across GUT tests to avoid double-free/orphan-window issues. Replace add_child_autofree with add_child and make after_each perform guarded teardown via safe_hard_free. Tighten and simplify test_main_scene_orphan_nodes.gd: explicit manual frees and nullification, added frame syncs, lambda type annotations, streamlined assertions, and cleanup of reloaded instances. These changes make test teardown deterministic and prevent orphan-node leaks and intermittent flakiness.
…ects leak into the measurement. Two concerns with this test: A hard < 1000 µs average over 60 script calls will occasionally fail on shared CI runners (GC pauses, other tests running concurrently, headless renderer under contention). A script-time performance bound this tight is better enforced as a benchmark trend rather than a pass/fail assertion. Consider widening the threshold substantially, running a larger sample, and/or tagging it so it only runs in a dedicated perf job. main_scene._process on line 108 has side effects: it mutates background.scroll_offset, and on the first call will trigger show_message(...) if Settings.has_unbound_critical_actions_for_current_device() is true, which schedules a get_tree().create_timer(4.0) and adds a label update. The first iteration is therefore not representative, and the timer node lingers past the test. Worth either pre-warming once outside the timed loop, or explicitly setting state to skip the unbound branch during the measurement.
…be easier to tune as a single constant/config. Since both setup_bushes_layer and setup_decor_layer use this screens_tall = 8.0 value, any future adjustment would require changing it in multiple places. Please extract it into a shared constant or exported property so the parallax layer height is defined once and stays consistent across both layers.
Remove duplicate safe_hard_free implementations from test/gut/test_decor_layer_transformations.gd and test/gut/test_main_scene_orphan_nodes.gd, add a const GutHelper preload, and replace local calls with GutHelper.safe_hard_free(...). Centralizes the safe-free logic to avoid orphan windows/double-frees and reduces duplicated test code.
…r-sprites-in-main_scenegd Fix main scene orphan leaks and tune parallax decor with tests
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
1 similar comment
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @ikostan, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Apr 17, 2026 4:15a.m. | Review ↗ |
| JavaScript | |
Apr 17, 2026 4:15a.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.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")