From 96ee1684cb99c2478a8f49a1850f48eaf3d95605 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 25 Feb 2024 19:23:00 +0000 Subject: [PATCH 1/6] lsp: Add `scope_for` method This adds a method that allows components to ask the configuration system which configuration scope a given uri would fall into --- lib/esbonio/esbonio/server/_configuration.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib/esbonio/esbonio/server/_configuration.py b/lib/esbonio/esbonio/server/_configuration.py index 11021397e..48dc346a6 100644 --- a/lib/esbonio/esbonio/server/_configuration.py +++ b/lib/esbonio/esbonio/server/_configuration.py @@ -257,6 +257,25 @@ def get(self, section: str, spec: Type[T], scope: Optional[Uri] = None) -> T: return self._get_config(section, spec, workspace_scope, file_scope) + def scope_for(self, uri: Uri) -> str: + """Return the configuration scope that corresponds to the given uri. + + Parameters + ---------- + uri + The uri to return the scope for + + Returns + ------- + str + The scope corresponding with the given uri + """ + + file_scope = self._uri_to_file_scope(uri) + workspace_scope = self._uri_to_workspace_scope(uri) + + return max([file_scope, workspace_scope], key=len) + def _get_config( self, section: str, spec: Type[T], workspace_scope: str, file_scope: str ) -> T: From 9ada6aeed50ee3110c40a5047617abd46106149d Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 25 Feb 2024 19:25:45 +0000 Subject: [PATCH 2/6] lsp: Make `Configuration.subscribe` syncronous By pushing the execution of async configuration change handlers into `asyncio.Task` objects, we can make the `subscribe()` method syncronous. Not only does this not force users of the function to be async, we now actually detect duplicate subscriptions and prevent unecessary calls to the given handler. --- lib/esbonio/esbonio/server/_configuration.py | 57 ++++++++++--------- .../server/features/directives/__init__.py | 4 +- lib/esbonio/esbonio/server/features/log.py | 4 +- lib/esbonio/esbonio/server/server.py | 3 +- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/lib/esbonio/esbonio/server/_configuration.py b/lib/esbonio/esbonio/server/_configuration.py index 48dc346a6..c936206c0 100644 --- a/lib/esbonio/esbonio/server/_configuration.py +++ b/lib/esbonio/esbonio/server/_configuration.py @@ -1,9 +1,12 @@ from __future__ import annotations +import asyncio import inspect import json import pathlib +import traceback import typing +from functools import partial from typing import Generic from typing import TypeVar @@ -111,6 +114,9 @@ def __init__(self, server: EsbonioLanguageServer): self._subscriptions: Dict[Subscription, Any] = {} """Subscriptions and their last known value""" + self._tasks: Set[asyncio.Task] = set() + """Holds tasks that are currently executing an async config handler.""" + @property def initialization_options(self): return self._initialization_options @@ -141,7 +147,7 @@ def supports_workspace_config(self): self.server.client_capabilities, "workspace.configuration", False ) - async def subscribe( + def subscribe( self, section: str, spec: Type[T], @@ -174,27 +180,12 @@ async def subscribe( self.logger.debug("Ignoring duplicate subscription: %s", subscription) return - # Wait until the server is ready before fetching the initial configuration - await self.server.ready - - result = self.get(section, spec, scope) - self._subscriptions[subscription] = result + self._subscriptions[subscription] = None - change_event = ConfigChangeEvent( - scope=max([file_scope, workspace_scope], key=len), value=result - ) - self.logger.info("%s", change_event) + # Once the server is ready, update all the subscriptions + self.server.ready.add_done_callback(self._notify_subscriptions) - try: - ret = callback(change_event) - if inspect.isawaitable(ret): - await ret - except Exception: - self.logger.error( - "Error in configuration callback: %s", callback, exc_info=True - ) - - async def _notify_subscriptions(self): + def _notify_subscriptions(self, *args): """Notify subscriptions about configuration changes, if necessary.""" for subscription, previous_value in self._subscriptions.items(): @@ -211,6 +202,7 @@ async def _notify_subscriptions(self): if previous_value == value: continue + self._subscriptions[subscription] = value change_event = ConfigChangeEvent( scope=max( [subscription.file_scope, subscription.workspace_scope], key=len @@ -222,10 +214,10 @@ async def _notify_subscriptions(self): try: ret = subscription.callback(change_event) - if inspect.isawaitable(ret): - await ret + if inspect.iscoroutine(ret): + task = asyncio.create_task(ret) + task.add_done_callback(partial(self._finish_task, subscription)) - self._subscriptions[subscription] = value except Exception: self.logger.error( "Error in configuration callback: %s", @@ -233,6 +225,17 @@ async def _notify_subscriptions(self): exc_info=True, ) + def _finish_task(self, subscription: Subscription, task: asyncio.Task[None]): + """Cleanup a finished task.""" + self._tasks.discard(task) + + if (exc := task.exception()) is not None: + self.logger.error( + "Error in async configuration handler '%s'\n%s", + subscription.callback, + traceback.format_exception(type(exc), exc, exc.__traceback__), + ) + def get(self, section: str, spec: Type[T], scope: Optional[Uri] = None) -> T: """Get the requested configuration section. @@ -346,9 +349,7 @@ def _discover_config_files(self) -> List[pathlib.Path]: return paths - async def update_file_configuration( - self, paths: Optional[List[pathlib.Path]] = None - ): + def update_file_configuration(self, paths: Optional[List[pathlib.Path]] = None): """Update the internal cache of configuration coming from files. Parameters @@ -375,7 +376,7 @@ async def update_file_configuration( "Unable to read configuration file: '%s'", exc_info=True ) - await self._notify_subscriptions() + self._notify_subscriptions() async def update_workspace_configuration(self): """Update the internal cache of the client's workspace configuration.""" @@ -416,7 +417,7 @@ async def update_workspace_configuration(self): self._workspace_config[scope or ""] = result - await self._notify_subscriptions() + self._notify_subscriptions() def _uri_to_scope(known_scopes: List[str], uri: Optional[Uri]) -> str: diff --git a/lib/esbonio/esbonio/server/features/directives/__init__.py b/lib/esbonio/esbonio/server/features/directives/__init__.py index 01a052f32..e00bb3582 100644 --- a/lib/esbonio/esbonio/server/features/directives/__init__.py +++ b/lib/esbonio/esbonio/server/features/directives/__init__.py @@ -63,9 +63,9 @@ def add_provider(self, provider: DirectiveProvider): completion_triggers = [RST_DIRECTIVE, MYST_DIRECTIVE] - async def initialized(self, params: types.InitializedParams): + def initialized(self, params: types.InitializedParams): """Called once the initial handshake between client and server has finished.""" - await self.configuration.subscribe( + self.configuration.subscribe( "esbonio.server.completion", server.CompletionConfig, self.update_configuration, diff --git a/lib/esbonio/esbonio/server/features/log.py b/lib/esbonio/esbonio/server/features/log.py index afbfa411d..140cb55ad 100644 --- a/lib/esbonio/esbonio/server/features/log.py +++ b/lib/esbonio/esbonio/server/features/log.py @@ -185,9 +185,9 @@ class LogManager(server.LanguageFeature): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - async def initialized(self, params: types.InitializedParams): + def initialized(self, params: types.InitializedParams): """Setup logging.""" - await self.server.configuration.subscribe( + self.server.configuration.subscribe( "esbonio.server", ServerLogConfig, self.setup_logging ) diff --git a/lib/esbonio/esbonio/server/server.py b/lib/esbonio/esbonio/server/server.py index ff2c7adbd..3e9494bfa 100644 --- a/lib/esbonio/esbonio/server/server.py +++ b/lib/esbonio/esbonio/server/server.py @@ -115,9 +115,10 @@ def initialize(self, params: types.InitializeParams): self.configuration.initialization_options = params.initialization_options async def initialized(self, params: types.InitializedParams): + self.configuration.update_file_configuration() + await asyncio.gather( self.configuration.update_workspace_configuration(), - self.configuration.update_file_configuration(), self._register_did_change_configuration_handler(), self._register_did_change_watched_files_handler(), ) From 1a6a5599e87546d40234ff573bbc72dcac88a959 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 25 Feb 2024 19:30:41 +0000 Subject: [PATCH 3/6] lsp: Refactor `SubprocessSphinxClient` This commit refactors the `SubprocessSphinxClient` so that it's hopefully much more robust. - There is no longer a separation between starting the client and creating a Sphinx application. There is now only a single `start()` method. - The `start()` method no longer takes any arguments (the `SphinxConfig` is passed to the constructor instead) and the method returns `self` - The client is now `await`-able. If required the `start()` method will be called and the client simply returned otherwise. - There is now a `ClientState` enum which better explains the current condition of the client. - The client is now an `EventSource` and will emit a `state-change` event whenever the state is changed. --- .../features/sphinx_manager/__init__.py | 2 + .../server/features/sphinx_manager/client.py | 53 +++-- .../sphinx_manager/client_subprocess.py | 190 ++++++++++++------ 3 files changed, 166 insertions(+), 79 deletions(-) diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py b/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py index 36bb44009..f80730418 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py @@ -2,6 +2,7 @@ from esbonio.server import EsbonioLanguageServer +from .client import ClientState from .client import SphinxClient from .client_mock import MockSphinxClient from .client_mock import mock_sphinx_client_factory @@ -10,6 +11,7 @@ from .manager import SphinxManager __all__ = [ + "ClientState", "SphinxClient", "SphinxConfig", "SphinxManager", diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/client.py b/lib/esbonio/esbonio/server/features/sphinx_manager/client.py index d593b70a9..45a9c802d 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/client.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/client.py @@ -1,11 +1,13 @@ from __future__ import annotations +import enum import typing from typing import Protocol if typing.TYPE_CHECKING: from typing import Any from typing import Dict + from typing import Generator from typing import List from typing import Optional from typing import Tuple @@ -15,47 +17,70 @@ from esbonio.server import Uri from esbonio.sphinx_agent import types - from .config import SphinxConfig + +class ClientState(enum.Enum): + """The set of possible states the client may be in.""" + + Starting = enum.auto() + """The client is starting.""" + + Running = enum.auto() + """The client is running normally.""" + + Building = enum.auto() + """The client is currently building.""" + + Errored = enum.auto() + """The client has enountered some unrecoverable error and should not be used.""" + + Exited = enum.auto() + """The client is no longer running.""" class SphinxClient(Protocol): """Describes the API language features can use to inspect/manipulate a Sphinx application instance.""" + state: Optional[ClientState] + sphinx_info: Optional[types.SphinxInfo] + @property - def id(self) -> Optional[str]: + def id(self) -> str: """The id of the Sphinx instance.""" + ... @property def db(self) -> Optional[aiosqlite.Connection]: """Connection to the associated database.""" @property - def builder(self) -> Optional[str]: + def builder(self) -> str: """The name of the Sphinx builder.""" + ... @property - def building(self) -> bool: - """Indicates if the Sphinx instance is building.""" - - @property - def build_uri(self) -> Optional[Uri]: + def build_uri(self) -> Uri: """The URI to the Sphinx application's build dir.""" + ... @property - def conf_uri(self) -> Optional[Uri]: + def conf_uri(self) -> Uri: """The URI to the Sphinx application's conf dir.""" + ... @property - def src_uri(self) -> Optional[Uri]: + def src_uri(self) -> Uri: """The URI to the Sphinx application's src dir.""" + ... - async def start(self, config: SphinxConfig): - """Start the client.""" + def __await__(self) -> Generator[Any, None, SphinxClient]: + """A SphinxClient should be awaitable""" ... - async def create_application(self, config: SphinxConfig) -> types.SphinxInfo: - """Create a new Sphinx application instance.""" + def add_listener(self, event: str, handler): ... + + async def start(self) -> SphinxClient: + """Start the client.""" ... async def build( diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py b/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py index 7c78ce313..ee1494eb4 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py @@ -7,6 +7,7 @@ import logging import os import subprocess +import sys import typing import aiosqlite @@ -14,8 +15,10 @@ from pygls.protocol import JsonRPCProtocol import esbonio.sphinx_agent.types as types +from esbonio.server import EventSource from esbonio.server import Uri +from .client import ClientState from .config import SphinxConfig if typing.TYPE_CHECKING: @@ -48,78 +51,113 @@ class SubprocessSphinxClient(JsonRPCClient): def __init__( self, + config: SphinxConfig, logger: Optional[logging.Logger] = None, protocol_cls=SphinxAgentProtocol, *args, **kwargs, ): super().__init__(protocol_cls=protocol_cls, *args, **kwargs) # type: ignore[misc] + + self.config = config + """Configuration values.""" + self.logger = logger or logging.getLogger(__name__) + """The logger instance to use.""" self.sphinx_info: Optional[types.SphinxInfo] = None + """Information about the Sphinx application the client is connected to.""" + + self.state: Optional[ClientState] = None + """The current state of the client.""" + + self.exception: Optional[Exception] = None + """The most recently encountered exception (if any)""" + + self._events = EventSource(self.logger) + """The sphinx client can emit events.""" + + self._task: Optional[asyncio.Task] = None + """The startup task.""" + + def __repr__(self): + if self.state is None: + return "SphinxClient" - self._connection: Optional[aiosqlite.Connection] = None - self._building = False + if self.state == ClientState.Errored: + return f"SphinxClient<{self.state.name}: {self.exception}>" + + state = self.state.name + command = " ".join(self.config.build_command) + return f"SphinxClient<{state}: {command}>" + + def __await__(self): + """Makes the client await-able""" + if self._task is None: + self._task = asyncio.create_task(self.start()) + + return self._task.__await__() @property def converter(self): return self.protocol._converter @property - def id(self) -> Optional[str]: + def id(self) -> str: """The id of the Sphinx instance.""" if self.sphinx_info is None: - return None + raise RuntimeError("sphinx_info is None, has the client been started?") return self.sphinx_info.id @property - def building(self) -> bool: - return self._building - - @property - def builder(self) -> Optional[str]: + def builder(self) -> str: """The sphinx application's builder name""" if self.sphinx_info is None: - return None + raise RuntimeError("sphinx_info is None, has the client been started?") return self.sphinx_info.builder_name @property - def src_uri(self) -> Optional[Uri]: + def src_uri(self) -> Uri: """The src uri of the Sphinx application.""" if self.sphinx_info is None: - return None + raise RuntimeError("sphinx_info is None, has the client been started?") return Uri.for_file(self.sphinx_info.src_dir) @property - def conf_uri(self) -> Optional[Uri]: + def conf_uri(self) -> Uri: """The conf uri of the Sphinx application.""" if self.sphinx_info is None: - return None + raise RuntimeError("sphinx_info is None, has the client been started?") return Uri.for_file(self.sphinx_info.conf_dir) @property def db(self) -> Optional[aiosqlite.Connection]: """Connection to the associated database.""" - return self._connection + return None @property - def build_uri(self) -> Optional[Uri]: + def build_uri(self) -> Uri: """The build uri of the Sphinx application.""" if self.sphinx_info is None: - return None + raise RuntimeError("sphinx_info is None, has the client been started?") return Uri.for_file(self.sphinx_info.build_dir) + def add_listener(self, event: str, handler): + self._events.add_listener(event, handler) + async def server_exit(self, server: asyncio.subprocess.Process): """Called when the sphinx agent process exits.""" # 0: all good # -15: terminated if server.returncode not in {0, -15}: + self.exception = RuntimeError(server.returncode) + self._set_state(ClientState.Errored) self.logger.error( f"sphinx-agent process exited with code: {server.returncode}" ) @@ -136,66 +174,59 @@ async def server_exit(self, server: asyncio.subprocess.Process): "%s future '%s' for pending request '%s'", message, fut, id_ ) - async def start(self, config: SphinxConfig): + if self.state != ClientState.Errored: + self._set_state(ClientState.Exited) + + async def start(self) -> SphinxClient: """Start the client.""" - if len(config.python_command) == 0: - raise ValueError("No python environment configured") + # Only try starting once. + if self.state is not None: + return self - command = [] + try: + self._set_state(ClientState.Starting) + command = get_start_command(self.config, self.logger) + env = get_sphinx_env(self.config) - if config.enable_dev_tools and ( - lsp_devtools := self._get_lsp_devtools_command() - ): - command.extend([lsp_devtools, "agent", "--"]) + self.logger.debug("Starting sphinx agent: %s", " ".join(command)) + await self.start_io(*command, env=env, cwd=self.config.cwd) - command.extend([*config.python_command, "-m", "sphinx_agent"]) - env = get_sphinx_env(config) + params = types.CreateApplicationParams( + command=self.config.build_command, + enable_sync_scrolling=self.config.enable_sync_scrolling, + ) - self.logger.debug("Starting sphinx agent: %s", " ".join(command)) - await self.start_io(*command, env=env, cwd=config.cwd) + self.sphinx_info = await self.protocol.send_request_async( + "sphinx/createApp", params + ) - def _get_lsp_devtools_command(self) -> Optional[str]: - # Assumes that the user has `lsp-devtools` available on their PATH - # TODO: Windows support - result = subprocess.run(["command", "-v", "lsp-devtools"], capture_output=True) - if result.returncode == 0: - lsp_devtools = result.stdout.decode("utf8").strip() - return lsp_devtools + self._set_state(ClientState.Running) + return self + except Exception as exc: + self.logger.debug("Unable to start SphinxClient: %s", exc, exc_info=True) - stderr = result.stderr.decode("utf8").strip() - self.logger.debug("Unable to locate lsp-devtools command", stderr) - return None + self.exception = exc + self._set_state(ClientState.Errored) + + return self + + def _set_state(self, new_state: ClientState): + """Change the state of the client.""" + old_state, self.state = self.state, new_state + self._events.trigger("state-change", self, old_state, new_state) async def stop(self): """Stop the client.""" - self.protocol.notify("exit", None) - if self._connection: - await self._connection.close() + if self.state in {ClientState.Running, ClientState.Building}: + self.protocol.notify("exit", None) # Give the agent a little time to close. # await asyncio.sleep(0.5) + self.logger.debug(self._async_tasks) await super().stop() - async def create_application(self, config: SphinxConfig) -> types.SphinxInfo: - """Create a sphinx application object.""" - - params = types.CreateApplicationParams( - command=config.build_command, - enable_sync_scrolling=config.enable_sync_scrolling, - ) - - sphinx_info = await self.protocol.send_request_async("sphinx/createApp", params) - self.sphinx_info = sphinx_info - - try: - self._connection = await aiosqlite.connect(sphinx_info.dbpath) - except Exception: - self.logger.error("Unable to connect to database", exc_info=True) - - return sphinx_info - async def build( self, *, @@ -345,7 +376,9 @@ async def get_diagnostics(self) -> Dict[Uri, List[Dict[str, Any]]]: return results -def make_subprocess_sphinx_client(manager: SphinxManager) -> SphinxClient: +def make_subprocess_sphinx_client( + manager: SphinxManager, config: SphinxConfig +) -> SphinxClient: """Factory function for creating a ``SubprocessSphinxClient`` instance. Parameters @@ -353,12 +386,15 @@ def make_subprocess_sphinx_client(manager: SphinxManager) -> SphinxClient: manager The manager instance creating the client + config + The Sphinx configuration + Returns ------- SphinxClient The configured client """ - client = SubprocessSphinxClient(logger=manager.logger) + client = SubprocessSphinxClient(config, logger=manager.logger) @client.feature("window/logMessage") def _on_msg(ls: SubprocessSphinxClient, params): @@ -371,17 +407,17 @@ def _on_progress(ls: SubprocessSphinxClient, params): return client -def make_test_sphinx_client() -> SubprocessSphinxClient: +def make_test_sphinx_client(config: SphinxConfig) -> SubprocessSphinxClient: """Factory function for creating a ``SubprocessSphinxClient`` instance to use for testing.""" logger = logging.getLogger("sphinx_client") logger.setLevel(logging.INFO) - client = SubprocessSphinxClient() + client = SubprocessSphinxClient(config) @client.feature("window/logMessage") def _(params): - logger.info("%s", params.message) + print(params.message, file=sys.stderr) @client.feature("$/progress") def _on_progress(params): @@ -404,3 +440,27 @@ def get_sphinx_env(config: SphinxConfig) -> Dict[str, str]: env[envname] = value return env + + +def get_start_command(config: SphinxConfig, logger: logging.Logger): + """Return the command to use to start the sphinx agent.""" + + command = [] + + if len(config.python_command) == 0: + raise ValueError("No python environment configured") + + if config.enable_dev_tools: + # Assumes that the user has `lsp-devtools` available on their PATH + # TODO: Windows support + result = subprocess.run(["command", "-v", "lsp-devtools"], capture_output=True) + if result.returncode == 0: + lsp_devtools = result.stdout.decode("utf8").strip() + command.extend([lsp_devtools, "agent", "--"]) + + else: + stderr = result.stderr.decode("utf8").strip() + logger.debug("Unable to locate lsp-devtools command", stderr) + + command.extend([*config.python_command, "-m", "sphinx_agent"]) + return command From 2f17256eea753e3c22c1a1f67fbdfc3d3b34d75d Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 25 Feb 2024 19:44:13 +0000 Subject: [PATCH 4/6] lsp: Refactor `SphinxManager` This commit builds on the changes introduced to the `SubprocessSphinxClient` to greatly simplify and improve the robustness of managing the set of `SphinxClients` in use by the language server. There were a few drawbacks to the previous approach - While await on the `_client_creating` task during client creation appeared to work, it still often led to the creation of duplicated clients. - By not recognizing when a given configuration was broken, the manager would continually attempt to spawn client instances. - It was not uncommon to enter an infinite loop of spawning clients! The new system introduced in this commit *shouldn't* suffer from any of the above issues. - Instead of indexing clients by `src_uri`, they are instead indexed by the scope of the configuration governing the instance. While still not perfect, we should be able to accurately determine the relevant client for a given uri in more scenarios. - To indicate that we are unable to create a client for a given config we now store a `None` in the clients dictionary - preventing any further attempts at spawning a client. - Taking advantage of the fact clients are indexed by scope and that the client itself is `await`-able, we can have multiple consumers of the client awaiting on the client - preventing the issue of duplicated clients. Currently, there is a quirk to the system where the first batch of callers to `get_client` in a given scope will be told there is no client. However, they have in fact "primed" the system by creating a config subscription which will asynchronously populate the `clients` dict with an unstarted client. The next batch of callers will then discover that there is a client instance available and `await` it, starting the underlying Sphinx process. Based on some initial testing this "quirk" may actually be desirable, the `get_client` method is called often enough that I wasn't able to notice a real delay in starting the Sphinx process up. It also has the nice property that when restarting the server with multiple projects open, the flood of `didOpen` notifications from the client is enough to "prime" the system for each project, but the project itself is only activated when working on the relevant file in the editor. One final note, there are plenty of improvements still to be made - error handling, config changes etc, but hopefully this is a solid base we can start building on. --- lib/esbonio/esbonio/server/__init__.py | 2 + .../server/features/sphinx_manager/manager.py | 159 ++++------- lib/esbonio/tests/e2e/test_sphinx_manager.py | 270 ++++++++++++++++++ .../server/features/test_sphinx_manager.py | 97 ------- 4 files changed, 333 insertions(+), 195 deletions(-) create mode 100644 lib/esbonio/tests/e2e/test_sphinx_manager.py delete mode 100644 lib/esbonio/tests/server/features/test_sphinx_manager.py diff --git a/lib/esbonio/esbonio/server/__init__.py b/lib/esbonio/esbonio/server/__init__.py index ae67baca5..91ccd317d 100644 --- a/lib/esbonio/esbonio/server/__init__.py +++ b/lib/esbonio/esbonio/server/__init__.py @@ -9,6 +9,7 @@ from .feature import LanguageFeature from .server import EsbonioLanguageServer from .server import EsbonioWorkspace +from .setup import create_language_server __all__ = ( "LOG_NAMESPACE", @@ -21,4 +22,5 @@ "LanguageFeature", "MemoryHandler", "Uri", + "create_language_server", ) diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py b/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py index 2dbf27e0e..560fadf41 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py @@ -1,30 +1,29 @@ from __future__ import annotations import asyncio -import sys import typing import uuid -from typing import Callable -from typing import Dict -from typing import Optional import lsprotocol.types as lsp import esbonio.sphinx_agent.types as types -from esbonio.server import EventSource -from esbonio.server import LanguageFeature +from esbonio import server from esbonio.server import Uri +from .client import ClientState from .config import SphinxConfig if typing.TYPE_CHECKING: - from .client import SphinxClient + from typing import Callable + from typing import Dict + from typing import Optional + from .client import SphinxClient -SphinxClientFactory = Callable[["SphinxManager"], "SphinxClient"] + SphinxClientFactory = Callable[["SphinxManager", "SphinxConfig"], "SphinxClient"] -class SphinxManager(LanguageFeature): +class SphinxManager(server.LanguageFeature): """Responsible for managing Sphinx application instances.""" def __init__(self, client_factory: SphinxClientFactory, *args, **kwargs): @@ -33,18 +32,18 @@ def __init__(self, client_factory: SphinxClientFactory, *args, **kwargs): self.client_factory = client_factory """Used to create new Sphinx client instances.""" - self.clients: Dict[Uri, SphinxClient] = {} + self.clients: Dict[str, Optional[SphinxClient]] = { + # Prevent any clients from being created in the global scope. + "": None, + } """Holds currently active Sphinx clients.""" - self._events = EventSource(self.logger) + self._events = server.EventSource(self.logger) """The SphinxManager can emit events.""" self._pending_builds: Dict[str, asyncio.Task] = {} """Holds tasks that will trigger a build after a given delay if not cancelled.""" - self._client_creating: Optional[asyncio.Task] = None - """If set, indicates we're in the process of setting up a new client.""" - self._progress_tokens: Dict[str, str] = {} """Holds work done progress tokens.""" @@ -56,7 +55,7 @@ async def document_change(self, params: lsp.DidChangeTextDocumentParams): return client = await self.get_client(uri) - if client is None or client.id is None: + if client is None: return # Cancel any existing pending builds @@ -78,7 +77,7 @@ async def document_save(self, params: lsp.DidSaveTextDocumentParams): return client = await self.get_client(uri) - if client is None or client.id is None: + if client is None: return # Cancel any existing pending builds @@ -90,20 +89,12 @@ async def document_save(self, params: lsp.DidSaveTextDocumentParams): async def shutdown(self, params: None): """Called when the server is instructed to ``shutdown``.""" - # Stop creating any new clients. - if self._client_creating and not self._client_creating.done(): - args = {} - if sys.version_info.minor > 8: - args["msg"] = "Server is shutting down" - - self.logger.debug("Aborting client creation") - self._client_creating.cancel(**args) - # Stop any existing clients. tasks = [] for client in self.clients.values(): - self.logger.debug("Stopping SphinxClient: %s", client) - tasks.append(asyncio.create_task(client.stop())) + if client: + self.logger.debug("Stopping SphinxClient: %s", client) + tasks.append(asyncio.create_task(client.stop())) await asyncio.gather(*tasks) @@ -118,7 +109,7 @@ async def trigger_build_after(self, uri: Uri, app_id: str, delay: float): async def trigger_build(self, uri: Uri): """Trigger a build for the relevant Sphinx application for the given uri.""" client = await self.get_client(uri) - if client is None or client.building: + if client is None or client.state != ClientState.Running: return # Pass through any unsaved content to the Sphinx agent. @@ -149,86 +140,61 @@ async def trigger_build(self, uri: Uri): async def get_client(self, uri: Uri) -> Optional[SphinxClient]: """Given a uri, return the relevant sphinx client instance for it.""" - # Wait until the new client is created - it might be the one we're looking for! - if self._client_creating: - await self._client_creating - - # Always check the fully resolved uri. - resolved_uri = uri.resolve() - - for src_uri, client in self.clients.items(): - if resolved_uri in (await client.get_src_uris()): - return client - - # For now assume a single client instance per srcdir. - # This *should* prevent us from spwaning endless client instances - # when given a file located near a valid Sphinx project - but not actually - # part of it. - in_src_dir = str(resolved_uri).startswith(str(src_uri)) - if in_src_dir: - # Of course, we can only tell if a uri truly is not in a project - # when the build file map is populated! - # if len(client.build_file_map) == 0: - return client - - # Create a new client instance. - self._client_creating = asyncio.create_task(self._create_client(uri)) - try: - return await self._client_creating - finally: - # Be sure to unset the task when it resolves - self._client_creating = None - - async def _create_client(self, uri: Uri) -> Optional[SphinxClient]: - """Create a new sphinx client instance.""" - # TODO: Replace with config subscription - await self.server.ready - config = self.server.configuration.get( - "esbonio.sphinx", SphinxConfig, scope=uri - ) - if config is None: + scope = self.server.configuration.scope_for(uri) + if scope not in self.clients: + self.logger.debug("No client found, creating new subscription") + self.server.configuration.subscribe( + "esbonio.sphinx", + SphinxConfig, + self._create_or_replace_client, + scope=uri, + ) + # The first few callers in a given scope will miss out, but that shouldn't matter + # too much return None - resolved = config.resolve(uri, self.server.workspace, self.logger) - if resolved is None: + if (client := self.clients[scope]) is None: + self.logger.debug("No applicable client for uri: %s", uri) return None - client = self.client_factory(self) + return await client - try: - await client.start(resolved) - except Exception as exc: - message = "Unable to start sphinx-agent" - self.logger.error(message, exc_info=True) - self.server.show_message(f"{message}: {exc}", lsp.MessageType.Error) + async def _create_or_replace_client( + self, event: server.ConfigChangeEvent[SphinxConfig] + ): + """Create or replace thesphinx client instance for the given config.""" - return None + # TODO: Handle replacement! + config = event.value - try: - sphinx_info = await client.create_application(resolved) - except Exception as exc: - message = "Unable to create sphinx application" - self.logger.error(message, exc_info=True) - self.server.show_message(f"{message}: {exc}", lsp.MessageType.Error) + # Do not try and create clients in the global scope + if event.scope == "": + return - await client.stop() - return None + resolved = config.resolve( + Uri.parse(event.scope), self.server.workspace, self.logger + ) + if resolved is None: + self.clients[event.scope] = None + return - if client.src_uri is None: - self.logger.error("No src uri!") - await client.stop() - return None + self.clients[event.scope] = client = self.client_factory(self, resolved) + client.add_listener("state-change", self._on_state_change) - self.server.lsp.notify("sphinx/appCreated", sphinx_info) - self.clients[client.src_uri] = client - return client + self.server.lsp.notify("sphinx/clientCreated", resolved) + self.logger.debug("Client created for scope %s", event.scope) + + def _on_state_change( + self, client: SphinxClient, old_state: ClientState, new_state: ClientState + ): + """React to state changes in the client.""" + + if old_state == ClientState.Starting and new_state == ClientState.Running: + self.server.lsp.notify("sphinx/appCreated", client.sphinx_info) async def start_progress(self, client: SphinxClient): """Start reporting work done progress for the given client.""" - if client.id is None: - return - token = str(uuid.uuid4()) self.logger.debug("Starting progress: '%s'", token) @@ -245,9 +211,6 @@ async def start_progress(self, client: SphinxClient): ) def stop_progress(self, client: SphinxClient): - if client.id is None: - return - if (token := self._progress_tokens.pop(client.id, None)) is None: return @@ -256,7 +219,7 @@ def stop_progress(self, client: SphinxClient): def report_progress(self, client: SphinxClient, progress: types.ProgressParams): """Report progress done for the given client.""" - if client.id is None: + if client.state not in {ClientState.Running, ClientState.Building}: return if (token := self._progress_tokens.get(client.id, None)) is None: diff --git a/lib/esbonio/tests/e2e/test_sphinx_manager.py b/lib/esbonio/tests/e2e/test_sphinx_manager.py new file mode 100644 index 000000000..bc1e8a85d --- /dev/null +++ b/lib/esbonio/tests/e2e/test_sphinx_manager.py @@ -0,0 +1,270 @@ +"""Technically, not a true end-to-end test as we're inspecting the internals of the server. +But since it depends on the sphinx agent, we may as well put it here.""" + +from __future__ import annotations + +import asyncio +import io +import pathlib +import sys +import typing + +import pytest +import pytest_asyncio +from lsprotocol import types as lsp +from pygls.server import StdOutTransportAdapter + +from esbonio.server import EsbonioLanguageServer +from esbonio.server import Uri +from esbonio.server import create_language_server +from esbonio.server.features.sphinx_manager import ClientState +from esbonio.server.features.sphinx_manager import SphinxManager +from esbonio.server.features.sphinx_manager import make_subprocess_sphinx_client + +if typing.TYPE_CHECKING: + from typing import Any + from typing import Callable + from typing import Tuple + + ServerManager = Callable[[Any], Tuple[EsbonioLanguageServer, SphinxManager]] + + +@pytest.fixture() +def demo_workspace(uri_for): + return uri_for("workspaces", "demo") + + +@pytest.fixture() +def docs_workspace(uri_for): + return uri_for("..", "..", "..", "docs") + + +@pytest_asyncio.fixture() +async def server_manager(demo_workspace: Uri, docs_workspace): + """An instance of the language server and sphinx manager to use for the test.""" + + loop = asyncio.get_running_loop() + + esbonio = create_language_server(EsbonioLanguageServer, [], loop=loop) + esbonio.lsp.transport = StdOutTransportAdapter(io.BytesIO(), sys.stderr.buffer) + + manager = SphinxManager(make_subprocess_sphinx_client, esbonio) + esbonio.add_feature(manager) + + def initialize(init_options): + # Initialize the server. + esbonio.lsp._procedure_handler( + lsp.InitializeRequest( + id=1, + params=lsp.InitializeParams( + capabilities=lsp.ClientCapabilities(), + initialization_options=init_options, + workspace_folders=[ + lsp.WorkspaceFolder(uri=str(demo_workspace), name="demo"), + lsp.WorkspaceFolder(uri=str(docs_workspace), name="docs"), + ], + ), + ) + ) + + esbonio.lsp._procedure_handler( + lsp.InitializedNotification(params=lsp.InitializedParams()) + ) + return esbonio, manager + + yield initialize + + await manager.shutdown(None) + + +@pytest.mark.asyncio +async def test_get_client( + server_manager: ServerManager, demo_workspace: Uri, tmp_path: pathlib.Path +): + """Ensure that we can create a SphinxClient correctly.""" + + server, manager = server_manager( + dict( + esbonio=dict( + sphinx=dict( + pythonCommand=[sys.executable], + buildCommand=["sphinx-build", "-M", "dirhtml", ".", str(tmp_path)], + ), + ), + ), + ) + # Ensure that the server is ready + await server.ready + + result = await manager.get_client(demo_workspace / "index.rst") + # At least for now, the first call to get_client will not return a client + # but will instead "prime the system" ready to return a client the next + # time it is called. + assert result is None + + # Give the async tasks chance to complete. + await asyncio.sleep(0.5) + + # The system should be "primed" meaning there is a client created, but not + # running. + client = manager.clients[str(demo_workspace)] + + assert client is not None + assert client.state is None + + # Now when we ask for the client, the client should be started and we should + # get back the same instance + result = await manager.get_client(demo_workspace / "index.rst") + + assert result is client + assert client.state == ClientState.Running + + # And that the client initialized correctly + assert client.builder == "dirhtml" + assert client.src_uri == demo_workspace + assert client.conf_uri == demo_workspace + assert client.build_uri == Uri.for_file(tmp_path / "dirhtml") + + +@pytest.mark.asyncio +async def test_get_client_with_error( + server_manager: ServerManager, demo_workspace: Uri +): + """Ensure that we correctly handle the case where there is an error with a client.""" + + server, manager = server_manager(None) + # Ensure that the server is ready + await server.ready + + result = await manager.get_client(demo_workspace / "index.rst") + # At least for now, the first call to get_client will not return a client + # but will instead "prime the system" ready to return a client the next + # time it is called. + assert result is None + + # Give the async tasks chance to complete. + await asyncio.sleep(0.5) + + # The system should be "primed" meaning there is a client created, but not + # running. + client = manager.clients[str(demo_workspace)] + + assert client is not None + assert client.state is None + + # Now when we ask for the client, the client should be started and we should + # get back the same instance + result = await manager.get_client(demo_workspace / "index.rst") + + assert result is client + assert client.state == ClientState.Errored + assert "No python environment configured" in str(client.exception) + + # Finally, if we request another uri from the same project we should get back + # the same client instance - even though it failed to start. + result = await manager.get_client(demo_workspace / "demo_myst.md") + assert result is client + + +@pytest.mark.asyncio +async def test_get_client_with_many_uris( + server_manager: ServerManager, demo_workspace: Uri, tmp_path: pathlib.Path +): + """Ensure that when called in rapid succession, with many uris we only create a + single client instance.""" + + server, manager = server_manager( + dict( + esbonio=dict( + sphinx=dict( + pythonCommand=[sys.executable], + buildCommand=["sphinx-build", "-M", "dirhtml", ".", str(tmp_path)], + ), + ), + ), + ) + + # Ensure that the server is ready + await server.ready + + src_uris = [Uri.for_file(f) for f in pathlib.Path(demo_workspace).glob("**/*.rst")] + coros = [manager.get_client(s) for s in src_uris] + + # As with the other cases, this should only "prime" the system + result = await asyncio.gather(*coros) + assert all([r is None for r in result]) + + # There should only have been one client created (in addition to the 'dummy' global + # scoped client) + assert len(manager.clients) == 2 + + client = manager.clients[str(demo_workspace)] + assert client is not None + assert client.state is None + + # Now if we do the same again we should get the same client instance for each case. + coros = [manager.get_client(s) for s in src_uris] + result = await asyncio.gather(*coros) + + assert all([r is client for r in result]) + + client = result[0] + assert client.state == ClientState.Running + + # And that the client initialized correctly + assert client.builder == "dirhtml" + assert client.src_uri == demo_workspace + assert client.conf_uri == demo_workspace + assert client.build_uri == Uri.for_file(tmp_path / "dirhtml") + + +@pytest.mark.asyncio +async def test_get_client_with_many_uris_in_many_projects( + server_manager: ServerManager, + demo_workspace: Uri, + docs_workspace: Uri, + tmp_path: pathlib.Path, +): + """Ensure that when called in rapid succession, with many uris we only create a + single client instance for each project.""" + + server, manager = server_manager( + dict( + esbonio=dict( + sphinx=dict(pythonCommand=[sys.executable]), + buildCommand=["sphinx-build", "-M", "dirhtml", ".", str(tmp_path)], + ), + ), + ) # Ensure that the server is ready + await server.ready + + src_uris = [Uri.for_file(f) for f in pathlib.Path(demo_workspace).glob("**/*.rst")] + src_uris += [Uri.for_file(f) for f in pathlib.Path(docs_workspace).glob("**/*.rst")] + coros = [manager.get_client(s) for s in src_uris] + + # As with the other cases, this should only "prime" the system + result = await asyncio.gather(*coros) + assert all([r is None for r in result]) + + # There should only have been one client created for each project (in addition to + # the 'dummy' global scoped client) + assert len(manager.clients) == 3 + + demo_client = manager.clients[str(demo_workspace)] + assert demo_client is not None + assert demo_client.state is None + + docs_client = manager.clients[str(docs_workspace)] + assert docs_client is not None + assert docs_client.state is None + + # Now if we do the same again we should get the same client instance for each case. + coros = [manager.get_client(s) for s in src_uris] + result = await asyncio.gather(*coros) + + assert all([(r is demo_client) or (r is docs_client) for r in result]) + + assert demo_client.state == ClientState.Running + + # When run in CI, the docs won't have all the required dependencies available. + assert docs_client.state in {ClientState.Running, ClientState.Errored} diff --git a/lib/esbonio/tests/server/features/test_sphinx_manager.py b/lib/esbonio/tests/server/features/test_sphinx_manager.py deleted file mode 100644 index 6067c0633..000000000 --- a/lib/esbonio/tests/server/features/test_sphinx_manager.py +++ /dev/null @@ -1,97 +0,0 @@ -import asyncio -from unittest.mock import AsyncMock -from unittest.mock import Mock - -import pytest -from lsprotocol import types as lsp -from pygls.exceptions import JsonRpcInternalError - -from esbonio.server import EsbonioLanguageServer -from esbonio.server import EsbonioWorkspace -from esbonio.server import Uri -from esbonio.server.features.sphinx_manager import MockSphinxClient -from esbonio.server.features.sphinx_manager import SphinxConfig -from esbonio.server.features.sphinx_manager import SphinxManager -from esbonio.server.features.sphinx_manager import mock_sphinx_client_factory -from esbonio.sphinx_agent import types - - -@pytest.fixture() -def workspace(uri_for): - return uri_for("workspaces", "demo") - - -@pytest.fixture() -def progress(): - p = Mock() - p.create_async = AsyncMock() - return p - - -@pytest.fixture() -def server(event_loop, progress, workspace: Uri): - """A mock instance of the language""" - ready = asyncio.Future() - ready.set_result(True) - - esbonio = EsbonioLanguageServer(loop=event_loop) - esbonio._ready = ready - esbonio.lsp.progress = progress - esbonio.show_message = Mock() - esbonio.configuration.get = Mock(return_value=SphinxConfig()) - esbonio.lsp._workspace = EsbonioWorkspace( - None, - workspace_folders=[lsp.WorkspaceFolder(str(workspace), "workspace")], - ) - return esbonio - - -@pytest.fixture() -def sphinx_info(workspace: Uri): - return types.SphinxInfo( - id="123", - version="6.0", - conf_dir=workspace.fs_path, - build_dir=(workspace / "_build" / "html").fs_path, - builder_name="html", - src_dir=workspace.fs_path, - dbpath=(workspace / "_build" / "html" / "esbonio.db").fs_path, - ) - - -@pytest.mark.asyncio -async def test_create_application_error(server, workspace: Uri): - """Ensure that we can handle errors during application creation correctly.""" - - client = MockSphinxClient(JsonRpcInternalError("create sphinx application failed.")) - client_factory = mock_sphinx_client_factory(client) - - manager = SphinxManager(client_factory, server) - - result = await manager.get_client(workspace / "index.rst") - assert result is None - - server.show_message.assert_called_with( - "Unable to create sphinx application: create sphinx application failed.", - lsp.MessageType.Error, - ) - - -@pytest.mark.asyncio -async def test_trigger_build_error(sphinx_info, server, workspace): - """Ensure that we can handle build errors correctly.""" - - client = MockSphinxClient( - sphinx_info, - build_result=JsonRpcInternalError("sphinx-build failed:"), - ) - client_factory = mock_sphinx_client_factory() - - manager = SphinxManager(client_factory, server) - manager.clients = {workspace: client} - - await manager.trigger_build(workspace / "index.rst") - - server.show_message.assert_called_with( - "sphinx-build failed:", lsp.MessageType.Error - ) From 5b1a898c77aa867f4309752801bece9791accf96 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 25 Feb 2024 20:08:15 +0000 Subject: [PATCH 5/6] lsp: Delete `MockSphinxClient` The tests now use the real thing and there does not seem to be much benefit in trying to maintain this. --- .../features/sphinx_manager/__init__.py | 4 - .../features/sphinx_manager/client_mock.py | 100 ------------------ 2 files changed, 104 deletions(-) delete mode 100644 lib/esbonio/esbonio/server/features/sphinx_manager/client_mock.py diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py b/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py index f80730418..f49e300cc 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/__init__.py @@ -4,8 +4,6 @@ from .client import ClientState from .client import SphinxClient -from .client_mock import MockSphinxClient -from .client_mock import mock_sphinx_client_factory from .client_subprocess import make_subprocess_sphinx_client from .config import SphinxConfig from .manager import SphinxManager @@ -15,8 +13,6 @@ "SphinxClient", "SphinxConfig", "SphinxManager", - "MockSphinxClient", - "mock_sphinx_client_factory", ] diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/client_mock.py b/lib/esbonio/esbonio/server/features/sphinx_manager/client_mock.py deleted file mode 100644 index 1836fb207..000000000 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/client_mock.py +++ /dev/null @@ -1,100 +0,0 @@ -from __future__ import annotations - -import asyncio -import typing -from typing import Dict -from typing import List -from typing import Optional -from typing import Union - -from esbonio.sphinx_agent import types - -if typing.TYPE_CHECKING: - from esbonio.server import Uri - - from .client import SphinxClient - from .manager import SphinxManager - - -class MockSphinxClient: - """A mock SphinxClient implementation, used for testing.""" - - def __init__( - self, - create_result: Union[types.SphinxInfo, Exception], - build_result: Optional[Union[types.BuildResult, Exception]] = None, - build_file_map: Optional[Dict[Uri, str]] = None, - startup_delay: float = 0.5, - ): - """Create a new instance. - - Parameters - ---------- - create_result - The result to return when calling ``create_application``. - If an exception is given it will be raised. - - build_file_map - The build file map to use. - - build_result - The result to return when calling ``build``. - If an exception is given it will be raised. - - startup_delay - The duration to sleep for when calling ``start`` - """ - self._create_result = create_result - self._startup_delay = startup_delay - self._build_result = build_result or types.BuildResult() - self._build_file_map = build_file_map or {} - self.building = False - - @property - def id(self) -> Optional[str]: - """The id of the Sphinx instance.""" - if isinstance(self._create_result, Exception): - return None - - return self._create_result.id - - async def start(self, *args, **kwargs): - await asyncio.sleep(self._startup_delay) - - async def stop(self, *args, **kwargs): - pass - - async def create_application(self, *args, **kwargs) -> types.SphinxInfo: - """Create an application.""" - - if isinstance(self._create_result, Exception): - raise self._create_result - - return self._create_result - - async def build(self, *args, **kwargs) -> types.BuildResult: - """Trigger a build""" - - if isinstance(self._build_result, Exception): - raise self._build_result - - return self._build_result - - async def get_src_uris(self) -> List[Uri]: - """Return all known source files.""" - return [s for s in self._build_file_map.keys()] - - async def get_build_path(self, src_uri: Uri) -> Optional[str]: - """Get the build path associated with the given ``src_uri``.""" - return self._build_file_map.get(src_uri) - - -def mock_sphinx_client_factory(client: Optional[SphinxClient] = None): - """Return a factory function that can be used with a ``SphinxManager`` instance.""" - - def factory(manager: SphinxManager): - if client is None: - raise RuntimeError("Unexpected client creation") - return client - - return factory From d2a425664218cd9953748a93bab558c4225398a1 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Mon, 26 Feb 2024 20:28:10 +0000 Subject: [PATCH 6/6] lsp: Realign test suite A number of tests have been marked as xfail or skipped as they will be fixed for real in the next batch of changes --- lib/esbonio/pyproject.toml | 4 - lib/esbonio/tests/e2e/conftest.py | 2 +- lib/esbonio/tests/e2e/test_e2e_directives.py | 2 + lib/esbonio/tests/e2e/test_e2e_symbols.py | 2 + lib/esbonio/tests/sphinx-agent/conftest.py | 32 ++++---- .../sphinx-agent/handlers/test_database.py | 1 + .../tests/sphinx-agent/test_sa_build.py | 30 ++++---- .../tests/sphinx-agent/test_sa_create_app.py | 76 +++++++++---------- 8 files changed, 72 insertions(+), 77 deletions(-) diff --git a/lib/esbonio/pyproject.toml b/lib/esbonio/pyproject.toml index 0e7562d09..1d359f4a5 100644 --- a/lib/esbonio/pyproject.toml +++ b/lib/esbonio/pyproject.toml @@ -67,10 +67,6 @@ profile = "black" [tool.pytest.ini_options] addopts = "--doctest-glob='*.txt'" asyncio_mode = "auto" -filterwarnings = [ - "ignore:'contextfunction' is renamed to 'pass_context',*:DeprecationWarning", - "ignore:'environmentfilter' is renamed to 'pass_environment',*:DeprecationWarning", -] [tool.mypy] mypy_path = "$MYPY_CONFIG_FILE_DIR" diff --git a/lib/esbonio/tests/e2e/conftest.py b/lib/esbonio/tests/e2e/conftest.py index b6681ac0d..e2f3b6a1a 100644 --- a/lib/esbonio/tests/e2e/conftest.py +++ b/lib/esbonio/tests/e2e/conftest.py @@ -67,7 +67,7 @@ async def client(lsp_client: LanguageClient, uri_for, tmp_path_factory): ) ) - await lsp_client.wait_for_notification("sphinx/appCreated") + await lsp_client.wait_for_notification("sphinx/clientCreated") # Save the document to trigger a build lsp_client.text_document_did_save( diff --git a/lib/esbonio/tests/e2e/test_e2e_directives.py b/lib/esbonio/tests/e2e/test_e2e_directives.py index 5229eab40..1fc3fecf3 100644 --- a/lib/esbonio/tests/e2e/test_e2e_directives.py +++ b/lib/esbonio/tests/e2e/test_e2e_directives.py @@ -56,6 +56,7 @@ ], ) @pytest.mark.asyncio(scope="session") +@pytest.mark.xfail async def test_rst_directive_completions( client: LanguageClient, uri_for, @@ -151,6 +152,7 @@ async def test_rst_directive_completions( ], ) @pytest.mark.asyncio(scope="session") +@pytest.mark.xfail async def test_myst_directive_completions( client: LanguageClient, uri_for, diff --git a/lib/esbonio/tests/e2e/test_e2e_symbols.py b/lib/esbonio/tests/e2e/test_e2e_symbols.py index 191e98a6f..feafbdc4b 100644 --- a/lib/esbonio/tests/e2e/test_e2e_symbols.py +++ b/lib/esbonio/tests/e2e/test_e2e_symbols.py @@ -96,6 +96,7 @@ def document_symbol( ], ) @pytest.mark.asyncio(scope="session") +@pytest.mark.xfail async def test_document_symbols( client: LanguageClient, uri_for, @@ -250,6 +251,7 @@ async def test_document_symbols( ], ) @pytest.mark.asyncio(scope="session") +@pytest.mark.xfail async def test_workspace_symbols( client: LanguageClient, query: str, diff --git a/lib/esbonio/tests/sphinx-agent/conftest.py b/lib/esbonio/tests/sphinx-agent/conftest.py index 9a65a990f..672b61cbf 100644 --- a/lib/esbonio/tests/sphinx-agent/conftest.py +++ b/lib/esbonio/tests/sphinx-agent/conftest.py @@ -1,26 +1,21 @@ +import logging import sys -import pytest_lsp +import pytest_asyncio from lsprotocol.types import WorkspaceFolder from pygls.workspace import Workspace -from pytest_lsp import ClientServerConfig -from esbonio.server.features.sphinx_manager.client_subprocess import ( - SubprocessSphinxClient, -) +from esbonio.server.features.sphinx_manager.client import ClientState from esbonio.server.features.sphinx_manager.client_subprocess import ( make_test_sphinx_client, ) from esbonio.server.features.sphinx_manager.config import SphinxConfig +logger = logging.getLogger(__name__) -@pytest_lsp.fixture( - config=ClientServerConfig( - server_command=[sys.executable, "-m", "esbonio.sphinx_agent"], - client_factory=make_test_sphinx_client, - ), -) -async def client(sphinx_client: SubprocessSphinxClient, uri_for, tmp_path_factory): + +@pytest_asyncio.fixture +async def client(uri_for, tmp_path_factory): build_dir = tmp_path_factory.mktemp("build") demo_workspace = uri_for("workspaces", "demo") test_uri = demo_workspace / "index.rst" @@ -28,10 +23,11 @@ async def client(sphinx_client: SubprocessSphinxClient, uri_for, tmp_path_factor workspace = Workspace( None, workspace_folders=[ - WorkspaceFolder(uri=str(demo_workspace), name="sphinx-default"), + WorkspaceFolder(uri=str(demo_workspace), name="demo"), ], ) config = SphinxConfig( + python_command=[sys.executable], build_command=[ "sphinx-build", "-M", @@ -40,11 +36,13 @@ async def client(sphinx_client: SubprocessSphinxClient, uri_for, tmp_path_factor str(build_dir), ], ) - resolved = config.resolve(test_uri, workspace, sphinx_client.logger) + resolved = config.resolve(test_uri, workspace, logger) assert resolved is not None - info = await sphinx_client.create_application(resolved) - assert info is not None + sphinx_client = await make_test_sphinx_client(resolved) + assert sphinx_client.state == ClientState.Running await sphinx_client.build() - yield + yield sphinx_client + + await sphinx_client.stop() diff --git a/lib/esbonio/tests/sphinx-agent/handlers/test_database.py b/lib/esbonio/tests/sphinx-agent/handlers/test_database.py index cbb1ddc37..db87329a7 100644 --- a/lib/esbonio/tests/sphinx-agent/handlers/test_database.py +++ b/lib/esbonio/tests/sphinx-agent/handlers/test_database.py @@ -14,6 +14,7 @@ def anuri(base, *args): @pytest.mark.asyncio +@pytest.mark.xfail async def test_files_table(client: SubprocessSphinxClient): """Ensure that we can correctly index all the files in the Sphinx project.""" diff --git a/lib/esbonio/tests/sphinx-agent/test_sa_build.py b/lib/esbonio/tests/sphinx-agent/test_sa_build.py index 35e686953..97003137b 100644 --- a/lib/esbonio/tests/sphinx-agent/test_sa_build.py +++ b/lib/esbonio/tests/sphinx-agent/test_sa_build.py @@ -1,14 +1,15 @@ +import logging import pathlib import re import sys import pytest -import pytest_lsp +import pytest_asyncio from lsprotocol.types import WorkspaceFolder from pygls.exceptions import JsonRpcInternalError from pygls.workspace import Workspace -from pytest_lsp import ClientServerConfig +from esbonio.server.features.sphinx_manager.client import ClientState from esbonio.server.features.sphinx_manager.client_subprocess import ( SubprocessSphinxClient, ) @@ -17,6 +18,8 @@ ) from esbonio.server.features.sphinx_manager.config import SphinxConfig +logger = logging.getLogger(__name__) + @pytest.mark.asyncio async def test_build_includes_webview_js(client: SubprocessSphinxClient, uri_for): @@ -65,16 +68,8 @@ async def test_build_content_override(client: SubprocessSphinxClient, uri_for): assert expected in index_html.read_text() -@pytest_lsp.fixture( - scope="module", - config=ClientServerConfig( - server_command=[sys.executable, "-m", "esbonio.sphinx_agent"], - client_factory=make_test_sphinx_client, - ), -) -async def client_build_error( - sphinx_client: SubprocessSphinxClient, uri_for, tmp_path_factory -): +@pytest_asyncio.fixture(scope="module") +async def client_build_error(uri_for, tmp_path_factory): """A sphinx client that will error when a build is triggered.""" build_dir = tmp_path_factory.mktemp("build") demo_workspace = uri_for("workspaces", "demo") @@ -89,6 +84,7 @@ async def client_build_error( conf_dir = uri_for("workspaces", "demo-error-build").fs_path config = SphinxConfig( + python_command=[sys.executable], build_command=[ "sphinx-build", "-b", @@ -99,13 +95,15 @@ async def client_build_error( str(build_dir), ], ) - resolved = config.resolve(test_uri, workspace, sphinx_client.logger) + resolved = config.resolve(test_uri, workspace, logger) assert resolved is not None - info = await sphinx_client.create_application(resolved) - assert info is not None + sphinx_client = await make_test_sphinx_client(resolved) + assert sphinx_client.state == ClientState.Running + + yield sphinx_client - yield + await sphinx_client.stop() @pytest.mark.asyncio(scope="module") diff --git a/lib/esbonio/tests/sphinx-agent/test_sa_create_app.py b/lib/esbonio/tests/sphinx-agent/test_sa_create_app.py index ca235bfd7..20015a54d 100644 --- a/lib/esbonio/tests/sphinx-agent/test_sa_create_app.py +++ b/lib/esbonio/tests/sphinx-agent/test_sa_create_app.py @@ -1,13 +1,12 @@ +import logging import sys import pytest -import pytest_lsp from lsprotocol.types import WorkspaceFolder from pygls import IS_WIN -from pygls.exceptions import JsonRpcInternalError from pygls.workspace import Workspace -from pytest_lsp import ClientServerConfig +from esbonio.server.features.sphinx_manager.client import ClientState from esbonio.server.features.sphinx_manager.client_subprocess import ( SubprocessSphinxClient, ) @@ -16,19 +15,11 @@ ) from esbonio.server.features.sphinx_manager.config import SphinxConfig - -@pytest_lsp.fixture( - config=ClientServerConfig( - server_command=[sys.executable, "-m", "esbonio.sphinx_agent"], - client_factory=make_test_sphinx_client, - ) -) -async def client(sphinx_client: SubprocessSphinxClient): - yield +logger = logging.getLogger("__name__") @pytest.mark.asyncio -async def test_create_application(client: SubprocessSphinxClient, uri_for): +async def test_create_application(uri_for): """Ensure that we can create a Sphinx application instance correctly.""" demo_workspace = uri_for("workspaces", "demo") @@ -40,29 +31,35 @@ async def test_create_application(client: SubprocessSphinxClient, uri_for): WorkspaceFolder(uri=str(demo_workspace), name="demo"), ], ) - config = SphinxConfig() - resolved = config.resolve(test_uri, workspace, client.logger) + config = SphinxConfig(python_command=[sys.executable]) + resolved = config.resolve(test_uri, workspace, logger) assert resolved is not None - - info = await client.create_application(resolved) - assert info is not None - assert info.builder_name == "dirhtml" - - # Paths are case insensitive on Windows - if IS_WIN: - assert info.src_dir.lower() == demo_workspace.fs_path.lower() - assert info.conf_dir.lower() == demo_workspace.fs_path.lower() - assert "cache" in info.build_dir.lower() - else: - assert info.src_dir == demo_workspace.fs_path - assert info.conf_dir == demo_workspace.fs_path - assert "cache" in info.build_dir + client = None + + try: + client = await make_test_sphinx_client(resolved) + assert client.state == ClientState.Running + + info = client.sphinx_info + assert info is not None + assert info.builder_name == "dirhtml" + + # Paths are case insensitive on Windows + if IS_WIN: + assert info.src_dir.lower() == demo_workspace.fs_path.lower() + assert info.conf_dir.lower() == demo_workspace.fs_path.lower() + assert "cache" in info.build_dir.lower() + else: + assert info.src_dir == demo_workspace.fs_path + assert info.conf_dir == demo_workspace.fs_path + assert "cache" in info.build_dir + finally: + if client: + await client.stop() @pytest.mark.asyncio -async def test_create_application_error( - client: SubprocessSphinxClient, uri_for, tmp_path_factory -): +async def test_create_application_error(uri_for, tmp_path_factory): """Ensure that we can handle errors during application creation.""" build_dir = tmp_path_factory.mktemp("build") @@ -78,6 +75,7 @@ async def test_create_application_error( conf_dir = uri_for("workspaces", "demo-error").fs_path config = SphinxConfig( + python_command=[sys.executable], build_command=[ "sphinx-build", "-b", @@ -86,13 +84,13 @@ async def test_create_application_error( conf_dir, demo_workspace.fs_path, str(build_dir), - ] + ], ) - resolved = config.resolve(test_uri, workspace, client.logger) + resolved = config.resolve(test_uri, workspace, logger) assert resolved is not None - with pytest.raises( - JsonRpcInternalError, - match="There is a programmable error in your configuration file:", - ): - await client.create_application(resolved) + client = await SubprocessSphinxClient(resolved) + assert client.state == ClientState.Errored + + message = "There is a programmable error in your configuration file:" + assert message in str(client.exception)