Repository navigation
chore: Remove Editor-freezing and obsolete tests from the Unity test suite - #1310
Merged
Merged
Conversation
UNITYCLILOOP_HAS_ROSLYN is not defined anywhere in the repository (ProjectSettings, csc.rsp, asmdef versionDefines, CI), so the three test files gated behind #if UNITYCLILOOP_HAS_ROSLYN have never been compiled or executed. - Delete RestrictedModeDangerousApiTests: all 37 tests run real dynamic-code compile-and-execute flows (compileOnly: false) in EditMode, which the Unity Freeze Prevention policy forbids, so the file must not be revived. Coverage loss is zero because it never ran. - Delete DynamicCodeExecutorDictionaryErrorTest: TDD debugging scaffolding for a long-fixed bug, with a vacuous Assert.Pass on the success path and coverage duplicated by DynamicCodeExecutorTests. - Revive ExecuteDynamicCodeParameterValidationTests by removing the stale #if gate: parameter validation throws before any compilation and the success path is compile-only, so the freeze risk is low. The CompileOnly:false auto-return test was dropped instead of revived because SourceShaperTests already covers return shaping as a pure unit test. Verified with uloop compile (0 errors, 0 warnings) and uloop run-tests: both revived tests pass; the 4 suite failures also fail on clean HEAD and are unrelated to this change.
These tests could never catch a real regression: - Delete Assets/Tests/_Test_For_Test_: its two tests are Assert.True(true) placeholders and nothing references the test_for_test assembly. - Delete BasicPlayModeTests: all five tests verify C# language and Unity engine basics (arithmetic, string concat, Time.time monotonicity) rather than project code. Other PlayMode tests keep the suite populated. - Remove RunTestsToolTests.ParseParameters_ShouldParseCorrectly: its own comment declares it obsolete since Schema classes replaced JSON parameter parsing, and it only reads back values it just assigned. - Remove the two DynamicCodeSecurityManager CanExecute tests: the implementation returns true for every defined enum value, so the assertions are tautologies with zero regression-detection power. Verified with uloop compile (0 errors, 0 warnings) and a filtered uloop run-tests pass on the remaining tests in the touched fixtures.
SwitchToMainThread_WhenCalledFromBackgroundThread_ShouldSwitchBackToMainThread waited on the background task with WaitUntil and no timeout, so a hung Task.Run body would block the Editor forever. The sibling test SwitchToMainThread_WhenCalledFromBackgroundThread_ShouldSwitchToMainThread verifies the same main-thread switch with a 5-second deadline, so no coverage is lost. Task.Run itself stays because exercising MainThreadSwitcher requires starting from a background thread. Verified with uloop compile (0 errors, 0 warnings); the remaining MainThreadSwitcherTests test and the full PlayMode suite (73 tests) pass.
…flows These tests drove the real DynamicCodeCompiler / executor / tool-dispatch pipeline end-to-end inside EditMode, which the Unity Freeze Prevention policy forbids: the AssemblyBuilder/Roslyn worker fires editor callbacks that the synchronous test runner can deadlock against. - Delete CommandRunnerTests: executes a prebuilt dynamic command whose generated wrapper calls GetAwaiter().GetResult(). - Delete ExecuteDynamicCodeToolAutoUsingTests and ExecuteDynamicCodeToolSecurityTests: full execute-dynamic-code tool dispatch into the real compiler. - Drop the AutoInjectedNamespacesIntegrationTests and PreUsingResolverIntegrationTests fixtures (real CompileAsync) while keeping the pure PreUsingResolver/WrapperTemplate unit tests. - Drop the ExecuteDynamicCodeParameterValidationTests cases that ran the compiler; keep the pure pre-compilation parameter-validation case. Verified: the C/D/E EditMode window completes twice (295/295) and the full EditMode suite no longer freezes.
A SIGQUIT managed-stack dump of a frozen Editor pinned the hang to CompiledAssemblyBuilderTests.AwaitBuildCompletionAsync_WhenCancellation IsRequested: the async test cancels an in-flight task, then NUnit blocks the main thread in Task.Wait() while the task's continuation needs that same main thread, producing a Monitor.Wait deadlock. The Test Runner dialog showed the *next* test name, which is why earlier single-test guesses kept missing the real culprit. - Delete both cancellation-race tests in CompiledAssemblyBuilderTests (in-flight CancellationTokenSource.Cancel + same-thread teardown wait); keep the pure CreateUniqueCompilationName test. - Delete EditorFrameWaiterTests: its [UnityTest] cases launch cross-frame WaitFramesAsync work via .Forget() fire-and-forget, which threw TaskCanceledException during teardown inside the freeze window. The fire-and-forget pattern is forbidden by the Unity Freeze Prevention policy. Verified: with these removed the C/D/E window completes twice (295/295) and the full EditMode suite finishes without freezing.
ProcessRequest_WhenFirstToolWaitsForMainThread and the dynamic-code variant asserted that server_busy error data omits isPlaying/isPaused. That held only in isolation: during a real suite run the editor update loop populates UnityCliLoopEditorStateSnapshot, so the background-thread CreateBusyException path includes both fields and the tests failed on a clean checkout. Seed the snapshot to a known (false, false) state before each request and assert those values, then clear it in the finally block, so the tests verify the documented contract regardless of editor play state.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (16)
💤 Files with no reviewable changes (14)
📝 WalkthroughWalkthroughThis PR removes multiple integration and unit test fixtures from the test suite, simplifies preprocessor guards in parameter validation tests, and hardens editor-state management in version gate tests. The changes consolidate DynamicCodeTool test coverage and improve async test reliability. ChangesTest Suite Cleanup and Infrastructure Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
RyanXie123
pushed a commit
to RyanXie123/unity-cli-loop
that referenced
this pull request
Sep 22, 2026
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.
Summary
User Impact
Task.Wait()while the awaited continuation also needs the main thread.Changes
All changes are test-only; no product, CLI, or Unity runtime behavior is affected.
CommandRunnerTests,ExecuteDynamicCodeToolAutoUsingTests,ExecuteDynamicCodeToolSecurityTests,RestrictedModeDangerousApiTests, plus the integration fixtures inAutoInjectedNamespacesTestsandPreUsingResolverTests. The pure unit tests in those files are kept.CompiledAssemblyBuilderTests(in-flight cancel + same-thread teardown wait) that the stack dump identified as the deadlock, andEditorFrameWaiterTests(fire-and-forget cross-frame work).MainThreadSwitcherTestscase with an unboundedWaitUntil, keeping the equivalent case that has a timeout.#if UNITYCLILOOP_HAS_ROSLYNtests (the symbol is never defined anywhere) and revive the pure parameter-validation cases; remove the obsolete TDD-scaffoldDynamicCodeExecutorDictionaryErrorTest._Test_For_Test_fixture,BasicPlayModeTests, a self-declared-obsoleteRunTestsToolTestscase, and theDynamicCodeSecurityManager.CanExecutetautologies.JsonRpcProcessorCliVersionGateTestsserver-busy assertions deterministic by seeding the editor play-state snapshot, fixing a pre-existing failure on a clean checkout.Verification
uloop compile: 0 errors, 0 warnings.uloop run-tests(EditMode): completes without freezing; the C/D/E window ran twice at 295/295.uloop run-tests --test-mode PlayMode: 73/73 pass.InputVisualizationCanvasPrefabTests.RuntimeSources_WhenScanned_AreEditorOnly,OnionAssemblyDependencyTests.ProductionEditorStartupHooks_WhenLoaded_AreOwnedOnlyByCompositionRootBootstrap); they also fail on a clean checkout and are outside this PR's scope.