Harden PlayMode test runs #396
Conversation
- Guard against starting tests while already in Play Mode. - Pre-save dirty scenes before PlayMode runs to avoid SaveModifiedSceneTask failures. - Temporarily disable domain reload during PlayMode tests to keep the MCP bridge alive; restore settings afterward. - Avoid runSynchronously because it can freeze Unity
WalkthroughUpdated TestRunnerService to add Play Mode-aware test execution: it blocks runs when the Editor is (or is entering) Play Mode, handles Play Mode tests by disabling domain reload temporarily, pre-saves dirty scenes, executes tests with adjusted settings, and restores Editor Enter Play Mode options. ConfigureAwait usage changed in async methods. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestRunner as TestRunnerService
participant EditorSettings as EnterPlayModeOptions
participant SceneMgr as EditorSceneManager
participant TestFramework as Unity Test Framework
User->>TestRunner: RunTestsAsync(mode="play")
TestRunner->>TestRunner: Check EditorPlayMode state
alt Editor is in/entering Play Mode
TestRunner-->>User: Reject / throw error (cannot run)
else Editor not in Play Mode
TestRunner->>SceneMgr: SaveDirtyScenesIfNeeded()
Note right of SceneMgr `#DDDDFF`: Pre-save modified scenes
TestRunner->>EditorSettings: EnsurePlayModeRunsWithoutDomainReload()
Note right of EditorSettings `#DDFFDD`: Store & disable domain reload
TestRunner->>TestFramework: Execute tests (play mode)
TestFramework-->>TestRunner: Tests complete / error
TestRunner->>EditorSettings: RestoreEnterPlayModeOptions()
Note right of EditorSettings `#FFEECC`: Restore previous settings
TestRunner-->>User: Return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/TestRunnerService.cs (1)
222-240: Consider handling new (unsaved) scenes explicitly.
EditorSceneManager.SaveScene(scene)will fail for new scenes that haven't been saved yet (no valid path). While the exception is caught and logged, the warning message may be confusing for users. Consider checking!string.IsNullOrEmpty(scene.path)before attempting to save, and logging a more specific message for pathless scenes:if (scene.isDirty) { + if (string.IsNullOrEmpty(scene.path)) + { + McpLog.Warn($"[TestRunnerService] Skipping unsaved scene '{scene.name}': save it manually before running PlayMode tests."); + continue; + } try { EditorSceneManager.SaveScene(scene); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Services/TestRunnerService.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Services/TestRunnerService.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Services/TestRunnerService.cs
🔇 Additional comments (5)
MCPForUnity/Editor/Services/TestRunnerService.cs (5)
36-36: Good: ConfigureAwait(true) is correct for Unity Editor context.Unity Editor APIs must execute on the main thread. Using
ConfigureAwait(true)ensures async continuations resume on the captured synchronization context, which is essential for subsequent calls toTestRunnerApi,EditorSettings, and other Unity APIs.
75-78: Good: Defensive guard prevents starting tests during Play Mode transition.Checking both
isPlayingandisPlayingOrWillChangePlaymodecorrectly covers the transitional state. This directly addresses the issue where starting tests mid-transition caused failures.
193-214: Settings modification is project-wide; consider edge case documentation.The logic correctly preserves existing flags while adding
DisableDomainReload. One edge case: if Unity crashes or the process is killed during the test run, the project's Enter Play Mode settings will remain modified until the next successful test run. This is likely an acceptable trade-off, but consider adding a brief note in documentation for users who notice unexpected project settings changes.
109-132: Clean separation of setup errors vs. run errors.The two-stage try structure correctly differentiates between exceptions during setup (caught and rethrown after cleanup) and exceptions during the test run (handled by the outer try-finally). Lock release and settings restoration occur exactly once in all paths.
97-105: Good: Avoids runSynchronously to prevent editor freezes.The async execution with callbacks pattern is correct. As noted in the PR description,
runSynchronouslycan freeze Unity in some projects, so this approach is the safer choice for automation scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Services/TestRunnerService.cs (2)
61-133: PlayMode guard and domain‑reload handling look correct; consider timeout resilience of the service lock
- The early guard against
EditorApplication.isPlaying || EditorApplication.isPlayingOrWillChangePlaymodeis a good safety net: it prevents starting a managed run while the user is already in (or entering) Play Mode and surfaces a clear error back to the caller.- The
adjustedPlayModeOptions/originalEnterPlayModeOptions*pattern plus the splittry/catchand outertry/finallycorrectly avoid double restore and ensure PlayMode options are restored both on start‑up failure and after the run Task completes or faults.- Holding
_operationLockfrom just before scheduling the run until afterrunTaskcompletes maintains the invariant of “one test operation at a time” across both EditMode and PlayMode runs.One thing to keep in mind (pre‑existing but still relevant): if, for any reason, Unity never invokes
RunFinishedand_runCompletionSourceis never completed, this method will hold_operationLockindefinitely. As a future improvement, you might consider adding an internal timeout/cancellation inTestRunnerServiceitself to ensure the semaphore is eventually released even when the upstream TestRunner misbehaves.Please confirm that in your manual testing (including scenarios similar to issue #367) you are always seeing
RunFinishedfire and the lock released, even when runs fail.
222-245: Dirty scene pre‑save works; consider stricter handling of unsaved scenesThe
SaveDirtyScenesIfNeededhelper is a good, targeted mitigation:
- Iterating
SceneManager.sceneCountand saving only dirty scenes with a non‑emptypathavoids theSaveModifiedSceneTaskhit for typical cases.- Warnings via
McpLog.Warnon unsaved scenes or failed saves give clear feedback without hard‑crashing the run.One behavioral gap to be aware of: if there are dirty unsaved scenes (no path), they remain dirty after this helper and the subsequent TestRunner pipeline may still trigger the problematic
SaveModifiedSceneTaskbehavior you’re working around. As a follow‑up improvement, you might want to either (a) fail fast when such unsaved dirty scenes are detected (returning a clear error) or (b) surface a stronger, user‑facing warning through the MCP response so it’s obvious that PlayMode runs are not reliable until those scenes are saved.Please verify the behavior when running PlayMode tests with:
- Only dirty saved scenes (should auto‑save and avoid the original exception).
- At least one dirty unsaved scene (confirm what Unity does and whether additional guarding is desirable).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Services/TestRunnerService.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Services/TestRunnerService.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Services/TestRunnerService.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Services/TestRunnerService.cs (4)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (2)
Task(18-36)Task(46-74)MCPForUnity/Editor/Tools/RunTests.cs (1)
Task(18-71)MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Warn(43-46)MCPForUnity/Editor/Tools/ManageScene.cs (1)
SaveScene(276-330)
🔇 Additional comments (3)
MCPForUnity/Editor/Services/TestRunnerService.cs (3)
8-11: Scene management usings are appropriate and scoped to new helpersThe added
UnityEditor.SceneManagementandUnityEngine.SceneManagementusings line up with the new PlayMode scene‑saving logic and don’t leak any unnecessary dependencies. No issues here.
34-59: Using ConfigureAwait(true) with the operation semaphore is correct for Unity editor APIsSwitching
_operationLock.WaitAsync()toConfigureAwait(true)here keeps continuations on the Unity editor context, which is required for the subsequent TestRunner/Editor calls. The lock usage and try/finally release pattern remain sound.Please verify by running a few
GetTestscommands while interacting with the editor (window focused/unfocused) to ensure there are no unexpected context/thread warnings from Unity.
193-221: PlayMode option toggling is minimal and safely reversible
EnsurePlayModeRunsWithoutDomainReloadandRestoreEnterPlayModeOptionscorrectly:
- Capture the original
enterPlayModeOptionsEnabledandenterPlayModeOptions.- Only mutate editor state when needed (options disabled or domain reload not already disabled).
- Restore both the options and the enabled flag on all error and completion paths in
RunTestsAsync.This keeps the MCP behavior scoped to the automated PlayMode run and avoids permanently altering project settings.
Confirm this behavior on a project where Enter Play Mode Options are already enabled with domain reload disabled, and one where they are disabled, to ensure no unexpected persistence of settings after the run.
* 'main' of https://github.com/CoplayDev/unity-mcp: Harden PlayMode test runs (CoplayDev#396) Enable the `rmcp_client` feature so it works with Codex CLI (CoplayDev#395) chore: bump version to 8.0.0 HTTP Server, uvx, C# only custom tools (CoplayDev#375) [CUSTOM TOOLS] Roslyn Runtime Compilation Feature (CoplayDev#371)
1 participant
Should fix #367
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.