Improve material ops logging and test coverage

Author: dsarno

MCPForUnity/Editor/Helpers/MaterialOps.cs

@@ -102,7 +102,10 @@ JToken GetValue(string key)
                             modified = true;
                         }
                     }
-                    catch { }
+                    catch (Exception ex)
+                    {
+                        Debug.LogWarning($"[MaterialOps] Failed to set float property '{propName}': {ex.Message}");
+                    }
                 }
             }
 
@@ -229,36 +232,57 @@ public static bool TrySetShaderProperty(Material material, string propertyName,
                     if (material.HasProperty(propertyName))
                     {
                         try { material.SetColor(propertyName, ParseColor(value, serializer)); return true; } 
-                        catch (Exception ex) { Debug.LogWarning($"[MaterialOps] SetColor failed: {ex.Message}"); }
-                        
+                        catch (Exception ex) 
+                        { 
+                            // Log at Debug level since we'll try other conversions
+                            Debug.Log($"[MaterialOps] SetColor attempt for '{propertyName}' failed: {ex.Message}"); 
+                        }
+
                         try { Vector4 vec = value.ToObject<Vector4>(serializer); material.SetVector(propertyName, vec); return true; } 
-                        catch (Exception ex) { Debug.LogWarning($"[MaterialOps] SetVector (Vec4) failed: {ex.Message}"); }
+                        catch (Exception ex) 
+                        { 
+                            Debug.Log($"[MaterialOps] SetVector (Vec4) attempt for '{propertyName}' failed: {ex.Message}"); 
+                        }
                     }
                 }
                 else if (jArray.Count == 3)
                 {
                     if (material.HasProperty(propertyName))
                     {
                         try { material.SetColor(propertyName, ParseColor(value, serializer)); return true; } 
-                        catch (Exception ex) { Debug.LogWarning($"[MaterialOps] SetColor (Vec3) failed: {ex.Message}"); }
+                        catch (Exception ex) 
+                        { 
+                             Debug.Log($"[MaterialOps] SetColor (Vec3) attempt for '{propertyName}' failed: {ex.Message}"); 
+                        }
                     }
                 }
                 else if (jArray.Count == 2)
                 {
                     if (material.HasProperty(propertyName))
                     {
                         try { Vector2 vec = value.ToObject<Vector2>(serializer); material.SetVector(propertyName, vec); return true; } 
-                        catch (Exception ex) { Debug.LogWarning($"[MaterialOps] SetVector (Vec2) failed: {ex.Message}"); }
+                        catch (Exception ex) 
+                        { 
+                            Debug.Log($"[MaterialOps] SetVector (Vec2) attempt for '{propertyName}' failed: {ex.Message}"); 
+                        }
                     }
                 }
             }
             else if (value.Type == JTokenType.Float || value.Type == JTokenType.Integer)
             {
-                try { material.SetFloat(propertyName, value.ToObject<float>(serializer)); return true; } catch { }
+                try { material.SetFloat(propertyName, value.ToObject<float>(serializer)); return true; } 
+                catch (Exception ex)
+                {
+                     Debug.Log($"[MaterialOps] SetFloat attempt for '{propertyName}' failed: {ex.Message}");
+                }
             }
             else if (value.Type == JTokenType.Boolean)
             {
-                try { material.SetFloat(propertyName, value.ToObject<bool>(serializer) ? 1f : 0f); return true; } catch { }
+                try { material.SetFloat(propertyName, value.ToObject<bool>(serializer) ? 1f : 0f); return true; } 
+                catch (Exception ex)
+                {
+                     Debug.Log($"[MaterialOps] SetFloat (bool) attempt for '{propertyName}' failed: {ex.Message}");
+                }
             }
             else if (value.Type == JTokenType.String)
             {
@@ -341,9 +365,21 @@ public static Color ParseColor(JToken token, JsonSerializer serializer)
                         1f
                     );
                 }
+                else
+                {
+                    throw new ArgumentException("Color array must have 3 or 4 elements.");
+                }
             }
 
-            return token.ToObject<Color>(serializer);
+            try
+            {
+                return token.ToObject<Color>(serializer);
+            }
+            catch (Exception ex)
+            {
+                Debug.LogWarning($"[MaterialOps] Failed to parse color from token: {ex.Message}");
+                throw;
+            }
         }
     }
 }

MCPForUnity/Editor/Tools/ManageMaterial.cs

