Feature/run tests summary clean #501
Merged
Conversation
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
… 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
- Make ManageScriptableObjectTests setup yield-based with longer Unity-ready timeout - Mark DomainReloadResilienceTests explicit to avoid triggering domain reload during Run All
Contributor
Reviewer's GuideAdds configurable verbosity to the run_tests tool so it returns a compact summary by default, with optional inclusion of failed-only or full per-test details, and updates supporting Unity tests and server-side schema accordingly. Sequence diagram for configurable run_tests verbosity handlingsequenceDiagram
actor Client
participant RunTestsTool as RunTests_HandleCommand
participant TestRunnerService
participant TestRunResult
Client->>RunTestsTool: HandleCommand(params: JObject)
RunTestsTool->>RunTestsTool: Parse mode, timeout, filters
RunTestsTool->>RunTestsTool: Read includeDetails, includeFailedTests
RunTestsTool->>TestRunnerService: RunTests(mode, filters, timeout)
TestRunnerService-->>RunTestsTool: TestRunResult
RunTestsTool->>RunTestsTool: message = FormatTestResultMessage(mode, result)
RunTestsTool->>TestRunResult: ToSerializable(mode, includeDetails, includeFailedTests)
TestRunResult-->>RunTestsTool: anonymous_object { mode, summary, results }
alt includeDetails == true
RunTestsTool->>TestRunResult: include all Results
else includeFailedTests == true
RunTestsTool->>TestRunResult: include only failed and skipped Results
else
RunTestsTool->>TestRunResult: omit individual Results (results = null)
end
RunTestsTool-->>Client: SuccessResponse(message, data)
Class diagram for updated test run result serialization and RunTests toolclassDiagram
class RunTests {
+Task~object~ HandleCommand(JObject @params)
+string FormatTestResultMessage(string mode, TestRunResult result)
-TestFilterOptions ParseFilterOptions(JObject @params)
-string[] ParseStringArray(JObject @params, string key)
}
class TestRunSummary {
+int Total
+int Passed
+int Failed
+int Skipped
+object ToSerializable()
}
class TestRunTestResult {
+string Name
+string State
+double Duration
+string Message
+object ToSerializable()
}
class TestRunResult {
-TestRunSummary Summary
-IReadOnlyList~TestRunTestResult~ Results
+int Total
+int Passed
+int Failed
+int Skipped
+object ToSerializable(string mode, bool includeDetails, bool includeFailedTests)
}
RunTests ..> TestRunResult : uses
RunTests ..> TestRunSummary : uses
RunTests ..> TestRunTestResult : uses
TestRunResult *-- TestRunSummary : summary
TestRunResult *-- TestRunTestResult : results
Flow diagram for TestRunResult.ToSerializable verbosity selectionflowchart TD
A["Start ToSerializable(mode, includeDetails, includeFailedTests)"] --> B["includeDetails == true?"]
B -- Yes --> C["resultsToSerialize = all Results (r.ToSerializable())"]
B -- No --> D["includeFailedTests == true?"]
D -- Yes --> E["resultsToSerialize = Results where State != Passed"]
D -- No --> F["resultsToSerialize = null"]
C --> G["Return { mode, summary = Summary.ToSerializable(), results = resultsToSerialize.ToList() }"]
E --> G
F --> G
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces optional parameters to the test result serialization pipeline, enabling selective inclusion of test details based on caller preferences. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
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 found 1 issue, and left some high level feedback:
- In
RunTests.HandleCommand, theincludeDetails/includeFailedTestsparsing can be simplified and made safer by using@params?['includeDetails']?.Value<bool?>()/Value<bool?>()instead ofToString()+bool.TryParsewrapped in a broadtry/catch. - In
TestRunResult.ToSerializable, consider explicitly documenting or enforcing the precedence when bothincludeDetailsandincludeFailedTestsare true (currentlyincludeDetailswins), so that callers and future maintainers have a clear expectation of which verbosity mode is applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RunTests.HandleCommand`, the `includeDetails`/`includeFailedTests` parsing can be simplified and made safer by using `@params?['includeDetails']?.Value<bool?>()` / `Value<bool?>()` instead of `ToString()` + `bool.TryParse` wrapped in a broad `try/catch`.
- In `TestRunResult.ToSerializable`, consider explicitly documenting or enforcing the precedence when both `includeDetails` and `includeFailedTests` are true (currently `includeDetails` wins), so that callers and future maintainers have a clear expectation of which verbosity mode is applied.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/run_tests.py:55-59` </location>
<code_context>
group_names: Annotated[list[str] | str, "Same as test_names, except it allows for Regex"] | None = None,
category_names: Annotated[list[str] | str, "NUnit category names to filter by (tests marked with [Category] attribute)"] | None = None,
assembly_names: Annotated[list[str] | str, "Assembly names to filter tests by"] | None = None,
+ include_failed_tests: Annotated[bool, "Include details for failed/skipped tests only (default: false)"] = False,
+ include_details: Annotated[bool, "Include details for all tests (default: false)"] = False,
) -> RunTestsResponse:
</code_context>
<issue_to_address>
**suggestion (testing):** Add server-side tests to verify wiring of `include_failed_tests` / `include_details` into the Unity command params.
The Python `run_tests` entrypoint now exposes these flags and forwards them as `includeFailedTests` / `includeDetails` params. Please add tests (e.g., using a mocked `send_with_unity_instance`) that:
- Call `run_tests` with default args and assert neither key appears in `params`.
- Call `run_tests(include_failed_tests=True)` and assert only `includeFailedTests` is present and `True`.
- Call `run_tests(include_details=True)` and assert only `includeDetails` is present and `True`.
- Optionally cover both flags `True` together to document combined behavior.
This will lock in the client/server contract for test-result verbosity as the feature evolves.
Suggested implementation:
```python
import asyncio
from unittest.mock import AsyncMock
import pytest
from Server.src.services.tools.run_tests import run_tests
@pytest.mark.asyncio
async def test_run_tests_does_not_include_verbosity_flags_by_default(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
# Simulate a minimal successful response structure
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
# Also stub out Unity instance resolution to avoid hitting real Unity
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None)
# Assert
assert "includeFailedTests" not in captured_params
assert "includeDetails" not in captured_params
@pytest.mark.asyncio
async def test_run_tests_includes_failed_tests_flag(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None, include_failed_tests=True)
# Assert
assert captured_params.get("includeFailedTests") is True
# Only includeFailedTests should be present
assert "includeDetails" not in captured_params
@pytest.mark.asyncio
async def test_run_tests_includes_details_flag(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None, include_details=True)
# Assert
assert captured_params.get("includeDetails") is True
# Only includeDetails should be present
assert "includeFailedTests" not in captured_params
@pytest.mark.asyncio
async def test_run_tests_includes_both_flags_when_true(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None, include_failed_tests=True, include_details=True)
# Assert
assert captured_params.get("includeFailedTests") is True
assert captured_params.get("includeDetails") is True
```
I assumed a pytest-based test suite and a module path `Server/tests/services/tools/test_run_tests.py`. If your tests live elsewhere (e.g., `tests/server/services/tools/test_run_tests.py` or similar), adjust the `file_path` and the import/monkeypatch targets accordingly so that:
- `from Server.src.services.tools.run_tests import run_tests` matches your actual import path.
- The `monkeypatch.setattr` targets for `send_with_unity_instance` and `get_unity_instance_from_context` use the correct fully-qualified module name.
If your async test runner differs (e.g., `pytest.mark.anyio` or a custom helper), swap `@pytest.mark.asyncio` for the appropriate decorator.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+55
to
59
| include_failed_tests: Annotated[bool, "Include details for failed/skipped tests only (default: false)"] = False, | ||
| include_details: Annotated[bool, "Include details for all tests (default: false)"] = False, | ||
| ) -> RunTestsResponse: | ||
| unity_instance = get_unity_instance_from_context(ctx) | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add server-side tests to verify wiring of include_failed_tests / include_details into the Unity command params.
The Python run_tests entrypoint now exposes these flags and forwards them as includeFailedTests / includeDetails params. Please add tests (e.g., using a mocked send_with_unity_instance) that:
- Call
run_testswith default args and assert neither key appears inparams. - Call
run_tests(include_failed_tests=True)and assert onlyincludeFailedTestsis present andTrue. - Call
run_tests(include_details=True)and assert onlyincludeDetailsis present andTrue. - Optionally cover both flags
Truetogether to document combined behavior.
This will lock in the client/server contract for test-result verbosity as the feature evolves.
Suggested implementation:
import asyncio
from unittest.mock import AsyncMock
import pytest
from Server.src.services.tools.run_tests import run_tests
@pytest.mark.asyncio
async def test_run_tests_does_not_include_verbosity_flags_by_default(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
# Simulate a minimal successful response structure
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
# Also stub out Unity instance resolution to avoid hitting real Unity
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None)
# Assert
assert "includeFailedTests" not in captured_params
assert "includeDetails" not in captured_params
@pytest.mark.asyncio
async def test_run_tests_includes_failed_tests_flag(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None, include_failed_tests=True)
# Assert
assert captured_params.get("includeFailedTests") is True
# Only includeFailedTests should be present
assert "includeDetails" not in captured_params
@pytest.mark.asyncio
async def test_run_tests_includes_details_flag(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None, include_details=True)
# Assert
assert captured_params.get("includeDetails") is True
# Only includeDetails should be present
assert "includeFailedTests" not in captured_params
@pytest.mark.asyncio
async def test_run_tests_includes_both_flags_when_true(monkeypatch):
captured_params = {}
async def fake_send_with_unity_instance(unity_instance, command, params):
nonlocal captured_params
captured_params = params
return {
"mode": "edit",
"summary": {
"total": 0,
"passed": 0,
"failed": 0,
"inconclusive": 0,
"skipped": 0,
"duration": 0.0,
},
"results": [],
}
monkeypatch.setattr(
"Server.src.services.tools.run_tests.send_with_unity_instance",
fake_send_with_unity_instance,
)
monkeypatch.setattr(
"Server.src.services.tools.run_tests.get_unity_instance_from_context",
lambda ctx: object(),
)
# Act
await run_tests(ctx=None, include_failed_tests=True, include_details=True)
# Assert
assert captured_params.get("includeFailedTests") is True
assert captured_params.get("includeDetails") is TrueI assumed a pytest-based test suite and a module path Server/tests/services/tools/test_run_tests.py. If your tests live elsewhere (e.g., tests/server/services/tools/test_run_tests.py or similar), adjust the file_path and the import/monkeypatch targets accordingly so that:
from Server.src.services.tools.run_tests import run_testsmatches your actual import path.- The
monkeypatch.setattrtargets forsend_with_unity_instanceandget_unity_instance_from_contextuse the correct fully-qualified module name.
If your async test runner differs (e.g., pytest.mark.anyio or a custom helper), swap @pytest.mark.asyncio for the appropriate decorator.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Optimizes run_test tool -- returns summary by default instead of entire test result dump. Options for failed tests only, and for detailed results. 98% less context used.
Summary by Sourcery
Optimize Unity MCP run_tests tool responses to be more concise by default while adding optional verbosity controls and improving test stability and coverage.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.