Add configurable init_timeout for PlayMode test initialization#1021
Open
Emerix wants to merge 3 commits intoCoplayDev:betafrom
Open
Add configurable init_timeout for PlayMode test initialization#1021Emerix wants to merge 3 commits intoCoplayDev:betafrom
Emerix wants to merge 3 commits intoCoplayDev:betafrom
Conversation
PlayMode tests require entering play mode which triggers a domain reload. On large projects this can take >15s, causing the hardcoded 15s init timeout to auto-fail the test job before tests actually start. This adds an `init_timeout` parameter to `run_tests` that flows through the Python server → C# RunTests handler → TestJobManager. When set, the per-job timeout overrides the 15s default. The value is persisted across domain reloads via SessionState. Changes: - Python: Add `init_timeout` param to `run_tests()` function signature - C# RunTests: Read `initTimeout` param and pass to `StartJob()` - C# TestJobManager: Per-job `InitTimeoutMs` field with fallback to `DefaultInitializationTimeoutMs` (15s), persisted in SessionState Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds a configurable initialization timeout for Unity PlayMode test jobs, threading an optional init_timeout parameter from the Python run_tests tool through the C# RunTests handler into TestJobManager, which now supports per-job init timeouts with persistence across domain reloads while keeping the default at 15s. Sequence diagram for configurable init_timeout through test job lifecyclesequenceDiagram
actor Developer
participant PythonRunTests as Python_run_tests_tool
participant MCPServer as MCP_Server
participant RunTestsHandler as RunTests_HandleCommand
participant TestJobManager as TestJobManager
Developer->>PythonRunTests: run_tests(mode=PlayMode, init_timeout=120000)
PythonRunTests->>MCPServer: Request with init_timeout
MCPServer->>RunTestsHandler: HandleCommand(params)
RunTestsHandler->>RunTestsHandler: initTimeoutMs = Params.GetInt(initTimeout) ?? 0
RunTestsHandler->>TestJobManager: StartJob(mode, filterOptions, initTimeoutMs)
TestJobManager->>TestJobManager: Create TestJob with InitTimeoutMs = initTimeoutMs
TestJobManager->>TestJobManager: PersistToSessionState(init_timeout_ms)
loop Poll job status
MCPServer->>TestJobManager: GetJob(jobId)
TestJobManager->>TestJobManager: initTimeout = InitTimeoutMs > 0 ? InitTimeoutMs : DefaultInitializationTimeoutMs
TestJobManager->>TestJobManager: Check now - StartedUnixMs > initTimeout
alt Exceeded initTimeout
TestJobManager->>TestJobManager: Status = Failed, Error = initialization timeout
else Within initTimeout
TestJobManager-->>MCPServer: Running or Completed status
end
end
TestJobManager->>TestJobManager: PersistToSessionState(init_timeout_ms)
Class diagram for updated TestJobManager and RunTests init timeout handlingclassDiagram
class TestJob {
string JobId
TestJobStatus Status
TestMode Mode
long StartedUnixMs
long FinishedUnixMs
long LastProgressUnixMs
long? LastFinishedUnixMs
List~TestJobFailure~ FailuresSoFar
string Error
TestRunResult Result
long InitTimeoutMs
}
class PersistedJob {
string job_id
string status
string mode
long started_unix_ms
long? finished_unix_ms
string last_finished_test_full_name
long? last_finished_unix_ms
List~TestJobFailure~ failures_so_far
string error
long init_timeout_ms
}
class TestJobManager {
<<static>>
const int FailureCap
const long StuckThresholdMs
const long DefaultInitializationTimeoutMs
const int MaxJobsToKeep
const long MinPersistIntervalMs
static string StartJob(TestMode mode, TestFilterOptions filterOptions, long initTimeoutMs)
static TestJob GetJob(string jobId)
static void TryRestoreFromSessionState()
static void PersistToSessionState(bool force)
}
class RunTests {
static Task~object~ HandleCommand(JObject params)
}
class ParamsHelper {
bool GetBool(string key)
int? GetInt(string key)
string GetString(string key)
}
RunTests --> TestJobManager : calls StartJob(mode, filterOptions, initTimeoutMs)
TestJobManager o-- TestJob : manages
TestJobManager o-- PersistedJob : serializes to SessionState
RunTests --> ParamsHelper : uses to read initTimeout
PersistedJob --> TestJob : maps init_timeout_ms to InitTimeoutMs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughPer-job initialization timeout support added: Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
Client->>Server: POST run_tests (mode, ..., init_timeout?)
end
rect rgba(240,255,220,0.5)
Server->>MCP Tool: send "run_tests" command with params (initTimeout if >0)
end
rect rgba(255,240,220,0.5)
MCP Tool / Unity Editor->>TestJobManager: StartJob(mode, filterOptions, initTimeoutMs)
Note right of TestJobManager: StartJob clamps value and sets TestJob.InitTimeoutMs
TestJobManager->>SessionState: Persist job including init_timeout_ms
TestJobManager->>TestJobManager: GetJob periodically checks init timeout using per-job InitTimeoutMs (if >0) or default
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 left some high level feedback:
- Consider validating/sanitizing the
initTimeoutMsvalue inRunTests.HandleCommand/TestJobManager.StartJob(e.g., clamp to a reasonable min/max and treat non-positive values as0) so that non-Python callers or malformed input can't set an accidentally huge or negative initialization timeout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating/sanitizing the `initTimeoutMs` value in `RunTests.HandleCommand`/`TestJobManager.StartJob` (e.g., clamp to a reasonable min/max and treat non-positive values as `0`) so that non-Python callers or malformed input can't set an accidentally huge or negative initialization timeout.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Services/TestJobManager.cs (1)
301-325: 🛠️ Refactor suggestion | 🟠 MajorAdd Unity tests for
initTimeoutMsoverride and domain-reload restore functionality.The feature is implemented (parameter passing, persistence, restoration, timeout enforcement) but completely lacks test coverage. Tests should verify:
- Override applied when
initTimeoutMs > 0is passed- Default fallback to
DefaultInitializationTimeoutMs(15 seconds) wheninitTimeoutMs == 0- Persistence and restoration of
initTimeoutMsafter domain reloadAdd tests in
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/RunTestsTests.csor create a dedicatedTestJobManagerTests.csfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/TestJobManager.cs` around lines 301 - 325, Add EditMode unit tests exercising StartJob's initTimeoutMs behavior: one test calls TestJobManager.StartJob(mode, filterOptions, initTimeoutMs>0) and asserts the returned/created TestJob has InitTimeoutMs equal to the passed value; a second test calls StartJob with initTimeoutMs==0 and asserts the TestJob.InitTimeoutMs equals TestJobManager.DefaultInitializationTimeoutMs (15s). Add a third test for persistence by starting a job with a nonzero initTimeoutMs, forcing the manager's persistence path to run, then simulate a domain-reload restore by invoking the TestJobManager restoration method (the same method used on domain reload) and assert the restored TestJob still has the original InitTimeoutMs. Reference StartJob, TestJob.InitTimeoutMs, DefaultInitializationTimeoutMs and the manager's persistence/restore methods in your new tests.
🧹 Nitpick comments (1)
Server/src/services/tools/run_tests.py (1)
170-172: Validate explicit invalidinit_timeoutvalues instead of silently dropping them.If callers pass
init_timeout <= 0, the current path ignores it and proceeds. Returning a clear error makes misconfiguration obvious.Proposed adjustment
- if init_timeout is not None and init_timeout > 0: - params["initTimeout"] = init_timeout + if init_timeout is not None: + if init_timeout <= 0: + return MCPResponse( + success=False, + error="init_timeout must be a positive integer (milliseconds)", + ) + params["initTimeout"] = init_timeoutAlso applies to: 203-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/run_tests.py` around lines 170 - 172, The init_timeout parameter should be validated so invalid values (<= 0) are not silently ignored; in the function signature/parameter handling for init_timeout (and the other occurrence around lines 203-204 where init_timeout is used) add a guard that if init_timeout is not None and init_timeout <= 0 you raise a ValueError (or return an explicit error) with a clear message like "init_timeout must be > 0 or None" instead of dropping the value—update validation in the run_tests entrypoint/handler that declares init_timeout to enforce this rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@MCPForUnity/Editor/Services/TestJobManager.cs`:
- Around line 301-325: Add EditMode unit tests exercising StartJob's
initTimeoutMs behavior: one test calls TestJobManager.StartJob(mode,
filterOptions, initTimeoutMs>0) and asserts the returned/created TestJob has
InitTimeoutMs equal to the passed value; a second test calls StartJob with
initTimeoutMs==0 and asserts the TestJob.InitTimeoutMs equals
TestJobManager.DefaultInitializationTimeoutMs (15s). Add a third test for
persistence by starting a job with a nonzero initTimeoutMs, forcing the
manager's persistence path to run, then simulate a domain-reload restore by
invoking the TestJobManager restoration method (the same method used on domain
reload) and assert the restored TestJob still has the original InitTimeoutMs.
Reference StartJob, TestJob.InitTimeoutMs, DefaultInitializationTimeoutMs and
the manager's persistence/restore methods in your new tests.
---
Nitpick comments:
In `@Server/src/services/tools/run_tests.py`:
- Around line 170-172: The init_timeout parameter should be validated so invalid
values (<= 0) are not silently ignored; in the function signature/parameter
handling for init_timeout (and the other occurrence around lines 203-204 where
init_timeout is used) add a guard that if init_timeout is not None and
init_timeout <= 0 you raise a ValueError (or return an explicit error) with a
clear message like "init_timeout must be > 0 or None" instead of dropping the
value—update validation in the run_tests entrypoint/handler that declares
init_timeout to enforce this rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f29e7e8-145c-4496-887a-6f3b43f421bc
📒 Files selected for processing (5)
MCPForUnity/Editor/Services/TestJobManager.csMCPForUnity/Editor/Tools/RunTests.csServer/src/services/tools/run_tests.pydocs/development/README-DEV-zh.mddocs/development/README-DEV.md
- Clamp initTimeoutMs in StartJob: negative values → 0, cap at 600s - Python: reject init_timeout <= 0 with explicit error before calling Unity - Add 3 C# EditMode tests for per-job InitTimeoutMs behavior (custom timeout, default timeout auto-fail, persist/restore) - Add 4 Python tests for init_timeout forwarding and validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.cs (2)
23-24: Remove unused field_originalJobs.This field is declared but never assigned or used anywhere in the class.
🧹 Remove dead code
private string _originalJobId; - private object _originalJobs;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.cs` around lines 23 - 24, The private field _originalJobs in the TestJobManagerInitTimeoutTests class is declared but never used; remove the declaration "private object _originalJobs;" from the class to eliminate dead code and any unused field warning, ensuring you only change the field declaration and not other test logic or setup/teardown methods.
97-124: Potential test flakiness if Editor is compiling or updating.The
GetJobauto-fail logic inTestJobManager.cs(line 505) checks!EditorApplication.isCompiling && !EditorApplication.isUpdatingbefore applying the timeout. If either condition is true during test execution, the job won't auto-fail and this test will fail unexpectedly.This is unlikely in a controlled EditMode test environment, but worth noting. If flakiness is observed, consider adding a brief delay or explicit check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.cs` around lines 97 - 124, The test can be flaky because TestJobManager.GetJob checks EditorApplication.isCompiling and EditorApplication.isUpdating before applying the timeout; modify the test GetJob_WithDefaultTimeout_AutoFailsAfter15Seconds to wait until EditorApplication.isCompiling and EditorApplication.isUpdating are false before invoking _getJobMethod (or explicitly poll with a short timeout and small delays), so the auto-fail path in TestJobManager.GetJob will be exercised deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.cs`:
- Around line 23-24: The private field _originalJobs in the
TestJobManagerInitTimeoutTests class is declared but never used; remove the
declaration "private object _originalJobs;" from the class to eliminate dead
code and any unused field warning, ensuring you only change the field
declaration and not other test logic or setup/teardown methods.
- Around line 97-124: The test can be flaky because TestJobManager.GetJob checks
EditorApplication.isCompiling and EditorApplication.isUpdating before applying
the timeout; modify the test GetJob_WithDefaultTimeout_AutoFailsAfter15Seconds
to wait until EditorApplication.isCompiling and EditorApplication.isUpdating are
false before invoking _getJobMethod (or explicitly poll with a short timeout and
small delays), so the auto-fail path in TestJobManager.GetJob will be exercised
deterministically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27b7cef6-d43c-4a0a-b4df-05acf680c2a7
📒 Files selected for processing (5)
MCPForUnity/Editor/Services/TestJobManager.csServer/src/services/tools/run_tests.pyServer/tests/integration/test_run_tests_async.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.cs.meta
✅ Files skipped from review due to trivial changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerInitTimeoutTests.cs.meta
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/tools/run_tests.py
- Remove unused _originalJobs field - Add Assume.That guards for EditorApplication.isCompiling/isUpdating so the test is skipped (inconclusive) rather than producing misleading results when the editor is mid-compilation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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.
Description
PlayMode tests require entering play mode which triggers a domain reload. On large Unity projects this can take >15s, causing the hardcoded 15s init timeout to
auto-fail the test job before tests actually start. This PR adds a configurable
init_timeoutparameter torun_testsso callers can specify a longerinitialization timeout for PlayMode tests.
Type of Change
Changes Made
Server/src/services/tools/run_tests.py): Addedinit_timeoutparameter torun_tests()function signature. When set, forwarded to Unity asinitTimeoutin the params dict.MCPForUnity/Editor/Tools/RunTests.cs): ReadsinitTimeoutparam from the request and passes it toTestJobManager.StartJob().MCPForUnity/Editor/Services/TestJobManager.cs):InitTimeoutMsfield toTestJobfor per-job timeout overrideInitializationTimeoutMstoDefaultInitializationTimeoutMs(15s, unchanged default)StartJob()accepts optionalinitTimeoutMsparameterGetJob()uses per-job timeout when set, falls back to 15s defaultInitTimeoutMspersisted/restored across domain reloads via SessionStateTesting/Screenshots/Recordings
Tested on a large Unity project (~18s domain reload time for PlayMode):
init_timeout=120000init_timeout(default 15s is sufficient)Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
None
Additional Notes
The default timeout remains 15s (no breaking change). The
init_timeoutparameter is optional and only needed for projects where PlayMode domain reload exceeds15s. Recommended value for PlayMode: 120000 (120s).
Summary by Sourcery
Add configurable per-job initialization timeout for Unity test runs and propagate it from the Python run_tests tool through the editor pipeline.
New Features:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests