Clean up Unity and Python tests #548
Conversation
…ale tests Python tests: - Delete 3 vestigial placeholder files (test_script_editing.py, test_find_in_file_minimal.py, test_resources_api.py) - Remove empty skip-marked tests from test_transport_framing.py and test_logging_stdout.py - Consolidate DummyMCP/setup_tools() into test_helpers.py - Fix flaky timing in test_telemetry_queue_worker.py (200ms → 500ms) - Remove placeholder from test_instance_routing_comprehensive.py C# tests: - Consolidate MCPToolParameterTests.cs (755 → 361 lines, -52%) - Fix 3 tests with stale "future feature" comments - features ARE implemented: - AutoGrow arrays (Phase 1.2) - Path normalization (Phase 1.1) - Bulk array mapping (Phase 3.1) - Strengthen assertions in ManageGameObjectTests.cs and CommandRegistryTests.cs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reviewer's GuideCleans up and strengthens both Python and Unity test suites by removing vestigial/skip-only tests, consolidating shared helpers, and updating several C# integration tests to assert the system’s current behavior (auto-grow arrays, path normalization, bulk array mapping, and tool handler error paths) while tightening telemetry timing and command registry expectations. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR centralizes test tool setup by adding Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 left some high level feedback:
- In
EndToEnd_PropertyHandling_AllScenarios, the texture-dependent scenarios (4/5/10) now silently skip iftexPathdoesn't exist; consider either creating the temp texture (as in the previous helper logic) or explicitly failing/inconclusive when the texture is missing so those behaviors don't accidentally go untested. - In
CommandRegistryTests.AutoDiscovery_RegistersAllBuiltInTools, calling each handler with an emptyJObjectonly asserts that it doesn't throw; you may want to additionally assert on the returned response shape (e.g., error vs success contract) so the test validates more than just handler existence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `EndToEnd_PropertyHandling_AllScenarios`, the texture-dependent scenarios (4/5/10) now silently skip if `texPath` doesn't exist; consider either creating the temp texture (as in the previous helper logic) or explicitly failing/inconclusive when the texture is missing so those behaviors don't accidentally go untested.
- In `CommandRegistryTests.AutoDiscovery_RegistersAllBuiltInTools`, calling each handler with an empty `JObject` only asserts that it doesn't throw; you may want to additionally assert on the returned response shape (e.g., error vs success contract) so the test validates more than just handler existence.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/tests/integration/test_edit_normalization_and_noop.py (1)
100-112: Remove unusedtools_structsetup.The
tools_structDummyMCP instance is populated (lines 102-112) but never used in the test. The test only usesapply_textfromtools_text. This appears to be leftover code from the refactoring.Proposed fix
async def test_atomic_multi_span_and_relaxed(monkeypatch): tools_text = setup_script_tools() apply_text = tools_text["apply_text_edits"] - tools_struct = DummyMCP() - # Import the tools module to trigger decorator registration - import services.tools.script_apply_edits - # Get the registered tools from the registry - from services.registry import get_registered_tools - tools = get_registered_tools() - # Add all script-related tools to our dummy MCP - for tool_info in tools: - tool_name = tool_info['name'] - if any(keyword in tool_name for keyword in ['script_apply', 'apply_edits']): - tools_struct.tools[tool_name] = tool_info['func'] # Fake send for read and write; verify atomic applyMode and validate=relaxed passes through sent = {}If
tools_structis indeed unused, also removeDummyMCPfrom the import on line 3.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (1)
337-344: Test 10 usestexPathwithout verifying texture exists.Tests 4 and 5 (lines 225, 239) correctly check if the texture exists before using
texPath. However, test 10 assignstexPathto["albedo"]without this guard. If the texture doesn't exist, the test may fail or produce inconsistent results.🐛 Proposed fix - add existence check or remove texture assignment
// 10. Mixed friendly and alias keys in one go var modify7 = new JObject { ["action"] = "modify", ["path"] = matPath, ["properties"] = new JObject { ["metallic"] = 0.8, - ["smoothness"] = 0.3, - ["albedo"] = texPath + ["smoothness"] = 0.3 } }; + // Optionally add albedo only if texture exists + if (AssetDatabase.LoadAssetAtPath<Texture>(texPath) != null) + ((JObject)modify7["properties"])["albedo"] = texPath;
🤖 Fix all issues with AI agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs:
- Around line 53-68: The TearDown method currently deletes TempDir, TempLiveDir
and "Assets/Temp" but does not call AssetDatabase.Refresh(), which can leave the
Unity asset database in a stale state for following tests; update the TearDown
(method TearDown in MCPToolParameterTests) to call AssetDatabase.Refresh() after
performing the deletions so the AssetDatabase reflects the removals (use
AssetDatabase.Refresh() or AssetDatabase.Refresh(ImportAssetOptions.Default) as
your project convention).
🧹 Nitpick comments (2)
Server/tests/integration/test_helpers.py (1)
64-68: Consider prefixing unused parameters to silence linter.The
argsandkwargsare intentionally unused for API compatibility with the real MCP decorator, but static analysis flags them. Consider using underscore prefixes.Proposed fix
- def tool(self, *args, **kwargs): + def tool(self, *_args, **_kwargs): def deco(fn): self.tools[fn.__name__] = fn return fn return decoTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (1)
43-51: Consider using[SetUp]for folder initialization.The
EnsureTempFolders()helper is manually called at the start of each test. For consistency with typical NUnit patterns and to ensure folders are always created before tests run, consider moving this to a[SetUp]method.♻️ Suggested refactor
+ [SetUp] + public void SetUp() + { + EnsureTempFolders(); + } + private static void EnsureTempFolders() { // ... existing implementation }Then remove the
EnsureTempFolders()calls from lines 77 and 151.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Server/tests/integration/test_edit_normalization_and_noop.pyServer/tests/integration/test_edit_strict_and_warnings.pyServer/tests/integration/test_find_in_file_minimal.pyServer/tests/integration/test_get_sha.pyServer/tests/integration/test_helpers.pyServer/tests/integration/test_instance_routing_comprehensive.pyServer/tests/integration/test_logging_stdout.pyServer/tests/integration/test_resources_api.pyServer/tests/integration/test_script_editing.pyServer/tests/integration/test_telemetry_queue_worker.pyServer/tests/integration/test_transport_framing.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs
💤 Files with no reviewable changes (6)
- Server/tests/integration/test_logging_stdout.py
- Server/tests/integration/test_instance_routing_comprehensive.py
- Server/tests/integration/test_find_in_file_minimal.py
- Server/tests/integration/test_resources_api.py
- Server/tests/integration/test_transport_framing.py
- Server/tests/integration/test_script_editing.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
🧬 Code graph analysis (5)
Server/tests/integration/test_get_sha.py (1)
Server/tests/integration/test_helpers.py (2)
DummyContext(16-55)setup_script_tools(71-88)
Server/tests/integration/test_edit_strict_and_warnings.py (1)
Server/tests/integration/test_helpers.py (2)
DummyContext(16-55)setup_script_tools(71-88)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
TearDown(24-32)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs (1)
TearDown(63-84)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.cs (2)
TearDown(38-61)Test(135-154)
Server/tests/integration/test_edit_normalization_and_noop.py (1)
Server/tests/integration/test_helpers.py (3)
DummyContext(16-55)DummyMCP(58-68)setup_script_tools(71-88)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs (2)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
HandleCommand(40-75)TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs (1)
JObject(21-25)
🪛 Ruff (0.14.10)
Server/tests/integration/test_helpers.py
64-64: Unused method argument: args
(ARG002)
64-64: Unused method argument: kwargs
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (22)
Server/tests/integration/test_telemetry_queue_worker.py (1)
37-40: LGTM! Reasonable threshold adjustment for CI stability.The increase from 200ms to 500ms is pragmatic—50 non-blocking calls should still complete well under this limit (microseconds each), while the added headroom accommodates CI load variability. The comment clearly documents the rationale.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
34-44: LGTM! Assertions properly strengthened.The test now verifies both the return type (
ErrorResponse) and its state (Success == false), which is more robust than only checking for non-null. This pattern is consistent with similar tests later in the file (e.g.,HandleCommand_WithPrefabPath_ReturnsGuidanceError_*).
46-57: LGTM! Consistent with the null params test.The parallel strengthening of both error tests improves test coverage by verifying the expected response type and failure state. The comment on line 53 helpfully documents that the error is due to missing required action.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs (1)
46-52: LGTM! Strengthened test now verifies handler callability.The updated assertions properly verify that each handler is:
- Registered and retrievable (not null)
- Callable without throwing exceptions
- Returns a non-null result for empty params
This is a meaningful improvement over just checking handler existence.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectStressTests.cs (4)
166-212: LGTM! Test correctly updated for implemented auto-grow feature.The test name and assertions now properly reflect that auto-grow arrays is an implemented feature. The verification steps are thorough: checking patch success, array length (
>= 100), and the value at index 99.
219-268: LGTM! Test correctly updated for implemented path normalization.The test now properly verifies that friendly bracket notation (
floatArray[2]) is normalized and works correctly. The added verification step (lines 262-265) that loads the asset and checks the value adds valuable end-to-end validation.
578-626: LGTM! Test correctly updated for implemented bulk array mapping.The test name and assertions now properly reflect that bulk array mapping is implemented. Good use of
CollectionAssert.AreEqual(line 623) to verify array contents match exactly, providing stronger validation than element-by-element checks.
630-630: No concerns with region header formatting.Also applies to: 679-679
Server/tests/integration/test_edit_normalization_and_noop.py (3)
3-3: LGTM on the import consolidation.The refactoring to use centralized
setup_script_toolsfromtest_helpersimproves maintainability and reduces code duplication across test files.
7-9: Correct usage of the new helper.The test correctly obtains tools via
setup_script_tools()and accesses theapply_text_editsfunction for testing normalization behavior.
66-69: LGTM.Straightforward refactoring to use the centralized helper.
Server/tests/integration/test_helpers.py (3)
1-14: LGTM on_DummyMetaimplementation.The class correctly provides attribute-style access to dict items with proper exception handling, and includes the
model_extraproperty andmodel_dumpmethod to mimic Pydantic model behavior.
16-55: Well-designed mock context.The
DummyContextclass properly:
- Generates unique session IDs to prevent test state leakage
- Provides async log methods matching the real context API
- Includes
warn()as an alias forwarning()for compatibility- Mimics FastMCP state management with
get_state/set_state
71-88: LGTM on centralized tool setup.The function correctly:
- Imports the tools module to trigger decorator-based registration
- Filters for script-related tools by name pattern
- Returns a dict of tool functions for test use
One minor note: the
mcp = DummyMCP()instance only uses itstoolsdict as a container. This is fine for consistency with the pattern, but a plain dict would also work.Server/tests/integration/test_edit_strict_and_warnings.py (3)
3-3: Clean import consolidation.Correctly imports only the needed helpers:
DummyContextandsetup_script_tools.
6-38: LGTM on test refactoring.The test correctly uses the centralized
setup_script_tools()helper while maintaining the original test logic for zero-based normalization warnings.
41-66: LGTM.Correctly refactored to use centralized helpers.
Server/tests/integration/test_get_sha.py (2)
3-3: Clean import consolidation.Correctly imports only the needed helpers from
test_helpers.
6-33: LGTM on test refactoring.The test correctly:
- Uses the centralized
setup_script_tools()helper- Verifies command routing to
manage_scriptwithaction=get_sha- Validates parameter extraction from the URI
- Confirms response filtering to only
sha256andlengthBytesTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (3)
74-133: LGTM!The test follows a clear naming convention, properly tests the JSON componentProperties coercion path, and includes comprehensive cleanup in the
finallyblock withAssetDatabase.Refresh().
148-329: LGTM - Comprehensive end-to-end test consolidation.The 10 scenarios cover diverse property handling paths with clear documentation. The use of
LogAssert.Expectfor invalid JSON handling (test 8) and tolerance-based color assertions (test 9) are appropriate.
349-358: LGTM!The
finallyblock properly cleans up all created resources (GameObject, Material, badJson asset) and callsAssetDatabase.Refresh()to ensure a clean state.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (1)
332-333: Consider usingAssertColorsEqualfor consistency.This inline tolerance check (0.02f) differs from the
AssertColorsEqualhelper (0.001f). If the wider tolerance is intentional due to shader switching precision, consider adding a comment explaining this, or extend the helper to accept a custom tolerance parameter.♻️ Suggested refactor
- Assert.IsTrue(Mathf.Abs(c9.r - 1f) < 0.02f && Mathf.Abs(c9.g - 1f) < 0.02f && Mathf.Abs(c9.b - 0f) < 0.02f, - "Test 9: Color should be near yellow"); + // Shader switching may introduce minor color precision loss + AssertColorsEqual(Color.yellow, c9, "Test 9: Color should be yellow");Or if the wider tolerance is required:
private static void AssertColorsEqual(Color expected, Color actual, string message, float tolerance = 0.001f)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/tests/integration/test_edit_normalization_and_noop.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/tests/integration/test_edit_normalization_and_noop.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (3)
70-133: LGTM!The test is well-structured with proper setup, assertions, and cleanup in the finally block. The material assignment verification through JSON string coercion is thoroughly validated.
169-177: Good addition of explicit texture creation.Creating the test texture programmatically ensures texture-dependent scenarios (4, 5, 10) run reliably without depending on external assets. This addresses the commit message objective of ensuring texture-dependent scenarios run properly.
43-51: Nice centralization of temp folder creation.The
EnsureTempFoldershelper reduces duplication and ensures consistent test directory setup. The idempotent checks viaIsValidFolderprevent errors on repeated calls.
- Remove unused tools_struct setup in test_edit_normalization_and_noop.py - Create test texture in EndToEnd_PropertyHandling_AllScenarios so texture-dependent scenarios (4, 5, 10) actually run instead of silently skipping - Add texture cleanup in finally block Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dsarno
force-pushed
the
test-audit-cleanup
branch
from
62db5bf to
405c218
Compare
1 participant
Removes placeholder tests, consolidates duplicates, and fixes tests with stale comments.
Changes
Python (-502 lines)
C# (-154 lines)
Results
Summary by Sourcery
Clean up and harden Unity and Python integration tests by removing vestigial cases, consolidating helpers, and strengthening behavioral assertions.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.