feat: added param:returnWithLogs for run_tests, count pass/fail with valid tests only #48
Conversation
📝 WalkthroughWalkthroughA new optional boolean parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ToolJS/TS
participant UnityTestRunnerService
participant TestExecution
Client->>ToolJS/TS: Request test run (with returnWithLogs)
ToolJS/TS->>UnityTestRunnerService: ExecuteTestsAsync(..., returnWithLogs, ...)
UnityTestRunnerService->>TestExecution: Run tests
TestExecution-->>UnityTestRunnerService: Test results (+ logs if requested)
UnityTestRunnerService-->>ToolJS/TS: JSON result (with or without logs)
ToolJS/TS-->>Client: Final test result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://app.smartcontext.ai/default/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Editor/Tools/RunTestsTool.cs (1)
29-34: XML docs are stale – newly-supported parameters aren’t documented
ExecuteAsyncnow acceptsreturnOnlyFailuresandreturnWithLogs, but the<param>description still only mentionstestModeandtestFilter. Anyone reading the tooltip or generated docs will miss the new capabilities.-/// <param name="parameters">Tool parameters, including optional 'testMode' and 'testFilter'.</param> +/// <param name="parameters"> +/// Tool parameters. Supported keys: +/// • testMode – string (EditMode | PlayMode) +/// • testFilter – string +/// • returnOnlyFailures – bool +/// • returnWithLogs – bool +/// </param>Editor/Services/ITestRunnerService.cs (1)
24-29: Interface change looks good – but wording can misleadThe summary says “all logs” are included, but the implementation only injects per-test
ITestResultAdaptor.Output, not the entire console log stream. Clarify to avoid confusion.-/// <param name="returnWithLogs">If true, all logs are included in the output.</param> +/// <param name="returnWithLogs"> +/// If true, the Unity `ITestResultAdaptor.Output` for each test is included in the +/// JSON results. (This does not include the full Editor console log.) +/// </param>Editor/Services/TestRunnerService.cs (1)
195-208: Avoid emitting"logs": nullwhen logs are disabledWhen
_returnWithLogsisfalse, the serializer still writes"logs": nullfor every test.
Clients must then filter these noisy keys.-["logs"] = _returnWithLogs ? r.Output : null, +// Only add the property when requested +].Concat( + _returnWithLogs ? new[] { new JProperty("logs", r.Output) } : Array.Empty<JProperty>() +))(or build the object imperatively and add the property conditionally)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Editor/Services/ITestRunnerService.cs(1 hunks)Editor/Services/TestRunnerService.cs(3 hunks)Editor/Tools/RunTestsTool.cs(2 hunks)Server~/build/tools/runTestsTool.js(2 hunks)Server~/src/tools/runTestsTool.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Editor/Tools/RunTestsTool.cs (1)
Editor/Services/TestRunnerService.cs (1)
JObject(195-223)
Editor/Services/ITestRunnerService.cs (3)
Editor/Services/TestRunnerService.cs (1)
JObject(195-223)Editor/Services/IConsoleLogsService.cs (1)
JObject(21-21)Editor/Services/ConsoleLogsService.cs (1)
JObject(88-161)
🔇 Additional comments (5)
Editor/Tools/RunTestsTool.cs (2)
40-41: Null-coalescing default is fine – consider explicit validationParsing
returnWithLogsmirrors the other booleans and is correct.
If stronger validation is desired (e.g. reject non-boolean JSON tokens instead of silently coercing), add a type-check here or rely on a schema upstream.
51-53: Call-site matches new interface signatureParameter order aligns with
ITestRunnerService.ExecuteTestsAsync. No further action.Server~/build/tools/runTestsTool.js (1)
45-54: Propagation of new flag is correct
returnWithLogsis forwarded unchanged – implementation is sound.Server~/src/tools/runTestsTool.ts (1)
56-63: Forwarding logic LGTMParameter plumbing is consistent; nothing further.
Editor/Services/TestRunnerService.cs (1)
210-221: Good fix for pass/fail countingSwitching to
result.PassCount/FailCountaccurately handles the “0/0 tests” edge-case – nice catch.
| testFilter: z.string().optional().default('').describe('The specific test filter to run (e.g. specific test name or class name, must include namespace) (optional)'), | ||
| returnOnlyFailures: z.boolean().optional().default(true).describe('Whether to show only failed tests in the results (optional)'), | ||
| returnWithLogs: z.boolean().optional().default(false).describe('Whether to return the test logs in the results (optional)') | ||
| }); |
There was a problem hiding this comment.
Default for returnOnlyFailures diverges from Editor default
Here the schema defaults returnOnlyFailures to true, while the Editor tool falls back to false.
With no explicit user input, the server will always request only failures, changing behaviour unexpectedly after this PR.
- returnOnlyFailures: z.boolean().optional().default(true)
+ // Keep default aligned with Editor side (false)
+ returnOnlyFailures: z.boolean().optional().default(false)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Server~/build/tools/runTestsTool.js around lines 8 to 11, the default value
for returnOnlyFailures is set to true, which conflicts with the Editor tool's
default of false. To fix this, change the default value of returnOnlyFailures in
the schema to false so that the server behavior aligns with the Editor tool when
no explicit user input is provided.
| testFilter: z.string().optional().default('').describe('The specific test filter to run (e.g. specific test name or class name, must include namespace) (optional)'), | ||
| returnOnlyFailures: z.boolean().optional().default(true).describe('Whether to show only failed tests in the results (optional)'), | ||
| returnWithLogs: z.boolean().optional().default(false).describe('Whether to return the test logs in the results (optional)') | ||
| }); |
There was a problem hiding this comment.
Same default mismatch as in JS build artifact
Align the TypeScript source with the Editor default to avoid accidental filtering.
- returnOnlyFailures: z.boolean().optional().default(true)
+ returnOnlyFailures: z.boolean().optional().default(false)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testFilter: z.string().optional().default('').describe('The specific test filter to run (e.g. specific test name or class name, must include namespace) (optional)'), | |
| returnOnlyFailures: z.boolean().optional().default(true).describe('Whether to show only failed tests in the results (optional)'), | |
| returnWithLogs: z.boolean().optional().default(false).describe('Whether to return the test logs in the results (optional)') | |
| }); | |
| testFilter: z.string().optional().default('').describe('The specific test filter to run (e.g. specific test name or class name, must include namespace) (optional)'), | |
| - returnOnlyFailures: z.boolean().optional().default(true).describe('Whether to show only failed tests in the results (optional)'), | |
| + returnOnlyFailures: z.boolean().optional().default(false).describe('Whether to show only failed tests in the results (optional)'), | |
| returnWithLogs: z.boolean().optional().default(false).describe('Whether to return the test logs in the results (optional)') | |
| }); |
🤖 Prompt for AI Agents
In Server~/src/tools/runTestsTool.ts around lines 13 to 16, the default value
for returnOnlyFailures is set to true, which mismatches the expected default in
the Editor. Change the default value of returnOnlyFailures from true to false to
align with the Editor default and prevent accidental filtering of test results.
yulcat
force-pushed
the
feature/return-with-logs
branch
from
ef8eac0 to
bf76cee
Compare
Problem
When running tests, the logs generated during execution were not returned. As a result, an additional get_console_log call was required to retrieve them.
Even if the testFilter was incorrectly set and no actual test ran, the root test group was still marked as "Passed" with passCount: 1/1. This caused the LLM to wrongly assume that the test succeeded.
Fixes
Added a returnWithLogs parameter to the run_test request, allowing logs to be returned alongside the test result in a single response.
Updated the pass/fail count logic to only include actual test tasks. When testFilter is misconfigured, the result is now 0/0 instead of 1/1, prompting the LLM to recognize that no valid test was executed.
Implementation Details
The changes include:
Added a new
returnWithLogsparameter to theExecuteTestsAsyncmethod in theITestRunnerServiceinterface and its implementation inTestRunnerService.Modified the
BuildResultJsonmethod to:returnWithLogsis true.Where(r => !r.HasChildren)to only count actual test tasksresult.PassCount,result.FailCount, etc.) instead of manually countingUpdated the parameter schemas in both JavaScript and TypeScript implementations to include the new
returnWithLogsparameter with a default value offalse.Improved the description of the
testFilterparameter to clarify that it must include the namespace.These changes ensure more accurate test reporting and reduce the need for additional API calls to retrieve test logs.
Created with Palmier
Summary by CodeRabbit