Optimize run tests #110
Optimize run tests #110
Conversation
…stering When registering with Claude Code, if a UnityMCP server already exists, remove it first before adding the new registration. This ensures the transport mode (HTTP vs stdio) is always updated to match the current UseHttpTransport EditorPref setting. Previously, if a stdio registration existed and the user tried to register with HTTP, the command would fail with 'already exists' and the old stdio configuration would remain unchanged.
…ctly
The validation code was incorrectly parsing the output of 'claude mcp get UnityMCP' by looking for JSON format ("transport": "http"), but the CLI actually returns human-readable text format ("Type: http"). This caused the transport mismatch detection to never trigger, allowing stdio to be selected in the UI while HTTP was registered with Claude Code.
Changes:
- Fix parsing logic to check for "Type: http" or "Type: stdio" in CLI output
- Add OnTransportChanged event to refresh client status when transport changes
- Wire up event handler to trigger client status refresh on transport dropdown change
This ensures that when the transport mode in Unity doesn't match what's registered with Claude Code, the UI will correctly show an error status with instructions to re-register.
…laude CLI status checks and HTTP simplifications
This commit resolves three issues with Claude Code registration: 1. UI blocking: Removed synchronous CheckStatus() call after registration that was blocking the editor. Status is now set immediately with async verification happening in the background. 2. Thread safety: Fixed "can only be called from the main thread" errors by capturing Application.dataPath and EditorPrefs.GetBool() on the main thread before spawning async status check tasks. 3. Transport mismatch detection: Transport mode changes now trigger immediate status checks to detect HTTP/stdio mismatches, instead of waiting for the 45-second refresh interval. The registration button now turns green immediately after successful registration without blocking, and properly detects transport mismatches when switching between HTTP and stdio modes.
Address code review feedback by making CheckStatusWithProjectDir thread-safe by design rather than by convention: 1. Made projectDir and useHttpTransport parameters non-nullable to prevent accidental background thread calls without captured values 2. Removed nullable fallback to EditorPrefs.GetBool() which would cause thread safety violations if called from background threads 3. Added ArgumentNullException for null projectDir instead of falling back to Application.dataPath (which is main-thread only) 4. Added XML documentation clearly stating threading contracts: - CheckStatus() must be called from main thread - CheckStatusWithProjectDir() is safe for background threads 5. Removed unreachable else branch in async status check code These changes make it impossible to misuse the API from background threads, with compile-time enforcement instead of runtime errors.
…-tasks Fix local HTTP server lifecycle and TCP connect observation
…y format
- Fix add_component to check top-level componentProperties when using
componentsToAdd as string array (e.g., ["Rigidbody"])
- Previously only worked with componentName or object array formats
- Makes API more consistent with modify action
- Add comprehensive unit tests for all three parameter formats
This allows the intuitive pattern:
{
"componentsToAdd": ["Rigidbody"],
"componentProperties": {"Rigidbody": {"mass": 5.0}}
}
… by 98% - Add includeFailedTests parameter: returns only failed/skipped test details - Add includeDetails parameter: returns all test details (original behavior) - Default behavior now returns summary only (~150 tokens vs ~13k tokens) - Make results field optional in Python schema for backward compatibility Token savings: - Default: ~13k tokens saved (98.9% reduction) - With failures: minimal tokens (only non-passing tests) - Full details: same as before when explicitly requested This prevents context bloat for typical test runs where you only need pass/fail counts, while still allowing detailed debugging when needed.
TDD Feature:
- Add warning message when filter criteria match zero tests
- New RunTestsTests.cs validates message formatting logic
- Modified RunTests.cs to append "(No tests matched the specified filters)" when total=0
Test Organization Fixes:
- Move MCPToolParameterTests.cs from EditMode/ to EditMode/Tools/ (matches folder hierarchy)
- Fix inconsistent namespaces to MCPForUnityTests.Editor.{Subfolder}:
- MCPToolParameterTests: Tests.EditMode → MCPForUnityTests.Editor.Tools
- DomainReloadResilienceTests: Tests.EditMode.Tools → MCPForUnityTests.Editor.Tools
- Matrix4x4ConverterTests: MCPForUnityTests.EditMode.Helpers → MCPForUnityTests.Editor.Helpers
WalkthroughAdds optional flags to control test-result serialization (includeDetails, includeFailedTests), propagates them through RunTests tools and server, applies per-component properties and material color parsing during creation, fixes test namespaces, and adds unit tests for component property application and test-result message formatting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / Tooling
participant Server as Server API
participant Editor as Unity Editor
participant Runner as Test Runner
CLI->>Server: POST /run_tests (includeDetails?, includeFailedTests?)
Server->>Editor: Forward request with flags
Editor->>Runner: Execute tests (mode/filter)
Runner-->>Editor: TestRunResult (with per-test entries)
Editor->>Editor: Serialize via ToSerializable(mode, includeDetails, includeFailedTests)
Editor-->>Server: Response (results may be full / filtered / null)
Server-->>CLI: Return RunTestsResult
CLI->>CLI: Format message via FormatTestResultMessage
Note over CLI,Server: If Total == 0 → append "No tests matched" warning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
Greptile SummaryThis PR optimizes the Key Changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Python as run_tests.py
participant Unity as RunTests.cs
participant Service as TestRunnerService
Client->>Python: run_tests(mode, include_details, include_failed_tests)
Note over Python: Parse parameters<br/>include_details=False (default)<br/>include_failed_tests=False (default)
Python->>Unity: send_command("run_tests", params)
Unity->>Unity: Parse includeDetails & includeFailedTests flags
Unity->>Service: RunTestsAsync(mode, filters)
Service->>Service: Execute tests
Service-->>Unity: TestRunResult (all test data)
Unity->>Service: result.ToSerializable(mode, includeDetails, includeFailedTests)
alt includeDetails = true
Service-->>Unity: {summary, results: [all tests]}
else includeFailedTests = true
Service-->>Unity: {summary, results: [failed/skipped only]}
else default (both false)
Service-->>Unity: {summary, results: null}
Note over Service,Unity: 98% token reduction
end
Unity->>Unity: Check if total == 0
alt No tests matched
Unity-->>Python: Warning message + data
else Tests found
Unity-->>Python: Success message + data
end
Python-->>Client: RunTestsResponse(message, data)
|
![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
🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (1)
64-79: Consider refactoring to avoid duplicating production logic.The
FormatTestResultMessagehelper duplicates the formatting logic from production code (as noted in the comment on Line 66). This creates a maintenance burden: if the production formatting changes, this test helper must be updated separately, and discrepancies may not be caught.Consider one of these approaches:
- Extract the formatting logic in
RunTests.csto a testable internal/public method and call it directly from tests- If testing the full command flow is too complex, at minimum add a comment explaining why duplication is necessary
Alternative approach: Make production method testable
In
RunTests.cs, extract the formatting logic:+internal static string FormatTestResultMessage(string mode, TestRunResult result) +{ + string message = $"{mode} tests completed: {result.Passed}/{result.Total} passed, {result.Failed} failed, {result.Skipped} skipped"; + if (result.Total == 0) + { + message += " (No tests matched the specified filters)"; + } + return message; +}Then in tests, call the production method directly:
-private static string FormatTestResultMessage(string mode, TestRunResult result) -{ - string message = $"{mode} tests completed: {result.Passed}/{result.Total} passed, {result.Failed} failed, {result.Skipped} skipped"; - if (result.Total == 0) - { - message += " (No tests matched the specified filters)"; - } - return message; -} +// Use MCPForUnity.Editor.Tools.RunTests.FormatTestResultMessage directlyMCPForUnity/Editor/Tools/ManageMaterial.cs (1)
503-503: Consider checking the actual target color property in the condition.The condition only checks if
propertiescontains_BaseColoror_Color, but doesn't account for a customcolorProperty. If a user specifies bothcolorwithproperty="_EmissionColor"and also includes_EmissionColorin thepropertiesobject, the color will be set twice (first to thecolorvalue, then overridden byproperties). While functionally correct (properties win as intended), it's slightly inefficient.🔎 Potential optimization
Consider determining the target property first, then checking if that specific property exists in the properties object:
-if (colorToken != null && (properties == null || (!properties.ContainsKey("_BaseColor") && !properties.ContainsKey("_Color")))) +// Determine which property would be used +string targetColorProperty = null; +if (!string.IsNullOrEmpty(colorProperty) && material.HasProperty(colorProperty)) +{ + targetColorProperty = colorProperty; +} +else if (material.HasProperty("_BaseColor")) +{ + targetColorProperty = "_BaseColor"; +} +else if (material.HasProperty("_Color")) +{ + targetColorProperty = "_Color"; +} + +if (colorToken != null && targetColorProperty != null && (properties == null || !properties.ContainsKey(targetColorProperty))) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
MCPForUnity/Editor/Services/TestRunnerService.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/RunTests.csServer/src/services/tools/run_tests.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/Matrix4x4ConverterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs.meta
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Tools/RunTests.cs (2)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
TryParse(79-104)MCPForUnity/Editor/Services/TestRunnerService.cs (3)
ToSerializable(359-387)ToSerializable(436-447)ToSerializable(478-490)
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
Color(2404-2417)MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
Color(345-395)MaterialOps(12-396)
⏰ 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: Greptile Review
🔇 Additional comments (12)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs (1)
11-11: LGTM! Namespace refactoring aligns with Editor structure.The namespace change from
Tests.EditModetoMCPForUnityTests.Editor.Toolsfollows the broader namespace consolidation across the test suite.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/Matrix4x4ConverterTests.cs (1)
8-8: LGTM! Namespace refactoring aligns with Editor structure.The namespace change from
MCPForUnityTests.EditMode.HelperstoMCPForUnityTests.Editor.Helpersfollows the broader namespace consolidation across the test suite.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/DomainReloadResilienceTests.cs (1)
10-10: LGTM! Namespace refactoring aligns with Editor structure.The namespace change from
Tests.EditMode.ToolstoMCPForUnityTests.Editor.Toolsfollows the broader namespace consolidation across the test suite.MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
1452-1457: LGTM! Enables property assignment during string-based component addition.The change allows properties to be applied when adding components via the string array format by checking
componentProperties[typeName]. This provides parity with the object format (lines 1458-1462) and enables a cleaner API for component addition with properties.Server/src/services/tools/run_tests.py (2)
37-37: LGTM! Optional results field supports selective serialization.Making the
resultsfield optional enables the optimization where per-test details are only included when requested viainclude_failed_testsorinclude_detailsflags.
55-56: Flag precedence is explicitly defined and clearly documented.The Unity-side implementation in
TestRunnerService.cs(lines 359–385) uses an if-else-else chain that establishes unambiguous precedence:includeDetailstakes precedence. When both flags are true, all test results are included (not just failed/skipped). The logic is documented with inline comments explaining each case, so no action needed.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
634-764: Excellent test coverage for component property application.The three new test methods thoroughly validate that component properties are correctly applied during the
add_componentaction across different input formats:
- String array format with top-level
componentProperties- Object format with inline
properties- ComponentName format with
componentPropertiesEach test follows a clear arrange-act-assert pattern with proper cleanup, and the assertions verify specific property values to ensure correctness.
MCPForUnity/Editor/Tools/RunTests.cs (2)
46-65: Safe flag parsing with appropriate defaults.The parsing logic for
includeDetailsandincludeFailedTestshandles edge cases well:
- Uses
bool.TryParsefor safe conversion- Preserves default values (
false) if parsing fails- Empty catch block is appropriate here since the intent is to silently fall back to defaults
93-99: Clear warning message for empty test results.The addition of a warning when no tests match filters improves user experience by making it clear why
Total == 0. The updatedToSerializablecall correctly passes the new flags to control result serialization.MCPForUnity/Editor/Services/TestRunnerService.cs (1)
359-386: Well-implemented conditional serialization with backward compatibility.The updated signature uses optional parameters with sensible defaults (
false), maintaining backward compatibility. The conditional logic correctly implements three serialization modes:
includeDetails=true: serialize all test resultsincludeFailedTests=true: serialize only non-passed tests- Both
false: omit per-test results entirelyThe use of
StringComparison.OrdinalIgnoreCase(Line 372) and the null-safe?.ToList()operator (Line 385) demonstrate defensive coding practices.Note on flag precedence: When both
includeDetailsandincludeFailedTestsaretrue, the implementation gives precedence toincludeDetails(returns all results). Consider documenting this behavior in the method's XML documentation to clarify the intended precedence for API consumers.MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
463-464: LGTM!The parameter extraction follows the established pattern and correctly handles optional values.
505-513: LGTM!The error handling for color parsing is robust and returns a clear error message to the user.
| if (!string.IsNullOrEmpty(colorProperty) && material.HasProperty(colorProperty)) | ||
| { | ||
| material.SetColor(colorProperty, color); | ||
| } | ||
| else if (material.HasProperty("_BaseColor")) | ||
| { | ||
| material.SetColor("_BaseColor", color); | ||
| } | ||
| else if (material.HasProperty("_Color")) | ||
| { | ||
| material.SetColor("_Color", color); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare error handling for non-existent properties in CreateMaterial vs SetMaterialColor
echo "=== SetMaterialColor behavior when property doesn't exist ==="
rg -n -A10 -B2 'if \(!string\.IsNullOrEmpty\(property\)\)' MCPForUnity/Editor/Tools/ManageMaterial.cs | head -20
echo -e "\n=== CreateMaterial behavior when colorProperty doesn't exist ==="
rg -n -A15 'if \(!string\.IsNullOrEmpty\(colorProperty\)' MCPForUnity/Editor/Tools/ManageMaterial.csRepository: dsarno/unity-mcp
Length of output: 1372
🏁 Script executed:
sed -n '155,195p' MCPForUnity/Editor/Tools/ManageMaterial.csRepository: dsarno/unity-mcp
Length of output: 1394
Inconsistent error handling for non-existent colorProperty across methods.
When a custom colorProperty is specified in CreateMaterial and doesn't exist on the material, the code silently falls back to _BaseColor or _Color. However, SetMaterialColor returns an error when a specified property doesn't exist, creating an inconsistency.
Consider aligning both methods: either log a warning when a custom property is silently ignored, or return an error to match SetMaterialColor's behavior.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Tools/ManageMaterial.cs around lines 515-527,
CreateMaterial silently falls back to _BaseColor/_Color when a custom
colorProperty is provided but missing, which is inconsistent with
SetMaterialColor returning an error; change CreateMaterial to log a warning when
a non-empty colorProperty is specified but material.HasProperty(colorProperty)
is false (include the property name and material.name/shader for context) and
then continue with the existing fallback logic so behavior is consistent and
debuggable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (2)
33-36: Simplify assertion to match actual implementation.The assertion checks for
"No tests matched" OR "no tests found", butFormatTestResultMessageonly produces"No tests matched the specified filters". The"no tests found"check will never match and may confuse future maintainers about what strings the method actually produces.🔎 Suggested simplification
- Assert.IsTrue( - message.Contains("No tests matched") || message.Contains("no tests found"), - $"Expected warning when total=0, but got: '{message}'" - ); + Assert.IsTrue( + message.Contains("No tests matched"), + $"Expected warning when total=0, but got: '{message}'" + );
57-61: Simplify assertion to match actual implementation.Same issue as the first test - the assertion checks for
"no tests found"whichFormatTestResultMessagenever produces. Consider removing this check for clarity.🔎 Suggested simplification
Assert.IsFalse( - message.Contains("No tests matched") || message.Contains("no tests found"), + message.Contains("No tests matched"), $"Should not have warning when tests exist, but got: '{message}'" );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/RunTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Tools/RunTests.cs (2)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
TryParse(79-104)MCPForUnity/Editor/Services/TestRunnerService.cs (6)
ToSerializable(359-387)ToSerializable(436-447)ToSerializable(478-490)TestRunResult(343-415)TestRunResult(345-349)TestRunResult(389-414)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (1)
MCPForUnity/Editor/Tools/RunTests.cs (1)
FormatTestResultMessage(123-135)
🔇 Additional comments (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (1)
1-13: LGTM!The test structure and documentation are clear. The explanation about why full
HandleCommandtesting is avoided (recursive test runner calls) is helpful context.MCPForUnity/Editor/Tools/RunTests.cs (3)
46-65: LGTM!The parameter parsing follows the established pattern used for
timeoutSecondswith safe defaults and appropriate error handling. The flags default tofalse, ensuring backward compatibility.
90-93: LGTM!Centralizing message formatting via
FormatTestResultMessageimproves maintainability, and the new flags are correctly propagated toToSerializablefor conditional serialization.
123-135: LGTM!The
FormatTestResultMessagehelper centralizes test result formatting and appropriately warns users when no tests match the specified filters. Theinternalvisibility allows it to be unit tested from the test assembly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (1)
64-65: Remove orphaned comment.This comment appears to be a leftover development note and doesn't add value, as the tests above already demonstrate direct usage of the method.
🔎 Proposed cleanup
- - // Use MCPForUnity.Editor.Tools.RunTests.FormatTestResultMessage directly. } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (1)
MCPForUnity/Editor/Tools/RunTests.cs (1)
FormatTestResultMessage(123-135)
🔇 Additional comments (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.cs (4)
6-12: Well-documented test constraints.The documentation clearly explains why the tests focus on message formatting rather than the full command handler, which helps future maintainers understand the testing approach.
15-37: LGTM - Test correctly verifies no-tests warning.The test properly validates that when no tests are found (total=0), the formatted message includes the "No tests matched" warning as implemented in FormatTestResultMessage.
39-62: LGTM - Test correctly verifies normal test output.The test properly validates that when tests exist, the warning is not included and the pass ratio is formatted correctly. The dual assertions (checking absence of warning and presence of pass ratio) provide good coverage.
30-30: InternalsVisibleTo is properly configured.The production assembly (
MCPForUnity.Editor) has[assembly: InternalsVisibleTo("MCPForUnityTests.EditMode")]configured inMCPForUnity/Editor/AssemblyInfo.cs, allowing test access to the internalFormatTestResultMessagemethod. No changes required.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.