@@ -9,7 +9,7 @@
 namespace MCPForUnity.Editor.Tools
 {
     [McpForUnityTool("manage_material", AutoRegister = false)]
-    public class ManageMaterial
+    public static class ManageMaterial
     {
         public static object HandleCommand(JObject @params)
         {
@@ -280,7 +280,7 @@ private static object SetRendererColor(JObject @params)
                 MaterialPropertyBlock block = new MaterialPropertyBlock();
                 renderer.GetPropertyBlock(block, slot);
 
-                if (slot < renderer.sharedMaterials.Length && renderer.sharedMaterials[slot] != null)
+                if (renderer.sharedMaterials[slot] != null)
                 {
                     Material mat = renderer.sharedMaterials[slot];
                     if (mat.HasProperty("_BaseColor")) block.SetColor("_BaseColor", color);
@@ -301,6 +301,10 @@ private static object SetRendererColor(JObject @params)
                 if (slot >= 0 && slot < renderer.sharedMaterials.Length)
                 {
                      Material mat = renderer.sharedMaterials[slot];
+                     if (mat == null)
+                     {
+                         return new { status = "error", message = $"No material in slot {slot}" };
+                     }
                      Undo.RecordObject(mat, "Set Material Color");
                      if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color);
                      else mat.SetColor("_Color", color);
@@ -314,6 +318,10 @@ private static object SetRendererColor(JObject @params)
                 if (slot >= 0 && slot < renderer.materials.Length)
                 {
                      Material mat = renderer.materials[slot]; 
+                     if (mat == null)
+                     {
+                         return new { status = "error", message = $"No material in slot {slot}" };
+                     }
                      // Note: Undo cannot fully revert material instantiation
                      Undo.RecordObject(mat, "Set Instance Material Color");
                      if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color);

Server/src/services/tools/manage_material.py

@@ -61,7 +61,10 @@ async def manage_material(
             try:
                 slot = int(slot)
             except ValueError:
-                pass  # Let it fail downstream; C# expects int
+                return {
+                    "success": False,
+                    "message": f"Invalid slot value: '{slot}' must be a valid integer"
+                }
 
     # Prepare parameters for the C# handler
     params_dict = {
@@ -82,6 +85,11 @@ async def manage_material(
     params_dict = {k: v for k, v in params_dict.items() if v is not None}
 
     # Use centralized async retry helper with instance routing
-    result = await send_with_unity_instance(async_send_command_with_retry, unity_instance, "manage_material", params_dict)
+    result = await send_with_unity_instance(
+        async_send_command_with_retry,
+        unity_instance,
+        "manage_material",
+        params_dict,
+    )
 
     return result if isinstance(result, dict) else {"success": False, "message": str(result)}

Server/tests/integration/test_manage_asset_json_parsing.py

@@ -120,7 +120,7 @@ class TestManageGameObjectJsonParsing:
 
     @pytest.mark.asyncio
     async def test_component_properties_json_string_parsing(self, monkeypatch):
-        """Test that JSON string component_properties are correctly parsed."""
+        """Test that JSON string component_properties result in successful operation."""
         from services.tools.manage_gameobject import manage_gameobject
 
         ctx = DummyContext()
@@ -140,20 +140,9 @@ async def fake_send(cmd, params, **kwargs):
             component_properties='{"MeshRenderer": {"material": "Assets/Materials/BlueMaterial.mat"}}'
         )
 
-        # Verify JSON parsing was logged
-        
         # Verify the result
         assert result["success"] is True
-        
-        # Verify that component_properties reached Unity as a dict
-        # We can't easily check 'captured_params' here without refactoring the test to capture args,
-        # but since we mocked the transport, we can trust the return value and rely on
-        # unit tests for parse_json_payload.
-        # However, to follow the feedback, let's verify implicit behavior or refactor.
-        # Since I cannot easily monkeypatch a capture variable here without changing the test structure significantly,
-        # I will rely on the fact that if it wasn't parsed, it would likely fail downstream or be passed as string.
-        # The feedback suggested: "captured_params = {} ... monkeypatch ... assert isinstance(captured_params...)"
-        # I'll implement that pattern.
+
 
     @pytest.mark.asyncio
     async def test_component_properties_parsing_verification(self, monkeypatch):

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs

@@ -15,39 +15,73 @@ public class ReadConsoleTests
         public void HandleCommand_Clear_Works()
         {
             // Arrange
-            var paramsObj = new JObject
-            {
-                ["action"] = "clear"
-            };
+            // Ensure there's something to clear
+            Debug.Log("Log to clear");
+            
+            // Verify content exists before clear
+            var getBefore = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "get", ["count"] = 10 }));
+            var entriesBefore = getBefore["data"] as JArray;
+            
+            // Ideally we'd assert count > 0, but other tests/system logs might affect this.
+            // Just ensuring the call doesn't fail is a baseline, but let's try to be stricter if possible.
+            // Since we just logged, there should be at least one entry.
+            Assert.IsTrue(entriesBefore != null && entriesBefore.Count > 0, "Setup failed: console should have logs.");
 
             // Act
-            var result = ToJObject(ReadConsole.HandleCommand(paramsObj));
+            var result = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "clear" }));
 
             // Assert
             Assert.IsTrue(result.Value<bool>("success"), result.ToString());
+            
+            // Verify clear effect
+            var getAfter = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "get", ["count"] = 10 }));
+            var entriesAfter = getAfter["data"] as JArray;
+            Assert.IsTrue(entriesAfter == null || entriesAfter.Count == 0, "Console should be empty after clear.");
         }
 
         [Test]
         public void HandleCommand_Get_Works()
         {
             // Arrange
-            Debug.Log("Test Log Message"); // Ensure there is at least one log
+            string uniqueMessage = $"Test Log Message {Guid.NewGuid()}";
+            Debug.Log(uniqueMessage);
+            
             var paramsObj = new JObject
             {
                 ["action"] = "get",
-                ["count"] = 5
+                ["count"] = 10 // Fetch enough to likely catch our message
             };
 
             // Act
             var result = ToJObject(ReadConsole.HandleCommand(paramsObj));
 
             // Assert
             Assert.IsTrue(result.Value<bool>("success"), result.ToString());
-            Assert.IsInstanceOf<JArray>(result["data"]);
+            var data = result["data"] as JArray;
+            Assert.IsNotNull(data, "Data array should not be null.");
+            Assert.IsTrue(data.Count > 0, "Should retrieve at least one log entry.");
+
+            // Verify content
+            bool found = false;
+            foreach (var entry in data)
+            {
+                if (entry["message"]?.ToString().Contains(uniqueMessage) == true)
+                {
+                    found = true;
+                    break;
+                }
+            }
+            Assert.IsTrue(found, $"The unique log message '{uniqueMessage}' was not found in retrieved logs.");
         }
 
         private static JObject ToJObject(object result)
         {
+            if (result == null)
+            {
+                Assert.Fail("ReadConsole.HandleCommand returned null.");
+                return new JObject(); // Unreachable, but satisfies return type.
+            }
+
             return result as JObject ?? JObject.FromObject(result);
         }
     }