Replace brittle waits with deterministic polling - #845
Conversation
Refactor test infrastructure for improved reliability: - Split fixture setup: session-scoped browser launch + function-scoped page contexts for better isolation and startup overhead reduction - Replace all page.wait_for_timeout() calls with deterministic page.wait_for_function() checks or expect() assertions - Add wait_for_console_log() helper to poll for matching console logs with predicates instead of fixed waits - Improve Godot initialization check: use explicit '=== true' instead of loose truthy check - Use expect() API for DOM element visibility assertions (canvas, buttons) - Add type hints to function parameters and imports (Any, Callable, Generator) - Improve shell script: better signal handling, robust server startup checks, git safety config - Remove verbose/redundant comments to improve readability These changes eliminate timing-dependent flakiness common in browser automation by making assertions wait for actual state changes rather than arbitrary durations.
ikostan
added this to the
Milestone 22: Optimize Test Suite Runtime & Fix Loading Screen milestone
Reviewer's GuideRefactors Playwright-based E2E tests and the browser test runner to replace brittle fixed timeouts with deterministic state-based polling, introduce reusable console-log waiting helpers, improve fixtures and typing, and harden the CI shell script and server startup behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPlaywright E2E tests now use explicit Godot, UI, and console-log synchronization. Browser contexts are isolated over shared Chromium, and the browser-test runner improves server readiness, cleanup, restoration checks, and failure handling. ChangesE2E determinism and test execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
wait_for_console_loghelper is duplicated across multiple test modules with slightly different defaults; consider extracting it into a shared utility or fixture inconftest.pyto avoid repetition and keep behavior consistent. - In several places you still use string-based
page.wait_for_functionchecks on DOM state (e.g.,getComputedStyle(...).display); you might simplify and harden these by wrapping them in reusable helpers or using Playwright locator assertions where possible to reduce reliance on raw JS snippets. - The session-scoped
browser_instanceplus per-testBrowserContextis a good optimization; you may want to add an explicitcontext.tracing.stop()or similar cleanup if you later enable tracing/recording features to avoid leaks across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `wait_for_console_log` helper is duplicated across multiple test modules with slightly different defaults; consider extracting it into a shared utility or fixture in `conftest.py` to avoid repetition and keep behavior consistent.
- In several places you still use string-based `page.wait_for_function` checks on DOM state (e.g., `getComputedStyle(...).display`); you might simplify and harden these by wrapping them in reusable helpers or using Playwright locator assertions where possible to reduce reliance on raw JS snippets.
- The session-scoped `browser_instance` plus per-test `BrowserContext` is a good optimization; you may want to add an explicit `context.tracing.stop()` or similar cleanup if you later enable tracing/recording features to avoid leaks across tests.
## Individual Comments
### Comment 1
<location path="tests/volume_sliders_mutes_test.py" line_range="66-75" />
<code_context>
page.on("console", on_console)
+
+ def wait_for_console_log(
+ predicate: Callable[[str], bool], start_idx: int, timeout_ms: int = TEST_TIMEOUT
+ ) -> None:
+ """
+ Helper to poll until a matching console log arrives or timeout expires.
+ """
+ start_time = time.time()
+ while (time.time() - start_time) * 1000 < timeout_ms:
+ if any(predicate(log["text"].lower()) for log in logs[start_idx:]):
+ return
+ page.wait_for_timeout(50) # Micro-poll for event loop progression
+ pytest.fail(f"Timed out waiting for expected console log matching predicate after {timeout_ms}ms")
+
try:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider centralizing `wait_for_console_log` as a shared helper to avoid duplication and keep behavior consistent across tests.
The same helper is now duplicated across multiple test modules (volume, reset, audio, difficulty, back, navigation). Extracting it into a shared location (e.g., `conftest.py` or `test_utils.py`) and reusing it as a fixture or helper function would simplify maintenance and keep polling/timeout behavior consistent, while still allowing each test to manage its own `start_idx`.
Suggested implementation:
```python
page.on("console", on_console)
from test_utils import wait_for_console_log as shared_wait_for_console_log
def wait_for_console_log(
predicate: Callable[[str], bool], start_idx: int, timeout_ms: int = TEST_TIMEOUT
) -> None:
"""
Delegate to shared helper to poll until a matching console log arrives or timeout expires.
"""
shared_wait_for_console_log(page=page, logs=logs, predicate=predicate, start_idx=start_idx, timeout_ms=timeout_ms)
```
To fully centralize the behavior across tests:
1. Create a shared helper in a common test module, for example `tests/test_utils.py`:
```python
import time
import pytest
TEST_TIMEOUT = 5_000 # or import from a shared constants module
def wait_for_console_log(page, logs, predicate, start_idx: int, timeout_ms: int = TEST_TIMEOUT) -> None:
"""
Helper to poll until a matching console log arrives or timeout expires.
"""
start_time = time.time()
while (time.time() - start_time) * 1000 < timeout_ms:
if any(predicate(log["text"].lower()) for log in logs[start_idx:]):
return
page.wait_for_timeout(50)
pytest.fail(f"Timed out waiting for expected console log matching predicate after {timeout_ms}ms")
```
2. Adjust the import in `tests/volume_sliders_mutes_test.py` if needed based on your package layout, e.g. use `from .test_utils import wait_for_console_log as shared_wait_for_console_log` if `tests` is a package.
3. In the other test modules (volume, reset, audio, difficulty, back, navigation), remove their duplicated `wait_for_console_log` implementations and either:
- Import and call `test_utils.wait_for_console_log(page, logs, predicate, start_idx, timeout_ms)`, or
- Use the same thin delegating wrapper pattern if they rely on closure variables.
4. Ensure all modules use the same timeout constant (`TEST_TIMEOUT`) from a shared place to keep behavior consistent.
</issue_to_address>
### Comment 2
<location path="tests/volume_sliders_mutes_test.py" line_range="129-134" />
<code_context>
- new_logs = logs[pre_change_log_count:]
- assert any(
- "log level changed to: debug" in log["text"].lower() for log in new_logs
+ wait_for_console_log(
+ lambda text: "log level changed to: debug" in text,
+ start_idx=pre_change_log_count,
</code_context>
<issue_to_address>
**issue (testing):** The SFX volume change assertions now only check a single log message, dropping verification of related side effects.
The prior test asserted multiple logs for an SFX volume change (e.g., `sfx volume level in audiomanager: 0.8` and `saved volumes to config`), validating propagation through the audio manager and persistence. With `wait_for_console_log`, it now only checks `applied loaded sfx volume to audioserver: 0.8`, so those side-effect verifications are lost. Please either broaden the predicate to include these messages or add additional `wait_for_console_log` calls/assertions to retain equivalent coverage of the persistence path.
</issue_to_address>
### Comment 3
<location path="tests/no_error_logs_test.py" line_range="66-71" />
<code_context>
- # Wait for Godot engine init (ensures 'godot' object is defined)
- page.wait_for_function("() => window.godotInitialized", timeout=DEFAULT_TIMEOUT)
+
+ # Wait deterministically for Godot engine initialization
+ page.wait_for_function("() => window.godotInitialized === true", timeout=DEFAULT_TIMEOUT)
# Verify canvas and title to ensure game is initialized
canvas = page.locator("canvas")
- page.wait_for_selector("canvas", state="visible", timeout=DEFAULT_TIMEOUT)
+ expect(canvas).to_be_visible(timeout=DEFAULT_TIMEOUT)
box: dict[str, float] | None = canvas.bounding_box()
assert box is not None, "Canvas not found on page"
</code_context>
<issue_to_address>
**suggestion (testing):** Dropping the post-load buffer removes coverage for late-appearing errors; consider a deterministic replacement rather than removing it entirely.
Previously, `BUFFER_TIMEOUT` and `page.wait_for_timeout(BUFFER_TIMEOUT)` gave `no_error_logs_test` a brief window to catch errors occurring just after initialization. Without that buffer, the test now effectively stops observing logs once the engine is initialized and the canvas is visible, so errors triggered shortly after the main menu appears (e.g., deferred signals or late resource loads) may be missed.
To retain the reduced flakiness while preserving coverage for these late errors, consider a more deterministic wait: for example, poll logs until no new entries appear for a short period (N ms), or wait for a specific “main menu ready” console message. This keeps the observation window open long enough without reintroducing arbitrary sleeps.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
⚡ Performance Metrics Summary📈 Suite Progression Benchmark
⏱️ Individual Test Execution BreakdownBy tracking the HTTP server
📋 Issue #772 Progress Checklist
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jul 26, 2026 5:02a.m. | Review ↗ |
| JavaScript | |
Jul 26, 2026 5:02a.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.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
Added the missing docstring to `on_console()` in `tests/audio_flow_test.py` to satisfy the DeepSource documentation requirement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/audio_flow_test.py (1)
53-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider hoisting
wait_for_console_loginto a shared fixture.This helper (and the
on_console/logssetup) is duplicated verbatim acrossaudio_flow_test.py,reset_audio_flow_test.py,volume_sliders_mutes_test.py,navigation_to_audio_test.py,back_flow_test.py, anddifficulty_flow_test.py. Aconftest.pyfixture returning(logs, wait_for_console_log)bound topagewould remove ~10 lines × 6 files and keep polling behavior consistent. Deferrable.🤖 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 `@tests/audio_flow_test.py` around lines 53 - 62, Hoist the duplicated on_console/logs setup and wait_for_console_log helper from the listed audio-flow tests into a shared conftest.py fixture bound to page. Have the fixture provide the logs collection and polling helper with the same predicate, start-index, timeout, and failure behavior, then update each test to consume the fixture and remove its local copies.tests/difficulty_flow_test.py (1)
72-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelper default timeout deviates from the shared contract.
wait_for_console_loghere defaultstimeout_ms=DEFAULT_TIMEOUT(30000), whereas the equivalent helper inback_flow_test.py,reset_audio_flow_test.py, andvolume_sliders_mutes_test.pydefaults toTEST_TIMEOUT. Calls that omittimeout_ms(lines 159, 210, 232, 246, 257) therefore wait 6× longer than the other suites. Align the default with the shared contract for consistency.♻️ Align default with shared contract
def wait_for_console_log( - predicate: Callable[[str], bool], start_idx: int, timeout_ms: int = DEFAULT_TIMEOUT + predicate: Callable[[str], bool], start_idx: int, timeout_ms: int = TEST_TIMEOUT ) -> None:🤖 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 `@tests/difficulty_flow_test.py` around lines 72 - 83, Update the wait_for_console_log helper’s timeout_ms default from DEFAULT_TIMEOUT to the shared TEST_TIMEOUT constant, matching the equivalent helpers in the other flow tests. Leave explicit timeout overrides and the polling behavior unchanged.
🤖 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 `@tests/audio_flow_test.py`:
- Line 232: Replace the bare assert in the warning-check logic with an explicit
exception so the failure remains enforced under python -O. Preserve the existing
message, including log['text'], and update only the assertion at the
unexpected-warning branch.
In `@tests/difficulty_flow_test.py`:
- Around line 42-44: Update the timeout configuration constants so
DEFAULT_TIMEOUT reads from its own environment variable with the 30-second
fallback, while TEST_TIMEOUT continues reading TEST_TIMEOUT with its 5-second
fallback. Keep the existing timeout usages unchanged.
In `@tests/no_error_logs_test.py`:
- Line 28: Update the DEFAULT_TIMEOUT definition so it no longer reads
TEST_TIMEOUT; use a separate environment variable with an independent safe
default of 30000 for page load/init visibility, while preserving TEST_TIMEOUT
for UI timing only.
In `@workspace/run_browser_tests.sh`:
- Around line 67-79: Update the readiness probe loop in run_browser_tests.sh to
require a successful HTTP response, such as by using curl’s fail-on-error
option, and add a per-request timeout so each attempt remains within the
existing retry budget. Preserve the current retry count, delay, server_ready
assignment, and failure exit behavior.
- Line 42: Update the server cleanup trap near the security-isolated web server
startup: perform server termination from an EXIT trap, and have INT and TERM
handlers exit with their corresponding signal status so cancellation cannot
continue into readiness checks or test execution. Preserve cleanup when the
script exits for any reason and keep the existing SERVER_PID behavior.
---
Nitpick comments:
In `@tests/audio_flow_test.py`:
- Around line 53-62: Hoist the duplicated on_console/logs setup and
wait_for_console_log helper from the listed audio-flow tests into a shared
conftest.py fixture bound to page. Have the fixture provide the logs collection
and polling helper with the same predicate, start-index, timeout, and failure
behavior, then update each test to consume the fixture and remove its local
copies.
In `@tests/difficulty_flow_test.py`:
- Around line 72-83: Update the wait_for_console_log helper’s timeout_ms default
from DEFAULT_TIMEOUT to the shared TEST_TIMEOUT constant, matching the
equivalent helpers in the other flow tests. Leave explicit timeout overrides and
the polling behavior unchanged.
🪄 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 Plus
Run ID: 6bd2d0c2-cca7-42f0-8e56-4020e047960b
📒 Files selected for processing (11)
tests/audio_flow_test.pytests/back_flow_test.pytests/conftest.pytests/difficulty_flow_test.pytests/load_main_menu_test.pytests/navigation_to_audio_test.pytests/no_error_logs_test.pytests/reset_audio_flow_test.pytests/validate_clean_load_test.pytests/volume_sliders_mutes_test.pyworkspace/run_browser_tests.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
- GitHub Check: Sourcery review
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-19T05:07:07.286Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 488
File: tests/difficulty_flow_test.py:194-200
Timestamp: 2026-03-19T05:07:07.286Z
Learning: When writing/adjusting tests that assert SkyLockAssault log output for difficulty (and other float settings), expect the decimal point to be preserved (e.g., logs like "setting 'difficulty' updated to: 1.0"). Do not use regexes that fail on floats due to the decimal point (e.g., patterns with a negative lookahead that assumes digits contain no '.'), since they will not match "1.0". Instead, use a simple substring check for the expected log prefix/value, or use a float-aware regex (e.g., matching `\d+(?:\.\d+)?`) / parse the logged value as a float before asserting.
Applied to files:
tests/validate_clean_load_test.pytests/conftest.pytests/load_main_menu_test.pytests/back_flow_test.pytests/no_error_logs_test.pytests/volume_sliders_mutes_test.pytests/navigation_to_audio_test.pytests/reset_audio_flow_test.pytests/audio_flow_test.pytests/difficulty_flow_test.py
🪛 ast-grep (0.44.1)
tests/back_flow_test.py
[warning] 292-294: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(
f"artifacts/test_back_failure_console_logs_{timestamp}.txt", "w"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
tests/volume_sliders_mutes_test.py
[warning] 402-404: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(
f"artifacts/test_volume_failure_console_logs_{timestamp}.txt", "w"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
tests/reset_audio_flow_test.py
[warning] 483-485: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(
f"artifacts/test_reset_failure_console_logs_{timestamp}.txt", "w"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.21)
tests/reset_audio_flow_test.py
[warning] 480-480: Use explicit conversion flag
Replace with conversion flag
(RUF010)
tests/audio_flow_test.py
[warning] 232-232: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🪛 Shellcheck (0.11.0)
workspace/run_browser_tests.sh
[info] 85-85: Not following: /opt/venv/bin/activate was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (20)
workspace/run_browser_tests.sh (3)
13-20: LGTM!
39-40: LGTM!
82-95: LGTM!tests/conftest.py (3)
15-30: LGTM!
45-51: LGTM!
40-43: 🩺 Stability & AvailabilityNo import issue found.
reandPathare already imported intests/conftest.py.tests/load_main_menu_test.py (1)
83-97: LGTM!tests/no_error_logs_test.py (1)
66-71: LGTM!tests/validate_clean_load_test.py (1)
56-58: LGTM!tests/navigation_to_audio_test.py (1)
93-108: LGTM!Also applies to: 120-148, 169-210
tests/audio_flow_test.py (1)
77-115: LGTM!Also applies to: 116-232
tests/reset_audio_flow_test.py (1)
92-131: LGTM!Also applies to: 151-223, 307-363, 374-421, 431-477
tests/volume_sliders_mutes_test.py (1)
92-160: LGTM!Also applies to: 162-394
tests/back_flow_test.py (4)
294-294: The static-analysis path-traversal warning here is a false positive:timestampisint(time.time()), not request- or user-derived, so the artifact path is fully controlled by the test.
55-79: LGTM!
93-131: LGTM!
142-282: LGTM!tests/difficulty_flow_test.py (3)
98-104: LGTM!
127-162: LGTM!Also applies to: 204-265
300-354: LGTM!
Formatted the single-line docstring for `wait_for_console_log()` onto a single line to comply with PEP 257 / DeepSource recommendations.
Split the `pytest.fail` string literal across multiple lines to satisfy the maximum line length constraint.
Split the JS expression string across multiple lines using implicit string concatenation to reduce line length below the limit.
Formatted JS predicate string across multiple lines using implicit concatenation to adhere to line length limits.
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 (4)
tests/reset_audio_flow_test.py (1)
37-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoth timeout constants read the same env var.
Same duplicated-env-var issue as the sibling flow tests.
🤖 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 `@tests/reset_audio_flow_test.py` around lines 37 - 38, Update DEFAULT_TIMEOUT in the timeout configuration near TEST_TIMEOUT to read its intended distinct environment variable, while keeping TEST_TIMEOUT mapped to TEST_TIMEOUT. Ensure the two constants no longer read the same env var, matching the sibling flow tests.tests/back_flow_test.py (1)
38-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoth timeout constants read the same env var.
Same issue as the other flow tests:
DEFAULT_TIMEOUTandTEST_TIMEOUTboth key offos.getenv("TEST_TIMEOUT", ...). SettingTEST_TIMEOUTin CI collapses both to the same value.🤖 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 `@tests/back_flow_test.py` around lines 38 - 39, Update the timeout constant definitions in back_flow_test.py so DEFAULT_TIMEOUT reads its intended distinct environment variable rather than TEST_TIMEOUT, while leaving TEST_TIMEOUT bound to TEST_TIMEOUT and preserving their existing fallback values.tests/volume_sliders_mutes_test.py (1)
37-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoth timeout constants read the same env var.
Same duplicated-env-var issue as the sibling flow tests.
🤖 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 `@tests/volume_sliders_mutes_test.py` around lines 37 - 38, Update DEFAULT_TIMEOUT and TEST_TIMEOUT in the volume slider mute tests to read their intended, distinct environment variables, matching the sibling flow tests; retain the existing default values and integer parsing.tests/audio_flow_test.py (1)
36-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoth timeout constants read the same env var.
DEFAULT_TIMEOUTandTEST_TIMEOUTboth callos.getenv("TEST_TIMEOUT", ...); only the fallback default differs. IfTEST_TIMEOUTis set in the environment,DEFAULT_TIMEOUTsilently collapses to the same value, defeating the intent of having a longer timeout for Godot init/networkidlenavigation vs. a shorter per-step timeout — undermining the flakiness fix this PR is meant to deliver.🐛 Proposed fix
-DEFAULT_TIMEOUT = int(os.getenv("TEST_TIMEOUT", "30000")) +DEFAULT_TIMEOUT = int(os.getenv("DEFAULT_TIMEOUT", "30000")) TEST_TIMEOUT = int(os.getenv("TEST_TIMEOUT", "5000"))🤖 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 `@tests/audio_flow_test.py` around lines 36 - 37, Update the DEFAULT_TIMEOUT definition in tests/audio_flow_test.py to read its own environment variable rather than TEST_TIMEOUT, preserving the separate longer default for Godot initialization/network-idle navigation while TEST_TIMEOUT remains the shorter per-step timeout.
🧹 Nitpick comments (4)
tests/reset_audio_flow_test.py (1)
66-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
wait_for_console_loghelper.Same helper duplicated again; candidate for a shared
conftest.pyutility.🤖 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 `@tests/reset_audio_flow_test.py` around lines 66 - 79, Remove the duplicated wait_for_console_log helper from the test and reuse a shared utility from conftest.py, preserving its predicate matching, start_idx handling, timeout behavior, and failure message.tests/volume_sliders_mutes_test.py (1)
66-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
wait_for_console_loghelper.Same helper duplicated again across the test suite.
🤖 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 `@tests/volume_sliders_mutes_test.py` around lines 66 - 79, Remove the duplicated wait_for_console_log helper from this test module and reuse the existing shared helper already provided by the test suite, updating references or imports as needed while preserving its polling and timeout behavior.tests/audio_flow_test.py (1)
54-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
wait_for_console_logis duplicated verbatim across every test file.Identical helper (and identical
on_console/logs-buffer setup) is copy-pasted here and inback_flow_test.py,reset_audio_flow_test.py,volume_sliders_mutes_test.py, and per graph context alsodifficulty_flow_test.py/navigation_to_audio_test.py. Extracting it intoconftest.py(e.g., as a fixture that wrapspageand exposeslogs/wait_for_console_log) would remove this duplication.🤖 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 `@tests/audio_flow_test.py` around lines 54 - 67, Extract the duplicated console-log buffer, on_console setup, and wait_for_console_log helper into a shared fixture in conftest.py. Update the affected test files to use that fixture instead of defining their own logs and polling helper, preserving the existing predicate, timeout, and failure behavior.tests/back_flow_test.py (1)
67-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
wait_for_console_loghelper.Same helper as in the sibling flow tests; candidate for extraction into
conftest.py.🤖 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 `@tests/back_flow_test.py` around lines 67 - 80, Extract the duplicated wait_for_console_log helper from the flow tests into the shared conftest.py test utilities, then update the affected tests to use the centralized helper while preserving its predicate, timeout, polling, and failure behavior.
🤖 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 `@tests/audio_flow_test.py`:
- Around line 36-37: Update the DEFAULT_TIMEOUT definition in
tests/audio_flow_test.py to read its own environment variable rather than
TEST_TIMEOUT, preserving the separate longer default for Godot
initialization/network-idle navigation while TEST_TIMEOUT remains the shorter
per-step timeout.
In `@tests/back_flow_test.py`:
- Around line 38-39: Update the timeout constant definitions in
back_flow_test.py so DEFAULT_TIMEOUT reads its intended distinct environment
variable rather than TEST_TIMEOUT, while leaving TEST_TIMEOUT bound to
TEST_TIMEOUT and preserving their existing fallback values.
In `@tests/reset_audio_flow_test.py`:
- Around line 37-38: Update DEFAULT_TIMEOUT in the timeout configuration near
TEST_TIMEOUT to read its intended distinct environment variable, while keeping
TEST_TIMEOUT mapped to TEST_TIMEOUT. Ensure the two constants no longer read the
same env var, matching the sibling flow tests.
In `@tests/volume_sliders_mutes_test.py`:
- Around line 37-38: Update DEFAULT_TIMEOUT and TEST_TIMEOUT in the volume
slider mute tests to read their intended, distinct environment variables,
matching the sibling flow tests; retain the existing default values and integer
parsing.
---
Nitpick comments:
In `@tests/audio_flow_test.py`:
- Around line 54-67: Extract the duplicated console-log buffer, on_console
setup, and wait_for_console_log helper into a shared fixture in conftest.py.
Update the affected test files to use that fixture instead of defining their own
logs and polling helper, preserving the existing predicate, timeout, and failure
behavior.
In `@tests/back_flow_test.py`:
- Around line 67-80: Extract the duplicated wait_for_console_log helper from the
flow tests into the shared conftest.py test utilities, then update the affected
tests to use the centralized helper while preserving its predicate, timeout,
polling, and failure behavior.
In `@tests/reset_audio_flow_test.py`:
- Around line 66-79: Remove the duplicated wait_for_console_log helper from the
test and reuse a shared utility from conftest.py, preserving its predicate
matching, start_idx handling, timeout behavior, and failure message.
In `@tests/volume_sliders_mutes_test.py`:
- Around line 66-79: Remove the duplicated wait_for_console_log helper from this
test module and reuse the existing shared helper already provided by the test
suite, updating references or imports as needed while preserving its polling and
timeout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37a523f8-df06-4307-9c86-1643e5b6700d
📒 Files selected for processing (10)
tests/audio_flow_test.pytests/back_flow_test.pytests/conftest.pytests/difficulty_flow_test.pytests/load_main_menu_test.pytests/navigation_to_audio_test.pytests/no_error_logs_test.pytests/reset_audio_flow_test.pytests/validate_clean_load_test.pytests/volume_sliders_mutes_test.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/conftest.py
- tests/load_main_menu_test.py
- tests/validate_clean_load_test.py
- tests/no_error_logs_test.py
- tests/navigation_to_audio_test.py
- tests/difficulty_flow_test.py
📜 Review details
⚠️ CI failures not shown inline (1)
Commit Status: DeepSource: Python: DeepSource: Python
Conclusion: failure
Analysis failed: Blocking issues or failing metrics found
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-19T05:07:07.286Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 488
File: tests/difficulty_flow_test.py:194-200
Timestamp: 2026-03-19T05:07:07.286Z
Learning: When writing/adjusting tests that assert SkyLockAssault log output for difficulty (and other float settings), expect the decimal point to be preserved (e.g., logs like "setting 'difficulty' updated to: 1.0"). Do not use regexes that fail on floats due to the decimal point (e.g., patterns with a negative lookahead that assumes digits contain no '.'), since they will not match "1.0". Instead, use a simple substring check for the expected log prefix/value, or use a float-aware regex (e.g., matching `\d+(?:\.\d+)?`) / parse the logged value as a float before asserting.
Applied to files:
tests/audio_flow_test.pytests/back_flow_test.pytests/reset_audio_flow_test.pytests/volume_sliders_mutes_test.py
🪛 ast-grep (0.44.1)
tests/back_flow_test.py
[warning] 300-302: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(
f"artifacts/test_back_failure_console_logs_{timestamp}.txt", "w"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
tests/reset_audio_flow_test.py
[warning] 502-504: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(
f"artifacts/test_reset_failure_console_logs_{timestamp}.txt", "w"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
tests/volume_sliders_mutes_test.py
[warning] 411-413: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(
f"artifacts/test_volume_failure_console_logs_{timestamp}.txt", "w"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.21)
tests/audio_flow_test.py
[warning] 289-289: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🔇 Additional comments (9)
tests/audio_flow_test.py (3)
288-290: 🎯 Functional Correctness | ⚡ Quick winBare
assert Falsestill unresolved.Ruff flags this (B011);
assertis stripped underpython -O. This was already raised in a previous review round with a proposedraise AssertionError(...)fix that hasn't been applied.Source: Linters/SAST tools
292-303: 🩺 Stability & Availability | ⚡ Quick win
except Exceptionwill not catchpytest.fail()timeouts fromwait_for_console_log.
pytest.fail()raisesFailed, which subclassesOutcomeException(BaseException), notException. Whenwait_for_console_log(lines 65-67) times out, the exception bypasses thisexcept Exception as e:block entirely, so the screenshot, console-log dump, and HTML capture never run for what is now the most likely failure mode this PR introduces. Thefinallyblock still runs (coverage save), but debugging artifacts are silently lost.🛡️ Proposed fix
- except Exception as e: + except (Exception, pytest.fail.Exception) as e:
82-267: LGTM!tests/back_flow_test.py (2)
296-306: 🩺 Stability & Availability | ⚡ Quick win
except Exceptionmissespytest.fail()-raised timeouts.Same gap as elsewhere:
wait_for_console_log'spytest.fail()(lines 78-80) raisesFailed(BaseException), so thisexcept Exception as e:never triggers for it, skipping screenshot/log capture on that failure path.
95-294: LGTM!tests/reset_audio_flow_test.py (2)
498-508: 🩺 Stability & Availability | ⚡ Quick win
except Exceptionmissespytest.fail()-raised timeouts.Same gap:
wait_for_console_log'spytest.fail()(lines 77-79) raisesFailed(BaseException), bypassing this catch and losing the failure screenshot/log dump.
94-497: LGTM!tests/volume_sliders_mutes_test.py (2)
405-417: 🩺 Stability & Availability | ⚡ Quick win
except Exceptionmissespytest.fail()-raised timeouts.Same gap:
wait_for_console_log'spytest.fail()(lines 77-79) raisesFailed(BaseException), bypassing this catch and losing the failure screenshot/log dump.
94-403: LGTM!
Reflow and clean up tests/back_flow_test.py: wrap long overview and docstring lines, split long JS predicate strings passed to wait_for_function for readability, tidy the wait_for_console_log signature and docstring, and remove an outdated comment about Playwright JS coverage. These are purely formatting and wording changes with no intended behavioral changes.
Wrap long docstrings and configuration text to improve line length compliance and code readability.
Reformat test file to improve code readability and comply with style guidelines: - Break long lines into multiple lines (especially JavaScript/CSS selectors and error messages) - Add blank lines in docstrings after summary line (PEP 257) - Simplify some error message text for clarity - No functional changes to test logic or behavior
Reformat volume_sliders_mutes_test.py to comply with line length limits: - Break long lines in function signatures and docstrings - Split long JavaScript string literals across multiple lines - Convert multi-line docstring to single-line format
Addressed! Switched `wait_for_console_log` in `tests/test_utils.py` from `time.time()` to `time.monotonic()` to protect polling deadlines against system clock adjustments and drift.
Addressed! Switched |
Addressed! Centralized `has_save_log` into `tests/test_utils.py` and updated `tests/volume_sliders_mutes_test.py` (and sibling test files) to import and share the common helper.
Addressed! Centralized |
Addressed! Updated `wait_for_console_log` in `tests/test_utils.py` to raise `AssertionError` instead of calling `pytest.fail`. This allows timeout exceptions to be caught by each test's `except Exception as e:` block, ensuring screenshots, console logs, and HTML snapshots are captured on failure before the exception is raised.
Add an additional wait_for_console_log assertion in the test_reset_flow test to verify that console logs contain messages about saving, encryption, or plaintext fallback after a state change.
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)
tests/volume_sliders_mutes_test.py (1)
68-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the primary test failure during V8 coverage cleanup.
The coverage block starts
cdp_sessionbefore the CDP commands finish, and thefinallyattests/volume_sliders_mutes_test.py:490-496unconditionally runs coverage shutdown and disk writes. Any setup, collection, shutdown, or write failure can overwrite or replace the original test failure. Track whether coverage collection has actually started and make shutdown/writing best-effort so the real failure is not lost.🤖 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 `@tests/volume_sliders_mutes_test.py` around lines 68 - 74, Update the coverage setup and cleanup in the test around cdp_session and its finally block to track whether coverage collection successfully started, rather than using cdp_session existence alone. Run coverage shutdown and disk writes only when collection started, and make all cleanup/write failures non-fatal so they cannot replace the primary test failure.
🤖 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 `@tests/volume_sliders_mutes_test.py`:
- Around line 68-74: Update the coverage setup and cleanup in the test around
cdp_session and its finally block to track whether coverage collection
successfully started, rather than using cdp_session existence alone. Run
coverage shutdown and disk writes only when collection started, and make all
cleanup/write failures non-fatal so they cannot replace the primary test
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 66a8e8bb-6c83-4889-a2fc-0516b1c3d2dd
📒 Files selected for processing (4)
tests/reset_audio_flow_test.pytests/test_utils.pytests/volume_sliders_mutes_test.pyworkspace/run_browser_tests.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- workspace/run_browser_tests.sh
- tests/reset_audio_flow_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-19T05:07:07.286Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 488
File: tests/difficulty_flow_test.py:194-200
Timestamp: 2026-03-19T05:07:07.286Z
Learning: When writing/adjusting tests that assert SkyLockAssault log output for difficulty (and other float settings), expect the decimal point to be preserved (e.g., logs like "setting 'difficulty' updated to: 1.0"). Do not use regexes that fail on floats due to the decimal point (e.g., patterns with a negative lookahead that assumes digits contain no '.'), since they will not match "1.0". Instead, use a simple substring check for the expected log prefix/value, or use a float-aware regex (e.g., matching `\d+(?:\.\d+)?`) / parse the logged value as a float before asserting.
Applied to files:
tests/test_utils.pytests/volume_sliders_mutes_test.py
🔇 Additional comments (4)
tests/test_utils.py (2)
17-24: LGTM!
35-40: LGTM!tests/volume_sliders_mutes_test.py (2)
179-186: The broad save-log fallback still permits false positives.The master and music waits can succeed on unrelated
"saved"or"encrypted"logs, so they do not necessarily verify the corresponding AudioManager volume update. Restrict these predicates to the operation-specific log, or add the same scoped follow-up assertion used for SFX.Also applies to: 234-241
5-11: LGTM!Also applies to: 35-47, 56-66, 76-178, 187-233, 242-301, 302-475, 477-489
Addressed! Added deterministic `wait_for_console_log` synchronization following both `toggleMuteMaster([1])` and `toggleMuteSfx([1])` calls in `tests/audio_flow_test.py` to eliminate timing race conditions before subsequent volume adjustments.
….com/ikostan/SkyLockAssault into code-audits-asynchronous-refactoring
Addressed! Updated `tests/volume_sliders_mutes_test.py` to track `coverage_started = True` only after CDP profiler commands complete successfully, and wrapped the `finally` block teardown and disk writes in a `try...except` block so coverage errors remain non-fatal and cannot mask primary test failures.
Addressed! Updated |
|
@sourcery-ai review |
|
@sourcery-ai review |
|
Sorry @ikostan, your pull request is larger than the review limit of 150000 diff characters |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
1 participant
Refactor test infrastructure for improved reliability:
These changes eliminate timing-dependent flakiness common in browser automation by making assertions wait for actual state changes rather than arbitrary durations.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
PR #845 Summary: Replace brittle waits with deterministic polling
Repository: ikostan/SkyLockAssault
Author: @ikostan
Branch:
code-audits-asynchronous-refactoring→mainLinked Issue: #772 – [TASK] Code Audits & Asynchronous Refactoring
Milestone: Milestone 22 – Optimize Test Suite Runtime & Fix Loading Screen
Labels:
enhancement,testing,refactoring,python,QAPurpose
Eliminate timing-dependent flakiness in the Playwright browser E2E test suite by replacing all fixed-duration waits (
page.wait_for_timeout(), arbitrary sleeps) with deterministic, state-based synchronization. The changes make tests wait for actual engine readiness, DOM visibility, or console-log events instead of relying on brittle timeouts.Core Improvements
1. Deterministic Waiting & Assertions
page.wait_for_timeout()with:page.wait_for_function()for Godot initialization (window.godotInitialized === true) and DOM style/display checksexpect(locator).to_be_visible()for canvas, buttons, and overlayswait_for_console_log(logs, predicate, start_idx, page)helper that polls captured console messages with a predicate until the condition is met or a timeout occurs=== trueinstead of truthy)2. Shared Test Utilities & Fixtures
tests/test_utils.pyDEFAULT_TIMEOUT/TEST_TIMEOUT(environment-configurable)tests/conftest.py:browser_instancefixture (single Chromium launch with GPU/WebGL flags)BrowserContext+Pageper testBrowser,BrowserContext,Page,Generator, etc.)3. Test Coverage Stabilization
Affected test files (all converted to deterministic waits):
tests/audio_flow_test.pytests/volume_sliders_mutes_test.pytests/reset_audio_flow_test.pytests/difficulty_flow_test.pytests/back_flow_test.pytests/navigation_to_audio_test.pytests/load_main_menu_test.pytests/no_error_logs_test.pytests/validate_clean_load_test.pyImprovements include:
window.xxxPressed)getComputedStyle(...).display4. Browser Test Runner Hardening (
workspace/run_browser_tests.sh)git config --global --add safe.directoryto avoid “dubious ownership” errors in containersgit restoreofexport_presets.cfgandglobals.gdtraponEXIT/INT/TERM) for the background HTTP serverkillof server PID in favor of trap-based cleanup5. Code Quality & Maintainability
Any,Callable,Generator,Dict,List,Optional)has_save_log) into the shared utility moduleBenefits
AI / Bot Assistance (for reference)
Status Notes
The PR fully addresses the objectives of issue #772 regarding wait refactoring, fixture lifecycle optimization, and runner robustness.
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
Refactor browser E2E test suite to use deterministic state-based waits instead of fixed timeouts for Godot initialization, UI visibility, and log-driven flows, improving reliability and reducing flakiness.
Enhancements:
Build:
Tests:
Summary by CodeRabbit
AI / Bot Contributors
@sourcery-ai
Generated the PR summary and Reviewer's Guide. Performed code review identifying duplication of
wait_for_console_log, suggesting extraction to a shared utility, recommending Playwright locator assertions over raw JSwait_for_functionchecks, and noting potential tracing cleanup for session-scoped browser fixtures.@coderabbitai
Generated the PR summary and detailed walkthrough. Conducted multiple rounds of code review with actionable suggestions (timeout centralization, helper extraction, assertion improvements, shell-script hardening). Co-authored commits updating
workspace/run_browser_tests.sh.@deepsource-io
Performed automated code review, published a PR Report Card (Security / Reliability / Complexity / Hygiene), and provided analysis status for Python and JavaScript.
@deepsource-autofix
Authored multiple automated style/format commits (
style: format code with Black and isort) to enforce formatting consistency across test files.Human Contributor
Primary author of the PR. Implemented the core refactor (deterministic polling with
wait_for_function/expect(),wait_for_console_loghelper, session-scoped browser + function-scoped contexts, type hints, and hardenedrun_browser_tests.sh). Authored the majority of commits, addressed review feedback, extracted shared utilities totests/test_utils.py, and created supporting documentation.