calling TestRunStatus.MarkFinished() to trigger callbacks#1067
Open
miketalley wants to merge 2 commits intoCoplayDev:betafrom
Open
calling TestRunStatus.MarkFinished() to trigger callbacks#1067miketalley wants to merge 2 commits intoCoplayDev:betafrom
miketalley wants to merge 2 commits intoCoplayDev:betafrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures that when a test job auto-fails due to initialization timeout, the global TestRunStatus is also marked finished so the system no longer thinks tests are perpetually running. Sequence diagram for test job auto-fail timeout handlingsequenceDiagram
actor User
participant Tool as Tool_run_tests
participant TestRunnerService
participant TestJobManager
participant TestRunStatus
participant EditorStateCache
User->>Tool: run_tests
Tool->>TestRunnerService: RunTestsAsync
TestRunnerService->>TestRunStatus: MarkStarted
TestRunnerService->>TestJobManager: Dispatch test job
TestRunnerService-->>Tool: job_id
rect rgb(255, 230, 230)
Note over TestJobManager,TestRunStatus: Initialization timeout auto-fail path
Tool->>TestJobManager: GetJob(job_id)
TestJobManager->>TestJobManager: Detect initialization timeout
TestJobManager->>TestJobManager: Set job status failed
TestJobManager->>TestJobManager: _currentJobId = null
TestJobManager->>TestRunStatus: MarkFinished
TestJobManager-->>Tool: Job status failed (failed_to_initialize)
end
Tool->>EditorStateCache: get_editor_state
EditorStateCache->>TestRunStatus: isRunning
EditorStateCache-->>Tool: tests.is_running = false
Class diagram for TestJobManager and TestRunStatus timeout fixclassDiagram
class TestJobManager {
<<static>>
- string _currentJobId
+ static TestJob GetJob(string jobId)
}
class TestRunStatus {
<<static>>
- bool _isRunning
+ static void MarkStarted(string jobId)
+ static void MarkFinished()
+ static bool IsRunning()
}
class EditorStateCache {
+ bool tests_is_running
+ string tests_current_job_id
+ string tests_started_by
+ void RefreshFromStatus()
}
TestJobManager ..> TestRunStatus : uses
EditorStateCache ..> TestRunStatus : reads
EditorStateCache ..> TestJobManager : reads current_job_id
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA single method in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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:
- In the timeout/auto-fail block,
TestRunStatus.MarkFinished()is invoked even when_currentJobId != jobId, which could prematurely clear the running flag for a newer job; consider only callingMarkFinishedwhen the timed-out job is still the active one. - Given this is a subtle lifecycle edge case, consider adding a brief code comment near
TestRunStatus.MarkFinished()to explain that it is needed to keepTestRunStatusin sync when initialization times out withoutRunStarted/RunFinishedfiring.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the timeout/auto-fail block, `TestRunStatus.MarkFinished()` is invoked even when `_currentJobId != jobId`, which could prematurely clear the running flag for a newer job; consider only calling `MarkFinished` when the timed-out job is still the active one.
- Given this is a subtle lifecycle edge case, consider adding a brief code comment near `TestRunStatus.MarkFinished()` to explain that it is needed to keep `TestRunStatus` in sync when initialization times out without `RunStarted`/`RunFinished` firing.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.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/TestJobManager.cs (1)
505-505: Consider applying the sameMarkFinished()fix to the other auto-fail paths.The same leak can occur via three other paths that transition a
Runningjob toFailedwithout going throughRunFinished:
FinalizeFromTask(L643-654) — the comment at L349-350 explicitly notes this is the safety net "in case RunFinished is not delivered". In exactly that case,TestRunStatus.MarkFinished()will never have been called, so_isRunningstays true.ClearStuckJob(L103-112) — manual clear path;MarkFinishedis never invoked.TryRestoreFromSessionStatestale-job cleanup (L220-231) — domain-reload orphan path.Paths 2 and 3 may be partially covered by
TestRunStatusbeing reset on domain reload, butFinalizeFromTaskin particular reproduces the exact same symptom described in the PR (stucktests.is_running: true) wheneverRunFinishedis dropped for reasons other than the 15s init timeout.♻️ Suggested additions
@@ FinalizeFromTask, after setting Status = Failed/Succeeded and clearing _currentJobId if (_currentJobId == jobId) { _currentJobId = null; } + // Mirror the RunFinished path: ensure TestRunStatus is cleared even when + // this safety-net finalizer runs because RunFinished was not delivered. + if (existing.Status != TestJobStatus.Running) + { + TestRunStatus.MarkFinished(); + } } PersistToSessionState(force: true);@@ ClearStuckJob _currentJobId = null; } + TestRunStatus.MarkFinished(); PersistToSessionState(force: true);@@ TryRestoreFromSessionState stale-job cleanup currentJob.FinishedUnixMs = now; _currentJobId = null; + TestRunStatus.MarkFinished(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/TestJobManager.cs` at line 505, The TestRunStatus.MarkFinished() call added at the RunFinished path must also be invoked in the other auto-fail transitions to avoid leaving _isRunning true: add TestRunStatus.MarkFinished() into FinalizeFromTask (the safety-net path that can run when RunFinished is dropped), into ClearStuckJob (manual clear path), and into the stale-job cleanup in TryRestoreFromSessionState; update those functions to call TestRunStatus.MarkFinished() immediately after you set the job to Failed (or right before returning) so the running flag is cleared in all failure paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Services/TestJobManager.cs`:
- Line 505: The TestRunStatus.MarkFinished() call added at the RunFinished path
must also be invoked in the other auto-fail transitions to avoid leaving
_isRunning true: add TestRunStatus.MarkFinished() into FinalizeFromTask (the
safety-net path that can run when RunFinished is dropped), into ClearStuckJob
(manual clear path), and into the stale-job cleanup in
TryRestoreFromSessionState; update those functions to call
TestRunStatus.MarkFinished() immediately after you set the job to Failed (or
right before returning) so the running flag is cleared in all failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbdd4cb8-cdfc-4bc4-b49e-1d032ae1f707
📒 Files selected for processing (1)
MCPForUnity/Editor/Services/TestJobManager.cs
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
When a PlayMode test job fails to initialize (test runner never reaches
RunStarted),TestJobManager.GetJobauto-fails the job afterInitializationTimeoutMs— but only updates the job record, notTestRunStatus. BecauseTestRunStatus.MarkStarted(...)was called synchronously at run dispatch and theRunStarted/RunFinishedcallbacks never fire,TestRunStatus._isRunningstaystrueforever.Downstream effect:
EditorStateCachereportstests.is_running: true,advice.blocking_reasons: ["running_tests"], andadvice.ready_for_tools: false. Every subsequentrun_tests,refresh_unity, and any other tool that gates on readiness returnsbusy: tests_running. The only recovery is restarting Unity — neither a Test Runner UI cancel normanage_editor stopclears the flag because neither path invokesTestRunStatus.MarkFinished.Type of Change
Bug fix (non-breaking change that fixes an issue)
Changes Made
One line, in
TestJobManager.GetJob, inside the auto-fail block (right after_currentJobId = null;):Symmetrical to the existing clear paths in
TestRunnerService.RunFinished(line 204) andTestRunnerService.ExecuteAsync's catch block (line 129). KeepsTestRunStatusand the job record in sync under all completion paths.Repro
run_testsright after a domain reload (e.g., immediately afterrefresh_unity) or while Unity is unfocused enough to miss its test-init window.editor_state.tests.started_unix_ms + 15s.get_test_jobreturnsstatus: failed, error: "Test job failed to initialize (tests did not start within timeout)".editor_statenow showstests.is_running: truewithtests.current_job_id: nullandtests.started_by: "unknown"— permanently stuck.run_testsreturnsbusy: tests_running.Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit