chore: Collapse the RunTests tool's redundant intermediate DTO layer#1515
Conversation
The RunTests pipeline previously carried a UnityCliLoopTestExecutionRequest / UnityCliLoopTestExecutionResult pair and an IUnityCliLoopTestExecutionService interface between RunTestsSchema and RunTestsResponse. Every hop was a pure field-for-field copy into an identically shaped record, so the intermediate layer only added indirection without changing behavior. Drop the interface and the two DTOs and thread RunTestsSchema / RunTestsResponse through the pipeline directly. RunTestsResponse is still constructed with every derived field passed explicitly so the constructor's derivation branches stay skipped, matching the pre-collapse wire output byte-for-byte. The UnityCliLoopTestMode, TestFilterType, and RunTestsExecutionStatus enums are the real domain vocabulary shared by the schema and services (for example TestExecutionStateValidationService.Validate takes UnityCliLoopTestMode), so they stay in the same file to preserve the .meta GUID.
The wrapper only existed to satisfy the deleted IUnityCliLoopTestExecutionService interface; the tool and every test call ExecuteAsync, so keeping two public entry points invited drift.
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRunTestsUseCase, RunTestsTool, and UnityCliLoopTestExecutionTypes are migrated from the UnityCliLoopTestExecutionRequest/Result and IUnityCliLoopTestExecutionService shape to RunTestsSchema/RunTestsResponse. The old interface, DTOs, and RunTestsAsync method are removed; RunTestsUseCaseTests and OnionAssemblyDependencyTests are updated accordingly. ChangesRunTests schema migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs (1)
112-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment contradicts the throw.
The comment says "Create a minimal error result", but the code now throws instead of building a result. Update or drop the comment to avoid misleading future readers of the error path.
♻️ Suggested tweak
- // Create a minimal error result + // Surface the failure; the tool layer converts it into an error response. throw new System.InvalidOperationException("Test execution failed. Please check the logs for details.", ex);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs` around lines 112 - 113, The comment in RunTestsUseCase’s error path is stale and contradicts the current behavior: the code now throws an InvalidOperationException instead of creating a minimal error result. Update or remove that comment near the throw in the RunTestsUseCase flow so it accurately reflects the exception-based handling and does not mislead readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs`:
- Around line 112-113: The comment in RunTestsUseCase’s error path is stale and
contradicts the current behavior: the code now throws an
InvalidOperationException instead of creating a minimal error result. Update or
remove that comment near the throw in the RunTestsUseCase flow so it accurately
reflects the exception-based handling and does not mislead readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e689779-3f3b-4a7a-904e-1db8152bd64f
📒 Files selected for processing (5)
Assets/Tests/Editor/OnionAssemblyDependencyTests.csAssets/Tests/Editor/RunTestsUseCaseTests.csPackages/src/Editor/FirstPartyTools/RunTests/RunTestsTool.csPackages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.csPackages/src/Editor/FirstPartyTools/RunTests/UnityCliLoopTestExecutionTypes.cs
💤 Files with no reviewable changes (1)
- Assets/Tests/Editor/OnionAssemblyDependencyTests.cs
Restore the explicit ArgumentNullException guard to match the Compile pipeline's Fail Fast precedent, reuse the existing RunTestsResponse.CreateTestFrameworkUnavailable factory instead of rebuilding the same shape by hand, and reword comments that referenced the deleted intermediate DTO or contradicted the exception-based error path.
Summary
IUnityCliLoopTestExecutionService,UnityCliLoopTestExecutionRequest,UnityCliLoopTestExecutionResult), which only performed field-by-field copies between the wire DTOs and the test-execution pipeline. Follows the same collapse applied to the Compile tool in chore: Collapse the Compile tool's redundant intermediate DTO layer #1514.User Impact
Changes
RunTestsUseCasenow takesRunTestsSchemaand returnsRunTestsResponsedirectly;RunTestsToolno longer needs its ToRequest/ToResponse mapping helpers.RunTestsResponse's null-triggered derivation branches stay skipped exactly as before — output is unchanged.UnityCliLoopTestMode,TestFilterType,RunTestsExecutionStatus) stay in place: they are the real shared vocabulary, not mapping artifacts.RunTestsAsyncpass-through (which only existed to satisfy the deleted interface) is removed;ExecuteAsyncis the single entry point.RunTestsUseCaseguard remains). Repo-wide grep confirms no remaining references, including non-C# files.Verification
dist/darwin-arm64/uloop compile→ 0 errors, 0 warnings.dist/darwin-arm64/uloop run-tests --filter-type regex --filter-value "(RunTests|OnionAssemblyDependency).*Tests"→ 124 passed, 0 failed, 0 skipped.