From 17a270fba5f4ad648d22503fd386948d42469b12 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:02:09 -0400 Subject: [PATCH 01/12] fix(ci): avoid publishing Unity results with a read-only token --- .github/workflows/unity-tests.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/unity-tests.yml b/.github/workflows/unity-tests.yml index 68a296271..c0406760b 100644 --- a/.github/workflows/unity-tests.yml +++ b/.github/workflows/unity-tests.yml @@ -179,6 +179,9 @@ jobs: unityVersion: ${{ matrix.unityVersion }} testMode: ${{ matrix.testMode }} customParameters: -testCategory domain_reload + # Results are gated locally; this read-only job cannot publish Checks API results. + githubToken: "" + artifactsPath: artifacts/domain-reload - name: Run tests uses: game-ci/unity-test-runner@v4 @@ -193,6 +196,9 @@ jobs: projectPath: ${{ matrix.projectPath }} unityVersion: ${{ matrix.unityVersion }} testMode: ${{ matrix.testMode }} + githubToken: "" + # Keep the preceding domain-reload XML out of the regular suite's result gate. + artifactsPath: artifacts/editmode - name: Check test results env: @@ -249,7 +255,7 @@ jobs: PY - uses: actions/upload-artifact@v4 - if: always() && steps.tests.outcome != 'skipped' + if: always() && steps.domain-tests.outcome != 'skipped' with: name: Test results for ${{ matrix.testMode }} on Unity ${{ matrix.unityVersion }} - path: ${{ steps.tests.outputs.artifactsPath }} + path: artifacts/ From 25cd9bf20f183afa24c5cebaac2155ae62c7d21b Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:02:18 -0400 Subject: [PATCH 02/12] fix(testing): target and restore Windows focus nudges safely --- Server/src/utils/focus_nudge.py | 253 +++++++++++++++++-------- Server/tests/test_focus_nudge.py | 209 ++++++++++++++++++++ website/docs/guides/troubleshooting.md | 22 +++ 3 files changed, 410 insertions(+), 74 deletions(-) diff --git a/Server/src/utils/focus_nudge.py b/Server/src/utils/focus_nudge.py index d36fd6c71..c860cd619 100644 --- a/Server/src/utils/focus_nudge.py +++ b/Server/src/utils/focus_nudge.py @@ -9,7 +9,9 @@ from __future__ import annotations import asyncio +import json import logging +import ntpath import os import platform import shutil @@ -51,6 +53,7 @@ def _parse_env_float(env_var: str, default: float) -> float: _last_nudge_time: float = 0.0 _consecutive_nudges: int = 0 _last_progress_time: float = 0.0 +_nudge_in_flight: bool = False @dataclass @@ -59,11 +62,18 @@ class _FrontmostAppInfo: name: str bundle_id: str | None = None # macOS only: bundle identifier for precise activation + window_handle: int | None = None # Windows only: stable identity for focus restore def __str__(self) -> str: return self.name +def _is_disabled() -> bool: + return os.environ.get("UNITY_MCP_DISABLE_FOCUS_NUDGE", "").strip().lower() in { + "1", "true", "yes", "on", + } + + def _is_available() -> bool: """Check if focus nudging is available on this platform.""" system = platform.system() @@ -356,7 +366,7 @@ def _focus_any_unity_macos() -> bool: def _get_frontmost_app_windows() -> _FrontmostAppInfo | None: - """Get the title of the frontmost window on Windows.""" + """Capture the foreground HWND and title without mixing native return values.""" try: # PowerShell command to get active window title script = ''' @@ -371,67 +381,152 @@ def _get_frontmost_app_windows() -> _FrontmostAppInfo | None: } "@ $hwnd = [Win32]::GetForegroundWindow() +if ($hwnd -eq [IntPtr]::Zero) { exit 1 } $sb = New-Object System.Text.StringBuilder 256 -[Win32]::GetWindowText($hwnd, $sb, 256) -$sb.ToString() +[void][Win32]::GetWindowText($hwnd, $sb, 256) +@{ name = $sb.ToString(); window_handle = $hwnd.ToInt64() } | ConvertTo-Json -Compress ''' result = subprocess.run( - ["powershell", "-Command", script], + ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], capture_output=True, text=True, timeout=5, ) if result.returncode == 0: - return _FrontmostAppInfo(name=result.stdout.strip()) + info = json.loads(result.stdout) + hwnd = info.get("window_handle") + if isinstance(hwnd, int) and not isinstance(hwnd, bool) and hwnd > 0: + return _FrontmostAppInfo(name=str(info.get("name", "")), window_handle=hwnd) except Exception as e: logger.debug(f"Failed to get frontmost window: {e}") return None -def _focus_app_windows(window_title: str) -> bool: - """Focus a window by title on Windows. For Unity, uses Unity Editor pattern.""" +def _find_unity_pid_by_project_path_windows(project_path: str) -> int | None: + """Resolve one Unity.exe by its exact absolute -projectPath argument.""" + if not ntpath.isabs(project_path) or not ntpath.splitdrive(project_path)[0]: + return None try: - # For Unity, we use a pattern match since the title varies - if window_title == "Unity": - script = ''' + # Native tokenization distinguishes a real flag from text inside another + # quoted argument and handles Windows quote/backslash escaping. + script = ''' +$ErrorActionPreference = 'Stop' Add-Type @" using System; +using System.ComponentModel; using System.Runtime.InteropServices; -public class Win32 { - [DllImport("user32.dll")] - public static extern bool SetForegroundWindow(IntPtr hWnd); - [DllImport("user32.dll")] - public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow); +public class UnityCommandLine { + [DllImport("shell32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + private static extern IntPtr CommandLineToArgvW(string commandLine, out int argc); + [DllImport("kernel32.dll")] + private static extern IntPtr LocalFree(IntPtr memory); + public static string[] Parse(string commandLine) { + if (String.IsNullOrWhiteSpace(commandLine)) return new string[0]; + int count; + IntPtr argv = CommandLineToArgvW(commandLine, out count); + if (argv == IntPtr.Zero) throw new Win32Exception(Marshal.GetLastWin32Error()); + try { + string[] args = new string[count]; + for (int i = 0; i < count; i++) { + args[i] = Marshal.PtrToStringUni(Marshal.ReadIntPtr(argv, i * IntPtr.Size)); + } + return args; + } finally { + LocalFree(argv); + } + } } "@ -$unity = Get-Process | Where-Object {$_.MainWindowTitle -like "*Unity*"} | Select-Object -First 1 -if ($unity) { - [Win32]::ShowWindow($unity.MainWindowHandle, 9) - [Win32]::SetForegroundWindow($unity.MainWindowHandle) -} +$processes = @(Get-CimInstance Win32_Process -Filter "Name = 'Unity.exe'" | ForEach-Object { + [PSCustomObject]@{ + ProcessId = $_.ProcessId + Arguments = [UnityCommandLine]::Parse($_.CommandLine) + } +}) +ConvertTo-Json -InputObject $processes -Depth 3 -Compress ''' + result = subprocess.run( + ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], + capture_output=True, text=True, timeout=5, + ) + if result.returncode != 0 or not result.stdout.strip(): + return None + processes = json.loads(result.stdout) + if isinstance(processes, dict): + processes = [processes] + if not isinstance(processes, list): + return None + target = ntpath.normcase(ntpath.normpath(project_path)) + matches = set() + for process in processes: + arguments = process.get("Arguments") + if not isinstance(arguments, list) or not all(isinstance(arg, str) for arg in arguments): + continue + project_arguments = [] + for index, argument in enumerate(arguments[1:], start=1): + if argument.lower() == "-projectpath": + project_arguments.append(arguments[index + 1] if index + 1 < len(arguments) else "") + elif argument.lower().startswith("-projectpath="): + project_arguments.append(argument.split("=", 1)[1]) + # Multiple flags are ambiguous even if they happen to agree. + if len(project_arguments) != 1: + continue + candidate = project_arguments[0] + if not candidate or ntpath.normcase(ntpath.normpath(candidate)) != target: + continue + pid = process.get("ProcessId") + if isinstance(pid, int) and not isinstance(pid, bool) and pid > 0: + matches.add(pid) + return next(iter(matches)) if len(matches) == 1 else None + except Exception as exc: + logger.debug("Failed to resolve Windows Unity project: %s", exc) + return None + + +def _focus_app_windows( + window_title: str, + unity_project_path: str | None = None, + window_handle: int | None = None, +) -> bool: + """Activate a saved HWND or the editor uniquely matching a project path.""" + try: + if window_handle is not None: + if not isinstance(window_handle, int) or isinstance(window_handle, bool) or window_handle <= 0: + return False + target_script = f"$targetHwnd = [IntPtr]{window_handle}" + elif window_title == "Unity" and unity_project_path: + pid = _find_unity_pid_by_project_path_windows(unity_project_path) + if pid is None: + logger.debug("Skipping focus nudge: no unique Unity process for %s", unity_project_path) + return False + target_script = f"$targetHwnd = (Get-Process -Id {pid} -ErrorAction Stop).MainWindowHandle" else: - # Try to find window by title - escape special PowerShell characters - safe_title = window_title.replace("'", "''").replace("`", "``") - script = f''' + return False + script = ''' +$ErrorActionPreference = 'Stop' Add-Type @" using System; using System.Runtime.InteropServices; -public class Win32 {{ +public class Win32 { [DllImport("user32.dll")] public static extern bool SetForegroundWindow(IntPtr hWnd); [DllImport("user32.dll")] public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow); -}} + [DllImport("user32.dll")] + public static extern bool IsWindow(IntPtr hWnd); + [DllImport("user32.dll")] + public static extern IntPtr GetForegroundWindow(); +} "@ -$proc = Get-Process | Where-Object {{$_.MainWindowTitle -eq '{safe_title}'}} | Select-Object -First 1 -if ($proc) {{ - [Win32]::ShowWindow($proc.MainWindowHandle, 9) - [Win32]::SetForegroundWindow($proc.MainWindowHandle) -}} +''' + target_script + ''' +if (-not [Win32]::IsWindow($targetHwnd)) { exit 1 } +[void][Win32]::ShowWindow($targetHwnd, 9) +if (-not [Win32]::SetForegroundWindow($targetHwnd)) { exit 1 } +if ([Win32]::GetForegroundWindow() -ne $targetHwnd) { exit 1 } +exit 0 ''' result = subprocess.run( - ["powershell", "-Command", script], + ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], capture_output=True, text=True, timeout=5, @@ -506,7 +601,7 @@ def _focus_app( Args: app_info: Application info (name + optional bundle_id) or plain name string - unity_project_path: For Unity apps on macOS, the full project root path for + unity_project_path: For Unity apps on macOS/Windows, the full project root path for multi-instance support """ if isinstance(app_info, str): @@ -516,7 +611,7 @@ def _focus_app( if system == "Darwin": return _focus_app_macos(app_info.name, unity_project_path, app_info.bundle_id) elif system == "Windows": - return _focus_app_windows(app_info.name) + return _focus_app_windows(app_info.name, unity_project_path, app_info.window_handle) elif system == "Linux": return _focus_app_linux(app_info.name) return False @@ -539,20 +634,25 @@ async def nudge_unity_focus( focus_duration_s: How long to keep Unity focused (seconds). If None, uses exponential backoff (3s/5s/8s/12s based on consecutive nudges). Can be overridden with UNITY_MCP_NUDGE_DURATION_S env var. - force: If True, ignore the minimum interval between nudges + force: If True, ignore the minimum interval, but respect the disable setting unity_project_path: Full path to Unity project root for multi-instance support. e.g., "/Users/name/project" (NOT "/Users/name/project/Assets") - If None, targets any Unity process. + Windows skips the nudge if the project cannot be uniquely resolved. Returns: True if nudge was performed, False if skipped or failed """ + if _is_disabled(): + return False if focus_duration_s is None: # Use exponential backoff for focus duration focus_duration_s = _get_current_focus_duration() if focus_duration_s <= 0: focus_duration_s = _DEFAULT_FOCUS_DURATION_S - global _last_nudge_time, _consecutive_nudges + global _last_nudge_time, _consecutive_nudges, _nudge_in_flight + + if _nudge_in_flight: + return False if not _is_available(): logger.debug("Focus nudging not available on this platform") @@ -565,48 +665,51 @@ async def nudge_unity_focus( logger.debug(f"Skipping nudge - too soon since last nudge (interval: {current_interval:.1f}s)") return False - # Get current frontmost app - original_app = _get_frontmost_app() - if original_app is None: - logger.debug("Could not determine frontmost app") - return False - - # Check if Unity is already focused (no nudge needed) - if "Unity" in original_app.name: - logger.debug("Unity already focused, no nudge needed") - return False - - project_info = f" for {unity_project_path}" if unity_project_path else "" - logger.info(f"Nudging Unity focus{project_info} (interval: {current_interval:.1f}s, consecutive: {_consecutive_nudges}, duration: {focus_duration_s:.1f}s, will return to {original_app})") - - # Focus Unity (with optional project path for multi-instance support) - if not _focus_app("Unity", unity_project_path): - logger.warning(f"Failed to focus Unity{project_info}") - return False - - # Wait for window switch animation to complete before starting timer - # macOS activate is asynchronous, so Unity might not be visible yet - await asyncio.sleep(0.5) + # Pollers may overlap before the first activation wait updates the backoff. + # Hold this guard through restoration so another nudge cannot save Unity as + # its original window and bring it back after we restore the user's app. + _nudge_in_flight = True + original_app = None + activation_attempted = False + try: + original_app = _get_frontmost_app() + if original_app is None: + logger.debug("Could not determine frontmost app") + return False - # Verify Unity is actually focused now - current_app = _get_frontmost_app() - if current_app and "Unity" not in current_app.name: - logger.warning(f"Unity activation didn't complete - current app is {current_app}") - # Continue anyway in case Unity is processing in background + if platform.system() != "Windows" and "Unity" in original_app.name: + logger.debug("Unity already focused, no nudge needed") + return False - # Only update state after successful activation attempt - _last_nudge_time = now - _consecutive_nudges += 1 + project_info = f" for {unity_project_path}" if unity_project_path else "" + logger.info(f"Nudging Unity focus{project_info} (interval: {current_interval:.1f}s, consecutive: {_consecutive_nudges}, duration: {focus_duration_s:.1f}s, will return to {original_app})") - # Wait for Unity to process (actual working time) - await asyncio.sleep(focus_duration_s) + # ShowWindow can change focus even if the later activation check fails. + # Restore after every attempt, including a subprocess failure or timeout. + activation_attempted = True + if not _focus_app("Unity", unity_project_path): + logger.warning(f"Failed to focus Unity{project_info}") + return False - # Return focus to original app - if original_app and original_app.name != "Unity": - if _focus_app(original_app): - logger.info(f"Returned focus to {original_app} after {focus_duration_s:.1f}s Unity focus") - else: - logger.warning(f"Failed to return focus to {original_app}") + # macOS activation is asynchronous; Windows verifies the target HWND itself. + await asyncio.sleep(0.5) + if platform.system() != "Windows": + current_app = _get_frontmost_app() + if current_app and "Unity" not in current_app.name: + logger.warning(f"Unity activation didn't complete - current app is {current_app}") + + _last_nudge_time = now + _consecutive_nudges += 1 + await asyncio.sleep(focus_duration_s) + finally: + try: + if activation_attempted and original_app is not None: + if _focus_app(original_app): + logger.info(f"Returned focus to {original_app}") + else: + logger.warning(f"Failed to return focus to {original_app}") + finally: + _nudge_in_flight = False return True @@ -637,6 +740,8 @@ def should_nudge( Returns: True if conditions suggest a nudge would help """ + if _is_disabled(): + return False # Only nudge running jobs if status != "running": return False diff --git a/Server/tests/test_focus_nudge.py b/Server/tests/test_focus_nudge.py index 8df3fc858..7e6831160 100644 --- a/Server/tests/test_focus_nudge.py +++ b/Server/tests/test_focus_nudge.py @@ -1,5 +1,8 @@ """Tests for focus_nudge utility — should_nudge() logic and nudge_unity_focus() gating.""" +import asyncio +import json +import subprocess import time from unittest.mock import patch, AsyncMock @@ -13,6 +16,15 @@ ) +@pytest.fixture(autouse=True) +def isolate_nudge_state(monkeypatch): + import utils.focus_nudge as fn + monkeypatch.delenv("UNITY_MCP_DISABLE_FOCUS_NUDGE", raising=False) + for name in ("_last_nudge_time", "_consecutive_nudges", "_last_progress_time"): + monkeypatch.setattr(fn, name, 0) + monkeypatch.setattr(fn, "_nudge_in_flight", False) + + class TestShouldNudge: """Tests for should_nudge() decision logic.""" @@ -80,6 +92,7 @@ async def test_skips_when_not_available(self): async def test_skips_when_unity_already_focused(self): from utils.focus_nudge import _FrontmostAppInfo with patch("utils.focus_nudge._is_available", return_value=True), \ + patch("utils.focus_nudge.platform.system", return_value="Darwin"), \ patch("utils.focus_nudge._get_frontmost_app", return_value=_FrontmostAppInfo(name="Unity")): result = await nudge_unity_focus(force=True) assert result is False @@ -102,3 +115,199 @@ async def test_rate_limited_by_backoff(self): patch("utils.focus_nudge._get_frontmost_app", return_value=_FrontmostAppInfo(name="Terminal")): result = await nudge_unity_focus(force=False) assert result is False + + +@pytest.mark.parametrize("value", ["1", "true", " YES ", "On"]) +@pytest.mark.asyncio +async def test_opt_out_prevents_even_forced_nudge_and_stall_decision(monkeypatch, value): + monkeypatch.setenv("UNITY_MCP_DISABLE_FOCUS_NUDGE", value) + with patch("utils.focus_nudge.subprocess.run") as run, \ + patch("utils.focus_nudge._get_frontmost_app") as frontmost: + assert not should_nudge("running", False, None) + assert not await nudge_unity_focus(force=True) + run.assert_not_called() + frontmost.assert_not_called() + + +@pytest.mark.parametrize("value", ["0", "false", "off", ""]) +def test_opt_out_false_values_preserve_stall_decision(monkeypatch, value): + monkeypatch.setenv("UNITY_MCP_DISABLE_FOCUS_NUDGE", value) + assert should_nudge("running", False, None) + + +class TestWindowsFocus: + def test_capture_preserves_hwnd_and_title_as_separate_values(self): + from utils.focus_nudge import _get_frontmost_app_windows + payload = {"window_handle": 54321, "name": "Terminal: Unity notes"} + with patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess( + [], 0, json.dumps(payload), "", + )) as run: + info = _get_frontmost_app_windows() + assert info.name == payload["name"] + assert info.window_handle == 54321 + assert "[void][Win32]::GetWindowText" in run.call_args.args[0][-1] + + @pytest.mark.parametrize("output", ["27\nTerminal", "{}", '{"window_handle": 0}', '{"window_handle": true}']) + def test_bad_foreground_capture_cannot_be_restored_by_title(self, output): + from utils.focus_nudge import _get_frontmost_app_windows + with patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess([], 0, output, "")): + assert _get_frontmost_app_windows() is None + + @pytest.mark.parametrize("project_args", [ + ["-projectPath", r"C:\Projects\My Game"], + ["-PROJECTPATH", "c:/projects/my game/"], + [r"-projectPath=C:\Projects\My Game"], + ]) + def test_resolves_exact_project_among_multiple_editors(self, project_args): + from utils.focus_nudge import _find_unity_pid_by_project_path_windows + processes = [ + {"ProcessId": 1, "Arguments": ["Unity.exe", "-projectPath", r"C:\Projects\My Game-other"]}, + {"ProcessId": 2, "Arguments": ["Unity.exe", "-projectPath", r"C:\Worktrees\My Game"]}, + {"ProcessId": 3, "Arguments": [r"C:\Unity\Editor\Unity.exe", *project_args, "-logFile", "editor.log"]}, + {"ProcessId": 4, "Arguments": []}, + ] + with patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess([], 0, json.dumps(processes), "")): + assert _find_unity_pid_by_project_path_windows(r"C:\Projects\My Game") == 3 + + @pytest.mark.parametrize("arguments", [ + ["Unity.exe", "-comment", r"literal -projectPath C:\Project", "-projectPath", r"C:\Other"], + ["Unity.exe", "-comment", r"literal -projectPath=C:\Project"], + ["Unity.exe", "-projectPath", r"C:\Project", "-projectPath", r"C:\Other"], + ["Unity.exe", "-projectPath", r"C:\Project", r"-projectPath=C:\Project"], + ["Unity.exe", "-projectPath"], + ]) + def test_quoted_flag_text_and_duplicate_or_missing_project_values_do_not_match(self, arguments): + from utils.focus_nudge import _find_unity_pid_by_project_path_windows + processes = [{"ProcessId": 123, "Arguments": arguments}] + with patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess([], 0, json.dumps(processes), "")): + assert _find_unity_pid_by_project_path_windows(r"C:\Project") is None + + @pytest.mark.parametrize("project_path", ["My Game", "Projects/My Game", r"\Projects\My Game"]) + def test_rejects_non_absolute_project_identity(self, project_path): + from utils.focus_nudge import _find_unity_pid_by_project_path_windows + with patch("utils.focus_nudge.subprocess.run") as run: + assert _find_unity_pid_by_project_path_windows(project_path) is None + run.assert_not_called() + + @pytest.mark.parametrize("processes", [ + [], + [{"ProcessId": 1, "Arguments": ["Unity.exe", "-projectPath", r"C:\Other"]}], + [ + {"ProcessId": 1, "Arguments": ["Unity.exe", "-projectPath", r"C:\Project"]}, + {"ProcessId": 2, "Arguments": ["Unity.exe", "-projectPath", r"C:\Project"]}, + ], + ]) + def test_missing_or_ambiguous_match_never_activates_an_editor(self, processes): + from utils.focus_nudge import _focus_app_windows + with patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess([], 0, json.dumps(processes), "")) as run: + assert not _focus_app_windows("Unity", r"C:\Project") + assert run.call_count == 1 # Process query only; no activation command. + + def test_missing_project_and_title_only_restore_never_activate(self): + from utils.focus_nudge import _focus_app_windows + with patch("utils.focus_nudge.subprocess.run") as run: + assert not _focus_app_windows("Unity") + assert not _focus_app_windows("Terminal") + run.assert_not_called() + + @pytest.mark.parametrize("returncode", [0, 1]) + def test_restore_uses_saved_hwnd_and_checks_native_activation(self, returncode): + from utils.focus_nudge import _FrontmostAppInfo, _focus_app + original = _FrontmostAppInfo(name="Old title: ' $() `", window_handle=54321) + with patch("utils.focus_nudge.platform.system", return_value="Windows"), \ + patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess([], returncode, "", "")) as run: + assert _focus_app(original) is (returncode == 0) + script = run.call_args.args[0][-1] + assert "$targetHwnd = [IntPtr]54321" in script + assert original.name not in script + assert "if (-not [Win32]::SetForegroundWindow($targetHwnd)) { exit 1 }" in script + assert "if ([Win32]::GetForegroundWindow() -ne $targetHwnd) { exit 1 }" in script + + def test_routes_project_identity_and_uses_only_selected_process(self): + from utils.focus_nudge import _focus_app + with patch("utils.focus_nudge.platform.system", return_value="Windows"), \ + patch("utils.focus_nudge._find_unity_pid_by_project_path_windows", return_value=234) as resolve, \ + patch("utils.focus_nudge.subprocess.run", return_value=subprocess.CompletedProcess([], 0, "", "")) as run: + assert _focus_app("Unity", r"C:\Project") + resolve.assert_called_once_with(r"C:\Project") + assert "Get-Process -Id 234" in run.call_args.args[0][-1] + assert "MainWindowTitle" not in run.call_args.args[0][-1] + + +@pytest.mark.asyncio +async def test_nudge_restores_saved_window_even_if_title_contains_unity(): + from utils.focus_nudge import _FrontmostAppInfo + original = _FrontmostAppInfo(name="Unity bug report - browser", window_handle=123) + with patch("utils.focus_nudge.platform.system", return_value="Windows"), \ + patch("utils.focus_nudge._is_available", return_value=True), \ + patch("utils.focus_nudge._get_frontmost_app", return_value=original), \ + patch("utils.focus_nudge._focus_app", return_value=True) as focus, \ + patch("utils.focus_nudge.asyncio.sleep", new_callable=AsyncMock): + assert await nudge_unity_focus(force=True, unity_project_path=r"C:\Project") + assert focus.call_args_list[0].args == ("Unity", r"C:\Project") + assert focus.call_args_list[-1].args == (original,) + + +@pytest.mark.asyncio +async def test_cancelled_nudge_restores_original_window(): + from utils.focus_nudge import _FrontmostAppInfo + import utils.focus_nudge as fn + original = _FrontmostAppInfo(name="Terminal", window_handle=123) + with patch("utils.focus_nudge.platform.system", return_value="Windows"), \ + patch("utils.focus_nudge._is_available", return_value=True), \ + patch("utils.focus_nudge._get_frontmost_app", return_value=original), \ + patch("utils.focus_nudge._focus_app", return_value=True) as focus, \ + patch("utils.focus_nudge.asyncio.sleep", new_callable=AsyncMock, side_effect=asyncio.CancelledError): + with pytest.raises(asyncio.CancelledError): + await nudge_unity_focus(force=True, unity_project_path=r"C:\Project") + assert focus.call_args_list[-1].args == (original,) + assert not fn._nudge_in_flight + + +@pytest.mark.asyncio +async def test_activation_failure_restores_focus_without_advancing_backoff_or_waiting(): + import utils.focus_nudge as fn + original = fn._FrontmostAppInfo(name="Terminal", window_handle=123) + with patch("utils.focus_nudge._is_available", return_value=True), \ + patch("utils.focus_nudge._get_frontmost_app", return_value=original), \ + patch("utils.focus_nudge._focus_app", side_effect=[False, True]) as focus, \ + patch("utils.focus_nudge.asyncio.sleep", new_callable=AsyncMock) as sleep: + assert not await nudge_unity_focus(force=True, unity_project_path=r"C:\Project") + assert fn._consecutive_nudges == 0 + assert focus.call_args_list[-1].args == (original,) + assert not fn._nudge_in_flight + sleep.assert_not_called() + + +@pytest.mark.parametrize("other_project", [r"C:\Project", r"C:\OtherWorktree"]) +@pytest.mark.asyncio +async def test_overlapping_nudge_does_not_capture_or_restore_unity_as_original(other_project): + import utils.focus_nudge as fn + original = fn._FrontmostAppInfo(name="Browser", window_handle=123) + unity = fn._FrontmostAppInfo(name="Unity", window_handle=456) + first_wait_started = asyncio.Event() + finish_first_wait = asyncio.Event() + + async def sleep_during_activation(duration): + if duration == 0.5: + first_wait_started.set() + await finish_first_wait.wait() + + with patch("utils.focus_nudge.platform.system", return_value="Windows"), \ + patch("utils.focus_nudge._is_available", return_value=True), \ + patch("utils.focus_nudge._get_frontmost_app", side_effect=[original, unity]) as capture, \ + patch("utils.focus_nudge._focus_app", return_value=True) as focus, \ + patch("utils.focus_nudge.asyncio.sleep", side_effect=sleep_during_activation): + first = asyncio.create_task(nudge_unity_focus(force=True, unity_project_path=r"C:\Project")) + try: + await asyncio.wait_for(first_wait_started.wait(), timeout=1) + assert not await nudge_unity_focus(force=True, unity_project_path=other_project) + capture.assert_called_once() + assert focus.call_count == 1 + finally: + finish_first_wait.set() + await first + assert first.result() is True + assert focus.call_count == 2 + assert focus.call_args_list[-1].args == (original,) + assert not fn._nudge_in_flight diff --git a/website/docs/guides/troubleshooting.md b/website/docs/guides/troubleshooting.md index 3dd425f4d..4bd2ec21d 100644 --- a/website/docs/guides/troubleshooting.md +++ b/website/docs/guides/troubleshooting.md @@ -216,6 +216,28 @@ If restarting doesn't fix it: --- +## Unity takes focus while tests are running + +The server may briefly focus Unity when a running test has not reported progress, +to help editors throttled in the background. To disable this behavior, set +`UNITY_MCP_DISABLE_FOCUS_NUDGE=1` in the environment of the **Python MCP server** +and restart that server. For a stdio client, add it to the server's `env` entry; +for HTTP, set it in the environment that launches the shared server. The values +`true`, `yes`, and `on` also disable nudges. Forced nudges respect this setting. + +On Windows, the nudge now requires an absolute project path matching exactly one +running `Unity.exe` process. If the path cannot be resolved, including some stdio +sessions, it skips activation. It restores the previous window by its saved HWND, +so a changing window title does not prevent focus restoration. Windows can still +deny an activation request, in which case the server reports failure. + +This mitigates the desktop disruption reported in +[#1407](https://github.com/CoplayDev/unity-mcp/issues/1407). A long healthy test can +still trigger the no-progress heuristic, and nudges currently have no per-job +attempt limit. Disable them when background tests already run reliably. + +--- + ## FAQ — Claude Code **Q: Unity can't find `claude` even though Terminal can.** From 6d0fe54f285278460ee537a06d11ded2817d434c Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:02:27 -0400 Subject: [PATCH 03/12] fix(ci): gate Unity results on runner outcome and valid NUnit output --- .github/workflows/unity-tests.yml | 45 +---------- tools/check_unity_test_results.py | 69 ++++++++++++++++ tools/tests/test_check_unity_test_results.py | 85 ++++++++++++++++++++ 3 files changed, 158 insertions(+), 41 deletions(-) create mode 100644 tools/check_unity_test_results.py create mode 100644 tools/tests/test_check_unity_test_results.py diff --git a/.github/workflows/unity-tests.yml b/.github/workflows/unity-tests.yml index c0406760b..201ba9738 100644 --- a/.github/workflows/unity-tests.yml +++ b/.github/workflows/unity-tests.yml @@ -22,6 +22,7 @@ on: - MCPForUnity/Editor/** - MCPForUnity/Runtime/** - .github/workflows/unity-tests.yml + - tools/check_unity_test_results.py # Same-repo PRs get a unity-tests status check on every open / push via this trigger # (mirrors python-tests.yml). Fork PRs ALSO fire this trigger but run in the fork's # context without secrets — the `license` gate job writes unity_ok=false and the test @@ -40,6 +41,7 @@ on: - MCPForUnity/Editor/** - MCPForUnity/Runtime/** - .github/workflows/unity-tests.yml + - tools/check_unity_test_results.py # Dedup runs for the same branch across push / pull_request / workflow_call. # Same-repo PRs would otherwise fire both push (on the branch SHA) AND pull_request (on the PR); @@ -203,6 +205,7 @@ jobs: - name: Check test results env: ARTIFACTS_PATH: ${{ steps.tests.outputs.artifactsPath }} + TEST_RUN_OUTCOME: ${{ steps.tests.outcome }} run: | set -euo pipefail # `|| true` so a missing $ARTIFACTS_PATH (Unity crashed before producing any) doesn't trip @@ -212,47 +215,7 @@ jobs: echo "::error::No test results XML found — Unity may have crashed" exit 1 fi - python3 - "$RESULTS_XML" <<'PY' - import sys, xml.etree.ElementTree as ET - # Escape workflow-command payloads so test-controlled XML can't break annotation - # rendering or inject extra workflow commands. - # https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions - def esc_data(s): - return s.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") - def esc_prop(s): - return esc_data(s).replace(":", "%3A").replace(",", "%2C") - root = ET.parse(sys.argv[1]).getroot() - totals = root.attrib - passed = totals.get("passed", "?") - failed = totals.get("failed", "?") - total = totals.get("total", "?") - incon = totals.get("inconclusive", "?") - skipped = totals.get("skipped", "?") - print(f"Results: {passed} passed, {failed} failed, {incon} inconclusive, {skipped} skipped (total: {total})") - fails = [tc for tc in root.iter("test-case") if tc.attrib.get("result") == "Failed"] - if not fails: - sys.exit(0) - # Surface every failure inline so a CI watcher doesn't need to download the NUnit XML artifact. - for tc in fails: - name = tc.attrib.get("fullname") or tc.attrib.get("name") or "" - f = tc.find("failure") - msg = (f.findtext("message") or "").strip() if f is not None else "" - stack = (f.findtext("stack-trace") or "").strip() if f is not None else "" - # First line of the message becomes the GitHub annotation title. - first_line = msg.splitlines()[0] if msg else "(no message)" - # GitHub annotations don't render multi-line bodies, so emit the full failure inside a collapsible group. - print(f"::error title=Failed: {esc_prop(name)}::{esc_data(first_line)}") - print(f"::group::Failure details — {esc_data(name)}") - if msg: - print("Message:") - print(msg) - if stack: - print("Stack trace:") - print(stack) - print("::endgroup::") - print(f"::error::{len(fails)} test(s) failed") - sys.exit(1) - PY + python3 tools/check_unity_test_results.py "$RESULTS_XML" --runner-outcome "$TEST_RUN_OUTCOME" - uses: actions/upload-artifact@v4 if: always() && steps.domain-tests.outcome != 'skipped' diff --git a/tools/check_unity_test_results.py b/tools/check_unity_test_results.py new file mode 100644 index 000000000..77832e504 --- /dev/null +++ b/tools/check_unity_test_results.py @@ -0,0 +1,69 @@ +"""Gate Unity CI on both the runner outcome and its NUnit result file.""" +from __future__ import annotations + +import argparse +from pathlib import Path +import xml.etree.ElementTree as ET + + +def escape_data(value: str) -> str: + return value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def escape_property(value: str) -> str: + return escape_data(value).replace(":", "%3A").replace(",", "%2C") + + +def check_results(path: Path, runner_outcome: str) -> int: + runner_failed = runner_outcome != "success" + if runner_failed: + print(f"::error::Unity test runner did not succeed: {escape_data(runner_outcome)}") + try: + root = ET.parse(path).getroot() + if root.tag != "test-run": + raise ValueError("Expected an NUnit test-run result") + total = int(root.attrib["total"]) + passed = int(root.attrib["passed"]) + failed = int(root.attrib["failed"]) + if min(total, passed, failed) < 0 or passed + failed > total: + raise ValueError("Invalid NUnit result counts") + except (OSError, ET.ParseError, ValueError, KeyError) as exc: + print(f"::error::Cannot validate Unity test results: {escape_data(str(exc))}") + return 1 + + print(f"Results: {passed} passed, {failed} failed (total: {total})") + failures = [case for case in root.iter("test-case") if case.get("result") == "Failed"] + for case in failures: + name = case.get("fullname") or case.get("name") or "" + failure = case.find("failure") + message = (failure.findtext("message") or "").strip() if failure is not None else "" + stack = (failure.findtext("stack-trace") or "").strip() if failure is not None else "" + first_line = message.splitlines()[0] if message else "(no message)" + print(f"::error title=Failed: {escape_property(name)}::{escape_data(first_line)}") + print(f"::group::Failure details — {escape_data(name)}") + # Escape test-controlled newlines so they cannot inject workflow commands. + if message: + print(f"Message: {escape_data(message)}") + if stack: + print(f"Stack trace: {escape_data(stack)}") + print("::endgroup::") + + if failures or failed or root.get("result") != "Passed": + print("::error::Unity reported an unsuccessful test run") + return 1 + if total == 0 or passed == 0: + print("::error::Unity did not execute any passing tests") + return 1 + return 1 if runner_failed else 0 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("results", type=Path) + parser.add_argument("--runner-outcome", required=True) + args = parser.parse_args() + return check_results(args.results, args.runner_outcome) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/tests/test_check_unity_test_results.py b/tools/tests/test_check_unity_test_results.py new file mode 100644 index 000000000..3a827cac6 --- /dev/null +++ b/tools/tests/test_check_unity_test_results.py @@ -0,0 +1,85 @@ +"""Exercise the CI gate with real result files and runner outcomes.""" +from pathlib import Path +import os +import subprocess +import sys + +import pytest + + +GATE = Path(__file__).resolve().parents[1] / "check_unity_test_results.py" +PASSING = '' + + +def run_gate(tmp_path, xml, outcome="success"): + results = tmp_path / "editmode-results.xml" + if xml is not None: + results.write_text(xml, encoding="utf-8") + return subprocess.run( + [sys.executable, str(GATE), str(results), "--runner-outcome", outcome], + capture_output=True, text=True, encoding="utf-8", + env={**os.environ, "PYTHONIOENCODING": "utf-8"}, + ) + + +def test_successful_runner_and_completed_results_pass(tmp_path): + result = run_gate(tmp_path, PASSING) + assert result.returncode == 0 + assert "1 passed" in result.stdout + + +@pytest.mark.parametrize("outcome", ["failure", "cancelled", "skipped", ""]) +def test_passing_xml_cannot_hide_runner_failure(tmp_path, outcome): + result = run_gate(tmp_path, PASSING, outcome) + assert result.returncode == 1 + assert "runner did not succeed" in result.stdout + + +@pytest.mark.parametrize("xml", [ + None, + "not XML", + '', + '', + '', + '', + '', +]) +def test_missing_or_invalid_results_fail(tmp_path, xml): + result = run_gate(tmp_path, xml) + assert result.returncode == 1 + assert "Cannot validate" in result.stdout + + +@pytest.mark.parametrize("xml", [ + '', + '', + '', + '', + '', +]) +def test_suite_or_case_failure_is_not_a_pass(tmp_path, xml): + result = run_gate(tmp_path, xml) + assert result.returncode == 1 + assert "unsuccessful test run" in result.stdout + + +@pytest.mark.parametrize("total", [0, 3]) +def test_empty_or_all_skipped_runs_fail(tmp_path, total): + result = run_gate(tmp_path, f'') + assert result.returncode == 1 + assert "did not execute" in result.stdout + + +def test_failure_details_cannot_inject_workflow_commands(tmp_path): + xml = ''' + + First line +::warning::injectedtrace +::error::injected + ''' + result = run_gate(tmp_path, xml) + assert result.returncode == 1 + assert "Suite%3AName%2CPercent%25" in result.stdout + assert "\n::warning::injected" not in result.stdout + assert "\n::error::injected" not in result.stdout + assert "%0A::warning::injected" in result.stdout From ff712fa8024d1f1c4b54dd4dd3a56cc49de9567c Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:02:52 -0400 Subject: [PATCH 04/12] fix(tests): bound focus recovery per job across concurrent polls --- Server/src/services/tools/run_tests.py | 290 ++++++++++---- Server/src/utils/focus_nudge.py | 2 +- Server/tests/test_test_job_focus_policy.py | 435 +++++++++++++++++++++ website/docs/guides/troubleshooting.md | 26 +- 4 files changed, 667 insertions(+), 86 deletions(-) create mode 100644 Server/tests/test_test_job_focus_policy.py diff --git a/Server/src/services/tools/run_tests.py b/Server/src/services/tools/run_tests.py index 9e94db32d..4994b2491 100644 --- a/Server/src/services/tools/run_tests.py +++ b/Server/src/services/tools/run_tests.py @@ -2,7 +2,12 @@ from __future__ import annotations import asyncio +from collections import OrderedDict +from dataclasses import dataclass +from itertools import count import logging +import ntpath +import posixpath import time from typing import Annotated, Any, Literal @@ -11,37 +16,59 @@ from pydantic import BaseModel from models import MCPResponse +from core.config import config from services.registry import mcp_for_unity_tool from services.tools import get_unity_instance_from_context from services.tools.preflight import preflight import transport.unity_transport as unity_transport from transport.legacy.unity_connection import async_send_command_with_retry from transport.plugin_hub import PluginHub -from utils.focus_nudge import nudge_unity_focus, should_nudge, reset_nudge_backoff +from utils import focus_nudge +from utils.focus_nudge import nudge_unity_focus, should_nudge logger = logging.getLogger(__name__) # Strong references to background fire-and-forget tasks to prevent premature GC. _background_tasks: set[asyncio.Task] = set() +_active_nudge_task: asyncio.Task | None = None +_MAX_JOB_NUDGES = 3 +_NUDGE_STATE_TTL_S = 3600.0 +_MAX_NUDGE_STATES = 256 +_poll_observation_order = count() -async def _get_unity_project_path(unity_instance: str | None) -> str | None: +@dataclass +class _JobNudgeState: + last_seen: float + last_update: int = 0 + completed: int = 0 + test_started: int = 0 + test_finished: int = 0 + attempts: int = 0 + last_attempt: float | None = None + task: asyncio.Task | None = None + run_in_background: bool = False + editor_is_focused: bool = True + latest_observation: int = -1 + + +_nudge_states: OrderedDict[tuple[str, str, str, str], _JobNudgeState] = OrderedDict() +_terminal_nudge_jobs: OrderedDict[tuple[str, str, str, str], float] = OrderedDict() + + +async def _get_unity_project_path(unity_instance: str | None, user_id: str | None = None) -> str | None: """Get the project root path for a Unity instance (for focus nudging). Args: unity_instance: Unity instance hash or "Name@hash" format or None Returns: - Project root path (e.g., "/Users/name/project"), or falls back to project_name if path unavailable + Exact absolute project root path, or None if the identity is unresolved. """ if not unity_instance: return None try: - registry = PluginHub._registry - if not registry: - return None - # Parse Name@hash format if present (middleware stores instances as "Name@hash") target_hash = unity_instance if "@" in target_hash: @@ -49,14 +76,32 @@ async def _get_unity_project_path(unity_instance: str | None) -> str | None: if not target_hash: return None - # Get session by hash - session_id = await registry.get_session_id_by_hash(target_hash) - if not session_id: + if unity_transport._is_http_transport(): + registry = PluginHub._registry + if not registry or (config.http_remote_hosted and not user_id): + return None + session_id = await registry.get_session_id_by_hash(target_hash, user_id=user_id) + session = await registry.get_session(session_id) if session_id else None + path = session.project_path if session else None + else: + from transport.legacy.stdio_port_registry import stdio_port_registry + instances = stdio_port_registry.get_instances() + matches = [instance for instance in instances if ( + instance.id == unity_instance or instance.hash == target_hash + )] + path = matches[0].path if len(matches) == 1 else None + # Stdio status files contain Application.dataPath, ending in Assets. + if path: + path_module = ntpath if ntpath.splitdrive(path)[0] or "\\" in path else posixpath + path = path_module.normpath(path) + if path_module.basename(path).lower() == "assets": + path = path_module.dirname(path) + if not path: return None - - session = await registry.get_session(session_id) - if not session: + path_module = ntpath if ntpath.splitdrive(path)[0] or "\\" in path else posixpath + if path_module is ntpath and not ntpath.splitdrive(path)[0]: return None + return path_module.normpath(path) if path_module.isabs(path) else None except Exception as e: # Re-raise cancellation errors so task cancellation propagates @@ -64,11 +109,142 @@ async def _get_unity_project_path(unity_instance: str | None) -> str | None: raise logger.debug(f"Could not get Unity project path: {e}") return None - else: - # Return full path if available, otherwise fall back to project name - if session.project_path: - return session.project_path - return session.project_name if session.project_name else None + + +async def _update_job_nudge( + unity_instance: str | None, user_id: str | None, job_id: str, + data: dict[str, Any], *, wait: bool, observation_order: int | None = None, +) -> None: + """Share a bounded, monotonic no-progress budget across every poll of a job.""" + global _active_nudge_task + if not unity_instance or (config.http_remote_hosted and not user_id): + return + if observation_order is None: + observation_order = next(_poll_observation_order) + instance_hash = unity_instance.rpartition("@")[2] + key = (config.transport_mode, user_id or "", instance_hash, job_id) + now = time.monotonic() + for terminal_key, observed_at in list(_terminal_nudge_jobs.items()): + if now - observed_at > _NUDGE_STATE_TTL_S: + del _terminal_nudge_jobs[terminal_key] + for old_key, old_state in list(_nudge_states.items()): + if now - old_state.last_seen > _NUDGE_STATE_TTL_S and not ( + old_state.task and not old_state.task.done() + ): + del _nudge_states[old_key] + if data.get("status") in ("succeeded", "failed", "cancelled"): + _terminal_nudge_jobs[key] = now + _terminal_nudge_jobs.move_to_end(key) + while len(_terminal_nudge_jobs) > _MAX_NUDGE_STATES: + _terminal_nudge_jobs.popitem(last=False) + state = _nudge_states.pop(key, None) + if state and state.task and not state.task.done(): + state.task.cancel() + return + if data.get("status") != "running": + return + progress = data.setdefault("progress", {}) or {} + data["progress"] = progress + if key in _terminal_nudge_jobs: + progress["focus_nudge_status"] = "terminal_already_observed" + return + state = _nudge_states.get(key) + if state is None: + # Do not evict an observed running job: that would renew its spent budget. + if len(_nudge_states) >= _MAX_NUDGE_STATES: + progress["focus_nudge_status"] = "tracking_limit" + return + state = _JobNudgeState(last_seen=now) + _nudge_states[key] = state + state.last_seen = now + _nudge_states.move_to_end(key) + advanced = False + for attr, value in ( + ("last_update", data.get("last_update_unix_ms")), + ("completed", progress.get("completed")), + ("test_started", progress.get("current_test_started_unix_ms")), + ("test_finished", progress.get("last_finished_unix_ms")), + ): + if isinstance(value, int) and not isinstance(value, bool) and value > getattr(state, attr): + setattr(state, attr, value) + advanced = True + if advanced: + state.attempts = 0 + state.last_attempt = None + # Focus can change without any test progress. Order UI observations by when + # their polls started, not by test timestamps or by reply arrival order. + if observation_order > state.latest_observation: + state.latest_observation = observation_order + state.run_in_background = progress.get("run_in_background") is True + state.editor_is_focused = progress.get("editor_is_focused", True) + progress["focus_nudge_attempts"] = state.attempts + progress["focus_nudge_limit"] = _MAX_JOB_NUDGES + if state.attempts >= _MAX_JOB_NUDGES: + progress["stuck_suspected"] = True + progress["focus_nudge_status"] = "attempt_limit_reached" + return + if state.run_in_background: + progress["focus_nudge_status"] = "background_execution_enabled" + return + if not should_nudge( + status="running", editor_is_focused=state.editor_is_focused, + last_update_unix_ms=state.last_update or None, + current_time_ms=int(time.time() * 1000), + ): + return + if _active_nudge_task is not None and not _active_nudge_task.done(): + return + interval = min(focus_nudge._BASE_NUDGE_INTERVAL_S * (2 ** state.attempts), focus_nudge._MAX_NUDGE_INTERVAL_S) + if state.last_attempt is not None and now - state.last_attempt < interval: + return + observed_progress = (state.last_update, state.completed, state.test_started, state.test_finished) + observed_reservation = (state.attempts, state.last_attempt) + project_path = await _get_unity_project_path(unity_instance, user_id) + if not project_path: + progress["focus_nudge_status"] = "project_path_unavailable" + return + # Resolution can await HTTP registry locks; recheck after another poll may + # have completed the job, reported progress, or reserved the desktop. + if _nudge_states.get(key) is not state or observed_progress != ( + state.last_update, state.completed, state.test_started, state.test_finished + ) or observed_reservation != (state.attempts, state.last_attempt) or ( + state.run_in_background or state.editor_is_focused + ) or ( + _active_nudge_task is not None and not _active_nudge_task.done() + ): + return + state.attempts += 1 + state.last_attempt = time.monotonic() + progress["focus_nudge_attempts"] = state.attempts + progress["focus_nudge_status"] = "scheduled" + + async def perform_nudge() -> None: + await nudge_unity_focus( + unity_project_path=project_path, force=True, + focus_duration_s=focus_nudge._DEFAULT_FOCUS_DURATION_S, + ) + + task = asyncio.create_task(perform_nudge()) + state.task = task + _active_nudge_task = task + _background_tasks.add(task) + + def finish(done: asyncio.Task) -> None: + global _active_nudge_task + _background_tasks.discard(done) + if _active_nudge_task is done: + _active_nudge_task = None + if state.task is done: + state.task = None + if not done.cancelled() and done.exception() is not None: + logger.warning("Test job focus nudge failed: %s", done.exception()) + + task.add_done_callback(finish) + if wait: + # Another poll may cancel the child after observing terminal status. + # Child cancellation must not cancel this request; caller cancellation + # still propagates through gather and cancels the nudge for focus restore. + await asyncio.gather(task, return_exceptions=True) class RunTestsSummary(BaseModel): @@ -122,6 +298,10 @@ class TestJobProgress(BaseModel): last_finished_unix_ms: int | None = None stuck_suspected: bool | None = None editor_is_focused: bool | None = None + run_in_background: bool | None = None + focus_nudge_attempts: int | None = None + focus_nudge_limit: int | None = None + focus_nudge_status: str | None = None blocked_reason: str | None = None failures_so_far: list[TestJobFailure] | None = None failures_capped: bool | None = None @@ -261,6 +441,7 @@ async def get_test_job( "Recommended: 30-60 seconds. Returns immediately if tests complete sooner."] = None, ) -> GetTestJobResponse | MCPResponse: unity_instance = await get_unity_instance_from_context(ctx) + user_id = await ctx.get_state("user_id") if config.http_remote_hosted else None params: dict[str, Any] = {"job_id": job_id} if include_failed_tests: @@ -268,25 +449,23 @@ async def get_test_job( if include_details: params["includeDetails"] = True - async def _fetch_status() -> dict[str, Any]: - return await unity_transport.send_with_unity_instance( + async def _fetch_status() -> tuple[Any, int]: + observation_order = next(_poll_observation_order) + response = await unity_transport.send_with_unity_instance( async_send_command_with_retry, unity_instance, "get_test_job", params, ) + return response, observation_order # If wait_timeout is specified, poll server-side until complete or timeout if wait_timeout and wait_timeout > 0: deadline = asyncio.get_event_loop().time() + wait_timeout poll_interval = 2.0 # Poll Unity every 2 seconds - prev_last_update_unix_ms = None - - # Get project path once for focus nudging (multi-instance support) - project_path = await _get_unity_project_path(unity_instance) while True: - response = await _fetch_status() + response, observation_order = await _fetch_status() if not isinstance(response, dict): return MCPResponse(success=False, error=str(response)) @@ -297,41 +476,12 @@ async def _fetch_status() -> dict[str, Any]: # Check if tests are done data = response.get("data", {}) status = data.get("status", "") + await _update_job_nudge( + unity_instance, user_id, job_id, data, wait=True, observation_order=observation_order, + ) if status in ("succeeded", "failed", "cancelled"): return GetTestJobResponse(**response) - # Detect progress and reset exponential backoff - last_update_unix_ms = data.get("last_update_unix_ms") - if prev_last_update_unix_ms is not None and last_update_unix_ms != prev_last_update_unix_ms: - # Progress detected - reset exponential backoff for next potential stall - reset_nudge_backoff() - logger.debug(f"Test job {job_id} made progress - reset nudge backoff") - prev_last_update_unix_ms = last_update_unix_ms - - # Check if Unity needs a focus nudge to make progress - # This handles OS-level throttling (e.g., macOS App Nap) that can - # stall PlayMode tests when Unity is in the background. - # Uses exponential backoff: 1s, 2s, 4s, 8s, 10s max between nudges. - progress = data.get("progress") or {} - editor_is_focused = progress.get("editor_is_focused", True) - current_time_ms = int(time.time() * 1000) - - if should_nudge( - status=status, - editor_is_focused=editor_is_focused, - last_update_unix_ms=last_update_unix_ms, - current_time_ms=current_time_ms, - # Use default stall_threshold_ms (3s) - ): - logger.info(f"Test job {job_id} appears stalled (unfocused Unity), attempting nudge...") - # Lazily resolve project path if not yet available (registry may have become ready) - if project_path is None: - project_path = await _get_unity_project_path(unity_instance) - # Pass project path for multi-instance support - nudged = await nudge_unity_focus(unity_project_path=project_path) - if nudged: - logger.info(f"Test job {job_id} nudge completed") - # Check timeout remaining = deadline - asyncio.get_event_loop().time() if remaining <= 0: @@ -342,32 +492,14 @@ async def _fetch_status() -> dict[str, Any]: await asyncio.sleep(min(poll_interval, remaining)) # No wait_timeout - return immediately (original behavior) - response = await _fetch_status() + response, observation_order = await _fetch_status() if not isinstance(response, dict): return MCPResponse(success=False, error=str(response)) if not response.get("success", True): return MCPResponse(**response) - # Fire-and-forget nudge check: even without wait_timeout, clients may poll - # externally. Check if Unity needs a nudge on every call so stalls get - # detected regardless of polling style. data = response.get("data", {}) - status = data.get("status", "") - if status == "running": - progress = data.get("progress") or {} - editor_is_focused = progress.get("editor_is_focused", True) - last_update_unix_ms = data.get("last_update_unix_ms") - current_time_ms = int(time.time() * 1000) - if should_nudge( - status=status, - editor_is_focused=editor_is_focused, - last_update_unix_ms=last_update_unix_ms, - current_time_ms=current_time_ms, - ): - logger.info(f"Test job {job_id} appears stalled (unfocused Unity), scheduling background nudge...") - project_path = await _get_unity_project_path(unity_instance) - task = asyncio.create_task(nudge_unity_focus(unity_project_path=project_path)) - _background_tasks.add(task) - task.add_done_callback(_background_tasks.discard) - + await _update_job_nudge( + unity_instance, user_id, job_id, data, wait=False, observation_order=observation_order, + ) return GetTestJobResponse(**response) diff --git a/Server/src/utils/focus_nudge.py b/Server/src/utils/focus_nudge.py index c860cd619..3da9ebc61 100644 --- a/Server/src/utils/focus_nudge.py +++ b/Server/src/utils/focus_nudge.py @@ -660,7 +660,7 @@ async def nudge_unity_focus( # Rate limit nudges using exponential backoff now = time.monotonic() - current_interval = _get_current_nudge_interval() + current_interval = 0.0 if force else _get_current_nudge_interval() if not force and (now - _last_nudge_time) < current_interval: logger.debug(f"Skipping nudge - too soon since last nudge (interval: {current_interval:.1f}s)") return False diff --git a/Server/tests/test_test_job_focus_policy.py b/Server/tests/test_test_job_focus_policy.py new file mode 100644 index 000000000..50efe9d80 --- /dev/null +++ b/Server/tests/test_test_job_focus_policy.py @@ -0,0 +1,435 @@ +"""Test-job focus budgets and identity routing without desktop activation.""" + +import asyncio +from collections import OrderedDict +from itertools import count +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest + +import services.tools.run_tests as mod + + +def snapshot(update=100, completed=0, **progress): + return {"job_id": "job", "status": "running", "last_update_unix_ms": update, + "progress": {"completed": completed, "editor_is_focused": False, **progress}} + + +@pytest.fixture +def policy(monkeypatch): + monkeypatch.setattr(mod, "_nudge_states", OrderedDict()) + monkeypatch.setattr(mod, "_terminal_nudge_jobs", OrderedDict()) + monkeypatch.setattr(mod, "_background_tasks", set()) + monkeypatch.setattr(mod, "_active_nudge_task", None) + monkeypatch.setattr(mod, "_poll_observation_order", count()) + monkeypatch.setattr(mod.config, "transport_mode", "stdio") + monkeypatch.setattr(mod.config, "http_remote_hosted", False) + monkeypatch.delenv("UNITY_MCP_DISABLE_FOCUS_NUDGE", raising=False) + monkeypatch.setattr(mod.focus_nudge, "_BASE_NUDGE_INTERVAL_S", 0) + monkeypatch.setattr(mod.focus_nudge, "_MAX_NUDGE_INTERVAL_S", 0) + monkeypatch.setattr(mod, "_get_unity_project_path", AsyncMock(return_value=r"C:\Project")) + nudge = AsyncMock(return_value=True) + monkeypatch.setattr(mod, "nudge_unity_focus", nudge) + return nudge + + +async def poll(data=None, instance="Game@hash", user=None, job="job", wait=True, observation_order=None): + data = snapshot() if data is None else data + await mod._update_job_nudge(instance, user, job, data, wait=wait, observation_order=observation_order) + return data + + +async def drain(): + if mod._background_tasks: + await asyncio.gather(*mod._background_tasks, return_exceptions=True) + + +@pytest.mark.asyncio +async def test_separate_polls_share_three_attempt_limit(policy): + for _ in range(6): + data = await poll(wait=False) + await drain() + assert policy.await_count == 3 + assert data["progress"]["focus_nudge_attempts"] == 3 + assert data["progress"]["stuck_suspected"] is True + assert data["progress"]["focus_nudge_status"] == "attempt_limit_reached" + + +@pytest.mark.asyncio +async def test_failed_attempts_are_also_bounded(policy): + policy.return_value = False + for _ in range(5): + await poll() + assert policy.await_count == 3 + + +@pytest.mark.asyncio +async def test_stale_alternating_pollers_cannot_renew_spent_budget(policy): + for update in (200, 100, 200, 100, 200, 100): + data = await poll(snapshot(update)) + assert policy.await_count == 3 + assert data["progress"]["focus_nudge_status"] == "attempt_limit_reached" + data = await poll(snapshot(300)) + assert policy.await_count == 4 + assert data["progress"]["focus_nudge_attempts"] == 1 + + +@pytest.mark.parametrize("progress", [ + {"completed": 1}, {"current_test_started_unix_ms": 200}, {"last_finished_unix_ms": 200}, +]) +@pytest.mark.asyncio +async def test_actual_test_progress_renews_only_its_job_budget(policy, progress): + for _ in range(3): + await poll(job="a") + await poll(job="b") + data = snapshot() + data["progress"].update(progress) + await poll(data, job="a") + capped = await poll(job="b") + assert policy.await_count == 7 + assert capped["progress"]["focus_nudge_status"] == "attempt_limit_reached" + + +@pytest.mark.asyncio +async def test_instance_and_user_budgets_are_isolated(policy): + for _ in range(3): + await poll(instance="Game@one", user="a") + await poll(instance="Game@two", user="a") + await poll(instance="Game@one", user="b") + await poll(instance="one", user="a") # Same canonical hash, same budget. + assert policy.await_count == 5 + assert len(mod._nudge_states) == 3 + + +@pytest.mark.asyncio +async def test_background_execution_skips_even_long_test_without_progress(policy): + data = await poll(snapshot(run_in_background=True)) + policy.assert_not_awaited() + assert data["progress"]["focus_nudge_status"] == "background_execution_enabled" + assert data["progress"]["focus_nudge_attempts"] == 0 + + +@pytest.mark.asyncio +async def test_old_editor_without_background_field_retains_bounded_nudges(policy): + await poll(snapshot()) + policy.assert_awaited_once() + assert policy.call_args.kwargs["force"] is True + assert policy.call_args.kwargs["focus_duration_s"] == mod.focus_nudge._DEFAULT_FOCUS_DURATION_S + + +@pytest.mark.asyncio +async def test_opt_out_does_not_spend_job_attempts(policy, monkeypatch): + monkeypatch.setenv("UNITY_MCP_DISABLE_FOCUS_NUDGE", "1") + data = await poll() + policy.assert_not_awaited() + assert data["progress"]["focus_nudge_attempts"] == 0 + + +@pytest.mark.asyncio +async def test_missing_identity_or_path_never_activates(policy): + await poll(instance=None) + assert not mod._nudge_states + mod._get_unity_project_path.return_value = None + data = await poll() + policy.assert_not_awaited() + assert data["progress"]["focus_nudge_attempts"] == 0 + assert data["progress"]["focus_nudge_status"] == "project_path_unavailable" + + +@pytest.mark.asyncio +async def test_one_active_task_does_not_spend_other_pollers_budgets(policy): + started, release = asyncio.Event(), asyncio.Event() + + async def wait_for_release(**kwargs): + started.set() + await release.wait() + return True + + policy.side_effect = wait_for_release + await poll(wait=False) + await asyncio.wait_for(started.wait(), 1) + try: + same = await poll(wait=False) + other = await poll(instance="Other@other", wait=False) + assert same["progress"]["focus_nudge_attempts"] == 1 + assert other["progress"]["focus_nudge_attempts"] == 0 + assert policy.await_count == 1 + finally: + release.set() + await drain() + await poll(instance="Other@other") + assert policy.await_count == 2 + + +@pytest.mark.asyncio +async def test_terminal_status_cancels_active_nudge_and_removes_state(policy): + started, release = asyncio.Event(), asyncio.Event() + + async def wait_forever(**kwargs): + started.set() + await release.wait() + + policy.side_effect = wait_forever + await poll(wait=False) + await asyncio.wait_for(started.wait(), 1) + task = mod._active_nudge_task + await poll({"status": "succeeded"}) + await drain() + assert task.cancelled() + assert not mod._nudge_states + + +@pytest.mark.asyncio +async def test_terminal_poll_does_not_cancel_another_waiting_poll(policy): + started, release = asyncio.Event(), asyncio.Event() + + async def wait_forever(**kwargs): + started.set() + await release.wait() + + policy.side_effect = wait_forever + waiting_poll = asyncio.create_task(poll(wait=True)) + await asyncio.wait_for(started.wait(), 1) + await poll({"status": "succeeded"}) + await waiting_poll + assert not waiting_poll.cancelled() + assert not mod._nudge_states + + +@pytest.mark.asyncio +async def test_caller_cancellation_still_cancels_nudge(policy): + started, release = asyncio.Event(), asyncio.Event() + + async def wait_forever(**kwargs): + started.set() + await release.wait() + + policy.side_effect = wait_forever + waiting_poll = asyncio.create_task(poll(wait=True)) + await asyncio.wait_for(started.wait(), 1) + child = mod._active_nudge_task + waiting_poll.cancel() + with pytest.raises(asyncio.CancelledError): + await waiting_poll + assert child.cancelled() + + +@pytest.mark.asyncio +async def test_stale_running_reply_after_terminal_does_not_recreate_budget(policy): + await poll({"status": "succeeded"}) + stale = await poll() + policy.assert_not_awaited() + assert not mod._nudge_states + assert stale["progress"]["focus_nudge_status"] == "terminal_already_observed" + + +@pytest.mark.parametrize("fresh_flags", [{"run_in_background": True}, {"editor_is_focused": True}]) +@pytest.mark.asyncio +async def test_stale_unfocused_reply_cannot_override_safe_fresh_observation(policy, fresh_flags): + await poll(snapshot(200, **fresh_flags), observation_order=2) + await poll(snapshot(100), observation_order=0) + await poll(snapshot(200), observation_order=1) + policy.assert_not_awaited() + + +@pytest.mark.parametrize("safe_flags", [{"editor_is_focused": True}, {"run_in_background": True}]) +@pytest.mark.asyncio +async def test_new_sequential_unfocused_observation_can_nudge_without_test_progress(policy, safe_flags): + await poll(snapshot(200, **safe_flags)) + policy.assert_not_awaited() + data = await poll(snapshot(200)) + policy.assert_awaited_once() + assert data["progress"]["focus_nudge_attempts"] == 1 + + +@pytest.mark.asyncio +async def test_actual_fetch_order_preserves_newer_flags_when_older_request_replies_late(policy, monkeypatch): + started, release = asyncio.Event(), asyncio.Event() + fetch_count = 0 + + async def send(*args, **kwargs): + nonlocal fetch_count + index = fetch_count + fetch_count += 1 + if index == 0: + started.set() + await release.wait() + return {"success": True, "data": snapshot(200, editor_is_focused=index == 1)} + + monkeypatch.setattr(mod, "get_unity_instance_from_context", AsyncMock(return_value="Game@hash")) + monkeypatch.setattr(mod.unity_transport, "send_with_unity_instance", send) + context = SimpleNamespace() + older_request = asyncio.create_task(mod.get_test_job(context, "job")) + try: + await asyncio.wait_for(started.wait(), 1) + await mod.get_test_job(context, "job") + finally: + release.set() + await older_request + await drain() + policy.assert_not_awaited() + # A later sequential unfocused observation is valid even with the same + # test-progress timestamp, and must not inherit the earlier focused flag. + await mod.get_test_job(context, "job") + await drain() + policy.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_staggered_concurrent_resolvers_cannot_bypass_budget_or_cooldown(policy, monkeypatch): + monkeypatch.setattr(mod.focus_nudge, "_BASE_NUDGE_INTERVAL_S", 60) + monkeypatch.setattr(mod.focus_nudge, "_MAX_NUDGE_INTERVAL_S", 60) + ready = [asyncio.Event() for _ in range(5)] + release = [asyncio.Event() for _ in range(5)] + arrivals = 0 + + async def delayed_resolve(*args): + nonlocal arrivals + index = arrivals + arrivals += 1 + ready[index].set() + await release[index].wait() + return r"C:\Project" + + mod._get_unity_project_path.side_effect = delayed_resolve + calls = [asyncio.create_task(poll()) for _ in range(5)] + try: + await asyncio.wait_for(asyncio.gather(*(event.wait() for event in ready)), 1) + for index, call in enumerate(calls): + release[index].set() + await call + finally: + for event in release: + event.set() + await asyncio.gather(*calls, return_exceptions=True) + assert policy.await_count == 1 + assert next(iter(mod._nudge_states.values())).attempts == 1 + + +@pytest.mark.asyncio +async def test_terminal_tombstones_are_bounded_and_expire(policy, monkeypatch): + monkeypatch.setattr(mod, "_MAX_NUDGE_STATES", 2) + for job in ("a", "b", "c"): + await poll({"status": "succeeded"}, job=job) + assert len(mod._terminal_nudge_jobs) == 2 + old_key = next(iter(mod._terminal_nudge_jobs)) + mod._terminal_nudge_jobs[old_key] -= mod._NUDGE_STATE_TTL_S + 1 + await poll(snapshot(run_in_background=True), job="other") + assert old_key not in mod._terminal_nudge_jobs + + +@pytest.mark.asyncio +async def test_state_capacity_does_not_evict_and_renew_capped_jobs(policy, monkeypatch): + monkeypatch.setattr(mod, "_MAX_NUDGE_STATES", 2) + for _ in range(3): + await poll(job="a") + await poll(snapshot(run_in_background=True), job="b") + rejected = await poll(job="c") + assert rejected["progress"]["focus_nudge_status"] == "tracking_limit" + assert len(mod._nudge_states) == 2 + await poll(job="a") + assert policy.await_count == 3 + await poll({"status": "succeeded"}, job="b") + await poll(job="c") + assert policy.await_count == 4 + + +@pytest.mark.asyncio +async def test_inactive_states_expire_without_unbounded_growth(policy): + await poll(snapshot(run_in_background=True), job="old") + old_key = next(iter(mod._nudge_states)) + mod._nudge_states[old_key].last_seen -= mod._NUDGE_STATE_TTL_S + 1 + await poll(snapshot(run_in_background=True), job="new") + assert old_key not in mod._nudge_states + assert len(mod._nudge_states) == 1 + + +@pytest.mark.asyncio +async def test_per_job_cooldown_is_not_reset_by_external_poll_boundaries(policy, monkeypatch): + monkeypatch.setattr(mod.focus_nudge, "_BASE_NUDGE_INTERVAL_S", 60) + monkeypatch.setattr(mod.focus_nudge, "_MAX_NUDGE_INTERVAL_S", 60) + await poll() + await poll() + assert policy.await_count == 1 + await poll(job="other") + assert policy.await_count == 2 + + +@pytest.mark.asyncio +async def test_resolution_race_with_terminal_status_does_not_schedule(policy): + entered, release = asyncio.Event(), asyncio.Event() + + async def resolve(*args): + entered.set() + await release.wait() + return r"C:\Project" + + mod._get_unity_project_path.side_effect = resolve + task = asyncio.create_task(poll()) + await asyncio.wait_for(entered.wait(), 1) + await poll({"status": "succeeded"}) + release.set() + await task + policy.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_external_and_wait_timeout_paths_share_policy_and_response_fields(policy, monkeypatch): + context = SimpleNamespace() + monkeypatch.setattr(mod, "get_unity_instance_from_context", AsyncMock(return_value="Game@hash")) + send = AsyncMock(side_effect=lambda *args, **kwargs: {"success": True, "data": snapshot()}) + monkeypatch.setattr(mod.unity_transport, "send_with_unity_instance", send) + for _ in range(3): + await mod.get_test_job(context, "job") + await drain() + capped = await mod.get_test_job(context, "job") + assert capped.data.progress.stuck_suspected is True + assert capped.data.progress.focus_nudge_attempts == 3 + send.side_effect = [ + {"success": True, "data": snapshot()}, + {"success": True, "data": {"job_id": "job", "status": "succeeded"}}, + ] + monkeypatch.setattr(mod.asyncio, "sleep", AsyncMock()) + completed = await mod.get_test_job(context, "job", wait_timeout=30) + assert completed.data.status == "succeeded" + assert policy.await_count == 3 + assert not mod._nudge_states + + +@pytest.mark.parametrize("path,expected", [ + (r"C:\Worktrees\Game\Assets", r"C:\Worktrees\Game"), + ("/worktrees/Game/Assets/", "/worktrees/Game"), + (r"C:\Game\MyAssets", r"C:\Game\MyAssets"), + ("Game/Assets", None), + (r"\Projects\Game\Assets", None), +]) +@pytest.mark.asyncio +async def test_stdio_path_uses_exact_registry_entry_and_strips_only_assets(monkeypatch, path, expected): + from transport.legacy.stdio_port_registry import stdio_port_registry + monkeypatch.setattr(mod.config, "transport_mode", "stdio") + monkeypatch.setattr(stdio_port_registry, "get_instances", lambda: [ + SimpleNamespace(id="Game@wrong", hash="wrong", path=r"C:\Wrong\Assets"), + SimpleNamespace(id="Game@right", hash="right", path=path), + ]) + assert await mod._get_unity_project_path("Game@right") == expected + assert await mod._get_unity_project_path("right") == expected + assert await mod._get_unity_project_path("Game") is None + assert await mod._get_unity_project_path("rig") is None + assert await mod._get_unity_project_path(None) is None + + +@pytest.mark.asyncio +async def test_http_path_uses_user_scope_and_never_name_fallback(monkeypatch): + monkeypatch.setattr(mod.config, "transport_mode", "http") + monkeypatch.setattr(mod.config, "http_remote_hosted", True) + registry = SimpleNamespace( + get_session_id_by_hash=AsyncMock(return_value="session"), + get_session=AsyncMock(return_value=SimpleNamespace(project_path=None, project_name="Game")), + ) + monkeypatch.setattr(mod.PluginHub, "_registry", registry) + assert await mod._get_unity_project_path("Game@hash", "user") is None + registry.get_session_id_by_hash.assert_awaited_once_with("hash", user_id="user") + registry.get_session.return_value.project_path = "/work/Game" + assert await mod._get_unity_project_path("Game@hash", "user") == "/work/Game" + assert await mod._get_unity_project_path("Game@hash") is None diff --git a/website/docs/guides/troubleshooting.md b/website/docs/guides/troubleshooting.md index 4bd2ec21d..07e53f905 100644 --- a/website/docs/guides/troubleshooting.md +++ b/website/docs/guides/troubleshooting.md @@ -226,15 +226,29 @@ for HTTP, set it in the environment that launches the shared server. The values `true`, `yes`, and `on` also disable nudges. Forced nudges respect this setting. On Windows, the nudge now requires an absolute project path matching exactly one -running `Unity.exe` process. If the path cannot be resolved, including some stdio -sessions, it skips activation. It restores the previous window by its saved HWND, +running `Unity.exe` process. Stdio sessions resolve that path from the selected +instance's registry entry. If the path cannot be resolved, it skips activation. +It restores the previous window by its saved HWND, so a changing window title does not prevent focus restoration. Windows can still deny an activation request, in which case the server reports failure. -This mitigates the desktop disruption reported in -[#1407](https://github.com/CoplayDev/unity-mcp/issues/1407). A long healthy test can -still trigger the no-progress heuristic, and nudges currently have no per-job -attempt limit. Disable them when background tests already run reliably. +For [#1407](https://github.com/CoplayDev/unity-mcp/issues/1407), the server now skips +nudges when the editor reports `run_in_background: true`. Otherwise, each test +job has a budget of three attempts without newer test progress. Both immediate +polls and `wait_timeout` polls share that budget, and only one nudge runs at a +time per server. Each attempt uses the configured focus duration instead of +escalating the duration after repeated polls. + +When the budget is exhausted, `get_test_job` reports +`progress.stuck_suspected: true` and +`progress.focus_nudge_status: "attempt_limit_reached"`. The test itself keeps +running; the server stops taking focus. Newer test progress renews that job's +budget, while older replies cannot renew it. Older Unity packages that do not +report `run_in_background` still use the bounded attempts. + +Budgets belong to the running Python server and expire after an hour without a +poll; separate stdio server processes maintain separate budgets. Disable nudges +entirely when background tests already run reliably. --- From 9f8152cc835f48ad1eed76b9b5ac50564462ceb7 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:03:00 -0400 Subject: [PATCH 05/12] fix(tests): recover test job callbacks and terminal errors after reload --- MCPForUnity/Editor/Services/TestJobManager.cs | 67 ++- .../Editor/Services/TestRunnerNoThrottle.cs | 8 +- .../Editor/Services/TestRunnerService.cs | 132 +++++- .../Services/TestJobManagerLifecycleTests.cs | 428 ++++++++++++++++++ .../TestJobManagerLifecycleTests.cs.meta | 11 + 5 files changed, 629 insertions(+), 17 deletions(-) create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs.meta diff --git a/MCPForUnity/Editor/Services/TestJobManager.cs b/MCPForUnity/Editor/Services/TestJobManager.cs index bdf626036..b20b2f7cf 100644 --- a/MCPForUnity/Editor/Services/TestJobManager.cs +++ b/MCPForUnity/Editor/Services/TestJobManager.cs @@ -46,6 +46,7 @@ internal sealed class TestJob /// /// Tracks async test jobs started via MCP tools. This is not intended to capture manual Test Runner UI runs. /// + [InitializeOnLoad] internal static class TestJobManager { // Keep this small to avoid ballooning payloads during polling. @@ -69,6 +70,42 @@ static TestJobManager() { // Restore after domain reloads (e.g., compilation while a job is running). TryRestoreFromSessionState(); + AssemblyReloadEvents.beforeAssemblyReload += BeforeAssemblyReload; + RestoreRunningJobCallbacks(); + } + + private static void BeforeAssemblyReload() + { + // Progress callbacks are normally throttled. Flush the last update before the + // managed domain (and the original RunTestsAsync task) is discarded. + PersistToSessionState(force: true); + } + + private static void RestoreRunningJobCallbacks() + { + TestJob job; + lock (LockObj) + { + if (string.IsNullOrEmpty(_currentJobId) || + !Jobs.TryGetValue(_currentJobId, out job) || job.Status != TestJobStatus.Running) + { + return; + } + } + + try + { + // Polling a restored job never otherwise touches the lazy test service. + // Re-register callbacks now, before the Test Runner resumes after reload. + if (MCPServiceLocator.Tests is TestRunnerService service) + { + service.ResumeJobAfterReload(job.JobId, job.Mode); + } + } + catch (Exception ex) + { + McpLog.Warn($"[TestJobManager] Failed to restore test callbacks: {ex.Message}"); + } } public static string CurrentJobId @@ -381,7 +418,33 @@ public static void FinalizeCurrentJobFromRunFinished(TestRunResult resultPayload : TestJobStatus.Succeeded; job.Error = null; job.Result = resultPayload; + if (resultPayload != null) + { + job.TotalTests = resultPayload.Total; + job.CompletedTests = resultPayload.Total; + } job.CurrentTestFullName = null; + job.CurrentTestStartedUnixMs = null; + _currentJobId = null; + } + PersistToSessionState(force: true); + } + + internal static void FinalizeCurrentJobFromRunError(string message) + { + long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); + lock (LockObj) + { + if (string.IsNullOrEmpty(_currentJobId) || !Jobs.TryGetValue(_currentJobId, out var job)) + { + return; + } + job.Status = TestJobStatus.Failed; + job.Error = message; + job.LastUpdateUnixMs = now; + job.FinishedUnixMs = now; + job.CurrentTestFullName = null; + job.CurrentTestStartedUnixMs = null; _currentJobId = null; } PersistToSessionState(force: true); @@ -539,7 +602,7 @@ internal static object ToSerializable(TestJob job, bool includeDetails, bool inc } object resultPayload = null; - if (job.Status == TestJobStatus.Succeeded && job.Result != null) + if (job.Status != TestJobStatus.Running && job.Result != null) { resultPayload = job.Result.ToSerializable(job.Mode, includeDetails, includeFailedTests); } @@ -562,6 +625,7 @@ internal static object ToSerializable(TestJob job, bool includeDetails, bool inc last_finished_unix_ms = job.LastFinishedUnixMs, stuck_suspected = IsStuck(job), editor_is_focused = InternalEditorUtility.isApplicationActive, + run_in_background = UnityEngine.Application.runInBackground, blocked_reason = GetBlockedReason(job), failures_so_far = BuildFailuresPayload(job.FailuresSoFar), failures_capped = (job.FailuresSoFar != null && job.FailuresSoFar.Count >= FailureCap) @@ -685,4 +749,3 @@ private static void FinalizeFromTask(string jobId, Task task) } } } - diff --git a/MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs b/MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs index ddcfe8702..b1a1dbfef 100644 --- a/MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs +++ b/MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs @@ -130,7 +130,7 @@ private static void ForceEditorToApplyInteractionPrefs() } } - private sealed class TestCallbacks : ICallbacks + private sealed class TestCallbacks : IErrorCallbacks { public void RunStarted(ITestAdaptor testsToRun) { @@ -143,6 +143,12 @@ public void RunFinished(ITestResultAdaptor result) RestoreThrottling(); } + public void OnError(string message) + { + // Build/prebuild failures may terminate before RunFinished is delivered. + RestoreThrottling(); + } + public void TestStarted(ITestAdaptor test) { } public void TestFinished(ITestResultAdaptor result) { } } diff --git a/MCPForUnity/Editor/Services/TestRunnerService.cs b/MCPForUnity/Editor/Services/TestRunnerService.cs index 13c6f0056..f16a389d9 100644 --- a/MCPForUnity/Editor/Services/TestRunnerService.cs +++ b/MCPForUnity/Editor/Services/TestRunnerService.cs @@ -34,10 +34,15 @@ internal static class PlayModeOptionsGuard private static readonly string MarkerPath = Path.Combine("Library", "MCPPlayModeOptionsBackup.txt"); static PlayModeOptionsGuard() + { + RestoreIfIdle(); + } + + internal static void RestoreIfIdle() { // After domain reload or editor restart: if a restore is pending and no test run // is active, restore now. TryLoad checks SessionState first, then the marker file. - if (TryLoad(out _, out _) && !TestRunStatus.IsRunning) + if (TryLoad(out _, out _) && !TestRunStatus.IsRunning && !TestJobManager.HasRunningJob) { Restore(); } @@ -143,7 +148,7 @@ private static bool TryLoad(out bool originalEnabled, out EnterPlayModeOptions o /// Concrete implementation of . /// Coordinates Unity Test Runner operations and produces structured results. /// - internal sealed class TestRunnerService : ITestRunnerService, ICallbacks, IDisposable + internal sealed class TestRunnerService : ITestRunnerService, IErrorCallbacks, IDisposable { private static readonly TestMode[] AllModes = { TestMode.EditMode, TestMode.PlayMode }; @@ -151,13 +156,33 @@ internal sealed class TestRunnerService : ITestRunnerService, ICallbacks, IDispo private readonly SemaphoreSlim _operationLock = new SemaphoreSlim(1, 1); private readonly List _leafResults = new List(); private TaskCompletionSource _runCompletionSource; + private string _trackedJobId; public TestRunnerService() { _testRunnerApi = ScriptableObject.CreateInstance(); + _testRunnerApi.hideFlags = HideFlags.HideAndDontSave; _testRunnerApi.RegisterCallbacks(this); } + internal void ResumeJobAfterReload(string jobId, string mode) + { + if (_runCompletionSource != null || _trackedJobId != null || + string.IsNullOrEmpty(jobId) || TestJobManager.CurrentJobId != jobId) + { + return; + } + + _trackedJobId = jobId; + if (Enum.TryParse(mode, out var testMode)) + { + TestRunStatus.MarkStarted(testMode); + } + } + + private bool IsTrackingCurrentJob => + !string.IsNullOrEmpty(_trackedJobId) && TestJobManager.CurrentJobId == _trackedJobId; + public async Task>> GetTestsAsync(TestMode? mode) { await _operationLock.WaitAsync().ConfigureAwait(true); @@ -187,6 +212,13 @@ public async Task>> GetTestsAsync(TestM public async Task RunTestsAsync(TestMode mode, TestFilterOptions filterOptions = null) { + // The pre-reload Task no longer exists, but its Unity run may still be active. + // Clearing a job must not let a second run consume the first run's callbacks. + if (_trackedJobId != null && _runCompletionSource == null) + { + throw new InvalidOperationException("A recovered Unity test run is still in progress."); + } + await _operationLock.WaitAsync().ConfigureAwait(true); Task runTask; bool adjustedPlayModeOptions = false; @@ -219,6 +251,7 @@ public async Task RunTestsAsync(TestMode mode, TestFilterOptions } _leafResults.Clear(); + _trackedJobId = TestJobManager.CurrentJobId; _runCompletionSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); // Mark running immediately so readiness snapshots reflect the busy state even before callbacks fire. TestRunStatus.MarkStarted(mode); @@ -252,6 +285,8 @@ public async Task RunTestsAsync(TestMode mode, TestFilterOptions } catch { + _trackedJobId = null; + _runCompletionSource = null; // Ensure the status is cleared if we failed to start the run. TestRunStatus.MarkFinished(); if (adjustedPlayModeOptions) @@ -302,6 +337,10 @@ public void Dispose() public void RunStarted(ITestAdaptor testsToRun) { _leafResults.Clear(); + if (!IsTrackingCurrentJob) + { + return; + } try { // Best-effort progress info for async polling (avoid heavy payloads). @@ -325,31 +364,64 @@ public void RunFinished(ITestResultAdaptor result) // is recreated and _runCompletionSource is lost, but TestJobManager state persists via // SessionState and the Test Runner still delivers the RunFinished callback. var payload = TestRunResult.Create(result, _leafResults); + CompleteRun(payload, null); + } - // Clean up state regardless of _runCompletionSource - these methods safely handle - // the case where no MCP job exists (e.g., manual test runs via Unity UI). - TestRunStatus.MarkFinished(); - TestJobManager.OnRunFinished(); - TestJobManager.FinalizeCurrentJobFromRunFinished(payload); + public void OnError(string message) + { + // Build/prebuild failures use IErrorCallbacks instead of RunFinished. + CompleteRun(null, new InvalidOperationException(message ?? "Unity test run failed.")); + } + + private void CompleteRun(TestRunResult payload, Exception error) + { + + // A late callback from a cleared job must not finish a newer job or restore + // settings belonging to it. Callbacks do not contain an MCP job identifier. + bool ownsCurrentJob = IsTrackingCurrentJob; + bool canCleanUp = ownsCurrentJob || !TestJobManager.HasRunningJob; + if (canCleanUp) + { + TestRunStatus.MarkFinished(); + } + if (ownsCurrentJob) + { + if (error == null) + { + TestJobManager.OnRunFinished(); + TestJobManager.FinalizeCurrentJobFromRunFinished(payload); + } + else + { + TestJobManager.FinalizeCurrentJobFromRunError(error.Message); + } + } // If a domain reload destroyed the original RunTestsAsync caller, the finally block // that would normally restore EditorSettings never ran. Restore from SessionState. - if (_runCompletionSource == null && PlayModeOptionsGuard.IsPending) + if (canCleanUp && _trackedJobId != null && _runCompletionSource == null && PlayModeOptionsGuard.IsPending) { PlayModeOptionsGuard.Restore(); } // Report result to awaiting caller if we have a completion source. // The caller's finally block handles restoration in this case. - if (_runCompletionSource != null) + var completion = _runCompletionSource; + _runCompletionSource = null; + _trackedJobId = null; + if (completion != null) { - _runCompletionSource.TrySetResult(payload); - _runCompletionSource = null; + if (error == null) completion.TrySetResult(payload); + else completion.TrySetException(error); } } public void TestStarted(ITestAdaptor test) { + if (!IsTrackingCurrentJob) + { + return; + } try { // Prefer FullName for uniqueness; fall back to Name. @@ -373,7 +445,7 @@ public void TestFinished(ITestResultAdaptor result) return; } - if (!result.HasChildren) + if (!result.HasChildren && result.Test?.IsSuite != true) { _leafResults.Add(result); try @@ -402,7 +474,10 @@ public void TestFinished(ITestResultAdaptor result) // ignore adaptor quirks } - TestJobManager.OnLeafTestFinished(fullName, isFailure, message); + if (IsTrackingCurrentJob) + { + TestJobManager.OnLeafTestFinished(fullName, isFailure, message); + } } catch { @@ -638,7 +713,16 @@ public object ToSerializable(string mode, bool includeDetails = false, bool incl internal static TestRunResult Create(ITestResultAdaptor summary, IReadOnlyList tests) { - var materializedTests = tests.Select(TestRunTestResult.FromAdaptor).ToList(); + // RunFinished includes the complete result tree, including tests completed + // before a domain reload erased the service's per-test callback list. + var resultLeaves = new List(); + if (summary != null && summary.HasChildren) + { + CollectResultLeaves(summary, resultLeaves); + } + var materializedTests = (resultLeaves.Count > 0 ? resultLeaves : tests) + .Where(t => t != null && t.Test?.IsSuite != true) + .Select(TestRunTestResult.FromAdaptor).ToList(); int passed = summary?.PassCount ?? materializedTests.Count(t => string.Equals(t.State, "Passed", StringComparison.OrdinalIgnoreCase)); @@ -662,6 +746,26 @@ internal static TestRunResult Create(ITestResultAdaptor summary, IReadOnlyList leaves) + { + if (result == null) + { + return; + } + if (!result.HasChildren) + { + if (result.Test?.IsSuite != true) leaves.Add(result); + return; + } + if (result.Children != null) + { + foreach (var child in result.Children) + { + CollectResultLeaves(child, leaves); + } + } + } } public sealed class TestRunSummary diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs new file mode 100644 index 000000000..a6f694bfa --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs @@ -0,0 +1,428 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Reflection; +using MCPForUnity.Editor.Services; +using Newtonsoft.Json.Linq; +using NUnit.Framework; +using NUnit.Framework.Interfaces; +using UnityEditor; +using UnityEditor.TestTools.TestRunner.Api; +using UnityEngine; +using UnityEngine.TestTools; +using RunState = UnityEditor.TestTools.TestRunner.Api.RunState; +using TestStatus = UnityEditor.TestTools.TestRunner.Api.TestStatus; + +namespace MCPForUnityTests.Editor.Services +{ + /// + /// Exercises reload recovery without starting a nested Unity test run. The synthetic + /// callbacks have the same result tree contract as TestRunnerApi.RunFinished. + /// + public class TestJobManagerLifecycleTests + { + private const BindingFlags PrivateStatic = BindingFlags.NonPublic | BindingFlags.Static; + private const string JobsKey = "MCPForUnity.TestJobsV1"; + private const string CurrentKey = "MCPForUnity.CurrentTestJobIdV1"; + private const string GuardPrefix = "MCPForUnity.PlayModeOptions."; + private const string MarkerPath = "Library/MCPPlayModeOptionsBackup.txt"; + private Dictionary _jobs; + private Dictionary _originalJobs; + private string _originalCurrent; + private string _originalSessionJobs; + private string _originalSessionCurrent; + private object _originalService; + private object _originalLastPersist; + private Dictionary _originalStatus; + private bool _originalOptionsEnabled; + private EnterPlayModeOptions _originalOptions; + private bool _guardPending; + private bool _guardEnabled; + private int _guardOptions; + private byte[] _marker; + + private static FieldInfo ManagerField(string name) => typeof(TestJobManager).GetField(name, PrivateStatic); + private static FieldInfo ServiceField => typeof(MCPServiceLocator).GetField("_testRunnerService", PrivateStatic); + private static void InvokeManager(string name, params object[] args) => + typeof(TestJobManager).GetMethod(name, PrivateStatic).Invoke(null, args); + + [SetUp] + public void SetUp() + { + _jobs = (Dictionary)ManagerField("Jobs").GetValue(null); + _originalJobs = new Dictionary(_jobs); + _originalCurrent = TestJobManager.CurrentJobId; + _originalSessionJobs = SessionState.GetString(JobsKey, string.Empty); + _originalSessionCurrent = SessionState.GetString(CurrentKey, string.Empty); + _originalLastPersist = ManagerField("_lastPersistUnixMs").GetValue(null); + _originalService = ServiceField.GetValue(null); + _originalStatus = typeof(TestRunStatus).GetFields(PrivateStatic) + .Where(f => !f.IsInitOnly).ToDictionary(f => f, f => f.GetValue(null)); + _originalOptionsEnabled = EditorSettings.enterPlayModeOptionsEnabled; + _originalOptions = EditorSettings.enterPlayModeOptions; + _guardPending = SessionState.GetBool(GuardPrefix + "PendingRestore", false); + _guardEnabled = SessionState.GetBool(GuardPrefix + "OriginalEnabled", false); + _guardOptions = SessionState.GetInt(GuardPrefix + "OriginalOptions", 0); + _marker = File.Exists(MarkerPath) ? File.ReadAllBytes(MarkerPath) : null; + _jobs.Clear(); + ManagerField("_currentJobId").SetValue(null, null); + ServiceField.SetValue(null, null); + TestRunStatus.MarkFinished(); + PlayModeOptionsGuard.Clear(); + } + + [TearDown] + public void TearDown() + { + // Do not reset the whole service locator: the outer Editor test run may own it. + (ServiceField.GetValue(null) as IDisposable)?.Dispose(); + ServiceField.SetValue(null, _originalService); + _jobs.Clear(); + foreach (var entry in _originalJobs) _jobs.Add(entry.Key, entry.Value); + ManagerField("_currentJobId").SetValue(null, _originalCurrent); + ManagerField("_lastPersistUnixMs").SetValue(null, _originalLastPersist); + SessionState.SetString(JobsKey, _originalSessionJobs); + SessionState.SetString(CurrentKey, _originalSessionCurrent); + foreach (var entry in _originalStatus) entry.Key.SetValue(null, entry.Value); + EditorSettings.enterPlayModeOptions = _originalOptions; + EditorSettings.enterPlayModeOptionsEnabled = _originalOptionsEnabled; + SessionState.SetBool(GuardPrefix + "PendingRestore", _guardPending); + SessionState.SetBool(GuardPrefix + "OriginalEnabled", _guardEnabled); + SessionState.SetInt(GuardPrefix + "OriginalOptions", _guardOptions); + if (_marker == null) + { + if (File.Exists(MarkerPath)) File.Delete(MarkerPath); + } + else File.WriteAllBytes(MarkerPath, _marker); + } + + private TestJob AddJob(string id = "lifecycle-job") + { + long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); + var job = new TestJob + { + JobId = id, Mode = "EditMode", Status = TestJobStatus.Running, + StartedUnixMs = now, LastUpdateUnixMs = now, + FailuresSoFar = new List() + }; + _jobs[id] = job; + ManagerField("_currentJobId").SetValue(null, id); + return job; + } + + private TestRunnerService RestoreCallbacks() + { + InvokeManager("RestoreRunningJobCallbacks"); + return ServiceField.GetValue(null) as TestRunnerService; + } + + [Test] + public void RestoreRunningJob_RecreatesLazyServiceOnce_WithoutRestartingRun() + { + var job = AddJob(); + job.CompletedTests = 3; + job.TotalTests = 8; + InvokeManager("PersistToSessionState", true); + _jobs.Clear(); + ManagerField("_currentJobId").SetValue(null, null); + InvokeManager("TryRestoreFromSessionState"); + + var service = RestoreCallbacks(); + Assert.NotNull(service); + Assert.AreSame(service, RestoreCallbacks(), "Repeated recovery must not register a second service."); + Assert.IsTrue(TestRunStatus.IsRunning); + Assert.AreEqual(TestMode.EditMode, TestRunStatus.Mode); + Assert.AreEqual(3, _jobs[job.JobId].CompletedTests, "Recovery must not reset progress or execute tests again."); + Assert.AreEqual(8, _jobs[job.JobId].TotalTests); + } + + [Test] + public void RestoreCallbacks_WithoutRunningJob_DoesNotCreateService() + { + Assert.IsNull(RestoreCallbacks()); + Assert.IsFalse(TestRunStatus.IsRunning); + } + + [Test] + public void BeforeReload_FlushesProgressDespitePersistenceThrottle() + { + var job = AddJob(); + InvokeManager("PersistToSessionState", true); + ManagerField("_lastPersistUnixMs").SetValue(null, DateTimeOffset.UtcNow.ToUnixTimeMilliseconds() + 1000); + TestJobManager.OnLeafTestFinished("Fixture.LastTest", true, "assertion failed"); + Assert.AreEqual(0, (int)JObject.Parse(SessionState.GetString(JobsKey, ""))["jobs"][0]["completed_tests"]); + + InvokeManager("BeforeAssemblyReload"); + _jobs.Clear(); + InvokeManager("TryRestoreFromSessionState"); + + Assert.AreEqual(1, _jobs[job.JobId].CompletedTests); + Assert.AreEqual("Fixture.LastTest", _jobs[job.JobId].LastFinishedTestFullName); + Assert.AreEqual("assertion failed", _jobs[job.JobId].FailuresSoFar.Single().Message); + } + + [Test] + public void RecoveredRunFinished_FinalizesWithoutOriginalTask_AndRetainsEarlierResults() + { + var job = AddJob(); + var service = RestoreCallbacks(); + var passedBeforeReload = new ResultStub("Fixture.BeforeReload", "Passed"); + var failedAfterReload = new ResultStub("Fixture.AfterReload", "Failed", "assertion failed"); + service.TestFinished(failedAfterReload); + service.RunFinished(ResultStub.Suite(passedBeforeReload, failedAfterReload)); + + Assert.AreEqual(TestJobStatus.Failed, job.Status); + Assert.IsNull(TestJobManager.CurrentJobId); + Assert.IsFalse(TestRunStatus.IsRunning); + Assert.AreEqual(2, job.CompletedTests); + Assert.AreEqual(2, job.Result.Results.Count); + var payload = JObject.FromObject(TestJobManager.ToSerializable(job, false, true)); + Assert.AreEqual(1, (int)payload["result"]["summary"]["failed"]); + Assert.AreEqual("assertion failed", (string)payload["result"]["results"][0]["message"]); + } + + [Test] + public void LateCallbacksFromRecoveredJob_DoNotMutateNewJobOrItsSettings() + { + AddJob("old-job"); + var service = RestoreCallbacks(); + var newer = AddJob("new-job"); + newer.CompletedTests = 7; + newer.TotalTests = 9; + PlayModeOptionsGuard.Save(false, EnterPlayModeOptions.None); + EditorSettings.enterPlayModeOptionsEnabled = true; + EditorSettings.enterPlayModeOptions = EnterPlayModeOptions.DisableDomainReload; + + service.RunStarted(null); + service.TestFinished(new ResultStub("Old.Finished", "Failed")); + service.RunFinished(ResultStub.Suite(new ResultStub("Old.Finished", "Failed"))); + + Assert.AreEqual("new-job", TestJobManager.CurrentJobId); + Assert.AreEqual(TestJobStatus.Running, newer.Status); + Assert.AreEqual(7, newer.CompletedTests); + Assert.AreEqual(9, newer.TotalTests); + Assert.IsTrue(TestRunStatus.IsRunning); + Assert.IsTrue(PlayModeOptionsGuard.IsPending); + Assert.IsTrue(EditorSettings.enterPlayModeOptionsEnabled); + Assert.AreEqual(EnterPlayModeOptions.DisableDomainReload, EditorSettings.enterPlayModeOptions); + } + + [Test] + public void RecoveredRun_CannotStartOverlappingRunAfterJobIsCleared() + { + AddJob(); + var service = RestoreCallbacks(); + TestJobManager.ClearStuckJob(); + Assert.Throws(() => service.RunTestsAsync(TestMode.EditMode).GetAwaiter().GetResult()); + } + + [Test] + public void RecoveredInitializationError_FailsJobAndRestoresSettings() + { + var job = AddJob(); + var service = RestoreCallbacks(); + PlayModeOptionsGuard.Save(false, EnterPlayModeOptions.None); + EditorSettings.enterPlayModeOptionsEnabled = true; + EditorSettings.enterPlayModeOptions = EnterPlayModeOptions.DisableDomainReload; + + ((IErrorCallbacks)service).OnError("Prebuild setup failed"); + + Assert.AreEqual(TestJobStatus.Failed, job.Status); + Assert.AreEqual("Prebuild setup failed", job.Error); + Assert.IsNull(TestJobManager.CurrentJobId); + Assert.IsFalse(TestRunStatus.IsRunning); + Assert.IsFalse(PlayModeOptionsGuard.IsPending); + Assert.IsFalse(EditorSettings.enterPlayModeOptionsEnabled); + } + + [UnityTest] + public IEnumerator ClearedRecoveredRun_AfterRunFinished_CanStartNextRun() => CheckRecoveredRunRestart(false); + + [UnityTest] + public IEnumerator ClearedRecoveredRun_AfterError_CanStartNextRun() => CheckRecoveredRunRestart(true); + + private IEnumerator CheckRecoveredRunRestart(bool error) + { + AddJob("old-job"); + var service = RestoreCallbacks(); + TestJobManager.ClearStuckJob(); + if (error) service.OnError("Old initialization failed"); + else service.RunFinished(ResultStub.Suite(new ResultStub("Old.Pass", "Passed"))); + + // Replace only this API instance's scheduler so this does not execute a nested + // test run. The project pins Test Framework 1.1.33, which exposes this test seam. + var api = typeof(TestRunnerService).GetField("_testRunnerApi", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(service); + var scheduler = typeof(TestRunnerApi).GetField("ScheduleJob", BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(scheduler); + bool scheduled = false; + scheduler.SetValue(api, new Func(_ => { scheduled = true; return "synthetic-run"; })); + var nextJob = AddJob("next-job"); + var pending = service.RunTestsAsync(TestMode.EditMode); + Assert.IsTrue(scheduled, "Terminal callbacks must release recovered ownership."); + service.RunFinished(ResultStub.Suite(new ResultStub("Next.Pass", "Passed"))); + double deadline = EditorApplication.timeSinceStartup + 5; + while (!pending.IsCompleted && EditorApplication.timeSinceStartup < deadline) yield return null; + Assert.IsTrue(pending.IsCompleted, "Completion must release the original async waiter."); + var result = pending.GetAwaiter().GetResult(); + Assert.AreEqual(1, result.Passed); + Assert.AreEqual(TestJobStatus.Succeeded, nextJob.Status); + } + + [Test] + public void ResultTree_ExcludesEmptySuitesFromIndividualResults() + { + var emptySuite = new ResultStub("Fixture.EmptySuite", "Failed") { IsSuite = true }; + var leaf = new ResultStub("Fixture.Test", "Passed"); + var result = TestRunResult.Create(ResultStub.Suite(emptySuite, leaf), new ITestResultAdaptor[] { emptySuite }); + Assert.AreEqual(1, result.Results.Count); + Assert.AreEqual("Fixture.Test", result.Results[0].FullName); + } + + [Test] + public void PlayModeGuard_KeepsOriginalSettingsBackupUntilRecoveredRunFinishes() + { + AddJob(); + PlayModeOptionsGuard.Save(false, EnterPlayModeOptions.None); + EditorSettings.enterPlayModeOptionsEnabled = true; + EditorSettings.enterPlayModeOptions = EnterPlayModeOptions.DisableDomainReload; + // TestRunStatus is transient and initially false after a reload. + PlayModeOptionsGuard.RestoreIfIdle(); + Assert.IsTrue(PlayModeOptionsGuard.IsPending); + Assert.IsTrue(EditorSettings.enterPlayModeOptionsEnabled); + var service = RestoreCallbacks(); + service.RunFinished(ResultStub.Suite(new ResultStub("Fixture.Pass", "Passed"))); + Assert.IsFalse(PlayModeOptionsGuard.IsPending); + Assert.IsFalse(EditorSettings.enterPlayModeOptionsEnabled); + Assert.AreEqual(EnterPlayModeOptions.None, EditorSettings.enterPlayModeOptions); + } + + [Test] + public void FailedRun_ReturnsSummaryWithoutDetailsUnlessRequested() + { + var job = AddJob(); + job.Status = TestJobStatus.Failed; + job.Result = TestRunResult.Create(ResultStub.Suite(new ResultStub("Fixture.Fail", "Failed")), Array.Empty()); + var payload = JObject.FromObject(TestJobManager.ToSerializable(job, false, false)); + Assert.AreEqual("failed", (string)payload["status"]); + Assert.AreEqual(1, (int)payload["result"]["summary"]["failed"]); + Assert.AreEqual(JTokenType.Null, payload["result"]["results"].Type); + } + + [Test] + public void Snapshot_ReportsActualRunInBackgroundSetting() + { + var payload = JObject.FromObject(TestJobManager.ToSerializable(AddJob(), false, false)); + Assert.AreEqual(Application.runInBackground, (bool)payload["progress"]["run_in_background"]); + } + + [Test] + public void NoThrottle_ErrorBeforeRunStarted_RestoresCapturedPreferences() + { + const string activeKey = "TestRunnerNoThrottle_TestRunActive"; + const string capturedKey = "TestRunnerNoThrottle_SettingsCaptured"; + const string idleKey = "TestRunnerNoThrottle_PrevIdleTime"; + const string modeKey = "TestRunnerNoThrottle_PrevInteractionMode"; + bool active = SessionState.GetBool(activeKey, false); + bool captured = SessionState.GetBool(capturedKey, false); + int previousIdle = SessionState.GetInt(idleKey, 4); + int previousMode = SessionState.GetInt(modeKey, 0); + bool hadIdle = EditorPrefs.HasKey("ApplicationIdleTime"); + bool hadMode = EditorPrefs.HasKey("InteractionMode"); + int idle = EditorPrefs.GetInt("ApplicationIdleTime", 4); + int mode = EditorPrefs.GetInt("InteractionMode", 0); + try + { + SessionState.SetBool(capturedKey, false); + EditorPrefs.SetInt("ApplicationIdleTime", 7); + EditorPrefs.SetInt("InteractionMode", 0); + TestRunnerNoThrottle.ApplyNoThrottlingPreemptive(); + Assert.AreEqual(0, EditorPrefs.GetInt("ApplicationIdleTime")); + Assert.AreEqual(1, EditorPrefs.GetInt("InteractionMode")); + + var callbackType = typeof(TestRunnerNoThrottle).GetNestedType("TestCallbacks", BindingFlags.NonPublic); + var callback = Activator.CreateInstance(callbackType, true) as IErrorCallbacks; + Assert.NotNull(callback, "Initialization errors must be observed even before RunStarted."); + callback.OnError("Prebuild setup failed"); + + Assert.AreEqual(7, EditorPrefs.GetInt("ApplicationIdleTime")); + Assert.AreEqual(0, EditorPrefs.GetInt("InteractionMode")); + Assert.IsFalse(SessionState.GetBool(activeKey, true)); + Assert.IsFalse(SessionState.GetBool(capturedKey, true)); + } + finally + { + SessionState.SetBool(activeKey, active); + SessionState.SetBool(capturedKey, captured); + SessionState.SetInt(idleKey, previousIdle); + SessionState.SetInt(modeKey, previousMode); + if (hadIdle) EditorPrefs.SetInt("ApplicationIdleTime", idle); + else EditorPrefs.DeleteKey("ApplicationIdleTime"); + if (hadMode) EditorPrefs.SetInt("InteractionMode", mode); + else EditorPrefs.DeleteKey("InteractionMode"); + typeof(TestRunnerNoThrottle).GetMethod("ForceEditorToApplyInteractionPrefs", PrivateStatic).Invoke(null, null); + } + } + + private sealed class ResultStub : ITestResultAdaptor + { + private readonly ITestResultAdaptor[] _children; + public ResultStub(string name, string state, string message = null, params ITestResultAdaptor[] children) + { + Name = FullName = name; + ResultState = state; + Message = message; + _children = children; + } + public static ResultStub Suite(params ITestResultAdaptor[] children) => new ResultStub("Suite", "Failed", null, children); + public bool IsSuite { get; set; } + public ITestAdaptor Test => new TestStub(FullName, IsSuite || HasChildren); + public string Name { get; } + public string FullName { get; } + public string ResultState { get; } + public TestStatus TestStatus => ResultState == "Passed" ? TestStatus.Passed : TestStatus.Failed; + public double Duration => 0.1; + public DateTime StartTime => DateTime.UtcNow; + public DateTime EndTime => DateTime.UtcNow; + public string Message { get; } + public string StackTrace => "Fixture.cs:12"; + public int AssertCount => 1; + public int FailCount => HasChildren ? _children.Sum(c => c.FailCount) : ResultState == "Failed" ? 1 : 0; + public int PassCount => HasChildren ? _children.Sum(c => c.PassCount) : ResultState == "Passed" ? 1 : 0; + public int SkipCount => 0; + public int InconclusiveCount => 0; + public bool HasChildren => _children.Length > 0; + public IEnumerable Children => _children; + public string Output => "test output"; + public TNode ToXml() => new TNode("test-case"); + } + + private sealed class TestStub : ITestAdaptor + { + public TestStub(string name, bool isSuite) { Name = name; IsSuite = isSuite; } + public string Id => Name; + public string Name { get; } + public string FullName => Name; + public int TestCaseCount => IsSuite ? 0 : 1; + public bool HasChildren => false; + public bool IsSuite { get; } + public IEnumerable Children => Array.Empty(); + public ITestAdaptor Parent => null; + public int TestCaseTimeout => 0; + public ITypeInfo TypeInfo => null; + public IMethodInfo Method => null; + public string[] Categories => Array.Empty(); + public bool IsTestAssembly => false; + public RunState RunState => RunState.Runnable; + public string Description => null; + public string SkipReason => null; + public string ParentId => null; + public string ParentFullName => null; + public string UniqueName => Name; + public string ParentUniqueName => null; + public int ChildIndex => 0; + public TestMode TestMode => TestMode.EditMode; + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs.meta new file mode 100644 index 000000000..db849dd94 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: d8f9272c68b34207bc6554cf156e6144 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From a1ae80ae88d2dbbeffd471daa973a3a1715df87b Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 12:02:23 -0400 Subject: [PATCH 06/12] fix(tests): adapt Unity test fixtures and validate CI results --- .github/workflows/unity-tests.yml | 16 +++- .../Services/TestJobManagerLifecycleTests.cs | 2 + tools/check_unity_test_results.py | 24 +++++- tools/tests/test_check_unity_test_results.py | 75 ++++++++++++++----- tools/tests/test_unity_tests_workflow.py | 24 ++++++ 5 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 tools/tests/test_unity_tests_workflow.py diff --git a/.github/workflows/unity-tests.yml b/.github/workflows/unity-tests.yml index 201ba9738..14c96feba 100644 --- a/.github/workflows/unity-tests.yml +++ b/.github/workflows/unity-tests.yml @@ -168,8 +168,11 @@ jobs: Library- # Run domain reload tests first (they're [Explicit] so need explicit category) + # Both runner steps pin the action and its CLI. The floating v4 tag and the CLI's "latest" + # release changed behavior under this workflow once (red beta since 2026-09-05); a pinned + # cliVersion also skips the unauthenticated releases/latest lookup. Bump both on purpose. - name: Run domain reload tests - uses: game-ci/unity-test-runner@v4 + uses: game-ci/unity-test-runner@32e57712352b500e17974b245a6dce9e11a73213 # v4 id: domain-tests env: UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} @@ -181,12 +184,20 @@ jobs: unityVersion: ${{ matrix.unityVersion }} testMode: ${{ matrix.testMode }} customParameters: -testCategory domain_reload + cliVersion: v0.1.69 # Results are gated locally; this read-only job cannot publish Checks API results. githubToken: "" artifactsPath: artifacts/domain-reload + # A runner failure already fails the job here; this also fails a run that executed no test. + - name: Check domain reload test results + env: + RESULTS_XML: artifacts/domain-reload/${{ matrix.testMode }}-results.xml + TEST_RUN_OUTCOME: ${{ steps.domain-tests.outcome }} + run: python3 tools/check_unity_test_results.py "$RESULTS_XML" --runner-outcome "$TEST_RUN_OUTCOME" + - name: Run tests - uses: game-ci/unity-test-runner@v4 + uses: game-ci/unity-test-runner@32e57712352b500e17974b245a6dce9e11a73213 # v4 id: tests continue-on-error: true env: @@ -198,6 +209,7 @@ jobs: projectPath: ${{ matrix.projectPath }} unityVersion: ${{ matrix.unityVersion }} testMode: ${{ matrix.testMode }} + cliVersion: v0.1.69 githubToken: "" # Keep the preceding domain-reload XML out of the regular suite's result gate. artifactsPath: artifacts/editmode diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs index a6f694bfa..8fbee8a02 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TestJobManagerLifecycleTests.cs @@ -13,6 +13,7 @@ using UnityEngine; using UnityEngine.TestTools; using RunState = UnityEditor.TestTools.TestRunner.Api.RunState; +using TestMode = UnityEditor.TestTools.TestRunner.Api.TestMode; using TestStatus = UnityEditor.TestTools.TestRunner.Api.TestStatus; namespace MCPForUnityTests.Editor.Services @@ -412,6 +413,7 @@ private sealed class TestStub : ITestAdaptor public int TestCaseTimeout => 0; public ITypeInfo TypeInfo => null; public IMethodInfo Method => null; + public object[] Arguments => Array.Empty(); public string[] Categories => Array.Empty(); public bool IsTestAssembly => false; public RunState RunState => RunState.Runnable; diff --git a/tools/check_unity_test_results.py b/tools/check_unity_test_results.py index 77832e504..77109f717 100644 --- a/tools/check_unity_test_results.py +++ b/tools/check_unity_test_results.py @@ -25,13 +25,25 @@ def check_results(path: Path, runner_outcome: str) -> int: total = int(root.attrib["total"]) passed = int(root.attrib["passed"]) failed = int(root.attrib["failed"]) - if min(total, passed, failed) < 0 or passed + failed > total: + inconclusive = int(root.attrib["inconclusive"]) + skipped = int(root.attrib["skipped"]) + # Unity writes total as exactly the sum of these four buckets (NUnit 3.5 has no Warning). + counts = (total, passed, failed, inconclusive, skipped) + if min(counts) < 0 or passed + failed + inconclusive + skipped != total: raise ValueError("Invalid NUnit result counts") except (OSError, ET.ParseError, ValueError, KeyError) as exc: print(f"::error::Cannot validate Unity test results: {escape_data(str(exc))}") return 1 - print(f"Results: {passed} passed, {failed} failed (total: {total})") + print(f"Results: {passed} passed, {failed} failed, {inconclusive} inconclusive, {skipped} skipped (total: {total})") + # Unity's command-line runner exits 2 (failed) for any Inconclusive test (Assert.Inconclusive, + # Assume.That), so the runner outcome already fails such a run. Name each one so the log says why. + inconclusive_cases = [case for case in root.iter("test-case") if case.get("result") == "Inconclusive"] + for case in inconclusive_cases: + name = case.get("fullname") or case.get("name") or "" + reason = (case.findtext("reason/message") or "").strip() + first_line = reason.splitlines()[0] if reason else "(no message)" + print(f"::error title=Inconclusive: {escape_property(name)}::{escape_data(first_line)}") failures = [case for case in root.iter("test-case") if case.get("result") == "Failed"] for case in failures: name = case.get("fullname") or case.get("name") or "" @@ -48,12 +60,18 @@ def check_results(path: Path, runner_outcome: str) -> int: print(f"Stack trace: {escape_data(stack)}") print("::endgroup::") - if failures or failed or root.get("result") != "Passed": + # result is NUnit's ResultState string: "Passed", "Failed(Child)", "Failed:Cancelled", ... A clean + # run that contains any [Ignore]d test reports "Skipped:Ignored", so accept Skipped as well. + status = root.get("result", "").split(":")[0].split("(")[0] + if failures or failed or status not in ("Passed", "Skipped"): print("::error::Unity reported an unsuccessful test run") return 1 if total == 0 or passed == 0: print("::error::Unity did not execute any passing tests") return 1 + if inconclusive or inconclusive_cases: + print("::error::Unity fails a run with inconclusive tests; use Assert.Ignore for environment guards") + return 1 return 1 if runner_failed else 0 diff --git a/tools/tests/test_check_unity_test_results.py b/tools/tests/test_check_unity_test_results.py index 3a827cac6..6c578e06a 100644 --- a/tools/tests/test_check_unity_test_results.py +++ b/tools/tests/test_check_unity_test_results.py @@ -8,7 +8,17 @@ GATE = Path(__file__).resolve().parents[1] / "check_unity_test_results.py" -PASSING = '' +COUNTS = 'inconclusive="0" skipped="0"' +PASSING = '' +# attributes exactly as Unity wrote them on green beta run 33978935244 (all four Unity versions): +# a clean run that contains [Ignore]d tests reports result="Skipped:Ignored", not "Passed". +UNITY_CLEAN_RUN = '' +# Beta run 29283113713 (6000.0.75f1), before #1294 moved ManageGraphicsTests off Assume.That. +UNITY_INCONCLUSIVE_RUN = ''' + + + ''' def run_gate(tmp_path, xml, outcome="success"): @@ -22,10 +32,25 @@ def run_gate(tmp_path, xml, outcome="success"): ) -def test_successful_runner_and_completed_results_pass(tmp_path): - result = run_gate(tmp_path, PASSING) - assert result.returncode == 0 - assert "1 passed" in result.stdout +@pytest.mark.parametrize("xml", [PASSING, UNITY_CLEAN_RUN]) +def test_successful_runner_and_completed_results_pass(tmp_path, xml): + result = run_gate(tmp_path, xml) + assert result.returncode == 0, result.stdout + assert "failed, 0 inconclusive" in result.stdout + + +# Unity exits 2 when any test is Inconclusive, so with githubToken "" game-ci fails the step and CI +# passes "failure"; that run logged "0 failed" and then "Exiting with code 2". A local caller may pass +# "success". Either way the gate must fail and name the inconclusive test. +@pytest.mark.parametrize("outcome", ["failure", "success"]) +def test_inconclusive_tests_fail_and_are_named(tmp_path, outcome): + result = run_gate(tmp_path, UNITY_INCONCLUSIVE_RUN, outcome) + assert result.returncode == 1, result.stdout + assert "18 inconclusive, 48 skipped (total: 1166)" in result.stdout + assert "::error title=Inconclusive: MCPForUnityTests.Editor.Tools.ManageGraphicsTests" in result.stdout + assert "URP not available" in result.stdout + assert "use Assert.Ignore" in result.stdout + assert "\n::error::injected" not in result.stdout @pytest.mark.parametrize("outcome", ["failure", "cancelled", "skipped", ""]) @@ -38,11 +63,16 @@ def test_passing_xml_cannot_hide_runner_failure(tmp_path, outcome): @pytest.mark.parametrize("xml", [ None, "not XML", - '', + f'', '', - '', - '', - '', + f'', + f'', + f'', + # Outcome buckets must account for every test, not just stay under total. + f'', + '', + '', + '', ]) def test_missing_or_invalid_results_fail(tmp_path, xml): result = run_gate(tmp_path, xml) @@ -51,11 +81,15 @@ def test_missing_or_invalid_results_fail(tmp_path, xml): @pytest.mark.parametrize("xml", [ - '', - '', - '', - '', - '', + f'', + f'', + f'', + f'', + f'', + f'', + '', + f'', + f'', ]) def test_suite_or_case_failure_is_not_a_pass(tmp_path, xml): result = run_gate(tmp_path, xml) @@ -63,15 +97,20 @@ def test_suite_or_case_failure_is_not_a_pass(tmp_path, xml): assert "unsuccessful test run" in result.stdout -@pytest.mark.parametrize("total", [0, 3]) -def test_empty_or_all_skipped_runs_fail(tmp_path, total): - result = run_gate(tmp_path, f'') +@pytest.mark.parametrize("result_state, inconclusive, skipped", [ + ("Passed", 0, 0), ("Skipped:Ignored", 0, 3), ("Skipped:Ignored", 2, 1), +]) +def test_runs_without_a_passing_test_fail(tmp_path, result_state, inconclusive, skipped): + total = inconclusive + skipped + xml = (f'') + result = run_gate(tmp_path, xml) assert result.returncode == 1 assert "did not execute" in result.stdout def test_failure_details_cannot_inject_workflow_commands(tmp_path): - xml = ''' + xml = f''' First line ::warning::injectedtrace diff --git a/tools/tests/test_unity_tests_workflow.py b/tools/tests/test_unity_tests_workflow.py new file mode 100644 index 000000000..a87dce4a8 --- /dev/null +++ b/tools/tests/test_unity_tests_workflow.py @@ -0,0 +1,24 @@ +"""The Unity test workflow must not float on game-ci's v4 tag or its CLI's latest release.""" +from pathlib import Path +import re + + +WORKFLOW = Path(__file__).resolve().parents[2] / ".github" / "workflows" / "unity-tests.yml" + + +def runner_steps(): + text = WORKFLOW.read_text(encoding="utf-8") + starts = [match.start() for match in re.finditer(r"^ - ", text, re.M)] + [len(text)] + steps = [text[begin:end] for begin, end in zip(starts, starts[1:])] + return [step for step in steps if "game-ci/unity-test-runner@" in step] + + +def test_every_runner_step_pins_the_action_commit_and_cli_release(): + steps = runner_steps() + assert len(steps) == 2 + for step in steps: + ref = re.search(r"uses: game-ci/unity-test-runner@(\S+)", step).group(1) + assert re.fullmatch(r"[0-9a-f]{40}", ref), ref + assert re.search(r"^ cliVersion: v\d+\.\d+\.\d+$", step, re.M), step + # The read-only job cannot create a check run; the local gate reads the XML instead. + assert re.search(r'^ githubToken: ""$', step, re.M), step From f11fc1d2c6c69bf5a9ec4cc9ecbcb46ab53946f3 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:13:41 -0400 Subject: [PATCH 07/12] fix(ci): activate and return Unity licenses consistently in E2E --- .github/workflows/e2e-bridge.yml | 89 ++++----------- tools/ci_unity_license.py | 164 +++++++++++++++++++++++++++ tools/local_harness.py | 1 + tools/tests/test_ci_unity_license.py | 148 ++++++++++++++++++++++++ 4 files changed, 334 insertions(+), 68 deletions(-) create mode 100644 tools/ci_unity_license.py create mode 100644 tools/tests/test_ci_unity_license.py diff --git a/.github/workflows/e2e-bridge.yml b/.github/workflows/e2e-bridge.yml index 62f8f305f..fb59e51c8 100644 --- a/.github/workflows/e2e-bridge.yml +++ b/.github/workflows/e2e-bridge.yml @@ -15,6 +15,8 @@ on: - "Server/src/**" - "Server/tests/e2e/**" - "tools/local_harness.py" + - "tools/ci_unity_license.py" + - "tools/tests/test_ci_unity_license.py" - ".github/workflows/e2e-bridge.yml" permissions: @@ -50,7 +52,7 @@ jobs: UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} run: | set -e - if [ -n "$UNITY_LICENSE" ] || { [ -n "$UNITY_EMAIL" ] && [ -n "$UNITY_PASSWORD" ] && [ -n "$UNITY_SERIAL" ]; }; then + if [ -n "$UNITY_LICENSE$UNITY_EMAIL$UNITY_PASSWORD$UNITY_SERIAL" ]; then echo "unity_ok=true" >> "$GITHUB_OUTPUT" else echo "unity_ok=false" >> "$GITHUB_OUTPUT" @@ -90,89 +92,39 @@ jobs: echo "$GITHUB_WORKSPACE/.venv/bin" >> "$GITHUB_PATH" uv pip install -e Server - # --- License staging (mirrors claude-nl-suite.yml) --- - - name: Decide license sources - id: lic - shell: bash + # Activate with the same pinned GameCI implementation that runs our tests. + # Personal account credentials do not require a professional serial key. + - name: Prepare Unity license and activation helpers + id: prepare_license env: UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} - run: | - set -eu - use_ulf=false; use_ebl=false - [[ -n "${UNITY_LICENSE:-}" ]] && use_ulf=true - [[ -n "${UNITY_EMAIL:-}" && -n "${UNITY_PASSWORD:-}" && -n "${UNITY_SERIAL:-}" ]] && use_ebl=true - echo "use_ulf=$use_ulf" >> "$GITHUB_OUTPUT" - echo "use_ebl=$use_ebl" >> "$GITHUB_OUTPUT" - - - name: Stage Unity .ulf license (from secret) - if: steps.lic.outputs.use_ulf == 'true' - id: ulf - env: - UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} - shell: bash - run: | - set -eu - mkdir -p "$RUNNER_TEMP/unity-license-ulf" "$RUNNER_TEMP/unity-local/Unity" - f="$RUNNER_TEMP/unity-license-ulf/Unity_lic.ulf" - if printf "%s" "$UNITY_LICENSE" | base64 -d - >/dev/null 2>&1; then - printf "%s" "$UNITY_LICENSE" | base64 -d - > "$f" - else - printf "%s" "$UNITY_LICENSE" > "$f" - fi - chmod 600 "$f" || true - if grep -qi '' "$f"; then - cp -f "$f" "$RUNNER_TEMP/unity-local/Unity/Unity_lic.ulf" - echo "ok=true" >> "$GITHUB_OUTPUT" - else - echo "ok=false" >> "$GITHUB_OUTPUT" - fi + run: python3 tools/ci_unity_license.py prepare - - name: Activate Unity (EBL via container) - if: steps.lic.outputs.use_ebl == 'true' - shell: bash + - name: Activate Unity license env: - UNITY_IMAGE: ${{ env.UNITY_IMAGE }} UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} - run: | - set -euo pipefail - mkdir -p "$RUNNER_TEMP/unity-config" "$RUNNER_TEMP/unity-local" - docker run --rm --network host \ - -e HOME=/root -e UNITY_EMAIL -e UNITY_PASSWORD -e UNITY_SERIAL \ - -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ - -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \ - "$UNITY_IMAGE" bash -lc ' - # No -x here: xtrace would echo the expanded -password/-serial arguments into - # the job log, and GitHub secret masking is a last line of defence, not a design. - set -euo pipefail - /opt/unity/Editor/Unity -batchmode -nographics -logFile - \ - -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -serial "$UNITY_SERIAL" -quit || true - ' + run: python3 tools/ci_unity_license.py activate - name: Warm up project (import Library once) shell: bash env: UNITY_IMAGE: ${{ env.UNITY_IMAGE }} - ULF_OK: ${{ steps.ulf.outputs.ok }} run: | set -euxo pipefail - manual_args=() - if [[ "${ULF_OK:-false}" == "true" ]]; then - manual_args=(-manualLicenseFile "/root/.local/share/unity3d/Unity/Unity_lic.ulf") - fi docker run --rm --network host \ -e HOME=/root \ + -v "$RUNNER_TEMP/unity-machine-id:/etc/machine-id:ro" \ -v "${{ github.workspace }}:${{ github.workspace }}" -w "${{ github.workspace }}" \ -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \ -v "$RUNNER_TEMP/unity-cache:/root/.cache/unity3d" \ "$UNITY_IMAGE" /opt/unity/Editor/Unity -batchmode -nographics -logFile - \ -projectPath "${{ github.workspace }}/TestProjects/UnityMCPTests" \ - "${manual_args[@]}" -quit + -quit - name: Clean old MCP status run: | @@ -184,23 +136,24 @@ jobs: shell: bash env: UNITY_IMAGE: ${{ env.UNITY_IMAGE }} - ULF_OK: ${{ steps.ulf.outputs.ok }} run: | set -euxo pipefail # In --ci mode the harness drives the DockerLauncher: it runs the same # docker container (repo .unity-mcp status dir, docker liveness/teardown, # log redaction), waits on the status file, derives the instance, then # runs the smoke + EditMode + PlayMode legs over the bridge. - license_args=() - if [[ "${ULF_OK:-false}" == "true" ]]; then - license_args=(--editor-arg -manualLicenseFile \ - --editor-arg "/root/.local/share/unity3d/Unity/Unity_lic.ulf") - fi python3 tools/local_harness.py --ci \ --legs smoke,editmode,playmode \ --project-path TestProjects/UnityMCPTests \ - --reports reports \ - "${license_args[@]}" + --reports reports + + - name: Return Unity license seat + if: always() && steps.prepare_license.outcome == 'success' + env: + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + run: python3 tools/ci_unity_license.py return - name: Unity logs on failure if: failure() diff --git a/tools/ci_unity_license.py b/tools/ci_unity_license.py new file mode 100644 index 000000000..d0d28776e --- /dev/null +++ b/tools/ci_unity_license.py @@ -0,0 +1,164 @@ +"""Prepare and activate CI licensing with GameCI's tested Linux activation paths. + +Only stages repository-provided secrets. XML checks select a candidate; Unity +itself decides whether it is licensed. Never print raw activation output, which +can contain credentials, license serials and account identifiers. +""" + +from __future__ import annotations + +import argparse +import base64 +import os +from pathlib import Path +import subprocess +import urllib.request +import uuid +import xml.etree.ElementTree as ET + +# The CLI version used by our successful Unity CI activation (v0.1.69). +# Pin the implementation, including its capability-based older-editor fallback. +GAME_CI_COMMIT = "75c5dcf81523f31b3cca9d7cd16228a2b53de9a8" +GAME_CI_SCRIPTS = ( + "activate.sh", "return_license.sh", "licensing_method.sh", "resolve_unity_path.sh", +) + + +def decode_ulf(value: str) -> bytes: + """Accept raw/base64 XML, including XML signatures with namespace/attributes.""" + raw = value.strip().encode("utf-8") + candidates = [raw] + try: + candidates.append(base64.b64decode(b"".join(raw.split()), validate=True)) + except ValueError: + pass + for candidate in candidates: + try: + root = ET.fromstring(candidate) + except ET.ParseError: + continue + if any(node.tag.rsplit("}", 1)[-1] == "Signature" for node in root.iter()): + return candidate + raise ValueError("UNITY_LICENSE is not signed XML (raw or base64 encoded).") + + +def prepare(directory: Path, environ: dict[str, str]) -> None: + directory.mkdir(parents=True, exist_ok=True) + credentials = bool(environ.get("UNITY_EMAIL") and environ.get("UNITY_PASSWORD")) + license_file = directory / "input.ulf" + license_file.unlink(missing_ok=True) + if environ.get("UNITY_LICENSE"): + try: + candidate = decode_ulf(environ["UNITY_LICENSE"]) + except ValueError: + if not credentials: + raise + print("::warning::UNITY_LICENSE is not signed XML; trying the configured Unity account.") + else: + license_file.write_bytes(candidate) + license_file.chmod(0o600) + if not license_file.exists() and not credentials: + raise ValueError("Unity activation requires a license file or UNITY_EMAIL and UNITY_PASSWORD.") + + steps = directory / "steps" + steps.mkdir(exist_ok=True) + for name in GAME_CI_SCRIPTS: + url = (f"https://raw.githubusercontent.com/game-ci/cli/{GAME_CI_COMMIT}/" + f"dist/platforms/ubuntu/steps/{name}") + with urllib.request.urlopen(url, timeout=30) as response: + (steps / name).write_bytes(response.read()) + # A new identity for this job, shared by activation, both Editors and return. + # Reuse it when preparation is retried within the same job. + identity = directory.parent / "unity-machine-id" + if not identity.exists(): + identity.write_text(uuid.uuid4().hex + "\n", encoding="ascii") + + +CONTAINER_SCRIPT = r''' +set +eux +export STEPS_DIR="${STEPS_DIR:-/steps}" ACTIVATE_LICENSE_PATH="${ACTIVATE_LICENSE_PATH:-/activation}" +# Match GameCI's headless editor launcher while retaining its licensing logic. +unity-editor() { /opt/unity/Editor/Unity -batchmode -nographics "$@"; } +export -f unity-editor +if [ "$1" = activate ]; then + source "$STEPS_DIR/activate.sh" || exit $? + # The file strategy can acquire a Personal seat through account fallback. + printf '%s' "${GAME_CI_ACTIVATED_VIA:-}" > "$ACTIVATE_LICENSE_PATH/activated-via" +else + export GAME_CI_ACTIVATED_VIA="$(cat "$ACTIVATE_LICENSE_PATH/activated-via" 2>/dev/null)" + source "$STEPS_DIR/return_license.sh" || exit $? + exit "${RETURN_EXIT_CODE:-0}" +fi +''' + + +def docker_args(directory: Path, image: str, operation: str) -> list[str]: + if operation not in {"activate", "return"}: + raise ValueError("Unsupported licensing operation") + runner_temp = directory.parent + args = ["docker", "run", "--rm", "--network", "host", "-e", "HOME=/root"] + # Environment names only: values are never put on the host command line. + for name in ("UNITY_EMAIL", "UNITY_PASSWORD", "UNITY_SERIAL"): + args.extend(["-e", name]) + args.extend(["-e", "UNITY_LICENSE=", "-e", "UNITY_LICENSING_SERVER="]) + args.extend(["-e", "UNITY_LICENSE_FILE=" + ( + "/activation/input.ulf" if (directory / "input.ulf").exists() else "")]) + for source, target in ( + (directory, "/activation"), + (directory / "steps", "/steps:ro"), + (runner_temp / "unity-machine-id", "/etc/machine-id:ro"), + (runner_temp / "unity-config", "/root/.config/unity3d"), + (runner_temp / "unity-local", "/root/.local/share/unity3d"), + (runner_temp / "unity-cache", "/root/.cache/unity3d"), + ): + args.extend(["-v", f"{source}:{target}"]) + return [*args, image, "bash", "-c", CONTAINER_SCRIPT, "ci-unity-license", operation] + + +def run_license(directory: Path, image: str, operation: str) -> int: + if operation == "return" and not (directory / "activation-attempted").exists(): + return 0 + if operation == "activate": + (directory / "activation-attempted").touch() + result = subprocess.run(docker_args(directory, image, operation), capture_output=True, + text=True, encoding="utf-8", errors="replace", check=False) + if result.returncode: + output = (result.stdout + result.stderr).lower() + reason = "Unity or Docker rejected the licensing operation" + for signature, description in ( + ("machine bindings don't match", "license file belongs to a different machine"), + ("no seat available", "no Unity license seat is available for this account"), + ("two-factor", "account requires two-factor authentication"), + ("invalid credentials", "Unity rejected the account credentials"), + ): + if signature in output: + reason = description + break + print(f"::error::Unity license {operation} failed: {reason} (exit {result.returncode}). " + "Raw licensing output is withheld because it can contain credentials. " + "Check the configured Unity license/account and available seats.") + return 1 + print(f"Unity license {operation} completed.") + return 0 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("operation", choices=("prepare", "activate", "return")) + args = parser.parse_args() + directory = Path(os.environ["RUNNER_TEMP"]) / "unity-activation" + try: + if args.operation == "prepare": + prepare(directory, dict(os.environ)) + return 0 + return run_license(directory, os.environ["UNITY_IMAGE"], args.operation) + except (OSError, ValueError) as exc: + # Only our constant validation messages are safe to expose; network and + # filesystem exceptions can include paths or provider response content. + message = str(exc) if isinstance(exc, ValueError) else "Unable to prepare or run Unity licensing." + print(f"::error::{message}") + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/local_harness.py b/tools/local_harness.py index 32f39fe74..4f5619bb5 100644 --- a/tools/local_harness.py +++ b/tools/local_harness.py @@ -894,6 +894,7 @@ def docker_run_argv(image: str, workspace: Path, project_path: Path, status_dir: "-v", f"{rt}/unity-config:/root/.config/unity3d", "-v", f"{rt}/unity-local:/root/.local/share/unity3d", "-v", f"{rt}/unity-cache:/root/.cache/unity3d", + "-v", f"{rt}/unity-machine-id:/etc/machine-id:ro", ] return [ "docker", "run", "-d", "--name", container, "--network", "host", diff --git a/tools/tests/test_ci_unity_license.py b/tools/tests/test_ci_unity_license.py new file mode 100644 index 000000000..6c48298f3 --- /dev/null +++ b/tools/tests/test_ci_unity_license.py @@ -0,0 +1,148 @@ +"""License inputs, container continuity, and failure handling without real secrets.""" +import base64 +import io +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import ci_unity_license as lic +from local_harness import DockerLauncher + + +@pytest.mark.parametrize("xml", [ + b'signed', + b'signed', + b'', +]) +@pytest.mark.parametrize("encoded", [False, True]) +def test_raw_and_base64_signed_candidates(xml, encoded): + value = base64.b64encode(xml).decode() if encoded else xml.decode() + assert lic.decode_ulf(value) == xml + + +@pytest.mark.parametrize("value", ["", "not XML", "", ""]) +def test_invalid_candidates_are_rejected(value): + with pytest.raises(ValueError, match="not signed XML"): + lic.decode_ulf(value) + + +def mock_downloads(monkeypatch): + urls = [] + def fetch(url, timeout): + urls.append(url) + return io.BytesIO(b"# pinned activation helper\n") + monkeypatch.setattr(lic.urllib.request, "urlopen", fetch) + return urls + + +def test_personal_credentials_do_not_require_serial_and_reuse_identity(tmp_path, monkeypatch): + urls = mock_downloads(monkeypatch) + directory = tmp_path / "unity-activation" + credentials = {"UNITY_EMAIL": "test@example.invalid", "UNITY_PASSWORD": "fake-password"} + lic.prepare(directory, credentials) + identity = (tmp_path / "unity-machine-id").read_text() + assert len(identity.strip()) == 32 + lic.prepare(directory, credentials) + assert (tmp_path / "unity-machine-id").read_text() == identity + assert not (directory / "input.ulf").exists() + assert len(urls) == 8 + assert all(f"/{lic.GAME_CI_COMMIT}/" in url for url in urls) + assert {p.name for p in (directory / "steps").iterdir()} == set(lic.GAME_CI_SCRIPTS) + + +def test_invalid_ulf_requires_fallback_credentials(tmp_path, monkeypatch, capsys): + urls = mock_downloads(monkeypatch) + directory = tmp_path / "unity-activation" + with pytest.raises(ValueError, match="not signed XML"): + lic.prepare(directory, {"UNITY_LICENSE": "private-invalid-value"}) + assert not urls + lic.prepare(directory, {"UNITY_LICENSE": "private-invalid-value", + "UNITY_EMAIL": "test@example.invalid", "UNITY_PASSWORD": "secret"}) + assert "trying the configured Unity account" in capsys.readouterr().out + assert not (directory / "input.ulf").exists() + + +def test_missing_or_partial_credentials_fail_before_download(tmp_path, monkeypatch): + urls = mock_downloads(monkeypatch) + for environment in ({}, {"UNITY_EMAIL": "test@example.invalid"}, {"UNITY_PASSWORD": "secret"}): + with pytest.raises(ValueError, match="requires"): + lic.prepare(tmp_path / "unity-activation", environment) + assert not urls + + +def test_staged_license_is_passed_as_a_file_and_stale_input_is_removed(tmp_path, monkeypatch): + mock_downloads(monkeypatch) + directory = tmp_path / "unity-activation" + lic.prepare(directory, {"UNITY_LICENSE": "fake"}) + args = lic.docker_args(directory, "test-image", "activate") + assert "UNITY_LICENSE_FILE=/activation/input.ulf" in args + assert "UNITY_LICENSE=" in args + assert not any("" in argument for argument in args) + lic.prepare(directory, {"UNITY_EMAIL": "test@example.invalid", "UNITY_PASSWORD": "secret"}) + assert "UNITY_LICENSE_FILE=" in lic.docker_args(directory, "test-image", "activate") + + +def test_all_container_paths_share_identity_and_license_state(tmp_path): + directory = tmp_path / "unity-activation" + directory.mkdir() + activate = lic.docker_args(directory, "test-image", "activate") + returned = lic.docker_args(directory, "test-image", "return") + bridge = DockerLauncher.docker_run_argv("test-image", tmp_path, tmp_path / "project", + tmp_path / "status", "-", [], runner_temp=str(tmp_path)) + bridge = [argument.replace("\\", "/") for argument in bridge] + activate = [argument.replace("\\", "/") for argument in activate] + returned = [argument.replace("\\", "/") for argument in returned] + for suffix, target in (("unity-machine-id", "/etc/machine-id:ro"), + ("unity-config", "/root/.config/unity3d"), + ("unity-local", "/root/.local/share/unity3d"), + ("unity-cache", "/root/.cache/unity3d")): + mount = f"{tmp_path / suffix}:{target}".replace("\\", "/") + assert mount in activate and mount in returned and mount in bridge + local = DockerLauncher.docker_run_argv("test-image", tmp_path, tmp_path / "project", + tmp_path / "status", "-", []) + assert not any("unity-machine-id" in item for item in local) + + +def test_failed_activation_reports_failure_without_raw_output(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(lic.subprocess, "run", lambda *a, **kw: subprocess.CompletedProcess( + a, 3, "No seat available. fake-password test@example.invalid", "private serial")) + assert lic.run_license(tmp_path, "test-image", "activate") == 1 + output = capsys.readouterr().out + assert "no Unity license seat" in output + assert "fake-password" not in output and "test@example.invalid" not in output + assert "private serial" not in output + assert (tmp_path / "activation-attempted").exists() + + +def test_return_does_not_start_docker_without_activation_attempt(tmp_path, monkeypatch): + def unexpected(*args, **kwargs): + pytest.fail("Docker should not be called before activation") + monkeypatch.setattr(lic.subprocess, "run", unexpected) + assert lic.run_license(tmp_path, "test-image", "return") == 0 + + +@pytest.mark.parametrize("source,expected", [("export GAME_CI_ACTIVATED_VIA=personal\n", 0), + ("exit 7\n", 7), ("return 7\n", 7)]) +def test_shell_propagates_failure_and_preserves_fallback_for_return(tmp_path, source, expected): + bash = "C:/Program Files/Git/bin/bash.exe" if os.name == "nt" else shutil.which("bash") + if not bash or not Path(bash).exists(): + pytest.skip("bash unavailable") + steps = tmp_path / "steps" + steps.mkdir() + (steps / "activate.sh").write_text(source, encoding="utf-8") + (steps / "return_license.sh").write_text( + 'test "$GAME_CI_ACTIVATED_VIA" = personal || exit 8\nRETURN_EXIT_CODE=0\n', encoding="utf-8") + environment = dict(os.environ, STEPS_DIR=steps.as_posix(), ACTIVATE_LICENSE_PATH=tmp_path.as_posix()) + result = subprocess.run([bash, "-c", lic.CONTAINER_SCRIPT, "test", "activate"], + env=environment, capture_output=True, text=True) + assert result.returncode == expected, result.stderr + if not expected: + assert (tmp_path / "activated-via").read_text() == "personal" + result = subprocess.run([bash, "-c", lic.CONTAINER_SCRIPT, "test", "return"], + env=environment, capture_output=True, text=True) + assert result.returncode == 0, result.stderr From 5fb77036203668ad972e75ce4d922d3fc7a3ce0d Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:23:47 -0400 Subject: [PATCH 08/12] fix(ci): verify NUnit records and report domain test failures --- .github/workflows/e2e-bridge.yml | 2 ++ .github/workflows/unity-tests.yml | 3 ++- tools/check_unity_test_results.py | 4 ++++ tools/tests/test_check_unity_test_results.py | 18 ++++++++++++++++-- tools/tests/test_unity_tests_workflow.py | 2 ++ 5 files changed, 26 insertions(+), 3 deletions(-) diff --git a/.github/workflows/e2e-bridge.yml b/.github/workflows/e2e-bridge.yml index fb59e51c8..3605336cc 100644 --- a/.github/workflows/e2e-bridge.yml +++ b/.github/workflows/e2e-bridge.yml @@ -52,6 +52,8 @@ jobs: UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} run: | set -e + # Only absent secrets skip a fork run. Partial configuration must fail + # explicitly in prepare, rather than silently look like missing access. if [ -n "$UNITY_LICENSE$UNITY_EMAIL$UNITY_PASSWORD$UNITY_SERIAL" ]; then echo "unity_ok=true" >> "$GITHUB_OUTPUT" else diff --git a/.github/workflows/unity-tests.yml b/.github/workflows/unity-tests.yml index 14c96feba..2e7bc94d5 100644 --- a/.github/workflows/unity-tests.yml +++ b/.github/workflows/unity-tests.yml @@ -174,6 +174,7 @@ jobs: - name: Run domain reload tests uses: game-ci/unity-test-runner@32e57712352b500e17974b245a6dce9e11a73213 # v4 id: domain-tests + continue-on-error: true env: UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} @@ -189,7 +190,7 @@ jobs: githubToken: "" artifactsPath: artifacts/domain-reload - # A runner failure already fails the job here; this also fails a run that executed no test. + # Preserve runner failures while reporting any NUnit failure details or missing results. - name: Check domain reload test results env: RESULTS_XML: artifacts/domain-reload/${{ matrix.testMode }}-results.xml diff --git a/tools/check_unity_test_results.py b/tools/check_unity_test_results.py index 77109f717..80474ce97 100644 --- a/tools/check_unity_test_results.py +++ b/tools/check_unity_test_results.py @@ -72,6 +72,10 @@ def check_results(path: Path, runner_outcome: str) -> int: if inconclusive or inconclusive_cases: print("::error::Unity fails a run with inconclusive tests; use Assert.Ignore for environment guards") return 1 + recorded_passes = sum(case.get("result") == "Passed" for case in root.iter("test-case")) + if recorded_passes != passed: + print(f"::error::NUnit declares {passed} passing tests but contains {recorded_passes} passing test-case records") + return 1 return 1 if runner_failed else 0 diff --git a/tools/tests/test_check_unity_test_results.py b/tools/tests/test_check_unity_test_results.py index 6c578e06a..db6cc9574 100644 --- a/tools/tests/test_check_unity_test_results.py +++ b/tools/tests/test_check_unity_test_results.py @@ -12,7 +12,11 @@ PASSING = '' # attributes exactly as Unity wrote them on green beta run 33978935244 (all four Unity versions): # a clean run that contains [Ignore]d tests reports result="Skipped:Ignored", not "Passed". -UNITY_CLEAN_RUN = '' +UNITY_CLEAN_RUN = ( + '' + '' + '' * 1162 + '' * 67 + + '' +) # Beta run 29283113713 (6000.0.75f1), before #1294 moved ManageGraphicsTests off Assume.That. UNITY_INCONCLUSIVE_RUN = ''' @@ -32,13 +36,23 @@ def run_gate(tmp_path, xml, outcome="success"): ) -@pytest.mark.parametrize("xml", [PASSING, UNITY_CLEAN_RUN]) +@pytest.mark.parametrize("xml", [PASSING, UNITY_CLEAN_RUN], ids=["passing", "unity-ignored"]) def test_successful_runner_and_completed_results_pass(tmp_path, xml): result = run_gate(tmp_path, xml) assert result.returncode == 0, result.stdout assert "failed, 0 inconclusive" in result.stdout +@pytest.mark.parametrize("records", ["", '', + '' * 2]) +def test_summary_cannot_claim_passes_without_matching_test_cases(tmp_path, records): + xml = (f'' + f'{records}') + result = run_gate(tmp_path, xml) + assert result.returncode == 1 + assert "passing test-case records" in result.stdout + + # Unity exits 2 when any test is Inconclusive, so with githubToken "" game-ci fails the step and CI # passes "failure"; that run logged "0 failed" and then "Exiting with code 2". A local caller may pass # "success". Either way the gate must fail and name the inconclusive test. diff --git a/tools/tests/test_unity_tests_workflow.py b/tools/tests/test_unity_tests_workflow.py index a87dce4a8..db4fba084 100644 --- a/tools/tests/test_unity_tests_workflow.py +++ b/tools/tests/test_unity_tests_workflow.py @@ -17,6 +17,8 @@ def test_every_runner_step_pins_the_action_commit_and_cli_release(): steps = runner_steps() assert len(steps) == 2 for step in steps: + # Both failures flow into their XML gate, which checks the raw outcome. + assert re.search(r'^ continue-on-error: true$', step, re.M), step ref = re.search(r"uses: game-ci/unity-test-runner@(\S+)", step).group(1) assert re.fullmatch(r"[0-9a-f]{40}", ref), ref assert re.search(r"^ cliVersion: v\d+\.\d+\.\d+$", step, re.M), step From 5fcd6711e628ac0891493170153ac6f7644db389 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:26:10 -0400 Subject: [PATCH 09/12] fix(ci): prepare a saved E2E scene and retain Unity diagnostics --- .github/workflows/e2e-bridge.yml | 9 ++- tools/local_harness.py | 102 +++++++++++++++++++++++---- tools/tests/test_ci_harness_scene.py | 47 ++++++++++++ tools/tests/test_local_harness.py | 78 ++++++++++++++++++++ 4 files changed, 216 insertions(+), 20 deletions(-) create mode 100644 tools/tests/test_ci_harness_scene.py diff --git a/.github/workflows/e2e-bridge.yml b/.github/workflows/e2e-bridge.yml index 3605336cc..75571a96c 100644 --- a/.github/workflows/e2e-bridge.yml +++ b/.github/workflows/e2e-bridge.yml @@ -157,14 +157,13 @@ jobs: UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} run: python3 tools/ci_unity_license.py return - - name: Unity logs on failure - if: failure() - run: docker logs unity-mcp --tail 200 | sed -E 's/((email|serial|license|password|token)[^[:space:]]*)/[REDACTED]/Ig' || true - - name: Upload E2E report if: always() uses: actions/upload-artifact@v4 with: name: e2e-bridge-report - path: reports/junit-*.xml + # The harness snapshots redacted Editor logs before removing its container. + path: | + reports/junit-*.xml + reports/unity-editor-*.log if-no-files-found: ignore diff --git a/tools/local_harness.py b/tools/local_harness.py index 4f5619bb5..19a8a47ac 100644 --- a/tools/local_harness.py +++ b/tools/local_harness.py @@ -67,6 +67,7 @@ import tempfile import threading import time +import uuid import xml.etree.ElementTree as ET from dataclasses import dataclass, field from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath @@ -503,17 +504,16 @@ def port_from_status(data: dict[str, Any] | None) -> int | None: r"(Bridge|MCP(For)?Unity|AutoConnect).*(listening|ready|started|port|bound)", re.IGNORECASE, ) -_REDACT_RE = re.compile(r"(?i)((email|serial|license|password|token)\S*)") +_REDACT_RE = re.compile( + r"(?im)^.*\b(?:e-?mail|serial|license|licensing|password|token|username)\b.*$" +) +_EMAIL_RE = re.compile(r"[^\s@]+@[^\s@]+\.[^\s@]+") _CS_ERROR_RE = re.compile(r"error CS\d") def redact(text: str) -> str: - """Redact secret-ish tokens from log echoes. - - Mirrors the CI sed idiom - `sed -E 's/((email|serial|license|password|token)[^[:space:]]*)/[REDACTED]/Ig'`. - """ - return _REDACT_RE.sub("[REDACTED]", text or "") + """Omit credential-related lines, including values separated from their labels.""" + return _EMAIL_RE.sub("[REDACTED]", _REDACT_RE.sub("[REDACTED]", text or "")) def classify_log(text: str, license_grace_elapsed: bool = True) -> str: @@ -940,13 +940,24 @@ def is_alive(self, handle: Handle) -> bool: return False def tail_log(self, handle: Handle, n: int) -> str: + container = handle.container or self.CONTAINER try: + # Unity uses -logFile, so its diagnostics are not on Docker stdout. + if handle.log_path: + file_log = subprocess.run( + ["docker", "exec", container, "tail", "-n", str(n), handle.log_path], + capture_output=True, text=True, encoding="utf-8", errors="replace", + check=False, timeout=10, + ) + if file_log.returncode == 0: + return file_log.stdout or "" out = subprocess.run( - ["docker", "logs", "--tail", str(n), self.CONTAINER], - capture_output=True, text=True, check=False, + ["docker", "logs", "--tail", str(n), container], + capture_output=True, text=True, encoding="utf-8", errors="replace", + check=False, timeout=10, ) return (out.stdout or "") + (out.stderr or "") - except OSError: + except (OSError, subprocess.TimeoutExpired): return "" def fixup_permissions(self, status_dir: Path) -> None: @@ -1039,6 +1050,20 @@ def _redacted_tail(launcher, handle: Handle, n: int = 200) -> None: print(redact(tail)) +def preserve_editor_diagnostics(launcher, handle: Handle, reports_dir: Path, stage: str) -> None: + """Keep a sanitized snapshot before a retry or teardown removes the Editor log.""" + tail = redact(launcher.tail_log(handle, 200)) + if not tail: + return + print(f"== Unity Editor diagnostics: {stage} ==", flush=True) + print(tail, flush=True) + try: + reports_dir.mkdir(parents=True, exist_ok=True) + (reports_dir / f"unity-editor-{stage}.log").write_text(tail, encoding="utf-8") + except OSError: + print("::warning::Unable to save the Unity Editor diagnostic snapshot.", flush=True) + + def _console_entries(resp: Any) -> list[Any]: """Pull read_console log entries out of either envelope shape. @@ -1129,6 +1154,28 @@ def run_smoke_leg(instance_id: str, junit_path: Path, max_retries: int, retry_ms return LegOutcome("smoke", "error", blocking=True, detail="no bridge reachable", exit_code=2) +def prepare_ci_scene(args: argparse.Namespace, instance_id: str, *, send=None) -> bool: + """Name the disposable CI scene before smoke can dirty it and block UTF's save task.""" + if not args.ci or args.reuse: + return True + if send is None: + _ensure_src_on_path() + from transport.legacy.unity_connection import send_command_with_retry as send + try: + response = send( + "manage_scene", + {"action": "save", "path": "Assets/__MCPHarness", "name": f"Scene_{uuid.uuid4().hex}"}, + instance_id=instance_id, max_retries=args.max_retries, + retry_ms=args.retry_ms, retry_on_reload=True, + ) + if _ok(response): + return True + except Exception: + pass + print("::error:: Could not save the disposable CI scene; UTF tests were not started.") + return False + + def _start_utf(send, mode: str, instance_id: str, init_timeout_ms: int | None, max_retries: int, retry_ms: int) -> tuple[str | None, dict[str, Any] | Any]: """Issue run_tests; return (job_id, raw_start_response). Gates on result.success.""" @@ -1296,7 +1343,8 @@ def _ensure_clean_editmode(send, instance_id: str, max_retries: int, retry_ms: i def run_playmode_with_retry(instance_id: str, deadline: float, max_retries: int, retry_ms: int, init_timeout_ms: int, strict: bool, - relaunch: Callable[[], str] | None = None) -> LegOutcome: + relaunch: Callable[[], str] | None = None, + before_retry: Callable[[], None] | None = None) -> LegOutcome: """PlayMode state machine: start, poll, classify-can-rerun, retry ONCE. Non-blocking by default; --strict-playmode promotes failure to blocking. @@ -1327,6 +1375,9 @@ def attempt(inst: str) -> LegOutcome: if not can_rerun: return first + if before_retry is not None: + before_retry() + # A wedge may need the editor relaunched (respecting the socket-release delay). inst = instance_id if "wedge" in error_text and relaunch is not None: @@ -1420,6 +1471,15 @@ def main(argv: list[str] | None = None) -> int: owns_editor = not (args.reuse or args.keep_alive) outcomes: list[LegOutcome] = [] + def capture_diagnostics(stage: str) -> None: + if handle is not None: + preserve_editor_diagnostics(launcher, handle, reports_dir, stage) + + def record_outcome(outcome: LegOutcome) -> None: + outcomes.append(outcome) + if outcome.status in ("fail", "error"): + capture_diagnostics(outcome.name) + def do_teardown() -> None: # Only kill the editor we started; clean only our own status files. if handle is not None and owns_editor and not args.keep_alive: @@ -1537,9 +1597,17 @@ def _watchdog() -> None: # pragma: no cover - timing/daemon path if not compile_ok: print("::error:: project does not compile -- skipping UTF legs") + # Creating and deleting smoke objects leaves an untitled scene dirty. UTF + # cancels its save dialog in batch mode without firing test callbacks. + if wants_utf and compile_ok and not prepare_ci_scene(args, instance_id): + record_outcome(LegOutcome("setup", "error", blocking=True, + detail="could not prepare CI scene", exit_code=2)) + write_reports(junit_path, reports_dir, outcomes) + return 2 + # --- Smoke leg --- if "smoke" in legs: - outcomes.append(run_smoke_leg(instance_id, junit_path, args.max_retries, + record_outcome(run_smoke_leg(instance_id, junit_path, args.max_retries, args.retry_ms, deadline=deadline)) # --- EditMode leg --- @@ -1548,7 +1616,7 @@ def _watchdog() -> None: # pragma: no cover - timing/daemon path outcomes.append(LegOutcome("editmode", "fail", blocking=True, detail="project does not compile", exit_code=3)) else: - outcomes.append(run_utf_leg("EditMode", instance_id, blocking=True, + record_outcome(run_utf_leg("EditMode", instance_id, blocking=True, deadline=deadline, max_retries=args.max_retries, retry_ms=args.retry_ms)) @@ -1568,12 +1636,16 @@ def _relaunch() -> str: ready = wait_for_ready(launcher, handle, status_dir, args.bridge_wait, time.time(), deadline) instance_id = ready.instance_id os.environ["UNITY_MCP_DEFAULT_INSTANCE"] = instance_id + if not prepare_ci_scene(args, instance_id): + capture_diagnostics("setup") + raise SystemExit(2) return instance_id relaunch = _relaunch if (owns_editor and not args.reuse) else None - outcomes.append(run_playmode_with_retry( + record_outcome(run_playmode_with_retry( instance_id, deadline, args.max_retries, args.retry_ms, - args.playmode_init_timeout, bool(args.strict_playmode), relaunch=relaunch)) + args.playmode_init_timeout, bool(args.strict_playmode), relaunch=relaunch, + before_retry=lambda: capture_diagnostics("playmode-before-retry"))) # Aggregate + write reports. write_reports(junit_path, reports_dir, outcomes) diff --git a/tools/tests/test_ci_harness_scene.py b/tools/tests/test_ci_harness_scene.py new file mode 100644 index 000000000..e05641bb1 --- /dev/null +++ b/tools/tests/test_ci_harness_scene.py @@ -0,0 +1,47 @@ +"""Prevent an unnamed dirty smoke scene from silently cancelling UTF in batch mode.""" +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import local_harness as lh + + +@pytest.mark.parametrize("argv", [[], ["--reuse"], ["--ci", "--reuse"]]) +def test_scene_preparation_does_not_save_user_editors(argv): + def unexpected(*args, **kwargs): + pytest.fail("must not save local or reused scenes") + + assert lh.prepare_ci_scene(lh.build_arg_parser().parse_args(argv), "inst@hash", send=unexpected) + + +def test_ci_saves_unique_scene_through_the_selected_bridge(): + calls = [] + + def send(command, params, **kwargs): + calls.append((command, params, kwargs)) + return {"success": True} + + args = lh.build_arg_parser().parse_args(["--ci"]) + assert lh.prepare_ci_scene(args, "inst@hash", send=send) + assert lh.prepare_ci_scene(args, "inst@hash", send=send) + assert calls[0][1]["name"] != calls[1][1]["name"] + for command, params, kwargs in calls: + assert command == "manage_scene" + assert params["action"] == "save" + assert params["path"] == "Assets/__MCPHarness" + assert kwargs["instance_id"] == "inst@hash" + + +@pytest.mark.parametrize("raises", [False, True]) +def test_ci_scene_failure_cannot_be_treated_as_ready(raises, capsys): + def send(*args, **kwargs): + if raises: + raise RuntimeError("sensitive transport output") + return {"success": False, "message": "save failed"} + + assert not lh.prepare_ci_scene(lh.build_arg_parser().parse_args(["--ci"]), "inst@hash", send=send) + output = capsys.readouterr().out + assert "tests were not started" in output + assert "sensitive transport output" not in output diff --git a/tools/tests/test_local_harness.py b/tools/tests/test_local_harness.py index 1737cfbd3..941c19dd6 100644 --- a/tools/tests/test_local_harness.py +++ b/tools/tests/test_local_harness.py @@ -828,3 +828,81 @@ def fake_send(cmd, params, **kw): job_id, _ = lh._start_utf(fake_send, "EditMode", "inst@hash", None, 8, 50) assert job_id == "J9" assert calls["n"] == 2 + + +class TestEditorDiagnostics: + def test_docker_reads_editor_file_instead_of_stdout(self, monkeypatch): + from types import SimpleNamespace + + calls = [] + + def run(argv, **kwargs): + calls.append(argv) + assert kwargs["timeout"] == 10 + return SimpleNamespace(returncode=0, stdout="Scene(s) Have Been Modified\n", stderr="") + + monkeypatch.setattr(lh.subprocess, "run", run) + launcher = lh.DockerLauncher(SimpleNamespace()) + handle = lh.Handle(container="owned-editor", log_path="/root/.config/unity3d/Editor.log") + assert "Scene(s)" in launcher.tail_log(handle, 30) + assert calls == [["docker", "exec", "owned-editor", "tail", "-n", "30", handle.log_path]] + + def test_docker_falls_back_to_startup_stdout_when_file_is_missing(self, monkeypatch): + from types import SimpleNamespace + + calls = [] + + def run(argv, **kwargs): + calls.append(argv) + if argv[1] == "exec": + return SimpleNamespace(returncode=1, stdout="", stderr="file missing") + return SimpleNamespace(returncode=0, stdout="Editor startup failed", stderr="") + + monkeypatch.setattr(lh.subprocess, "run", run) + launcher = lh.DockerLauncher(SimpleNamespace()) + handle = lh.Handle(container="owned-editor", log_path="/root/Editor.log") + assert launcher.tail_log(handle, 30) == "Editor startup failed" + assert calls[-1] == ["docker", "logs", "--tail", "30", "owned-editor"] + + def test_snapshot_keeps_diagnostics_but_omits_credentials(self, tmp_path, capsys): + from types import SimpleNamespace + + raw = ("Scene(s) Have Been Modified\n" + "Serial number assigned to: 'private-serial'\n" + "password = private-password\n" + "Connected account developer@example.com\n") + launcher = SimpleNamespace(tail_log=lambda *_args: raw) + lh.preserve_editor_diagnostics(launcher, lh.Handle(), tmp_path, "editmode") + saved = (tmp_path / "unity-editor-editmode.log").read_text(encoding="utf-8") + output = capsys.readouterr().out + for text in (saved, output): + assert "Scene(s) Have Been Modified" in text + assert "[REDACTED]" in text + assert "private-serial" not in text + assert "private-password" not in text + assert "developer@example.com" not in text + + def test_first_failure_is_captured_before_relaunch_and_retry(self, monkeypatch): + from types import SimpleNamespace + + monkeypatch.setitem(sys.modules, "transport.legacy.unity_connection", + SimpleNamespace(send_command_with_retry=lambda *_a, **_k: None)) + monkeypatch.setattr(lh, "_ensure_src_on_path", lambda: None) + monkeypatch.setattr(lh, "_ensure_clean_editmode", lambda *_a: None) + monkeypatch.setattr(lh, "_start_utf", lambda *_a: ("job", {})) + monkeypatch.setattr(lh.time, "sleep", lambda *_a: None) + events = [] + attempts = iter([lh.LegOutcome("playmode", "fail", False, "wedge", 1), + lh.LegOutcome("playmode", "pass", False, "completed", 0)]) + monkeypatch.setattr(lh, "_outcome_from_terminal", lambda *_a: next(attempts)) + monkeypatch.setattr(lh, "_poll_utf", lambda *_a: events.append("poll")) + + def relaunch(): + events.append("teardown-and-relaunch") + return "new-instance" + + outcome = lh.run_playmode_with_retry( + "instance", lh.time.time() + 100, 1, 10, 1000, False, + relaunch=relaunch, before_retry=lambda: events.append("snapshot")) + assert outcome.status == "pass" + assert events == ["poll", "snapshot", "teardown-and-relaunch", "poll"] From b128e61f69ec8bb25a76b51537024e223aca005b Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:38:33 -0400 Subject: [PATCH 10/12] fix(tests): isolate transport fixtures from the resident MCP session --- .../Clients/SupportedTransportsTests.cs | 2 +- .../Helpers/ClientConfigFormatTests.cs | 2 +- .../Helpers/CodexConfigHelperTests.cs | 2 +- .../EditMode/Helpers/WriteToConfigTests.cs | 2 +- ...rManagementServiceCharacterizationTests.cs | 2 +- .../Services/EditorConfigurationCacheTests.cs | 39 ++++++++++++++- .../Services/HttpAutoStartHandlerTests.cs | 2 +- .../Services/HttpBridgeReloadHandlerTests.cs | 2 +- .../Server/ServerCommandBuilderTests.cs | 2 +- .../Services/StdioBrokerResendTests.cs | 33 ++++++++++--- .../EditMode/TransportPreferenceTestBase.cs | 49 +++++++++++++++++++ .../TransportPreferenceTestBase.cs.meta | 2 + .../Windows_Characterization.cs | 2 +- 13 files changed, 124 insertions(+), 17 deletions(-) create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs.meta diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Clients/SupportedTransportsTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Clients/SupportedTransportsTests.cs index 75263c81c..94c8aa790 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Clients/SupportedTransportsTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Clients/SupportedTransportsTests.cs @@ -8,7 +8,7 @@ namespace MCPForUnityTests.Editor.Clients { [TestFixture] - public class SupportedTransportsTests + public class SupportedTransportsTests : TransportPreferenceTestBase { [Test] public void IMcpClientConfigurator_ExposesSupportedTransports() diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs index ef9301566..271ed516d 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs @@ -17,7 +17,7 @@ namespace MCPForUnityTests.Editor.Helpers // uses an "mcp" container, type:"remote" for HTTP servers, and an "enabled" flag. Writing the // old format left the server showing as "stdio" + disabled. These tests pin the new Kilo format // while guarding that Cline keeps "streamableHttp" and generic clients keep plain "http". - public class ClientConfigFormatTests + public class ClientConfigFormatTests : TransportPreferenceTestBase { private const string UseHttpTransportPrefKey = EditorPrefKeys.UseHttpTransport; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs index 32a5aef37..4e395d891 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs @@ -10,7 +10,7 @@ namespace MCPForUnityTests.Editor.Helpers { - public class CodexConfigHelperTests + public class CodexConfigHelperTests : TransportPreferenceTestBase { /// /// Validates that a TOML args array contains the expected uvx structure: diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs index 8b18ce042..8c8925325 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs @@ -13,7 +13,7 @@ namespace MCPForUnityTests.Editor.Helpers { - public class WriteToConfigTests + public class WriteToConfigTests : TransportPreferenceTestBase { private const string UseHttpTransportPrefKey = EditorPrefKeys.UseHttpTransport; private const string HttpUrlPrefKey = EditorPrefKeys.HttpBaseUrl; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Characterization/ServerManagementServiceCharacterizationTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Characterization/ServerManagementServiceCharacterizationTests.cs index 6941d8c3f..6620ac66c 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Characterization/ServerManagementServiceCharacterizationTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Characterization/ServerManagementServiceCharacterizationTests.cs @@ -19,7 +19,7 @@ namespace MCPForUnityTests.Editor.Services.Characterization /// no regressions during the decomposition into focused components. /// [TestFixture] - public class ServerManagementServiceCharacterizationTests + public class ServerManagementServiceCharacterizationTests : TransportPreferenceTestBase { private ServerManagementService _service; private bool _savedUseHttpTransport; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs index d97f6debc..2d68816bf 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs @@ -9,7 +9,7 @@ namespace MCPForUnityTests.Editor.Services /// Unit tests for EditorConfigurationCache. /// [TestFixture] - public class EditorConfigurationCacheTests + public class EditorConfigurationCacheTests : TransportPreferenceTestBase { private bool _originalUseHttpTransport; private bool _originalDebugLogs; @@ -31,7 +31,6 @@ public void SetUp() public void TearDown() { // Restore original values - EditorConfigurationCache.Instance.UnpinStdioForSession(); EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, _originalUseHttpTransport); EditorPrefs.SetBool(EditorPrefKeys.DebugLogs, _originalDebugLogs); EditorPrefs.SetString(EditorPrefKeys.UvxPathOverride, _originalUvxPath); @@ -264,6 +263,42 @@ public void Refresh_UpdatesAllCachedValues() #region Session Pin Tests + private sealed class TransportPreferenceScopeProbe : TransportPreferenceTestBase { } + + [TestCase(0)] + [TestCase(1)] + [TestCase(2)] + public void ConfigurationFixture_RestoresSessionPinAndHttpPreference(int pinState) + { + string key = EditorConfigurationCache.SessionKeyForceStdio; + if (pinState == 0) + SessionState.EraseBool(key); + else + SessionState.SetBool(key, pinState == 2); + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, false); + EditorConfigurationCache.Instance.Refresh(); + + var scope = new TransportPreferenceScopeProbe(); + scope.SuspendSessionTransportOverride(); + try + { + EditorConfigurationCache.Instance.SetUseHttpTransport(true); + Assert.IsTrue(EditorConfigurationCache.Instance.UseHttpTransport, + "The HTTP branch must be testable even when the resident harness pinned stdio."); + } + finally + { + scope.RestoreSessionTransportOverride(); + } + + Assert.AreEqual(pinState == 2, SessionState.GetBool(key, false)); + Assert.AreEqual(pinState != 1, SessionState.GetBool(key, true), + "A missing pin must remain missing rather than become an explicit false."); + Assert.IsFalse(EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true), + "Fixture cleanup must also restore the persisted transport preference."); + Assert.IsFalse(EditorConfigurationCache.Instance.UseHttpTransport); + } + [Test] public void PinStdioForSession_OverridesHttpPreference_WithoutWritingEditorPrefs() { diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs index f4229bf2c..974998f7d 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpAutoStartHandlerTests.cs @@ -14,7 +14,7 @@ namespace MCPForUnityTests.Editor.Services /// The TryBeginReconnect tests only exercise its deliberate-drop paths, which never /// dispatch the async connect. /// - public class HttpAutoStartHandlerTests + public class HttpAutoStartHandlerTests : TransportPreferenceTestBase { private FakeTransportClient _fakeClient; private TransportManager _savedManager; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs index 83e042d71..99deb6305 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/HttpBridgeReloadHandlerTests.cs @@ -14,7 +14,7 @@ namespace MCPForUnityTests.Editor.Services /// Uses fake transports and a zero-delay retry schedule so every path completes /// synchronously (UTF 1.1 cannot run async tests). /// - public class HttpBridgeReloadHandlerTests + public class HttpBridgeReloadHandlerTests : TransportPreferenceTestBase { private static readonly TimeSpan[] ZeroSchedule = { diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/ServerCommandBuilderTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/ServerCommandBuilderTests.cs index 624a98e4f..0fe428a25 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/ServerCommandBuilderTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/ServerCommandBuilderTests.cs @@ -10,7 +10,7 @@ namespace MCPForUnityTests.Editor.Services.Server /// Unit tests for ServerCommandBuilder component. /// [TestFixture] - public class ServerCommandBuilderTests + public class ServerCommandBuilderTests : TransportPreferenceTestBase { private ServerCommandBuilder _builder; private bool _savedUseHttpTransport; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBrokerResendTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBrokerResendTests.cs index ee64bec5a..905df25ee 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBrokerResendTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/StdioBrokerResendTests.cs @@ -2,6 +2,7 @@ using System.Collections; using System.IO; using System.Net.Sockets; +using System.Reflection; using System.Text; using System.Threading; using NUnit.Framework; @@ -73,8 +74,11 @@ public IEnumerator IdenticalCommandResentOnNewConnection_IsQueuedOnce() } int port = StdioBridgeHost.GetCurrentPort(); - byte[] command = Encoding.UTF8.GetBytes( - "{\"type\":\"read_console\",\"params\":{\"action\":\"get\",\"count\":1}}"); + // MCP polls this same bridge while the test runs. Identify only our command, + // so unrelated get_test_job traffic does not look like a duplicate resend. + string commandJson = "{\"type\":\"read_console\",\"test_nonce\":\"" + + Guid.NewGuid().ToString("N") + "\",\"params\":{\"action\":\"get\",\"count\":1}}"; + byte[] command = Encoding.UTF8.GetBytes(commandJson); TcpClient first = null; TcpClient second = null; @@ -90,13 +94,13 @@ public IEnumerator IdenticalCommandResentOnNewConnection_IsQueuedOnce() // has to happen before the second connect: a new connection closes stale clients, // and if it wins that race the first frame is never read at all. Thread.Sleep(1500); - queuedAfterFirst = StdioBridgeHost.QueuedCommandCount; + queuedAfterFirst = CountQueuedPayload(commandJson); // A second connection is what the broker opens after giving up on the first. second = Connect(port); SendFrame(second.GetStream(), command); Thread.Sleep(1500); - queuedAfterResend = StdioBridgeHost.QueuedCommandCount; + queuedAfterResend = CountQueuedPayload(commandJson); } finally { @@ -110,10 +114,27 @@ public IEnumerator IdenticalCommandResentOnNewConnection_IsQueuedOnce() Assert.AreEqual(1, queuedAfterFirst, "precondition: the first command must be sitting in the queue undrained — " - + $"found {queuedAfterFirst} entries, so this run proves nothing about the resend"); + + $"found {queuedAfterFirst} matching entries, so this run proves nothing about the resend"); Assert.AreEqual(1, queuedAfterResend, $"the resend should have attached to the in-flight command, but {queuedAfterResend} " - + "entries were queued — the command would run that many times"); + + "matching entries were queued — the command would run that many times"); + } + + private static int CountQueuedPayload(string commandJson) + { + const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Static; + var queueField = typeof(StdioBridgeHost).GetField("commandQueue", flags); + var lockField = typeof(StdioBridgeHost).GetField("lockObj", flags); + Assert.NotNull(queueField); + Assert.NotNull(lockField); + lock (lockField.GetValue(null)) + { + var queue = (IDictionary)queueField.GetValue(null); + int count = 0; + foreach (QueuedCommand queued in queue.Values) + if (queued.CommandJson == commandJson) count++; + return count; + } } private static TcpClient Connect(int port) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs new file mode 100644 index 000000000..4f11f1357 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs @@ -0,0 +1,49 @@ +using MCPForUnity.Editor.Constants; +using MCPForUnity.Editor.Services; +using NUnit.Framework; +using UnityEditor; + +namespace MCPForUnityTests.Editor +{ + /// + /// Synchronous configuration tests exercise user preferences independently of a resident + /// harness's stdio pin. Restore the pin before returning to the Editor update loop. + /// + public abstract class TransportPreferenceTestBase + { + private bool _hadSessionPin; + private bool _sessionPin; + private bool _hadHttpPreference; + private bool _httpPreference; + + [SetUp] + public void SuspendSessionTransportOverride() + { + string key = EditorConfigurationCache.SessionKeyForceStdio; + _sessionPin = SessionState.GetBool(key, false); + _hadSessionPin = _sessionPin == SessionState.GetBool(key, true); + _hadHttpPreference = EditorPrefs.HasKey(EditorPrefKeys.UseHttpTransport); + _httpPreference = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); + + // Do not call UnpinStdioForSession: fixture setup must not emit a transport change. + SessionState.EraseBool(key); + EditorConfigurationCache.Instance.Refresh(); + } + + [TearDown] + public void RestoreSessionTransportOverride() + { + if (_hadHttpPreference) + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, _httpPreference); + else + EditorPrefs.DeleteKey(EditorPrefKeys.UseHttpTransport); + + string key = EditorConfigurationCache.SessionKeyForceStdio; + if (_hadSessionPin) + SessionState.SetBool(key, _sessionPin); + else + SessionState.EraseBool(key); + EditorConfigurationCache.Instance.Refresh(); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs.meta new file mode 100644 index 000000000..77e7faeac --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/TransportPreferenceTestBase.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 97aa20bec04543c2a44a415ec59d6390 diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs index 801b8bc3a..a6e8a941c 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs @@ -19,7 +19,7 @@ namespace MCPForUnityTests.Editor.Windows.Characterization /// Covers: MCPSetupWindow, EditorPrefsWindow, McpConnectionSection, and component patterns /// [TestFixture] - public class WindowsCharacterizationTests + public class WindowsCharacterizationTests : TransportPreferenceTestBase { #region Section 1: EditorPrefsWindow Tests (3 tests) From b7a2f5a0b8073fec5dc54d1131e8c1e7e2615e27 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:44:49 -0400 Subject: [PATCH 11/12] fix(ci): preserve reports when retry scene preparation fails --- tools/local_harness.py | 14 ++++++--- tools/tests/test_ci_harness_scene.py | 44 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/tools/local_harness.py b/tools/local_harness.py index 19a8a47ac..1ad2e7ccb 100644 --- a/tools/local_harness.py +++ b/tools/local_harness.py @@ -1480,6 +1480,14 @@ def record_outcome(outcome: LegOutcome) -> None: if outcome.status in ("fail", "error"): capture_diagnostics(outcome.name) + def record_scene_setup_failure() -> None: + detail = "could not prepare CI scene" + record_outcome(LegOutcome( + "setup", "error", blocking=True, detail=detail, exit_code=2, + junit_suite=JUnitSuite(name="setup", cases=[JUnitCase(name="setup.scene", failure=detail)]), + )) + write_reports(junit_path, reports_dir, outcomes) + def do_teardown() -> None: # Only kill the editor we started; clean only our own status files. if handle is not None and owns_editor and not args.keep_alive: @@ -1600,9 +1608,7 @@ def _watchdog() -> None: # pragma: no cover - timing/daemon path # Creating and deleting smoke objects leaves an untitled scene dirty. UTF # cancels its save dialog in batch mode without firing test callbacks. if wants_utf and compile_ok and not prepare_ci_scene(args, instance_id): - record_outcome(LegOutcome("setup", "error", blocking=True, - detail="could not prepare CI scene", exit_code=2)) - write_reports(junit_path, reports_dir, outcomes) + record_scene_setup_failure() return 2 # --- Smoke leg --- @@ -1637,7 +1643,7 @@ def _relaunch() -> str: instance_id = ready.instance_id os.environ["UNITY_MCP_DEFAULT_INSTANCE"] = instance_id if not prepare_ci_scene(args, instance_id): - capture_diagnostics("setup") + record_scene_setup_failure() raise SystemExit(2) return instance_id diff --git a/tools/tests/test_ci_harness_scene.py b/tools/tests/test_ci_harness_scene.py index e05641bb1..6f268d217 100644 --- a/tools/tests/test_ci_harness_scene.py +++ b/tools/tests/test_ci_harness_scene.py @@ -45,3 +45,47 @@ def send(*args, **kwargs): output = capsys.readouterr().out assert "tests were not started" in output assert "sensitive transport output" not in output + + +@pytest.mark.parametrize("fail_on_relaunch", [False, True]) +def test_setup_failure_preserves_results_before_editor_teardown(tmp_path, monkeypatch, fail_on_relaunch): + from types import SimpleNamespace + import xml.etree.ElementTree as ET + + reports = tmp_path / "reports" + snapshots = [] + launcher = SimpleNamespace( + resolve_editor=lambda *_: lh.EditorSpec("fake-editor", "2021"), + launch=lambda *_: lh.Handle(container="owned-editor", log_path="Editor.log"), + tail_log=lambda *_: "scene setup diagnostics", + teardown=lambda *_: snapshots.append((reports / "junit-all.xml").exists()), + ) + monkeypatch.setenv("UNITY_IMAGE", "fake-image") + monkeypatch.setattr(lh, "make_launcher", lambda *_: launcher) + monkeypatch.setattr(lh, "wait_for_ready", lambda *_: lh.ReadyInfo(6400, "instance", "status.json")) + monkeypatch.setattr(lh, "compile_probe", lambda *_: True) + monkeypatch.setattr(lh.time, "sleep", lambda *_: None) + monkeypatch.setattr(lh.signal, "signal", lambda *_: None) + # main sets these in the process; monkeypatch restores their original values. + monkeypatch.setenv("UNITY_MCP_STATUS_DIR", "test-original-status") + monkeypatch.setenv("UNITY_MCP_DEFAULT_INSTANCE", "test-original-instance") + preparations = iter([True, False] if fail_on_relaunch else [False]) + monkeypatch.setattr(lh, "prepare_ci_scene", lambda *_: next(preparations)) + + def passed_leg(name): + return lh.LegOutcome(name, "pass", True, "passed", 0, + lh.JUnitSuite(name=name, cases=[lh.JUnitCase(name=f"{name}.passed")])) + + monkeypatch.setattr(lh, "run_smoke_leg", lambda *_a, **_kw: passed_leg("smoke")) + monkeypatch.setattr(lh, "run_utf_leg", lambda *_a, **_kw: passed_leg("editmode")) + monkeypatch.setattr(lh, "run_playmode_with_retry", lambda *_a, **kw: kw["relaunch"]()) + result = lh.main(["--ci", "--project-path", str(tmp_path / "project"), + "--status-dir", str(tmp_path / "status"), "--reports", str(reports), + "--junit", str(reports / "junit-smoke.xml")]) + + assert result == 2 + root = ET.parse(reports / "junit-all.xml").getroot() + assert root.get("failures") == "1" + assert root.get("tests") == ("3" if fail_on_relaunch else "1") + assert root.find(".//testcase[@name='setup.scene']/failure") is not None + assert snapshots[-1], "reports must exist before the failing Editor is removed" From 0abe62dffc3b49aef3a57cb91211eedf2205420d Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:48:52 -0400 Subject: [PATCH 12/12] fix(ci): report complete Unity results and preserve skipped states --- tools/local_harness.py | 70 ++++++++++++++++---------- tools/tests/test_local_harness.py | 82 +++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 27 deletions(-) diff --git a/tools/local_harness.py b/tools/local_harness.py index 1ad2e7ccb..1bf40f102 100644 --- a/tools/local_harness.py +++ b/tools/local_harness.py @@ -1219,7 +1219,9 @@ def _poll_utf(send, job_id: str, instance_id: str, deadline: float, """ while time.time() < deadline: try: - poll = send("get_test_job", {"job_id": job_id, "includeFailedTests": True}, + # Running jobs have no result payload. Request complete rows for the terminal + # response so JUnit represents passes as well as failures and ignored tests. + poll = send("get_test_job", {"job_id": job_id, "includeDetails": True}, instance_id=instance_id, max_retries=max_retries, retry_ms=retry_ms, retry_on_reload=True) except Exception: @@ -1254,48 +1256,62 @@ def _outcome_from_terminal(name: str, mode: str, terminal: dict[str, Any] | Any, return LegOutcome(name, "fail", blocking=blocking, detail="wedge (no terminal status)", exit_code=1, junit_suite=suite) - if status == "succeeded": - result = _dig(terminal, "result") or {} + result = _dig(terminal, "result") + if status == "succeeded" or (status == "failed" and isinstance(result, dict)): summary = (result.get("summary") if isinstance(result, dict) else None) or {} - total = int(summary.get("total", 0) or 0) - passed = int(summary.get("passed", 0) or 0) - failed = int(summary.get("failed", 0) or 0) - skipped = int(summary.get("skipped", 0) or 0) - duration = float(summary.get("durationSeconds", 0.0) or 0.0) - rows = result.get("results") if isinstance(result, dict) else None - if isinstance(rows, list) and rows: + + def invalid_results(detail: str) -> LegOutcome: + suite.cases.append(JUnitCase(name=f"{mode}.results", failure=detail)) + return LegOutcome(name, "fail", blocking=blocking, detail=detail, + exit_code=1, junit_suite=suite) + + try: + total, passed, failed, skipped = ( + int(summary.get(key, 0) or 0) for key in ("total", "passed", "failed", "skipped") + ) + if min(total, passed, failed, skipped) < 0 or total != passed + failed + skipped: + return invalid_results("inconsistent Unity test summary counts") + rows = result.get("results") if isinstance(result, dict) else None + if not isinstance(rows, list) or len(rows) != total: + return invalid_results("Unity test result rows do not match the complete summary") + recorded = {"passed": 0, "failed": 0, "skipped": 0} for r in rows: if not isinstance(r, dict): - continue + return invalid_results("invalid Unity test result row") rname = str(r.get("fullName") or r.get("name") or f"{mode}.test") rtime = float(r.get("durationSeconds", 0.0) or 0.0) - state = str(r.get("state") or "") - if state.lower() in ("failed", "error"): + # NUnit ResultState can include a label/site, e.g. Skipped:Ignored. + state = str(r.get("state") or "").lower().split(":", 1)[0].split("(", 1)[0] + if state in ("failed", "error"): + recorded["failed"] += 1 fmsg = str(r.get("message") or "") + "\n" + str(r.get("stackTrace") or "") - suite.cases.append(JUnitCase(name=rname, time_s=rtime, failure=fmsg.strip())) - elif state.lower() in ("skipped", "ignored", "inconclusive"): + suite.cases.append(JUnitCase(name=rname, time_s=rtime, failure=fmsg.strip() or "test failed")) + elif state in ("skipped", "ignored"): + recorded["skipped"] += 1 suite.cases.append(JUnitCase(name=rname, time_s=rtime, skipped=True)) - else: + elif state == "passed": + recorded["passed"] += 1 suite.cases.append(JUnitCase(name=rname, time_s=rtime)) - else: - # No per-test rows: synthesize from the summary. - for i in range(passed): - suite.cases.append(JUnitCase(name=f"{mode}.passed.{i}")) - for i in range(failed): - suite.cases.append(JUnitCase(name=f"{mode}.failed.{i}", failure="failed (no detail)")) - for i in range(skipped): - suite.cases.append(JUnitCase(name=f"{mode}.skipped.{i}", skipped=True)) - if not suite.cases and total == 0: - suite.cases.append(JUnitCase(name=f"{mode}.empty", time_s=duration)) + else: + return invalid_results(f"unsupported Unity test state for {rname}: {state or ''}") + except (AttributeError, TypeError, ValueError, OverflowError): + return invalid_results("invalid Unity test summary or duration") + + if recorded != {"passed": passed, "failed": failed, "skipped": skipped}: + return invalid_results("Unity test result states disagree with the summary counts") if failed > 0: return LegOutcome(name, "fail", blocking=blocking, detail=f"{failed}/{total} {mode} tests failed", exit_code=1, junit_suite=suite) + if status == "failed": + return invalid_results(str(_dig(terminal, "error") or "test job failed")) + if passed == 0: + return invalid_results("Unity did not execute any passing tests") return LegOutcome(name, "pass", blocking=blocking, detail=f"{passed}/{total} {mode} tests passed", exit_code=0, junit_suite=suite) - # status == "failed": data.result is null; surface error + capped failures. + # Initialization/runtime failures may have no result: surface error + capped failures. error = _dig(terminal, "error") or "test job failed" failures = _dig(terminal, "failures_so_far") or [] detail = str(error) diff --git a/tools/tests/test_local_harness.py b/tools/tests/test_local_harness.py index 941c19dd6..080879755 100644 --- a/tools/tests/test_local_harness.py +++ b/tools/tests/test_local_harness.py @@ -906,3 +906,85 @@ def relaunch(): relaunch=relaunch, before_retry=lambda: events.append("snapshot")) assert outcome.status == "pass" assert events == ["poll", "snapshot", "teardown-and-relaunch", "poll"] + + +class TestTerminalJUnit: + @staticmethod + def terminal(rows, passed, failed=0, skipped=0, status="succeeded"): + return {"success": True, "data": { + "status": status, + "result": {"summary": {"total": passed + failed + skipped, + "passed": passed, "failed": failed, "skipped": skipped}, + "results": rows}, + }} + + def test_complete_ci_result_keeps_passes_and_qualified_ignored_tests(self): + rows = [{"fullName": f"Suite.Pass{i}", "state": "Passed", "durationSeconds": 0.01} + for i in range(1244)] + rows += [{"fullName": f"Suite.Ignore{i}", "state": "Skipped:Ignored"} + for i in range(73)] + outcome = lh._outcome_from_terminal("editmode", "EditMode", self.terminal(rows, 1244, skipped=73), True) + root = lh.merge_junit([outcome.junit_suite]).getroot() + assert outcome.status == "pass" + assert root.get("tests") == "1317" + assert root.get("skipped") == "73" + assert root.get("failures") == "0" + assert len(root.findall(".//testcase/skipped")) == 73 + assert root.find(".//testcase[@name='Suite.Pass1243']") is not None + + def test_compact_nonpassing_rows_cannot_claim_complete_results(self): + rows = [{"state": "Skipped:Ignored"} for _ in range(73)] + outcome = lh._outcome_from_terminal("editmode", "EditMode", self.terminal(rows, 1244, skipped=73), True) + assert outcome.status == "fail" + assert "do not match" in outcome.detail + + def test_failed_job_with_results_preserves_individual_failure_details(self): + rows = [{"fullName": "Suite.Passed", "state": "Passed"}, + {"fullName": "Suite.Failed", "state": "Failed:Error", "message": "assertion", + "stackTrace": "fixture.cs:42"}, + {"fullName": "Suite.Ignored", "state": "Skipped:Ignored"}] + outcome = lh._outcome_from_terminal( + "editmode", "EditMode", self.terminal(rows, 1, failed=1, skipped=1, status="failed"), True) + root = lh.merge_junit([outcome.junit_suite]).getroot() + assert outcome.status == "fail" + assert root.get("tests") == "3" + assert root.get("failures") == "1" + failure = root.find(".//testcase[@name='Suite.Failed']/failure") + assert "assertion" in failure.text and "fixture.cs:42" in failure.text + + @pytest.mark.parametrize("rows,passed,skipped", [([], 0, 0), + ([{"state": "Skipped:Ignored"}], 0, 1)]) + def test_no_passing_test_cannot_report_success(self, rows, passed, skipped): + outcome = lh._outcome_from_terminal( + "editmode", "EditMode", self.terminal(rows, passed, skipped=skipped), True) + assert outcome.status == "fail" + assert "did not execute" in outcome.detail + + @pytest.mark.parametrize("state", ["Unknown", "", "Inconclusive"]) + def test_unrecognized_or_inconclusive_leaf_is_not_counted_as_a_pass(self, state): + outcome = lh._outcome_from_terminal( + "editmode", "EditMode", self.terminal([{"state": state}], 1), True) + assert outcome.status == "fail" + assert "unsupported" in outcome.detail + + def test_row_buckets_must_match_summary(self): + outcome = lh._outcome_from_terminal( + "editmode", "EditMode", self.terminal([{"state": "Passed"}], 0, skipped=1), True) + assert outcome.status == "fail" + assert "disagree" in outcome.detail + + def test_initialization_failure_without_results_retains_progress_errors(self): + response = {"status": "failed", "result": None, "error": "initialization timed out", + "progress": {"failures_so_far": [{"full_name": "Suite.Start", "message": "callback error"}]}} + outcome = lh._outcome_from_terminal("editmode", "EditMode", response, True) + assert outcome.status == "fail" + assert "callback error" in outcome.junit_suite.cases[0].failure + + def test_poll_requests_complete_terminal_details(self): + def send(command, params, **kwargs): + assert command == "get_test_job" + assert params["includeDetails"] is True + return self.terminal([{"state": "Passed"}], 1) + + result = lh._poll_utf(send, "job", "instance", lh.time.time() + 10, 1, 10) + assert lh._dig(result, "status") == "succeeded"