Implement test profiling & metrics baseline - #870
Merged
ikostan merged 57 commits intoAug 7, 2026
Merged
Conversation
Establishes automated profiling infrastructure and statistical baseline for E2E test suite. Implements pytest hooks to capture per-test execution duration, overall session metrics, and WASM initialization latency. Exports baseline JSON to artifacts/ for tracking progress toward Epic #771's ~70% runtime reduction goal. Integrates profiling artifact preservation into CI/CD pipeline with 5-run median baseline of 93.16 seconds.
ikostan
added this to the
Milestone 22: Optimize Test Suite Runtime & Fix Loading Screen milestone
ikostan
added
documentation
6 tasks
ikostan
added
performance
refactoring
python
Contributor
Reviewer's GuideIntroduces a pytest-based profiling infrastructure that records per-test and session-level metrics (including Godot WASM boot time), exports them as a metrics_baseline.json artifact, wires the artifact into CI and local scripts, and documents the new baseline and supporting AI model guidance, backed by targeted tests. Sequence diagram for pytest profiling hooks and WASM latency capturesequenceDiagram
participant PytestRunner
participant TestCase
participant TestUtils
participant PytestHooks
participant MetricsFile
PytestRunner->>PytestHooks: pytest_sessionstart
PytestRunner->>TestCase: run_test
TestCase->>TestUtils: navigate_and_profile_godot_wasm
TestUtils->>TestUtils: init_page_and_wait_ready
TestUtils-->>TestCase: wasm_boot_duration
TestCase->>PytestHooks: pytest_runtest_makereport
PytestRunner->>PytestHooks: pytest_sessionfinish
PytestHooks->>MetricsFile: [metrics_baseline.json written]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR adds WASM boot timing and Playwright session metrics. It exports baseline metrics as JSON, preserves shard-specific CI artifacts, updates pipeline cleanup, and documents profiling results and local AI model guidance. ChangesBrowser profiling metrics
Local AI model reference
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PlaywrightTest
participant init_page_and_wait_ready
participant GodotWebPage
participant PytestHooks
participant CIArtifacts
PlaywrightTest->>init_page_and_wait_ready: initialize page with request
init_page_and_wait_ready->>GodotWebPage: wait for WASM readiness
GodotWebPage-->>init_page_and_wait_ready: return boot duration
PlaywrightTest->>PytestHooks: report test duration and outcome
PytestHooks->>CIArtifacts: export and upload baseline metrics
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
19 tasks
![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 found 2 issues, and left some high level feedback:
- The metrics_baseline.json path is inconsistent between the pytest export (artifacts/metrics_baseline.json) and the GitHub Actions workflow (tests/metrics/metrics_baseline.json); align these so CI reliably finds and uploads the same file.
- In pytest_sessionfinish you print a warning on JSON export failure; consider routing this through pytest's logging/terminal reporting so the message is clearly visible and consistent with other test output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The metrics_baseline.json path is inconsistent between the pytest export (artifacts/metrics_baseline.json) and the GitHub Actions workflow (tests/metrics/metrics_baseline.json); align these so CI reliably finds and uploads the same file.
- In pytest_sessionfinish you print a warning on JSON export failure; consider routing this through pytest's logging/terminal reporting so the message is clearly visible and consistent with other test output.
## Individual Comments
### Comment 1
<location path="tests/conftest.py" line_range="84-93" />
<code_context>
+ if _SESSION_START_TIME
+ else 0.0
+ )
+ metrics_payload = {
+ "timestamp": _SESSION_START_TIMESTAMP,
+ "total_duration_sec": total_duration,
+ "summary": _SUMMARY_COUNTS,
+ "tests": _TEST_PROFILING_DATA,
+ }
+
+ metrics_file = ARTIFACTS_DIR / "metrics_baseline.json"
+ try:
+ with open(metrics_file, "w", encoding="utf-8") as f:
+ json.dump(metrics_payload, f, indent=2)
+ except Exception as exc: # noqa: BLE001 - best effort export
+ print(f"Warning: Failed to write metrics baseline: {exc}")
+
+ # 2. Safely terminate tracked sub-processes
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that asserts `metrics_baseline.json` structure and graceful failure behaviour.
The new `pytest_sessionfinish` hook writes `metrics_baseline.json` and logs a warning on failure, but this isn’t covered by tests. Please add a test that runs a small suite and verifies that `metrics_baseline.json` is created with the expected keys (`timestamp`, `total_duration_sec`, `summary`, `tests`) and that `tests` length matches the executed tests. Also consider a test that simulates an I/O failure (e.g., monkeypatching `open` to raise) to confirm the session still completes and a warning is emitted instead of failing the run.
Suggested implementation:
```python
from __future__ import annotations
import json
from types import SimpleNamespace
def _invoke_sessionfinish(conftest_module, artifacts_dir, *, payload):
"""Helper to invoke pytest_sessionfinish with controlled globals."""
# Override globals used by pytest_sessionfinish
conftest_module.ARTIFACTS_DIR = artifacts_dir
conftest_module._SESSION_START_TIME = payload.get("_SESSION_START_TIME")
conftest_module._SESSION_START_TIMESTAMP = payload.get("_SESSION_START_TIMESTAMP")
conftest_module._SUMMARY_COUNTS = payload.get("_SUMMARY_COUNTS", {})
conftest_module._TEST_PROFILING_DATA = payload.get("_TEST_PROFILING_DATA", [])
# Dummy session object; pytest_sessionfinish only needs the signature
dummy_session = SimpleNamespace()
conftest_module.pytest_sessionfinish(dummy_session, exitstatus=0)
def test_metrics_baseline_file_structure(tmp_path, monkeypatch):
"""
The pytest_sessionfinish hook should export metrics_baseline.json
with the expected top-level keys and the correct tests length.
"""
from tests import conftest as conf # import the same module that defines the hook
payload = {
"_SESSION_START_TIME": 1.0,
"_SESSION_START_TIMESTAMP": 1_700_000_000.0,
"_SUMMARY_COUNTS": {"passed": 1, "failed": 0, "skipped": 0},
"_TEST_PROFILING_DATA": [
{"nodeid": "test_example[case-1]", "duration_sec": 0.1234},
{"nodeid": "test_example[case-2]", "duration_sec": 0.5678},
],
}
_invoke_sessionfinish(conf, tmp_path, payload=payload)
metrics_file = tmp_path / "metrics_baseline.json"
assert metrics_file.is_file()
data = json.loads(metrics_file.read_text(encoding="utf-8"))
# Structure
assert set(data.keys()) == {
"timestamp",
"total_duration_sec",
"summary",
"tests",
}
# Content sanity
assert data["timestamp"] == payload["_SESSION_START_TIMESTAMP"]
assert isinstance(data["total_duration_sec"], float)
assert data["summary"] == payload["_SUMMARY_COUNTS"]
assert isinstance(data["tests"], list)
assert len(data["tests"]) == len(payload["_TEST_PROFILING_DATA"])
assert data["tests"] == payload["_TEST_PROFILING_DATA"]
def test_metrics_baseline_io_failure_is_graceful(tmp_path, monkeypatch, capsys):
"""
If writing metrics_baseline.json fails, the hook should not crash the session
and must emit a warning instead.
"""
from tests import conftest as conf # import the same module that defines the hook
payload = {
"_SESSION_START_TIME": None, # ensures total_duration_sec falls back to 0.0
"_SESSION_START_TIMESTAMP": 1_700_000_000.0,
"_SUMMARY_COUNTS": {},
"_TEST_PROFILING_DATA": [],
}
# Ensure we don't touch any real directories
conf.ARTIFACTS_DIR = tmp_path
# Force an I/O error when pytest_sessionfinish tries to open the metrics file
def failing_open(*args, **kwargs):
raise OSError("simulated write failure")
monkeypatch.setattr(conf, "open", failing_open)
_invoke_sessionfinish(conf, tmp_path, payload=payload)
out = capsys.readouterr().out
assert "Warning: Failed to write metrics baseline" in out
```
These tests assume that:
1. `pytest_sessionfinish` and the globals `_SESSION_START_TIME`, `_SESSION_START_TIMESTAMP`, `_SUMMARY_COUNTS`, `_TEST_PROFILING_DATA`, and `ARTIFACTS_DIR` are defined in `tests/conftest.py` (as in your snippet).
2. `tests` is the top-level tests package so `from tests import conftest as conf` imports the same module where the hook is defined.
If your project structure differs (e.g., `conftest.py` is not importable as `tests.conftest`), adjust the import in `test_metrics_baseline.py` to match how the module is actually imported (for example, `import conftest as conf`). No changes inside `tests/conftest.py` are required beyond what you already have.
</issue_to_address>
### Comment 2
<location path="tests/test_utils.py" line_range="106-115" />
<code_context>
def init_page_and_wait_ready(
- page: Page, url: str = "http://localhost:8080/index.html"
-) -> None:
</code_context>
<issue_to_address>
**issue (testing):** Extend or add tests for `init_page_and_wait_ready` to cover new timing and request behaviors.
The helper now returns a boot time and may write `_wasm_boot_time` on `request.node`, but there are no tests covering this. Please add tests that:
- When `window.godotInitialized === true`, the function returns `0.0` and does not set `_wasm_boot_time`.
- When initialization occurs, the returned float is > 0 and `request.node._wasm_boot_time` matches the rounded return value (when a `request` with `node` is provided).
- The function still behaves correctly when `request` is `None`.
You can mock `time.perf_counter` and `page.evaluate`/`page.locator` as needed to verify the profiling behavior and WASM timing hooks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Update file paths for metrics baseline from tests/metrics/ to artifacts/ directory. Remove unnecessary YAML front matter marker (---) from workflow file.
Owner
Author
Sourcery AI caught a genuine path mismatch. In .github/workflows/browser_tests.yml, the workflow steps were still checking for tests/metrics/metrics_baseline.json. Because conftest.py writes directly to artifacts/metrics_baseline.json, the check if [ -f "tests/metrics/metrics_baseline.json" ] evaluated to false, preventing CI from renaming and uploading the shard metrics. |
![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: 6
🧹 Nitpick comments (1)
workspace/run_browser_tests.sh (1)
106-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the runtime after optional activation.
When
/opt/venv/bin/activateis missing or fails,|| trueleaves the script on the existingPATH. The script can then use a different Python or Playwright version, which can invalidate the profiling baseline, or fail later with an unclear error.Check the required commands and versions after activation, or exit when the required CI environment is missing.
🤖 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 `@workspace/run_browser_tests.sh` at line 106, Update the runtime setup around the optional /opt/venv/bin/activate source command to validate that the required Python and Playwright commands and versions are available afterward. If activation fails or the expected CI runtime is missing, exit with a clear error instead of continuing on the existing PATH.
🤖 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 @.github/workflows/browser_test.yml:
- Around line 240-241: Shorten the two long commands in the metrics artifact
step by assigning the source and destination paths to concise variables, then
reuse them for the cp command and status message; preserve the existing artifact
filename and behavior while keeping each YAML line under 90 characters.
- Around line 236-243: Update the “Preserve Baseline Profiling Metrics (`#776`)”
workflow step to read the baseline from artifacts/metrics_baseline.json,
matching the path written by tests/conftest.py, while preserving the existing
artifact naming and copy behavior.
In `@files/docs/milestones/22/Part_7_Test_Profiling_`&_Metrics_Baseline.md:
- Line 48: Update the checklist entry in
Part_7_Test_Profiling_&_Metrics_Baseline.md to reference the existing
`.github/workflows/browser_test.yml` filename instead of the pluralized
`browser_tests.yml`.
- Line 52: Correct the closing markdownlint directive in the document by
replacing the invalid “markdownlint-denable” token with “markdownlint-enable,”
preserving the existing rule identifiers so the opening directive’s disabled
rules are restored.
In `@tests/conftest.py`:
- Around line 52-73: Update pytest_runtest_makereport to aggregate setup, call,
and teardown outcomes into exactly one finalized record per test, rather than
recording only the call phase. Ensure setup failures/skips and teardown failures
affect the final outcome, while preserving duration and wasm boot profiling data
in _TEST_PROFILING_DATA and updating _SUMMARY_COUNTS only when the test’s final
phase result is determined.
In `@tests/test_utils.py`:
- Around line 117-121: Update the helper containing the
`page.evaluate("window.godotInitialized === true")` branch so it assigns the
zero boot duration to `request.node._wasm_boot_time` before returning. Apply the
same assignment in the normal initialization path, ensuring both paths provide a
value for `pytest_runtest_makereport` to record.
---
Nitpick comments:
In `@workspace/run_browser_tests.sh`:
- Line 106: Update the runtime setup around the optional /opt/venv/bin/activate
source command to validate that the required Python and Playwright commands and
versions are available afterward. If activation fails or the expected CI runtime
is missing, exit with a clear error instead of continuing on the existing PATH.
🪄 Autofix
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: 3b8cb41a-4c1a-4d04-821f-b2313e10d72a
📒 Files selected for processing (6)
.github/workflows/browser_test.ymlfiles/docs/milestones/22/Part_7_Test_Profiling_&_Metrics_Baseline.mdtests/conftest.pytests/test_utils.pyworkspace/run_browser_tests.shworkspace/run_pipeline.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
- GitHub Check: GDUnit4 Unit Tests / unit-test
- GitHub Check: Sourcery review
🧰 Additional context used
🧠 Learnings (3)
📚 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/conftest.pytests/test_utils.py
📚 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
🪛 ast-grep (0.45.0)
tests/conftest.py
[warning] 94-94: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(metrics_file, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 GitHub Check: YAML Lint / build (3.x)
.github/workflows/browser_test.yml
[warning] 241-241:
241:91 [line-length] line too long (112 > 90 characters)
[warning] 240-240:
240:91 [line-length] line too long (116 > 90 characters)
🪛 LanguageTool
files/docs/milestones/22/Part_7_Test_Profiling_&_Metrics_Baseline.md
[uncategorized] ~48-~48: The official name of this software platform is spelled with a capital “H”.
Context: ...eserve profiling metrics. - [x] Updated .github/workflows/browser_tests.yml to preserv...
(GITHUB)
🪛 Shellcheck (0.11.0)
workspace/run_browser_tests.sh
[info] 106-106: Not following: /opt/venv/bin/activate was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (6)
tests/test_utils.py (2)
273-280: 🗄️ Data Integrity & IntegrationVerify the request is passed on profiled call paths.
_wasm_boot_timeis written only when the optionalrequestis supplied. The provided callers intests/conftest.pyLine 222 andtests/load_main_menu_test.pycallinit_page_and_wait_ready(page)without it. Those tests producewasm_boot_duration_sec: null.If those tests belong in the profiling baseline, pass the pytest request through each call. For the module-scoped
shared_pagefixture, use a fixture-level metric or document the exclusion.
107-114: LGTM!Also applies to: 142-148
tests/conftest.py (1)
4-11: LGTM!Also applies to: 25-50, 76-117, 122-122
files/docs/milestones/22/Part_7_Test_Profiling_&_Metrics_Baseline.md (1)
1-47: LGTM!Also applies to: 49-51
workspace/run_browser_tests.sh (1)
2-2: LGTM!Also applies to: 54-54, 118-118
workspace/run_pipeline.sh (1)
39-39: LGTM!
Improve report management by moving GDUnit4 test reports to artifacts earlier in cleanup and removing intermediate copy step. Changes include: - Clean up reports directory at pipeline start - Move reports to artifacts during EXIT trap for early artifact collection - Remove intermediate mkdir/cp step that duplicated reports - Use atomic mv operations with cleanup instead of cp This ensures reports are reliably captured in artifacts and avoids leaving stray files in the project root.
…tion' of https://github.com/ikostan/SkyLockAssault into optimize-test-suite-runtime-and-prevent-ci-limit-exhaustion
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Aug 7, 2026 4:49a.m. | Review ↗ |
| JavaScript | |
Aug 7, 2026 4:49a.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}
Update workflow and pipeline to account for reports relocated to artifacts/gdunit-reports. The GH Actions step that finds the latest report now searches both reports/* and artifacts/gdunit-reports/*, and the upload-artifact step uploads both paths (with if-no-files-found: ignore). The run_pipeline.sh script now ensures the artifacts/ directory exists before moving reports into artifacts/gdunit-reports to avoid move failures.
…tion' of https://github.com/ikostan/SkyLockAssault into optimize-test-suite-runtime-and-prevent-ci-limit-exhaustion
Update run_browser_tests.sh and run_pipeline.sh to invoke pytest with python3 -m pytest and verify availability using python3 -m pytest --version. This avoids relying on PATH/command -v and ensures the pytest from the active Python/virtualenv is used when running tests and checking prerequisites.
6 tasks
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
pytest_terminal_summary, you recompute the total suite duration withtime.perf_counter()instead of reusing the value calculated inpytest_sessionfinish, which can cause minor discrepancies between the printed summary and the JSON baseline; consider storing and reusing the computed duration for consistency. - For
init_page_and_wait_readyandnavigate_and_profile_godot_wasm, you currently accept a looserequest: Any | Noneand silently skip attaching_wasm_boot_timeifrequestlacks anodeattribute; tightening the type (e.g., topytest.FixtureRequestor a protocol) or adding an explicit guard/assert would make misuse easier to detect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pytest_terminal_summary`, you recompute the total suite duration with `time.perf_counter()` instead of reusing the value calculated in `pytest_sessionfinish`, which can cause minor discrepancies between the printed summary and the JSON baseline; consider storing and reusing the computed duration for consistency.
- For `init_page_and_wait_ready` and `navigate_and_profile_godot_wasm`, you currently accept a loose `request: Any | None` and silently skip attaching `_wasm_boot_time` if `request` lacks a `node` attribute; tightening the type (e.g., to `pytest.FixtureRequest` or a protocol) or adding an explicit guard/assert would make misuse easier to detect.
## Individual Comments
### Comment 1
<location path="tests/test_utils_test.py" line_range="19-28" />
<code_context>
+)
+
+
+def test_init_page_already_initialized_returns_zero_and_sets_node() -> None:
+ """Verify short-circuit returns 0.0 and sets request.node._wasm_boot_time.
+
+ When window.godotInitialized is True, the function must return 0.0
+ and assign 0.0 to request.node._wasm_boot_time.
+ """
+ mock_page = MagicMock()
+ mock_page.evaluate.return_value = True
+
+ mock_request = SimpleNamespace(node=SimpleNamespace())
+
+ with patch("tests.test_utils.expect"):
+ boot_time = init_page_and_wait_ready(mock_page, request=mock_request)
+
+ assert boot_time == 0.0
+ assert hasattr(mock_request.node, "_wasm_boot_time")
+ assert mock_request.node._wasm_boot_time == 0.0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for exception path and request objects without `node` to cover init_page_and_wait_ready edge cases
The current tests cover only the happy paths (already-initialized page, fresh loads, and wrapper delegation). Please also cover:
1) The branch where `page.evaluate` raises and we fall back to navigation, verifying boot timing is still computed and stored on `request.node` when present.
2) A `request` with no `node` (or `request.node` is `None`), to confirm we don’t hit an `AttributeError` when setting `_wasm_boot_time` and to clarify the expected `request` contract.
These tests will ensure `init_page_and_wait_ready` handles these edge cases correctly.
</issue_to_address>
### Comment 2
<location path="tests/test_utils_test.py" line_range="73-82" />
<code_context>
+ assert boot_time == 2.5
+
+
+def test_navigate_and_profile_godot_wasm_delegates_to_init_page() -> None:
+ """Verify wrapper helper delegates arguments to init_page_and_wait_ready."""
+ mock_page = MagicMock()
+ mock_request = SimpleNamespace(node=SimpleNamespace())
+
+ with patch(
+ "tests.test_utils.init_page_and_wait_ready", return_value=1.5
+ ) as mock_init:
+ result = navigate_and_profile_godot_wasm(
+ mock_page, url="http://localhost:8080/test.html", request=mock_request
+ )
+
+ assert result == 1.5
+ mock_init.assert_called_once_with(
+ mock_page, url="http://localhost:8080/test.html", request=mock_request
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a focused test that start_game_and_wait_ready propagates the pytest request for WASM boot profiling
Right now only `navigate_and_profile_godot_wasm`’s delegation is tested. Since `start_game_and_wait_ready` now owns the responsibility of forwarding the pytest `request` so `_wasm_boot_time` can be attached, it would be good to add a direct test for that propagation.
A minimal test could:
- Stub out `init_cdp_coverage`, `open_options_menu`, and other Playwright-facing helpers.
- Call `start_game_and_wait_ready` with a `SimpleNamespace(node=SimpleNamespace())` as `request`.
- Assert that `init_page_and_wait_ready` receives the same `request` and that `_wasm_boot_time` is set on `request.node`.
This ensures the E2E helper actually wires the profiling hook into real tests.
Suggested implementation:
```python
def test_navigate_and_profile_godot_wasm_delegates_to_init_page() -> None:
"""Verify wrapper helper delegates arguments to init_page_and_wait_ready."""
mock_page = MagicMock()
mock_request = SimpleNamespace(node=SimpleNamespace())
with patch(
"tests.test_utils.init_page_and_wait_ready", return_value=1.5
) as mock_init:
result = navigate_and_profile_godot_wasm(
mock_page, url="http://localhost:8080/test.html", request=mock_request
)
assert result == 1.5
mock_init.assert_called_once_with(
mock_page, url="http://localhost:8080/test.html", request=mock_request
)
def test_start_game_and_wait_ready_propagates_request_and_sets_wasm_boot_time() -> None:
"""Ensure start_game_and_wait_ready forwards pytest request and sets _wasm_boot_time."""
mock_page = MagicMock()
# Force the slow-path so init_page_and_wait_ready measures a non-zero boot time.
mock_page.evaluate.return_value = False
request = SimpleNamespace(node=SimpleNamespace())
# Import inside test to avoid circular imports and to get the real implementation.
import tests.test_utils as utils
real_init_page = utils.init_page_and_wait_ready
with (
patch("tests.test_utils.init_cdp_coverage"),
patch("tests.test_utils.open_options_menu"),
patch("tests.test_utils.close_options_menu"),
patch("tests.test_utils.stop_cdp_coverage"),
# Wrap the real init_page_and_wait_ready so we can assert on arguments
# while still executing its logic (including setting _wasm_boot_time).
patch(
"tests.test_utils.init_page_and_wait_ready",
wraps=real_init_page,
) as mock_init,
patch("time.perf_counter", side_effect=[5.0, 7.5]),
patch("tests.test_utils.expect"),
):
start_game_and_wait_ready(
mock_page,
url="http://localhost:8080/test.html",
request=request,
)
# The pytest request must be forwarded into init_page_and_wait_ready
_, kwargs = mock_init.call_args
assert kwargs["request"] is request
# And the profiling helper must attach the measured boot time to request.node
assert request.node._wasm_boot_time == 2.5
```
This test assumes the following helpers exist in `tests.test_utils` and are used by `start_game_and_wait_ready`: `init_cdp_coverage`, `open_options_menu`, `close_options_menu`, and `stop_cdp_coverage`. If the actual helper names differ, update the corresponding `patch("tests.test_utils.<name>")` calls to match your implementation.
It also assumes `start_game_and_wait_ready` is already imported into this test module (likely alongside `navigate_and_profile_godot_wasm`). If not, add an import such as:
`from tests.test_utils import start_game_and_wait_ready, navigate_and_profile_godot_wasm`
near the top of `tests/test_utils_test.py`.
</issue_to_address>
### Comment 3
<location path="tests/ci/test_metrics_baseline.py" line_range="53-62" />
<code_context>
+def test_metrics_baseline_file_structure(tmp_path: Path) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for metrics_baseline behavior when no start_time is recorded or when profiling data is empty
To fully cover `pytest_sessionfinish`, please add tests for:
- `_SESSION_STATE['start_time']` being `0.0` (or absent), asserting `total_duration_sec` falls back to `0.0`.
- `_TEST_PROFILING_DATA` being empty, asserting the exporter still writes a baseline JSON with an empty `tests` list.
This will lock in the expected behavior for minimal/misconfigured runs.
Suggested implementation:
```python
def test_metrics_baseline_file_structure(tmp_path: Path) -> None:
"""Verify metrics_baseline.json schema and test count match state."""
from tests import conftest as conf
payload = {
"start_time": 1.0,
"timestamp": "2026-08-06T03:00:00Z",
"summary_counts": {"passed": 2, "failed": 0, "skipped": 0},
"test_profiling_data": [
{
"nodeid": "tests/test_a.py::test_one",
},
{
"nodeid": "tests/test_b.py::test_two",
},
],
}
# This helper is assumed to write metrics_baseline.json into tmp_path
conf.write_metrics_baseline(tmp_path, payload)
baseline = json.loads((tmp_path / "metrics_baseline.json").read_text())
assert baseline["start_time"] == payload["start_time"]
assert baseline["timestamp"] == payload["timestamp"]
assert baseline["summary_counts"] == payload["summary_counts"]
assert isinstance(baseline["tests"], list)
assert len(baseline["tests"]) == len(payload["test_profiling_data"])
def test_metrics_baseline_missing_start_time_uses_zero_duration(tmp_path: Path) -> None:
"""When no start_time is recorded, total_duration_sec should default to 0.0."""
from tests import conftest as conf
payload = {
# Explicitly omit start_time to simulate missing session state
"timestamp": "2026-08-06T03:00:00Z",
"summary_counts": {"passed": 0, "failed": 0, "skipped": 0},
"test_profiling_data": [
{
"nodeid": "tests/test_a.py::test_one",
}
],
}
conf.write_metrics_baseline(tmp_path, payload)
baseline = json.loads((tmp_path / "metrics_baseline.json").read_text())
# Plugin should fall back to 0.0 when no start_time is recorded
assert baseline.get("total_duration_sec", 0.0) == 0.0
def test_metrics_baseline_empty_profiling_data_writes_empty_tests_list(tmp_path: Path) -> None:
"""Exporter should still write a baseline when profiling data is empty."""
from tests import conftest as conf
payload = {
"start_time": 0.0,
"timestamp": "2026-08-06T03:00:00Z",
"summary_counts": {"passed": 0, "failed": 0, "skipped": 0},
"test_profiling_data": [],
}
conf.write_metrics_baseline(tmp_path, payload)
baseline = json.loads((tmp_path / "metrics_baseline.json").read_text())
assert isinstance(baseline["tests"], list)
assert baseline["tests"] == []
```
1. Ensure `json` is imported at the top of `tests/ci/test_metrics_baseline.py` (e.g., `import json`) if it is not already present.
2. Replace `conf.write_metrics_baseline(tmp_path, payload)` with the actual helper or plugin entry point your existing `test_metrics_baseline_file_structure` uses to trigger `pytest_sessionfinish` and write `metrics_baseline.json`. For example, if you currently call a plugin function directly or run a pytest subprocess, reuse that instead.
3. If your exporter uses a different key than `"total_duration_sec"` for the computed duration, update the assertion in `test_metrics_baseline_missing_start_time_uses_zero_duration` to match the real field name, e.g., `baseline.get("duration_sec", 0.0)`.
4. If `metrics_baseline.json` is written to a different location or filename than `tmp_path / "metrics_baseline.json"`, adjust the path in all three tests to match your existing convention.
</issue_to_address>
### Comment 4
<location path="tests/ci/test_metrics_baseline.py" line_range="125-134" />
<code_context>
+def test_runtest_makereport_aggregates_phases() -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Extend runtest_makereport tests to cover failed/skipped outcomes and missing phases
The hook also handles failure/skip aggregation and missing phases. To cover this, please add at least two tests:
1) A case where setup or call is skipped and teardown passes, asserting the final outcome is `"skipped"` and the duration is the sum of executed phases.
2) A case where setup fails and call/teardown are not invoked, asserting the final outcome is `"failed"`, the duration equals the setup duration only, and `_SUMMARY_COUNTS['failed']` is incremented.
These will exercise the error-path aggregation logic, not just the happy path.
Suggested implementation:
```python
def test_runtest_makereport_aggregates_phases() -> None:
"""Verify makereport aggregates setup, call, and teardown phase outcomes."""
from types import SimpleNamespace
from tests import conftest as conf
# Reset state.
conf._TEST_PROFILING_DATA.clear()
conf._SUMMARY_COUNTS = {"passed": 0, "failed": 0, "skipped": 0}
mock_item = SimpleNamespace(
nodeid="tests/test_demo.py::test_demo",
_wasm_boot_time=0.5,
)
# Simulate three passed phases with explicit durations.
setup_duration = 0.2
call_duration = 0.3
teardown_duration = 0.1
setup_report = SimpleNamespace(
when="setup",
outcome="passed",
duration=setup_duration,
)
call_report = SimpleNamespace(
when="call",
outcome="passed",
duration=call_duration,
)
teardown_report = SimpleNamespace(
when="teardown",
outcome="passed",
duration=teardown_duration,
)
# Feed each phase report through the hook under test.
conf._runtest_makereport(mock_item, setup_report)
conf._runtest_makereport(mock_item, call_report)
conf._runtest_makereport(mock_item, teardown_report)
# Verify we recorded a single aggregated entry with the total duration.
profiling_entry = conf._TEST_PROFILING_DATA[mock_item.nodeid]
assert profiling_entry["outcome"] == "passed"
assert profiling_entry["duration"] == pytest.approx(
setup_duration + call_duration + teardown_duration
)
# Verify summary counts.
assert conf._SUMMARY_COUNTS == {"passed": 1, "failed": 0, "skipped": 0}
def test_runtest_makereport_skipped_phase_aggregates_outcome_and_duration() -> None:
"""Setup or call skipped, teardown passes: outcome is 'skipped' and duration is summed."""
from types import SimpleNamespace
from tests import conftest as conf
conf._TEST_PROFILING_DATA.clear()
conf._SUMMARY_COUNTS = {"passed": 0, "failed": 0, "skipped": 0}
mock_item = SimpleNamespace(
nodeid="tests/test_demo.py::test_demo::skipped",
_wasm_boot_time=0.0,
)
setup_duration = 0.15
call_duration = 0.0 # skipped call has zero duration by convention
teardown_duration = 0.05
setup_report = SimpleNamespace(
when="setup",
outcome="skipped",
duration=setup_duration,
)
call_report = SimpleNamespace(
when="call",
outcome="skipped",
duration=call_duration,
)
teardown_report = SimpleNamespace(
when="teardown",
outcome="passed",
duration=teardown_duration,
)
# Only setup and teardown will meaningfully contribute to duration;
# outcome should be 'skipped' because at least one phase was skipped.
conf._runtest_makereport(mock_item, setup_report)
conf._runtest_makereport(mock_item, call_report)
conf._runtest_makereport(mock_item, teardown_report)
profiling_entry = conf._TEST_PROFILING_DATA[mock_item.nodeid]
assert profiling_entry["outcome"] == "skipped"
assert profiling_entry["duration"] == pytest.approx(
setup_duration + teardown_duration
)
assert conf._SUMMARY_COUNTS == {"passed": 0, "failed": 0, "skipped": 1}
def test_runtest_makereport_setup_failure_short_circuits_and_counts_failed() -> None:
"""Setup failure prevents call/teardown, counts 'failed' and only uses setup duration."""
from types import SimpleNamespace
from tests import conftest as conf
conf._TEST_PROFILING_DATA.clear()
conf._SUMMARY_COUNTS = {"passed": 0, "failed": 0, "skipped": 0}
mock_item = SimpleNamespace(
nodeid="tests/test_demo.py::test_demo::failed_setup",
_wasm_boot_time=0.0,
)
setup_duration = 0.25
setup_report = SimpleNamespace(
when="setup",
outcome="failed",
duration=setup_duration,
)
# Only a failing setup report is delivered: hook should aggregate a failed
# outcome, use the setup duration, and increment the failed summary count.
conf._runtest_makereport(mock_item, setup_report)
profiling_entry = conf._TEST_PROFILING_DATA[mock_item.nodeid]
assert profiling_entry["outcome"] == "failed"
assert profiling_entry["duration"] == pytest.approx(setup_duration)
assert conf._SUMMARY_COUNTS == {"passed": 0, "failed": 1, "skipped": 0}
```
The edits above assume the following, which you may need to align with your existing `conftest` implementation:
1. There is a helper function `conf._runtest_makereport(item, report)` that mirrors the logic of your `pytest_runtest_makereport` hook. If your helper has a different name or signature, update the calls accordingly.
2. `conf._TEST_PROFILING_DATA[...]` returns a dict-like structure with `"outcome"` and `"duration"` keys. Adjust the keys or access pattern if your actual structure differs.
3. The tests use `pytest.approx`, so ensure `pytest` is imported at the top of this file if it is not already.
4. If, in your implementation, skipped phases contribute their own non-zero duration or are handled differently (e.g., missing teardown on skip), adjust the per-phase durations and expectations in the new tests to match the real behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Add tests for exception path and request objects without node to cover init_page_and_wait_ready edge cases
adding a focused test that start_game_and_wait_ready propagates the pytest request for WASM boot profiling
Owner
Author
@sourcery-ai Thanks for the feedback! We are keeping the current implementation as-is: recomputing the duration independently in the terminal summary is lightweight and keeps the reporting hooks decoupled, and using |
Replace hasattr(request, "node") checks with getattr(request, "node", None) is not None in init_page_and_wait_ready to avoid potential attribute access issues and ensure a consistent None-check. Also adjusted surrounding inline comments; removed a redundant visual-assertion comment and clarified the updated checks. No functional behavior change aside from the safer attribute test.
…tion' of https://github.com/ikostan/SkyLockAssault into optimize-test-suite-runtime-and-prevent-ci-limit-exhaustion
Add pytest import and narrow request type to pytest.FixtureRequest. Remove a duplicated nested function and streamline the page-init flow: start_time moved earlier, skip reload when window.godotInitialized is true, use hasattr for request.node checks, and keep the canvas visibility assertion and boot_time assignment.
Wrap state mutations in _invoke_sessionfinish with a try/finally that restores conftest_module.ARTIFACTS_DIR after calling pytest_sessionfinish. This prevents the helper from leaving a modified ARTIFACTS_DIR that could affect subsequent test runs/exports. Also shortens the helper docstring for brevity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
deleted the
optimize-test-suite-runtime-and-prevent-ci-limit-exhaustion
branch
August 7, 2026 05:14
6 tasks
14 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.
Establishes automated profiling infrastructure and statistical baseline for E2E test suite. Implements pytest hooks to capture per-test execution duration, overall session metrics, and WASM initialization latency. Exports baseline JSON to artifacts/ for tracking progress toward Epic #771's ~70% runtime reduction goal. Integrates profiling artifact preservation into CI/CD pipeline with 5-run median baseline of 93.16 seconds.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Test Profiling & Metrics Baseline (#776)
PR #870 Summary: Implement test profiling & metrics baseline
Repository: ikostan/SkyLockAssault
Author: @ikostan
Branch:
optimize-test-suite-runtime-and-prevent-ci-limit-exhaustion→mainLinked Issues: #776 ([TASK] Test Profiling & Metrics Baseline), Epic #771
Milestone: Milestone 22 – Optimize Test Suite Runtime & Fix Loading Screen
Labels: documentation, enhancement, web, testing, CI/CD, performance, refactoring, python, EPIC, QA
Purpose
Establish automated profiling infrastructure and a statistical baseline for the Playwright E2E browser test suite. Capture per-test execution duration, overall session metrics, and Godot WASM initialization latency, then export a baseline JSON artifact. This provides the measurable starting point for Epic #771’s goal of ~70% runtime reduction and prevents CI limit exhaustion.
Core Improvements
1. Pytest Profiling Hooks (
tests/conftest.py)pytest_sessionstart– records session start time and UTC timestamp.pytest_runtest_makereport(hookwrapper) – records per-test duration, outcome (aggregating setup/call/teardown phases), and optional WASM boot time.pytest_sessionfinish– computes total session duration, assembles metrics payload, writesartifacts/metrics_baseline.json, and cleans up tracked subprocesses.pytest_terminal_summary– prints a human-readable profiling baseline summary alongside existing browser memory metrics.2. WASM Initialization Latency Helpers (
tests/test_utils.py)init_page_and_wait_readyto accept an optionalrequest, measure time-to-ready withtime.perf_counter, attach boot duration torequest.node, and return the duration.navigate_and_profile_godot_wasm.start_game_and_wait_readyto forward the pytest request object so E2E setup tests automatically record boot time.3. CI Integration (
.github/workflows/browser_test.yml)metrics_baseline.jsonto a shard-specific artifact name usingmatrix.artifact_suffix.4. Local Scripts & Artifact Hygiene
workspace/run_browser_tests.shandworkspace/run_pipeline.shupdated for consistent artifact preservation, non-fatal venv activation, clearer cleanup, and validation of required Python/Playwright tools.artifacts/.5. Documentation & Benchmark
files/docs/milestones/22/Part_7_Test_Profiling_&_Metrics_Baseline.md6. Tests
tests/ci/test_metrics_baseline.py, updates totest_utils_test.py).Benefits
Baseline Result
5-run median suite runtime: 93.16 seconds (official starting point for optimization work).
🎯 Baseline Objectives & Key Performance Indicators (KPIs)
artifacts/metrics_baseline.json.📊 5-Run Profiling Benchmark Results
📈 Statistical Target Analysis
90.5947s104.5763s94.7907s93.1585s<= 27.95s🏆 Deliverables Checklist
pytest_sessionstart,pytest_runtest_makereport, andpytest_sessionfinishintests/conftest.py.artifacts/metrics_baseline.json.navigate_and_profile_godot_wasm()intests/test_utils.py.workspace/run_browser_tests.shandworkspace/run_pipeline.shto preserve profiling metrics..github/workflows/browser_test.ymlto preserve and upload sharded profiling baseline artifacts.93.16s).PR #870 Summary: Bots / AI Contributions
AI / Bot Contributors
@sourcery-ai
Generated the PR summary and Reviewer’s Guide. Performed code review with suggestions (including adding tests for WASM boot-timing helpers and aligning the metrics baseline path between pytest export and CI).
@coderabbitai
Generated the PR summary, walkthrough, and poem. Conducted code reviews with actionable feedback (e.g., aggregating pytest setup/call/teardown phases for accurate metrics, validating Python/Playwright availability in scripts, and improving artifact handling).
@deepsource-io
Performed automated DeepSource Code Review, published a PR Report Card (Security / Reliability / Complexity / Hygiene), and left multiple review comments across commits.
@deepsource-autofix
Authored multiple automated style/format commits (
style: format code with Black and isort) to enforce Black + isort consistency.@copilot (GitHub Copilot)
Co-authored the commit that added tests for the metrics baseline exporter.
Human Contributor
Primary author of the PR. Implemented the full test profiling & metrics baseline infrastructure (pytest hooks for per-test duration, session metrics, and Godot WASM initialization latency; export of
metrics_baseline.jsontoartifacts/; CI artifact preservation/upload across shards; updates totest_utils.py, shell scripts, and workflow). Authored the milestone documentation (including 5-run benchmark results with a median of 93.16 s), added supporting tests, and iteratively addressed review feedback.What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Establish a profiling and metrics baseline for the Playwright E2E browser test suite and integrate its artifacts into the CI pipeline.
New Features:
Enhancements:
CI:
Documentation:
Summary by Sourcery
Establish automated profiling and runtime baseline for the Playwright E2E test suite and integrate its artifacts into local workflows and CI.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests