Add PR A CLI browser command parity with MCP (#4789)
This commit is contained in:
Marc Kelechava
@@ -3,7 +3,10 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from datetime import datetime, timezone
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
import typer
|
||||
@@ -147,3 +150,149 @@ class TestResolveConnection:
|
||||
monkeypatch.setattr("skyvern.cli.commands._state.STATE_FILE", tmp_path / "nonexistent.json")
|
||||
with pytest.raises(typer.BadParameter, match="No active browser connection"):
|
||||
_resolve_connection(None, None)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Browser command helpers and command behavior
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestBrowserCommandGuards:
|
||||
def test_resolve_ai_target_requires_selector_or_intent(self) -> None:
|
||||
from skyvern.cli.commands.browser import _resolve_ai_target
|
||||
from skyvern.cli.core.guards import GuardError
|
||||
|
||||
with pytest.raises(GuardError, match="Must provide intent, selector, or both"):
|
||||
_resolve_ai_target(None, None, operation="click")
|
||||
|
||||
def test_validate_wait_state_rejects_invalid(self) -> None:
|
||||
from skyvern.cli.commands.browser import _validate_wait_state
|
||||
from skyvern.cli.core.guards import GuardError
|
||||
|
||||
with pytest.raises(GuardError, match="Invalid state"):
|
||||
_validate_wait_state("bad-state")
|
||||
|
||||
|
||||
class TestBrowserCommands:
|
||||
def test_session_get_outputs_session_details(
|
||||
self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture
|
||||
) -> None:
|
||||
from skyvern.cli.commands import browser as browser_cmd
|
||||
|
||||
session_obj = SimpleNamespace(
|
||||
browser_session_id="pbs_123",
|
||||
status="active",
|
||||
started_at=datetime(2026, 2, 17, 12, 0, tzinfo=timezone.utc),
|
||||
completed_at=None,
|
||||
timeout=60,
|
||||
runnable_id=None,
|
||||
)
|
||||
skyvern = SimpleNamespace(get_browser_session=AsyncMock(return_value=session_obj))
|
||||
monkeypatch.setattr(browser_cmd, "get_skyvern", lambda: skyvern)
|
||||
monkeypatch.setattr(browser_cmd, "load_state", lambda: CLIState(session_id="pbs_123", mode="cloud"))
|
||||
|
||||
browser_cmd.session_get(session="pbs_123", json_output=True)
|
||||
|
||||
parsed = json.loads(capsys.readouterr().out)
|
||||
assert parsed["ok"] is True
|
||||
assert parsed["action"] == "session_get"
|
||||
assert parsed["data"]["session_id"] == "pbs_123"
|
||||
assert parsed["data"]["is_current"] is True
|
||||
|
||||
def test_evaluate_blocks_password_js_before_connection(
|
||||
self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture
|
||||
) -> None:
|
||||
from skyvern.cli.commands import browser as browser_cmd
|
||||
|
||||
monkeypatch.setattr(
|
||||
browser_cmd,
|
||||
"_resolve_connection",
|
||||
lambda _session, _cdp: (_ for _ in ()).throw(AssertionError("should not resolve connection")),
|
||||
)
|
||||
|
||||
with pytest.raises(SystemExit, match="1"):
|
||||
browser_cmd.evaluate(
|
||||
expression='document.querySelector("input[type=password]").value = ""', json_output=True
|
||||
)
|
||||
|
||||
parsed = json.loads(capsys.readouterr().out)
|
||||
assert parsed["ok"] is False
|
||||
assert "Cannot set password field values" in parsed["error"]["message"]
|
||||
|
||||
def test_click_requires_target_before_connection(
|
||||
self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture
|
||||
) -> None:
|
||||
from skyvern.cli.commands import browser as browser_cmd
|
||||
|
||||
monkeypatch.setattr(
|
||||
browser_cmd,
|
||||
"_resolve_connection",
|
||||
lambda _session, _cdp: (_ for _ in ()).throw(AssertionError("should not resolve connection")),
|
||||
)
|
||||
|
||||
with pytest.raises(SystemExit, match="1"):
|
||||
browser_cmd.click(
|
||||
intent=None,
|
||||
selector=None,
|
||||
session=None,
|
||||
cdp=None,
|
||||
timeout=30000,
|
||||
button=None,
|
||||
click_count=None,
|
||||
json_output=True,
|
||||
)
|
||||
|
||||
parsed = json.loads(capsys.readouterr().out)
|
||||
assert parsed["ok"] is False
|
||||
assert "Must provide intent, selector, or both" in parsed["error"]["message"]
|
||||
|
||||
def test_click_with_intent_uses_proactive_ai_mode(
|
||||
self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture
|
||||
) -> None:
|
||||
from skyvern.cli.commands import browser as browser_cmd
|
||||
|
||||
page = MagicMock()
|
||||
page.click = AsyncMock(return_value="xpath=//button[@id='submit']")
|
||||
browser = SimpleNamespace(get_working_page=AsyncMock(return_value=page))
|
||||
|
||||
monkeypatch.setattr(
|
||||
browser_cmd,
|
||||
"_resolve_connection",
|
||||
lambda _session, _cdp: browser_cmd.ConnectionTarget(mode="cloud", session_id="pbs_123"),
|
||||
)
|
||||
monkeypatch.setattr(browser_cmd, "_connect_browser", AsyncMock(return_value=browser))
|
||||
|
||||
browser_cmd.click(
|
||||
intent="the Submit button",
|
||||
selector=None,
|
||||
session="pbs_123",
|
||||
cdp=None,
|
||||
timeout=30000,
|
||||
button=None,
|
||||
click_count=None,
|
||||
json_output=True,
|
||||
)
|
||||
|
||||
parsed = json.loads(capsys.readouterr().out)
|
||||
assert parsed["ok"] is True
|
||||
assert parsed["action"] == "click"
|
||||
assert parsed["data"]["ai_mode"] == "proactive"
|
||||
assert parsed["data"]["resolved_selector"] == "xpath=//button[@id='submit']"
|
||||
|
||||
def test_wait_rejects_invalid_state_before_connection(
|
||||
self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture
|
||||
) -> None:
|
||||
from skyvern.cli.commands import browser as browser_cmd
|
||||
|
||||
monkeypatch.setattr(
|
||||
browser_cmd,
|
||||
"_resolve_connection",
|
||||
lambda _session, _cdp: (_ for _ in ()).throw(AssertionError("should not resolve connection")),
|
||||
)
|
||||
|
||||
with pytest.raises(SystemExit, match="1"):
|
||||
browser_cmd.wait(state="bad-state", time_ms=1000, json_output=True)
|
||||
|
||||
parsed = json.loads(capsys.readouterr().out)
|
||||
assert parsed["ok"] is False
|
||||
assert "Invalid state" in parsed["error"]["message"]
|
||||
|
||||
264
tests/unit/test_mcp_session_lifecycle.py
Normal file
264
tests/unit/test_mcp_session_lifecycle.py
Normal file
@@ -0,0 +1,264 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from skyvern.cli.core import client as client_mod
|
||||
from skyvern.cli.core import session_manager
|
||||
from skyvern.cli.core.result import BrowserContext
|
||||
from skyvern.cli.core.session_ops import SessionCloseResult
|
||||
from skyvern.cli.mcp_tools import session as mcp_session
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _reset_singletons() -> None:
|
||||
client_mod._skyvern_instance.set(None)
|
||||
client_mod._global_skyvern_instance = None
|
||||
|
||||
session_manager._current_session.set(None)
|
||||
session_manager._global_session = None
|
||||
mcp_session.set_current_session(mcp_session.SessionState())
|
||||
|
||||
|
||||
def test_get_skyvern_reuses_global_instance_across_contexts(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
created: list[object] = []
|
||||
|
||||
class FakeSkyvern:
|
||||
def __init__(self, *args: object, **kwargs: object) -> None:
|
||||
created.append(self)
|
||||
|
||||
@classmethod
|
||||
def local(cls) -> FakeSkyvern:
|
||||
return cls()
|
||||
|
||||
async def aclose(self) -> None:
|
||||
return None
|
||||
|
||||
monkeypatch.setattr(client_mod, "Skyvern", FakeSkyvern)
|
||||
monkeypatch.setattr(client_mod.settings, "SKYVERN_API_KEY", None)
|
||||
monkeypatch.setattr(client_mod.settings, "SKYVERN_BASE_URL", None)
|
||||
|
||||
first = client_mod.get_skyvern()
|
||||
client_mod._skyvern_instance.set(None) # Simulate a new async context.
|
||||
second = client_mod.get_skyvern()
|
||||
|
||||
assert first is second
|
||||
assert len(created) == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_skyvern_closes_singleton() -> None:
|
||||
fake = MagicMock()
|
||||
fake.aclose = AsyncMock()
|
||||
|
||||
client_mod._skyvern_instance.set(fake)
|
||||
client_mod._global_skyvern_instance = fake
|
||||
|
||||
await client_mod.close_skyvern()
|
||||
|
||||
fake.aclose.assert_awaited_once()
|
||||
assert client_mod._skyvern_instance.get() is None
|
||||
assert client_mod._global_skyvern_instance is None
|
||||
|
||||
|
||||
def test_get_current_session_falls_back_to_global_state() -> None:
|
||||
state = session_manager.SessionState(
|
||||
browser=MagicMock(),
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_123"),
|
||||
)
|
||||
session_manager.set_current_session(state)
|
||||
|
||||
session_manager._current_session.set(None) # Simulate a new async context.
|
||||
recovered = session_manager.get_current_session()
|
||||
|
||||
assert recovered is state
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_browser_reuses_matching_cloud_session(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
current_browser = MagicMock()
|
||||
current_state = session_manager.SessionState(
|
||||
browser=current_browser,
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_123"),
|
||||
)
|
||||
session_manager.set_current_session(current_state)
|
||||
|
||||
fake_skyvern = MagicMock()
|
||||
fake_skyvern.connect_to_cloud_browser_session = AsyncMock()
|
||||
monkeypatch.setattr(session_manager, "get_skyvern", lambda: fake_skyvern)
|
||||
|
||||
browser, ctx = await session_manager.resolve_browser(session_id="pbs_123")
|
||||
|
||||
assert browser is current_browser
|
||||
assert ctx.session_id == "pbs_123"
|
||||
fake_skyvern.connect_to_cloud_browser_session.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_session_close_with_matching_session_id_closes_browser_handle(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
current_browser = MagicMock()
|
||||
current_browser.close = AsyncMock()
|
||||
mcp_session.set_current_session(
|
||||
mcp_session.SessionState(
|
||||
browser=current_browser,
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_456"),
|
||||
)
|
||||
)
|
||||
|
||||
fake_skyvern = MagicMock()
|
||||
monkeypatch.setattr(mcp_session, "get_skyvern", lambda: fake_skyvern)
|
||||
|
||||
do_session_close = AsyncMock(return_value=SessionCloseResult(session_id="pbs_456", closed=True))
|
||||
monkeypatch.setattr(mcp_session, "do_session_close", do_session_close)
|
||||
|
||||
result = await mcp_session.skyvern_session_close(session_id="pbs_456")
|
||||
|
||||
assert result["ok"] is True
|
||||
assert result["data"] == {"session_id": "pbs_456", "closed": True}
|
||||
current_browser.close.assert_awaited_once()
|
||||
do_session_close.assert_awaited_once_with(fake_skyvern, "pbs_456")
|
||||
assert mcp_session.get_current_session().browser is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_session_close_chains_exceptions_when_both_api_and_browser_fail(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""When both do_session_close (API) and browser.close() raise, the browser
|
||||
exception should chain the API exception via __cause__ so neither is lost."""
|
||||
current_browser = MagicMock()
|
||||
browser_error = RuntimeError("browser close failed")
|
||||
current_browser.close = AsyncMock(side_effect=browser_error)
|
||||
mcp_session.set_current_session(
|
||||
mcp_session.SessionState(
|
||||
browser=current_browser,
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_dual"),
|
||||
)
|
||||
)
|
||||
|
||||
fake_skyvern = MagicMock()
|
||||
monkeypatch.setattr(mcp_session, "get_skyvern", lambda: fake_skyvern)
|
||||
|
||||
api_error = ConnectionError("API close failed")
|
||||
do_session_close = AsyncMock(side_effect=api_error)
|
||||
monkeypatch.setattr(mcp_session, "do_session_close", do_session_close)
|
||||
|
||||
result = await mcp_session.skyvern_session_close(session_id="pbs_dual")
|
||||
|
||||
# The outer exception handler catches and returns an error result
|
||||
assert result["ok"] is False
|
||||
assert "browser close failed" in result["error"]["message"]
|
||||
# Session state is cleaned up regardless
|
||||
assert mcp_session.get_current_session().browser is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_session_close_matching_context_without_browser_returns_error(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
mcp_session.set_current_session(
|
||||
mcp_session.SessionState(
|
||||
browser=None,
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_999"),
|
||||
)
|
||||
)
|
||||
|
||||
fake_skyvern = MagicMock()
|
||||
monkeypatch.setattr(mcp_session, "get_skyvern", lambda: fake_skyvern)
|
||||
|
||||
do_session_close = AsyncMock(return_value=SessionCloseResult(session_id="pbs_999", closed=True))
|
||||
monkeypatch.setattr(mcp_session, "do_session_close", do_session_close)
|
||||
|
||||
result = await mcp_session.skyvern_session_close(session_id="pbs_999")
|
||||
|
||||
assert result["ok"] is False
|
||||
assert result["error"]["code"] == mcp_session.ErrorCode.SDK_ERROR
|
||||
assert "Expected active browser for matching cloud session" in result["error"]["message"]
|
||||
do_session_close.assert_awaited_once_with(fake_skyvern, "pbs_999")
|
||||
assert mcp_session.get_current_session().context is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests for close_current_session() — cloud session API cleanup
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_current_session_calls_api_close_for_cloud_session(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""close_current_session() should call do_session_close for cloud sessions
|
||||
and clear _browser_session_id to avoid a duplicate API call from browser.close()."""
|
||||
browser = MagicMock()
|
||||
browser.close = AsyncMock()
|
||||
browser._browser_session_id = "pbs_api"
|
||||
session_manager.set_current_session(
|
||||
session_manager.SessionState(
|
||||
browser=browser,
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_api"),
|
||||
)
|
||||
)
|
||||
|
||||
fake_skyvern = MagicMock()
|
||||
monkeypatch.setattr(session_manager, "get_skyvern", lambda: fake_skyvern)
|
||||
|
||||
do_session_close = AsyncMock(return_value=SessionCloseResult(session_id="pbs_api", closed=True))
|
||||
monkeypatch.setattr("skyvern.cli.core.session_ops.do_session_close", do_session_close)
|
||||
|
||||
await session_manager.close_current_session()
|
||||
|
||||
do_session_close.assert_awaited_once_with(fake_skyvern, "pbs_api")
|
||||
browser.close.assert_awaited_once()
|
||||
# _browser_session_id should be cleared to prevent redundant API call
|
||||
assert browser._browser_session_id is None
|
||||
assert session_manager.get_current_session().browser is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_current_session_skips_api_close_for_local_session(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""close_current_session() should NOT call do_session_close for local sessions."""
|
||||
browser = MagicMock()
|
||||
browser.close = AsyncMock()
|
||||
session_manager.set_current_session(
|
||||
session_manager.SessionState(
|
||||
browser=browser,
|
||||
context=BrowserContext(mode="local"),
|
||||
)
|
||||
)
|
||||
|
||||
do_session_close = AsyncMock()
|
||||
monkeypatch.setattr("skyvern.cli.core.session_ops.do_session_close", do_session_close)
|
||||
|
||||
await session_manager.close_current_session()
|
||||
|
||||
do_session_close.assert_not_awaited()
|
||||
browser.close.assert_awaited_once()
|
||||
assert session_manager.get_current_session().browser is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_current_session_still_closes_browser_when_api_fails(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""When do_session_close raises, browser.close() should still run and state should be cleared."""
|
||||
browser = MagicMock()
|
||||
browser.close = AsyncMock()
|
||||
browser._browser_session_id = "pbs_fail"
|
||||
session_manager.set_current_session(
|
||||
session_manager.SessionState(
|
||||
browser=browser,
|
||||
context=BrowserContext(mode="cloud_session", session_id="pbs_fail"),
|
||||
)
|
||||
)
|
||||
|
||||
fake_skyvern = MagicMock()
|
||||
monkeypatch.setattr(session_manager, "get_skyvern", lambda: fake_skyvern)
|
||||
|
||||
do_session_close = AsyncMock(side_effect=ConnectionError("API unreachable"))
|
||||
monkeypatch.setattr("skyvern.cli.core.session_ops.do_session_close", do_session_close)
|
||||
|
||||
await session_manager.close_current_session()
|
||||
|
||||
do_session_close.assert_awaited_once_with(fake_skyvern, "pbs_fail")
|
||||
# browser.close() should still be called despite API failure
|
||||
browser.close.assert_awaited_once()
|
||||
# _browser_session_id should NOT be cleared (API close failed, let browser.close() try)
|
||||
assert browser._browser_session_id == "pbs_fail"
|
||||
assert session_manager.get_current_session().browser is None
|
||||
63
tests/unit/test_run_commands_cleanup.py
Normal file
63
tests/unit/test_run_commands_cleanup.py
Normal file
@@ -0,0 +1,63 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from skyvern.cli import run_commands
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _reset_cleanup_state() -> None:
|
||||
run_commands._mcp_cleanup_done = False
|
||||
|
||||
|
||||
def test_cleanup_mcp_resources_sync_runs_without_running_loop(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
cleanup = AsyncMock()
|
||||
monkeypatch.setattr(run_commands, "_cleanup_mcp_resources", cleanup)
|
||||
|
||||
run_commands._cleanup_mcp_resources_sync()
|
||||
|
||||
cleanup.assert_awaited_once()
|
||||
assert run_commands._mcp_cleanup_done is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cleanup_mcp_resources_sync_skips_when_loop_running(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
cleanup = AsyncMock()
|
||||
monkeypatch.setattr(run_commands, "_cleanup_mcp_resources", cleanup)
|
||||
|
||||
run_commands._cleanup_mcp_resources_sync()
|
||||
await asyncio.sleep(0)
|
||||
|
||||
cleanup.assert_not_awaited()
|
||||
assert run_commands._mcp_cleanup_done is False
|
||||
|
||||
|
||||
def test_cleanup_mcp_resources_sync_keeps_retry_possible_on_task_failure(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
async def failing_cleanup() -> None:
|
||||
raise RuntimeError("cleanup failed")
|
||||
|
||||
monkeypatch.setattr(run_commands, "_cleanup_mcp_resources", failing_cleanup)
|
||||
|
||||
run_commands._cleanup_mcp_resources_sync()
|
||||
|
||||
assert run_commands._mcp_cleanup_done is False
|
||||
|
||||
|
||||
def test_run_mcp_calls_blocking_cleanup_in_finally(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
cleanup_blocking = MagicMock()
|
||||
register = MagicMock()
|
||||
run = MagicMock(side_effect=RuntimeError("boom"))
|
||||
|
||||
monkeypatch.setattr(run_commands, "_cleanup_mcp_resources_blocking", cleanup_blocking)
|
||||
monkeypatch.setattr(run_commands.atexit, "register", register)
|
||||
monkeypatch.setattr(run_commands.mcp, "run", run)
|
||||
|
||||
with pytest.raises(RuntimeError, match="boom"):
|
||||
run_commands.run_mcp()
|
||||
|
||||
register.assert_called_once_with(run_commands._cleanup_mcp_resources_sync)
|
||||
run.assert_called_once_with(transport="stdio")
|
||||
cleanup_blocking.assert_called_once()
|
||||
Reference in New Issue
Block a user