test: Consolidate pytest suite to MCPForUnity and improve test infrastructure #332
Conversation
…om tests; route tools.manage_script via unity_connection for reliable monkeypatching; fix ctx usage; all tests green (39 pass, 5 skip, 7 xpass)
…lidate_script; tests: tidy manage_script_uri unused symbols and arg names
…ests/test_helpers and import; add telemetry stubs in tests to avoid pyproject I/O
…config and opt-in env override; test stubs and CWD fixes; exclude Bridge from pytest discovery
…ore hook); telemetry: validate config endpoint; read_console: action optional
- Add [project.optional-dependencies] section with dev group - Include pytest>=8.0.0 and pytest-anyio>=0.6.0 - Add Development Setup section to README-DEV.md with installation and testing instructions
This reverts commit 8ae595d.
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR refactors module imports across the codebase, changing direct function imports to module-level imports for better organization. It updates test infrastructure to use dynamic tool registration from a registry instead of decorator-based registration, introduces a DummyContext test helper for mocking, modifies endpoint selection logic in telemetry, and adds test collection safeguards to prevent duplicate test discovery across mirrored source directories. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve consistent refactoring patterns across multiple test files (homogeneous import and patching updates) combined with moderate complexity in telemetry logic changes and test infrastructure updates. While the spread is broad (20+ files), the repetitive nature of test refactors reduces per-file review burden. The production code changes (import strategy, endpoint logic) require focused analysis but are relatively localized. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates the pytest test suite to exclusively target the MCPForUnity package, removing legacy UnityMcpBridge references and significantly improving test infrastructure. The refactoring ensures all 51 tests execute successfully with proper import handling, shared test helpers, and reliable monkeypatching through unity_connection.
Key changes include:
- Refactoring import mechanisms from deprecated package-style imports to direct module imports with proper sys.path management
- Fixing bugs including missing
unity_connectionprefix invalidate_scriptand makingactionparameter optional inread_console - Adding development dependencies to
pyproject.tomland comprehensive setup instructions toREADME-DEV.md
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_validate_script_summary.py |
Migrated to registry-based tool registration and proper unity_connection patching |
tests/test_telemetry_subaction.py |
Replaced package-style import with manual module loading and telemetry stubs |
tests/test_telemetry_queue_worker.py |
Added telemetry stub and working directory management for reliable module loading |
tests/test_telemetry_endpoint_validation.py |
Refactored to use direct imports with sys.path injection and proper working directory |
tests/test_script_tools.py |
Converted to registry-based registration and unity_connection patching |
tests/test_resources_api.py |
Added telemetry stubs and migrated to registry-based tool setup |
tests/test_read_resource_minimal.py |
Added DummyContext and registry-based tool registration |
tests/test_read_console_truncate.py |
Updated to patch tools.read_console directly and added action parameter |
tests/test_manage_script_uri.py |
Refactored to use registry-based registration and unity_connection patching |
tests/test_improved_anchor_matching.py |
Updated module name from manage_script_edits to script_apply_edits |
tests/test_helpers.py |
New shared test helper containing DummyContext class |
tests/test_get_sha.py |
Migrated to registry-based registration and unity_connection patching |
tests/test_find_in_file_minimal.py |
Added telemetry stubs and DummyContext import |
tests/test_edit_strict_and_warnings.py |
Added telemetry stubs and refactored to registry-based registration |
tests/test_edit_normalization_and_noop.py |
Updated module reference and migrated to registry-based registration |
tests/conftest.py |
Added pytest collection ignores to prevent duplicate package imports |
pytest.ini |
New configuration file defining test paths and exclusion rules |
docs/README-DEV.md |
Added development setup instructions including dependency installation |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py |
Changed import from direct function to module-based for unity_connection |
UnityMcpBridge/UnityMcpServer~/src/conftest.py |
Added collection filter to avoid import conflicts |
MCPForUnity/UnityMcpServer~/src/tools/read_console.py |
Made action parameter optional with default value |
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py |
Changed import from direct function to module-based for unity_connection |
MCPForUnity/UnityMcpServer~/src/telemetry.py |
Updated config module resolution order and environment override logic |
MCPForUnity/UnityMcpServer~/src/pyproject.toml |
Added optional dev dependencies section |
.github/workflows/github-repo-stats.yml |
Commented out scheduled workflow trigger |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| collector.record(telemetry.RecordType.TOOL_EXECUTION, {"i": i}) | ||
| elapsed_ms = (time.perf_counter() - start) * 1000.0 | ||
|
|
||
| # Should be fast despite backpressure (non-blocking enqueue or drop) |
There was a problem hiding this comment.
The timeout assertion was increased from 80ms to 200ms (a 150% increase). This suggests potential test flakiness or actual performance degradation. Consider investigating the root cause rather than simply relaxing the timeout threshold. If the backpressure test legitimately needs more time, document why this tolerance is appropriate.
| # Should be fast despite backpressure (non-blocking enqueue or drop) | |
| # Should be fast despite backpressure (non-blocking enqueue or drop) | |
| # NOTE: The timeout threshold was increased from 80ms to 200ms due to observed test flakiness | |
| # in CI and local environments, likely caused by thread scheduling variability and system load. | |
| # If this test fails or performance degrades further, investigate the root cause (e.g., worker | |
| # thread delays, queue contention, or changes in telemetry implementation) before increasing | |
| # the threshold again. The 200ms value is chosen to balance reliability and detection of regressions. |
There was a problem hiding this comment.
Comment added to explain
| "config", | ||
| "src.config", | ||
| "MCPForUnity.UnityMcpServer~.src.config", | ||
| "MCPForUnity.UnityMcpServer.src.config", |
There was a problem hiding this comment.
The module resolution order change is significant but lacks explanation. Add a comment explaining why plain 'config' is now preferred over package-qualified paths, especially regarding the test-time override behavior mentioned in the comment.
There was a problem hiding this comment.
Comment added to explain
- Add explanation for 200ms timeout in backpressure test - Replace manual event loop creation with asyncio.run() - Add assertion message with actual elapsed time for easier debugging
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
tests/test_edit_strict_and_warnings.py (1)
13-32: Telemetry stub duplication noted.This is the same telemetry stub pattern duplicated across multiple test files. See the earlier comment on
test_find_in_file_minimal.pyrecommending extraction to a shared fixture.
🧹 Nitpick comments (3)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
1-4: Suppress the unused parameter warning.The
configparameter is required by pytest's hook signature. Consider prefixing it with an underscore to indicate it's intentionally unused and silence the static analysis warning.Apply this diff:
-def pytest_ignore_collect(path, config): +def pytest_ignore_collect(path, _config): # Avoid duplicate import mismatches between Bridge and MCPForUnity copies p = str(path) return p.endswith("test_telemetry.py")tests/test_telemetry_endpoint_validation.py (1)
10-18: Extract duplicated path setup into a fixture or module-level constant.The path computation and sys.path manipulation is duplicated across all three test functions (lines 10-18, 33-39, 64-72). This should be extracted to avoid repetition and improve maintainability.
Consider extracting to module level:
+import sys +import pathlib +import importlib +import os + +ROOT = pathlib.Path(__file__).resolve().parents[1] +SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" +sys.path.insert(0, str(SRC)) + def test_endpoint_rejects_non_http(tmp_path, monkeypatch): # Point data dir to temp to avoid touching real files monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path)) monkeypatch.setenv("UNITY_MCP_TELEMETRY_ENDPOINT", "file:///etc/passwd") - # Import the telemetry module from the correct path - import sys - import pathlib - ROOT = pathlib.Path(__file__).resolve().parents[1] - SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" - sys.path.insert(0, str(SRC)) - monkeypatch.chdir(str(SRC)) telemetry = importlib.import_module("telemetry") importlib.reload(telemetry)tests/test_find_in_file_minimal.py (1)
14-33: Consider extracting telemetry stub setup into a shared fixture.This telemetry stub pattern is duplicated across multiple test files (e.g.,
test_edit_strict_and_warnings.py, this file). Consider extracting it toconftest.pyor a shared test utilities module.Example approach in
tests/conftest.py:@pytest.fixture(autouse=True) def stub_telemetry(): """Stub telemetry modules to avoid file I/O during imports.""" import sys import types telemetry = types.ModuleType("telemetry") def _noop(*args, **kwargs): pass class MilestoneType: pass telemetry.record_resource_usage = _noop telemetry.record_tool_usage = _noop telemetry.record_milestone = _noop telemetry.MilestoneType = MilestoneType telemetry.get_package_version = lambda: "0.0.0" sys.modules.setdefault("telemetry", telemetry) telemetry_decorator = types.ModuleType("telemetry_decorator") def telemetry_tool(*dargs, **dkwargs): def _wrap(fn): return fn return _wrap telemetry_decorator.telemetry_tool = telemetry_tool sys.modules.setdefault("telemetry_decorator", telemetry_decorator)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/github-repo-stats.yml(1 hunks).gitignore(1 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/telemetry.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py(9 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(1 hunks)TestProjects/UnityMCPTests/Assets/Temp.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs.meta(1 hunks)UnityMcpBridge/UnityMcpServer~/src/conftest.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(9 hunks)docs/README-DEV.md(1 hunks)pytest.ini(1 hunks)tests/conftest.py(1 hunks)tests/test_edit_normalization_and_noop.py(7 hunks)tests/test_edit_strict_and_warnings.py(4 hunks)tests/test_find_in_file_minimal.py(3 hunks)tests/test_get_sha.py(2 hunks)tests/test_helpers.py(1 hunks)tests/test_improved_anchor_matching.py(4 hunks)tests/test_manage_script_uri.py(3 hunks)tests/test_read_console_truncate.py(3 hunks)tests/test_read_resource_minimal.py(2 hunks)tests/test_resources_api.py(4 hunks)tests/test_script_tools.py(6 hunks)tests/test_telemetry_endpoint_validation.py(3 hunks)tests/test_telemetry_queue_worker.py(4 hunks)tests/test_telemetry_subaction.py(1 hunks)tests/test_validate_script_summary.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
tests/conftest.py (1)
pytest_ignore_collect(20-28)
tests/test_find_in_file_minimal.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
record_resource_usage(454-472)
tests/conftest.py (1)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
pytest_ignore_collect(1-4)
tests/test_improved_anchor_matching.py (1)
tests/test_script_tools.py (1)
load_module(32-36)
tests/test_validate_script_summary.py (3)
tests/test_manage_script_uri.py (5)
DummyContext(60-69)info(62-63)warning(65-66)error(68-69)DummyMCP(46-54)tests/test_edit_normalization_and_noop.py (6)
DummyContext(50-59)info(52-53)warning(55-56)error(58-59)setup_tools(62-74)DummyMCP(42-47)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(443-500)validate_script(409-439)
tests/test_script_tools.py (3)
tests/test_manage_script_uri.py (3)
info(62-63)error(68-69)DummyMCP(46-54)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(15-83)
tests/test_telemetry_queue_worker.py (2)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
get_package_version(39-46)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
get_package_version(37-44)
tests/test_get_sha.py (2)
tests/test_manage_script_uri.py (3)
info(62-63)error(68-69)DummyMCP(46-54)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(443-500)get_sha(536-552)
tests/test_telemetry_subaction.py (1)
MCPForUnity/UnityMcpServer~/src/telemetry.py (4)
MilestoneType(65-73)record_resource_usage(454-472)record_tool_usage(425-451)get_package_version(39-46)
tests/test_read_console_truncate.py (2)
tests/test_manage_script_uri.py (4)
info(62-63)warning(65-66)error(68-69)DummyMCP(46-54)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
read_console(14-87)
tests/test_resources_api.py (4)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
get_package_version(39-46)MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
list_resources(137-186)
tests/test_edit_normalization_and_noop.py (3)
tests/test_manage_script_uri.py (3)
info(62-63)error(68-69)DummyMCP(46-54)tests/test_get_sha.py (4)
info(54-55)error(60-61)setup_tools(64-76)DummyMCP(41-49)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)
tests/test_read_resource_minimal.py (4)
tests/test_manage_script_uri.py (5)
DummyContext(60-69)info(62-63)warning(65-66)error(68-69)DummyMCP(46-54)tests/test_edit_normalization_and_noop.py (5)
DummyContext(50-59)info(52-53)warning(55-56)error(58-59)DummyMCP(42-47)tests/test_resources_api.py (2)
resource_tools(48-60)DummyMCP(33-41)tests/test_find_in_file_minimal.py (2)
resource_tools(48-60)DummyMCP(36-44)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
tests/test_manage_script_uri.py (2)
tests/test_edit_normalization_and_noop.py (5)
DummyContext(50-59)info(52-53)warning(55-56)error(58-59)DummyMCP(42-47)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)
tests/test_telemetry_endpoint_validation.py (1)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
TelemetryCollector(212-398)
tests/test_edit_strict_and_warnings.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)
🪛 Ruff (0.14.1)
UnityMcpBridge/UnityMcpServer~/src/conftest.py
1-1: Unused function argument: config
(ARG001)
tests/test_find_in_file_minimal.py
28-28: Unused function argument: dargs
(ARG001)
28-28: Unused function argument: dkwargs
(ARG001)
tests/test_edit_strict_and_warnings.py
27-27: Unused function argument: dargs
(ARG001)
27-27: Unused function argument: dkwargs
(ARG001)
🔇 Additional comments (28)
.github/workflows/github-repo-stats.yml (1)
4-7: Clarify why the scheduled workflow trigger was disabled.This change disables the automatic daily stats collection and transitions the workflow to manual-only mode (
workflow_dispatch). However, this change appears unrelated to the stated PR objectives (consolidating the pytest suite and improving test infrastructure).Is this intentional? If so, please document the rationale in the PR description or as a separate change. If this was accidental, consider reverting it or moving it to a dedicated PR.
.gitignore (1)
42-43: LGTM!The addition of
.wt-origin-main/fits naturally with the existing tool artifact patterns (e.g.,.windsurf,.cursorignore) and cleanly ignores local development directory artifacts.TestProjects/UnityMCPTests/Assets/Temp.meta (1)
1-8: Standard Unity asset metadata.These are auto-generated Unity metadata files with proper YAML formatting and valid structure. No issues identified.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs.meta (1)
1-11: Standard Unity asset metadata.These are auto-generated Unity metadata files with proper YAML formatting and valid structure. No issues identified.
docs/README-DEV.md (1)
8-37: LGTM! Clear and accurate development setup documentation.The instructions for installing dev dependencies and running tests are correct and align well with the pyproject.toml changes in this PR.
MCPForUnity/UnityMcpServer~/src/pyproject.toml (1)
14-18: LGTM! Standard dev dependencies configuration.The optional dev dependencies are properly configured and align with the documentation changes in README-DEV.md.
MCPForUnity/UnityMcpServer~/src/telemetry.py (2)
96-98: Good addition for test-time flexibility.Prioritizing plain "config" and "src.config" modules enables test-time sys.path injection and module overrides, which aligns well with the test infrastructure improvements in this PR.
120-126: No backward compatibility concern – behavior is identical.The review comment's concern about empty string handling is not founded. Both the old and new code produce the same outcome when
UNITY_MCP_TELEMETRY_ENDPOINTis set to an empty string:
- Old code:
os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT", default_ep)would pass""to_validated_endpoint(), which would fail thenetloccheck and fall back todefault_ep.- New code: Explicitly checks
if env_ep != ""and also falls back todefault_ep.Result: identical behavior. Additionally, no code in the codebase explicitly sets this environment variable to an empty string, and the test suite contains no tests for this scenario, indicating it's not a deployed pattern.
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
16-16: LGTM! Backward-compatible parameter defaulting.Making the
actionparameter optional with a sensible default ('get') improves the API ergonomics while maintaining full backward compatibility.pytest.ini (1)
1-4: LGTM! Proper test collection configuration.The exclusions prevent duplicate imports and collection conflicts between Bridge and MCPForUnity copies, which aligns well with the pytest hooks in the conftest.py files.
tests/test_improved_anchor_matching.py (1)
41-42: LGTM! Consistent module rename.The module reference has been updated from
manage_script_edits_moduletoscript_apply_edits_moduleand all usages are consistently updated throughout the test file.tests/test_helpers.py (1)
1-10: LGTM! Clean test helper extraction.Centralizing the
DummyContextmock in a shared test helper improves test maintainability and reduces code duplication across the test suite.MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (2)
9-9: LGTM: Import change enables better test isolation.The change from direct function import to module import allows tests to patch
unity_connection.send_command_with_retryat the module level, improving test reliability and maintainability.
106-110: LGTM: Consistent usage of module-level RPC calls.All call sites correctly updated to use
unity_connection.send_command_with_retry(...)instead of the direct import. The change preserves all parameters and behavior while enabling module-level patching in tests.Also applies to: 307-307, 339-344, 389-390, 404-405, 429-429, 476-476, 544-545
tests/test_telemetry_queue_worker.py (2)
33-52: LGTM: Telemetry stub prevents file I/O during test setup.The stub approach ensures
get_package_versiondoesn't attempt to readpyproject.tomlduring module loading, improving test isolation and speed.
83-83: Timing assertion adjustment is reasonable.The relaxation from 80ms to 200ms accounts for the additional overhead introduced by the stubbing and dynamic module loading infrastructure. The test still validates non-blocking behavior.
tests/test_find_in_file_minimal.py (2)
8-8: LGTM: Proper reuse of shared test helper.Good use of the centralized
DummyContextfromtest_helpers.pyinstead of defining it locally.
50-59: LGTM: Dynamic tool registration via registry.The switch to registry-based tool discovery aligns with the PR's infrastructure improvements and enables better test isolation.
tests/test_validate_script_summary.py (2)
65-75: LGTM: Registry-based tool population.The dynamic tool registration approach correctly aligns with the PR's infrastructure improvements and enables better test isolation.
95-98: LGTM: Module-level patching strategy.Patching
unity_connection.send_command_with_retryat the module level (rather than patching individual tool references) is the correct approach that aligns with the production code changes inmanage_script.py.tests/test_edit_strict_and_warnings.py (3)
6-7: LGTM: Proper import of shared test helper.Good use of the centralized
DummyContextfromtest_helpers.py.
70-77: LGTM: Consistent registry-based tool discovery.The dynamic tool registration pattern is correctly implemented and aligns with the PR's test infrastructure improvements.
88-89: LGTM: Correct module-level patching.Patching
unity_connection.send_command_with_retryenables reliable monkeypatching as intended by the production code changes.Also applies to: 113-114
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
9-9: The review comment's premise is incorrect. Both UnityMcpBridge and MCPForUnity directories are maintained in parallel and independently.The script results confirm:
manage_script.pyexists in both/UnityMcpBridge/and/MCPForUnity/as separate implementations- Both directories are treated equally in test collection (both ignored together)
- No cross-imports exist between them; each is self-contained
- "Deprecated" references in UnityMcpBridge files refer to specific functionality (e.g., line 442 describes the tool as a "compatibility router for legacy script operations"), not directory-level deprecation
The PR description "removes deprecated UnityMcpBridge paths" is misleading. It likely means removing deprecated functionality within UnityMcpBridge (like legacy script operations), not removing the UnityMcpBridge directory itself. Updating
manage_script.pyin that directory is appropriate if the goal is to refactor or clean up its deprecated features while keeping both implementations maintained.Likely an incorrect or invalid review comment.
tests/test_resources_api.py (2)
44-44: LGTM: Correct usage of shared test helper.This file correctly imports
DummyContextfromtests.test_helpers, which is the intended pattern for sharing test utilities across the test suite.
83-90: LGTM: Proper event loop management.Creating a dedicated event loop per test call and ensuring cleanup in the finally block is good practice for async testing isolation.
tests/test_telemetry_subaction.py (1)
4-43: LGTM: Isolated telemetry decorator testing.The dynamic import setup with minimal stubs is appropriate for testing telemetry_decorator in isolation without file I/O dependencies. The attribute guards at lines 33-42 ensure monkeypatch targets exist even if not exported, which prevents test failures.
tests/conftest.py (1)
9-28: LGTM: Robust test collection configuration.The combination of
collect_ignore,collect_ignore_glob, andpytest_ignore_collectprovides comprehensive exclusion of src directories to prevent duplicate-package import conflicts. Path normalization at line 22 handles cross-platform differences correctly.
Replace 7 duplicate DummyContext class definitions with imports from tests.test_helpers. This follows DRY principles and ensures consistency across the test suite.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_telemetry_queue_worker.py (1)
82-84: Timeout relaxation partially addresses past review concerns.The updated comment on line 83 acknowledging "thread scheduling variance in CI/local environments" is an improvement over the previous version. However, the 150% increase in timeout (80ms → 200ms) is still significant.
As noted in the past review, consider:
- Was this increase based on observed test failures in CI, or is it a proactive buffer?
- Could there be actual performance degradation that should be investigated rather than hidden by a larger timeout?
- The past review suggested documenting the specific conditions that justify this threshold
If this increase resolves observed flakiness, consider adding that context (e.g., "Based on 10 CI runs, max observed time was 150ms").
🧹 Nitpick comments (1)
tests/test_telemetry_queue_worker.py (1)
33-36: Improve documentation for the intentional but confusing module stubbing strategy.The pattern is now clear: the stub prevents normal imports while the real module is loaded separately with CWD manipulation (to support
get_package_version()'s relative path topyproject.toml, which is called at module import time).However, the current comments are misleading. Line 46's comment "it will reuse stubbed helpers" is unclear—the real module defines its own
get_package_version(), not reusing the stub. The stub's purpose (isolating the real module from normal import paths) isn't obvious from the code.Recommendation:
Replace the comments on lines 33–36 and 46 to clearly explain this isolation pattern and why it's necessary. For example:
- Lines 33–36: Document that the stub prevents normal imports of telemetry (avoiding side effects)
- Lines 46–52: Explain that CWD manipulation is required because the real telemetry module calls
get_package_version()at import time, which openspyproject.tomlvia a relative pathAlternatively, consider simplifying by mocking only the necessary parts (e.g.,
get_package_version) where it's actually used, rather than pre-registering a stub module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_resources_api.py(4 hunks)tests/test_telemetry_queue_worker.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_telemetry_queue_worker.py (1)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
get_package_version(39-46)
tests/test_resources_api.py (4)
MCPForUnity/UnityMcpServer~/src/telemetry.py (6)
MilestoneType(65-73)record_resource_usage(454-472)record_tool_usage(425-451)record_milestone(271-293)record_milestone(420-422)get_package_version(39-46)MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
list_resources(137-186)
🔇 Additional comments (5)
tests/test_resources_api.py (5)
8-9: LGTM! Path consolidation aligns with PR objectives.The SRC path update to MCPForUnity correctly reflects the consolidation effort mentioned in the PR summary, removing deprecated UnityMcpBridge references.
11-31: Well-designed test isolation.The telemetry stub modules effectively prevent file I/O during tests while providing all necessary no-op implementations. This aligns with the PR's goal of making telemetry opt-in with explicit configuration.
44-44: Good practice: module-level import for test helper.Moving the DummyContext import to the top level improves code organization and aligns with the PR's effort to extract common test utilities.
47-60: LGTM! Registry-based tool discovery implemented correctly.The fixture refactor successfully adopts the dynamic registry pattern mentioned in the PR summary. The filtering logic appropriately identifies resource-related tools, and the implementation is consistent with similar patterns in other test files.
83-86: Correct modernization:asyncio.run()is the recommended approach.The change from
asyncio.get_event_loop().run_until_complete()toasyncio.run()is actually the correct modern Python practice (Python 3.7+). Whileasyncio.run()creates a new event loop for each call, this provides clean isolation between test cases and prevents loop state pollution—which is exactly what you want in tests. The overhead is negligible and the benefits (simpler code, better isolation) outweigh any performance concerns.Also applies to: 99-101
| # Load real telemetry on top of stub (it will reuse stubbed helpers) | ||
| _prev_cwd = os.getcwd() | ||
| os.chdir(str(SRC)) | ||
| try: | ||
| telemetry = _load_module(SRC / "telemetry.py", "telemetry_mod") | ||
| finally: | ||
| os.chdir(_prev_cwd) |
There was a problem hiding this comment.
Avoid CWD manipulation in test setup; consider mocking or fixing the source.
Changing the current working directory to load the telemetry module is fragile and can cause issues:
- Side effects: If tests run concurrently or if other code depends on CWD, this can cause flakiness
- Design smell: The need for CWD manipulation indicates that
get_package_version()in telemetry.py uses a relative path to openpyproject.toml, which is not robust - Maintenance burden: Future changes to telemetry module imports or dependencies may break unexpectedly
Recommendations:
- Preferred: Mock
get_package_versiondirectly to return a test value without file I/O - Fix the source code to use
__file__or accept a base directory parameter for locatingpyproject.toml - If CWD change is unavoidable, document specifically why it's necessary and what breaks without it
Example mock approach:
-# Load real telemetry on top of stub (it will reuse stubbed helpers)
-_prev_cwd = os.getcwd()
-os.chdir(str(SRC))
-try:
- telemetry = _load_module(SRC / "telemetry.py", "telemetry_mod")
-finally:
- os.chdir(_prev_cwd)
+# Load real telemetry module
+telemetry = _load_module(SRC / "telemetry.py", "telemetry_mod")
+# Mock get_package_version to avoid file I/O dependency
+telemetry.get_package_version = lambda: "0.0.0"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_telemetry_queue_worker.py around lines 46-52, avoid changing the
process CWD to load telemetry.py; instead either mock get_package_version in the
test to return a deterministic value (use patch to replace
telemetry.get_package_version before importing/using the module) or change
telemetry.py to resolve pyproject.toml relative to its own file (use __file__ or
accept a base_dir parameter) and update the test to import normally; remove the
os.chdir/_prev_cwd block and replace it with the mock or source fix to eliminate
fragile CWD manipulation.
…s.py Dead code cleanup - function was no longer used after refactoring to dynamic tool registration.
Clarify why os.chdir() is necessary: telemetry.py calls get_package_version() at module load time, which reads pyproject.toml using a relative path. Acknowledges the fragility while explaining why it's currently required.
1 participant
This PR consolidates the pytest test harness to focus exclusively on the
MCPForUnitypackage, removing deprecatedUnityMcpBridgepaths and significantly improving test reliability and maintainability. All 51 tests now execute successfully (39 passed, 5 skipped, 7 xpassed).The python server test infrastructure has been refactored to use proper imports and shared test helpers. Tests now correctly route through
unity_connectionfor reliable monkeypatching, and telemetry has been updated to prefer explicit configuration with opt-in environment overrides rather than automatic file I/O. Common test utilities likeDummyContexthave been extracted totests/test_helpers.pyfor reuse across the suite.Several bugs were fixed, including a missing
unity_connectionprefix invalidate_scriptand making theactionparameter optional inread_console. Thepyproject.tomlnow includes a[project.optional-dependencies]section withdevgroup containing pytest dependencies, andREADME-DEV.mdhas been updated with clear development setup instructions. This ensures contributors can quickly install dependencies withpip install -e .[dev]and run the full test suite.Stats: 27 files changed, 606 insertions, 140 deletions
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores