fix(run_tests): support snake_case params and handle double-serialized arrays#690
Open
bruno1308 wants to merge 2 commits intoCoplayDev:betafrom
Open
fix(run_tests): support snake_case params and handle double-serialized arrays#690bruno1308 wants to merge 2 commits intoCoplayDev:betafrom
bruno1308 wants to merge 2 commits intoCoplayDev:betafrom
Conversation
…d arrays The MCP tool schema defines filter parameters in snake_case (test_names, group_names, etc.) but GetFilterOptions() only checked camelCase keys, causing test filtering to silently fail — all tests would run instead of the filtered subset. Additionally, some MCP clients serialize array parameters as a stringified JSON array inside an outer array (e.g. ["[\"name1\"]"]), which was not handled by ParseStringArray(). Changes: - Add snake_case fallback to all ParseStringArray calls in RunTests.cs - Handle stringified JSON arrays in String token branch - Handle double-serialized arrays in Array token branch - Handle nested arrays ([[name1, name2]]) in Array token branch - Add snake_case fallback for include_details/include_failed_tests in both RunTests.cs and GetTestJob.cs Fixes CoplayDev#689 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds snake_case parameter support and more robust array parsing for MCP test commands, ensuring both naming conventions and double-serialized arrays are correctly handled in RunTests and GetTestJob. Sequence diagram for run_tests parameter parsing and filter applicationsequenceDiagram
actor Dev
participant MCPClient
participant MCPBridge
participant RunTests
participant ParamCoercion
participant TestJobManager
Dev->>MCPClient: Invoke run_tests(test_names=["SomeTest"], include_details=true)
MCPClient->>MCPBridge: JSON RPC with snake_case params
MCPBridge->>RunTests: HandleCommand(@params)
activate RunTests
RunTests->>ParamCoercion: CoerceBool(@params[includeDetails] ?? @params[include_details], false)
ParamCoercion-->>RunTests: includeDetails
RunTests->>ParamCoercion: CoerceBool(@params[includeFailedTests] ?? @params[include_failed_tests], false)
ParamCoercion-->>RunTests: includeFailedTests
RunTests->>RunTests: GetFilterOptions(@params)
activate RunTests
RunTests->>RunTests: ParseStringArray("testNames", "test_names")
Note over RunTests: token = @params["testNames"] ?? @params["test_names"]
RunTests-->>RunTests: Detect stringified or double-serialized array
RunTests-->>RunTests: Normalize to string[] testNames
RunTests-->>RunTests: Build TestFilterOptions
deactivate RunTests
RunTests->>TestJobManager: StartJob(mode, filterOptions)
TestJobManager-->>RunTests: jobId
deactivate RunTests
RunTests-->>MCPBridge: Success response with jobId
MCPBridge-->>MCPClient: Response
MCPClient-->>Dev: Shows filtered test run started
Updated class diagram for RunTests and GetTestJob parameter handlingclassDiagram
class RunTests {
+static Task~object~ HandleCommand(JObject params)
-static TestFilterOptions GetFilterOptions(JObject params)
-static string[] ParseStringArray(string camelKey, string snakeKey)
}
class GetTestJob {
+static object HandleCommand(JObject params)
}
class TestFilterOptions {
+string[] testNames
+string[] groupNames
+string[] categoryNames
+string[] assemblyNames
}
class ParamCoercion {
+static bool CoerceBool(JToken token, bool defaultValue)
}
class TestJobManager {
+static string StartJob(string mode, TestFilterOptions filterOptions)
+static TestJob GetJob(string jobId)
}
class TestJob {
+string jobId
+string status
+bool includeDetails
+bool includeFailedTests
}
RunTests ..> ParamCoercion : uses
RunTests ..> TestFilterOptions : creates
RunTests ..> TestJobManager : starts_job
GetTestJob ..> ParamCoercion : uses
GetTestJob ..> TestJobManager : retrieves_job
TestJobManager --> TestJob : manages
RunTests --> TestFilterOptions : configures_filters
GetTestJob --> TestJob : returns_job_state
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughReplace direct param access with ParamCoercion utilities: add GetParam and CoerceStringArray helpers, use them in RunTests and GetTestJob to accept camelCase and snake_case keys and to handle double-serialized/nested string-array formats. Changes
Sequence Diagram(s)(omitted — changes are parsing-focused and do not introduce a multi-component sequential flow requiring a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The snake_case / camelCase fallback pattern (
@params?["includeDetails"] ?? @params?["include_details"]and inParseStringArray) is repeated in a few places; consider extracting a small helper for parameter lookup to keep this logic centralized and reduce the chance of future inconsistencies. - In
ParseStringArray, the broadcatch { /* not a valid JSON array, treat as plain string */ }aroundJArray.Parsesilently swallows all exceptions; consider narrowing this to JSON-specific exceptions or at least logging in debug builds so unexpected issues with parameter formats are easier to diagnose. - The nested-array handling (
[[name1, name2]]) inParseStringArraycurrently collapses only a single-element outer array; if multi-level or mixed nesting is a realistic input, you may want to generalize this unwrapping to avoid surprising behavior for inputs like[["a"], ["b"]].
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The snake_case / camelCase fallback pattern (`@params?["includeDetails"] ?? @params?["include_details"]` and in `ParseStringArray`) is repeated in a few places; consider extracting a small helper for parameter lookup to keep this logic centralized and reduce the chance of future inconsistencies.
- In `ParseStringArray`, the broad `catch { /* not a valid JSON array, treat as plain string */ }` around `JArray.Parse` silently swallows all exceptions; consider narrowing this to JSON-specific exceptions or at least logging in debug builds so unexpected issues with parameter formats are easier to diagnose.
- The nested-array handling (`[[name1, name2]]`) in `ParseStringArray` currently collapses only a single-element outer array; if multi-level or mixed nesting is a realistic input, you may want to generalize this unwrapping to avoid surprising behavior for inputs like `[["a"], ["b"]]`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Address code review feedback: - Extract snake_case/camelCase param lookup into ParamCoercion.GetParam() - Extract string array coercion (with double-serialization handling) into ParamCoercion.CoerceStringArray() - Narrow catch clauses from bare catch to JsonException - Add explicit comment clarifying nested array unwrapping limitation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I've applied the changed files in this PR (as of 854b421) straight into my |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GetFilterOptions()inRunTests.csonly checked camelCase keys (testNames,groupNames, etc.) but the MCP tool schema defines them in snake_case (test_names,group_names, etc.). Added fallback lookups so both naming conventions work.["[\"name1\"]"]). Added detection and unwrapping for this pattern inParseStringArray().GetTestJob.cs:include_detailsandinclude_failed_testsparameters also only checked camelCase.Root Cause
When an MCP client sends
test_names: ["MyTest"], the C# handler looked up@params["testNames"]which returned null, so no filter was applied. The double-serialization issue compounded this — even if the key matched, the array value["[\"MyTest\"]"]would extract the stringified JSON as a literal test name, which never matched any test.Testing
Verified with Claude Code MCP client:
run_tests(test_names=["SomeTest"])always ran 0 testsFixes #689
🤖 Generated with Claude Code
Summary by Sourcery
Improve MCP test commands parameter handling for better compatibility with different clients.
Bug Fixes:
Summary by CodeRabbit