Update TestRunnerService.cs#936
Merged
Scriptwonder merged 1 commit intoCoplayDev:betafrom Mar 15, 2026
Merged
Conversation
Contributor
Reviewer's GuideAdds a PlayModeOptionsGuard helper that persists and restores Unity Editor play mode options across domain reloads and editor crashes, and wires it into the test runner lifecycle to ensure original settings are always restored after interrupted or completed test runs. Updated class diagram for TestRunnerService and PlayModeOptionsGuardclassDiagram
class PlayModeOptionsGuard {
<<static>>
-const string KeyPending
-const string KeyEnabled
-const string KeyOptions
-static string MarkerPath
+static bool IsPending
+static PlayModeOptionsGuard()
+static void Save(bool originalEnabled, EnterPlayModeOptions originalOptions)
+static void Restore()
+static void Clear()
-static bool TryLoad(out bool originalEnabled, out EnterPlayModeOptions originalOptions)
}
class TestRunnerService {
+void RunFinished(ITestResultAdaptor result)
-static bool EnsurePlayModeRunsWithoutDomainReload(bool originalEnterPlayModeOptionsEnabled, EnterPlayModeOptions originalEnterPlayModeOptions)
-static void RestoreEnterPlayModeOptions(bool originalEnabled, EnterPlayModeOptions originalOptions)
-TaskCompletionSource~TestRunPayload~ _runCompletionSource
}
class SessionState {
+static void SetBool(string key, bool value)
+static bool GetBool(string key, bool defaultValue)
+static void SetInt(string key, int value)
+static int GetInt(string key, int defaultValue)
}
class EditorSettings {
+static bool enterPlayModeOptionsEnabled
+static EnterPlayModeOptions enterPlayModeOptions
}
class McpLog {
+static void Warn(string message)
+static void Info(string message)
}
class TestRunStatus {
+static bool IsRunning
}
class File {
+static void WriteAllText(string path, string contents)
+static bool Exists(string path)
+static string[] ReadAllLines(string path)
+static void Delete(string path)
}
TestRunnerService --> PlayModeOptionsGuard : uses
PlayModeOptionsGuard --> SessionState : persists_state
PlayModeOptionsGuard --> EditorSettings : restores_options
PlayModeOptionsGuard --> McpLog : logs_status
PlayModeOptionsGuard --> File : marker_file_io
PlayModeOptionsGuard --> TestRunStatus : checks_IsRunning
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR enhances TestRunnerService.cs with a new PlayModeOptionsGuard utility that persists and recovers Unity Editor play mode options across domain reloads and crashes using SessionState and marker files. The integration adds state-saving when entering play mode and restoration logic upon test completion to prevent permanent mutations of EditorSettings during interrupted test runs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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:
- The
MarkerPathcurrently uses a relative "Library" directory, which can be fragile if the working directory changes; consider resolving it viaApplication.dataPath(or another Unity API) to compute an absolute path under the project root. - In
TryLoad, when the marker file is malformed or contains invalid ints, the method silently returnsfalse; you might want to log a warning and clear the corrupt marker to avoid repeatedly hitting the same bad state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `MarkerPath` currently uses a relative "Library" directory, which can be fragile if the working directory changes; consider resolving it via `Application.dataPath` (or another Unity API) to compute an absolute path under the project root.
- In `TryLoad`, when the marker file is malformed or contains invalid ints, the method silently returns `false`; you might want to log a warning and clear the corrupt marker to avoid repeatedly hitting the same bad state.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Services/TestRunnerService.cs" line_range="119-116" />
<code_context>
+ return false;
+ }
+
+ string[] lines = File.ReadAllLines(MarkerPath);
+ if (lines.Length < 2)
+ {
+ return false;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Consider clearing or replacing a malformed marker file instead of silently retrying every load.
Right now, if the file exists but is malformed (too few lines or non‑integer values), `TryLoad` just returns `false` and leaves the bad file in place, so every reload hits the same failure. It would be more robust to treat this as corruption and invoke `Clear()` or delete the file so the system can recover instead of repeatedly parsing invalid content.
Suggested implementation:
```csharp
string[] lines = File.ReadAllLines(MarkerPath);
if (lines.Length < 2)
{
// Marker file is malformed; treat as corruption and clear it
File.Delete(MarkerPath);
return false;
}
```
To fully apply your suggestion, similar handling should be added around any parsing of `lines` later in this method. Specifically:
1. Wrap the parsing logic (e.g., `int.Parse`, `Enum.Parse`, etc.) in a `try/catch` block catching `FormatException` and `OverflowException`.
2. Inside the `catch`, delete the marker file with `File.Delete(MarkerPath);` and `return false;`.
3. Ensure this logic is only applied when the file exists (which it already is, given the preceding `File.Exists(MarkerPath)` check).
This will prevent the system from repeatedly attempting to parse a permanently malformed marker file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| { | ||
| if (!File.Exists(MarkerPath)) | ||
| { | ||
| return false; |
Contributor
There was a problem hiding this comment.
suggestion: Consider clearing or replacing a malformed marker file instead of silently retrying every load.
Right now, if the file exists but is malformed (too few lines or non‑integer values), TryLoad just returns false and leaves the bad file in place, so every reload hits the same failure. It would be more robust to treat this as corruption and invoke Clear() or delete the file so the system can recover instead of repeatedly parsing invalid content.
Suggested implementation:
string[] lines = File.ReadAllLines(MarkerPath);
if (lines.Length < 2)
{
// Marker file is malformed; treat as corruption and clear it
File.Delete(MarkerPath);
return false;
}
To fully apply your suggestion, similar handling should be added around any parsing of lines later in this method. Specifically:
- Wrap the parsing logic (e.g.,
int.Parse,Enum.Parse, etc.) in atry/catchblock catchingFormatExceptionandOverflowException. - Inside the
catch, delete the marker file withFile.Delete(MarkerPath);andreturn false;. - Ensure this logic is only applied when the file exists (which it already is, given the preceding
File.Exists(MarkerPath)check).
This will prevent the system from repeatedly attempting to parse a permanently malformed marker file.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes issue #932 by persisting the original Unity Play Mode enter options so they can be restored after interruptions (domain reload, crash/force-quit), preventing the editor from getting stuck with modified settings.
Changes:
- Added
PlayModeOptionsGuardto persist originalEditorSettings.enterPlayModeOptionsEnabled/enterPlayModeOptionsviaSessionState+ aLibrary/marker file. - Restores persisted Play Mode options in
RunFinishedwhen the originalRunTestsAsynccaller was lost to a domain reload. - Clears persisted state when normal restoration runs in the
RunTestsAsyncfinally path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+38
to
+43
| // After domain reload or editor restart: if a restore is pending and no test run | ||
| // is active, restore now. TryLoad checks SessionState first, then the marker file. | ||
| if (TryLoad(out _, out _) && !TestRunStatus.IsRunning) | ||
| { | ||
| Restore(); | ||
| } |
Comment on lines
+122
to
+136
| return false; | ||
| } | ||
|
|
||
| if (!int.TryParse(lines[0].Trim(), out int enabledInt) || | ||
| !int.TryParse(lines[1].Trim(), out int optionsInt)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| originalEnabled = enabledInt != 0; | ||
| originalOptions = (EnterPlayModeOptions)optionsInt; | ||
| return true; | ||
| } | ||
| catch | ||
| { |
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
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.
Fix on issue#932, prevent the two params being the same after Domain Reload or Editor Crash/Force Quit by storing a Library/ mark file and a SessionState.
Summary by Sourcery
Add a guard to persist and restore Unity play mode options across domain reloads or editor crashes during test runs to prevent leaving modified settings behind.
Enhancements:
Summary by CodeRabbit