From fea3679fadc8612e00810d1e935ea596907d58d5 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Mon, 13 Jul 2026 14:48:28 +0900 Subject: [PATCH 1/3] fix: run-tests now times out hung Test Runner waits instead of holding the tool slot for 30 minutes (#1739) Co-authored-by: Cursor --- .agents/skills/uloop-run-tests/SKILL.md | 1 + .claude/skills/uloop-run-tests/SKILL.md | 1 + Assets/Tests/Editor/RunTestsUseCaseTests.cs | 52 +++++++ .../RunTests/RunTestsExecutionTimeout.cs | 97 +++++++++++++ .../RunTests/RunTestsExecutionTimeout.cs.meta | 11 ++ .../RunTests/RunTestsSchema.cs | 5 + .../RunTests/RunTestsUseCase.cs | 38 +++++- .../FirstPartyTools/RunTests/Skill/SKILL.md | 1 + cli/common/tools/default-tools.json | 5 + .../RunTestsExecutionTimeout.UnitTests.csproj | 19 +++ .../RunTestsExecutionTimeoutTests.cs | 128 ++++++++++++++++++ 11 files changed, 352 insertions(+), 6 deletions(-) create mode 100644 Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs create mode 100644 Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs.meta create mode 100644 tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj create mode 100644 tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs diff --git a/.agents/skills/uloop-run-tests/SKILL.md b/.agents/skills/uloop-run-tests/SKILL.md index 1ee2b80f4..45d2de971 100644 --- a/.agents/skills/uloop-run-tests/SKILL.md +++ b/.agents/skills/uloop-run-tests/SKILL.md @@ -30,6 +30,7 @@ uloop run-tests [options] | `--filter-type` | string | `all` | Filter type: `all`, `exact`, `regex`, `assembly` | | `--filter-value` | string | - | Filter value (test name, pattern, or assembly) | | `--fail-on-unsaved-changes` | flag | - | Fail before test execution if unsaved editor changes remain instead of auto-saving them | +| `--timeout-seconds` | integer | `600` | Maximum seconds to wait for RunFinished before canceling the await (max `1500`). Increase for long suites; on timeout the Test Runner may still be running until stop handling lands | ## Output diff --git a/.claude/skills/uloop-run-tests/SKILL.md b/.claude/skills/uloop-run-tests/SKILL.md index 1ee2b80f4..45d2de971 100644 --- a/.claude/skills/uloop-run-tests/SKILL.md +++ b/.claude/skills/uloop-run-tests/SKILL.md @@ -30,6 +30,7 @@ uloop run-tests [options] | `--filter-type` | string | `all` | Filter type: `all`, `exact`, `regex`, `assembly` | | `--filter-value` | string | - | Filter value (test name, pattern, or assembly) | | `--fail-on-unsaved-changes` | flag | - | Fail before test execution if unsaved editor changes remain instead of auto-saving them | +| `--timeout-seconds` | integer | `600` | Maximum seconds to wait for RunFinished before canceling the await (max `1500`). Increase for long suites; on timeout the Test Runner may still be running until stop handling lands | ## Output diff --git a/Assets/Tests/Editor/RunTestsUseCaseTests.cs b/Assets/Tests/Editor/RunTestsUseCaseTests.cs index 3628ed41c..36d0d5016 100644 --- a/Assets/Tests/Editor/RunTestsUseCaseTests.cs +++ b/Assets/Tests/Editor/RunTestsUseCaseTests.cs @@ -48,6 +48,58 @@ public async Task ExecuteAsync_WithInvalidExecutionState_ShouldFailFastWithoutRu Assert.That(validationService.SaveBeforeRun, Is.True); } + [Test] + public async Task ExecuteAsync_WithNonPositiveTimeoutSeconds_ShouldFailFastWithoutRunningTests() + { + // Verifies invalid TimeoutSeconds is rejected before validation or Test Runner work. + StubTestExecutionService executionService = new(); + StubTestExecutionStateValidationService validationService = new(ValidationResult.Success()); + RunTestsUseCase useCase = new( + new TestFilterCreationService(), + executionService, + validationService, + waitForTestRunnerCleanupAsync: NoCleanupWait + ); + RunTestsSchema parameters = new() + { + TimeoutSeconds = 0 + }; + + RunTestsResponse response = await useCase.ExecuteAsync(parameters, CancellationToken.None); + + Assert.That(response.Success, Is.False); + Assert.That(response.Status, Is.EqualTo(RunTestsExecutionStatus.ExecutionFailed)); + Assert.That(response.Message, Does.Contain("greater than zero")); + Assert.That(executionService.WasCalled, Is.False); + Assert.That(validationService.WasCalled, Is.False); + } + + [Test] + public async Task ExecuteAsync_WithTimeoutSecondsAboveMax_ShouldFailFastWithoutRunningTests() + { + // Verifies TimeoutSeconds above MaxTimeoutSeconds fail before arming CancelAfter. + StubTestExecutionService executionService = new(); + StubTestExecutionStateValidationService validationService = new(ValidationResult.Success()); + RunTestsUseCase useCase = new( + new TestFilterCreationService(), + executionService, + validationService, + waitForTestRunnerCleanupAsync: NoCleanupWait + ); + RunTestsSchema parameters = new() + { + TimeoutSeconds = RunTestsExecutionTimeout.MaxTimeoutSeconds + 1 + }; + + RunTestsResponse response = await useCase.ExecuteAsync(parameters, CancellationToken.None); + + Assert.That(response.Success, Is.False); + Assert.That(response.Status, Is.EqualTo(RunTestsExecutionStatus.ExecutionFailed)); + Assert.That(response.Message, Does.Contain(RunTestsExecutionTimeout.MaxTimeoutSeconds.ToString())); + Assert.That(executionService.WasCalled, Is.False); + Assert.That(validationService.WasCalled, Is.False); + } + [Test] public async Task ExecuteAsync_WithUnknownTestMode_ShouldFailFastWithoutRunningTests() { diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs new file mode 100644 index 000000000..915646e2a --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs @@ -0,0 +1,97 @@ +using System; +using System.Diagnostics; +using System.Threading; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Bounds run-tests execution with a linked CancelAfter timeout so a missing + /// RunFinished callback cannot hold the single-flight tool slot until the CLI + /// absolute response limit. + /// + internal static class RunTestsExecutionTimeout + { + /// + /// Default budget for hung Test Runner detection. Chosen to free the slot well + /// before the CLI 30-minute absolute response timeout while leaving headroom + /// for normal agent-driven suites (seconds to a few minutes). + /// + public const int DefaultTimeoutSeconds = 600; + + /// + /// Upper bound kept below the CLI finalResponseTimeout (30 minutes) so the C# + /// CancelAfter always fires first when a caller asks for a long wait. + /// + public const int MaxTimeoutSeconds = 1500; + + /// + /// Validates TimeoutSeconds before creating a CancelAfter source. + /// + public static (bool IsValid, string ErrorMessage) Validate(int timeoutSeconds) + { + if (timeoutSeconds <= 0) + { + return ( + false, + "TimeoutSeconds must be greater than zero. " + + "Pass --timeout-seconds with a positive integer."); + } + + if (timeoutSeconds > MaxTimeoutSeconds) + { + return ( + false, + $"TimeoutSeconds must be less than or equal to {MaxTimeoutSeconds} " + + $"(CLI absolute response limit is 30 minutes). " + + $"Received: {timeoutSeconds}."); + } + + return (true, null); + } + + /// + /// Creates a linked token source that cancels after timeoutSeconds. + /// + public static CancellationTokenSource CreateLinkedTimeoutSource( + CancellationToken parentCancellationToken, + int timeoutSeconds) + { + Debug.Assert( + timeoutSeconds > 0 && timeoutSeconds <= MaxTimeoutSeconds, + "timeoutSeconds must be validated with Validate before CreateLinkedTimeoutSource"); + + CancellationTokenSource linkedCancellationTokenSource = + CancellationTokenSource.CreateLinkedTokenSource(parentCancellationToken); + // Why CancelAfter here: frees the single-flight slot when RunFinished never fires. + // PR 1-1 only completes the await via cancellation; TestRunnerApi may keep running + // until PR 1-2 routes this same cancel path through an explicit stop + PlayMode restore. + linkedCancellationTokenSource.CancelAfter(TimeSpan.FromSeconds(timeoutSeconds)); + return linkedCancellationTokenSource; + } + + /// + /// Builds the agent-facing message for a CancelAfter timeout. + /// + public static string CreateTimeoutMessage(int timeoutSeconds) + { + Debug.Assert(timeoutSeconds > 0, "timeoutSeconds must be greater than zero"); + + return + $"run-tests timed out after {timeoutSeconds} seconds without a RunFinished callback. " + + "The Unity Test Runner may still be running in the background. " + + "Increase --timeout-seconds for long suites, or restart with `uloop launch -r` if Unity is stuck."; + } + + /// + /// True when cancellation came from CancelAfter (or another linked child) rather + /// than the parent request/disconnect token. + /// + public static bool IsTimeoutCancellation(CancellationToken parentCancellationToken) + { + // Why parent-only check is enough today: the linked source has exactly two cancel + // causes (CancelAfter and parent/disconnect). If another linked cancel source is + // added later, also require timeoutCancellationTokenSource.IsCancellationRequested. + return !parentCancellationToken.IsCancellationRequested; + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs.meta b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs.meta new file mode 100644 index 000000000..fed775e31 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: ec4d1f084af74468c80eab93d0494817 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsSchema.cs b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsSchema.cs index ff1d8b511..36dba324f 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsSchema.cs +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsSchema.cs @@ -27,5 +27,10 @@ public class RunTestsSchema : UnityCliLoopToolSchema public string FilterValue { get; set; } = ""; public bool SaveBeforeRun { get; set; } = true; + /// + /// Maximum seconds to wait for Unity Test Runner RunFinished before canceling the await. + /// Kept below the CLI absolute response timeout so hung runs free the single-flight slot first. + /// + public int TimeoutSeconds { get; set; } = RunTestsExecutionTimeout.DefaultTimeoutSeconds; } } diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs index 197213947..aa9aaa7d9 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs @@ -73,6 +73,12 @@ public async Task ExecuteAsync(RunTestsSchema parameters, Canc return RunTestsResponse.CreateTestFrameworkUnavailable(); } + (bool isTimeoutValid, string timeoutError) = RunTestsExecutionTimeout.Validate(parameters.TimeoutSeconds); + if (!isTimeoutValid) + { + return CreateFailureResponse(timeoutError); + } + ValidationResult validation = _validationService.Validate(parameters.TestMode, parameters.SaveBeforeRun); if (!validation.IsValid) { @@ -100,18 +106,38 @@ public async Task ExecuteAsync(RunTestsSchema parameters, Canc // pause source is a human manually pausing via the Editor UI, which is native // Unity behavior and out of scope. ct.ThrowIfCancellationRequested(); + using CancellationTokenSource timeoutCancellationTokenSource = + RunTestsExecutionTimeout.CreateLinkedTimeoutSource(ct, parameters.TimeoutSeconds); + CancellationToken executionCt = timeoutCancellationTokenSource.Token; SerializableTestResult result; - if (parameters.TestMode == UnityCliLoopTestMode.PlayMode) + try { - result = await _executionService.ExecutePlayModeTestAsync(filter, ct); + if (parameters.TestMode == UnityCliLoopTestMode.PlayMode) + { + result = await _executionService.ExecutePlayModeTestAsync(filter, executionCt); + } + else + { + result = await _executionService.ExecuteEditModeTestAsync(filter, executionCt); + } + + // Why parent ct (not executionCt): CancelAfter only guards the RunFinished wait. + // Using the linked token here would mis-report a successful run as timed out when + // the fixed cleanup delay straddles the deadline after RunFinished already arrived. + await _waitForTestRunnerCleanupAsync(ct); } - else + catch (OperationCanceledException) when (RunTestsExecutionTimeout.IsTimeoutCancellation(ct)) { - result = await _executionService.ExecuteEditModeTestAsync(filter, ct); + // Why return a tool failure instead of rethrowing: agents need an actionable + // timeout message (extend --timeout-seconds / launch -r). Parent/disconnect + // cancellation still propagates so the IPC session can tear down normally. + // Why not stop TestRunnerApi here yet: PR 1-2 will route this same cancel path + // through explicit runner stop + PlayMode restore; until then the runner may + // keep running in the background after the await is released. + return CreateFailureResponse( + RunTestsExecutionTimeout.CreateTimeoutMessage(parameters.TimeoutSeconds)); } - await _waitForTestRunnerCleanupAsync(ct); - // 3. Response creation. RunTestsResponse response = new( success: result.success, diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/Skill/SKILL.md b/Packages/src/Editor/FirstPartyTools/RunTests/Skill/SKILL.md index 1ee2b80f4..45d2de971 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/Skill/SKILL.md +++ b/Packages/src/Editor/FirstPartyTools/RunTests/Skill/SKILL.md @@ -30,6 +30,7 @@ uloop run-tests [options] | `--filter-type` | string | `all` | Filter type: `all`, `exact`, `regex`, `assembly` | | `--filter-value` | string | - | Filter value (test name, pattern, or assembly) | | `--fail-on-unsaved-changes` | flag | - | Fail before test execution if unsaved editor changes remain instead of auto-saving them | +| `--timeout-seconds` | integer | `600` | Maximum seconds to wait for RunFinished before canceling the await (max `1500`). Increase for long suites; on timeout the Test Runner may still be running until stop handling lands | ## Output diff --git a/cli/common/tools/default-tools.json b/cli/common/tools/default-tools.json index c36516930..877cfa314 100644 --- a/cli/common/tools/default-tools.json +++ b/cli/common/tools/default-tools.json @@ -99,6 +99,11 @@ "type": "boolean", "description": "Save unsaved loaded Scene changes and current Prefab Stage changes before running tests", "default": true + }, + "TimeoutSeconds": { + "type": "integer", + "description": "Maximum seconds to wait for Unity Test Runner RunFinished before canceling the await and freeing the tool slot", + "default": 600 } } } diff --git a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj new file mode 100644 index 000000000..5230da9bb --- /dev/null +++ b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj @@ -0,0 +1,19 @@ + + + net10.0 + disable + disable + false + latest + + + + + + + + + + + + diff --git a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs new file mode 100644 index 000000000..16002ca66 --- /dev/null +++ b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs @@ -0,0 +1,128 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.UnitTests +{ + /// + /// Pure unit coverage for run-tests CancelAfter timeout helpers. + /// + [TestFixture] + public class RunTestsExecutionTimeoutTests + { + [Test] + public void Validate_WhenTimeoutSecondsIsZero_ShouldReject() + { + // Verifies non-positive TimeoutSeconds fail before CancelAfter is armed. + (bool isValid, string errorMessage) = RunTestsExecutionTimeout.Validate(0); + + Assert.That(isValid, Is.False); + Assert.That(errorMessage, Does.Contain("greater than zero")); + Assert.That(errorMessage, Does.Contain("--timeout-seconds")); + } + + [Test] + public void Validate_WhenTimeoutSecondsExceedsMax_ShouldReject() + { + // Verifies values above MaxTimeoutSeconds are rejected so C# CancelAfter stays ahead of the CLI 30-minute absolute limit. + int tooLarge = RunTestsExecutionTimeout.MaxTimeoutSeconds + 1; + + (bool isValid, string errorMessage) = RunTestsExecutionTimeout.Validate(tooLarge); + + Assert.That(isValid, Is.False); + Assert.That(errorMessage, Does.Contain(RunTestsExecutionTimeout.MaxTimeoutSeconds.ToString())); + Assert.That(errorMessage, Does.Contain(tooLarge.ToString())); + } + + [Test] + public void Validate_WhenTimeoutSecondsIsDefault_ShouldAccept() + { + // Verifies the agreed default (600s) is inside the allowed range. + (bool isValid, string errorMessage) = RunTestsExecutionTimeout.Validate( + RunTestsExecutionTimeout.DefaultTimeoutSeconds); + + Assert.That(isValid, Is.True); + Assert.That(errorMessage, Is.Null); + } + + [Test] + public void Validate_WhenTimeoutSecondsIsMax_ShouldAccept() + { + // Verifies the inclusive upper bound remains usable for long suites. + (bool isValid, string errorMessage) = RunTestsExecutionTimeout.Validate( + RunTestsExecutionTimeout.MaxTimeoutSeconds); + + Assert.That(isValid, Is.True); + Assert.That(errorMessage, Is.Null); + } + + [Test] + public void CreateTimeoutMessage_ShouldGuideCallerToExtendTimeoutAndWarnAboutBackgroundRunner() + { + // Verifies timeout copy tells agents how to extend the budget and that Test Runner may still be running. + string message = RunTestsExecutionTimeout.CreateTimeoutMessage(600); + + Assert.That(message, Does.Contain("600")); + Assert.That(message, Does.Contain("--timeout-seconds")); + Assert.That(message, Does.Contain("background")); + Assert.That(message, Does.Contain("uloop launch -r")); + } + + [Test] + public void IsTimeoutCancellation_WhenParentIsNotCanceled_ShouldReturnTrue() + { + // Verifies CancelAfter-driven cancellation is distinguished from parent/disconnect cancellation. + using CancellationTokenSource parent = new(); + + Assert.That(RunTestsExecutionTimeout.IsTimeoutCancellation(parent.Token), Is.True); + } + + [Test] + public void IsTimeoutCancellation_WhenParentIsCanceled_ShouldReturnFalse() + { + // Verifies parent/disconnect cancellation is not reported as a run-tests timeout. + using CancellationTokenSource parent = new(); + parent.Cancel(); + + Assert.That(RunTestsExecutionTimeout.IsTimeoutCancellation(parent.Token), Is.False); + } + + [Test] + public async Task CreateLinkedTimeoutSource_WhenTimeoutElapses_ShouldCancelWithoutCancelingParent() + { + // Verifies CancelAfter fires on the linked source while leaving the parent token usable. + using CancellationTokenSource parent = new(); + using CancellationTokenSource linked = RunTestsExecutionTimeout.CreateLinkedTimeoutSource( + parent.Token, + timeoutSeconds: 1); + + TaskCompletionSource canceled = + new(TaskCreationOptions.RunContinuationsAsynchronously); + using CancellationTokenRegistration registration = linked.Token.Register(() => canceled.TrySetResult(true)); + + bool observed = await canceled.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.That(observed, Is.True); + Assert.That(linked.IsCancellationRequested, Is.True); + Assert.That(parent.IsCancellationRequested, Is.False); + Assert.That(RunTestsExecutionTimeout.IsTimeoutCancellation(parent.Token), Is.True); + } + + [Test] + public void CreateLinkedTimeoutSource_WhenParentCancels_ShouldCancelLinkedSource() + { + // Verifies parent cancellation still propagates through the linked timeout source. + using CancellationTokenSource parent = new(); + using CancellationTokenSource linked = RunTestsExecutionTimeout.CreateLinkedTimeoutSource( + parent.Token, + timeoutSeconds: RunTestsExecutionTimeout.DefaultTimeoutSeconds); + + parent.Cancel(); + + Assert.That(linked.IsCancellationRequested, Is.True); + Assert.That(RunTestsExecutionTimeout.IsTimeoutCancellation(parent.Token), Is.False); + } + } +} From e6fb0c9ab9c9da75bc70dceb66e18bcd106e8ae9 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Mon, 13 Jul 2026 15:04:54 +0900 Subject: [PATCH 2/3] fix: run-tests cancel now exits Play Mode before re-enabling domain reload (#1740) Co-authored-by: Cursor --- .../RunTests/RunTestsCancelStopRestore.cs | 289 ++++++++++++++++++ .../RunTestsCancelStopRestore.cs.meta | 11 + .../RunTests/RunTestsExecutionTimeout.cs | 18 +- .../RunTests/RunTestsUseCase.cs | 13 +- .../TestFramework/PlayModeTestExecuter.cs | 39 ++- .../RunTestsCancelStopRestoreUnityHooks.cs | 51 ++++ ...unTestsCancelStopRestoreUnityHooks.cs.meta | 11 + .../RunTestsCancelStopRestoreTests.cs | 194 ++++++++++++ .../RunTestsExecutionTimeout.UnitTests.csproj | 1 + .../RunTestsExecutionTimeoutTests.cs | 13 + 10 files changed, 626 insertions(+), 14 deletions(-) create mode 100644 Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs create mode 100644 Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs create mode 100644 Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs.meta create mode 100644 tests/RunTestsExecutionTimeout.UnitTests/RunTestsCancelStopRestoreTests.cs diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs new file mode 100644 index 000000000..0df821428 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs @@ -0,0 +1,289 @@ +using System; +using System.Diagnostics; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Injectable side effects for cancel-time Test Runner stop and PlayMode restore. + /// + internal sealed class RunTestsCancelStopRestoreHooks + { + /// + /// Attempts to cancel an in-flight Test Runner job. Returns false when unavailable. + /// + public Func TryCancelTestRun { get; set; } + + /// + /// True when a Test Runner job is still active. Optional; null skips EditMode run polling. + /// + public Func IsRunActive { get; set; } + + /// + /// True while the Editor is in Play Mode. + /// + public Func IsPlaying { get; set; } + + /// + /// Requests Play Mode exit. Must be idempotent. + /// + public Action RequestExitPlayMode { get; set; } + + /// + /// Wall-clock delay used for upper-bounded polling. Callers must pass CancellationToken.None. + /// + public Func DelayAsync { get; set; } + + /// + /// Warning logger for soft failures inside the no-throw stop path. + /// + public Action LogWarning { get; set; } + } + + /// + /// Outcome of cancel-time stop/restore. Used to extend agent-facing timeout messages. + /// + internal readonly struct RunTestsCancelStopRestoreResult + { + public RunTestsCancelStopRestoreResult( + bool testRunCancelAttempted, + bool testRunCancelSucceeded, + bool playModeExitRequested, + bool playModeExitConfirmed, + bool stopWaitTimedOut, + string degradationNote) + { + TestRunCancelAttempted = testRunCancelAttempted; + TestRunCancelSucceeded = testRunCancelSucceeded; + PlayModeExitRequested = playModeExitRequested; + PlayModeExitConfirmed = playModeExitConfirmed; + StopWaitTimedOut = stopWaitTimedOut; + DegradationNote = degradationNote ?? string.Empty; + } + + public bool TestRunCancelAttempted { get; } + public bool TestRunCancelSucceeded { get; } + public bool PlayModeExitRequested { get; } + public bool PlayModeExitConfirmed { get; } + public bool StopWaitTimedOut { get; } + public string DegradationNote { get; } + } + + /// + /// Stops Test Runner work and restores Editor Play Mode after run-tests cancellation. + /// + internal static class RunTestsCancelStopRestore + { + public const int DefaultStopWaitTimeoutMilliseconds = 10000; + public const int DefaultPollIntervalMilliseconds = 100; + + /// + /// Performs stop/restore without observing the already-canceled execution token. + /// Why CancellationToken.None: executionCt is already canceled when this runs; waiting on + /// it would throw immediately and skip PlayMode restore / scope lifetime guarantees. + /// Why never throw: callers invoke this from cancel catch paths; throwing would hide the + /// original OperationCanceledException and leave DomainReloadDisableScope dispose timing undefined. + /// + public static async Task StopAndRestoreAsync( + bool isPlayMode, + string runGuid, + RunTestsCancelStopRestoreHooks hooks, + int stopWaitTimeoutMilliseconds = DefaultStopWaitTimeoutMilliseconds, + int pollIntervalMilliseconds = DefaultPollIntervalMilliseconds) + { + Debug.Assert(hooks != null, "hooks must not be null"); + Debug.Assert(hooks.IsPlaying != null, "hooks.IsPlaying must not be null"); + Debug.Assert(hooks.RequestExitPlayMode != null, "hooks.RequestExitPlayMode must not be null"); + Debug.Assert(hooks.DelayAsync != null, "hooks.DelayAsync must not be null"); + Debug.Assert(hooks.LogWarning != null, "hooks.LogWarning must not be null"); + Debug.Assert(stopWaitTimeoutMilliseconds > 0, "stopWaitTimeoutMilliseconds must be positive"); + Debug.Assert(pollIntervalMilliseconds > 0, "pollIntervalMilliseconds must be positive"); + + try + { + return await StopAndRestoreCoreAsync( + isPlayMode, + runGuid, + hooks, + stopWaitTimeoutMilliseconds, + pollIntervalMilliseconds); + } + catch (Exception exception) + { + // Why catch-all: this method is an absolute no-throw boundary for cancel paths. + hooks.LogWarning( + "run-tests stop/restore failed unexpectedly and was swallowed: " + exception); + return new RunTestsCancelStopRestoreResult( + testRunCancelAttempted: false, + testRunCancelSucceeded: false, + playModeExitRequested: false, + playModeExitConfirmed: false, + stopWaitTimedOut: false, + degradationNote: + "Stop/restore failed unexpectedly; Editor state may still be dirty. " + + "Restart with `uloop launch -r` if Unity remains stuck."); + } + } + + private static async Task StopAndRestoreCoreAsync( + bool isPlayMode, + string runGuid, + RunTestsCancelStopRestoreHooks hooks, + int stopWaitTimeoutMilliseconds, + int pollIntervalMilliseconds) + { + bool testRunCancelAttempted = false; + bool testRunCancelSucceeded = false; + if (hooks.TryCancelTestRun != null && !string.IsNullOrEmpty(runGuid)) + { + testRunCancelAttempted = true; + try + { + testRunCancelSucceeded = hooks.TryCancelTestRun(runGuid); + } + catch (Exception exception) + { + hooks.LogWarning( + "TryCancelTestRun failed and was ignored (falling back to public stop path): " + + exception); + testRunCancelSucceeded = false; + } + } + + bool playModeExitRequested = false; + bool playModeExitConfirmed = !isPlayMode || !hooks.IsPlaying(); + bool stopWaitTimedOut = false; + + if (isPlayMode && hooks.IsPlaying()) + { + playModeExitRequested = true; + try + { + hooks.RequestExitPlayMode(); + } + catch (Exception exception) + { + hooks.LogWarning("RequestExitPlayMode failed and was ignored: " + exception); + } + + (bool confirmed, bool timedOut) = await WaitUntilAsync( + () => !hooks.IsPlaying(), + hooks.DelayAsync, + stopWaitTimeoutMilliseconds, + pollIntervalMilliseconds); + playModeExitConfirmed = confirmed; + stopWaitTimedOut = timedOut; + } + else if (!isPlayMode && hooks.IsRunActive != null && hooks.IsRunActive()) + { + (bool confirmed, bool timedOut) = await WaitUntilAsync( + () => !hooks.IsRunActive(), + hooks.DelayAsync, + stopWaitTimeoutMilliseconds, + pollIntervalMilliseconds); + stopWaitTimedOut = timedOut; + if (!confirmed) + { + // EditMode has no public job-cancel API under TF 1.3.9 without Option A. + } + } + + string degradationNote = BuildDegradationNote( + isPlayMode, + testRunCancelAttempted, + testRunCancelSucceeded, + playModeExitRequested, + playModeExitConfirmed, + stopWaitTimedOut); + + return new RunTestsCancelStopRestoreResult( + testRunCancelAttempted, + testRunCancelSucceeded, + playModeExitRequested, + playModeExitConfirmed, + stopWaitTimedOut, + degradationNote); + } + + private static async Task<(bool Confirmed, bool TimedOut)> WaitUntilAsync( + Func isDone, + Func delayAsync, + int timeoutMilliseconds, + int pollIntervalMilliseconds) + { + int elapsedMilliseconds = 0; + while (elapsedMilliseconds < timeoutMilliseconds) + { + if (isDone()) + { + return (true, false); + } + + await delayAsync(pollIntervalMilliseconds, CancellationToken.None); + elapsedMilliseconds += pollIntervalMilliseconds; + } + + return (isDone(), true); + } + + private static string BuildDegradationNote( + bool isPlayMode, + bool testRunCancelAttempted, + bool testRunCancelSucceeded, + bool playModeExitRequested, + bool playModeExitConfirmed, + bool stopWaitTimedOut) + { + StringBuilder note = new(); + // Why guard on PlayMode exit confirmed: once Play Mode has exited, the in-flight + // PlayMode job is effectively stopped; warning about missing CancelTestRun only + // confuses agents. Keep the warning for EditMode and for unconfirmed PlayMode exit. + if ((!testRunCancelAttempted || !testRunCancelSucceeded) && + (!isPlayMode || !playModeExitConfirmed)) + { + note.Append( + "Unity Test Framework 1.3.9 has no public API to cancel an in-flight test job"); + if (!isPlayMode) + { + note.Append("; an EditMode job may still be running until it finishes"); + } + note.Append(". "); + } + + if (playModeExitRequested && !playModeExitConfirmed) + { + note.Append("Play Mode exit was requested but not confirmed within the stop wait. "); + } + else if (playModeExitRequested && playModeExitConfirmed) + { + note.Append("Play Mode exit was requested and confirmed. "); + } + + if (stopWaitTimedOut) + { + note.Append("Stop-wait polling reached its upper bound. "); + } + + note.Append("Restart with `uloop launch -r` if Unity remains stuck."); + return note.ToString(); + } + } + + /// + /// OperationCanceledException that carries cancel-time stop/restore details for timeout messaging. + /// + internal sealed class RunTestsExecutionCanceledException : OperationCanceledException + { + public RunTestsExecutionCanceledException( + CancellationToken token, + RunTestsCancelStopRestoreResult stopResult) + : base("run-tests execution was canceled", token) + { + StopResult = stopResult; + } + + public RunTestsCancelStopRestoreResult StopResult { get; } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs.meta b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs.meta new file mode 100644 index 000000000..07cb2fa08 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsCancelStopRestore.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 969616ef8ae9c4fefab838ab6daa5edf +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs index 915646e2a..bdc792713 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsExecutionTimeout.cs @@ -63,8 +63,8 @@ public static CancellationTokenSource CreateLinkedTimeoutSource( CancellationTokenSource linkedCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(parentCancellationToken); // Why CancelAfter here: frees the single-flight slot when RunFinished never fires. - // PR 1-1 only completes the await via cancellation; TestRunnerApi may keep running - // until PR 1-2 routes this same cancel path through an explicit stop + PlayMode restore. + // Cancel paths then run StopAndRestore (PlayMode exit / optional job cancel) before + // DomainReloadDisableScope dispose so reload is not re-enabled mid-run. linkedCancellationTokenSource.CancelAfter(TimeSpan.FromSeconds(timeoutSeconds)); return linkedCancellationTokenSource; } @@ -72,14 +72,22 @@ public static CancellationTokenSource CreateLinkedTimeoutSource( /// /// Builds the agent-facing message for a CancelAfter timeout. /// - public static string CreateTimeoutMessage(int timeoutSeconds) + public static string CreateTimeoutMessage( + int timeoutSeconds, + string stopRestoreDegradationNote = null) { Debug.Assert(timeoutSeconds > 0, "timeoutSeconds must be greater than zero"); - return + string message = $"run-tests timed out after {timeoutSeconds} seconds without a RunFinished callback. " + - "The Unity Test Runner may still be running in the background. " + "Increase --timeout-seconds for long suites, or restart with `uloop launch -r` if Unity is stuck."; + if (!string.IsNullOrEmpty(stopRestoreDegradationNote)) + { + return message + " " + stopRestoreDegradationNote; + } + + return message + + " The Unity Test Runner may still be running in the background."; } /// diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs index aa9aaa7d9..983b0380b 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.cs @@ -126,14 +126,19 @@ public async Task ExecuteAsync(RunTestsSchema parameters, Canc // the fixed cleanup delay straddles the deadline after RunFinished already arrived. await _waitForTestRunnerCleanupAsync(ct); } - catch (OperationCanceledException) when (RunTestsExecutionTimeout.IsTimeoutCancellation(ct)) + catch (RunTestsExecutionCanceledException canceledException) + when (RunTestsExecutionTimeout.IsTimeoutCancellation(ct)) { // Why return a tool failure instead of rethrowing: agents need an actionable // timeout message (extend --timeout-seconds / launch -r). Parent/disconnect // cancellation still propagates so the IPC session can tear down normally. - // Why not stop TestRunnerApi here yet: PR 1-2 will route this same cancel path - // through explicit runner stop + PlayMode restore; until then the runner may - // keep running in the background after the await is released. + return CreateFailureResponse( + RunTestsExecutionTimeout.CreateTimeoutMessage( + parameters.TimeoutSeconds, + canceledException.StopResult.DegradationNote)); + } + catch (OperationCanceledException) when (RunTestsExecutionTimeout.IsTimeoutCancellation(ct)) + { return CreateFailureResponse( RunTestsExecutionTimeout.CreateTimeoutMessage(parameters.TimeoutSeconds)); } diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs b/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs index 8bcfbfae7..6dc74d0a3 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs +++ b/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs @@ -42,8 +42,25 @@ public static async Task ExecutePlayModeTest( { ct.ThrowIfCancellationRequested(); DomainReloadDisableScope.RecoverAbandonedScopeBeforeNewRun(); - using DomainReloadDisableScope scope = new DomainReloadDisableScope(); - return await ExecuteTestWithEventNotification(TestMode.PlayMode, filter, ct); + // Why not `using`: Dispose must run only after cancel-time stop/restore finishes. + // Disposing earlier re-enables domain reload while Test Runner / Play Mode may still be active. + DomainReloadDisableScope scope = new DomainReloadDisableScope(); + try + { + return await ExecuteTestWithEventNotification(TestMode.PlayMode, filter, ct); + } + catch (OperationCanceledException originalException) + { + RunTestsCancelStopRestoreResult stopResult = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: true, + runGuid: null, + RunTestsCancelStopRestoreUnityHooks.Resolve()); + throw new RunTestsExecutionCanceledException(originalException.CancellationToken, stopResult); + } + finally + { + scope.Dispose(); + } } public static async Task ExecuteEditModeTest( @@ -51,7 +68,18 @@ public static async Task ExecuteEditModeTest( CancellationToken ct) { ct.ThrowIfCancellationRequested(); - return await ExecuteTestWithEventNotification(TestMode.EditMode, filter, ct); + try + { + return await ExecuteTestWithEventNotification(TestMode.EditMode, filter, ct); + } + catch (OperationCanceledException originalException) + { + RunTestsCancelStopRestoreResult stopResult = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: false, + runGuid: null, + RunTestsCancelStopRestoreUnityHooks.Resolve()); + throw new RunTestsExecutionCanceledException(originalException.CancellationToken, stopResult); + } } private static async Task ExecuteTestWithEventNotification( @@ -96,7 +124,7 @@ internal static string TrySaveFailureXml(ITestResultAdaptor rawResult) } catch (Exception exception) { - Debug.LogWarning($"Failed to save NUnit XML result file: {exception}"); + Debug.LogWarning($"Failed to save failure XML result file: {exception}"); return null; } } @@ -108,7 +136,8 @@ private static void StartTestExecution(TestMode testMode, TestExecutionFilter fi testRunnerApi.RegisterCallbacks(callback); Filter unityFilter = CreateUnityFilter(testMode, filter); - testRunnerApi.Execute(new ExecutionSettings(unityFilter)); + // runGuid is discarded until Option A stores it for CancelTestRun. + _ = testRunnerApi.Execute(new ExecutionSettings(unityFilter)); } private static Filter CreateUnityFilter(TestMode testMode, TestExecutionFilter filter) diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs b/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs new file mode 100644 index 000000000..85defc942 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs @@ -0,0 +1,51 @@ +#if ULOOP_HAS_TEST_FRAMEWORK +using System; +using System.Threading; +using System.Threading.Tasks; +using UnityEditor; +using UnityEngine; + +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Unity Editor hooks for cancel-time stop/restore. Option B baseline (public API only). + /// Option A (CancelTestRun reflection) can plug into TryCancelTestRun later with B fallback. + /// + internal static class RunTestsCancelStopRestoreUnityHooks + { + /// + /// Optional override for pure unit / EditMode stubbing. Null uses production hooks. + /// + internal static RunTestsCancelStopRestoreHooks OverrideHooksForTests { get; set; } + + internal static RunTestsCancelStopRestoreHooks Resolve() + { + return OverrideHooksForTests ?? CreateDefault(); + } + + internal static RunTestsCancelStopRestoreHooks CreateDefault() + { + return new RunTestsCancelStopRestoreHooks + { + // Why null TryCancelTestRun until Option A is user-approved: TF 1.3.9 exposes + // CancelTestRun only as an internal API, and reflection requires explicit permission. + // When Option A lands, resolve CancelTestRun once, cache it, and fall back here on failure. + TryCancelTestRun = null, + IsRunActive = null, + IsPlaying = () => EditorApplication.isPlaying, + RequestExitPlayMode = () => + { + if (EditorApplication.isPlaying) + { + EditorApplication.isPlaying = false; + } + }, + DelayAsync = (milliseconds, ct) => TimerDelay.Wait(milliseconds, ct), + LogWarning = message => Debug.LogWarning(message) + }; + } + } +} +#endif diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs.meta b/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs.meta new file mode 100644 index 000000000..7b0b38913 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/RunTests/TestFramework/RunTestsCancelStopRestoreUnityHooks.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 6f2ab0cbfd3b84cb1bb59bd7edb2a5b5 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsCancelStopRestoreTests.cs b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsCancelStopRestoreTests.cs new file mode 100644 index 000000000..f5d2d12c1 --- /dev/null +++ b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsCancelStopRestoreTests.cs @@ -0,0 +1,194 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.UnitTests +{ + /// + /// Pure unit coverage for cancel-time Test Runner stop and PlayMode restore orchestration. + /// + [TestFixture] + public class RunTestsCancelStopRestoreTests + { + [Test] + public async Task StopAndRestoreAsync_WhenPlayModeIsActive_ShouldExitThenConfirmBeforeReturning() + { + // Verifies PlayMode cancel path requests exit and waits until not playing. + int playingChecks = 0; + bool exitRequested = false; + RecordingHooks hooks = new( + isPlaying: () => + { + playingChecks++; + return !exitRequested; + }, + requestExitPlayMode: () => exitRequested = true); + + RunTestsCancelStopRestoreResult result = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: true, + runGuid: null, + hooks.Create(), + stopWaitTimeoutMilliseconds: 1000, + pollIntervalMilliseconds: 1); + + Assert.That(exitRequested, Is.True); + Assert.That(result.PlayModeExitRequested, Is.True); + Assert.That(result.PlayModeExitConfirmed, Is.True); + Assert.That(result.StopWaitTimedOut, Is.False); + Assert.That(playingChecks, Is.GreaterThan(0)); + Assert.That(result.DegradationNote, Does.Contain("Play Mode exit was requested and confirmed")); + Assert.That(result.DegradationNote, Does.Not.Contain("no public API")); + } + + [Test] + public async Task StopAndRestoreAsync_WhenPlayModeExitNeverConfirms_ShouldTimeOutWithoutThrowing() + { + // Verifies the upper bound frees cancel handling when PlayMode exit never completes. + RecordingHooks hooks = new( + isPlaying: () => true, + requestExitPlayMode: () => { }); + + RunTestsCancelStopRestoreResult result = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: true, + runGuid: null, + hooks.Create(), + stopWaitTimeoutMilliseconds: 5, + pollIntervalMilliseconds: 1); + + Assert.That(result.PlayModeExitRequested, Is.True); + Assert.That(result.PlayModeExitConfirmed, Is.False); + Assert.That(result.StopWaitTimedOut, Is.True); + Assert.That(result.DegradationNote, Does.Contain("not confirmed")); + } + + [Test] + public async Task StopAndRestoreAsync_WhenTryCancelThrows_ShouldFallBackToPlayModeExit() + { + // Verifies Option A style cancel failures degrade to the public PlayMode exit path. + bool exitRequested = false; + RecordingHooks hooks = new( + isPlaying: () => !exitRequested, + requestExitPlayMode: () => exitRequested = true, + tryCancelTestRun: _ => throw new InvalidOperationException("lookup failed")); + + RunTestsCancelStopRestoreResult result = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: true, + runGuid: "run-guid", + hooks.Create(), + stopWaitTimeoutMilliseconds: 1000, + pollIntervalMilliseconds: 1); + + Assert.That(result.TestRunCancelAttempted, Is.True); + Assert.That(result.TestRunCancelSucceeded, Is.False); + Assert.That(exitRequested, Is.True); + Assert.That(result.PlayModeExitConfirmed, Is.True); + Assert.That(hooks.Warnings.Count, Is.EqualTo(1)); + } + + [Test] + public async Task StopAndRestoreAsync_WhenDelayThrows_ShouldReturnDegradedResultWithoutThrowing() + { + // Verifies the no-throw contract: unexpected hook failures become degradation notes. + RecordingHooks hooks = new( + isPlaying: () => true, + requestExitPlayMode: () => { }, + delayAsync: (_, _) => throw new InvalidOperationException("delay failed")); + + RunTestsCancelStopRestoreResult result = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: true, + runGuid: null, + hooks.Create(), + stopWaitTimeoutMilliseconds: 1000, + pollIntervalMilliseconds: 1); + + Assert.That(result.DegradationNote, Does.Contain("failed unexpectedly")); + Assert.That(hooks.Warnings.Count, Is.EqualTo(1)); + } + + [Test] + public async Task StopAndRestoreAsync_WhenEditModeAndCancelUnavailable_ShouldSkipPlayModeExit() + { + // Verifies EditMode Option B path does not touch PlayMode and documents job-stop limits. + bool exitRequested = false; + RecordingHooks hooks = new( + isPlaying: () => false, + requestExitPlayMode: () => exitRequested = true); + + RunTestsCancelStopRestoreResult result = await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: false, + runGuid: null, + hooks.Create()); + + Assert.That(exitRequested, Is.False); + Assert.That(result.PlayModeExitRequested, Is.False); + Assert.That(result.DegradationNote, Does.Contain("no public API")); + Assert.That(result.DegradationNote, Does.Contain("EditMode")); + } + + [Test] + public async Task StopAndRestoreAsync_ShouldNotObserveAlreadyCanceledTokensForInternalWaits() + { + // Verifies internal polling uses CancellationToken.None rather than a canceled execution token. + using CancellationTokenSource canceled = new(); + canceled.Cancel(); + CancellationToken observedToken = CancellationToken.None; + bool exitRequested = false; + RecordingHooks hooks = new( + isPlaying: () => !exitRequested, + requestExitPlayMode: () => exitRequested = true, + delayAsync: (milliseconds, ct) => + { + observedToken = ct; + ct.ThrowIfCancellationRequested(); + return Task.CompletedTask; + }); + + await RunTestsCancelStopRestore.StopAndRestoreAsync( + isPlayMode: true, + runGuid: null, + hooks.Create(), + stopWaitTimeoutMilliseconds: 1000, + pollIntervalMilliseconds: 1); + + Assert.That(observedToken.IsCancellationRequested, Is.False); + Assert.That(canceled.IsCancellationRequested, Is.True); + } + + private sealed class RecordingHooks + { + private readonly Func _isPlaying; + private readonly Action _requestExitPlayMode; + private readonly Func _tryCancelTestRun; + private readonly Func _delayAsync; + + public RecordingHooks( + Func isPlaying, + Action requestExitPlayMode, + Func tryCancelTestRun = null, + Func delayAsync = null) + { + _isPlaying = isPlaying; + _requestExitPlayMode = requestExitPlayMode; + _tryCancelTestRun = tryCancelTestRun; + _delayAsync = delayAsync ?? ((_, _) => Task.CompletedTask); + } + + public System.Collections.Generic.List Warnings { get; } = new(); + + public RunTestsCancelStopRestoreHooks Create() + { + return new RunTestsCancelStopRestoreHooks + { + TryCancelTestRun = _tryCancelTestRun, + IsRunActive = null, + IsPlaying = _isPlaying, + RequestExitPlayMode = _requestExitPlayMode, + DelayAsync = _delayAsync, + LogWarning = warning => Warnings.Add(warning) + }; + } + } + } +} diff --git a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj index 5230da9bb..1f0dcb9a0 100644 --- a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj +++ b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeout.UnitTests.csproj @@ -15,5 +15,6 @@ + diff --git a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs index 16002ca66..51583ebe6 100644 --- a/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs +++ b/tests/RunTestsExecutionTimeout.UnitTests/RunTestsExecutionTimeoutTests.cs @@ -70,6 +70,19 @@ public void CreateTimeoutMessage_ShouldGuideCallerToExtendTimeoutAndWarnAboutBac Assert.That(message, Does.Contain("uloop launch -r")); } + [Test] + public void CreateTimeoutMessage_WhenStopRestoreNoteProvided_ShouldAppendDegradationDetails() + { + // Verifies cancel-time stop/restore notes are appended for agent guidance. + string message = RunTestsExecutionTimeout.CreateTimeoutMessage( + 600, + "Play Mode exit was requested and confirmed."); + + Assert.That(message, Does.Contain("600")); + Assert.That(message, Does.Contain("Play Mode exit was requested and confirmed.")); + Assert.That(message, Does.Not.Contain("may still be running in the background")); + } + [Test] public void IsTimeoutCancellation_WhenParentIsNotCanceled_ShouldReturnTrue() { From 642bf138646427c76e89283945bace640cd2f158 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Mon, 13 Jul 2026 15:10:15 +0900 Subject: [PATCH 3/3] fix: abandoned DomainReloadDisableScope state no longer blocks the next PlayMode test run (#1741) Co-authored-by: Cursor --- .../Editor/DomainReloadDisableScopeTests.cs | 86 +++++++++++++++++++ .../EditorUtility/DomainReloadDisableScope.cs | 51 +++++++++-- 2 files changed, 130 insertions(+), 7 deletions(-) diff --git a/Assets/Tests/Editor/DomainReloadDisableScopeTests.cs b/Assets/Tests/Editor/DomainReloadDisableScopeTests.cs index c2fe04bd9..8df4123f3 100644 --- a/Assets/Tests/Editor/DomainReloadDisableScopeTests.cs +++ b/Assets/Tests/Editor/DomainReloadDisableScopeTests.cs @@ -118,6 +118,11 @@ public void Constructor_RestoresStaleMarker_BeforeSavingNewRunMarker() Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.True); DomainReloadDisableScope.RecoverAbandonedScopeBeforeNewRun(); + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(0)); + Assert.That(EditorSettings.enterPlayModeOptionsEnabled, Is.False); + Assert.That(EditorSettings.enterPlayModeOptions, Is.EqualTo(EnterPlayModeOptions.None)); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.False); + DomainReloadDisableScope nextScope = new DomainReloadDisableScope(); DomainReloadDisableScopeRecoveryData markerData = DomainReloadDisableScopeRecovery.ReadMarkerDataForTests(); @@ -132,6 +137,87 @@ public void Constructor_RestoresStaleMarker_BeforeSavingNewRunMarker() System.GC.KeepAlive(abandonedScope); } + [Test] + public void RecoverAbandonedScopeBeforeNewRun_WhenCountIsPositiveWithoutMarker_ShouldClearPhantomCount() + { + // Verifies inconsistent abandoned counts cannot block the next run from saving a fresh marker. + SetEnterPlayModeSettings(false, EnterPlayModeOptions.None); + DomainReloadDisableScope abandonedScope = new DomainReloadDisableScope(); + DomainReloadDisableScopeRecovery.ClearPendingRestoreForTests(); + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(1)); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.False); + + DomainReloadDisableScope.RecoverAbandonedScopeBeforeNewRun(); + + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(0)); + + DomainReloadDisableScope nextScope = new DomainReloadDisableScope(); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.True); + nextScope.Dispose(); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.False); + System.GC.KeepAlive(abandonedScope); + } + + [Test] + public void Dispose_AfterRecoverAbandonedScope_ShouldBeIdempotent() + { + // Verifies disposing a live instance after Recover no longer asserts or double-restores. + SetEnterPlayModeSettings(false, EnterPlayModeOptions.None); + DomainReloadDisableScope abandonedScope = new DomainReloadDisableScope(); + DomainReloadDisableScope.RecoverAbandonedScopeBeforeNewRun(); + + Assert.DoesNotThrow(() => abandonedScope.Dispose()); + Assert.That(EditorSettings.enterPlayModeOptionsEnabled, Is.False); + Assert.That(EditorSettings.enterPlayModeOptions, Is.EqualTo(EnterPlayModeOptions.None)); + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(0)); + } + + [Test] + public void Dispose_OfAbandonedInstance_AfterNewScopeStarted_ShouldLeaveNewScopeIntact() + { + // Verifies a delayed Dispose from an abandoned scope cannot restore settings under a newer scope. + SetEnterPlayModeSettings(false, EnterPlayModeOptions.None); + DomainReloadDisableScope abandonedScope = new DomainReloadDisableScope(); + int generationBeforeRecover = DomainReloadDisableScope.GetGenerationForTests(); + DomainReloadDisableScope.RecoverAbandonedScopeBeforeNewRun(); + Assert.That(DomainReloadDisableScope.GetGenerationForTests(), Is.EqualTo(generationBeforeRecover + 1)); + + DomainReloadDisableScope nextScope = new DomainReloadDisableScope(); + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(1)); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.True); + Assert.That(EditorSettings.enterPlayModeOptions, Is.EqualTo(EnterPlayModeOptions.DisableDomainReload)); + + abandonedScope.Dispose(); + + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(1)); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.True); + Assert.That(EditorSettings.enterPlayModeOptionsEnabled, Is.True); + Assert.That(EditorSettings.enterPlayModeOptions, Is.EqualTo(EnterPlayModeOptions.DisableDomainReload)); + + nextScope.Dispose(); + Assert.That(DomainReloadDisableScope.GetActiveScopeCountForTests(), Is.EqualTo(0)); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.False); + Assert.That(EditorSettings.enterPlayModeOptionsEnabled, Is.False); + Assert.That(EditorSettings.enterPlayModeOptions, Is.EqualTo(EnterPlayModeOptions.None)); + } + + [Test] + public void RecoverAbandonedScopeBeforeNewRun_WhenCountIsZeroWithPendingMarker_ShouldRestoreSettings() + { + // Verifies domain-reload-like state (count reset, marker retained) is cleared before a new run. + SetEnterPlayModeSettings(false, EnterPlayModeOptions.None); + DomainReloadDisableScope abandonedScope = new DomainReloadDisableScope(); + DomainReloadDisableScope.ResetActiveScopeCountForTests(); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.True); + + DomainReloadDisableScope.RecoverAbandonedScopeBeforeNewRun(); + + Assert.That(EditorSettings.enterPlayModeOptionsEnabled, Is.False); + Assert.That(EditorSettings.enterPlayModeOptions, Is.EqualTo(EnterPlayModeOptions.None)); + Assert.That(DomainReloadDisableScopeRecovery.HasPendingRestoreForTests(), Is.False); + System.GC.KeepAlive(abandonedScope); + } + [Test] public void RestoreIfPending_LeavesNoMarkerFile_AfterSuccessfulRestore() { diff --git a/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.cs b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.cs index ead86df2e..185fb8612 100644 --- a/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.cs +++ b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.cs @@ -9,6 +9,9 @@ namespace io.github.hatayama.UnityCliLoop.FirstPartyTools public class DomainReloadDisableScope : IDisposable { private static int _activeScopeCount; + private static int _generation; + + private readonly int _createdGeneration; private bool _disposed; public DomainReloadDisableScope() @@ -20,6 +23,9 @@ public DomainReloadDisableScope() } _activeScopeCount++; + // Why capture generation: RecoverAbandoned may invalidate this instance while a newer + // scope owns the static count; Dispose must ignore stale instances. + _createdGeneration = _generation; EditorSettings.enterPlayModeOptionsEnabled = true; EditorSettings.enterPlayModeOptions = EnterPlayModeOptions.DisableDomainReload; @@ -33,7 +39,22 @@ public void Dispose() } _disposed = true; - System.Diagnostics.Debug.Assert(_activeScopeCount > 0, "active scope count must be positive before dispose"); + + if (_createdGeneration != _generation) + { + // Why return: RecoverAbandoned already invalidated this instance. Decrementing + // would steal the count from a newer active scope (e.g. delayed cancel finally). + return; + } + + System.Diagnostics.Debug.Assert( + _activeScopeCount > 0, + "active scope count must be positive for the current generation before dispose"); + if (_activeScopeCount == 0) + { + return; + } + _activeScopeCount--; if (_activeScopeCount == 0) @@ -43,26 +64,42 @@ public void Dispose() } /// - /// Resets stale scope state before a new PlayMode test run starts. + /// Clears abandoned static scope state and restores pending Enter Play Mode settings + /// before a new PlayMode test run starts. /// internal static void RecoverAbandonedScopeBeforeNewRun() { + // Why always restore when a marker exists (even if count is already 0): domain reload + // resets the static count while leaving the recovery marker on disk. if (_activeScopeCount == 0) { + DomainReloadDisableScopeRecovery.RestoreIfPending(); return; } - if (!DomainReloadDisableScopeRecovery.HasPendingRestore()) - { - return; - } - + // Why clear count even without a marker: a non-zero count would skip SaveCurrentSettings + // on the next constructor and nest on a phantom scope, delaying restore indefinitely. + // Why bump generation: live instances from the abandoned run must not Dispose into the + // next run's count if their await completes late. _activeScopeCount = 0; + _generation++; + DomainReloadDisableScopeRecovery.RestoreIfPending(); } internal static void ResetActiveScopeCountForTests() { _activeScopeCount = 0; + _generation = 0; + } + + internal static int GetActiveScopeCountForTests() + { + return _activeScopeCount; + } + + internal static int GetGenerationForTests() + { + return _generation; } } }